docker: the deferred findings from #12, and the dedup behind them #19
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/issue-12-followups"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #12. 25 commits on
main, including a merge of the task-runs refactor (#21) and the adaptation onto it.deno task cigreen: fmt, lint, typecheck, 427 tests.deno task buildclean.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/retagsplit on the last colon, which inpostgres@sha256:2a1f…belongs tosha256:—retagbuiltpostgres@sha256:16.4and the registry got blamed for a missing image nobody asked for. Covers the baresha256: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-digestread 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.
nextPagereturnedstring | nulland null meant both "no next page" and "one that left the origin"; every caller read the first. Fixed at the type —NextPageis{next|declined|end}, so collapsing the branch is a compile error. Then the same claim, three more times: an unparseable base, an offered-but-unreadablenext(including a comma inside the URI, whichlink.split(",")tears in two), andlistTagsexhausting 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-checksent its bearer token wherever theLinkheader pointed.updates.tshad refused cross-host pagination for exactly this reason; the copy never had the guard. Both compare origin, not hostname — names alone accepthttp://on the registry's own name and put the token on the wire in clear text.relread as a token list.rel="nextish"andrel=next-archivematched. Anchoring the end would have traded that forrel="next"; type="…"andrel="next last", both legal, both silently stopping at page one.Chooser bands vs
withinScope(1.2.3.4offered a minor upgrade minor would never pick);checkUpdatestail starvation (images behind a slow registry skipped forever);rootOfcalled twice per file;ConfirmDialogfocus grab; the prerelease word lists differing by one word each way.B — the dedup
packages/registry: reference parsing, bearer handshake, Link pagination. The origin guard now covers dep-check's forge pagination too. Verified by runningdep-check --dry-runagainst Docker Hub and two forges.compare()pads with0in dep-check and-1in the module and both are right; merging them breaks one silently, so the shared module says so. Intended behaviour change: the module accepted onlyvas a prefix, sobin-2.5.6andbookworm-20240110never 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.mergeProgressin the SDK saying it exists so no client can disagree — then left an identical copy inserver/src/tasks/service.tsand the docker frontend still folding by hand. Both collapsed onto the SDK's. Matters becausesync()re-hydrates every 15 s, so both readings appear alternately in one table.applyPin, re-aimed atactions.tsafter #21 moved the work there — the duplication survived that refactor intact. Refusals are returned as reasons and the action turns them intoTaskFailure, so the helper states the fact and the action owns the presentation.What review changed
Julian found four things worth the round trip: the
nextPagereturn 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-nextbranch, 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
relmatch dropsrel="next last", andif (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
applyPinhave no test beyond the guard cases.docker/dev-rigis the honest answer; happy to run it before merge.Split out: #20, the four Windows test failures, assigned to @julian.
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.WIP: docker: deferred findings from the #10 reviewto docker: deferred findings from the #10 reviewdocker: deferred findings from the #10 reviewto WIP: docker: deferred findings from the #10 reviewWIP: docker: deferred findings from the #10 reviewto docker: deferred findings from the #10 reviewAll 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 survivesparseImageRef, 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()),A 200 with no
docker-content-digestheader 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 emptyarrayBuffer()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));/;\s*rel\s*=\s*"?next"?/ihas no boundary afternext, so it also matchesrel="nextish"or a hypotheticalrel=next-archive. Anchor the end:/;\s*rel\s*=\s*"?next"?\s*$/ion the trimmed part, or match the token explicitly ("?next"?(?=\s*(;|$))). Same regex was added to updates.ts — fix both.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:
e3b0c442…b855is 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 manifestandregistry 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:
\s*$; rel="next"; rel=next; rel="nextish"; rel=next-archive; rel="next"; type="application/json"; rel="next last"Anchoring the end drops a Link part that carries a further parameter after
rel— ordinary, and something registries do send. And both suggestions droprel="next last", becauserelis 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.tsand I copied it intodep-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 cigreen — 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.docker: deferred findings from the #10 reviewto docker: the deferred findings from #12, and the dedup behind themRe-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 cirun locally on17d7a6b: green, 344 tests.Both previous findings are resolved, and the pushback on the
relanchoring is right:relis a space-separated list andrel="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.tsimports nothing, so the frontend bundle constraint holds.mergeProgressis the two folds it replaces — the frontend previously wrotephase/total/current/fractionas explicitundefinedwhere this omits the keys, which reads identically at every consumer.packages/registry:parseImageRefis unchanged from the module's copy (diffed againstb11a279), so nothing moved behaviour on the way into the package — the port survives,docker.iostill maps toregistry-1.docker.io, and a baresha256:…still reachesdigestRefrather than this. The origin tests coverhttp://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 againstrestore.original(). The stricter re-read path is unreachable from the container route (oneapplyPincall, each file visited once, sorestore.has()is always false there) and the stack route's call sits inside thetrythat 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;A call for you rather than a defect: adopting the shared parser widens this module by more than
bin-2.5.6.plausiblePrefixaccepts any prefix ending in-or_, where the old regex here allowed onlyv— so date-stamped distro tags now parse as versions in the docker module:So a container on
debian:bookworm-20240110gains anewestTagand a major-band option it never had, andcheckUpdatesstarts 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 runningscope=majorwould 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);nextPagereturns 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
17d7a6bwith a stubbed fetch, page one carrying<http://registry-1.docker.io/…>; rel="next":Before this branch dep-check followed that link with the token attached, which
4e2fa09correctly 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.isNextRelis already exported for exactly this, andupdates.ts:188already uses it to tell the two apart:Same two lines in
tagsFromHostat :124. A forge is unlikely to send a cross-originnext, 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 | nullcannot express "declined". Something like{ url } | { declined: string } | nullwould 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.Both addressed in
27bd4ceandcb98ad1. 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:
Worse than the diff shows: I hit this same conflation in
updates.tsduring 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.
NextPageis now{next|declined|end}, so the URL is unreachable without saying what happens to the other two. Deleting thedeclinedbranch is now a compile error — the mutation check couldn't even run until I made it type-valid first:Forced type-valid (
declined→ return as end), both layers go red:Three call sites now refuse: both dep-check loops fail with the same wording as the page-cap truncation, and
listTagsreturns atagsError, whichcachedTagsalready declines to cache so the next pass retries. Also renamed apagethat 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.
newerTagreaches exactly two places — a badge and a button inStacksPagetitled "edits the compose file". The scheduler only callsrefreshStacks/refreshUpdates; there is no auto-pin path, soscope=majorwidens 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.Rebased onto the task-runs world.
ceff745merges main,216043dandca55a13adapt. 421 tests,deno task cigreen,deno task buildclean.#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
runNow("…-pin", …)shimsmod.tsjobs.ts+ itsmerge()docker/shared/progress.tssdk/clientalready exportsmergeProgressfor exactly this reasonTwo 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
digestRefsurvived as a function nobody called — andpin_apply, left with no importer. Both restored.The duplication survived #21 — it just moved
actions.tsstill carries both copies of the pin walk, so the extraction was still worth having.applyPinnow speaks the actions' vocabulary instead of HTTP's: refusals come back as reasons (image-from-variablewith the file,no-image-line) and the action turns them into theTaskFailureit 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, andpinMessage, 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
mergeProgressto the SDK saying it exists "so no client can disagree with another about what a keyed entity means" — then left an identical copy inserver/src/tasks/service.tsand 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.mergeProgressis generic in the progress type now, so a narrowed one survives the fold — the docker frontend needsComposeProgressback, notTaskProgress. 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/clientrather than inlining it, so the singleton contract holds.Unchanged
Part A survived intact —
pin.ts,updates.ts,tags.ts,commit.ts,packages/registry,ConfirmDialogand dep-check were barely touched by #21.Still open from before: the actions above
applyPinhave no test (docker/dev-rigis what would cover them), andlistTags's 20-page cap still truncates silently.Re-reviewed everything since
17d7a6b— the two fixes from the last round,045f977, and the whole post-merge adaptation.deno task cionca55a13: green, 421 tests.deno task build: exit 0, and the docker bundle emitsimport{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
declinedback intoendat both dep-check call sites and inupdates.ts. Three tests go red across both layers:You were right to take the type over the two-line version.
NextPagereads as the thing it is now, and thedeclined-for-an-unparseable-base branch in045f977is 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-slimis still not a version, which is the half that would have been easy to lose.Post-merge adaptation, verified rather than skimmed:
digestRefis reachable again atactions.ts:334and:474,pin_applyhas an importer.PIN_STATUS["no-tag-to-move"]andpinMessagereach the samepinResponseboth routes use, so the 400 and the sentence land on both.applyPinre-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.lengthto "already, therefore" is correct:applyPinreturnsno-image-linewhen!patches.length && !already, sook: truewith no patches can only be the already-written case. The stack loop'schangespush is still per-service and still gated on a real write.mergeProgresscopy really was byte-identical apart from comments, and the generic is what letsComposeProgresssurvive the fold rather than being widened toTaskProgressat the door.restoreNote, which is dead onmaintoo (7a9b745), so not yours. Same for the doubled JSDoc abovepinResponse.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)) {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:
So F5 can be deleted from both actions and nothing goes red. That is exactly how #21's merge orphaned
digestRefin 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.tscoversdigestRefandretagas 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-moveforpostgres@sha256:2a1f…, and one for the baresha256:9f3a…shape that has no@to catch it by.docker/dev-rigis the heavier answer to the untested routes aboveapplyPinand 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 };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-checkalready made this exact call for this exact reason (MAX_PAGES,TRUNCATED), the two now sharepackages/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/debianat 2976 tags fits inside 20 pages only while the registry honoursn=1000; a registry that caps page size the way Forgejo capslimit=100at 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.cachedTagsalready declines to cache atagsError, so a retry costs a pass rather than a stuck verdict — the same reason thedeclinedpath 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;if (!raw) continuefalls through toreturn { status: "end" }— anextthat was offered and could not be read, reported as "none offered: the listing really is complete". Same claim045f977just refused to make for an unparseable base, one branch away.Probed against this commit:
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(norel) andb>; 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 theLinkheader is registry-controlled, the loop consuming it treatsendas 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.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.All three taken —
37b4855andd380361. 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.defineand invoked directly, withfetchandctx.host.runrigged to throw, so a guard that lets the reference through fails on reaching the network rather than passing quietly. Deleting both guards: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'slink.split(",")tearing one part in two so thereland the URI land in different halves. That case is tested explicitly.One correction to the repro: a bare
rel="next"with no preceding semicolon staysend, and your one-liner doesn't change it either —isNextRelrequires the;, so it never matches and nothing was ever offered. I asserteddeclinedthere first and the test caught me. It's now tested asendwith 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/debianat 2976 tags only fits in 20 pages whilen=1000is honoured, and a registry capping page size the way Forgejo capslimit=100at 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
applyPinhave no test beyond the guard cases —docker/dev-rigremains the honest answer, and I'll run it if you want it before merge rather than after.4e9d156— a last pass over the seam the merge left between the two designs, then this is everything. 427 tests, ci green, 25 commits onmain.What's in it
Neither is a defect; both are this PR's own subject matter left half-done by the adaptation.
PinPatchnames it — a duplicated type in a branch about duplicated code.NO_TAG_MSGwas the pre-merge design showing through. The actions passed a whole sentence asdetailso a route could print it, which made sense when the routes did the printing. Now that actions state reasons andpinMessageowns 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. TheImageEditimport went with it; it fell out of use when the walk moved intopin_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
nextread as "end"One correction stands from that round: a bare
rel="next"with no semicolon staysend—isNextRelnever matches it, so nothing was offered. Tested asendwith 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.tsmeanspinMessageserves both routes, so there is no wording divergence left for me to have introduced.The one thing still open
The pin actions above
applyPinhave no test beyond the guard cases.docker/dev-rigis 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.Re-reviewed everything since
ca55a13—37b4855,d380361,4e9d156.deno task cion4e9d156: 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:if (!raw) return { status: "declined", link }put back tocontinue:The page-cap refusal removed from
listTags: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
fetchandctx.host.runto throw means a guard that lets the reference through fails on reaching the network instead of passing quietly, and assertingreasonrather 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:
isNextRelrequires the;, so a barerel="next"was never offered anything andendis 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:
listTagsreturns — 401-with-no-token,!res.ok, the declined hop — so falling out of the loop with aurlstill in hand has exactly one cause, and the new branch can't fire on a normal last page. The caps differ (MAX_TAG_PAGES20, dep-check'sMAX_PAGES60), 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_MSGis gone with no reader left — grepped the tree, backend and frontend.PIN_STATUS["no-tag-to-move"]is still 400 andpinMessagenow says the whole sentence, so the removal didn't take the status or the wording with it. Droppingdetailfrom the twoTaskFailures is safe:pinMessage'sdetailis still read byimage-not-foundandcompose-rejected.PinPatchwas alreadyapplyPin'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 aboveapplyPinare 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",Nit, non-blocking, same root as the other one: this message, and the
log.warnabove it, name a cause that may not be the one that happened.After
d380361adeclinedhere can mean the header was unreadable —<https://…?last=a,b>; rel="next"torn bylink.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:90and:137, bothOFF_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
declinedvariant 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 */Nit, non-blocking: this line now describes one of three producers.
declinedis 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.
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:
declinedgrew 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 fmthad wrapped theOFFconstant 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, onee15e3cTwo things worth reporting before the results.
up.shexited 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 onRIG_PORT=8099; served bundle md5 matches the built one exactly.A real pin, real registry, real git:
One line changed, both comments above it intact, working tree clean.
ALREADY, on a re-pin to the same tag:
{"committed":null,"alreadyWritten":true}— andfromstill reports1.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
cachetoredis@sha256:05a97a…, force-recreated it, and waited for the module to see it:400, not 500 — which is the
PIN_STATUSentry 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: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
applyPinbehave. 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
:8099if you want to poke at it;docker/dev-rig/down.shremoves it and its volumes.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.
restoreNotehad 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 — thatrun()reports the file it could not put back — stays, because that is the behaviour, not the formatting.pinResponsewas 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 inf8c59fc, 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. Greppedpackagesandtools; 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-baseimages 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, plusrefactor/dedup-shared-codenow that #24 is closed and its commits are here by fast-forward. Each was verified an ancestor ofmainfirst, 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, andci/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
mainruns the deploy workflow against the production host.