OpsDeck/core

Watch

Fork 0

refactor: the structural dedup deferred from #12 #24

Closed

thisilike wants to merge 0 commits from refactor/dedup-shared-code into fix/issue-12-followups

thisilike commented

2026-08-11 08:11:45 +02:00

Owner

The four duplication items from #12 that #19 deliberately left alone. Targets fix/issue-12-followups so the diff is only the refactor — retarget to main once #19 merges.

No behaviour change is intended anywhere except the one called out below. deno task ci green: fmt, lint, typecheck, 343 tests (up from 328; every new one covers code that had no direct test before).

1. One compose-progress shape and one carry-forward rule

ComposeProgress, ComposeState, PullPhase and PullSummary were declared twice, and the fold that carries totals/current/fraction/phase across a terminal event existed as merge() in backend/jobs.ts and again inside the frontend reducer's push. Both now in packages/modules/docker/shared/progress.ts, which imports nothing — the frontend reaches it by relative path, and the built bundle inlines it (checked, not assumed).

Not cosmetic: sync() re-hydrates from the server every 15 s while a job runs, so live rows and the re-attach snapshot are shown alternately in the same table. When the two folds disagree, a completed layer visibly flips between readings. They had already drifted once.

2. One registry v2 client

The docker module and tools/dep-check had each grown their own reference parsing, bearer handshake and Link pagination. packages/registry owns what the protocol decides; each caller keeps what genuinely differs (timeouts, page caps, caching, result shapes).

Falls out of having one copy: the origin guard now covers dep-check's forge pagination too, not just the registry path.

Verified past the unit tests by running the tool for real — dep-check --dry-run reaches Docker Hub and two forges through the shared client, paginates, and reports the same four available updates as before.

3. One version-tag parser — but two orderings, on purpose

The parse and the prerelease word list are now shared. The ordering is not, and that is the interesting part: compare() pads a missing component with 0 in dep-check and -1 in the module. dep-check treats v6 and v6.0.0 as one release, because a project shipping v6 then v6.0.1 has published something the reader wants; the module sorts 1.2 below 1.2.0, because a pin's granularity is a choice the operator made. Merging them would have quietly broken one. Both files already carried comments warning whoever tried — so the shared module carries it too.

The one intended behaviour change: the module's parser accepted only v as a prefix, so bin-2.5.6 did not parse and a service pinned to that line could never be offered an upgrade. The shared rule accepts any prefix ending in - or _, as dep-check's did. The guard that matters is unchanged — an arbitrary alphabetic prefix is still refused, which is what keeps sha256-9f3a… from being read as version 256 of "sha". Both halves are tested.

4. One pin pipeline — and a test for it at last

The two pin routes each walked a stack's compose files applying one service's new image, and the copies had diverged in ways that cost a review pass each: the ALREADY branch, the no-break-on-first-hit rule, which text a hunk is measured against. mod.ts loses 147 lines and gains 50.

applyPin is the stack route's version, which is the general one — a file may be visited more than once across services, so a hunk is measured against the text as it was before any of this request's edits. With a single service that reduces exactly to what the container route did. Two behaviours the container route inherits are strictly safer and unreachable in its single-service case: a file we already wrote that then fails to re-read throws rather than being skipped, and hunks come from the restore ledger's original.

The point of this one is the test file. This logic sat behind an admin guard, a job slot and a compose validation, so none of it was ever exercised directly — which is exactly how two copies drifted unnoticed for as long as they did. It now takes its filesystem and restore ledger as arguments and runs against a real directory: nine cases, one per rule that had to be learned the hard way.

What I'd look at hardest

Item 4 touches the code that writes operator compose files, and a pure refactor has no failing-test-before/after to prove it. The nine new cases are the best evidence I can offer; the routes themselves still have no test above applyPin. Worth a run through docker/dev-rig before this merges.

The four duplication items from #12 that #19 deliberately left alone. Targets `fix/issue-12-followups` so the diff is only the refactor — retarget to `main` once #19 merges. No behaviour change is intended anywhere except the one called out below. `deno task ci` green: fmt, lint, typecheck, **343 tests** (up from 328; every new one covers code that had no direct test before). ### 1. One compose-progress shape and one carry-forward rule `ComposeProgress`, `ComposeState`, `PullPhase` and `PullSummary` were declared twice, and the fold that carries totals/current/fraction/phase across a terminal event existed as `merge()` in `backend/jobs.ts` and again inside the frontend reducer's `push`. Both now in `packages/modules/docker/shared/progress.ts`, which imports nothing — the frontend reaches it by relative path, and the built bundle inlines it (checked, not assumed). Not cosmetic: `sync()` re-hydrates from the server every 15 s while a job runs, so live rows and the re-attach snapshot are shown alternately in the same table. When the two folds disagree, a completed layer visibly flips between readings. They had already drifted once. ### 2. One registry v2 client The docker module and `tools/dep-check` had each grown their own reference parsing, bearer handshake and Link pagination. `packages/registry` owns what the protocol decides; each caller keeps what genuinely differs (timeouts, page caps, caching, result shapes). Falls out of having one copy: the origin guard now covers dep-check's **forge** pagination too, not just the registry path. Verified past the unit tests by running the tool for real — `dep-check --dry-run` reaches Docker Hub and two forges through the shared client, paginates, and reports the same four available updates as before. ### 3. One version-tag parser — but two orderings, on purpose The parse and the prerelease word list are now shared. The **ordering is not**, and that is the interesting part: `compare()` pads a missing component with `0` in dep-check and `-1` in the module. dep-check treats `v6` and `v6.0.0` as one release, because a project shipping `v6` then `v6.0.1` has published something the reader wants; the module sorts `1.2` below `1.2.0`, because a pin's granularity is a choice the operator made. Merging them would have quietly broken one. Both files already carried comments warning whoever tried — so the shared module carries it too. **The one intended behaviour change:** the module's parser accepted only `v` as a prefix, so `bin-2.5.6` did not parse and a service pinned to that line could never be offered an upgrade. The shared rule accepts any prefix ending in `-` or `_`, as dep-check's did. The guard that matters is unchanged — an arbitrary alphabetic prefix is still refused, which is what keeps `sha256-9f3a…` from being read as version 256 of "sha". Both halves are tested. ### 4. One pin pipeline — and a test for it at last The two pin routes each walked a stack's compose files applying one service's new image, and the copies had diverged in ways that cost a review pass each: the ALREADY branch, the no-break-on-first-hit rule, which text a hunk is measured against. `mod.ts` loses **147 lines and gains 50**. `applyPin` is the stack route's version, which is the general one — a file may be visited more than once across services, so a hunk is measured against the text as it was before *any* of this request's edits. With a single service that reduces exactly to what the container route did. Two behaviours the container route inherits are strictly safer and unreachable in its single-service case: a file we already wrote that then fails to re-read throws rather than being skipped, and hunks come from the restore ledger's original. **The point of this one is the test file.** This logic sat behind an admin guard, a job slot and a compose validation, so none of it was ever exercised directly — which is exactly how two copies drifted unnoticed for as long as they did. It now takes its filesystem and restore ledger as arguments and runs against a real directory: nine cases, one per rule that had to be learned the hard way. ### What I'd look at hardest Item 4 touches the code that writes operator compose files, and a pure refactor has no failing-test-before/after to prove it. The nine new cases are the best evidence I can offer; the routes themselves still have no test above `applyPin`. Worth a run through `docker/dev-rig` before this merges.

thisilike added 4 commits

2026-08-11 08:11:45 +02:00

refactor(docker): one compose-progress shape, one carry-forward rule bbbdd672bc

ComposeProgress, ComposeState, PullPhase and PullSummary were declared twice —
once in backend/compose_progress.ts, once in frontend/update_feed.svelte.ts —
and the fold that carries totals, current, fraction and phase across a terminal
event existed as merge() in backend/jobs.ts and again inside the frontend
reducer's push.

Both now live in packages/modules/docker/shared/progress.ts, which imports
nothing: the module contract forbids bare imports in the frontend build, and
the frontend reaches this by relative path. Verified in the built bundle — the
code is inlined, not left as a dangling import.

This pairing is not cosmetic. sync() re-hydrates the feed from the server every
15 seconds while a job runs, so the live rows and the re-attach snapshot are
displayed alternately in the same table; when the two folds disagree, a
completed layer visibly flips between readings. They had already drifted once,
which is how the phase carry-forward came to be patched in both places in
lockstep.

One behaviour difference, deliberately taken: the frontend used to write
`{ ...p, phase, total, current, fraction }`, setting those keys even when
undefined, where the shared fold omits them. Every read site tests
`!== undefined`, so nothing changes — but the objects now match the wire shape
the server actually sends, which is what hydrate() puts in the same map.

refactor: one registry v2 client instead of two 1db3355b6d

The docker module and tools/dep-check had each grown their own implementation
of the same protocol: reference parsing, the anonymous bearer handshake, and
Link pagination. They asked different questions of a registry, but the protocol
answers both the same way, and the copies drifted in ways that only surfaced
one at a time — the dep-check copy was the one that followed a Link header to
another host with the token attached, and the one that never handled unquoted
`rel=next`, so it answered "up to date" from page one.

packages/registry now owns what the protocol decides: parseImageRef,
anonymousToken, isNextRel and nextPage. Each caller keeps what genuinely
differs — timeouts (the module holds a job slot while it waits, the tool runs
in CI), page caps, caching, result shapes.

Two things fall out of having one copy:

- The origin guard now covers dep-check's forge pagination too, not just the
  registry path. Same reasoning, same code.
- parseImageRef declines a digest-pinned reference, which dep-check's parser
  could not express. discover.ts already strips the tag and the digest before
  calling, so this is a guard rather than a change; the old parseImageName's
  doc comment claimed it stripped tags itself, which it never did.

Verified beyond the unit tests by running the tool for real:
`dep-check --dry-run` reaches Docker Hub and two forges through the shared
client, paginates, and reports the same four available updates as before.

refactor: one version-tag parser, and keep the two orderings apart 1943d1115f

The docker module and dep-check each parsed release tags into prefix, numbers,
flavour and prerelease, with their own copy of the word list — the copy that
had already drifted by a word in each direction.

packages/registry/version.ts now owns the parse and the list.

What is NOT shared is the ordering, and that is the point of the exercise:
compare() pads a missing component with 0 in dep-check and with -1 in the
module, and both are right for their own question. dep-check treats `v6` and
`v6.0.0` as one release, because a project shipping `v6` then `v6.0.1` has
published something the reader wants to hear about; the module sorts `1.2`
below `1.2.0`, because a pin's granularity is a choice the operator made.
Merging them would have quietly broken one. Both files already said so, in
comments written for whoever tried this — so the shared module says it too.

One behaviour change, deliberate. The module's parser accepted only "v" as a
prefix, so `bin-2.5.6` did not parse and a service pinned to that line could
never be offered an upgrade; the shared rule accepts any prefix ending in - or
_ , as dep-check's did. The guard that matters is unchanged: an arbitrary
alphabetic prefix is still refused, which is what keeps `sha256-9f3a…` from
being read as version 256 of "sha". Both halves are tested.

`suffix` is now `flavour` in dep-check, which is what the module already called
it and the better name for what it holds.

refactor(docker): one pin pipeline, and a test for it at last

Build and Deploy / verify (pull_request) Successful in 49s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m11s

Details

1cb1f11674

The two pin routes each walked a stack's compose files applying one service's
new image, and the copies had diverged in ways that cost a review pass each:
the ALREADY branch, the no-break-on-first-hit rule, and which text a hunk is
measured against. The single-service copy was the simpler one, and simpler here
meant missing what the other had already learned.

applyPin is the stack route's version, which is the general one: a file may be
visited more than once across services, so a hunk is measured against the text
as it was before ANY of this request's edits — what git compares the index to.
With a single service that reduces exactly to what the container route did.
mod.ts loses 147 lines and gains 50.

Two behaviours the container route inherits, both strictly safer and neither
reachable in its single-service case: a file we have already written that then
fails to re-read throws rather than being skipped, and hunks come from the
restore ledger's original rather than the text just read.

The point of the extraction is the test file. This logic was reachable only
through a route behind an admin guard, a job slot and a compose validation, so
none of it was ever exercised directly — which is precisely how two copies
drifted unnoticed for as long as they did. It now takes its filesystem and its
restore ledger as arguments and runs against a real directory: nine cases, one
per rule that had to be learned the hard way, including base+override, ALREADY
not ending the walk, two services in one file diffing against the pre-edit
text, and the restore ledger keeping the pre-edit original.

thisilike requested review from julian

2026-08-11 08:11:55 +02:00

thisilike added 1 commit

2026-08-11 08:24:08 +02:00

fix(docker): do not warn about pagination that simply ended

Build and Deploy / verify (pull_request) Successful in 51s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m10s

Details

17d7a6b5cc

Reviewing the dedup: listTags warned "left the registry's origin" whenever a
Link header produced no next page. A last page legitimately carries a Link
header with rel="prev" or rel="last" and no next at all, so that fired on the
final page of every paginated repository — a scary message about a credential
boundary, logged for the most ordinary event there is.

The warning belongs to a next that was OFFERED and declined, which is what
isNextRel now decides. Verified by reverting the condition and watching the new
test report the false alarm on the quiet case.

Also documents the two things the dedup introduced and neither doc mentioned:
packages/registry in CLAUDE.md, including the ordering it deliberately does not
own, and the `shared/` directory convention in docs/modules.md, since a module
author reaching for it needs to know the file must import nothing.

thisilike commented

2026-08-11 08:24:28 +02:00

Author

Owner

Self-review of the four refactor commits before asking anyone else to read them. One real defect, one documentation gap, both fixed in 17d7a6b. 344 tests.

The defect: a false alarm on the last page of every paginated repository

listTags warned "tag pagination left the registry's origin; stopping" whenever a Link header produced no next page. But a last page legitimately carries rel="prev" or rel="last" and no next — so the most ordinary event in pagination logged a message about a credential boundary being crossed.

Mine, introduced by the dedup: collapsing the old explicit next.startsWith("/") branch into one call lost the distinction between "no next was offered" and "a next was offered and refused". The warning belongs only to the second, which isNextRel now decides. Test covers all three shapes — quiet last page, no header, genuinely off-origin — and reverting the condition makes it report the false alarm.

The gap: two new conventions, neither documented

packages/registry in CLAUDE.md, including the ordering it deliberately does not own — that is the part a future reader is most likely to "tidy up".
The shared/ directory in docs/modules.md, since a module author reaching for it needs to know the file must import nothing, or the frontend build rejects it.

Checked and clean

deno install --frozen against the new workspace member — CI would have failed on a stale lockfile, and does not.
Every frontend read of total/current/fraction/phase uses !== undefined, ?? 0 or truthiness, so mergeProgress omitting undefined keys where the old inline fold set them is genuinely inert. No in/hasOwnProperty/Object.keys anywhere near a progress object.
The pin extraction read line by line against the two originals: the already/done collapse, the changes push condition, the 404-vs-alreadyWritten split and the hunk base all behave as before. The container route's re-derivation of the hunk against restore.original is a no-op there, since it equals the text just read.
deno task build end to end; the registry and shared tests are picked up by the root task rather than only when named.

Two API message changes, deliberate and worth a second opinion

The container route's 404 went from "no compose file here declares an image for that service" to "no compose file declares an image for <service>", and its interpolation error now names the service too. Both come from using the stack route's wording for both callers. Naming the service is better, but they are response strings — say if you would rather they stayed byte-identical.

Still true from the PR body: the routes above applyPin have no test, and docker/dev-rig is the thing that would exercise them.

Self-review of the four refactor commits before asking anyone else to read them. One real defect, one documentation gap, both fixed in `17d7a6b`. 344 tests. ### The defect: a false alarm on the last page of every paginated repository `listTags` warned *"tag pagination left the registry's origin; stopping"* whenever a `Link` header produced no next page. But a last page legitimately carries `rel="prev"` or `rel="last"` and no next — so the most ordinary event in pagination logged a message about a credential boundary being crossed. Mine, introduced by the dedup: collapsing the old explicit `next.startsWith("/")` branch into one call lost the distinction between *"no next was offered"* and *"a next was offered and refused"*. The warning belongs only to the second, which `isNextRel` now decides. Test covers all three shapes — quiet last page, no header, genuinely off-origin — and reverting the condition makes it report the false alarm. ### The gap: two new conventions, neither documented - `packages/registry` in CLAUDE.md, **including the ordering it deliberately does not own** — that is the part a future reader is most likely to "tidy up". - The `shared/` directory in `docs/modules.md`, since a module author reaching for it needs to know the file must import nothing, or the frontend build rejects it. ### Checked and clean - `deno install --frozen` against the new workspace member — CI would have failed on a stale lockfile, and does not. - Every frontend read of `total`/`current`/`fraction`/`phase` uses `!== undefined`, `?? 0` or truthiness, so `mergeProgress` omitting undefined keys where the old inline fold set them is genuinely inert. No `in`/`hasOwnProperty`/`Object.keys` anywhere near a progress object. - The pin extraction read line by line against the two originals: the `already`/`done` collapse, the `changes` push condition, the 404-vs-alreadyWritten split and the hunk base all behave as before. The container route's re-derivation of the hunk against `restore.original` is a no-op there, since it equals the text just read. - `deno task build` end to end; the registry and shared tests are picked up by the root task rather than only when named. ### Two API message changes, deliberate and worth a second opinion The container route's 404 went from *"no compose file here declares an image for that service"* to *"no compose file declares an image for `<service>`"*, and its interpolation error now names the service too. Both come from using the stack route's wording for both callers. Naming the service is better, but they are response strings — say if you would rather they stayed byte-identical. Still true from the PR body: the routes above `applyPin` have no test, and `docker/dev-rig` is the thing that would exercise them.

thisilike commented

2026-08-11 08:37:20 +02:00

Author

Owner

Folded into #19 and closing unmerged — nothing is lost. fix/issue-12-followups fast-forwarded onto this branch, so all five commits (bbbdd67, 1db3355, 1943d11, 1cb1f11, 17d7a6b) are now in #19 at 17d7a6b, byte-identical.

Splitting it out was my call and the reasoning still holds — a behaviour-preserving refactor and a set of behaviour-changing fixes touching the same three files are easier to read apart than together. But it left a PR stacked on an unmerged branch needing a retarget later, and that shape has already cost this stack one bad merge. One PR against main is the safer trade.

Review #19 instead; it carries the whole thing.

Folded into #19 and closing unmerged — nothing is lost. `fix/issue-12-followups` fast-forwarded onto this branch, so all five commits (`bbbdd67`, `1db3355`, `1943d11`, `1cb1f11`, `17d7a6b`) are now in #19 at `17d7a6b`, byte-identical. Splitting it out was my call and the reasoning still holds — a behaviour-preserving refactor and a set of behaviour-changing fixes touching the same three files are easier to read apart than together. But it left a PR stacked on an unmerged branch needing a retarget later, and that shape has already cost this stack one bad merge. One PR against `main` is the safer trade. Review #19 instead; it carries the whole thing.

thisilike closed this pull request

2026-08-11 08:37:30 +02:00

~~thisilike referenced this pull request 2026-08-11 08:38:05 +02:00~~

docker: the deferred findings from #12, and the dedup behind them #19

thisilike referenced this pull request

2026-08-11 16:45:26 +02:00

docker: the deferred findings from #12, and the dedup behind them #19