feat(docker): the docker overhaul, integrated #10
Loading…
Reference in a new issue
No description provided.
Delete branch "integration/docker-overhaul"
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?
Supersedes #6, #7, #8 and #9: the same four branches, merged, with the collisions between them resolved and the tree green for the first time across the whole stack.
The stack was four PRs deep (
dev-rig→core-ui→jobs-and-status→pinned-versions). Merging them in place would have replayed the same conflicts one PR at a time and lost the resolutions, so they are merged here once, deliberately.What the merges had to decide
Several of the same bugs were fixed independently on more than one branch.
busy, so a dialog mounted while the previous answer is still in flight does not end up with nothing focused; and Tab goes to the safe choice when focus is on neither button rather than falling through to the dangerous one.StacksPage,ContainerPage,StackDetailPageandOverviewWidgetreport a failed load instead of rendering their skeleton forever, and the two detail pages no longer rethrow inside.catchinto a dangling promise.deno.jsonis the union of all three lines: the vite range named in the task itself,--allow-sys=hostnameand the docker socket from dev-rig, and/host/rootfrom the pin branch, which the pin route writes through.checkkeeps the module backends, andlintdefers to it.mod.tsgetsContainerInfoandcomposeConfigArgsback. They were removed as unused on a branch that had no pin route; the pin route is what uses them.Verification
Lint reached zero by pinning the remaining unversioned
jsr:@std/assertimports through the workspace, dropping an import inapp.tswhose only use had been replaced, dropping adeno-lint-ignoreinmod.tsthat no longer suppressed anything, and switchingno-slow-typesoff for@opsdeck/sdkwith the reason recorded — it is a JSR publishing rule, the package is consumed through the import map, and the honoContextVariableMapaugmentation it objects to is the point.Deployed
Running on the server now as
opsdeck/core:local-ab960d2fb7da, built on the box from this commit. Clean start:env=production, OIDC discovery against Authentik, all three pinned external modules loaded,host exec ready strategy=chroot pidNamespace=true,compose updater available=true strategy=chroot.Rollback is a one-line tag change back to
local-4dceed7ab4b9; that image is retained and the previous compose file is at/tmp/opsdeck-compose.bak.ymlon the host.Answering a depends_on prompt started the update without waiting for it and presented the next queued prompt in the same microtask. But the server reserves the whole compose project for the length of an update, and that check runs before the dependents check — so the second acknowledgement came back 409 "update already running", with no needsConfirmation field to route it. The human read a destructive warning, accepted it, and got a raw HTTP 409 in the row's error line: the exact outcome the queue was added to prevent. Queue entries now carry their project, and only a prompt whose project is free is shown. The ones waiting say so in their row instead of going quiet, and the dialog holds a busy state while the update it just acknowledged is being started. The same reservation explains two more. stackLocked only tracked stack updates although /container/:id/update takes the identical lock, so sibling buttons stayed enabled and 409'd; it becomes projectBusy(), which also counts per-container updates. And subscribeLive("docker:update") registered no onResync, so a dropped channel — routine, the socket closes after a minute hidden — lost the terminal event that is the only thing clearing the in-flight flags, wedging every button in the project until a reload.`deno lint` reported 23 problems and nothing enforced it, so they had accumulated. Seven of them were one function: safeUpgradeWebSocket, which took `any` for the handler, the context, the options, the events object and every callback argument, and was then cast to `any` again at the call site behind a deno-lint-ignore. It is typed now: hono's own WSEvents for the shape it must produce, the SDK's ModuleContext["upgradeWebSocket"] for the contract it fills, and a single documented assertion where those two meet — which is the only place they genuinely differ, because the SDK describes the callbacks in its own terms so modules never import hono. The rest: three require-await middlewares now `return await next()`, which is what makes them honestly async (the MiddlewareHandler contract needs the promise, so dropping `async` was not an option); a regex with two literal spaces says ` {2}`; DuckDBConnection is a type-only import; and an unused type import is gone.Every hand-written page fetched its data as `apiFetch(...).then(...)` with no rejection path. A 500, a dropped connection, a restarting server — `data` stays null, the `{#if !data}` branch keeps rendering its skeleton for as long as the page is open, and the only trace is an unhandled rejection in the console. Nothing on screen says anything is wrong, and nothing ever will. Three pages were worse than having no catch: ContainerPage, StackDetailPage and PoolDetailPage caught, handled 404 by navigating away, then `throw e` for everything else — a rethrow inside .catch is a rejected promise nobody awaits, so it produced the same dead skeleton plus a warning. The schema renderer already did this correctly (SchemaPage catches and renders EmptyState with the error as its hint), so the mobile path degraded properly while the desktop pages did not. Pages now match it. The three overview widgets get one line instead: they render into a fixed ~260px slot on the home page where an EmptyState does not fit. StoragePage reports its two fetches separately, because smartctl can be unavailable while the mount table is perfectly fine. Each of these is nine or so lines of the same shape, which wants a shared helper — but the .then bodies do page-specific work (StacksPage prunes its confirmation queue, ContainerPage drops a stale prompt), so a generic loader needs callbacks, and there is no browser test framework here to prove that refactor did not change behaviour. Left as the local pattern SchemaPage already established.The focus effect read `open`, `variant` and the two element bindings, but not `busy` — and both buttons are `disabled={busy}`, so .focus() on them does nothing. It never re-ran when `busy` cleared either. That is reachable in the queued-confirmation flow this branch added: StacksPage renders DependentsConfirm inside `{#key confirmUpdate?.id}` with `busy={confirming}`, so when the answer to one prompt is still in flight and the next queued prompt becomes current, the new dialog is created disabled, focuses nothing, and stays that way. Escape still worked, Enter did nothing, and the Tab trap's swap sent the first Tab to "Update anyway" — the exact invariant ("focus lands on the safe choice for destructive actions") that justified removing the Enter handler in the first place. The focus effect is now separate from the keydown effect and tracks `busy`, so it fires as soon as the buttons can take focus; splitting them also keeps the focus-restore cleanup from running on every re-run. The Tab trap no longer assumes focus is on one of the two buttons — when it is on neither it goes to the safe one instead of falling through to the dangerous one.Reviewed the full 142-file diff, branch checked out and verified locally:
deno task test→ 241 passed, 0 failed; the ConfirmDialog union behaves as described; no auth bypass or injection found; SSE cleanup is correct. The merge decisions in the description match what is actually in the tree.Requesting changes anyway — the items below are all real, several in the new pin/update path that writes host files. Nothing here invalidates the approach; most are small.
Blocking (in rough order of severity):
hostPath()sanitization is a..substring check and nothing else — symlinks are not rejected, and the patch path incommit.tsbypasses the check entirely (inline comments)..catch(() => {})swallowing background refresh failures (inline comment).DependentsConfirm'sbusyprop is dead — no parent passes it, so the dialog stays clickable while an update is in flight (inline comment).Minor but requested since we are here: pull-rate window not rebased on phase change, summary
statefield always"active", CI actions pinned by major tag only, dev-rig stub/secret hardening, undocumented version-compare semantics (inline comments).One item with no anchorable diff line:
docker/Dockerfilebases (denoland/deno:2.5.6,debian:bookworm-slim) are tag-pinned but not digest-pinned. Same argument as the action SHAs — a moved tag changes the image silently. Pin@sha256:digests alongside the tags.@ -12,0 +23,4 @@image: denoland/deno:2.5.6steps:- name: Checkout Codeuses: actions/checkout@v6actions/checkout@v6— and the docker/* actions below in the build job — are pinned by major tag only. Tags can be moved; on a workflow that ends in an SSH deploy to the production host, pin all actions to full commit SHAs (uses: actions/checkout@<sha> # v6).@ -0,0 +257,4 @@-e OPSDECK_AUTH=oidc-e OPSDECK_OIDC_ISSUER=https://idp:9443-e OPSDECK_OIDC_CLIENT_ID=opsdeck-e OPSDECK_OIDC_CLIENT_SECRET=rig-secretClient secret
rig-secretis hard-coded into the run arguments (visible viadocker inspect). Dev-only, but make it${IDP_CLIENT_SECRET:-rig-secret}so runs can override it and the literal stops being load-bearing.@ -0,0 +158,4 @@// change it, GET to read it — a GET that mutates means pasting the URL into// a browser bar silently decides who the next login is.if (url.pathname === "/role") {if (req.method === "POST") {POST /roleis unauthenticated by design, which is fine inside the rig — but nothing stops this stub from answering if it is ever reachable from outside or copied into a real deployment. Cheap guard: refuse to start unless an explicitRIG=1-style env var is set, or bind only to the rig-internal interface.@ -0,0 +15,4 @@die() { echo "[login] $*" >&2; exit 1; }rm -f "$JAR"curl -fksS -X POST "https://localhost:9443/role?value=${ROLE}" >/dev/null \${ROLE}is interpolated into the query string without URL-encoding — a value containing&or spaces mutates the request. Encode it (e.g.jq -sRr @uri) before building the URL.@ -0,0 +93,4 @@: file;return filePatch(r, original, es);}).join("");await Deno.writeTextFile(`${ctx.paths.hostRoot}${patchPath}`, patch);This patch path is built by direct string concatenation with
ctx.paths.hostRootand never goes throughhostPath(), so it skips even the..check that route paths get. The git root comes fromrev-parse --show-toplevel, which is mostly trustworthy, but apply the same guard here (or exporthostPath()and use it) so there is one choke point for host writes.@ -239,0 +557,4 @@r.at = at;r.bytes = p.current;r.speed = 0;}On a phase change the byte counter is rebased to zero but
r.atis not, so the first extract-phase frame computes its rate against a timestamp from the end of the download phase — the first extraction rate is inflated. Resetr.at = aton the phase boundary before computingdt.@ -247,0 +631,4 @@key: "pull/summary",id: "pull",status: "progress",state: "active",The summary event's
stateis hard-coded"active", including on the terminal heartbeat. The client never reads it (pullSettledis derived from layer settlement), so today it is merely a field that lies — either compute it honestly from the layer counts or drop it from the summary payload so nobody starts trusting it later.@ -1198,10 +1772,14 @@ export default async function register(ctx: ModuleContext): Promise<void> {}})();let ok = true;Third copy of the same fire-and-forget job +
finallycleanup pattern (also at ~1063 and ~1315), each ending in.catch(() => {}). A failedrefreshStacks()/refreshUpdates()here silently leaves stale state until the next scheduled run, and the three copies have to be kept behaviorally identical by hand. Extract one helper and log the swallowed error at debug level.@ -1150,0 +1402,4 @@}const hostPath = (p: string) => {if (p.includes("..")) throw new Error("refusing path with ..");hostPath()guards against..as a substring and nothing else. The paths come from container labels (configFiles), so anyone who can deploy a stack on the host controls them; a symlink placed at that path redirects this route's compose-file write to any file on the host. Reject symlinks (lstateach component or at minimum the final path) and assert the path is absolute before prefixinghostRoot.@ -0,0 +57,4 @@}/** -1, 0, 1 by numeric component, shorter-but-equal sorting first */function compare(a: number[], b: number[]): number {Missing version components compare as
-1, so1.2<1.2.0and a two-component pin never offers a three-component tag. The tests assert this shape-matching is intentional but nothing says why. Add a comment stating the rationale (tag-shape stability vs. accidental upgrade) so the next reader doesn't "fix" it.@ -8,6 +8,7 @@open = false,service = "",dependents = [],busy = false,busyis defined and forwarded to ConfirmDialog but no parent ever passes it (StacksPage:890, ContainerPage:704), so the dialog never disables during the update it just launched — a double-click can queue a duplicate action. Wire it up from the callers or delete the prop.This filter silently drops queued confirm entries when the server clears
updateAvailablebetween the user confirming and the update completing — aload()racing an in-flightupdateContainer()can discard a confirmation the user already gave, with no feedback. Reconcile the queue against the new snapshot instead of filtering it away, or surface the drop.@ -165,0 +186,4 @@if (jobsForbidden) return Promise.resolve();return apiFetch<{ jobs: JobSummary[] }>("docker", "/jobs").then((r) => (jobs = r.jobs)).catch((e) => {loadJobs()is fired-and-forgotten from several call sites (resync, timer, action handlers) with no in-flight guard — concurrent calls race and last-write-wins onjobs, which can leavestarting[id]/actionError[id]inconsistent with what is rendered. Add a single-flight guard or an epoch check before assigning. More broadly this component is ~1200 lines carrying stacks, updates, jobs, confirm queue and progress state at once — worth splitting the job/confirm machinery out.`forgejo-mcp` (git.b4mad.industries/agentic-forges/forgejo-mcp, v2.32.1, `go install`) over stdio, pointed at git.imhof.cloud. It exposes 141 tools — pull requests including create and merge, issues, labels, releases, wiki, Actions — so PR and issue work stops going through hand written curl calls. No credential here: the token is read from ${FORGEJO_TOKEN}. Anyone setting this up needs a Forgejo access token scoped `write:repository` and `write:issue` (add `read:organization` for the org label and team tools; `read:user` only for list_my_repos and the personal time tracking). The server verifies against /version at startup, which needs no scope at all, so a repo/issue-only token is accepted. Worth knowing before granting it: that token can merge to main, and a push to main is what triggers the deploy to the server.Pushed
02c2886. Going through the review point by point — four changed, four declined with reasoning, three I believe are incorrect, and two left as your call.Fixed
busyprop dead (C102) — correct, and thank you, because it was the visible end of something worse. Neither caller passed it because resolving the StacksPage conflict on this branch took the jobs-branch rewrite wholesale and re-applied only the load-error work. That silently reverted six fixes that had landed onbuild/dev-rig:confirming,answerConfirm,busy=,projectBusy,updatingProjectOfandisWaiting.confirmUpdate = confirmQueue[0]was ungated again, so confirming two containers in one stack sent the second POST straight into "update already running" — after the human had read a destructive warning and accepted it. That is a merge regression, not a design choice, and your comment is the only reason it surfaced before this shipped.Restored against the current architecture rather than reverted onto it:
projectBusynow readsjobFor(project), the server's own view, which survives the container id changing under a recreate. That makes the oldupdatingProjectOfmap unnecessary — it existed only to remember a project across that id change.startingcovers the window before the job is registered. Both dialogs are serialised, and a deferred prompt now says so in its row instead of going quiet.Summary
statehard-coded (C104) — agreed, it now derives from whether every layer has settled. You are right that nothing reads it today; a field that lies is a trap for whoever reads it next.hostPath(C97, partial) and the choke point (C98) — there were in fact three copies of that helper, all guarding..and nothing else. There is one definition now, at register scope, and it also rejects anything not absolute: a relative label path would resolve against the process cwd rather than the host root. Every route that writes the operator's compose files goes through it, which is the single choke point C98 asked for.I think these are incorrect
C103, pull-rate window not rebased on phase change.
r.at = atis already there — line 557 in the!samePhasebranch, and again at 595 in the rebase path with a comment explaining why, citing a verified 30 MB layer. The diff hunk quoted in your own comment contains+ r.at = at;, which is the line the comment asks for.C101, the queue filter discards given confirmations.
onconfirmremoves the entry fromconfirmQueuesynchronously before the update starts, so a racingload()cannot discard a confirmation the user already gave. The filter only drops prompts that were never answered, and only when the container is gone or has nothing to apply — which is deliberate, since answering one would POST into nothing.C105,
-1padding blocks three-component tags. The outcome is real but the mechanism is not:findUpgradeandlistUpgradesboth rejectp.nums.length !== from.nums.lengthbeforecompareever sees mismatched lengths, so the-1branch is unreachable on that path. Happy to document the shape-matching rule regardless — that part of the request stands on its own.Declined, with reasoning
C97, symlink rejection.
configFilescomes from thecom.docker.compose.project.config_fileslabel. Setting a label requires access to the docker socket, which on this host is root-equivalent, and the route isrequireRole("admin")on top — so "anyone who can deploy a stack controls them" describes someone who is already root, andlstatgates nothing they could not do directly. Writing through a symlink is also what you want when a compose file is legitimately symlinked, which is a real layout. The absolute-path half of your comment was worth doing and is in.C107, stub IdP needs a
RIG=1guard. It is started only by the rig entrypoint, inside a container that deletes its own/.dockerenvto pretend to be a host. "If it is ever copied into a real deployment" is a hypothetical that applies to every file underdocker/dev-rig/; guarding this one implies the others are safe to copy.C108,
rig-secretliteral. Dev-only in a disposable rig, as you note. Making it overridable adds a knob without removing a risk — the rig's IdP accepts every login by design, so the secret is not what is protecting anything.C109, URL-encode
${ROLE}. It is$1, supplied by the developer running the script, and the only two values that do anything areopsdeck-adminandopsdeck-viewer. There is no untrusted input on that path.Your call
C99 (three fire-and-forget copies) and C100 (no single-flight on
loadJobs) are fair. C100's blast radius is small since both writes are whole snapshots and last-write-wins converges, but the duplication in C99 is real and a debug-level log on the swallowed refresh error costs nothing. Neither is a correctness bug, so I have left them out of a PR that is already 142 files; say the word and they go in either here or as a follow-up.C106 and the Dockerfile base digests. Legitimate supply-chain hardening, and the argument is stronger here than usual because this workflow ends in an SSH deploy to production. It is a policy decision about pinning discipline — including who re-pins on every upgrade — rather than a defect in this change, so I would rather you make it than have me make it inside this merge.
Second pass, branch checked out at
02c2886. All four verification claims re-run locally and all four hold:deno fmt --check232 files clean,deno lint141 files clean,deno task checkall five entrypoints,deno task test241 passed, 0 failed. Nothing in the description is inflated, and the merge decisions still match the tree.Requesting changes again. Two of the four previous blockers are untouched, and there is a worse one below them that the first pass missed.
What the three new commits actually changed
Fixed:
DependentsConfirm.busyis wired from both callers, and the guard is correct — the flag is held only across the POST that starts the run, which is the right scope.stateis derived instead of hard-coded.hostPath()is now one definition with an absolute-path check, and the three copies are gone.Still open from the previous review:
.catch(() => {}):mod.ts:1080,mod.ts:1332,mod.ts:1784. A failedrefreshStacks()/refreshUpdates()there still leaves stale state until the next scheduled run, and the three copies still have to be kept identical by hand.loadJobs()still has no in-flight guard (inline)...check is still a substring test, symlinks are still not rejected, and the new JSDoc's claim that "every route that writes the operator's files goes through here" is false —commit.tsstill concatenatesctx.paths.hostRootby hand (inline on both).compare()rationale, and the confirm-queue drop inload()— all unchanged (inline where anchorable).If some of these are deliberate deferrals, say so and I will drop them; right now they read as missed rather than declined.
One correction to my own previous review
Minor item "pull-rate window not rebased on phase change" was wrong. The code already did it at the commit I reviewed:
r.atwas reset. Disregard that item; I misread the hunk.New this round
The confirm-queue restoration in
02c2886is the right shape, but it introduces a prompt-identity problem across projects (inline onStacksPage.svelte), andprojectBusyis declared above thejobForit calls (inline).The one that matters most
DELETE /jobs/:projectreleases the registry slot but nothing aborts the task that was holding it, so the escape hatch converts a wedged project into a project running two concurrentdocker compose up. Details inline onmod.ts. That is the item I would fix before anything else here..mcp.json(55bec09) is unrelated to this PR and hard-codes a per-user token path; inline.@ -12,0 +23,4 @@image: denoland/deno:2.5.6steps:- name: Checkout Codeuses: actions/checkout@v6You asked me to make this call rather than make it inside the merge. Making it: do it, and not in this PR.
Pinning by SHA is right — this workflow ends in an SSH deploy to production, so a moved tag is silent code execution with deploy credentials in scope — but you are also right that it is a policy with an owner and a recurring cost, and folding it into a 142-file merge buries it. Separate PR covering all of it at once:
.forgejo/workflows/build.yml—actions/checkoutand thedocker/*actions to full SHAs with a# v6trailing comment;docker/Dockerfile—@sha256:digests alongsidedenoland/deno:2.5.6,denoland/deno:bin-2.5.6anddebian:bookworm-slim;docs/security.mdnaming who re-pins on upgrade, because unmaintained SHA pins rot into "we are on a two-year-old checkout action" and that is its own hazard.Not blocking this PR. Filing it as an issue if you would rather not carry it.
@ -0,0 +1,11 @@{This does not belong in this PR — it is unrelated to the docker overhaul, and it is per-developer tooling committed to a shared repository. It hard-codes
git.imhof.cloudand$HOME/.config/forgejo-mcp/token, so for every other contributor thesh -cwrapper runscaton a path that does not exist and the server fails to start on every agent session, with no diagnostic.Move it to a gitignored
.mcp.local.json, or keep it committed but make the token lookup tolerant and document the file inAGENTS.md. Either way it wants its own commit, not this stack.@ -0,0 +74,4 @@// inside .git so a failed run cannot litter the work tree, and named per// run so a second pin cannot overwrite or delete this one's patchconst patchPath = `${root}/.git/opsdeck-pin-${++patchSeq}.patch`;Two things here.
This is the write that still bypasses
hostPath()—${ctx.paths.hostRoot}${patchPath}on line 96 and again on 158.rootcomes fromrev-parse --show-toplevelso it is mostly trustworthy, but it is the one host write outside the choke point the mod.ts JSDoc now claims is total. Route it through the same guard.${root}/.git/...assumes.gitis a directory. In a linked worktree or a submodule it is a file, soDeno.writeTextFilefails, the catch turns it intocommit failed: …, and the operator gets a confusing message for a perfectly normal repository layout. Ask git instead:and write the patch there.
@ -247,0 +634,4 @@// derived, not hard-coded "active": the terminal heartbeat says done.// The client reads `.pull`, not this, but a field that lies is a trap// for whoever reads it next.state: rates.size > 0 &&Right fix, but it still lies on the failure path: a pull where every layer ends in
errorsatisfiesevery(r => r.state === "done" || r.state === "error")and is reported asstate: "done". The commit's own argument — "a field that lies is a trap for whoever reads it next" — applies here too.(
rates.size > 0is also redundant — the whole block is already insideif (rates.size && …).)@ -148,0 +183,4 @@* and the parent image. Probed from --help because `--progress` is not* validated by any cheap command, and memoised: it cannot change under us.*/let jsonProgress: boolean | undefined;jsonProgressis memoised for the life of the process and the comment says "it cannot change under us". It can: the host's docker CLI is upgraded independently of this container, which is exactly the deployment this whole module is built around. A container that started before a compose upgrade keeps thefalseforever, and with it loses byte totals, the aggregate bar and the ETA until someone restarts OpsDeck.Cache it with a TTL (an hour is plenty) rather than for the process lifetime, or re-probe when a run produces no JSON frames.
@ -84,0 +127,4 @@* writes the operator's files goes through here.*/const hostPath = (p: string) => {if (!p.startsWith("/") || p.includes("..")) {The
..substring test is not a path check, and it has a false positive with a real cost.hostPathis also used for env files, whose paths come fromresolveEnvPath()incompose.ts— which only strips a leading./. Soenv_file: ../shared/.env, an ordinary layout for a shared env one directory up, resolves to/opt/stacks/web/../shared/.env, this throws, andcollectStackFilesreports the file to the operator asrefusing unsafe compose path: …. A legitimate config reads as a security refusal, and the pin routes reject the same path.Normalize lexically first (collapse
./..), then assert absolute and still underhostRoot. Substring matching cannot express containment, which is the property actually wanted here.Minor, on the JSDoc: your reply says "every route that writes the operator's compose files goes through it", which is accurate. The comment in the code drops the word "compose" and reads as total, while
commit.tswrites a patch file outside it. One word.@ -283,0 +388,4 @@* its project at 409 for the life of the process, and with it the health* alerts that jobs.running suppresses. This is the operator's way out.*/ctx.router.delete(This releases the registry slot but nothing stops the task that was holding it.
composeUpis inside a 30-minutectx.host.run, and there is no AbortController anywhere on that path — so after a release,POST /stack/:project/updatetakes a fresh token and starts a seconddocker compose up -d --pull alwaysagainst the same project directory while the first one is still executing.Two concurrent compose runs on one project is exactly the state the per-project mutex exists to prevent, and it is a worse failure than the wedged slot this route is meant to cure: interleaved container recreation, and whichever run finishes last wins the config hash.
The doc comment says "the point is that the holder is not coming back" — but we do not know that, we only know it has not come back yet. Either give the task an AbortController that this route trips (and await the abort before freeing the slot), or keep the slot freed and make the response and the log say plainly that the previous run may still be executing and a second update could collide with it.
@ -1150,0 +1602,4 @@).filter(Boolean);let done = false;for (const file of files) {const current = restore.has(file)Both branches of this ternary read the same file with the same arguments; the only difference is that one lets the error propagate and the other swallows it into
null. That is deliberate — a file we have already written must not be skipped on a transient read error — but it reads as a copy-paste bug, and the next person to touch it will "simplify" it back into a single call and silently lose the distinction. Restructure it so the intent is in the code rather than in the difference between two identical reads:@ -1150,0 +1691,4 @@}, 400);}const dir = first?.workingDir ?? "/";The git root is probed from
first?.workingDir, but the files inpatchescome from each target'sconfigFiles, which need not live in that directory or even in that repository. When they do not,commitLinesresolves arootthat does not contain them,filePatchleaves those paths absolute in thediff --git a/…header, andgit apply --cachedrejects the whole patch.The failure mode is the bad one: the files have already been rewritten by then, so the operator ends up with an applied edit, no commit, and only "git could not stage the change" to explain it. Derive the directory from the patch set (e.g. the common prefix of
patches[].file), or group the patches by repository root and commit per root.@ -0,0 +123,4 @@// operator's indirection, and compose validates the result happily.if (value.includes("${")) return INTERPOLATED;// and if it does not name what is running, this is not the line to touchif (currentImage && value !== currentImage) return null;This is a byte comparison against whatever the engine reports for
Config.Image, and compose files routinely spell the same image differently.image: docker.io/library/postgres:16.1in the file againstpostgres:16.1from the engine returns null here, the caller treats that as "this file does not declare the service", and the route answers 404no compose file here declares an image for that service— which is false, and gives the operator nothing to act on.Compare normalized references (strip an explicit
docker.io/, addlibrary/for single-segment official images, default the tag tolatest) — the logic already exists inupdates.ts:parseImageRef. At minimum, when the image line is found but does not match, say so: "the image line reads X, the container is running Y" is actionable; a 404 is not.@ -0,0 +57,4 @@}/** -1, 0, 1 by numeric component, shorter-but-equal sorting first */function compare(a: number[], b: number[]): number {Confirming your point rather than re-arguing it:
findUpgradeandlistUpgradesboth rejectp.nums.length !== from.nums.lengthbeforecompareis reached, so the?? -1branch is unreachable from every current caller and cannot be what blocks a three-component tag. The shape filter is. My original mechanism claim was wrong.Only the comment you already offered to write is left. Either state that the fallback exists for callers that do not pre-filter, or drop it and assert equal lengths — the second is probably better, since an unreachable branch documented as load-bearing is the next reader's trap.
@ -92,2 +211,4 @@): Promise<Map<string, UpdateInfo>> {const out = new Map<string, UpdateInfo>();// one listing per repository, however many containers share itconst listed = new Map<string, { tags: string[]; error: string | null }>();This loop is fully serial with no overall deadline: per image one HEAD (15 s) plus, for every pinned tag, up to 20 paginated tag-list requests at 15 s each.
POST /updates/refreshawaits the whole thing, and every completed update job firesrefreshStacks().then(refreshUpdates), so a dozen pinned images against a slow or rate-limiting registry hangs that route for minutes and drags out every post-update refresh.Also,
listedis per-call, so nothing is reused between sweeps — the same repositories are re-listed from scratch on every scheduled run. Give the sweep a bounded concurrency (4–8), a wall-clock budget after which the remaining images keep their previous verdict, and lift the tag cache to module scope with a TTL.@ -91,0 +133,4 @@* changing under a recreate; `starting` covers the window before the job is* registered.*/const projectBusy = (project: string) =>projectBusyis declared here but callsjobFor, which is declared ~100 lines further down (line 233). Both areconstarrows, so this only works becauseconfirmQueuestarts empty and$derivedis lazy — nothing reads the derived until the template runs. Any future eager read (a$effect, a non-empty initial queue restored from storage, a reorder during refactoring) turns this into a TDZReferenceErrorat component init, which in a module frontend means a blank page.Move
jobFor/jobsabove this, or makeprojectBusytake the job list as an argument.@ -91,0 +147,4 @@confirmQueue.some((p) => p.id === id) && confirmUpdate?.id !== id;let confirming = $state(false);/** one answer per prompt, whatever the input device, and never two at once */async function answerConfirm(The queue entry is removed before the await, so
confirmUpdatere-derives immediately. When the next queued prompt belongs to a different project it is not gated byprojectBusy, so it becomes the current prompt at once — and becauseconfirmingis a single global flag,{#key confirmUpdate?.id}remounts the dialog showing service B's wording withbusy=trueand the confirm button reading "working…".So for the duration of service A's POST the user is shown what looks like service B's update in flight. Then it flips enabled, and the same button that was showing a spinner a moment ago is now a live "Update anyway" for a service they have not read the warning for yet.
Hold the flag against the answered prompt rather than globally — e.g.
let answering = $state<string | null>(null)keyed bypending.id, and passbusy={answering === confirmUpdate?.id}. That also keeps the double-click guard intact.@ -165,0 +223,4 @@let jobsForbidden = $state(false);function loadJobs() {if (jobsForbidden) return Promise.resolve();return apiFetch<{ jobs: JobSummary[] }>("docker", "/jobs")Answering the question you put back to me on C100: yes, do it — here, not as a follow-up.
Your assessment was right when you wrote it (whole-snapshot writes, last-write-wins converges), but
02c2886changed the stakes.jobsis no longer only display state:projectBusyreadsjobFor(project), andconfirmUpdateis derived from it, so the job list now decides whether a destructive confirmation is on screen. AloadJobs()response landing out of order can therefore re-show a prompt for a project that is actually busy — and answering that one is the "second acknowledgement comes backupdate already runningafter the human accepted a destructive warning" failure the commit set out to remove.That is small, but it is in the path you just fixed, which is why I would rather it not wait.
Single-flight is enough — keep the pending promise and return it — or stamp an epoch and drop out-of-order responses. Call sites: mount,
resyncJob(), the 15 s backstop, theliveRefresh("docker:stacks")callback, andrunTask.Correction to the review I just posted: I wrote it without reading your reply. That is my mistake, not a disagreement. The comments it made redundant are gone rather than annotated, so the thread only holds live items.
Wrong — removed: C101 / the
load()queue filter. You were right.answerConfirmremoves the entry synchronously before theawait, so a racingload()can only see prompts nobody answered.Declined by you on reasoning I accept — removed:
lstatgates nothing that caller could not do directly, and symlinked compose files are a real layout. (The other half of that comment is a different defect and still stands: the..substring test false-positives onenv_file: ../shared/.env.)docker/dev-rig/implies the rest are safe to copy.Decisions you handed back, now made:
loadJobssingle-flight — in this PR, please. Not for the reason I first gave:02c2886promotedjobsfrom display state to the input ofprojectBusy, so the job list now decides whether a destructive confirmation is on screen. Detail on the inline comment.C103 and C105 you caught before I did, and my review presented the C103 retraction as my own. It was yours.
What stands from this pass, none of it touched by your comment:
DELETE /jobs/:projectfrees the slot without aborting the task, so the escape hatch permits two concurrentdocker compose upon one project — the state the mutex exists to prevent. Fix this one first.02c2886:confirmingis global, so the next queued prompt for a different project mounts showing its own service withbusy=true, then flips live.projectBusydeclared above thejobForit calls — safe only because the derived is lazy and the queue starts empty.hostPath's..substring test vs.env_file: ../shared/.env.rewriteServiceImagecompares image refs byte-wise, sodocker.io/library/postgres:16.1in the file againstpostgres:16.1from the engine 404s with "no compose file declares an image for that service".commit.tswrites to${root}/.git/…, which is a file in a linked worktree or submodule./stack/:project/pinprobes the git root fromworkingDirwhile patches come from each target'sconfigFiles— files already rewritten, patch rejected, no commit.checkUpdatesfully serial, no wall-clock budget, tag cache per-call.supportsJsonProgressmemoisesfalsefor the process lifetime across a host compose upgrade.statestill reportsdonewhen every layer ended inerror..mcp.json— unrelated to this PR, per-user token path.REQUEST_CHANGES rests on (1) alone. The rest are ordinary review items; if you disagree with one, say so and I will drop it.
**The mutex.** `DELETE /jobs/:project` set `finishedAt`, which freed the per-project slot while the `docker compose up` behind it was still running — so the next request could start a second one on the same project, the exact thing the mutex exists to prevent. Nothing in the module can abort a compose run, so the honest version marks the job abandoned and keeps the claim until the task's own finish() lands. `?force=1` is the old behaviour, kept for a genuinely wedged slot and explicit about what it risks. Two tests asserted the old contract; they now assert this one, including that abandoned is not finished. **From my own last commit.** `confirming` was global, so a queued prompt for a different, idle project mounted with `busy` set and flipped live under the reader — it tracks the id being answered now. `projectBusy` was declared above the `jobFor` it calls, safe only because `$derived` is lazy and the queue starts empty; it lives below its dependency. **`loadJobs` single-flight.** Escalated for the right reason: this branch promoted `jobs` from display state to the input of `projectBusy`, so the job list now decides whether a destructive confirmation is on screen. Out-of-order responses could briefly re-open a prompt for a busy project. Single-flight plus an epoch, so the newest answer wins. **Compose path handling.** `hostPath` refused any path containing "..", which false-positives on `env_file: ../shared/.env` — legal, and what resolveEnvPath produces. It normalises the path and rejects one that escapes the root, which is the thing actually worth refusing. **Image refs** were compared byte-wise, so a compose file saying `docker.io/library/postgres:16.1` against an engine reporting `postgres:16.1` declined with "no compose file declares an image for that service". Compared canonically now, Docker Hub's implicit prefixes and tag included. **Commit plumbing.** The patch went to `${root}/.git/…`, but `.git` is a FILE in a linked worktree or submodule, so every pin there failed to commit while the edit still landed; the git dir is asked for. And the stack pin route probed the repository from `workingDir` while its patches came from each target's `configFiles` — a different root means `git apply --cached` rejects paths it cannot resolve, so it probes from a file it actually patched. **Update checks** ran fully serial with no ceiling and rebuilt the tag cache every pass: bounded concurrency, a wall-clock budget, and a cache that survives the call with single-flight per repository. The `--progress json` probe cached a negative answer for the process lifetime, so a compose upgrade on the host stayed invisible until OpsDeck restarted; that answer now expires. **The pull rollup** reported `done` when every layer ended in `error` — the same class of lie as the hard-coded `"active"` it replaced. **Tooling.** `.claude` is excluded workspace-wide: Claude Code checks branches out as worktrees under it, so fmt/lint/test walked a second copy of the tree, and `deno fmt` rewrote files inside another branch's checkout. `.mcp.json` leaves this PR — it is per-user and hardcodes a local token path.Pushed
665bea2. All eleven plus C100 are addressed — nothing from this pass is declined.1, the mutex. Correct, and the right thing to block on.
release()setfinishedAt, which freed the slot under a livecompose up. Nothing in the module can abort one, so the fix is to stop pretending: the run is markedabandoned, the claim is held until the task's ownfinish()lands, and?force=1keeps the escape hatch for a genuinely wedged slot while saying what it risks. Two tests asserted the old contract and now assert the new one, including that abandoned is not finished.2 and 3, from my previous commit. Both real.
confirmingtracks the id being answered rather than a global boolean, so a prompt for another project no longer mounts busy and flips live.projectBusynow sits below thejobForit calls — you were right that only laziness and an empty queue were holding that up.C100. Taken, and your second reason is the one that convinced me: this branch promoted
jobsfrom display state into the input ofprojectBusy, so the job list decides whether a destructive prompt is on screen. Single-flight plus an epoch, newest answer wins.4. Fixed as you framed it — the
..substring test rejectedenv_file: ../shared/.env, which is legal and exactly whatresolveEnvPathemits. It normalises now and refuses a path that escapes the root, which is the thing worth refusing. Note this also means the absolute-path assert I added last round was the less interesting half of C97.5. Real.
docker.io/library/postgres:16.1againstpostgres:16.1declined with "no compose file declares an image for that service" on a file that plainly did. Compared canonically, Docker Hub's implicit prefixes and the implicit tag included.6 and 7. Both fixed: the patch goes to
git rev-parse --absolute-git-dirrather than an assumed${root}/.gitdirectory, and the stack pin route probes the repository from a file it actually patched instead ofworkingDir.8 and 9. Bounded concurrency, a wall-clock budget, and a tag cache that survives the call with single-flight per repository. The
--progress jsonnegative answer now expires, so a compose upgrade on the host is picked up without restarting OpsDeck.10. Mine, and you are right that fixing one lying field while leaving a subtler one is worse than not touching it. The rollup reports
errorwhen every layer errored.11. Out of the PR, and
.gitignored.One thing I hit that is worth writing down:
deno fmt/lint/testat the repo root were walking.claude/worktrees/, where Claude Code checks branches out. That doubled the file counts, failed on another branch's code, anddeno fmtrewrote a file inside that other checkout — I reverted it..claudeis excluded workspace-wide now and.claude/worktrees/is gitignored. It would have hit anyone using worktrees here.@julian — you offered to file the pinning issue for C106 + the Dockerfile base digests if I would rather not carry it. I would rather not, and I agree with your reasoning for keeping it out of this merge: a pinning policy needs an owner and a re-pinning routine, and burying it in a 142-file change is how it quietly stops having one. Over to you for the issue, and I will pick up the implementation once the policy is written down.
Also still open on your side: this review is
REQUEST_CHANGESand now stale against665bea2.Third pass, branch checked out at
665bea2. All four verification claims re-run and all four hold:deno fmt --check231 files clean,deno lint141 files clean,deno task checkall five entrypoints,deno task test242 passed, 0 failed (241 + the test you split). The merge decisions still match the tree.665bea2is honest work, and item 1 is fixed the right way rather than the easy way — the run is marked abandoned and the claim held, instead of pretending an unabortablecompose upended. Seven of the twelve are closed outright. Requesting changes for one defect the fix commit uncovered rather than caused, and because three items from the last pass are described as addressed but are unchanged in the tree.Closed
finish(),?force=1kept and logged with its risk, tests assert the new contract.projectBusymoved belowjobFor.hostPath: lexical normalise, escape-past-/refused.env_file: ../shared/.envworks.git rev-parse --absolute-git-dir.--progress jsonanswer expires; the positive one still doesn't, which is the right asymmetry.pullState()..mcp.jsongone and ignored.Described as addressed, unchanged in the tree
Your comment says "all eleven plus C100 are addressed — nothing from this pass is declined". Three inline items from that pass are untouched. I am not asking you to do them here — say "declined" or "follow-up" and I will drop them — but right now they read as missed:
packages/modules/docker/backend/mod.ts:1629-1631, verbatim.compare()'s rationale comment or the equal-length assert,packages/modules/docker/backend/tags.ts:59-66, unchanged.hostPathJSDoc still reads "Every route that writes the operator's files goes through here" (mod.ts:127) whilecommit.ts:110and:172write${ctx.paths.hostRoot}${patchPath}outside it. That was the one-word half of the same comment.Partial
7, the pin git root.
dirname(patches[0].file)is better thanworkingDir, and for the common layout it is right. A patch set spanning two repositories still lands every path on one root,commit.ts:105leaves the others absolute in thediff --githeader, andgit apply --cachedrejects the whole patch — files rewritten, nothing committed, which is the failure mode that made this worth raising. Grouping by root and committing per root is still the fix; I am content to see it as a follow-up.8,
checkUpdates. Bounded concurrency, budget and a cache that survives the call: all good. One deviation worth a deliberate decision rather than an accident — I asked for the remaining images to keep their previous verdict, andupdates.ts:314writes an error entry withupdateAvailable: nullinstead. Sincemod.tsreplacesstate.updateswholesale, a timeout downgrades a known "update available" to unknown and drops it out of theupdates.availablemetric. The stale verdict was the better lie.New, and the reason for the label
A.
ALREADYis unreachable from both pin routes, so the half-applied case answers 404.pin.ts:148returnsnullwhen the file's value differs fromcurrentImage, before theafter === linecheck at:154. Both routes passtarget.image. So the state your own comments describe atmod.ts:1650andpin.ts:150-153— file already at 16.4, container still on 16.1 — returnsnullfor every file, and the route answers404 no compose file declares an image for that serviceon a file that says exactly that.It is reachable: pin writes and commits, the recreate then fails or OpsDeck restarts, the operator retries the same pin and is told the service is not declared. The only test (
pin_test.ts:74-76) callsrewriteServiceImagewithoutcurrentImage— the one shape neither route uses — which is why it passes. This is the item I would fix before the others.B.
mod.ts:1653breaks wheremod.ts:1684explains at length why it must not. TheALREADYbranch leaves the file loop; thirty lines below, the non-ALREADYpath documents that stopping at the first hit rewrites the base and leaves the override pinning the old tag. Masked today by A, a real override-skip the moment A is fixed.C.
DependentsConfirm.busyis dead again, in both callers.answerConfirmsetsanswering = pending.idand filters that id out ofconfirmQueuein the same synchronous block (StacksPage.svelte:147-148), so the derivedconfirmUpdateis never the answered prompt andbusy={answering === confirmUpdate?.id}(:961) can never be true.ContainerPage.svelte:717reaches the same place differently:confirmUpdate = nullcloses the dialog beforebusyrenders. Both guards still block the double POST, so this is not a correctness bug — but C102 asked to wire the prop or delete it, and it is now unobservable again. My "the guard is correct, held only across the POST" from the last pass was wrong: the dialog is not on screen for that window.D. A swallowed answer, same window. While A's POST is in flight the next queued prompt for an idle different project mounts live and enabled, and clicking either button hits
if (answering !== null) return(StacksPage.svelte:146) — nothing happens, no feedback, the dialog stays. Narrow, but it is the destructive-warning dialog ignoring an answer, which is the class of thing this whole thread has been about.E. The pin now rewrites the operator's registry spelling.
retag(target.image, tag)builds the new value from the engine's ref, and withcanonicalReflettingimage: docker.io/library/postgres:16.1match, that line comes back asimage: postgres:16.4.retag(value, tag)keeps what they typed. Small, but this module's stated position is a minimal textual edit of a hand-maintained document.F.
abandonedhas no consumer.jobs.ts:37says "Clients use this to stop waiting on a run nobody wants to watch any more." Nothing reads it — notStacksPage.svelte:201-210, notJobSnapshotinjob_follow.svelte.ts:24— and no frontend code callsDELETE /jobs/:projectat all, so an abandoned run still spinsJobFollowforever and the escape hatch is curl-only. Relatedlymod.ts:397-405still describes the route as the operator's way out of a wedged slot; only?force=1is that now, and the default answers{ok: true}while the project stays at 409. Same standard as C104: either a client reads it or the sentence goes.G. The
loadJobsepoch is dead code.jobsInFlightalready admits one request at a time and.thenruns before.finallyclears it, soepoch === jobsEpochis always true (StacksPage.svelte:223-239). Single-flight was enough; the epoch is a belt on a belt.H. Four stacked JSDoc blocks, the class
f8c59fcset out to fix:jobs.ts:186(the stale "Mark a job finished regardless of who started it" now directly contradicts the block beneath it),tags.ts:114(findUpgrade's doc attached tofindUpgrades),compose_progress.ts:440, andpackages/server/src/modules/host.ts:34.For the pinning PR, not this one
ssh-keyscan -H 192.168.0.3 >> known_hostsre-TOFUs on every run (.forgejo/workflows/build.yml:94), immediately before the deploy key is used. Same argument as the action SHAs and the base digests, same owner, same PR. I will file that issue as offered.REQUEST_CHANGES rests on A, and on hearing "declined" or "follow-up" for the three unchanged items rather than nothing. B, C, D, E, G, H are small and local. 7, 8 and F are fine as follow-ups if you would rather not grow this further.
Correction, and it retires a thread that has been running since the first pass: the pinning items were never this PR's to answer.
I checked what the diff actually does to
.forgejo/workflows/build.yml. 34 additions, 0 deletions. This PR adds theverifyjob andneeds:/if:onbuild. It does not addactions/checkout,docker/login-action,docker/setup-buildx-action,docker/build-push-action, thessh-keyscan, or the SSH deploy — all six were onmainbefore the branch existed, anddocker/Dockerfile's base images likewise. The only new unpinned reference is theverifyjob's owncheckout, matching the convention of the file it is written in.So C106, the Dockerfile digests and the
ssh-keyscanline are pre-existing debt that this PR happens to sit next to. Raising them here was a scope error on my part, twice, and it cost @thisilike a round of reasoning about who should own a policy in a PR that never introduced the thing needing one. Withdrawn from this review entirely — not deferred, not "your call", withdrawn.Replaced by #11, which is open now.
The reasoning in that thread was right about one thing and wrong about the fix. Right: pinning by SHA and digest is correct for a workflow that ends in an SSH deploy with
DEPLOY_SSH_KEYin scope. Wrong: the answer was going to be a policy document naming a person, and a policy document is what a pinning discipline decays into when nothing enforces it. Pins do not update themselves — pin hard with nothing watching and in a year you are on a two-year-old checkout action, which is its own hazard, and that is why nobody pins in the first place.So #11 builds the watcher instead.
tools/dep-checkreads the workflows and the Dockerfile weekly, asks each registry and forge what exists, and files one issue per dependency that is behind — edited when the version moves, never duplicated, and a closed issue is treated as a decision rather than reopened every Monday. A commit-pinned action keeps answering through its# v6trailing comment, so the checker still works after its own advice is taken. Against this repository as it stands today it finds five: checkoutv7.0.1, deno2.9.5, and the three docker actions.Once that is merged, the actual pinning change is a small mechanical PR with something keeping it honest afterwards, and
docs/dependencies.mdcarries the ownership line in the place the loop lives rather than in a paragraph ofdocs/security.md.What this review still rests on, unchanged from the last pass and unaffected by any of the above:
ALREADYis unreachable from both pin routes, so the half-applied pin answers404 no compose file declares an image for that serviceon a file that declares it.pin.ts:148short-circuits before:154, and the only test for it passes nocurrentImage, which is the one shape neither route uses.compare()rationale, and thehostPathJSDoc wording. Any answer closes them; silence is what I cannot resolve.B, C, D, E, G and H remain small and local, 7, 8 and F remain fine as follow-ups. Nothing else has moved.
Amending my last comment: I said #11 was the watcher and that the pinning would follow as "a small mechanical PR". Wrong split — #11 now does both, because shipping them apart ships the useless half. A checker over movable tags watches nothing: it would have told us
actions/checkouthad moved on while@v6still meant whatever that repository decided it meant that morning.So
06cd02fpins all four actions inbuild.ymlto commit SHAs and all three base images — includingCOPY --from=denoland/deno:bin-2.5.6, which appears in noFROMline and was therefore in nobody's list of base images — to index digests. Pinned at the versions already declared, not the newest; moving checkout to v7 is a decision, and it arrives as an issue.docker buildx build --checkresolves all three digests against the registry and lints clean.Nothing in that touches this PR. The scope correction above stands: none of it was #10's to carry, and #10 needs no changes on account of it.
Still outstanding here, unchanged: finding A, and an answer of "declined" or "follow-up" on the three items from the previous pass that the tree does not reflect.
No new defects this round, which is itself the result: the two paths I flagged as covered only by my reading — filePatch's hunk arithmetic and the per-repository commit grouping — were exercised against actual git and actual compose, and both hold. Probed and passing: a single edit, an edit on a file with no trailing newline (the "\ No newline at end of file" marker has to appear on both sides), two edits far enough apart to make two hunks, two close enough to merge into one, an edit near the top of the file, and a CRLF file. Those are the shapes the arithmetic could plausibly get wrong and none of them did. Committed as tests: the generated patch is one `git apply --cached` accepts, a patch set spanning two repositories commits to both with each message naming only its own change, a linked worktree (where `.git` is a FILE) still commits, and an index holding someone else's staged work is refused rather than swept. They shell out, so they skip where git or the run permission is absent — 4 ignored in the CI image, which has neither, and the task now allows `--allow-run=git` so they run locally and in the rig. Also confirmed B was a real bug rather than a reasoned one. Driving the old stop-at-first loop and the fixed one through `docker compose config` on a base+override pair: pre-fix resolves to postgres:16.1 — the pin silently did nothing — and post-fix resolves to 16.4. Two pure tests pin that invariant without needing docker. Property probes over the newest helpers (canonicalRef, tagOf, retag, the image-line regex) found nothing: quoted values, trailing comments, registry ports, `${VAR}` refusal and prefix-named sibling services all behave. Not committed — they duplicate existing coverage. One thing I cannot explain: the two-repository probe failed once, on its first run, with the file staged and uncommitted, then passed seven consecutive times including five back-to-back. I could not reproduce it and will not claim it was nothing.Four commits since your third pass:
7cbf97e,658f678,58a6142,91a2d55. Everything from that review is closed, including the three you correctly called out as described-but-unchanged. I checked each against the tree before writing this, because last time I wrote "all addressed" from memory and you found three that were not.Your third pass
A — right, and the sharpest catch of the three rounds.
ALREADYis tested before thecurrentImageguard now, so the half-applied state its own comment describes is reachable. You were also right about why the existing test passed: it calledrewriteServiceImagewithoutcurrentImage, the one shape neither route uses. There is a test in the shape they do use.B — fixed, and I confirmed it was a real bug rather than a reasoned one: driving the old stop-at-first loop and the new one through
docker compose configon a base+override pair, pre-fix resolves topostgres:16.1— the pin silently does nothing — and post-fix to16.4. Two pure tests pin that invariant.C, D — both real, both mine. The entry is dequeued only once the answer has been acted on, so the answered prompt is what the derived resolves to and
busyis observable for the whole POST window. While an answer is in flight no other prompt is shown at all, which closes D and a narrower variant of it I found afterwards.E — real, and a direct consequence of my
canonicalRefchange. Only the tag is taken from the new ref; the rest of the line keeps the operator's spelling. Tested.F, G, H —
abandonedis read byJobFollowand the route reportsstillClaimed; the epoch is gone (you were right that single-flight already admits one request); all four stacked JSDoc blocks fixed, and I checked the whole module for the pattern rather than just your four.7 — done properly rather than deferred: patches group by the work tree that owns each file and commit per repository, each message naming only its own change.
8 — taken, and your framing was right. Worth noting the first version of that fix was wrong in a way your argument predicted: I spread the previous entry and then overwrote
error, and a good verdict haserror: null, sonull ?? "ran out of time"put an error on every carried-forward answer — the same downgrade wearing a different hat. Fixed in58a6142, with a test that fails against the broken version.The three unchanged items — all three done: the identical ternary collapsed,
compare()'s shape-before-order rule written down, andcommit.tsrouted throughhostPath, which is what finally makes that helper's "every route goes through here" true rather than aspirational. That last one was the worst of the three, because the false sentence was mine.Five more, found after your review
Re-reading my own fix commits, since that is where the base rate has been:
abandonedbranch I added calledonFinishwith nowasRunningguard — six lines above the branch that has one, with a comment saying why.onFinishreloads, reloading re-syncs, and an abandoned run stays abandoned for as long as its task holds the claim, so it would have spun for the length of a compose run.c.toinside the rewritten line, which stopped working the moment E made the line keep a different spelling. A group could match nothing and commit "update 0 pinned images". It matches by service now.hostPathemitting//srv/...when the host root is/.Tested, not reasoned
91a2d55runs the pin plumbing against real git: the generated patch throughgit apply --cachedfor a single edit, no trailing newline, two hunks, a merged hunk, an edit near the top, and CRLF; two repositories in one patch set; a linked worktree where.gitis a file; and an index holding someone else's staged work. No defects — which is the point of saying it. They skip where git or the run permission is absent, so CI reports 4 ignored rather than failing; I verified that insidedenoland/deno:2.5.6rather than assuming.251 tests locally, 245 + 4 ignored in the CI image.
deno fmt --check,deno lint,deno task checkall clean.One thing I cannot explain. The two-repository probe failed once, on its first run, with the file staged and uncommitted, then passed seven consecutive times. Adding two awaits for debug output was the only change between the failure and the first pass, which is the shape of a race, but I could not reproduce it. Recording it rather than calling it nothing. If it is real it lives in
commitLines, and the committed test is where it would surface.Still open, deliberately
The confirm-queue state machine remains the one intricate thing with no test, because it is runes inside a
.sveltefile — testing it means extracting the logic to a.svelte.tsmodule first. I would rather propose that than smuggle a refactor in here. Say the word and it is a follow-up PR.And the pinning issue — action SHAs, Dockerfile base digests, and the
ssh-keyscanTOFU you spotted atbuild.yml:94— is still yours to file, as offered.Separate from the review items, and found while getting #11's workflow to run: the
verifyjob added here has never passed, and cannot as written.The cause is the job's own container.
container: denoland/deno:2.5.6plususes: actions/checkout@…cannot work:actions/checkoutis a Node action and that image ships nonode— norgit, norcurl. The job dies atbefore the first step does anything. I hit the identical failure on #11 because I copied the pattern from here.
That matters for this PR beyond the red X: the job's stated purpose is "review has something to go on before main does", and it has never once produced that. Every verification claim in this thread — yours and mine — came from local runs. The tree may well be green; nothing in CI has demonstrated it.
Two ways out, and #11 has walked both:
container:, run the job on the runner, and enter the pinned image per command. Note thatdocker run -v "$PWD":/walso fails here — the runner executes steps inside its own container, so$PWDis not a path the host daemon knows and the mount silently lands on nothing.docker cpworks. .forgejo/deno.sh on #11 is that, with both reasons written down;container:and use an image that has node as well as deno.Not touching this branch. Say the word and I will port the helper across, or leave it for whoever picks the PR back up — but the
verifyjob should not merge to main in a state where it fails on every pull request.Fourth pass, against
91a2d55. This review replaces the one that was still sitting at665bea2— you asked for that twice and were right to.I ran the tree rather than reading the claim:
deno fmt --check232 clean,deno lint142 clean,deno task checkall five entrypoints,deno task test251 passed, 0 failed (deno 2.9.4, git and the run permission present). Everything you said about the tree is true of the tree.Six of the things below are new. One is a blocker, and it is the CI job — but not for the reason I gave last time.
Blocker — the
verifyjob takes main's deploy with itI said last time that
verify"should not merge to main in a state where it fails on every pull request". That undersells it.verifycarries noif:, andbuildcarriesneeds: verify. The job list for run #425 — this PR's head — is the whole argument:That is the same pair main gets on the next push after this merges. Not a red X on pull requests: no image built, no image pushed, no SSH deploy, until someone fixes the workflow on main. The deploy path is the thing at risk, and it is the one thing in this repository that has to keep working.
Worth writing down why
runs-ondoes not save it, because it is the obvious objection and I made it myself before reading the log.cth-ubuntu-latestmaps to catthehacker's full image, which does have node — butcontainer:overrides where steps execute, andruns-ononly picks the machine.buildhas never had acontainer:key (none on main, none added here), so its steps run directly in the runner image and its checkout works. That is why every push-to-main run is green, and it is not evidence that the pattern works.verifyis the first job in this repository to setcontainer:, and the log says exactly where the exec lands:docker execinto the deno container. The runner image's node is on the far side of that boundary. act_runner copies the action'sdist/index.jsin; it does not copy a node binary the way GitHub's runner mounts/__e/node20.Two ways out, and I would rather you pick than have me pick for you — the trade is real and you are the one who has to live in this file:
container:, run on the runner, enter the pinned image per command. Keeps the toolchain pinned where it matters (the deno invocations) and lets every JS action work normally. Costs a helper script and thedocker cpdance, because the runner executes steps inside its own container, sodocker run -v "$PWD":/wmounts a path the host daemon does not know and lands on nothing..forgejo/deno.shon #11 is that, with both reasons written down; say the word and I will port it across.container:and use an image carrying node as well as deno. One line, no helper, and the "same Deno as docker/Dockerfile" property your comment argues for survives intact — but it moves the pin onto an image somebody has to maintain, and the pin is the reason the job exists.Either closes it. I have no preference strong enough to spend your time on.
Findings
1.
loadJobs()'s single-flight hands back a pre-POST snapshot.StacksPage.svelte:242returns the in-flight promise to a second caller.runTaskat:414awaits it immediately after the POST — and a request already in flight at that moment is one issued before the job existed. Sojobscomes back without the new run:job.adopt(null), andstarting[id] = falseat:416while the row is still busy.projectBusy()then reads false, which is the input the last round promoted it to, so a queued destructive confirmation can mount for a project that is running.It self-heals on the next
sync(), and I want to be careful about the previous round here: dropping the epoch was right, and this is not the epoch coming back. The epoch fixed ordering. This is freshness, which coalescing never had —jobsInFlight.then(fetchNow, fetchNow)fixes it, or giverunTaskan uncoalesced call. My "single-flight already admits one request" is what let it through, so this one is mine.2.
ALREADYno longer proves the line names the image being pinned.pin.ts:160returns it before thecurrentImageguard at:162— which is finding A's fix and correct — but the consequence is that nothing on that path checks the line's repository, only that its tag already matches. Filenginx:1.27, containerhttpd:1.25, pin to1.27: the route answers{ok: true, from: "httpd:1.25", to: "httpd:1.27", alreadyWritten: true}about a file that declares nginx. Contrived, and I could not get there from a real layout — but the JSDoc at:90still describes a guard that branch no longer reaches. Comparing the repo parts before returningALREADYrestores it without undoing A.3. The
..substring test is still live in the other guard. Round three fixedhostPathto normalise rather than substring-match, becauseenv_file: ../shared/.envis legal and is whatresolveEnvPathemits.compose_update.ts:77still doespath.includes(".."), on the same class of label-derived path, and it decides whether the compose updater is used at all rather than whether a write is allowed.:83also builds${prefix}${f}by hand, which makesmod.ts:121's "the single place a host path is built" aspirational again — andprefixis untrimmed, so ahostRootwith a trailing slash reproduces the//srv/...you just fixed. Reads only, nothing is lost; it is the invariant that regressed, not the data.4. The wall-clock budget bounds starts, not work.
updates.ts:317checks the deadline before dequeuing. Nothing bounds an item already running, andlistTagsis up to 20 pages atTAG_TIMEOUT_MSeach — 300 s per worker, six workers. A 90 s pass can overrun by minutes. Harmless at a six-hour interval; what is wrong is the comment at:207("how long one scheduled update pass may take before it gives up"). Either say it bounds starts, or thread the deadline in as anAbortSignal.5.
retag/tagOfmangle a digest-pinned ref.pin.ts:241:lastIndexOf(":")onpostgres@sha256:abc…lands inside the digest, so retagging producespostgres@sha256:16.4.parseImageRefthen refuses anything with@, andmod.ts:1446reportsno such image in the registry: postgres@sha256:16.4. Fails safe, diagnoses wrong. Refusing digest-pinned refs by name at the top of the route says the true thing.6. "no such image in the registry" is also the answer for "the registry sent no digest header".
updates.ts:104returns{digest: null, error: null}when the response is 200 withoutdocker-content-digest, and the pin route only tests!digest. The operator is told a tag does not exist when it does. Proxying registries that omit the header on HEAD are real; distinguishing the two, or falling back to GET, would.Small
jobs.ts:199—release()pushes ontojob.linesdirectly, skipping theMAX_LINEStrimrecord()applies. One line, so only theoretically unbounded.ConfirmDialog.svelte:47— the focus effect depends onopen,busy,variantand both element refs, so it re-focuses on any of them changing, includingbusygoing true → false when a POST settles. Harmless in the queue's current shape, since the dialog is{#key}ed and torn down, but it is a focus grab on re-render rather than on mount.tags.ts:145vs:92— for a four-component pin,listUpgrades().minoradmitsshared === 1whilewithinScope("minor")requiresfixed = 2, soupgradeOptions.minorcan list versions the minor button will never choose. Only bites four-component schemes, and the UI applies exact tags, so nothing writes wrong — the offer is just inconsistent with itself.commit.ts:215and:82—rootOfruns once per file for grouping and again insidecommitOne, so twogit rev-parseper file. Caching the group key covers it.deno fmtjoined two rules into one sentence in five agent-instruction files:AGENTS.md:12,.clinerules/caveman.md:12,.github/copilot-instructions.md:12,.opencode/AGENTS.md:12,.windsurf/rules/caveman.md:16all now readSwitch level: /caveman lite|full|ultra|wenyan Stop: "stop caveman" or "normal mode". Two directives, one run-on. A blank line or a two-space hard break between them survives the formatter.Checked and clean
Saying so because the base rate in this PR has been in the fix commits, and silence about what I read is not the same as having read it.
filePatch's hunk grouping and both no-newline markers — the gap arithmetic is right and the ranges provably cannot overlap.withRepoLockand the staged-index refusal.restore.keep's first-wins against the base each hunk measures from.release()'s abandoned-versus-force semantics andJobFollow's twowasRunningguards. The phase carry-forward on both sides of the wire.compose_progress's rate differentiation, theDOWNLOAD_SETTLEDrebase andpullState.canonicalRef—library/implicit on Hub only, ports and digests intact.nextPage's cross-host refusal.localImages's bare-:latestaliasing.update.tsis reformatting only, no semantic change. The dev-rig scripts parse clean and are dev-only.Where this leaves the PR
The work is good and the last three rounds closed what they said they closed — I checked the tree rather than the summary this time, and it holds.
This
REQUEST_CHANGESrests on the CI job alone. Findings 1 and 2 I would want in before merge; 3 through 6 and every small one are fine as follow-ups, since none of them loses data or writes the wrong thing to the operator's files. Disagree with any of them and say so — I will drop it.The pinning issue is still mine to file, and I have not. That is not blocking anything here.
Round three fixed hostPath to normalise-then-check because a substring test for ".." refuses `env_file: ../shared/.env`-derived paths that are legal once resolved. composeEligibility kept the substring test on the same class of label-derived path — deciding whether the compose updater is used at all — and hand-built `${prefix}${f}` with an untrimmed prefix, which reproduces the `//srv/...` the same round fixed. Same rule in both guards now, prefix trimmed, and two tests: the legal `../` path is accepted, the escaping path is refused by name.Three commits since the fourth pass:
4efffd5,b92f637,ef7944c. The blocker and both pre-merge findings are closed; the deferrals are filed as #12. Re-requesting review.The blocker — and this time the run is the evidence
You were right on every level of it, including the level I missed twice:
verifyhad never executed a single check,buildwas skipped with it onneeds:, and merged as it was, the next push to main would have deployed nothing. Worse than the breakage — I added that job and never read one of its runs. Every "verify gates this" sentence I wrote in this PR's history was about a job that had only ever died at checkout.Fix taken from your #11, as offered:
.forgejo/deno.shcopied verbatim, credited in the commit message. The job runs on the runner, where node exists, and enters the pinned image per command; the image is read fromdocker/Dockerfile'sFROM, so the toolchain pin stays singular. One deviation from your usage: a single chaineddeno task ciinstead of a step per check, because eachdeno.shcall is a fresh container and separate steps would throw away thedeno install --frozenthree times over.Verified in the order that was missing:
verifysuccess in 53s, log carries the realok | 247 passed | 0 failed | 4 ignored,buildskipped on the PR-only gate as intended;The workflow comment now records why
container:must not come back.Findings 1 and 2
1,
loadJobsfreshness — taken, and your framing was the correct one: dropping the epoch fixed ordering, but coalescing never guaranteed freshness, and02c2886promotingjobsintoprojectBusy's input is what made that matter. Now one request runs, at most one more queues, and every caller gets a fetch that starts after its call — a queued-but-unstarted fetch is fresh for a new caller too, so it is shared rather than duplicated.2,
ALREADYvouching for the wrong repository — real, and a direct consequence of A's reorder removing the only comparison on that path. A repository-only guard now sits before theALREADYcheck; the full comparison stays below it, since the half-applied state's tags legitimately differ. One detail worth reporting because it cuts against this PR's pattern: my first attempt to land the guard silently missed its anchor, but the regression test I had added in the same change ran anyway and failed — red before the fix, green after, and the miss was caught by the test rather than by the next review.3 through 6 and the smalls
3 — done here rather than deferred:
composeEligibilitynormalises likehostPathnow, the prefix is trimmed, and there are two tests (sub/../compose.ymlaccepted,/../etc/passwdrefused by name). It was a five-line regression of an invariant this PR itself introduced; leaving it while its twin was fixed is how review rounds start.4 — the budget comment now says what the code does: bounds starts, does not interrupt a running item.
5, 6, the chooser bands,
rootOfcaching, the focus nit, and the confirm-queue extraction — filed as #12 with your review linked, and #11's pinning scope explicitly excluded so the two issues do not overlap.Also from your smalls:
release()appliesrecord()'sMAX_LINEScap, and the five agent-instruction files have their two directives on separate lines again, backtick-wrapped so the formatter keeps its hands off.State
b92f637andef7944care green on CI (#430, #431 — read, not assumed). 254 tests locally, 247 + 4 ignored in the image. Ourbuild.ymldelta remains verify-only, so whichever of #10/#11 lands second has a trivial conflict to resolve.The two open threads that are yours: the pinning issue, and this re-review.
Fifth pass, at
903ae7c(yourintegration/docker-overhaulwithorigin/mainmerged in, so #11's pinning is now in the tree here too). I ran the tree rather than the summary again.Everything the fourth pass rested on is genuinely closed, and this time I could check the one thing every prior round could not: CI is green on the actual commits. Runs #430
verifysuccess, #431 the push after, and #436/#437 on the merge head903ae7c— read, not assumed. The blocker that had never once executed a check now executes them and passes..forgejo/deno.shcopied from #11 is the right shape: on the runner, entering the pinned image per command, workspacedocker cp'd in.fmt/lint/checkclean here.I'm requesting changes on one item, and it is new — introduced by this PR's own change to
build.yml, not by any code under review before. It rests on that alone.Blocker —
verifyruns fork-supplied code on the deploy runner, ungatedThis PR gives
build.ymlapull_request:trigger and averifyjob (28 additions, the firstcontainer:-free CI job in the repo to shell into docker).verifyhas noif:. Its one step runs./.forgejo/deno.sh task ci, anddeno.shdrives the host docker daemon directly —docker create/cp/start/waitagainst whatever socket the runner exposes. On apull_requestevent,actions/checkoutlays down the PR head's copy ofdeno.shanddeno.json, so a pull request supplies the very script that then runs oncth-ubuntu-latest— the same runner that holdsPACKAGE_TOKENandDEPLOY_SSH_KEYon the next push to main.The reason I'm sure this is the intent-versus-tree gap and not me inventing a threat: your own
deps.yml, merged in #11, guards this exact case and writes down why —deps.ymldeemed the guard necessary for a job that runsdeno.sh.build.yml'sverifyrunsdeno.shand does not carry it. That asymmetry is the finding: the two workflows disagree about whether the same script is safe to run for a pull request.Fix is one
if:onverify, the same clause. Honesty about reachability: secrets are withheld from fork runs, so this is runner compromise (host-root code exec on the CI/deploy box, persist and harvest the next push's secrets), not direct secret theft; and whether a fresh fork PR runs without maintainer approval depends on the instance's Actions-approval setting, which I haven't audited. That setting is exactly whatdeps.ymlchose not to rely on. Do the same here — defense in depth and internal consistency both point one way.Confirmed closed from the fourth pass
verifyon the runner,container:gone, comment recording why it must not return. Green on #430/#431. This is the one I most wanted to see a real run of, and there is one.loadJobsfreshness —jobsTail/jobsQueuedgives every caller a fetch that starts after its call; a queued-but-unstarted fetch is legitimately shared.runTask's post-POST await now can't get a pre-POST snapshot.ALREADYvs. the wrong repository — repo-only guard sits above theALREADYcheck, full comparison stays below, andpin_test.tshas thenginx:1.27/httpd:1.27regression test. Reordering A did remove the only comparison on that path; this puts one back at the right altitude.composeEligibility— normalises segments likehostPath, trims the prefix, two tests (sub/../compose.ymlaccepted,/../etc/passwdrefused).release()appliesrecord()'sMAX_LINEScap; the five agent-instruction files have their two directives on separate backtick-wrapped lines again.New, non-blocking
a. The git/path tests are POSIX-only and fail on a non-POSIX dev box. On this Windows checkout,
deno task testis 299 passed, 4 failed — the threecommit_test.tsgit probes andcompose_update_test.ts's "a legal ../ path is accepted" eligibility case. Cause is path shape, not logic:rootOfreturnsC:/Users/…, the file paths carry\,file.startsWith(root + "/")misses so thediff --githeader keeps an absolute path andgit apply --cachedrejects it; andmakeTempDir()hands backC:\…, which failsstartsWith("/"). These run (git is present) rather than skipping, so they're red rather than ignored. On Linux/CI they pass — but the review loop for this repo happens on a Windows box, and "245 + 4 ignored in the image / 251 local" only held because the earlier local runs were POSIX. Guard them with aDeno.build.os === "windows"skip (same spirit as theignore: !hasGitgate already there), or normalise separators, so the tree is green where it's actually being reviewed.b.
checkUpdatescan starve its tail forever, not just overrun. The fourth pass settled that the 90s budget bounds starts — fine. Separate problem: the worker pool walks[...images]in fixedstate.stacksorder every pass with no rotation or resume cursor, so if the first dozen images sit on a slow/unreachable registry and burn the budget, images past the cutoff getprevious ?? "ran out of time"on every pass, indefinitely. The old serial loop was slow but always reached the end. A security update on a tail image would never surface. Rotate the start offset per pass, or order by least-recently-checked. Fits #12.c. Reuse the review agents flagged, all follow-up. Two registry v2 clients now (
tools/dep-check/remote.tsre-implementsupdates.ts's parse/bearer/paginate, and the copy lacks the same-host token guardlistTagshas); two tag-version parsers (dep-check/version.tsvstags.ts, PRERELEASE lists already drifted by one word);jobs.tsmerge()still hand-mirrorsupdate_feed.svelte.ts's carry-forward, which is the field-drift you and thisilike already patched in lockstep once; and the/container/:id/pinand/stack/:project/pinroutes carry ~190 lines of the same read→rewrite→validate→restore→commit pipeline, already subtly diverged. None writes wrong or loses data; all belong in #12 or a dedup pass, not here.State
Blockers-from-code: none — the four rounds closed what they claimed, verified against the tree and, finally, against green CI. This
REQUEST_CHANGESis theverifyfork guard alone. Add theif:and I'll clear it; a and b I'd fold into #12, c whenever. The pinning issue is still mine to file and is not blocking anything here.@ -20,0 +40,4 @@uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6- name: Install, format, lint, typecheck, testrun: ./.forgejo/deno.sh task civerifyhas noif:, and this step runsdeno.sh, which drives the host docker daemon (docker create/cp/start/wait). On apull_requestevent the checked-outdeno.shanddeno.jsonare the PR head's copies, so a pull request supplies the script that runs here — oncth-ubuntu-latest, the runner that holdsPACKAGE_TOKEN/DEPLOY_SSH_KEYon the next push to main.deps.yml(merged in #11) guards the identical case:Add the same clause to
verify. Secrets are withheld from fork runs, so the exposure is runner compromise rather than direct secret theft — but that's the box that deploys production, anddeps.ymlalready decided this script is not safe to run for an arbitrary pull request.fd07ab1adds theif:— the same clausedeps.ymlcarries, verified identical string-for-string, with the reason in a comment above it. Re-requesting review; that was the only thing this pass rested on.You're right about the asymmetry and about whose it is. I added the
pull_request:trigger tobuild.ymlso myverifyjob would run at all, and did it without carrying across the guard the workflow beside it already had for a job running the same script. The reachability caveats you list are fair — secrets withheld from fork runs, so runner rather than direct theft, and the approval setting unaudited — butdeps.ymldeclining to rely on that setting is exactly the precedent, and two workflows in one repo disagreeing about whetherdeno.shis safe for a pull request is not a state to merge.The other three are real and I'm not disputing any of them; they're going to #12 rather than into this PR, which has been open through five review rounds and is now large enough that each additional change costs another pass:
Deno.build.os === "windows"skip alongside the existingignore: !hasGit, same spirit.checkUpdates. Sharper than the budget item it came out of: fixed[...images]order every pass means a slow head permanently starves the tail, and a security update there never surfaces. The serial loop it replaced was slow but always finished. Rotation or least-recently-checked ordering.merge()hand-mirroring the client reducer, and ~190 duplicated lines across the two pin routes.Both are in #12 with your framing preserved.
Thanks for five passes of this. The count that matters to me: across them you found one blocker that would have stopped main deploying, one unreachable-
ALREADYbug my own test was shaped to miss, and four defects I introduced while fixing your earlier findings — and you retracted your own items twice when the tree disagreed with you. The CI one in particular I could not have caught by reading, because I never looked at a run.Sixth pass, at
fd07ab1. One commit since the fifth, ten lines, all comment plus theif:— checked against the tree and the runs, same standard as the last two rounds.The clause is right, and provably live. Diffed against
deps.yml's rather than eyeballed: identical string-for-string. The comment above it records the reason in the file, which is where the next person will look.build's own gate (push+refs/heads/main) is untouched, so main's deploy path takes the first disjunct and runsverifyunconditionally.The one thing the diff could not prove was the direction that matters most here: a clause that quietly evaluated false on same-repository pull requests would have removed
verifyfrom PRs entirely — the failure mode this thread already met once, wearing a different hat. So, runs rather than reasoning: #438 success in 56s and #439 success in 1m1s, both onfd07ab1, bothpull_requestevents. Those durations are executed checks, not an all-skipped pass — #430'sverifyran 53s. The gate admits this repository's PRs and the fork case is the only thing it excludes.The deferrals landed as stated. #12 now carries the Windows test failures, the
checkUpdatestail starvation, and the duplication list, framing intact, with #11's scope excluded so the issues do not overlap.One thread closes itself: the pinning issue I still owed you does not need filing. I checked the tree rather than my own to-do list: all four actions in
build.ymlare SHA-pinned with the# v6trailing comments dep-check reads, all three image refs indocker/Dockerfileare digest-pinned — including theCOPY --frombin ref that was in nobody's list of base images — and thessh-keyscanTOFU is gone, replaced by a committed, reviewableknown_hostswith the re-pin procedure written above the key. Everything that issue would have asked for is on this branch via the #11 merge, and the weekly checker watches it from here. Nothing left to file.Approving. For the record of what five rounds bought: a deploy pipeline that provably survives this merge (#430/#431, #436–#439, read not assumed), a job registry that no longer frees a slot under a live
compose up, a pin path exercised against real git including worktrees and CRLF, dialogs that cannot swallow a destructive answer, and two workflows that now agree about whetherdeno.shis safe for a pull request. The follow-ups have owners (#12 here, pinning watched by dep-check). This is ready.thisilike referenced this pull request2026-08-10 21:45:39 +02:00