docker: deferred findings from the #10 review (digest refs, registry headers, chooser bands, small cleanups) #12

New issue

Closed

opened 2026-08-10 15:27:35 +02:00 by thisilike · 1 comment

thisilike commented

2026-08-10 15:27:35 +02:00

Owner

F5 — digest-pinned refs mangle instead of being refused. retag/tagOf treat the : inside @sha256:… as a tag separator, so pinning a digest-pinned service produces postgres@sha256:16.4, which fetchRemoteDigest then reports as no such image in the registry. Fails safe, diagnoses wrong. Refuse digest-pinned refs by name at the top of both pin routes: "this service is pinned by digest; move it to a tag first".

F6 — a missing docker-content-digest header reads as "no such image". updates.ts returns {digest: null, error: null} on a 200 without the header, and the pin route only tests !digest. Proxying registries that omit the header on HEAD are real. Distinguish the two, or fall back to a GET manifest request.

Chooser bands inconsistent for four-component schemes. listUpgrades().minor admits shared === 1 while withinScope("minor") fixes two components, so the offered list can contain versions the minor jump will never choose. Align the band arithmetic with withinScope. UI applies exact tags, so nothing writes wrong meanwhile.

Small:

commit.ts: rootOf runs once per file for grouping and again inside commitOne — cache per directory, halves the git rev-parse count.
ConfirmDialog: the focus effect re-runs when busy flips false, a focus grab on re-render rather than mount. Harmless while the dialog is {#key}ed; worth splitting the deps if that ever changes.
The confirm-queue state machine in StacksPage.svelte is the one intricate piece with no direct test — extracting it to a .svelte.ts module would make it testable. This is the largest item here and probably its own PR.

Not in scope: action SHAs / Dockerfile base digests / known_hosts pinning — owned by #11 and its pinning issue.

Follow-ups agreed in the fourth review pass on #10 — none loses data or writes anything wrong, which is why they were deferred rather than fixed there. Sources: the review at #10 and the fix commits `b92f637`/`ef7944c`. **F5 — digest-pinned refs mangle instead of being refused.** `retag`/`tagOf` treat the `:` inside `@sha256:…` as a tag separator, so pinning a digest-pinned service produces `postgres@sha256:16.4`, which `fetchRemoteDigest` then reports as `no such image in the registry`. Fails safe, diagnoses wrong. Refuse digest-pinned refs by name at the top of both pin routes: "this service is pinned by digest; move it to a tag first". **F6 — a missing `docker-content-digest` header reads as "no such image".** `updates.ts` returns `{digest: null, error: null}` on a 200 without the header, and the pin route only tests `!digest`. Proxying registries that omit the header on HEAD are real. Distinguish the two, or fall back to a GET manifest request. **Chooser bands inconsistent for four-component schemes.** `listUpgrades().minor` admits `shared === 1` while `withinScope("minor")` fixes two components, so the offered list can contain versions the minor jump will never choose. Align the band arithmetic with `withinScope`. UI applies exact tags, so nothing writes wrong meanwhile. **Small:** - `commit.ts`: `rootOf` runs once per file for grouping and again inside `commitOne` — cache per directory, halves the `git rev-parse` count. - `ConfirmDialog`: the focus effect re-runs when `busy` flips false, a focus grab on re-render rather than mount. Harmless while the dialog is `{#key}`ed; worth splitting the deps if that ever changes. - The confirm-queue state machine in `StacksPage.svelte` is the one intricate piece with no direct test — extracting it to a `.svelte.ts` module would make it testable. This is the largest item here and probably its own PR. Not in scope: action SHAs / Dockerfile base digests / `known_hosts` pinning — owned by #11 and its pinning issue.

thisilike referenced this issue

2026-08-10 15:33:13 +02:00

feat(docker): the docker overhaul, integrated #10

thisilike referenced this issue

2026-08-10 21:24:09 +02:00

feat(docker): the docker overhaul, integrated #10

thisilike commented

2026-08-10 21:24:31 +02:00

Author

Owner

Three more from the fifth review pass on #10, deferred here rather than growing that PR further.

Windows: four tests fail where the review actually happens. deno task test on a Windows checkout is 299 passed / 4 failed — the three commit_test.ts git probes and compose_update_test.ts's "a legal ../ path is accepted". Path shape, not logic: rootOf returns C:/Users/… while the file paths carry \, so file.startsWith(root + "/") misses, the diff --git header keeps an absolute path and git apply --cached rejects it; and makeTempDir() returns C:\…, which fails startsWith("/"). They run rather than skip because git is present, so they are red rather than ignored. Guard with Deno.build.os === "windows" alongside the existing ignore: !hasGit, or normalise separators. These are tests added in #10, and the "251 local / 245 + 4 ignored" figure only ever held because every run was POSIX.

checkUpdates can starve its tail indefinitely. Separate from the budget bounding starts rather than work: the worker pool walks [...images] in fixed state.stacks order every pass, with no rotation and no resume cursor. If the leading images sit on a slow or unreachable registry and burn the budget, everything past the cutoff gets previous ?? "ran out of time" on every pass, forever — a security update on a tail image would never surface. The serial loop this replaced was slower but always reached the end. Rotate the start offset per pass, or order by least-recently-checked.

Duplication worth one dedup pass:

two registry v2 clients — tools/dep-check/remote.ts re-implements updates.ts's parse/bearer/paginate, and the copy is missing the same-host token guard listTags has;
two tag-version parsers — dep-check/version.ts vs tags.ts, PRERELEASE lists already drifted by one word;
jobs.ts merge() still hand-mirrors update_feed.svelte.ts's carry-forward — the field drift that had to be patched in lockstep once already;
/container/:id/pin and /stack/:project/pin carry ~190 lines of the same read → rewrite → validate → restore → commit pipeline, already subtly diverged.

None of these loses data or writes anything wrong, which is why they are here. Credit to @julian's fifth pass on #10 for all three.

Three more from the fifth review pass on #10, deferred here rather than growing that PR further. **Windows: four tests fail where the review actually happens.** `deno task test` on a Windows checkout is 299 passed / **4 failed** — the three `commit_test.ts` git probes and `compose_update_test.ts`'s "a legal `../` path is accepted". Path shape, not logic: `rootOf` returns `C:/Users/…` while the file paths carry `\`, so `file.startsWith(root + "/")` misses, the `diff --git` header keeps an absolute path and `git apply --cached` rejects it; and `makeTempDir()` returns `C:\…`, which fails `startsWith("/")`. They run rather than skip because git is present, so they are red rather than ignored. Guard with `Deno.build.os === "windows"` alongside the existing `ignore: !hasGit`, or normalise separators. These are tests added in #10, and the "251 local / 245 + 4 ignored" figure only ever held because every run was POSIX. **`checkUpdates` can starve its tail indefinitely.** Separate from the budget bounding starts rather than work: the worker pool walks `[...images]` in fixed `state.stacks` order every pass, with no rotation and no resume cursor. If the leading images sit on a slow or unreachable registry and burn the budget, everything past the cutoff gets `previous ?? "ran out of time"` on *every* pass, forever — a security update on a tail image would never surface. The serial loop this replaced was slower but always reached the end. Rotate the start offset per pass, or order by least-recently-checked. **Duplication worth one dedup pass:** - two registry v2 clients — `tools/dep-check/remote.ts` re-implements `updates.ts`'s parse/bearer/paginate, and the copy is missing the same-host token guard `listTags` has; - two tag-version parsers — `dep-check/version.ts` vs `tags.ts`, PRERELEASE lists already drifted by one word; - `jobs.ts` `merge()` still hand-mirrors `update_feed.svelte.ts`'s carry-forward — the field drift that had to be patched in lockstep once already; - `/container/:id/pin` and `/stack/:project/pin` carry ~190 lines of the same read → rewrite → validate → restore → commit pipeline, already subtly diverged. None of these loses data or writes anything wrong, which is why they are here. Credit to @julian's fifth pass on #10 for all three.

thisilike referenced this issue from a pull request that will close it,

2026-08-10 21:45:39 +02:00

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

thisilike referenced this issue

2026-08-10 21:53:30 +02:00

docker: four tests fail on Windows — path separators, not logic #20

thisilike referenced this issue

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

refactor: the structural dedup deferred from #12 #24

thisilike closed this issue

2026-08-11 16:51:54 +02:00

thisilike referenced this issue from a commit