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

Merged
thisilike merged 27 commits from fix/issue-12-followups into main 2026-08-11 16:51:53 +02:00
Owner

Closes #12. 25 commits on main, including a merge of the task-runs refactor (#21) and the adaptation onto it. deno task ci green: fmt, lint, typecheck, 427 tests. deno task build clean.

Two halves. A changes behaviour and every fix has a test that fails without it. B changes none and exists so the same bugs stop recurring.


A — the defects

  • F5 — a reference with no tag to move was mangled instead of refused. tagOf/retag split on the last colon, which in postgres@sha256:2a1f… belongs to sha256:retag built postgres@sha256:16.4 and the registry got blamed for a missing image nobody asked for. Covers the bare sha256:9f3a… an untagged image reports, which has no @ to catch it by. Both pin actions refuse by name before fetching or writing.

  • F6 — a missing docker-content-digest read as "no such image". Falls through to a GET of the same manifest; if that has no header either, the digest is computed from the bytes — exact, since a manifest's digest is the SHA-256 of what was served. 405/501 take the same path; a real 404 still reports absence without a second request.

  • A non-manifest 200 is refused, not hashed. Zero bytes hash to e3b0c442…b855: stable, equal to no image's digest, so that image reported "update available" forever. The same hole swallowed an HTML error page served with 200.

  • Four ways a partial tag list was reported as complete. nextPage returned string | null and null meant both "no next page" and "one that left the origin"; every caller read the first. Fixed at the type — NextPage is {next|declined|end}, so collapsing the branch is a compile error. Then the same claim, three more times: an unparseable base, an offered-but-unreadable next (including a comma inside the URI, which link.split(",") tears in two), and listTags exhausting its page cap. All refuse now. This matters because a partial list is how "no newer version" gets said about a repository that has one.

  • dep-check sent its bearer token wherever the Link header pointed. updates.ts had refused cross-host pagination for exactly this reason; the copy never had the guard. Both compare origin, not hostname — names alone accept http:// on the registry's own name and put the token on the wire in clear text.

  • rel read as a token list. rel="nextish" and rel=next-archive matched. Anchoring the end would have traded that for rel="next"; type="…" and rel="next last", both legal, both silently stopping at page one.

  • Chooser bands vs withinScope (1.2.3.4 offered a minor upgrade minor would never pick); checkUpdates tail starvation (images behind a slow registry skipped forever); rootOf called twice per file; ConfirmDialog focus grab; the prerelease word lists differing by one word each way.

B — the dedup

  1. One registry v2 clientpackages/registry: reference parsing, bearer handshake, Link pagination. The origin guard now covers dep-check's forge pagination too. Verified by running dep-check --dry-run against Docker Hub and two forges.
  2. One version-tag parser — two orderings, on purpose. compare() pads with 0 in dep-check and -1 in the module and both are right; merging them breaks one silently, so the shared module says so. Intended behaviour change: the module accepted only v as a prefix, so bin-2.5.6 and bookworm-20240110 never parsed and such a pin could not be offered an upgrade at all. sha256-9f3a… is still refused, and nothing applies an upgrade on its own.
  3. One progress fold for the server, the shell and the modules. #21 put mergeProgress in the SDK saying it exists so no client can disagree — then left an identical copy in server/src/tasks/service.ts and the docker frontend still folding by hand. Both collapsed onto the SDK's. Matters because sync() re-hydrates every 15 s, so both readings appear alternately in one table.
  4. One pin pipeline. applyPin, re-aimed at actions.ts after #21 moved the work there — the duplication survived that refactor intact. Refusals are returned as reasons and the action turns them into TaskFailure, so the helper states the fact and the action owns the presentation.

What review changed

Julian found four things worth the round trip: the nextPage return type (I took the type change he said he wasn't asking for, because it makes the bug a compile error), the untested F5 guard, the unreadable-next branch, and the page cap — where he changed my mind with my own measurement, since after this branch the two callers would have disagreed about what a truncated list means.

Twice his suggested fix would have broken something: anchoring the rel match drops rel="next last", and if (false) neutering doesn't compile. Both are in the comments with the evidence.

I also reviewed this branch myself twice and found five defects I had introduced — the focus latch burning on a null ref, both same-host guards comparing hostname rather than origin, an orphaned JSDoc, a false pagination warning on every last page, and the F5 guard silently orphaned by the merge. All fixed here rather than left for review.

Every guard is verified by reverting it and watching the relevant test fail.

Still open

The pin actions above applyPin have no test beyond the guard cases. docker/dev-rig is the honest answer; happy to run it before merge.

Split out: #20, the four Windows test failures, assigned to @julian.

Closes #12. 25 commits on `main`, including a merge of the task-runs refactor (#21) and the adaptation onto it. `deno task ci` green: fmt, lint, typecheck, **427 tests**. `deno task build` clean. Two halves. **A** changes behaviour and every fix has a test that fails without it. **B** changes none and exists so the same bugs stop recurring. --- ## A — the defects - **F5 — a reference with no tag to move was mangled instead of refused.** `tagOf`/`retag` split on the last colon, which in `postgres@sha256:2a1f…` belongs to `sha256:` — `retag` built `postgres@sha256:16.4` and the registry got blamed for a missing image nobody asked for. Covers the bare `sha256:9f3a…` an untagged image reports, which has no `@` to catch it by. Both pin actions refuse by name before fetching or writing. - **F6 — a missing `docker-content-digest` read as "no such image".** Falls through to a GET of the same manifest; if that has no header either, the digest is computed from the bytes — exact, since a manifest's digest *is* the SHA-256 of what was served. 405/501 take the same path; a real 404 still reports absence without a second request. - **A non-manifest 200 is refused, not hashed.** Zero bytes hash to `e3b0c442…b855`: stable, equal to no image's digest, so that image reported "update available" *forever*. The same hole swallowed an HTML error page served with 200. - **Four ways a partial tag list was reported as complete.** `nextPage` returned `string | null` and null meant both "no next page" and "one that left the origin"; every caller read the first. Fixed at the type — `NextPage` is `{next|declined|end}`, so collapsing the branch is a compile error. Then the same claim, three more times: an unparseable base, an offered-but-unreadable `next` (including a comma inside the URI, which `link.split(",")` tears in two), and `listTags` exhausting its page cap. All refuse now. This matters because a partial list is how "no newer version" gets said about a repository that has one. - **`dep-check` sent its bearer token wherever the `Link` header pointed.** `updates.ts` had refused cross-host pagination for exactly this reason; the copy never had the guard. Both compare **origin**, not hostname — names alone accept `http://` on the registry's own name and put the token on the wire in clear text. - **`rel` read as a token list.** `rel="nextish"` and `rel=next-archive` matched. Anchoring the end would have traded that for `rel="next"; type="…"` and `rel="next last"`, both legal, both silently stopping at page one. - **Chooser bands vs `withinScope`** (`1.2.3.4` offered a minor upgrade minor would never pick); **`checkUpdates` tail starvation** (images behind a slow registry skipped *forever*); **`rootOf` called twice per file**; **`ConfirmDialog` focus grab**; **the prerelease word lists** differing by one word each way. ## B — the dedup 1. **One registry v2 client** — `packages/registry`: reference parsing, bearer handshake, Link pagination. The origin guard now covers dep-check's forge pagination too. Verified by running `dep-check --dry-run` against Docker Hub and two forges. 2. **One version-tag parser — two orderings, on purpose.** `compare()` pads with `0` in dep-check and `-1` in the module and both are right; merging them breaks one silently, so the shared module says so. *Intended behaviour change:* the module accepted only `v` as a prefix, so `bin-2.5.6` and `bookworm-20240110` never parsed and such a pin could not be offered an upgrade at all. `sha256-9f3a…` is still refused, and nothing applies an upgrade on its own. 3. **One progress fold** for the server, the shell and the modules. #21 put `mergeProgress` in the SDK saying it exists so no client can disagree — then left an identical copy in `server/src/tasks/service.ts` and the docker frontend still folding by hand. Both collapsed onto the SDK's. Matters because `sync()` re-hydrates every 15 s, so both readings appear alternately in one table. 4. **One pin pipeline.** `applyPin`, re-aimed at `actions.ts` after #21 moved the work there — the duplication survived that refactor intact. Refusals are returned as reasons and the action turns them into `TaskFailure`, so the helper states the fact and the action owns the presentation. ## What review changed Julian found four things worth the round trip: the `nextPage` return type (I took the type change he said he wasn't asking for, because it makes the bug a compile error), the untested F5 guard, the unreadable-`next` branch, and the page cap — where he changed my mind with my own measurement, since after this branch the two callers would have *disagreed* about what a truncated list means. Twice his suggested fix would have broken something: anchoring the `rel` match drops `rel="next last"`, and `if (false)` neutering doesn't compile. Both are in the comments with the evidence. I also reviewed this branch myself twice and found **five defects I had introduced** — the focus latch burning on a null ref, both same-host guards comparing hostname rather than origin, an orphaned JSDoc, a false pagination warning on every last page, and the F5 guard silently orphaned by the merge. All fixed here rather than left for review. Every guard is verified by reverting it and watching the relevant test fail. ## Still open The pin actions above `applyPin` have no test beyond the guard cases. `docker/dev-rig` is the honest answer; happy to run it before merge. Split out: **#20**, the four Windows test failures, assigned to @julian.
fix(docker): refuse digest-pinned refs instead of mangling them
All checks were successful
Build and Deploy / verify (pull_request) Successful in 53s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m9s
f295927cee
tagOf and retag both split on the last colon, and in `postgres@sha256:2a1f…`
that colon belongs to `sha256:`. So tagOf reported the hex as the current tag
and retag built `postgres@sha256:16.4` — a reference no registry can serve.
fetchRemoteDigest then answered "no such image in the registry", which is true
of the string we built and says nothing about the operator's actual problem.

Both pin routes now decline by name. The container route checks before taking
the job slot, since there is nothing to hold the project for; the stack route
checks inside the loop and goes out through the same restore path as the other
per-service refusals.

Tests cover both the recognition and the mangle it prevents, so the reason the
guard exists stays visible if someone tries to make retag handle digests.
A 200 with no docker-content-digest came back as {digest: null, error: null},
and every caller reads a missing digest as absence: the pin route refused an
image that was plainly in the registry, and the update checker recorded
"unknown" with no reason attached. Proxying registries that drop the header on
HEAD are real, as are registries that refuse the method with 405/501.

Both now fall through to a GET of the same manifest. If that carries the header
we use it; otherwise the digest is computed from the bytes, which is exact
rather than approximate — a manifest's digest is defined as the SHA-256 of the
bytes served, and the GET reuses MANIFEST_ACCEPT so it is the same manifest the
header would have named. A real 404 still returns absence without a second
request.

The bearer token moved from a local into a closure so the fallback GET is
authorised too; the 401 path has a test because that is what the restructure
could plausibly have broken.
listUpgrades computed its own band arithmetic and withinScope computed the
scope's, and on a four-component scheme they disagreed: 1.2.3.4 -> 1.9.0.0
shares one leading component, which the bands called minor while
withinScope("minor") fixes two and would never have picked it. The chooser
offered a version the minor jump could not reach.

The bands now ask withinScope for the narrowest scope that accepts each
candidate, so the two cannot drift again — there is one definition. Three- and
two-component schemes, and the one-component case, band exactly as before.

Both new tests fail against the previous arithmetic with
"1.9.0.0 is listed under minor but minor declines it", which is the property
worth keeping: anything offered in a band is reachable by that band.
Grouping asked `git rev-parse --show-toplevel` for every file's directory and
commitOne then asked again for the group's own, so a two-repository pin ran
four lookups where two would do.

The answers are shared through a map that lives for one commitLines() call and
no longer. Caching for the module's lifetime would be the wrong trade: an
operator can `git init` a stack directory between two pins, and a remembered
"not a work tree" would then be wrong until the container restarts.

The new test counts the lookups rather than trusting the reasoning — two
directories, two lookups, and both commits still land.
The focus effect depends on `busy`, which it has to: both buttons are disabled
while it is set, .focus() on a disabled button does nothing, and a dialog that
opens busy would otherwise have nothing focused at all.

But it re-ran on every change, so `busy` going false again — an action that
failed and left the dialog up — grabbed focus back to the default button and
discarded wherever the operator had tabbed to.

A latch makes it once per open cycle. Deliberately a plain `let` rather than
$state: it records whether this cycle has had its grab, and making it reactive
would put the effect's own write into its dependencies.
fix(docker): check the stalest image first so no tail is starved
All checks were successful
Build and Deploy / verify (pull_request) Successful in 49s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m10s
3d80422e44
The worker pool walked the images in state.stacks order on every pass, with no
rotation and no resume cursor. If the leading images sat on a slow or
unreachable registry and burned the budget, everything past the cutoff got its
previous verdict back on every pass — forever, not for one pass. A security
update on a tail image would never have surfaced. The serial loop this
concurrency replaced was slower but did always reach the end.

Ordering by staleness restores that guarantee without giving up the
concurrency, and is self-correcting: a skipped image keeps its previous
checkedAt, so it drifts to the front on its own.

The subtlety is the skip entry itself. Its checkedAt is the moment we gave up,
not the moment we knew anything, so sorting on it would push a never-answered
image to the BACK — precisely the images that most need to go first. The
sentinel is now one exported constant read by both the entry and the ordering,
and the test pins the case: a skipped image and an unseen one both sort ahead
of one answered long ago.
The tag walk re-sends its token with each next page, and nextLink followed
whatever the Link header named — including an absolute URL on another host. A
registry answering with someone else's address was handed the credential. The
copy of this loop in docker/backend/updates.ts already refuses exactly this;
only this one did not.

The same drift had cost the other half of the header parsing: `rel=next` is
legal unquoted, and matching only `rel="next"` stopped paginating at page one
against registries that send it bare — answering "is there a newer version"
from a silently partial tag list.

Anonymous pull tokens are not much of a prize, which is why this is a fix and
not an advisory, but handing a credential to whoever asks is not something to
do by omission.
fix: align the two prerelease word lists
All checks were successful
Build and Deploy / verify (pull_request) Successful in 42s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m10s
0fc0fe4661
dep-check knew "next" and the docker module knew "onbuild", each missing the
other's, so a tag one of them held back from a stable pin the other offered as
an upgrade.

Both now carry the union. That is the safe direction: a word in this list only
ever keeps a tag OUT of what is offered against a stable pin, never in.

Two parsers is the actual defect and one list copied twice is not a fix for it.
Until they share a definition — a structural change with no obvious home yet,
since dep-check is standalone and imports nothing from packages/ — the new test
is what notices the next drift.
thisilike changed title from WIP: docker: deferred findings from the #10 review to docker: deferred findings from the #10 review 2026-08-10 22:12:06 +02:00
fix: three defects found reviewing this branch
All checks were successful
Build and Deploy / verify (pull_request) Successful in 52s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m11s
64b788f57d
Self-review of the eight commits before it. All three are mine, introduced in
this branch.

1. ConfirmDialog's new focus latch was set BEFORE the optional-chained
   .focus(), so a null element ref burned the latch and left the dialog with
   nothing focused for the rest of its open cycle — precisely the hole the
   effect exists to close, since the Tab trap then hands the first Tab to the
   danger button. It now latches only once a button has actually taken focus.

2. The registry pagination guard compared hostnames, so a registry could point
   the walk at http:// on its own name: same host, guard satisfied, bearer
   token on the wire in clear text. Both copies compare origin now — the one I
   added to dep-check and the older one in updates.ts that I had cited as the
   example to follow. My first test for this checked a different PORT and
   never the scheme, which is why it passed.

3. Adding NOT_CHECKED put a new declaration between CHECK_BUDGET_MS's doc
   comment and CHECK_BUDGET_MS, leaving the block describing the wrong thing —
   the same orphaned-JSDoc defect this stack already fixed once.

Both guards are verified by reverting them and watching the new tests fail;
the first attempt at that reverted nothing, because the string replacement
missed a line `deno fmt` had rewrapped and reported success anyway.
fix(docker): refuse a bare image id as well as a digest pin
All checks were successful
Build and Deploy / verify (pull_request) Successful in 52s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m11s
0036326141
digestPinned tested for `@`, which a container running an untagged image does
not have: docker reports its image as a bare `sha256:9f3a…`, and retag treats
the digest's own colon as the tag separator, so pinning one produced
`sha256:16.4` and the registry lookup blamed a missing image.

The predicate covers both shapes now and is named digestRef, since a bare id
is a digest reference but not a pin. The message says what the operator can
actually do about either: the service has no tag to move, set a tagged image
in the compose file first.

The bare form is matched on shape rather than on `@` — an algorithm name and
32+ hex characters with no registry or repository in front. A repository whose
whole name is lowercase alphanumeric and whose tag is that long would match
too; declining to pin it beats mangling it.
thisilike changed title from docker: deferred findings from the #10 review to WIP: docker: deferred findings from the #10 review 2026-08-10 22:30:22 +02:00
thisilike changed title from WIP: docker: deferred findings from the #10 review to docker: deferred findings from the #10 review 2026-08-10 22:30:47 +02:00
julian left a comment

All eight claimed fixes verified against the code — digest refusal, the GET fallback with the computed digest, the withinScope-derived bands, the staleness ordering, the rootOf cache (null is cached, checked with !== undefined), the focus latch, the origin guard (port survives parseImageRef, so non-default-port registries still pass), and the unified prerelease list. Tests exercise the logic rather than the mocks.

Two small items before merge, both inline.

All eight claimed fixes verified against the code — digest refusal, the GET fallback with the computed digest, the withinScope-derived bands, the staleness ordering, the rootOf cache (null is cached, checked with `!== undefined`), the focus latch, the origin guard (port survives `parseImageRef`, so non-default-port registries still pass), and the unified prerelease list. Tests exercise the logic rather than the mocks. Two small items before merge, both inline.
@ -104,3 +148,2 @@
if (!res.ok) return { digest: null, error: `registry HTTP ${res.status}` };
return {
digest: res.headers.get("docker-content-digest"),
digest: await digestOfBytes(await got.arrayBuffer()),
Owner

A 200 with no docker-content-digest header and an empty body hashes to the SHA-256 of zero bytes, which can never equal the local digest — the checker would report a spurious update forever against that (broken) registry. Cheap guard: treat an empty arrayBuffer() as { digest: null, error: "empty manifest response" } instead of hashing it.

A 200 with no `docker-content-digest` header and an empty body hashes to the SHA-256 of zero bytes, which can never equal the local digest — the checker would report a spurious update forever against that (broken) registry. Cheap guard: treat an empty `arrayBuffer()` as `{ digest: null, error: "empty manifest response" }` instead of hashing it.
@ -46,3 +61,1 @@
const rel = link?.split(",").find((p) => p.includes('rel="next"'));
const url = rel?.match(/<([^>]+)>/)?.[1];
return url ? new URL(url, base).toString() : null;
const rel = link?.split(",").find((p) => /;\s*rel\s*=\s*"?next"?/i.test(p));
Owner

/;\s*rel\s*=\s*"?next"?/i has no boundary after next, so it also matches rel="nextish" or a hypothetical rel=next-archive. Anchor the end: /;\s*rel\s*=\s*"?next"?\s*$/i on the trimmed part, or match the token explicitly ("?next"?(?=\s*(;|$))). Same regex was added to updates.ts — fix both.

`/;\s*rel\s*=\s*"?next"?/i` has no boundary after `next`, so it also matches `rel="nextish"` or a hypothetical `rel=next-archive`. Anchor the end: `/;\s*rel\s*=\s*"?next"?\s*$/i` on the trimmed part, or match the token explicitly (`"?next"?(?=\s*(;|$))`). Same regex was added to updates.ts — fix both.
julian removed their assignment 2026-08-10 23:12:29 +02:00
fix: refuse a non-manifest 200, and read rel as a token list
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m1s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m12s
2a688892a5
Both from julian's review of this branch, both real.

A 200 with no digest header and an empty body was hashed like any other
response. Zero bytes hash to e3b0c442…b855, which is stable and equal to no
image's digest, so that image would have reported "update available" on every
pass forever. The same hole swallows an HTML error page served with 200, so the
guard covers both: a manifest has to be non-empty and has to parse as JSON
before its bytes are worth hashing.

The rel test matched `rel="nextish"` and `rel=next-archive`, because nothing
said where the token ended. Anchoring the end — the first fix suggested —
would have traded that for two legal shapes: `rel="next"; type="…"`, which
carries a further parameter, and `rel="next last"`, since rel is a
space-separated LIST of relation types. Both are now parsed rather than
pattern-matched, in the dep-check copy and the older updates.ts one the regex
came from.

Each fix is verified by reverting it and watching the new test fail. The first
attempt at that reverted nothing again, so the mutation now asserts the
replacement landed before running anything.
Author
Owner

Both real, both fixed in 2a68889. One of them came with a suggested fix that would have broken working headers, so that part went a different way — detail below.

The empty manifest — right, and it reaches further than stated

Confirmed by running it rather than reading it:

empty body            {"digest":"sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"}
HTML error page, 200  {"digest":"sha256:39b659fd9260d48f7dc59f52a77a5fb39258e82c013f5ee2276fc845ea18a40e"}

e3b0c442…b855 is exactly SHA-256 of zero bytes, as you said, and against any real local digest that is a permanent "update available".

The second line is the same hole with a different body: any 200 hashes to something stable, so an error page served with 200 fails identically. The guard therefore covers both — a manifest must be non-empty and parse as JSON before its bytes are worth hashing. Errors are registry sent an empty manifest and registry sent a manifest that is not JSON, so the two stay distinguishable in the UI.

The rel boundary — right, but neither suggested fix is safe

The finding holds:

part want shipped \s*$ lookahead
; rel="next" ok ok ok
; rel=next ok ok ok
; rel="nextish" WRONG ok ok
; rel=next-archive WRONG ok ok
; rel="next"; type="application/json" ok WRONG ok
; rel="next last" ok WRONG WRONG

Anchoring the end drops a Link part that carries a further parameter after rel — ordinary, and something registries do send. And both suggestions drop rel="next last", because rel is a space-separated list of relation types, not a single token. Each would have swapped a theoretical bug for a real one: silently stopping pagination at page one, which is the failure mode this same function was already fixed for once.

So it parses the value and looks for the token instead — quoted or bare, list or single, further parameters or not. Eleven shapes covered in the tests, including the two above and a lowercase-folded rel=NEXT.

Applied to both copies, though the provenance is the other way round from your note: the regex was already in updates.ts and I copied it into dep-check. So it was an old bug in the module that the copy inherited, not a new one spreading outward — which makes it worth having caught.

On the verification

Each fix is confirmed by reverting it and watching the new test fail. Worth flagging that my first attempt at that verified nothing: the patch script silently matched no lines and reported success, so the mutation "passed" against tests that could not have detected anything. It now asserts the replacement landed before running. That is the second time in this branch the same trap has cost me a bogus green.

deno task ci green — fmt, lint, typecheck, 324 tests.

Thanks for both — the empty-body one in particular would have sat there quietly reporting a phantom update.

Both real, both fixed in `2a68889`. One of them came with a suggested fix that would have broken working headers, so that part went a different way — detail below. ### The empty manifest — right, and it reaches further than stated Confirmed by running it rather than reading it: ``` empty body {"digest":"sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"} HTML error page, 200 {"digest":"sha256:39b659fd9260d48f7dc59f52a77a5fb39258e82c013f5ee2276fc845ea18a40e"} ``` `e3b0c442…b855` is exactly SHA-256 of zero bytes, as you said, and against any real local digest that is a permanent "update available". The second line is the same hole with a different body: any 200 hashes to *something* stable, so an error page served with 200 fails identically. The guard therefore covers both — a manifest must be non-empty **and** parse as JSON before its bytes are worth hashing. Errors are `registry sent an empty manifest` and `registry sent a manifest that is not JSON`, so the two stay distinguishable in the UI. ### The rel boundary — right, but neither suggested fix is safe The finding holds: | part | want | shipped | `\s*$` | lookahead | |---|---|---|---|---| | `; rel="next"` | ✓ | ok | ok | ok | | `; rel=next` | ✓ | ok | ok | ok | | `; rel="nextish"` | ✗ | **WRONG** | ok | ok | | `; rel=next-archive` | ✗ | **WRONG** | ok | ok | | `; rel="next"; type="application/json"` | ✓ | ok | **WRONG** | ok | | `; rel="next last"` | ✓ | ok | **WRONG** | **WRONG** | Anchoring the end drops a Link part that carries a further parameter after `rel` — ordinary, and something registries do send. And both suggestions drop `rel="next last"`, because `rel` is a space-separated **list** of relation types, not a single token. Each would have swapped a theoretical bug for a real one: silently stopping pagination at page one, which is the failure mode this same function was already fixed for once. So it parses the value and looks for the token instead — quoted or bare, list or single, further parameters or not. Eleven shapes covered in the tests, including the two above and a lowercase-folded `rel=NEXT`. Applied to both copies, though the provenance is the other way round from your note: the regex was already in `updates.ts` and I copied it into `dep-check`. So it was an old bug in the module that the copy inherited, not a new one spreading outward — which makes it worth having caught. ### On the verification Each fix is confirmed by reverting it and watching the new test fail. Worth flagging that my first attempt at that verified nothing: the patch script silently matched no lines and reported success, so the mutation "passed" against tests that could not have detected anything. It now asserts the replacement landed before running. That is the second time in this branch the same trap has cost me a bogus green. `deno task ci` green — fmt, lint, typecheck, **324 tests**. Thanks for both — the empty-body one in particular would have sat there quietly reporting a phantom update.
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.
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.
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
All checks were successful
Build and Deploy / verify (pull_request) Successful in 49s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m11s
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.
fix(docker): do not warn about pagination that simply ended
All checks were successful
Build and Deploy / verify (pull_request) Successful in 51s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m10s
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 changed title from docker: deferred findings from the #10 review to docker: the deferred findings from #12, and the dedup behind them 2026-08-11 08:38:05 +02:00
julian left a comment

Re-reviewed everything since 0036326 — the two fixes from the last round plus all of part B, which had not been reviewed at all. deno task ci run locally on 17d7a6b: green, 344 tests.

Both previous findings are resolved, and the pushback on the rel anchoring is right: rel is a space-separated list and rel="next"; type="…" is legal, so parsing the value is the only correct answer — my suggestion would have stopped pagination at page one. The empty/non-JSON guard reaches further than the finding did, and the HTML-page-with-200 case is the one that would actually have bitten.

Part B, verified rather than skimmed:

  • shared/progress.ts imports nothing, so the frontend bundle constraint holds. mergeProgress is the two folds it replaces — the frontend previously wrote phase/total/current/fraction as explicit undefined where this omits the keys, which reads identically at every consumer.
  • packages/registry: parseImageRef is unchanged from the module's copy (diffed against b11a279), so nothing moved behaviour on the way into the package — the port survives, docker.io still maps to registry-1.docker.io, and a bare sha256:… still reaches digestRef rather than this. The origin tests cover http:// on the registry's own name and a different port.
  • pin_apply: the container route reduces to what it did — the ALREADY branch, the no-break rule, the hunk measured against restore.original(). The stricter re-read path is unreachable from the container route (one applyPin call, each file visited once, so restore.has() is always false there) and the stack route's call sits inside the try that restores. Nine cases against a real directory is the right answer to logic that previously had none.
  • version.ts: the two orderings stay in their own files and the tests hold them apart.

One blocking item and one call for you, both inline.

Re-reviewed everything since `0036326` — the two fixes from the last round plus all of part B, which had not been reviewed at all. `deno task ci` run locally on `17d7a6b`: green, 344 tests. Both previous findings are resolved, and the pushback on the `rel` anchoring is right: `rel` is a space-separated list and `rel="next"; type="…"` is legal, so parsing the value is the only correct answer — my suggestion would have stopped pagination at page one. The empty/non-JSON guard reaches further than the finding did, and the HTML-page-with-200 case is the one that would actually have bitten. Part B, verified rather than skimmed: - `shared/progress.ts` imports nothing, so the frontend bundle constraint holds. `mergeProgress` is the two folds it replaces — the frontend previously wrote `phase`/`total`/`current`/`fraction` as explicit `undefined` where this omits the keys, which reads identically at every consumer. - `packages/registry`: `parseImageRef` is unchanged from the module's copy (diffed against `b11a279`), so nothing moved behaviour on the way into the package — the port survives, `docker.io` still maps to `registry-1.docker.io`, and a bare `sha256:…` still reaches `digestRef` rather than this. The origin tests cover `http://` on the registry's own name and a different port. - `pin_apply`: the container route reduces to what it did — the ALREADY branch, the no-break rule, the hunk measured against `restore.original()`. The stricter re-read path is unreachable from the container route (one `applyPin` call, each file visited once, so `restore.has()` is always false there) and the stack route's call sits inside the `try` that restores. Nine cases against a real directory is the right answer to logic that previously had none. - `version.ts`: the two orderings stay in their own files and the tests hold them apart. One blocking item and one call for you, both inline.
@ -58,0 +31,4 @@
export type ParsedTag = ParsedVersion;
/** kept under this module's own name, since every caller here uses it */
export const parseTag = parseVersionTag;
Owner

A call for you rather than a defect: adopting the shared parser widens this module by more than bin-2.5.6. plausiblePrefix accepts any prefix ending in - or _, where the old regex here allowed only v — so date-stamped distro tags now parse as versions in the docker module:

parseTag("bookworm-20240110")
  → { prefix: "bookworm-", nums: [20240110], flavour: "", prerelease: false }
listUpgrades("bookworm-20240110", [..., "bookworm-20240211"])
  → { patch: [], minor: [], major: ["bookworm-20240211"] }

So a container on debian:bookworm-20240110 gains a newestTag and a major-band option it never had, and checkUpdates starts listing tags for those repositories. Defensible — it is a newer image on the same line, and dep-check has answered this way all along — but "major" is a strange word for a dated rebuild, and an operator running scope=major would now have it applied automatically.

Either answer is fine; the tags_test case only pins bin-…, so add a date-stamped case so whichever behaviour you want is stated somewhere rather than inherited.

A call for you rather than a defect: adopting the shared parser widens this module by more than `bin-2.5.6`. `plausiblePrefix` accepts any prefix ending in `-` or `_`, where the old regex here allowed only `v` — so date-stamped distro tags now parse as versions in the docker module: ``` parseTag("bookworm-20240110") → { prefix: "bookworm-", nums: [20240110], flavour: "", prerelease: false } listUpgrades("bookworm-20240110", [..., "bookworm-20240211"]) → { patch: [], minor: [], major: ["bookworm-20240211"] } ``` So a container on `debian:bookworm-20240110` gains a `newestTag` and a major-band option it never had, and `checkUpdates` starts listing tags for those repositories. Defensible — it *is* a newer image on the same line, and dep-check has answered this way all along — but "major" is a strange word for a dated rebuild, and an operator running `scope=major` would now have it applied automatically. Either answer is fine; the tags_test case only pins `bin-…`, so add a date-stamped case so whichever behaviour you want is stated somewhere rather than inherited.
@ -118,3 +79,3 @@
const body = await res.json();
tags.push(...(body.tags ?? []));
url = nextLink(res, url);
url = nextPage(res.headers.get("link"), url);
Owner

nextPage returns null for two different things — there is no next page, and there was one but it left the origin — and both call sites here read it as the first: if (!url) return { tags, source, error: null }. So a registry whose pagination points off-origin now yields page one as a complete, authoritative answer, which is the one failure this file's own header says it must never produce: "a truncated list is returned as an ERROR rather than as a short answer. A partial tag list produces a confident 'up to date' that is wrong, which is the failure this whole tool is supposed to prevent."

Reproduced against 17d7a6b with a stubbed fetch, page one carrying <http://registry-1.docker.io/…>; rel="next":

{"tags":["1.0.0","1.0.1"],"source":"`registry-1.docker.io/library/x`","error":null}

Before this branch dep-check followed that link with the token attached, which 4e2fa09 correctly fixed. The guard is right; answering silently from a partial list in its place is not — the tool's whole job is to not do that.

isNextRel is already exported for exactly this, and updates.ts:188 already uses it to tell the two apart:

const link = res.headers.get("link");
url = nextPage(link, url);
if (!url) {
  if (link?.split(",").some(isNextRel)) {
    return fail(
      source,
      "pagination left the registry's origin; refusing to answer from a partial list",
    );
  }
  return { tags, source, error: null };
}

Same two lines in tagsFromHost at :124. A forge is unlikely to send a cross-origin next, but the shape is identical and that answer is treated as authoritative either way.

Not asking for it in this PR, but the reason both call sites got this wrong is the return type: string | null cannot express "declined". Something like { url } | { declined: string } | null would make dropping it impossible rather than easy.

`nextPage` returns null for two different things — there is no next page, and there was one but it left the origin — and both call sites here read it as the first: `if (!url) return { tags, source, error: null }`. So a registry whose pagination points off-origin now yields page one as a complete, authoritative answer, which is the one failure this file's own header says it must never produce: *"a truncated list is returned as an ERROR rather than as a short answer. A partial tag list produces a confident 'up to date' that is wrong, which is the failure this whole tool is supposed to prevent."* Reproduced against `17d7a6b` with a stubbed fetch, page one carrying `<http://registry-1.docker.io/…>; rel="next"`: ``` {"tags":["1.0.0","1.0.1"],"source":"`registry-1.docker.io/library/x`","error":null} ``` Before this branch dep-check followed that link with the token attached, which `4e2fa09` correctly fixed. The guard is right; answering silently from a partial list in its place is not — the tool's whole job is to not do that. `isNextRel` is already exported for exactly this, and `updates.ts:188` already uses it to tell the two apart: ```ts const link = res.headers.get("link"); url = nextPage(link, url); if (!url) { if (link?.split(",").some(isNextRel)) { return fail( source, "pagination left the registry's origin; refusing to answer from a partial list", ); } return { tags, source, error: null }; } ``` Same two lines in `tagsFromHost` at :124. A forge is unlikely to send a cross-origin `next`, but the shape is identical and that answer is treated as authoritative either way. Not asking for it in this PR, but the reason both call sites got this wrong is the return type: `string | null` cannot express "declined". Something like `{ url } | { declined: string } | null` would make dropping it impossible rather than easy.
nextPage returned `string | null` and null meant two different things — there
is no next page, and there was one but it left the origin. Every caller read
it as the first. So a registry pointing its pagination off-origin produced a
complete, authoritative answer built from page one: dep-check with
`error: null`, and listTags feeding findUpgrade a partial list, which reports
"no newer version" about a repository that has one. That is precisely the
failure dep-check's own header says the tool exists to prevent, and the
same-origin guard added earlier in this branch is what created it — a
credential leak traded for a wrong answer.

Fixed at the type rather than at the call sites. NextPage is now
{next|declined|end}, so reaching the URL is impossible without saying what
happens to the other two; collapsing the branch is a compile error rather than
a silent truncation. That is the actual defect: three call sites got this wrong
because the type let them.

All three now refuse. dep-check fails the listing with the same wording as its
page-cap truncation; listTags returns a tagsError, which its caller already
declines to cache.

Also renames a `page` that shadowed the loop counter in both dep-check loops.
test(docker): state what a date-stamped tag now does
All checks were successful
Build and Deploy / verify (pull_request) Successful in 49s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m10s
cb98ad1f3e
Adopting the shared parser widened this module past `bin-…`: any prefix ending
in - or _ parses, so `debian:bookworm-20240110` is a version where it used to
be nothing at all.

Kept deliberately, and now written down. A dated rebuild IS a newer image on
the same line — usually the security patch an operator most wants offered — and
showing nothing was the worse answer. It lands in `major` because the scheme
has one component, and the rule for those was already written down: with
nothing below the major to move, every move is a major decision, the same
answer this module gives for `forgejo:14` -> `15`.

Nothing applies it on its own. newerTag reaches a badge and a button labelled
"edits the compose file"; the scheduler only refreshes.
Author
Owner

Both addressed in 27bd4ce and cb98ad1. 349 tests.

The blocker — right, and I took the fix you said you weren't asking for

Reproduced exactly as you described before touching anything:

{"tags":["1.0.0","1.0.1"],"source":"`registry-1.docker.io/library/x`","error":null}
pages fetched: 1

Worse than the diff shows: I hit this same conflation in updates.ts during my own review pass and fixed it only for the log message — I had the insight and failed to carry it thirty lines to the call sites that return an answer. And the guard that created it (4e2fa09) was mine too, so this branch traded a credential leak for the one wrong answer dep-check exists to prevent.

I did the type change rather than the two-line version, because your last paragraph is the actual finding. NextPage is now {next|declined|end}, so the URL is unreachable without saying what happens to the other two. Deleting the declined branch is now a compile error — the mutation check couldn't even run until I made it type-valid first:

error: Type checking failed.

Forced type-valid (declined → return as end), both layers go red:

a next page off the origin is an error, not a short answer ... FAILED
the forge path refuses the same way ... FAILED

Three call sites now refuse: both dep-check loops fail with the same wording as the page-cap truncation, and listTags returns a tagsError, which cachedTags already declines to cache so the next pass retries. Also renamed a page that was shadowing the loop counter in both dep-check loops.

Not fixed, flagged: listTags's own 20-page cap still stops silently rather than erroring — the same class, but pre-existing rather than introduced here, and changing it changes update-check behaviour for very large repositories. Say if you want it in scope.

The date-stamped call — kept, and one correction

Kept the widening, with the test you asked for. A dated rebuild is a newer image on the same line and usually the security patch an operator most wants surfaced; showing nothing was the worse answer. "Major" is odd as a word but it is the rule already written down for single-component schemes — the same answer this module gives for forgejo:1415.

One correction to the premise: nothing applies it automatically. newerTag reaches exactly two places — a badge and a button in StacksPage titled "edits the compose file". The scheduler only calls refreshStacks/refreshUpdates; there is no auto-pin path, so scope=major widens what is offered behind a click, not what happens on its own. That is what made the call easy. If I've missed a path where scope drives a write, point at it and I'll revisit.

The test pins the bands, the prefix, and that a different line (trixie-…) is still not an upgrade.

Both addressed in `27bd4ce` and `cb98ad1`. 349 tests. ### The blocker — right, and I took the fix you said you weren't asking for Reproduced exactly as you described before touching anything: ``` {"tags":["1.0.0","1.0.1"],"source":"`registry-1.docker.io/library/x`","error":null} pages fetched: 1 ``` Worse than the diff shows: I hit this same conflation in `updates.ts` during my own review pass and fixed it *only for the log message* — I had the insight and failed to carry it thirty lines to the call sites that return an answer. And the guard that created it (`4e2fa09`) was mine too, so this branch traded a credential leak for the one wrong answer dep-check exists to prevent. I did the type change rather than the two-line version, because your last paragraph is the actual finding. `NextPage` is now `{next|declined|end}`, so the URL is unreachable without saying what happens to the other two. Deleting the `declined` branch is now a **compile error** — the mutation check couldn't even run until I made it type-valid first: ``` error: Type checking failed. ``` Forced type-valid (`declined` → return as end), both layers go red: ``` a next page off the origin is an error, not a short answer ... FAILED the forge path refuses the same way ... FAILED ``` Three call sites now refuse: both dep-check loops fail with the same wording as the page-cap truncation, and `listTags` returns a `tagsError`, which `cachedTags` already declines to cache so the next pass retries. Also renamed a `page` that was shadowing the loop counter in both dep-check loops. **Not fixed, flagged:** `listTags`'s own 20-page cap still stops silently rather than erroring — the same class, but pre-existing rather than introduced here, and changing it changes update-check behaviour for very large repositories. Say if you want it in scope. ### The date-stamped call — kept, and one correction Kept the widening, with the test you asked for. A dated rebuild *is* a newer image on the same line and usually the security patch an operator most wants surfaced; showing nothing was the worse answer. "Major" is odd as a word but it is the rule already written down for single-component schemes — the same answer this module gives for `forgejo:14` → `15`. One correction to the premise: **nothing applies it automatically.** `newerTag` reaches exactly two places — a badge and a button in `StacksPage` titled *"edits the compose file"*. The scheduler only calls `refreshStacks`/`refreshUpdates`; there is no auto-pin path, so `scope=major` widens what is *offered* behind a click, not what happens on its own. That is what made the call easy. If I've missed a path where scope drives a write, point at it and I'll revisit. The test pins the bands, the prefix, and that a different line (`trixie-…`) is still not an upgrade.
test: cover the pagination happy path, and refuse an unprovable origin
All checks were successful
Build and Deploy / verify (pull_request) Successful in 43s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m11s
045f97747e
Self-review of the two commits before this one turned up no defect in them,
but one hole and one soft spot.

The hole: no test ever made listTags FOLLOW a legitimate next page. That loop
was rewritten twice in this branch — once moving to the shared client, once
adding the declined branch — with only refusal-path coverage, so a break would
have shown up as "no newer version" in production rather than as a red test.
Probed first to be sure it worked before writing an assertion around it: three
pages, relative links resolved against the page they came from, every page's
tags kept. Reverting the follow makes the new test fail.

The soft spot: nextPage answered "end" when the BASE could not be parsed. End
means the listing is complete, and we cannot know that if we could not parse
the base well enough to compare origins — it is the same silent truncation the
union was introduced to make impossible, arriving through the one door left
open. Unprovable is now refused.
main gained the host-owned task runs (#21), which moved the docker module out
from under most of part B: the pin routes are now thin shims over a task run
and the work lives in actions.ts, jobs.ts is deleted, and the SDK has grown a
mergeProgress of its own that module frontends may import.

Resolved by taking main's shape wherever it superseded mine, rather than
re-imposing an older structure:

- mod.ts: main's. My route bodies are gone because the routes are gone; both
  pin routes are now `runNow("…-pin", …)`.
- jobs.ts: accepted the deletion. The merge() it carried was the thing part B
  deduplicated, and the task system replaced the whole file.
- docker/shared/progress.ts: deleted. sdk/client already exports mergeProgress
  and TaskProgress to module frontends for exactly the reason part B gave, so
  keeping mine would have been a third copy rather than one fewer.
- compose_progress.ts and update_feed.svelte.ts: main's.
- deno.json: both workspace members.

Two things the merge orphaned, restored in the commits that follow rather than
quietly dropped: the F5 digest guard, whose only call sites were the route
bodies, and pin_apply, which now has no importer. actions.ts still carries both
copies of the pin walk, so the extraction is still worth having — it just needs
re-aiming.

Part A survives intact: pin.ts, updates.ts, tags.ts, commit.ts,
packages/registry, ConfirmDialog and dep-check were barely touched by #21.

421 tests green.
#21 moved the pin work out of the routes and into actions.ts, and both copies
of the file walk moved with it — the duplication survived the refactor intact,
so the extraction is still worth having.

applyPin now speaks the actions' vocabulary rather than HTTP's: refusals are
returned as reasons ("image-from-variable" with the file, "no-image-line") and
the caller turns them into the TaskFailure it already knows how to render. That
is the better shape anyway — the helper states the fact, the action owns the
presentation, and the compensating undo stays with whoever holds the run.
An onWrite hook keeps the per-file lines in the run log.

Also restores the F5 guard, whose only call sites were the route bodies the
merge replaced: a digest ref or a bare image id has no tag to move, and retag
would splice the new tag into the digest so the registry gets blamed for an
image nobody asked for. Both actions refuse by name before fetching or writing.

The new failure needed two entries it would not otherwise have had: PIN_STATUS,
without which it answered 500 instead of 400, and pinMessage, without which the
operator got the bare reason string.
refactor: one progress fold for the server, the shell and the modules
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m11s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m12s
ca55a13506
#21 introduced mergeProgress in the SDK with the comment that it exists so no
client can disagree with another about what a keyed entity means — and then
left two more copies: an identical one in server/src/tasks/service.ts, and the
docker frontend still folding by hand. Three implementations of a rule whose
whole value is that everyone applies the same one.

The server's copy is deleted; it now imports the SDK's, which it was already
importing from. Byte-identical apart from comments, so nothing moved.

The docker frontend now calls it too. That one matters most: sync() re-hydrates
the feed from the server every 15 s while a run is live, so the server's rows
and the client's are displayed alternately in the same table, and any
disagreement makes a finished layer flip between two readings.

mergeProgress is generic in the progress type so a narrowed one survives the
fold — the docker frontend needs ComposeProgress back, not TaskProgress. That
required saying what two under-specified test literals actually are; without an
annotation the generic infers the literal's own shape and hides the very fields
under test.

The module bundle still imports @opsdeck/sdk/client rather than inlining it, so
the singleton contract holds.
Author
Owner

Rebased onto the task-runs world. ceff745 merges main, 216043d and ca55a13 adapt. 421 tests, deno task ci green, deno task build clean.

#21 moved the docker module out from under most of part B, so I took main's shape wherever it superseded mine rather than re-imposing an older structure.

What the merge dropped, and what I did about it

gone why now
my route bodies both pin routes are runNow("…-pin", …) shims took main's mod.ts
jobs.ts + its merge() replaced by the task system accepted the deletion
docker/shared/progress.ts sdk/client already exports mergeProgress for exactly this reason deleted; mine would have been a third copy

Two things it orphaned silently, which is why I checked rather than trusting a green build: the F5 digest guard — its only call sites were the route bodies, so digestRef survived as a function nobody called — and pin_apply, left with no importer. Both restored.

The duplication survived #21 — it just moved

actions.ts still carries both copies of the pin walk, so the extraction was still worth having. applyPin now speaks the actions' vocabulary instead of HTTP's: refusals come back as reasons (image-from-variable with the file, no-image-line) and the action turns them into the TaskFailure it already renders. Better shape than before — the helper states the fact, the action owns the presentation, the compensating undo stays with whoever holds the run.

Restoring F5 needed two entries it would not otherwise have had: PIN_STATUS, without which the new reason answered 500 instead of 400, and pinMessage, without which the operator got a bare reason string. Both are the kind of thing a green typecheck says nothing about.

And the fold, done properly this time

#21 added mergeProgress to the SDK saying it exists "so no client can disagree with another about what a keyed entity means" — then left an identical copy in server/src/tasks/service.ts and the docker frontend still folding by hand. Three implementations of a rule whose entire value is that everyone applies the same one.

Both collapsed onto the SDK's. The frontend one matters most: sync() re-hydrates from the server every 15 s while a run is live, so both readings appear alternately in one table and a disagreement makes a finished layer visibly flip.

mergeProgress is generic in the progress type now, so a narrowed one survives the fold — the docker frontend needs ComposeProgress back, not TaskProgress. That flushed out two under-specified test literals: without an annotation the generic infers the literal's own shape and hides the very fields under test.

Verified the module bundle still imports @opsdeck/sdk/client rather than inlining it, so the singleton contract holds.

Unchanged

Part A survived intact — pin.ts, updates.ts, tags.ts, commit.ts, packages/registry, ConfirmDialog and dep-check were barely touched by #21.

Still open from before: the actions above applyPin have no test (docker/dev-rig is what would cover them), and listTags's 20-page cap still truncates silently.

Rebased onto the task-runs world. `ceff745` merges main, `216043d` and `ca55a13` adapt. **421 tests**, `deno task ci` green, `deno task build` clean. #21 moved the docker module out from under most of part B, so I took main's shape wherever it superseded mine rather than re-imposing an older structure. ### What the merge dropped, and what I did about it | gone | why | now | |---|---|---| | my route bodies | both pin routes are `runNow("…-pin", …)` shims | took main's `mod.ts` | | `jobs.ts` + its `merge()` | replaced by the task system | accepted the deletion | | `docker/shared/progress.ts` | `sdk/client` already exports `mergeProgress` for exactly this reason | deleted; mine would have been a *third* copy | Two things it orphaned silently, which is why I checked rather than trusting a green build: **the F5 digest guard** — its only call sites were the route bodies, so `digestRef` survived as a function nobody called — and **`pin_apply`**, left with no importer. Both restored. ### The duplication survived #21 — it just moved `actions.ts` still carries **both** copies of the pin walk, so the extraction was still worth having. `applyPin` now speaks the actions' vocabulary instead of HTTP's: refusals come back as reasons (`image-from-variable` with the file, `no-image-line`) and the action turns them into the `TaskFailure` it already renders. Better shape than before — the helper states the fact, the action owns the presentation, the compensating undo stays with whoever holds the run. Restoring F5 needed two entries it would not otherwise have had: **`PIN_STATUS`**, without which the new reason answered **500 instead of 400**, and **`pinMessage`**, without which the operator got a bare reason string. Both are the kind of thing a green typecheck says nothing about. ### And the fold, done properly this time #21 added `mergeProgress` to the SDK saying it exists *"so no client can disagree with another about what a keyed entity means"* — then left an identical copy in `server/src/tasks/service.ts` and the docker frontend still folding by hand. Three implementations of a rule whose entire value is that everyone applies the same one. Both collapsed onto the SDK's. The frontend one matters most: `sync()` re-hydrates from the server every 15 s while a run is live, so both readings appear alternately in one table and a disagreement makes a finished layer visibly flip. `mergeProgress` is generic in the progress type now, so a narrowed one survives the fold — the docker frontend needs `ComposeProgress` back, not `TaskProgress`. That flushed out two under-specified test literals: without an annotation the generic infers the literal's own shape and hides the very fields under test. Verified the module bundle still imports `@opsdeck/sdk/client` rather than inlining it, so the singleton contract holds. ### Unchanged Part A survived intact — `pin.ts`, `updates.ts`, `tags.ts`, `commit.ts`, `packages/registry`, `ConfirmDialog` and dep-check were barely touched by #21. Still open from before: the actions above `applyPin` have no test (`docker/dev-rig` is what would cover them), and `listTags`'s 20-page cap still truncates silently.
julian left a comment

Re-reviewed everything since 17d7a6b — the two fixes from the last round, 045f977, and the whole post-merge adaptation. deno task ci on ca55a13: green, 421 tests. deno task build: exit 0, and the docker bundle emits import{mergeProgress as Sa,…} rather than a copy, so the singleton contract does hold.

The blocker is resolved, and it is load-bearing. I didn't take the type change on faith — I forced it type-valid and collapsed declined back into end at both dep-check call sites and in updates.ts. Three tests go red across both layers:

a partial tag list is an error, not a quiet 'up to date' ... FAILED
a next page off the origin is an error, not a short answer ... FAILED
the forge path refuses the same way ... FAILED

You were right to take the type over the two-line version. NextPage reads as the thing it is now, and the declined-for-an-unparseable-base branch in 045f977 is the better half of that change — refusing to claim completeness you cannot prove is the rule, not a special case. Which is why one branch away still breaks it; see inline.

The date-stamped call is settled the right way and, more to the point, is now written down — including that bookworm-slim is still not a version, which is the half that would have been easy to lose.

Post-merge adaptation, verified rather than skimmed:

  • The orphan check was the right instinct. Both restorations hold: digestRef is reachable again at actions.ts:334 and :474, pin_apply has an importer. PIN_STATUS["no-tag-to-move"] and pinMessage reach the same pinResponse both routes use, so the 400 and the sentence land on both.
  • applyPin re-aimed at the actions is a better shape than the route version. Reasons as data, presentation at the caller, undo with whoever holds the run. Reducing !edits.length to "already, therefore" is correct: applyPin returns no-image-line when !patches.length && !already, so ok: true with no patches can only be the already-written case. The stack loop's changes push is still per-service and still gated on a real write.
  • The server's mergeProgress copy really was byte-identical apart from comments, and the generic is what lets ComposeProgress survive the fold rather than being widened to TaskProgress at the door.
  • Checked for anything else the merge left dangling: only restoreNote, which is dead on main too (7a9b745), so not yours. Same for the doubled JSDoc above pinResponse.

Two required items and one answer, all inline. Both required items are small; neither touches the shape of the branch.

Re-reviewed everything since `17d7a6b` — the two fixes from the last round, `045f977`, and the whole post-merge adaptation. `deno task ci` on `ca55a13`: green, **421 tests**. `deno task build`: exit 0, and the docker bundle emits `import{mergeProgress as Sa,…}` rather than a copy, so the singleton contract does hold. **The blocker is resolved, and it is load-bearing.** I didn't take the type change on faith — I forced it type-valid and collapsed `declined` back into `end` at both dep-check call sites and in `updates.ts`. Three tests go red across both layers: ``` a partial tag list is an error, not a quiet 'up to date' ... FAILED a next page off the origin is an error, not a short answer ... FAILED the forge path refuses the same way ... FAILED ``` You were right to take the type over the two-line version. `NextPage` reads as the thing it is now, and the `declined`-for-an-unparseable-base branch in `045f977` is the better half of that change — refusing to claim completeness you cannot prove is the rule, not a special case. Which is why one branch away still breaks it; see inline. The date-stamped call is settled the right way and, more to the point, is now written down — including that `bookworm-slim` is still not a version, which is the half that would have been easy to lose. Post-merge adaptation, verified rather than skimmed: - **The orphan check was the right instinct.** Both restorations hold: `digestRef` is reachable again at `actions.ts:334` and `:474`, `pin_apply` has an importer. `PIN_STATUS["no-tag-to-move"]` and `pinMessage` reach the same `pinResponse` both routes use, so the 400 and the sentence land on both. - **`applyPin` re-aimed at the actions is a better shape than the route version.** Reasons as data, presentation at the caller, undo with whoever holds the run. Reducing `!edits.length` to "already, therefore" is correct: `applyPin` returns `no-image-line` when `!patches.length && !already`, so `ok: true` with no patches can only be the already-written case. The stack loop's `changes` push is still per-service and still gated on a real write. - **The server's `mergeProgress` copy really was byte-identical** apart from comments, and the generic is what lets `ComposeProgress` survive the fold rather than being widened to `TaskProgress` at the door. - Checked for anything else the merge left dangling: only `restoreNote`, which is dead on `main` too (`7a9b745`), so not yours. Same for the doubled JSDoc above `pinResponse`. Two required items and one answer, all inline. Both required items are small; neither touches the shape of the branch.
@ -335,0 +331,4 @@
// treat the digest's own colon as the separator — producing
// `postgres@sha256:16.4`, which the registry then reports as a missing
// image. Refused by name before anything is fetched or written.
if (digestRef(target.image)) {
Owner

Required: this guard has no test, and it is the one guard in the branch with a demonstrated history of vanishing.

Neutered both copies and ran the suite:

guards neutered: 2
294 passed | 0 additional failures

So F5 can be deleted from both actions and nothing goes red. That is exactly how #21's merge orphaned digestRef in the first place — it survived as a function nobody called, and a green typecheck said nothing. You caught it by reading. The next person merging main past this will not.

pin_test.ts covers digestRef and retag as units, which is why the mangling itself is pinned down. What is unpinned is that a pin action asks. That is the part that went missing.

Against the standard this PR sets for itself — "every guard here is verified by reverting it and watching the relevant test fail" — this is the one that does not meet it, and the cheapest to bring up to it: one action-level case per route asserting no-tag-to-move for postgres@sha256:2a1f…, and one for the bare sha256:9f3a… shape that has no @ to catch it by. docker/dev-rig is the heavier answer to the untested routes above applyPin and I am not asking for that here; two cases against this specific guard are enough to make the orphaning loud.

**Required: this guard has no test, and it is the one guard in the branch with a demonstrated history of vanishing.** Neutered both copies and ran the suite: ``` guards neutered: 2 294 passed | 0 additional failures ``` So F5 can be deleted from both actions and nothing goes red. That is exactly how #21's merge orphaned `digestRef` in the first place — it survived as a function nobody called, and a green typecheck said nothing. You caught it by reading. The next person merging main past this will not. `pin_test.ts` covers `digestRef` and `retag` as units, which is why the mangling itself is pinned down. What is unpinned is that a pin action *asks*. That is the part that went missing. Against the standard this PR sets for itself — *"every guard here is verified by reverting it and watching the relevant test fail"* — this is the one that does not meet it, and the cheapest to bring up to it: one action-level case per route asserting `no-tag-to-move` for `postgres@sha256:2a1f…`, and one for the bare `sha256:9f3a…` shape that has no `@` to catch it by. `docker/dev-rig` is the heavier answer to the untested routes above `applyPin` and I am not asking for that here; two cases against this specific guard are enough to make the orphaning loud.
@ -198,3 +197,4 @@
}
url = hop.status === "next" ? hop.url : null;
}
return { tags, error: null };
Owner

Answering the question you asked twice: yes, take it, and take it here.

The loop exits on url && page < 20, and hitting the cap lands on this line — { tags, error: null }, a partial list reported as complete. It is the same defect as the one you just fixed thirty lines up, in the same function, reached by a different exit.

The reason I am not treating it as merely pre-existing: dep-check already made this exact call for this exact reason (MAX_PAGES, TRUNCATED), the two now share packages/registry, and after this branch they disagree about what a truncated list means. One of them refuses and one of them answers. That is the duplication this PR exists to remove, surviving as a behavioural split instead of a code one.

On your reservation about very large repositories — your own measurement is the argument. library/debian at 2976 tags fits inside 20 pages only while the registry honours n=1000; a registry that caps page size the way Forgejo caps limit=100 at 50 puts debian past the cap, and the failure is silent. "No newer version" about a repository that has one is worse than an error, which is the sentence this branch has been written around.

cachedTags already declines to cache a tagsError, so a retry costs a pass rather than a stuck verdict — the same reason the declined path was safe to make an error.

**Answering the question you asked twice: yes, take it, and take it here.** The loop exits on `url && page < 20`, and hitting the cap lands on this line — `{ tags, error: null }`, a partial list reported as complete. It is the same defect as the one you just fixed thirty lines up, in the same function, reached by a different exit. The reason I am not treating it as merely pre-existing: `dep-check` already made this exact call for this exact reason (`MAX_PAGES`, `TRUNCATED`), the two now share `packages/registry`, and after this branch they disagree about what a truncated list means. One of them refuses and one of them answers. That is the duplication this PR exists to remove, surviving as a behavioural split instead of a code one. On your reservation about very large repositories — your own measurement is the argument. `library/debian` at 2976 tags fits inside 20 pages only while the registry honours `n=1000`; a registry that caps page size the way Forgejo caps `limit=100` at 50 puts debian past the cap, and the failure is silent. "No newer version" about a repository that has one is worse than an error, which is the sentence this branch has been written around. `cachedTags` already declines to cache a `tagsError`, so a retry costs a pass rather than a stuck verdict — the same reason the `declined` path was safe to make an error.
@ -0,0 +149,4 @@
for (const part of link.split(",")) {
if (!isNextRel(part)) continue;
const raw = /<([^>]+)>/.exec(part)?.[1];
if (!raw) continue;
Owner

if (!raw) continue falls through to return { status: "end" } — a next that was offered and could not be read, reported as "none offered: the listing really is complete". Same claim 045f977 just refused to make for an unparseable base, one branch away.

Probed against this commit:

{"status":"end"}   <= <https://reg.example/v2/x/tags/list?n=100&last=a,b>; rel="next"
{"status":"end"}   <= <>; rel="next"
{"status":"end"}   <= rel="next"

The first is the interesting one and it is link.split(","), not the <…> match: a comma inside the URI splits one part into <https://…last=a (no rel) and b>; rel="next" (no <), so the header is read as offering no next page at all. Registry tag names cannot contain commas, so I am not claiming Docker Hub triggers this today — but the Link header is registry-controlled, the loop consuming it treats end as authoritative, and the resulting answer is a confident "up to date" built from page one.

Optional but one line, and it is the rule the union already states: if (!raw) return { status: "declined", link };. Unreadable is unprovable is refused.

`if (!raw) continue` falls through to `return { status: "end" }` — a `next` that was offered and could not be read, reported as *"none offered: the listing really is complete"*. Same claim `045f977` just refused to make for an unparseable base, one branch away. Probed against this commit: ``` {"status":"end"} <= <https://reg.example/v2/x/tags/list?n=100&last=a,b>; rel="next" {"status":"end"} <= <>; rel="next" {"status":"end"} <= rel="next" ``` The first is the interesting one and it is `link.split(",")`, not the `<…>` match: a comma inside the URI splits one part into `<https://…last=a` (no `rel`) and `b>; rel="next"` (no `<`), so the header is read as offering no next page at all. Registry tag names cannot contain commas, so I am not claiming Docker Hub triggers this today — but the `Link` header is registry-controlled, the loop consuming it treats `end` as authoritative, and the resulting answer is a confident "up to date" built from page one. Optional but one line, and it is the rule the union already states: `if (!raw) return { status: "declined", link };`. Unreadable is unprovable is refused.
The guard could be deleted from both pin actions and nothing went red. That is
not hypothetical: it is exactly how #21's merge orphaned it — digestRef
survived as a function nobody called, and a green typecheck said nothing.
pin_test.ts pins down what digestRef and retag DO; what was unpinned is that a
pin action ASKS, which is the part that went missing.

Four cases, two per action, covering both shapes: the digest-pinned ref and the
bare image id that has no `@` to catch it by. The actions are captured through
a stub ctx.tasks.define and invoked directly; fetch and ctx.host.run are rigged
to throw, so a guard that let the reference through fails on reaching the
network rather than passing quietly.

Deleting both guards turns all four red.
fix: two more places that claimed a partial list was complete
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m9s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m11s
d38036162a
Both are the rule the NextPage union already states, applied where it had not
reached yet.

An offered `next` we cannot read fell through to "end" — the same claim the
unparseable-base branch refuses to make, one branch away. It also covers a URI
containing a comma, which is the interesting shape: link.split(",") tears such
a part in two, leaving the `rel` in one half and the `<…>` in the other, so the
header reads as offering no next page at all and page one becomes the whole
answer. Registry tag names cannot contain commas, but the Link header is the
registry's to write.

listTags exhausting its page cap returned `{ tags, error: null }` — a partial
list reported as complete, the same defect as the declined branch by a
different exit. dep-check already refuses here for this reason, from the client
these two now share, so leaving it turned a code duplication into a
behavioural disagreement. The cap has a name now, and the doc says why 20 pages
is not obviously enough: `n=1000` is a request, not a promise, and a registry
capping page size the way Forgejo caps `limit=100` at 50 puts an ordinary
repository past it. cachedTags already declines to cache an errored listing, so
the cost is a retry next pass rather than a stuck verdict.

One correction to the reported repro: a bare `rel="next"` with no preceding
semicolon stays "end", because isNextRel never matches it — nothing was
offered. Tested alongside the others so the difference is on the record.
Author
Owner

All three taken — 37b4855 and d380361. 427 tests, ci green.

The untested guard — fair, and the sharpest finding of the review

You neutered it and nothing went red. My own PR body says "every guard here is verified by reverting it and watching the relevant test fail", and this was the one that didn't meet it — the one guard with a demonstrated history of vanishing.

Four cases now, two per action, both shapes. The actions are captured through a stub ctx.tasks.define and invoked directly, with fetch and ctx.host.run rigged to throw, so a guard that lets the reference through fails on reaching the network rather than passing quietly. Deleting both guards:

guards deleted: 2
container-pin refuses a digest-pinned ref ... FAILED
stack-pin refuses a digest-pinned ref ... FAILED
container-pin refuses a bare image id ... FAILED
stack-pin refuses a bare image id ... FAILED

Worth noting your if (false) neutering wouldn't have compiled — I had to delete the blocks outright, which is closer to how the orphaning actually happens anyway.

The unreadable next — taken, and the comma is the real finding

if (!raw) return { status: "declined", link }. Your diagnosis of the mechanism is the valuable part: it isn't the <…> match, it's link.split(",") tearing one part in two so the rel and the URI land in different halves. That case is tested explicitly.

One correction to the repro: a bare rel="next" with no preceding semicolon stays end, and your one-liner doesn't change it either — isNextRel requires the ;, so it never matches and nothing was ever offered. I asserted declined there first and the test caught me. It's now tested as end with a comment, since it looks like the case above and isn't.

The page cap — you're right, and the argument that moved me is the one I'd made myself

I'd flagged it as pre-existing and worried about large repositories. That was the wrong frame: after this branch the two callers disagree about what a truncated list means, one refusing and one answering, from a client they now share. A behavioural split replacing a code split is worse than what the dedup removed.

And my own measurement was the counter-argument to my own reservation — library/debian at 2976 tags only fits in 20 pages while n=1000 is honoured, and a registry capping page size the way Forgejo caps limit=100 at 50 puts it past the cap silently. The cap has a name now and the doc says exactly that.

Both new refusals are mutation-checked; reverting either turns its test red.

Still open, unchanged

The actions above applyPin have no test beyond the guard cases — docker/dev-rig remains the honest answer, and I'll run it if you want it before merge rather than after.

All three taken — `37b4855` and `d380361`. **427 tests**, ci green. ### The untested guard — fair, and the sharpest finding of the review You neutered it and nothing went red. My own PR body says *"every guard here is verified by reverting it and watching the relevant test fail"*, and this was the one that didn't meet it — the one guard with a demonstrated history of vanishing. Four cases now, two per action, both shapes. The actions are captured through a stub `ctx.tasks.define` and invoked directly, with `fetch` and `ctx.host.run` rigged to throw, so a guard that lets the reference through fails on reaching the network rather than passing quietly. Deleting both guards: ``` guards deleted: 2 container-pin refuses a digest-pinned ref ... FAILED stack-pin refuses a digest-pinned ref ... FAILED container-pin refuses a bare image id ... FAILED stack-pin refuses a bare image id ... FAILED ``` Worth noting your `if (false)` neutering wouldn't have compiled — I had to delete the blocks outright, which is closer to how the orphaning actually happens anyway. ### The unreadable next — taken, and the comma is the real finding `if (!raw) return { status: "declined", link }`. Your diagnosis of the mechanism is the valuable part: it isn't the `<…>` match, it's `link.split(",")` tearing one part in two so the `rel` and the URI land in different halves. That case is tested explicitly. **One correction to the repro:** a bare `rel="next"` with no preceding semicolon stays `end`, and your one-liner doesn't change it either — `isNextRel` requires the `;`, so it never matches and nothing was ever offered. I asserted `declined` there first and the test caught me. It's now tested as `end` with a comment, since it looks like the case above and isn't. ### The page cap — you're right, and the argument that moved me is the one I'd made myself I'd flagged it as pre-existing and worried about large repositories. That was the wrong frame: **after this branch the two callers disagree about what a truncated list means**, one refusing and one answering, from a client they now share. A behavioural split replacing a code split is worse than what the dedup removed. And my own measurement was the counter-argument to my own reservation — `library/debian` at 2976 tags only fits in 20 pages while `n=1000` is honoured, and a registry capping page size the way Forgejo caps `limit=100` at 50 puts it past the cap silently. The cap has a name now and the doc says exactly that. Both new refusals are mutation-checked; reverting either turns its test red. ### Still open, unchanged The actions above `applyPin` have no test beyond the guard cases — `docker/dev-rig` remains the honest answer, and I'll run it if you want it before merge rather than after.
refactor(docker): the reason is data, the sentence is presentation
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m21s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m10s
4e9d1561f5
Tidying the seam the merge left between the two designs.

The stack action still declared the patch shape inline where PinPatch now names
it — a duplicated type in a branch about duplicated code.

NO_TAG_MSG was the pre-merge design showing through: the actions passed the
whole sentence as `detail` so a route could print it. Actions state reasons and
pinMessage owns the wording for every other failure, so this one now reads like
its siblings — "db has no tag to move: …" rather than "db: …" — and the
constant, whose only remaining reader was a field nobody read, is gone.

Also drops the ImageEdit import that went unused when the walk moved into
pin_apply.
Author
Owner

4e9d156 — a last pass over the seam the merge left between the two designs, then this is everything. 427 tests, ci green, 25 commits on main.

What's in it

Neither is a defect; both are this PR's own subject matter left half-done by the adaptation.

  • The stack action still declared the patch shape inline where PinPatch names it — a duplicated type in a branch about duplicated code.
  • NO_TAG_MSG was the pre-merge design showing through. The actions passed a whole sentence as detail so a route could print it, which made sense when the routes did the printing. Now that actions state reasons and pinMessage owns the wording for every other failure, that one reads like its siblings — "db has no tag to move: …" rather than "db: …" — and the constant, whose only remaining reader was a field nobody read, is gone. The ImageEdit import went with it; it fell out of use when the walk moved into pin_apply.

A near-miss worth recording, since this branch has made a habit of them: my removal script's assertion double-counted (the 10-space pattern is a substring of the 14-space one), so the first edit aborted while the second still deleted the export. The typecheck caught it. The assertion now checks the result — "no reference survived" — rather than a count, which is the third time in this branch that counting has lied to me and reading the outcome has not.

The three from your last review

finding state
F5 guard untested 4 action-level cases; deleting both guards turns all four red
unreadable next read as "end" declined now, including the comma-in-URI shape you diagnosed
page cap truncating silently refuses, cap named, doc says why 20 pages is not obviously enough

One correction stands from that round: a bare rel="next" with no semicolon stays endisNextRel never matches it, so nothing was offered. Tested as end with a comment, since it looks like the unreadable case and isn't.

Also closed itself

The two response strings I flagged before the merge are moot — taking main's mod.ts means pinMessage serves both routes, so there is no wording divergence left for me to have introduced.

The one thing still open

The pin actions above applyPin have no test beyond the guard cases. docker/dev-rig is the honest answer and you said you weren't asking for it here, so I've left it — but I'd rather run it before this merges than after. Say the word and I will; otherwise this is ready as far as I can take it by reading and testing.

`4e9d156` — a last pass over the seam the merge left between the two designs, then this is everything. **427 tests**, ci green, 25 commits on `main`. ### What's in it Neither is a defect; both are this PR's own subject matter left half-done by the adaptation. - **The stack action still declared the patch shape inline** where `PinPatch` names it — a duplicated type in a branch about duplicated code. - **`NO_TAG_MSG` was the pre-merge design showing through.** The actions passed a whole sentence as `detail` so a route could print it, which made sense when the routes did the printing. Now that actions state reasons and `pinMessage` owns the wording for every other failure, that one reads like its siblings — *"db has no tag to move: …"* rather than *"db: …"* — and the constant, whose only remaining reader was a field nobody read, is gone. The `ImageEdit` import went with it; it fell out of use when the walk moved into `pin_apply`. **A near-miss worth recording,** since this branch has made a habit of them: my removal script's assertion double-counted (the 10-space pattern is a substring of the 14-space one), so the first edit aborted while the second still deleted the export. The typecheck caught it. The assertion now checks the *result* — "no reference survived" — rather than a count, which is the third time in this branch that counting has lied to me and reading the outcome has not. ### The three from your last review | finding | state | |---|---| | F5 guard untested | 4 action-level cases; deleting both guards turns all four red | | unreadable `next` read as "end" | declined now, including the comma-in-URI shape you diagnosed | | page cap truncating silently | refuses, cap named, doc says why 20 pages is not obviously enough | One correction stands from that round: a bare `rel="next"` with no semicolon stays `end` — `isNextRel` never matches it, so nothing was offered. Tested as `end` with a comment, since it looks like the unreadable case and isn't. ### Also closed itself The **two response strings** I flagged before the merge are moot — taking main's `mod.ts` means `pinMessage` serves both routes, so there is no wording divergence left for me to have introduced. ### The one thing still open The pin actions above `applyPin` have no test beyond the guard cases. `docker/dev-rig` is the honest answer and you said you weren't asking for it here, so I've left it — but I'd rather run it before this merges than after. Say the word and I will; otherwise this is ready as far as I can take it by reading and testing.
julian approved these changes 2026-08-11 15:28:06 +02:00
julian left a comment

Re-reviewed everything since ca55a1337b4855, d380361, 4e9d156. deno task ci on 4e9d156: exit 0, 427 tests, fmt/lint/typecheck clean. deno task build: exit 0.

All three findings are fixed, and I checked them the way you did rather than by reading the diff.

Both guards deleted from actions.ts:

container-pin refuses a digest-pinned ref ... FAILED
stack-pin refuses a digest-pinned ref ... FAILED
container-pin refuses a bare image id ... FAILED
stack-pin refuses a bare image id ... FAILED
FAILED | 0 passed | 4 failed

if (!raw) return { status: "declined", link } put back to continue:

an offered next that cannot be read is declined, not the end ... FAILED

The page-cap refusal removed from listTags:

a tag list cut off by the page cap is an error, not a short answer ... FAILED

That is the standard the PR body sets, met by the one guard that previously didn't meet it. The action-level tests are the right shape too — rigging fetch and ctx.host.run to throw means a guard that lets the reference through fails on reaching the network instead of passing quietly, and asserting reason rather than a message means the test survives the wording change in this same branch.

Your correction stands and I checked it rather than taking it: isNextRel requires the ;, so a bare rel="next" was never offered anything and end is right. The test says so with the comment explaining why it isn't the case above it, which is the part that stops someone "fixing" it later.

The rest, verified:

  • The page cap can only be reached by the cap. Every other exit in listTags returns — 401-with-no-token, !res.ok, the declined hop — so falling out of the loop with a url still in hand has exactly one cause, and the new branch can't fire on a normal last page. The caps differ (MAX_TAG_PAGES 20, dep-check's MAX_PAGES 60), which is fine: the complaint was that the two callers disagreed about what truncation means, not about the number, and the page sizes differ anyway.
  • NO_TAG_MSG is gone with no reader left — grepped the tree, backend and frontend. PIN_STATUS["no-tag-to-move"] is still 400 and pinMessage now says the whole sentence, so the removal didn't take the status or the wording with it. Dropping detail from the two TaskFailures is safe: pinMessage's detail is still read by image-not-found and compose-rejected.
  • PinPatch was already applyPin's return shape, so hoisting it deletes a duplicate rather than introducing a type.

Two nits inline, neither blocking, both about wording rather than behaviour — the refusals themselves are right.

On docker/dev-rig: run it before you merge. I'm not gating on it and I said last round I wasn't asking for it, but the actions above applyPin are the one path in this branch that writes an operator's files with no test above the guard cases, and you've offered twice. If it turns up nothing, say so and merge.

Approved.

Re-reviewed everything since `ca55a13` — `37b4855`, `d380361`, `4e9d156`. `deno task ci` on `4e9d156`: exit 0, **427 tests**, fmt/lint/typecheck clean. `deno task build`: exit 0. **All three findings are fixed, and I checked them the way you did rather than by reading the diff.** Both guards deleted from `actions.ts`: ``` container-pin refuses a digest-pinned ref ... FAILED stack-pin refuses a digest-pinned ref ... FAILED container-pin refuses a bare image id ... FAILED stack-pin refuses a bare image id ... FAILED FAILED | 0 passed | 4 failed ``` `if (!raw) return { status: "declined", link }` put back to `continue`: ``` an offered next that cannot be read is declined, not the end ... FAILED ``` The page-cap refusal removed from `listTags`: ``` a tag list cut off by the page cap is an error, not a short answer ... FAILED ``` That is the standard the PR body sets, met by the one guard that previously didn't meet it. The action-level tests are the right shape too — rigging `fetch` and `ctx.host.run` to throw means a guard that lets the reference through fails on reaching the network instead of passing quietly, and asserting `reason` rather than a message means the test survives the wording change in this same branch. Your correction stands and I checked it rather than taking it: `isNextRel` requires the `;`, so a bare `rel="next"` was never offered anything and `end` is right. The test says so with the comment explaining why it isn't the case above it, which is the part that stops someone "fixing" it later. The rest, verified: - **The page cap can only be reached by the cap.** Every other exit in `listTags` returns — 401-with-no-token, `!res.ok`, the declined hop — so falling out of the loop with a `url` still in hand has exactly one cause, and the new branch can't fire on a normal last page. The caps differ (`MAX_TAG_PAGES` 20, dep-check's `MAX_PAGES` 60), which is fine: the complaint was that the two callers disagreed about what truncation *means*, not about the number, and the page sizes differ anyway. - **`NO_TAG_MSG` is gone with no reader left** — grepped the tree, backend and frontend. `PIN_STATUS["no-tag-to-move"]` is still 400 and `pinMessage` now says the whole sentence, so the removal didn't take the status or the wording with it. Dropping `detail` from the two `TaskFailure`s is safe: `pinMessage`'s `detail` is still read by `image-not-found` and `compose-rejected`. - **`PinPatch` was already `applyPin`'s return shape**, so hoisting it deletes a duplicate rather than introducing a type. Two nits inline, neither blocking, both about wording rather than behaviour — the refusals themselves are right. **On `docker/dev-rig`:** run it before you merge. I'm not gating on it and I said last round I wasn't asking for it, but the actions above `applyPin` are the one path in this branch that writes an operator's files with no test above the guard cases, and you've offered twice. If it turns up nothing, say so and merge. Approved.
@ -198,0 +200,4 @@
return {
tags: [],
error:
"pagination left the registry's origin; the tag list is partial",
Owner

Nit, non-blocking, same root as the other one: this message, and the log.warn above it, name a cause that may not be the one that happened.

After d380361 a declined here can mean the header was unreadable — <https://…?last=a,b>; rel="next" torn by link.split(",") — and the operator is then told "pagination left the registry's origin", which reads as the registry pointing the bearer token somewhere else. That is a security-flavoured claim about a registry that did nothing of the sort, and it is the one message that would send someone looking at the wrong thing.

Same at tools/dep-check/remote.ts:90 and :137, both OFF_ORIGIN.

Cheapest honest version is to widen the sentence rather than split the status — "could not follow pagination safely; the tag list is partial" — or carry the cause on the declined variant if you want to keep the specific wording where it is true. Refusing is right either way; only the sentence is off, and this branch has spent 25 commits on saying the true thing.

**Nit, non-blocking, same root as the other one:** this message, and the `log.warn` above it, name a cause that may not be the one that happened. After `d380361` a `declined` here can mean the header was unreadable — `<https://…?last=a,b>; rel="next"` torn by `link.split(",")` — and the operator is then told *"pagination left the registry's origin"*, which reads as the registry pointing the bearer token somewhere else. That is a security-flavoured claim about a registry that did nothing of the sort, and it is the one message that would send someone looking at the wrong thing. Same at `tools/dep-check/remote.ts:90` and `:137`, both `OFF_ORIGIN`. Cheapest honest version is to widen the sentence rather than split the status — *"could not follow pagination safely; the tag list is partial"* — or carry the cause on the `declined` variant if you want to keep the specific wording where it is true. Refusing is right either way; only the sentence is off, and this branch has spent 25 commits on saying the true thing.
@ -0,0 +118,4 @@
export type NextPage =
/** fetch this */
| { status: "next"; url: string }
/** one was offered and refused for leaving the origin; answer with an error */
Owner

Nit, non-blocking: this line now describes one of three producers.

declined is returned for an unparseable base (045f977), for an offered-but-unreadable URI (d380361), and for an off-origin next. The doc still says "refused for leaving the origin", which is the one case a reader is least likely to hit — the comma-in-URI shape you just tested reaches this status without any origin being involved.

The union comment is doing real work here (it is the reason the type change was worth taking over the two-liner), so it is worth it saying the general rule: offered, and we cannot prove where it goes or that the list is complete — answer with an error. The three call sites all treat it that way already.

**Nit, non-blocking:** this line now describes one of three producers. `declined` is returned for an unparseable base (`045f977`), for an offered-but-unreadable URI (`d380361`), and for an off-origin next. The doc still says *"refused for leaving the origin"*, which is the one case a reader is least likely to hit — the comma-in-URI shape you just tested reaches this status without any origin being involved. The union comment is doing real work here (it is the reason the type change was worth taking over the two-liner), so it is worth it saying the general rule: *offered, and we cannot prove where it goes or that the list is complete — answer with an error*. The three call sites all treat it that way already.
fix: say which pagination refusal actually happened
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m8s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m10s
ee15e3c1be
`declined` grew three producers — an unparsable base, a Link header offering a
next page with no readable URL, and a next on another origin — while every
message still named the third. So an operator whose registry wrote a header we
could not parse was told "pagination left the registry's origin", which reads
as the registry pointing a bearer token somewhere else. A security-flavoured
accusation about a registry that did nothing of the sort, and the one sentence
that would send someone looking at the wrong thing.

The cause now travels on the variant. Callers splice it rather than choosing a
sentence, so all three sites are accurate without branching, and the union's
doc states the general rule instead of one of its three cases.

Tests assert the CAUSE rather than the sentence — the wording has now changed
twice in this branch — and there is a case asserting that an unreadable header
is not reported as an origin problem, which is the misattribution itself.
Author
Owner

Both nits fixed in ee15e3c, and I ran the rig. It turned up nothing. 428 tests.

The nits — one root, so one fix

You were right that they share a cause: declined grew three producers while every message still named the third. The cause now travels on the variant (why), so the three call sites splice it instead of choosing a sentence, and the union doc states the general rule rather than one of its cases.

Tests assert the cause, not the wording — it has changed twice in this branch now — plus a case asserting an unreadable header is not reported as an origin problem, which is the misattribution itself.

That fix hit the silent-no-op trap a fourth time: deno fmt had wrapped the OFF constant across two lines, so the replacement matched nothing and the test kept asserting the old sentence. It went red with the new message printed in the failure, which is the only reason I saw it.

docker/dev-rig, on ee15e3c

Two things worth reporting before the results. up.sh exited 125 the first time — port 8080 was held by an unrelated container of mine — and something else was answering there, so the md5 check in your README is what stopped me reading a different app's bundle as OpsDeck's. Re-ran on RIG_PORT=8099; served bundle md5 matches the built one exactly.

A real pin, real registry, real git:

{"ok":true,"files":["/srv/stacks/web/compose.yaml"],
 "from":"nginx:1.25-alpine","to":"nginx:1.27-alpine",
 "committed":"chore(api): nginx:1.25-alpine -> nginx:1.27-alpine"}

ce7b3ca chore(api): nginx:1.25-alpine -> nginx:1.27-alpine
 web/compose.yaml | 2 +-

One line changed, both comments above it intact, working tree clean.

ALREADY, on a re-pin to the same tag: {"committed":null,"alreadyWritten":true} — and from still reports 1.25-alpine, because the container has not been recreated. That is the half-applied state the branch is written around, showing up on its own.

F5 against a genuinely digest-pinned container — I rewrote cache to redis@sha256:05a97a…, force-recreated it, and waited for the module to see it:

container-pin → HTTP 400  reason=no-tag-to-move
stack-pin     → HTTP 400  reason=no-tag-to-move
"cache has no tag to move: it is pinned by digest or running an untagged image…"

400, not 500 — which is the PIN_STATUS entry earning its place, since without it the operator would have got a server error for a thing they can fix. Nothing written, nothing committed.

The multi-service path, which is applyPin's general case:

changes: cache 7.2-alpine → 7.4-alpine, api 1.27-alpine → 1.28-alpine
63ab281 chore(web): update 2 pinned images
 web/compose.yaml | 4 ++--

One commit naming both, per-service body, two lines changed, clean tree — the "two services in one file, each hunk against the pre-edit text" case confirmed against real git rather than a temp directory.

So: the actions above applyPin behave. That is a pass, not a proof — the rig exercises the paths I chose to drive — but it is the evidence you asked for and it is negative.

The rig is still up on :8099 if you want to poke at it; docker/dev-rig/down.sh removes it and its volumes.

Both nits fixed in `ee15e3c`, and I ran the rig. **It turned up nothing.** 428 tests. ### The nits — one root, so one fix You were right that they share a cause: `declined` grew three producers while every message still named the third. The cause now travels on the variant (`why`), so the three call sites splice it instead of choosing a sentence, and the union doc states the general rule rather than one of its cases. Tests assert the **cause**, not the wording — it has changed twice in this branch now — plus a case asserting an unreadable header is *not* reported as an origin problem, which is the misattribution itself. That fix hit the silent-no-op trap a fourth time: `deno fmt` had wrapped the `OFF` constant across two lines, so the replacement matched nothing and the test kept asserting the old sentence. It went red with the new message printed in the failure, which is the only reason I saw it. ### `docker/dev-rig`, on `ee15e3c` Two things worth reporting before the results. `up.sh` exited **125** the first time — port 8080 was held by an unrelated container of mine — and something else was answering there, so the md5 check in your README is what stopped me reading a different app's bundle as OpsDeck's. Re-ran on `RIG_PORT=8099`; served bundle md5 matches the built one exactly. **A real pin, real registry, real git:** ``` {"ok":true,"files":["/srv/stacks/web/compose.yaml"], "from":"nginx:1.25-alpine","to":"nginx:1.27-alpine", "committed":"chore(api): nginx:1.25-alpine -> nginx:1.27-alpine"} ce7b3ca chore(api): nginx:1.25-alpine -> nginx:1.27-alpine web/compose.yaml | 2 +- ``` One line changed, both comments above it intact, working tree clean. **ALREADY, on a re-pin to the same tag:** `{"committed":null,"alreadyWritten":true}` — and `from` still reports `1.25-alpine`, because the container has not been recreated. That is the half-applied state the branch is written around, showing up on its own. **F5 against a genuinely digest-pinned container** — I rewrote `cache` to `redis@sha256:05a97a…`, force-recreated it, and waited for the module to see it: ``` container-pin → HTTP 400 reason=no-tag-to-move stack-pin → HTTP 400 reason=no-tag-to-move "cache has no tag to move: it is pinned by digest or running an untagged image…" ``` **400, not 500** — which is the `PIN_STATUS` entry earning its place, since without it the operator would have got a server error for a thing they can fix. Nothing written, nothing committed. **The multi-service path**, which is `applyPin`'s general case: ``` changes: cache 7.2-alpine → 7.4-alpine, api 1.27-alpine → 1.28-alpine 63ab281 chore(web): update 2 pinned images web/compose.yaml | 4 ++-- ``` One commit naming both, per-service body, two lines changed, clean tree — the "two services in one file, each hunk against the pre-edit text" case confirmed against real git rather than a temp directory. So: the actions above `applyPin` behave. That is a pass, not a proof — the rig exercises the paths I chose to drive — but it is the evidence you asked for and it is negative. The rig is still up on `:8099` if you want to poke at it; `docker/dev-rig/down.sh` removes it and its volumes.
chore(docker): remove what the task-runs refactor left behind
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m10s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m11s
93cdf225c4
Neither is from this branch — both are #21's leftovers, found while auditing
what the merge had orphaned — but this branch is the one holding the area open
and they are the exact defects it has spent its length removing.

restoreNote had no reader once the routes became actions: the actions log their
compensation through the run rather than splicing a sentence onto an HTTP
error. Its two test assertions go with it; the test that mattered there is that
run() REPORTS the file it could not put back, and that one stays.

The doubled JSDoc above pinResponse was a rewritten comment whose predecessor
was never deleted, so two blocks described one declaration and the first one
was wrong about where the sentence lives. Merged into one, keeping the sentence
each got right. Same defect this stack already fixed once in f8c59fc.
Author
Owner

93cdf22 — the last of it: two dead artifacts removed, both the leftovers you spotted. 427 tests, ci green. Re-requesting review since this is a fresh commit on top of your approval, not because anything is in question.

What went

Neither is from this branch. You called both out as #21's and "not yours", which is true — but this branch is the one holding the area open, and they are precisely the defects it has spent its length removing.

  • restoreNote had no reader left once the routes became actions: an action logs its compensation through the run instead of splicing a sentence onto an HTTP error. Its two assertions went with it. The one that mattered there — that run() reports the file it could not put back — stays, because that is the behaviour, not the formatting.
  • The doubled JSDoc above pinResponse was a rewritten comment whose predecessor was never deleted, so two blocks described one declaration and the older one was wrong about where the sentence lives. Merged, keeping what each got right. Same defect this stack already fixed once in f8c59fc, which is why it was worth not walking past.

restoreNote's removal is the one to sanity-check in review: it was exported, so "no readers" is a claim about the whole tree rather than one file. Grepped packages and tools; the only hits were its definition and its own tests.

Housekeeping outside the diff

The rig is down and its two volumes are gone (opsdeck:rig / rig-base images kept — they are the expensive part to rebuild). Merged branches are cleaned up locally and on the remote: build/dev-rig, feat/core-tasks, feat/core-ui-for-docker, feat/docker-jobs-and-status, feat/docker-pinned-versions, integration/docker-overhaul, plus refactor/dedup-shared-code now that #24 is closed and its commits are here by fast-forward. Each was verified an ancestor of main first, so the commits live on and only the refs are gone.

Left alone on purpose: fix/issue-20-windows-test-paths — that is yours and #20 is open — feat/ui-access-denied-and-rate, which is live work elsewhere, and ci/dependency-update-issues, which is merged but yours to retire.

That is everything I know of. Ready to merge on your say-so; I have not merged it myself because landing on main runs the deploy workflow against the production host.

`93cdf22` — the last of it: two dead artifacts removed, both the leftovers you spotted. **427 tests**, ci green. Re-requesting review since this is a fresh commit on top of your approval, not because anything is in question. ### What went Neither is from this branch. You called both out as #21's and "not yours", which is true — but this branch is the one holding the area open, and they are precisely the defects it has spent its length removing. - **`restoreNote`** had no reader left once the routes became actions: an action logs its compensation through the run instead of splicing a sentence onto an HTTP error. Its two assertions went with it. The one that mattered there — that `run()` *reports* the file it could not put back — stays, because that is the behaviour, not the formatting. - **The doubled JSDoc above `pinResponse`** was a rewritten comment whose predecessor was never deleted, so two blocks described one declaration and the older one was wrong about where the sentence lives. Merged, keeping what each got right. Same defect this stack already fixed once in `f8c59fc`, which is why it was worth not walking past. `restoreNote`'s removal is the one to sanity-check in review: it was exported, so "no readers" is a claim about the whole tree rather than one file. Grepped `packages` and `tools`; the only hits were its definition and its own tests. ### Housekeeping outside the diff The rig is down and its two volumes are gone (`opsdeck:rig` / `rig-base` images kept — they are the expensive part to rebuild). Merged branches are cleaned up locally and on the remote: `build/dev-rig`, `feat/core-tasks`, `feat/core-ui-for-docker`, `feat/docker-jobs-and-status`, `feat/docker-pinned-versions`, `integration/docker-overhaul`, plus `refactor/dedup-shared-code` now that #24 is closed and its commits are here by fast-forward. Each was verified an ancestor of `main` first, so the commits live on and only the refs are gone. **Left alone on purpose:** `fix/issue-20-windows-test-paths` — that is yours and #20 is open — `feat/ui-access-denied-and-rate`, which is live work elsewhere, and `ci/dependency-update-issues`, which is merged but yours to retire. That is everything I know of. Ready to merge on your say-so; I have not merged it myself because landing on `main` runs the deploy workflow against the production host.
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
OpsDeck/core!19
No description provided.