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

Merged
thisilike merged 38 commits from integration/docker-overhaul into main 2026-08-10 21:39:08 +02:00
Owner

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-rigcore-uijobs-and-statuspinned-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.

  • ConfirmDialog is the union, not a pick. core-ui's Enter handler stays — it acts on what has focus and still covers the case where focus escaped both buttons, which deleting the handler does not — plus the two fixes from the dev-rig line: the focus effect is separate and tracks 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.
  • The docker pages were rewritten on the jobs branch from versions predating the load-error work, so taking that side dropped it. Re-applied: StacksPage, ContainerPage, StackDetailPage and OverviewWidget report a failed load instead of rendering their skeleton forever, and the two detail pages no longer rethrow inside .catch into a dangling promise.
  • deno.json is the union of all three lines: the vite range named in the task itself, --allow-sys=hostname and the docker socket from dev-rig, and /host/root from the pin branch, which the pin route writes through. check keeps the module backends, and lint defers to it.
  • mod.ts gets ContainerInfo and composeConfigArgs back. They were removed as unused on a branch that had no pin route; the pin route is what uses them.

Verification

deno fmt --check   231 files clean
deno lint          141 files clean   (was 10 problems across the stack)
deno task check    all five entrypoints
deno task test     241 passed, 0 failed
deno task build    exit 0

Lint reached zero by pinning the remaining unversioned jsr:@std/assert imports through the workspace, dropping an import in app.ts whose only use had been replaced, dropping a deno-lint-ignore in mod.ts that no longer suppressed anything, and switching no-slow-types off for @opsdeck/sdk with the reason recorded — it is a JSR publishing rule, the package is consumed through the import map, and the hono ContextVariableMap augmentation 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.yml on the host.

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. - **ConfirmDialog** is the union, not a pick. core-ui's Enter handler stays — it acts on what has focus and still covers the case where focus escaped both buttons, which deleting the handler does not — plus the two fixes from the dev-rig line: the focus effect is separate and tracks `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. - **The docker pages** were rewritten on the jobs branch from versions predating the load-error work, so taking that side dropped it. Re-applied: `StacksPage`, `ContainerPage`, `StackDetailPage` and `OverviewWidget` report a failed load instead of rendering their skeleton forever, and the two detail pages no longer rethrow inside `.catch` into a dangling promise. - **`deno.json`** is the union of all three lines: the vite range named in the task itself, `--allow-sys=hostname` and the docker socket from dev-rig, and `/host/root` from the pin branch, which the pin route writes through. `check` keeps the module backends, and `lint` defers to it. - **`mod.ts`** gets `ContainerInfo` and `composeConfigArgs` back. They were removed as unused on a branch that had no pin route; the pin route is what uses them. ### Verification ``` deno fmt --check 231 files clean deno lint 141 files clean (was 10 problems across the stack) deno task check all five entrypoints deno task test 241 passed, 0 failed deno task build exit 0 ``` Lint reached zero by pinning the remaining unversioned `jsr:@std/assert` imports through the workspace, dropping an import in `app.ts` whose only use had been replaced, dropping a `deno-lint-ignore` in `mod.ts` that no longer suppressed anything, and switching `no-slow-types` off for `@opsdeck/sdk` with the reason recorded — it is a JSR publishing rule, the package is consumed through the import map, and the hono `ContextVariableMap` augmentation 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.yml` on the host.
`deno task check` and the unit tests cannot see the defects that matter most
in the docker module — a progress bar that walks backwards, a spinner that
never stops, a compose file rewritten wrongly. Those need a real engine, real
compose files and a real pull.

This is one privileged throwaway container running its own dockerd, with
OpsDeck nested inside it using the production mounts. The nesting is the
point: only then does `chroot /host/root` land somewhere that is not our own
mount namespace, and only then is the compose path taken at all — otherwise
hostexec's probe refuses every strategy and the engine-API fallback runs
instead, leaving `--progress json` and the pin route untested.

Two consequences of standing a container up as a host, both handled in the
entrypoint: `/` is remounted rshared so the nested bind has something to
propagate from, and the container markers are deleted so the probe believes
the chroot left the container.

Includes a hardcoded OIDC provider, because OPSDECK_AUTH=disabled injects a
static admin and leaves every role-dependent path untestable.
Split out of the docker work so the module change can be read on its own.
Nothing here is docker-specific:

- HealthDot carries a lifecycle glyph. A dot has three colours and a
  container has more states than that — paused, restarting and finished
  are not the same news, and colour alone cannot say which.
- Modal gets room for a log, ConfirmDialog acts on what has focus rather
  than always confirming, and a long message scrolls instead of pushing
  the buttons off screen.
- The live channel and the SSE broadcast gate per-topic on the requester's
  role, so a viewer's socket no longer carries admin-only payloads.
- Notifications get a stable order within a millisecond. Two written in the
  same tick came back in either order, which made a test flake one run in
  six and would have reordered a burst in the UI.
- `deno task check` walks the module backends, not just the server graph;
  the ContextVariableMap augmentation moves to the SDK so a module backend
  sees the same `user` type the server does. That immediately surfaced a
  Timeout-vs-number in the docker module, fixed here.
Three things the module got wrong, all of which show up the moment an update
takes longer than a click.

**An update was not addressable.** Progress went out on an at-most-once
stream and nowhere else, so dismissing the modal, switching tabs or reloading
lost it — and the "done" that ends a run can simply not arrive. The server
now keeps each run in a registry with its progress and log, hands out a job
per project atomically, and answers `/jobs` and `/jobs/:project`; the pages
re-attach to what is already running instead of guessing. One primitive
(`job_follow.svelte.ts`) does that for all three of them.

**Compose progress was appended verbatim.** `--ansi never` makes compose
print one line per redraw frame — about 3000 for a 3 GB pull — which is why
the log was unreadable. `compose_progress.ts` parses those frames back into
the keyed entity events the redraw was expressing, so a layer replaces its
own row and the UI can draw bars, rates and an aggregate. Byte totals only
exist in `--progress json`, which is probed for and used where available.

**A stopped container was reported as a fault.** A one-shot that exits 0 by
design — a migration, a seeder — is finished, not broken, and marking it red
meant any stack with an init container never looked healthy. `status.ts`
identifies one by what compose says about it (a sibling waiting with
`service_completed_successfully`), and the same rule now rolls up to a stack
health dot on the card and the detail page.

The pin work rides on top of this and lands separately.
A pinned tag cannot be updated by pulling — `nginx:1.25` is `nginx:1.25`
forever — so the module reported "up to date" for images with years of
releases behind them. Answering "is there a newer version" means reading the
registry's tag list, and applying the answer means editing the operator's
compose file.

Both halves are here:

- `tags.ts` parses versions and offers what exists at each component, scoped
  by OPSDECK_MOD_DOCKER_UPGRADE_SCOPE (patch by default, because applying one
  rewrites a file). `updates.ts` lists tags once per repository, paginated,
  and only for images whose tag looks like a version at all.
- The pin route rewrites the one line and commits just that line. The file is
  the operator's and may hold work in progress, so `git commit <file>` is not
  an option: it commits the working tree copy and would sweep their edits in.
  `commit.ts` applies a one-hunk patch to the index instead, serialised per
  repository, and declines with a reason if the index is already dirty.

The route is ordered to fail safely: validate the tag against the registry
first, keep the original text in hand, prove the edited file still parses
before anything is started, and put the original back if it does not. The
chooser is per image rather than one button — a database may be worth a patch
and nothing more while a sidecar can take the major — and its rules live in
`pin_model.ts`, where they can be tested without clicking.

This is why the container's write permission now covers the host mount, which
docs/security.md states rather than leaving implied.
The keydown handler was bound to window and confirmed on Enter whatever
had focus, calling preventDefault() so the focused button never saw the
key itself. That defeated the danger variant's focus-on-Cancel: the
comment there says the dangerous button should never be one stray Enter
away, and it was exactly one.

Behind a queue of prompts it is worse than one. Each answered prompt
mounts the next, so a held Enter drains the queue at the key repeat
rate — one destructive action per repeat, none of them read.

Drop the branch. A focused button activates on Enter natively, which is
the documented behaviour without the reach: Enter confirms when Confirm
has focus, and cancels on the danger variant, where Cancel has it.
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.
Nine defects, all of them cases where the rig said one thing and did
another.

The exported app image landed in the production build context and
.dockerignore did not exclude it (docker/dev-rig/.gitignore has no
bearing on a build context). Every run baked the previous run's image
into the image being built, and its always-different bytes meant `COPY
. .` never cache-hit, so deno install, build and check ran cold on every
start. The Dockerfile's claim that what runs here is the artifact we
ship was false: it shipped a copy of itself.

The stub IdP readiness loop logged on timeout and carried on. OpsDeck
then started against a dead issuer, /api/core/health answered 200
because it is registered before the auth middleware, and up.sh printed
"ready" for a rig where every login 503s. It exits now, like the dockerd
loop next to it.

The stub only implemented authorization_code while advertising and
issuing refresh tokens. OpsDeck re-validates every session against the
IdP each OPSDECK_SESSION_REVALIDATE_MINUTES, so at 15 minutes the grant
answered invalid_grant, the session was deleted, and a role test died
looking like an OpsDeck auth bug. Adds the grant with rotation, and
RIG_REVALIDATE_MINUTES so the path is testable in seconds rather than
quarter-hours.

`exec docker logs -f` made the log follower PID 1, so docker stop
SIGKILLed dockerd and the nested DuckDB mid-write — and the retained
volume carried that torn database into the next run. There is a trap
now. The budget is shaped by measurement: OpsDeck closes its database
and exits 0 in about 150ms, several seed containers ignore SIGTERM
outright, and a full dockerd teardown does not fit in the outer ten
seconds at all. Waiting on it is what got the rig killed at 137; a
bounded wait brings the whole shutdown in at ~3s.

login.sh used no -f and checked nothing, so a failed login exited 0 with
an empty jar and every follow-up "viewer" check measured an anonymous
request instead. Each leg is checked, and the jar has to hold a session
cookie.

The nested engine ran on vfs, which has no copy-on-write and materialises
~1.6GB just to load the app image; it is overlay2 now, falling back to
vfs, and keeping whichever driver an existing volume was written with
rather than silently orphaning its images. python:3.12-slim — the image
the web stack exists to make progress bars watchable with — was missing
from the pre-pull list and was fetched inline on the readiness path.
Pulls and compose runs are concurrent, the wait is 420s and configurable,
and it fails immediately if the rig has exited.

/srv/stacks gets a volume, so it no longer resets while the engine that
built its containers persists, and --reset drops the set together.
`docker load` re-tagged rig-base every run and orphaned the previous
image with nothing to prune it.

The rig image's OPSDECK_* block was dead — the entrypoint runs OpsDeck
as a nested container with its own -e flags — and its comment was
inverted: hostexec only considers chroot when the host root is not "/",
so HOST_ROOT=/ is precisely what reduces the probe to ["direct"].

Docs last, same class of defect. CLAUDE.md and the README described
`--progress json`, a pin route and /api/mod/docker/jobs, none of which
exist on main; the flag is `--ansi never` and the 403 example now uses
/stack/:project/files, which is genuinely admin-only. The seed-stack
table no longer claims rendering that is not implemented — exited maps
to serious, restarting to unknown, and there is no stack-level rollup.
Addresses the review on #2. Four of its points were already covered by
the previous commit (IdP exit 1, the dead OPSDECK_* ENV block, the
python:3.12-slim pre-pull, the volumes surviving `docker rm -f`); this
is the rest.

`mount --make-rshared /` was a warning. It is the one precondition the
whole rig rests on — without the propagation the nested rslave bind
quietly shows the wrong thing and every conclusion drawn from the rig is
worthless — so it exits.

The nested run passed OPSDECK_AUTH twice, betting on docker's
last-flag-wins for as long as that holds, mounted all of /certs
including the CA's private key when OpsDeck needs exactly ca.pem, and
left OPSDECK_HOST_ROOT riding on an image default while setting its two
siblings explicitly. One argument array now, built once, with AUTH set
in exactly one branch. The host-root bind still exposes /host/root/certs
— a real host has its secrets on it too — but nothing hands the key over
a second time.

The log follower reattaches instead of ending with the nested container,
so `docker exec opsdeck-rig docker restart opsdeck` no longer takes the
rig down with it. The README documented that wart; now it documents that
it works.

up.sh rejects unknown arguments instead of treating `-rerun` as a
ten-minute full build, skips re-exporting a tar that --rerun cannot have
changed, and publishes 9443 only under RIG_OIDC. down.sh removes the
container and every volume it owns, which is what makes "disposable"
true; --reset is the same cleanup followed by a build.

The stub's /role mutated on GET, so pasting the URL into a browser
decided who the next login was; it is POST to write, GET to read. Its
doc comment now says which of the things discovery advertises are
deliberately unchecked (client secret, client_id, redirect_uri), since
the comment already inventoried what IS checked.

login.sh checks its arguments and finishes on /api/core/me — behind the
auth middleware and open to every role, unlike /api/core/health, which
answers 200 to anybody and would have proved nothing.

Dockerfile: the apt line hardcoded bookworm while the base is whatever
production ships; it reads VERSION_CODENAME, as the same line already
does for the architecture.

Verified on a cold rig (no volumes): overlay2, only 8080 published,
hostExec chroot with hostRoot=/host/root, all three stacks up. Under
RIG_OIDC: one OPSDECK_AUTH, ca.pem the only file in the nested /certs,
GET /role read-only, login.sh reporting mapped roles, viewer 403 and
admin 200 on /stack/web/files, and the rig surviving a restart of the
nested container.
`deno task check` failed on any Deno newer than the 2.5.6 the image pins:
two TS2322s for `Type 'Timeout' is not assignable to type 'number'`. With
npm dependencies in the graph the Node typings win, so setInterval and
setTimeout return a Timeout, not a number — and the annotations said
number.

It passed in CI only because the Dockerfile builds on the pinned version,
which meant the documented `deno task check` failed for a contributor on
current Deno, in files they had not touched. ReturnType<typeof setInterval>
follows the call instead of asserting a shape, so it is right on both.
`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.
Three things were held in place by the lockfile rather than by anything
that says so.

Six test files imported `jsr:@std/assert` with no version at all (four
others pinned @1). Whatever the lock captured is what they got, and a
fresh resolve takes the next major. It is a workspace import now, pinned
^1, imported by bare specifier — which is what deno lint's
no-unversioned-import and no-import-prefix were both pointing at.

`deno task dev`, `build:shell` and `build:shared` ran `npm:vite` with no
range. Inside the workspace that resolves to 7.3.6 through the "vite"
import-map entry; from anywhere else it resolves to 8.2.1, and CLAUDE.md
records that Vite 8 fails under Deno for want of node:util parseEnv. The
pin was real but it lived in an entry nothing imports by bare specifier,
so it reads as dead weight. The tasks name the range themselves now.

`deno install` in the image is `--frozen`: the container is supposed to
be the lockfile made real, and without it a stale lock is resolved past
instead of failing the build.

Also here, since they are the same file: `start` and `dev:server` had
drifted from the CMD the image runs. They lacked --allow-sys=hostname
and write access to the docker socket, both of which the shipped command
has. Deno.hostname() is try/caught at both call sites, so it degraded
silently rather than crashing — the hardware module reported the host as
literally "server", and docker's self-container detection returned null.

And `deno fmt` now skips html/css/svg. Deno's formatting of those is not
stable across versions: 2.5.6 (the image, and now CI) and 2.9.x disagree
on three files in opposite directions, one wanting `<!doctype html>` and
the other `<!DOCTYPE html>`. Left in, `deno fmt --check` could only ever
pass on whichever version last ran `deno fmt`. TypeScript and Svelte
formatting agree across both.
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 workflow ran checkout, build, push, deploy — and nothing else. No
deno fmt --check, no deno lint, no deno test. Typecheck happened only as
a side effect of the image build, on main, after a full install and
build. So nothing was enforced, and it showed: 85 of 220 files were
unformatted and deno lint reported 23 problems.

It also only ran on push to main, meaning a pull request got no signal
at all until after it was merged and deployed.

A verify job now runs the four checks, and build depends on it but is
gated to push-on-main, so a PR is verified and stops there. Verify runs
in denoland/deno:2.5.6 — the same version docker/Dockerfile builds with,
deliberately: a toolchain difference between where code is written and
where it is validated is its own class of failure, and this repo already
had two of them (the setInterval typings, and deno fmt disagreeing with
itself on html and css).
Mechanical, no behaviour. 85 of 220 files had drifted because nothing
ever checked — the CI job added in the previous commit is what stops it
happening again.
Introduced two commits ago and never declared: the page assigned
`loadError` in .then and .catch and branched on it in the template, but
there was no `let loadError = $state(...)`. Svelte compiles a free
identifier straight through as a global reference, so `/m/zfs` threw
ReferenceError on first render and displayed nothing at all — on the
page the change was meant to make more robust.

Nothing in the pipeline sees this. The module build succeeds, and
deno check, deno lint and deno fmt --check do not read Svelte template
scope, so the verify job added in this branch would have passed it too.
A free identifier does survive minification (the minifier cannot rename
a global), so `grep loadError packages/shell/dist/modules/*/index.js`
returning nothing is a real, if narrow, check — it is empty now.

svelte-check would catch the whole class properly, but it reports 203
errors and 21 warnings on this repo today, nearly all of them module
resolution it cannot follow through Deno's import map. That is a
separate piece of work, not a drive-by.
The three detail pages set `loadError` in .catch but only ever assigned
`data` in .then, so the error was sticky. All three reload on a timer
(liveRefresh at 30 s) and on live-channel resync, which makes the
failure mode worse than a one-off: one transient 502 from a proxy
restart pins "failed to load" on screen permanently while fresh data
arrives underneath it every 30 seconds, until the user hard-reloads.

GpuPage and DemoPage fetch exactly once today so they cannot hit it, but
they get the same reset rather than leaving the trap set for whoever
adds a poll.
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.
projectBusy() resolved the in-flight flag through the current container
list: does any container in this project have updatingC[c.id] set. But
updatingC is keyed by the pre-update container id and a successful
recreate mints a new one, while load() runs on the 30 s poll and on
every docker:stacks push — including the ones the update itself
provokes. Once that refresh lands, the old id is gone from `data` and
projectBusy returns false even though the server still holds
updating.has(project).

So the gate opened mid-update: sibling update buttons re-enabled, and a
queued depends_on prompt for another container in the same project was
released. The user read the destructive warning, accepted it, and got
409 "update already running" with nothing done — which is the outcome
the queue exists to prevent.

The project is captured in updatingProjectOf when the update starts,
which is the last moment the association is resolvable, and projectBusy
reads that instead of the live list. The confirmation queue uses the
same helper rather than repeating the lookup.
test(notifications): stop the ordering assertion racing the clock
Some checks failed
Build and Deploy / verify (pull_request) Failing after 14s
Build and Deploy / build (pull_request) Has been skipped
cbf044965e
"create, list, dismiss round-trip" asserts newest-first, but created_at
is Date.now() at millisecond resolution and both creates routinely land
in the same millisecond, leaving ORDER BY created_at DESC to break the
tie however DuckDB feels. Measured at one failed run in six across the
full suite — enough to make the verify job added in this branch
intermittently red, which is how a CI signal gets ignored.

Pre-existing, and not something the review turned up; it surfaced while
re-running the suite. Fixed in the test rather than the query because
which of two same-millisecond notifications is "newer" is genuinely
undefined, and inventing a tiebreaker in SQL would assert a fact the
data does not carry.

55 consecutive full-suite runs since. One earlier failure in that window
went unattributed — its output was discarded — and did not reproduce, so
this closes the measured flake, not necessarily every flake.
Six defects from a review of c5a8c5c, all in the new job machinery.

`claimed` was a latch, not a window. It marks the gap between our own POST
and the server registering the run, so that a 404 in that gap reads as
"not yet" rather than "over" — but nothing reliably closed it. `adopt()`
left it set when the id was null, and the container page never calls
`adopt` at all. Sleep a laptop past the finished-job TTL, lose the `done`
event to at-most-once delivery, and every later sync 404s without ever
clearing: modal spinning, button disabled, until a reload. It now expires
30 s after start() — a round trip's worth, since every update route calls
jobs.start() before it responds — and adopt() closes it whatever the id.

The container page called job.start() before its POST, and start() resets
the feed. If the page had adopted a stack-wide run via sync, a 409 (the
depends_on ack, or "update already running") wiped that run's hydrated
rows and left accepts() rejecting its events. It only starts, clears,
opens the modal and writes "failed to start" into the feed when the run
is its own; otherwise the failure goes to actionError alone.

merge() dropped `phase` on terminal frames, which arrive without one, so
a finished layer went phase-less and the pre-rollup fallback in
downloadLayers() counted its extract (uncompressed) bytes as a download —
the compressed/uncompressed mixing PullPhase warns about. The client
reducer is the documented mirror of that function and had the same bug;
fixing only one side would have been worse than neither, because sync()
re-hydrates every 15 s and completed layers would have flipped between
the two readings, jumping the byte pair and the per-layer bar colour.

hydrate() zeroed the monotonic pull-fraction guard on every call, and
sync() calls it every 15 s while a job runs, so "never decreases within
one run" held for 15 s at a time. It takes the run id now and only starts
over for a different run — reset() and start() still clear it outright.

/jobs and /jobs/:project are both admin-only, and a viewer's page asked
on mount, on every docker:stacks push, and every backstop tick, forever.
The first 403 stops the asking. That latch lives in JobFollow rather than
the page, so sync() is covered for every caller, not just the two here.

Also drops three unused imports from mod.ts (ContainerInfo,
fetchRemoteDigest, composeConfigArgs), which were the file's five lint
errors. composeConfigArgs is not dead: its callers are the pin route on
feat/docker-pinned-versions, one branch up. Rebasing that branch will
conflict in this import block — take its side.
The container pin route put files back with a swallowed write and then
told the operator "files restored" either way. Its twin, the stack pin
route, already did this properly — collecting the files it could not
restore, logging each, and appending " — COULD NOT RESTORE: …" to the
error — with a comment recording exactly why: a read-only mount or an
ownership change has to be said out loud, or the file keeps a tag
compose rejected and the next `compose up` uses it.

That logic is now one helper both routes call, rather than one good copy
and one bad one, with the keep-the-FIRST-text rule that the stack route
depends on: a second service edited in the same file reads text that
already holds our first edit, and restoring that leaves the file half
applied.

Five tests, on a path that had none — including the one that matters,
where the restore write itself fails.
fix: resolve the stack's collisions and green the integrated tree
Some checks failed
Build and Deploy / verify (pull_request) Failing after 15s
Build and Deploy / build (pull_request) Has been skipped
ab960d2fb7
The four branches fixed several of the same things independently, so the
merges needed decisions rather than markers.

ConfirmDialog is the union, not a pick. core-ui's Enter handler stays —
it acts on what has focus and still covers the case where focus escaped
both buttons, which simply deleting the handler does not — and the two
fixes from the dev-rig line go on top: the focus effect is separate and
tracks `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.

The docker pages were rewritten on the jobs branch from versions that
predate the load-error work, so taking that side dropped it. Re-applied:
StacksPage, ContainerPage, StackDetailPage and OverviewWidget report a
failed load instead of rendering their skeleton forever, and the two
detail pages no longer rethrow inside .catch into a dangling promise.

deno.json is the union of all three lines: the vite range in the task
itself, --allow-sys=hostname and the docker socket from dev-rig, and
/host/root from the pin branch, which the pin route writes through.
`check` keeps the module backends the core-ui branch added, and `lint`
now defers to it rather than naming one entrypoint.

mod.ts gets ContainerInfo and composeConfigArgs back: they were removed
as unused on a branch with no pin route, and the pin route is what uses
them.

Lint clean for the first time across the whole stack — the remaining
unversioned jsr:@std/assert imports are pinned through the workspace,
app.ts loses an import whose only use was replaced, and mod.ts loses a
deno-lint-ignore that no longer suppressed anything. no-slow-types is
switched off for @opsdeck/sdk with the reason recorded: it is a JSR
publishing rule, the package is consumed through the import map, and the
hono augmentation it objects to is deliberate.

deno fmt over the whole tree.
julian requested changes 2026-08-09 23:49:00 +02:00
Dismissed
julian left a comment

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):

  1. hostPath() sanitization is a .. substring check and nothing else — symlinks are not rejected, and the patch path in commit.ts bypasses the check entirely (inline comments).
  2. The fire-and-forget update-route pattern is pasted three times with .catch(() => {}) swallowing background refresh failures (inline comment).
  3. StacksPage has grown into a ~1200-line component with racy job/confirm state (inline comments).
  4. DependentsConfirm's busy prop 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 state field 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/Dockerfile bases (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.

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): 1. `hostPath()` sanitization is a `..` substring check and nothing else — symlinks are not rejected, and the patch path in `commit.ts` bypasses the check entirely (inline comments). 2. The fire-and-forget update-route pattern is pasted three times with `.catch(() => {})` swallowing background refresh failures (inline comment). 3. StacksPage has grown into a ~1200-line component with racy job/confirm state (inline comments). 4. `DependentsConfirm`'s `busy` prop 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 `state` field 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/Dockerfile` bases (`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.6
steps:
- name: Checkout Code
uses: actions/checkout@v6
Owner

actions/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).

`actions/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-secret
Owner

Client secret rig-secret is hard-coded into the run arguments (visible via docker inspect). Dev-only, but make it ${IDP_CLIENT_SECRET:-rig-secret} so runs can override it and the literal stops being load-bearing.

Client secret `rig-secret` is hard-coded into the run arguments (visible via `docker 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") {
Owner

POST /role is 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 explicit RIG=1-style env var is set, or bind only to the rig-internal interface.

`POST /role` is 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 explicit `RIG=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 \
Owner

${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.

`${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);
Owner

This patch path is built by direct string concatenation with ctx.paths.hostRoot and never goes through hostPath(), so it skips even the .. check that route paths get. The git root comes from rev-parse --show-toplevel, which is mostly trustworthy, but apply the same guard here (or export hostPath() and use it) so there is one choke point for host writes.

This patch path is built by direct string concatenation with `ctx.paths.hostRoot` and never goes through `hostPath()`, so it skips even the `..` check that route paths get. The git root comes from `rev-parse --show-toplevel`, which is mostly trustworthy, but apply the same guard here (or export `hostPath()` 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;
}
Owner

On a phase change the byte counter is rebased to zero but r.at is 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. Reset r.at = at on the phase boundary before computing dt.

On a phase change the byte counter is rebased to zero but `r.at` is 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. Reset `r.at = at` on the phase boundary before computing `dt`.
@ -247,0 +631,4 @@
key: "pull/summary",
id: "pull",
status: "progress",
state: "active",
Owner

The summary event's state is hard-coded "active", including on the terminal heartbeat. The client never reads it (pullSettled is 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.

The summary event's `state` is hard-coded `"active"`, including on the terminal heartbeat. The client never reads it (`pullSettled` is 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;
Owner

Third copy of the same fire-and-forget job + finally cleanup pattern (also at ~1063 and ~1315), each ending in .catch(() => {}). A failed refreshStacks()/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.

Third copy of the same fire-and-forget job + `finally` cleanup pattern (also at ~1063 and ~1315), each ending in `.catch(() => {})`. A failed `refreshStacks()`/`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 ..");
Owner

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 (lstat each component or at minimum the final path) and assert the path is absolute before prefixing hostRoot.

`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 (`lstat` each component or at minimum the final path) and assert the path is absolute before prefixing `hostRoot`.
@ -0,0 +57,4 @@
}
/** -1, 0, 1 by numeric component, shorter-but-equal sorting first */
function compare(a: number[], b: number[]): number {
Owner

Missing version components compare as -1, so 1.2 < 1.2.0 and 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.

Missing version components compare as `-1`, so `1.2` < `1.2.0` and 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,
Owner

busy is 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.

`busy` is 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.
Owner

This filter silently drops queued confirm entries when the server clears updateAvailable between the user confirming and the update completing — a load() racing an in-flight updateContainer() 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.

This filter silently drops queued confirm entries when the server clears `updateAvailable` between the user confirming and the update completing — a `load()` racing an in-flight `updateContainer()` 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) => {
Owner

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 on jobs, which can leave starting[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.

`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 on `jobs`, which can leave `starting[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.
chore: add the Forgejo MCP server to the project config
Some checks failed
Build and Deploy / verify (pull_request) Failing after 6s
Build and Deploy / build (pull_request) Has been skipped
55bec09403
`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.
docs: attach two JSDoc blocks to the declarations they describe
Some checks failed
Build and Deploy / verify (pull_request) Failing after 6s
Build and Deploy / build (pull_request) Has been skipped
f8c59fc018
Both were left stranded when something was inserted between a comment
and its subject, and TypeScript attaches only the last block before a
declaration.

In the SDK, adminOnly() went in between provide()'s documentation and
provide itself, so adminOnly showed provide's docs' neighbour and
provide — the method external module authors most need on hover — showed
none. adminOnly moves above its own block instead.

In sseHandler the second block simply displaced the first, which is the
one explaining that admin-only topics are dropped for non-admins. Merged
rather than reordered, because the displaced text and the new text are
about the same parameter.

The new text was also wrong for half the signature: it said
adminOnlyTopics is "read on every send, not copied", but only the
ReadonlySet path is live — an array is copied into a Set, so later
additions to it are not seen. main.ts passes the shared Set, so the
behaviour was right and only the description was misleading; it now says
which overload does which, and why the shared Set is the one to pass.
fix(docker): restore the confirm-queue gating the merge dropped
Some checks failed
Build and Deploy / verify (pull_request) Failing after 16s
Build and Deploy / build (pull_request) Has been skipped
02c28864aa
Review on #10 found DependentsConfirm's `busy` prop dead — no caller
passes it. It was the visible end of a bigger regression: resolving the
StacksPage conflict by taking the jobs-branch rewrite and re-applying
only the load-error work silently reverted six fixes that had landed on
build/dev-rig. `confirming`, `answerConfirm`, `busy=`, `projectBusy`,
`updatingProjectOf` and `isWaiting` were all gone, and
`confirmUpdate = confirmQueue[0]` was ungated again — so confirming two
containers in one stack put the second POST straight into "update
already running" after the human had accepted a destructive prompt.

Restored against the new architecture rather than reverted onto it. The
jobs branch has a server-side registry, so `projectBusy` now reads
`jobFor(project)` — the server's own view, which survives the container
id changing under a recreate — with `starting` covering the window
before the job is registered. That makes the old `updatingProjectOf`
map unnecessary; it existed only to remember a project across the id
change.

Both dialogs are serialised: one answer per prompt, `busy` held while
the acknowledged update starts, and a deferred prompt says so in its row
instead of going quiet.

Also from the review: the pull summary's `state` was hard-coded
"active", including on the terminal heartbeat, so it now derives from
whether every layer has settled. The client reads `.pull` rather than
this, but a field that lies is a trap for whoever reads it next.

And `hostPath` was three copies of the same helper, guarding `..` only.
One definition now, at register scope, refusing anything not absolute as
well — a relative label path would resolve against the process cwd
instead of the host root. Every route that writes the operator's compose
files goes through it.
Author
Owner

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

busy prop 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 on build/dev-rig: confirming, answerConfirm, busy=, projectBusy, updatingProjectOf and isWaiting. 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: projectBusy now reads jobFor(project), the server's own view, which survives the container id changing under a recreate. That makes the old updatingProjectOf map unnecessary — it existed only to remember a project across that id change. starting covers 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 state hard-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 = at is already there — line 557 in the !samePhase branch, 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. onconfirm removes the entry from confirmQueue synchronously before the update starts, so a racing load() 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, -1 padding blocks three-component tags. The outcome is real but the mechanism is not: findUpgrade and listUpgrades both reject p.nums.length !== from.nums.length before compare ever sees mismatched lengths, so the -1 branch 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. configFiles comes from the com.docker.compose.project.config_files label. Setting a label requires access to the docker socket, which on this host is root-equivalent, and the route is requireRole("admin") on top — so "anyone who can deploy a stack controls them" describes someone who is already root, and lstat gates 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=1 guard. It is started only by the rig entrypoint, inside a container that deletes its own /.dockerenv to pretend to be a host. "If it is ever copied into a real deployment" is a hypothetical that applies to every file under docker/dev-rig/; guarding this one implies the others are safe to copy.

C108, rig-secret literal. 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 are opsdeck-admin and opsdeck-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.

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 **`busy` prop 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 on `build/dev-rig`: `confirming`, `answerConfirm`, `busy=`, `projectBusy`, `updatingProjectOf` and `isWaiting`. `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: `projectBusy` now reads `jobFor(project)`, the server's own view, which survives the container id changing under a recreate. That makes the old `updatingProjectOf` map unnecessary — it existed only to remember a project across that id change. `starting` covers 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 `state` hard-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 = at` is already there — line 557 in the `!samePhase` branch, 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.** `onconfirm` removes the entry from `confirmQueue` synchronously *before* the update starts, so a racing `load()` 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, `-1` padding blocks three-component tags.** The outcome is real but the mechanism is not: `findUpgrade` and `listUpgrades` both reject `p.nums.length !== from.nums.length` before `compare` ever sees mismatched lengths, so the `-1` branch 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.** `configFiles` comes from the `com.docker.compose.project.config_files` label. Setting a label requires access to the docker socket, which on this host is root-equivalent, and the route is `requireRole("admin")` on top — so "anyone who can deploy a stack controls them" describes someone who is already root, and `lstat` gates 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=1` guard.** It is started only by the rig entrypoint, inside a container that deletes its own `/.dockerenv` to pretend to be a host. "If it is ever copied into a real deployment" is a hypothetical that applies to every file under `docker/dev-rig/`; guarding this one implies the others are safe to copy. **C108, `rig-secret` literal.** 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 are `opsdeck-admin` and `opsdeck-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.
julian requested changes 2026-08-10 10:53:37 +02:00
Dismissed
julian left a comment

Second pass, branch checked out at 02c2886. All four verification claims re-run locally and all four hold: deno fmt --check 232 files clean, deno lint 141 files clean, deno task check all five entrypoints, deno task test 241 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.busy is 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.
  • The pull summary's state is 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:

  • Blocker 2 — the fire-and-forget update-route pattern is still pasted three times, each ending in .catch(() => {}): mod.ts:1080, mod.ts:1332, mod.ts:1784. A failed refreshStacks()/refreshUpdates() there still leaves stale state until the next scheduled run, and the three copies still have to be kept identical by hand.
  • Blocker 3 — StacksPage. It grew to 1288 lines, and loadJobs() still has no in-flight guard (inline).
  • Blocker 1 is only half done. The .. 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.ts still concatenates ctx.paths.hostRoot by hand (inline on both).
  • Minors: CI action SHAs, Dockerfile digest pins, dev-rig stub/secret hardening, compare() rationale, and the confirm-queue drop in load() — 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:

} else if (!samePhase) {
  r.at = at;
  r.bytes = p.current;
  r.speed = 0;
}

r.at was reset. Disregard that item; I misread the hunk.

New this round

The confirm-queue restoration in 02c2886 is the right shape, but it introduces a prompt-identity problem across projects (inline on StacksPage.svelte), and projectBusy is declared above the jobFor it calls (inline).

The one that matters most

DELETE /jobs/:project releases 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 concurrent docker compose up. Details inline on mod.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.

Second pass, branch checked out at `02c2886`. All four verification claims re-run locally and all four hold: `deno fmt --check` 232 files clean, `deno lint` 141 files clean, `deno task check` all five entrypoints, `deno task test` **241 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.busy` is 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. - The pull summary's `state` is 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: - **Blocker 2 — the fire-and-forget update-route pattern is still pasted three times**, each ending in `.catch(() => {})`: `mod.ts:1080`, `mod.ts:1332`, `mod.ts:1784`. A failed `refreshStacks()`/`refreshUpdates()` there still leaves stale state until the next scheduled run, and the three copies still have to be kept identical by hand. - **Blocker 3 — StacksPage.** It grew to 1288 lines, and `loadJobs()` still has no in-flight guard (inline). - **Blocker 1 is only half done.** The `..` 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.ts` still concatenates `ctx.paths.hostRoot` by hand (inline on both). - Minors: CI action SHAs, Dockerfile digest pins, dev-rig stub/secret hardening, `compare()` rationale, and the confirm-queue drop in `load()` — 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: ```ts } else if (!samePhase) { r.at = at; r.bytes = p.current; r.speed = 0; } ``` `r.at` was reset. Disregard that item; I misread the hunk. ### New this round The confirm-queue restoration in `02c2886` is the right shape, but it introduces a prompt-identity problem across projects (inline on `StacksPage.svelte`), and `projectBusy` is declared above the `jobFor` it calls (inline). ### The one that matters most `DELETE /jobs/:project` releases 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 concurrent `docker compose up`. Details inline on `mod.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.6
steps:
- name: Checkout Code
uses: actions/checkout@v6
Owner

You 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.ymlactions/checkout and the docker/* actions to full SHAs with a # v6 trailing comment;
  • docker/Dockerfile@sha256: digests alongside denoland/deno:2.5.6, denoland/deno:bin-2.5.6 and debian:bookworm-slim;
  • a line in docs/security.md naming 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.

You 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/checkout` and the `docker/*` actions to full SHAs with a `# v6` trailing comment; - `docker/Dockerfile` — `@sha256:` digests alongside `denoland/deno:2.5.6`, `denoland/deno:bin-2.5.6` and `debian:bookworm-slim`; - a line in `docs/security.md` naming 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 @@
{
Owner

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.cloud and $HOME/.config/forgejo-mcp/token, so for every other contributor the sh -c wrapper runs cat on 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 in AGENTS.md. Either way it wants its own commit, not this stack.

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.cloud` and `$HOME/.config/forgejo-mcp/token`, so for every other contributor the `sh -c` wrapper runs `cat` on 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 in `AGENTS.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 patch
const patchPath = `${root}/.git/opsdeck-pin-${++patchSeq}.patch`;
Owner

Two things here.

  1. This is the write that still bypasses hostPath()${ctx.paths.hostRoot}${patchPath} on line 96 and again on 158. root comes from rev-parse --show-toplevel so 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.

  2. ${root}/.git/... assumes .git is a directory. In a linked worktree or a submodule it is a file, so Deno.writeTextFile fails, the catch turns it into commit failed: …, and the operator gets a confusing message for a perfectly normal repository layout. Ask git instead:

const gitDir = await ctx.host.run("git", ["-C", root, "rev-parse", "--absolute-git-dir"], { timeoutMs: 15_000 });

and write the patch there.

Two things here. 1. This is the write that still bypasses `hostPath()` — `${ctx.paths.hostRoot}${patchPath}` on line 96 and again on 158. `root` comes from `rev-parse --show-toplevel` so 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. 2. `${root}/.git/...` assumes `.git` is a directory. In a linked worktree or a submodule it is a *file*, so `Deno.writeTextFile` fails, the catch turns it into `commit failed: …`, and the operator gets a confusing message for a perfectly normal repository layout. Ask git instead: ```ts const gitDir = await ctx.host.run("git", ["-C", root, "rev-parse", "--absolute-git-dir"], { timeoutMs: 15_000 }); ``` 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 &&
Owner

Right fix, but it still lies on the failure path: a pull where every layer ends in error satisfies every(r => r.state === "done" || r.state === "error") and is reported as state: "done". The commit's own argument — "a field that lies is a trap for whoever reads it next" — applies here too.

const all = [...rates.values()];
const settled = all.every((r) => r.state === "done" || r.state === "error");
const state = !settled ? "active" : all.some((r) => r.state === "error") ? "error" : "done";

(rates.size > 0 is also redundant — the whole block is already inside if (rates.size && …).)

Right fix, but it still lies on the failure path: a pull where every layer ends in `error` satisfies `every(r => r.state === "done" || r.state === "error")` and is reported as `state: "done"`. The commit's own argument — "a field that lies is a trap for whoever reads it next" — applies here too. ```ts const all = [...rates.values()]; const settled = all.every((r) => r.state === "done" || r.state === "error"); const state = !settled ? "active" : all.some((r) => r.state === "error") ? "error" : "done"; ``` (`rates.size > 0` is also redundant — the whole block is already inside `if (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;
Owner

jsonProgress is 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 the false forever, 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.

`jsonProgress` is 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 the `false` forever, 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("..")) {
Owner

The .. substring test is not a path check, and it has a false positive with a real cost.

hostPath is also used for env files, whose paths come from resolveEnvPath() in compose.ts — which only strips a leading ./. So env_file: ../shared/.env, an ordinary layout for a shared env one directory up, resolves to /opt/stacks/web/../shared/.env, this throws, and collectStackFiles reports the file to the operator as refusing 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 under hostRoot. 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.ts writes a patch file outside it. One word.

**The `..` substring test is not a path check, and it has a false positive with a real cost.** `hostPath` is also used for env files, whose paths come from `resolveEnvPath()` in `compose.ts` — which only strips a leading `./`. So `env_file: ../shared/.env`, an ordinary layout for a shared env one directory up, resolves to `/opt/stacks/web/../shared/.env`, this throws, and `collectStackFiles` reports the file to the operator as `refusing 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 under `hostRoot`. 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.ts` writes 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(
Owner

This releases the registry slot but nothing stops the task that was holding it. composeUp is inside a 30-minute ctx.host.run, and there is no AbortController anywhere on that path — so after a release, POST /stack/:project/update takes a fresh token and starts a second docker compose up -d --pull always against 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.

This releases the registry slot but nothing stops the task that was holding it. `composeUp` is inside a 30-minute `ctx.host.run`, and there is no AbortController anywhere on that path — so after a release, `POST /stack/:project/update` takes a fresh token and starts a second `docker compose up -d --pull always` against 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)
Owner

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:

let current: string | null;
try {
  current = await Deno.readTextFile(hostPath(file));
} catch (e) {
  // a file we have already rewritten must not be skipped: let it unwind
  // to the restore path rather than pretending the service is not here
  if (restore.has(file)) throw e;
  continue;
}
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: ```ts let current: string | null; try { current = await Deno.readTextFile(hostPath(file)); } catch (e) { // a file we have already rewritten must not be skipped: let it unwind // to the restore path rather than pretending the service is not here if (restore.has(file)) throw e; continue; } ```
@ -1150,0 +1691,4 @@
}, 400);
}
const dir = first?.workingDir ?? "/";
Owner

The git root is probed from first?.workingDir, but the files in patches come from each target's configFiles, which need not live in that directory or even in that repository. When they do not, commitLines resolves a root that does not contain them, filePatch leaves those paths absolute in the diff --git a/… header, and git apply --cached rejects 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.

The git root is probed from `first?.workingDir`, but the files in `patches` come from each *target's* `configFiles`, which need not live in that directory or even in that repository. When they do not, `commitLines` resolves a `root` that does not contain them, `filePatch` leaves those paths absolute in the `diff --git a/…` header, and `git apply --cached` rejects 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 touch
if (currentImage && value !== currentImage) return null;
Owner

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.1 in the file against postgres:16.1 from the engine returns null here, the caller treats that as "this file does not declare the service", and the route answers 404 no 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/, add library/ for single-segment official images, default the tag to latest) — the logic already exists in updates.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.

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.1` in the file against `postgres:16.1` from the engine returns null here, the caller treats that as "this file does not declare the service", and the route answers 404 `no 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/`, add `library/` for single-segment official images, default the tag to `latest`) — the logic already exists in `updates.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 {
Owner

Confirming your point rather than re-arguing it: findUpgrade and listUpgrades both reject p.nums.length !== from.nums.length before compare is reached, so the ?? -1 branch 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.

Confirming your point rather than re-arguing it: `findUpgrade` and `listUpgrades` both reject `p.nums.length !== from.nums.length` before `compare` is reached, so the `?? -1` branch 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 it
const listed = new Map<string, { tags: string[]; error: string | null }>();
Owner

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/refresh awaits the whole thing, and every completed update job fires refreshStacks().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, listed is 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.

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/refresh` awaits the whole thing, and every completed update job fires `refreshStacks().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, `listed` is 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) =>
Owner

projectBusy is declared here but calls jobFor, which is declared ~100 lines further down (line 233). Both are const arrows, so this only works because confirmQueue starts empty and $derived is 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 TDZ ReferenceError at component init, which in a module frontend means a blank page.

Move jobFor/jobs above this, or make projectBusy take the job list as an argument.

`projectBusy` is declared here but calls `jobFor`, which is declared ~100 lines further down (line 233). Both are `const` arrows, so this only works because `confirmQueue` starts empty and `$derived` is 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 TDZ `ReferenceError` at component init, which in a module frontend means a blank page. Move `jobFor`/`jobs` above this, or make `projectBusy` take 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(
Owner

The queue entry is removed before the await, so confirmUpdate re-derives immediately. When the next queued prompt belongs to a different project it is not gated by projectBusy, so it becomes the current prompt at once — and because confirming is a single global flag, {#key confirmUpdate?.id} remounts the dialog showing service B's wording with busy=true and 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 by pending.id, and pass busy={answering === confirmUpdate?.id}. That also keeps the double-click guard intact.

The queue entry is removed before the await, so `confirmUpdate` re-derives immediately. When the next queued prompt belongs to a *different* project it is not gated by `projectBusy`, so it becomes the current prompt at once — and because `confirming` is a single global flag, `{#key confirmUpdate?.id}` remounts the dialog showing **service B's** wording with `busy=true` and 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 by `pending.id`, and pass `busy={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")
Owner

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 02c2886 changed the stakes. jobs is no longer only display state: projectBusy reads jobFor(project), and confirmUpdate is derived from it, so the job list now decides whether a destructive confirmation is on screen. A loadJobs() 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 back update already running after 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, the liveRefresh("docker:stacks") callback, and runTask.

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 `02c2886` changed the stakes. `jobs` is no longer only display state: `projectBusy` reads `jobFor(project)`, and `confirmUpdate` is derived from it, so the job list now decides *whether a destructive confirmation is on screen*. A `loadJobs()` 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 back `update already running` after 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, the `liveRefresh("docker:stacks")` callback, and `runTask`.
Owner

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. answerConfirm removes the entry synchronously before the await, so a racing load() can only see prompts nobody answered.

Declined by you on reasoning I accept — removed:

  • C97 symlinks: socket access is root-equivalent, the route is admin-gated, lstat gates 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 on env_file: ../shared/.env.)
  • C107, C108, C109: dev-rig only, no untrusted input, and singling out one file under docker/dev-rig/ implies the rest are safe to copy.

Decisions you handed back, now made:

  • C99, fire-and-forget ×3 — your call to defer stands; not a correctness bug. Follow-up is fine.
  • C100, loadJobs single-flightin this PR, please. Not for the reason I first gave: 02c2886 promoted jobs from display state to the input of projectBusy, so the job list now decides whether a destructive confirmation is on screen. Detail on the inline comment.
  • C106 + Dockerfile digestsdo it, separate PR. Right change, wrong place; burying a pinning policy in a 142-file merge is how it stops having an owner. I will file the issue if you would rather not carry it.

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:

  1. DELETE /jobs/:project frees the slot without aborting the task, so the escape hatch permits two concurrent docker compose up on one project — the state the mutex exists to prevent. Fix this one first.
  2. Cross-project prompt swap in 02c2886: confirming is global, so the next queued prompt for a different project mounts showing its own service with busy=true, then flips live.
  3. projectBusy declared above the jobFor it calls — safe only because the derived is lazy and the queue starts empty.
  4. hostPath's .. substring test vs. env_file: ../shared/.env.
  5. rewriteServiceImage compares image refs byte-wise, so docker.io/library/postgres:16.1 in the file against postgres:16.1 from the engine 404s with "no compose file declares an image for that service".
  6. commit.ts writes to ${root}/.git/…, which is a file in a linked worktree or submodule.
  7. /stack/:project/pin probes the git root from workingDir while patches come from each target's configFiles — files already rewritten, patch rejected, no commit.
  8. checkUpdates fully serial, no wall-clock budget, tag cache per-call.
  9. supportsJsonProgress memoises false for the process lifetime across a host compose upgrade.
  10. The derived pull state still reports done when every layer ended in error.
  11. .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.

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. `answerConfirm` removes the entry synchronously before the `await`, so a racing `load()` can only see prompts nobody answered. **Declined by you on reasoning I accept — removed:** - C97 symlinks: socket access is root-equivalent, the route is admin-gated, `lstat` gates 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 on `env_file: ../shared/.env`.) - C107, C108, C109: dev-rig only, no untrusted input, and singling out one file under `docker/dev-rig/` implies the rest are safe to copy. **Decisions you handed back, now made:** - **C99, fire-and-forget ×3** — your call to defer stands; not a correctness bug. Follow-up is fine. - **C100, `loadJobs` single-flight** — **in this PR, please.** Not for the reason I first gave: `02c2886` promoted `jobs` from display state to the input of `projectBusy`, so the job list now decides whether a destructive confirmation is on screen. Detail on the inline comment. - **C106 + Dockerfile digests** — **do it, separate PR.** Right change, wrong place; burying a pinning policy in a 142-file merge is how it stops having an owner. I will file the issue if you would rather not carry it. 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: 1. `DELETE /jobs/:project` frees the slot without aborting the task, so the escape hatch permits two concurrent `docker compose up` on one project — the state the mutex exists to prevent. Fix this one first. 2. Cross-project prompt swap in `02c2886`: `confirming` is global, so the next queued prompt for a *different* project mounts showing its own service with `busy=true`, then flips live. 3. `projectBusy` declared above the `jobFor` it calls — safe only because the derived is lazy and the queue starts empty. 4. `hostPath`'s `..` substring test vs. `env_file: ../shared/.env`. 5. `rewriteServiceImage` compares image refs byte-wise, so `docker.io/library/postgres:16.1` in the file against `postgres:16.1` from the engine 404s with "no compose file declares an image for that service". 6. `commit.ts` writes to `${root}/.git/…`, which is a *file* in a linked worktree or submodule. 7. `/stack/:project/pin` probes the git root from `workingDir` while patches come from each target's `configFiles` — files already rewritten, patch rejected, no commit. 8. `checkUpdates` fully serial, no wall-clock budget, tag cache per-call. 9. `supportsJsonProgress` memoises `false` for the process lifetime across a host compose upgrade. 10. The derived pull `state` still reports `done` when every layer ended in `error`. 11. `.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.
fix(docker): close the job-slot hole and the rest of the review
Some checks failed
Build and Deploy / verify (pull_request) Failing after 13s
Build and Deploy / build (pull_request) Has been skipped
665bea2c21
**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.
Author
Owner

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() set finishedAt, which freed the slot under a live compose up. Nothing in the module can abort one, so the fix is to stop pretending: the run is marked abandoned, the claim is held until the task's own finish() lands, and ?force=1 keeps 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. confirming tracks the id being answered rather than a global boolean, so a prompt for another project no longer mounts busy and flips live. projectBusy now sits below the jobFor it 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 jobs from display state into the input of projectBusy, 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 rejected env_file: ../shared/.env, which is legal and exactly what resolveEnvPath emits. 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.1 against postgres:16.1 declined 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-dir rather than an assumed ${root}/.git directory, and the stack pin route probes the repository from a file it actually patched instead of workingDir.

8 and 9. Bounded concurrency, a wall-clock budget, and a tag cache that survives the call with single-flight per repository. The --progress json negative 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 error when every layer errored.

11. Out of the PR, and .gitignored.

One thing I hit that is worth writing down: deno fmt/lint/test at the repo root were walking .claude/worktrees/, where Claude Code checks branches out. That doubled the file counts, failed on another branch's code, and deno fmt rewrote a file inside that other checkout — I reverted it. .claude is 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_CHANGES and now stale against 665bea2.

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()` set `finishedAt`, which freed the slot under a live `compose up`. Nothing in the module can abort one, so the fix is to stop pretending: the run is marked `abandoned`, the claim is held until the task's own `finish()` lands, and `?force=1` keeps 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. `confirming` tracks the id being answered rather than a global boolean, so a prompt for another project no longer mounts busy and flips live. `projectBusy` now sits below the `jobFor` it 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 `jobs` from display state into the input of `projectBusy`, 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 rejected `env_file: ../shared/.env`, which is legal and exactly what `resolveEnvPath` emits. 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.1` against `postgres:16.1` declined 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-dir` rather than an assumed `${root}/.git` directory, and the stack pin route probes the repository from a file it actually patched instead of `workingDir`. **8 and 9.** Bounded concurrency, a wall-clock budget, and a tag cache that survives the call with single-flight per repository. The `--progress json` negative 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 `error` when every layer errored. **11.** Out of the PR, and `.gitignore`d. One thing I hit that is worth writing down: `deno fmt`/`lint`/`test` at the repo root were walking `.claude/worktrees/`, where Claude Code checks branches out. That doubled the file counts, failed on another branch's code, and `deno fmt` rewrote a file inside that other checkout — I reverted it. `.claude` is 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_CHANGES` and now stale against `665bea2`.
julian requested changes 2026-08-10 11:58:55 +02:00
Dismissed
julian left a comment

Third pass, branch checked out at 665bea2. All four verification claims re-run and all four hold: deno fmt --check 231 files clean, deno lint 141 files clean, deno task check all five entrypoints, deno task test 242 passed, 0 failed (241 + the test you split). The merge decisions still match the tree.

665bea2 is 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 unabortable compose up ended. 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

  • 1, job slot: claim held until the task's own finish(), ?force=1 kept and logged with its risk, tests assert the new contract.
  • 3, projectBusy moved below jobFor.
  • 4, hostPath: lexical normalise, escape-past-/ refused. env_file: ../shared/.env works.
  • 6, git rev-parse --absolute-git-dir.
  • 9, negative --progress json answer expires; the positive one still doesn't, which is the right asymmetry.
  • 10, pullState().
  • 11, .mcp.json gone 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:

  • The identical-ternary read, 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.
  • The hostPath JSDoc still reads "Every route that writes the operator's files goes through here" (mod.ts:127) while commit.ts:110 and :172 write ${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 than workingDir, and for the common layout it is right. A patch set spanning two repositories still lands every path on one root, commit.ts:105 leaves the others absolute in the diff --git header, and git apply --cached rejects 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, and updates.ts:314 writes an error entry with updateAvailable: null instead. Since mod.ts replaces state.updates wholesale, a timeout downgrades a known "update available" to unknown and drops it out of the updates.available metric. The stale verdict was the better lie.

New, and the reason for the label

A. ALREADY is unreachable from both pin routes, so the half-applied case answers 404. pin.ts:148 returns null when the file's value differs from currentImage, before the after === line check at :154. Both routes pass target.image. So the state your own comments describe at mod.ts:1650 and pin.ts:150-153 — file already at 16.4, container still on 16.1 — returns null for every file, and the route answers 404 no compose file declares an image for that service on 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) calls rewriteServiceImage without currentImage — the one shape neither route uses — which is why it passes. This is the item I would fix before the others.

B. mod.ts:1653 breaks where mod.ts:1684 explains at length why it must not. The ALREADY branch leaves the file loop; thirty lines below, the non-ALREADY path 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.busy is dead again, in both callers. answerConfirm sets answering = pending.id and filters that id out of confirmQueue in the same synchronous block (StacksPage.svelte:147-148), so the derived confirmUpdate is never the answered prompt and busy={answering === confirmUpdate?.id} (:961) can never be true. ContainerPage.svelte:717 reaches the same place differently: confirmUpdate = null closes the dialog before busy renders. 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 with canonicalRef letting image: docker.io/library/postgres:16.1 match, that line comes back as image: 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. abandoned has no consumer. jobs.ts:37 says "Clients use this to stop waiting on a run nobody wants to watch any more." Nothing reads it — not StacksPage.svelte:201-210, not JobSnapshot in job_follow.svelte.ts:24 — and no frontend code calls DELETE /jobs/:project at all, so an abandoned run still spins JobFollow forever and the escape hatch is curl-only. Relatedly mod.ts:397-405 still describes the route as the operator's way out of a wedged slot; only ?force=1 is 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 loadJobs epoch is dead code. jobsInFlight already admits one request at a time and .then runs before .finally clears it, so epoch === jobsEpoch is 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 f8c59fc set 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 to findUpgrades), compose_progress.ts:440, and packages/server/src/modules/host.ts:34.

For the pinning PR, not this one

ssh-keyscan -H 192.168.0.3 >> known_hosts re-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.

Third pass, branch checked out at `665bea2`. All four verification claims re-run and all four hold: `deno fmt --check` 231 files clean, `deno lint` 141 files clean, `deno task check` all five entrypoints, `deno task test` **242 passed, 0 failed** (241 + the test you split). The merge decisions still match the tree. `665bea2` is 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 unabortable `compose up` ended. 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 - **1**, job slot: claim held until the task's own `finish()`, `?force=1` kept and logged with its risk, tests assert the new contract. - **3**, `projectBusy` moved below `jobFor`. - **4**, `hostPath`: lexical normalise, escape-past-`/` refused. `env_file: ../shared/.env` works. - **6**, `git rev-parse --absolute-git-dir`. - **9**, negative `--progress json` answer expires; the positive one still doesn't, which is the right asymmetry. - **10**, `pullState()`. - **11**, `.mcp.json` gone 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: - The identical-ternary read, `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. - The `hostPath` JSDoc still reads "Every route that writes the operator's files goes through here" (`mod.ts:127`) while `commit.ts:110` and `:172` write `${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 than `workingDir`, and for the common layout it is right. A patch set spanning two repositories still lands every path on one root, `commit.ts:105` leaves the others absolute in the `diff --git` header, and `git apply --cached` rejects 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, and `updates.ts:314` writes an error entry with `updateAvailable: null` instead. Since `mod.ts` replaces `state.updates` wholesale, a timeout *downgrades* a known "update available" to unknown and drops it out of the `updates.available` metric. The stale verdict was the better lie. ### New, and the reason for the label **A. `ALREADY` is unreachable from both pin routes, so the half-applied case answers 404.** `pin.ts:148` returns `null` when the file's value differs from `currentImage`, *before* the `after === line` check at `:154`. Both routes pass `target.image`. So the state your own comments describe at `mod.ts:1650` and `pin.ts:150-153` — file already at 16.4, container still on 16.1 — returns `null` for every file, and the route answers `404 no compose file declares an image for that service` on 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`) calls `rewriteServiceImage` **without** `currentImage` — the one shape neither route uses — which is why it passes. This is the item I would fix before the others. **B. `mod.ts:1653` breaks where `mod.ts:1684` explains at length why it must not.** The `ALREADY` branch leaves the file loop; thirty lines below, the non-`ALREADY` path 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.busy` is dead again, in both callers.** `answerConfirm` sets `answering = pending.id` and filters that id out of `confirmQueue` in the same synchronous block (`StacksPage.svelte:147-148`), so the derived `confirmUpdate` is never the answered prompt and `busy={answering === confirmUpdate?.id}` (`:961`) can never be true. `ContainerPage.svelte:717` reaches the same place differently: `confirmUpdate = null` closes the dialog before `busy` renders. 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 with `canonicalRef` letting `image: docker.io/library/postgres:16.1` match, that line comes back as `image: 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. `abandoned` has no consumer.** `jobs.ts:37` says "Clients use this to stop waiting on a run nobody wants to watch any more." Nothing reads it — not `StacksPage.svelte:201-210`, not `JobSnapshot` in `job_follow.svelte.ts:24` — and no frontend code calls `DELETE /jobs/:project` at all, so an abandoned run still spins `JobFollow` forever and the escape hatch is curl-only. Relatedly `mod.ts:397-405` still describes the route as the operator's way out of a wedged slot; only `?force=1` is 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 `loadJobs` epoch is dead code.** `jobsInFlight` already admits one request at a time and `.then` runs before `.finally` clears it, so `epoch === jobsEpoch` is 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 `f8c59fc` set 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 to `findUpgrades`), `compose_progress.ts:440`, and `packages/server/src/modules/host.ts:34`. ### For the pinning PR, not this one `ssh-keyscan -H 192.168.0.3 >> known_hosts` re-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.
Owner

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 the verify job and needs:/if: on build. It does not add actions/checkout, docker/login-action, docker/setup-buildx-action, docker/build-push-action, the ssh-keyscan, or the SSH deploy — all six were on main before the branch existed, and docker/Dockerfile's base images likewise. The only new unpinned reference is the verify job's own checkout, matching the convention of the file it is written in.

So C106, the Dockerfile digests and the ssh-keyscan line 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_KEY in 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-check reads 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 # v6 trailing comment, so the checker still works after its own advice is taken. Against this repository as it stands today it finds five: checkout v7.0.1, deno 2.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.md carries the ownership line in the place the loop lives rather than in a paragraph of docs/security.md.


What this review still rests on, unchanged from the last pass and unaffected by any of the above:

  1. AALREADY is unreachable from both pin routes, so the half-applied pin answers 404 no compose file declares an image for that service on a file that declares it. pin.ts:148 short-circuits before :154, and the only test for it passes no currentImage, which is the one shape neither route uses.
  2. An answer of "declined" or "follow-up" on the three items from the previous pass that are unchanged in the tree — the identical-ternary read, the compare() rationale, and the hostPath JSDoc 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.

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 the `verify` job and `needs:`/`if:` on `build`. It does not add `actions/checkout`, `docker/login-action`, `docker/setup-buildx-action`, `docker/build-push-action`, the `ssh-keyscan`, or the SSH deploy — all six were on `main` before the branch existed, and `docker/Dockerfile`'s base images likewise. The only new unpinned reference is the `verify` job's own `checkout`, matching the convention of the file it is written in. So C106, the Dockerfile digests and the `ssh-keyscan` line 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_KEY` in 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-check` reads 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 `# v6` trailing comment, so the checker still works after its own advice is taken. Against this repository as it stands today it finds five: checkout `v7.0.1`, deno `2.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.md` carries the ownership line in the place the loop lives rather than in a paragraph of `docs/security.md`. --- **What this review still rests on**, unchanged from the last pass and unaffected by any of the above: 1. **A** — `ALREADY` is unreachable from both pin routes, so the half-applied pin answers `404 no compose file declares an image for that service` on a file that declares it. `pin.ts:148` short-circuits before `:154`, and the only test for it passes no `currentImage`, which is the one shape neither route uses. 2. An answer of "declined" or "follow-up" on the three items from the previous pass that are unchanged in the tree — the identical-ternary read, the `compare()` rationale, and the `hostPath` JSDoc 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.
fix(docker): the half-applied pin, and the rest of the third pass
Some checks failed
Build and Deploy / verify (pull_request) Failing after 14s
Build and Deploy / build (pull_request) Has been skipped
7cbf97eb54
**A. The half-applied state was unreachable.** `rewriteServiceImage`
tested `currentImage` before it tested whether the line already said what
we were about to write, and both routes pass the running container's
image — so exactly the state the ALREADY branch exists for (file at
16.4, container still on 16.1) returned null from every file and the
route answered "no compose file declares an image for that service"
about a file that declares it. Reachable by a pin whose recreate then
fails. The only test for it passed no currentImage, which is the one
shape neither route uses; there is now a test in the shape they do use.

**B.** The ALREADY branch left the file loop, thirty lines above the
comment explaining why the write path must not: compose merges left to
right, so a base that already reads correctly says nothing about an
override still pinning the old tag. Masked by A, real once A is fixed.

**C and D.** `answerConfirm` removed the entry from the queue in the same
synchronous block that set `answering`, so the derived could never be
the answered prompt: `busy` was unobservable again — the thing C102
asked for — and the next prompt for an idle project mounted live while
the guard silently swallowed clicks on it. The entry is dequeued when
the answer has been acted on, and the answered prompt is what the
derived resolves to until then.

**E.** `retag(target.image, tag)` built the new line from the ENGINE's
ref, so with canonical matching `image: docker.io/library/postgres:16.1`
came back as `image: postgres:16.4`. Only the tag is taken from it now;
the rest of the line keeps the spelling the operator chose.

**F.** `abandoned` had no reader, and the route's comment called the
default the escape hatch when only `?force=1` is. JobFollow stops
following an abandoned run, and the response says `stillClaimed`.

**G.** The `loadJobs` epoch could never disagree — single-flight already
admits one request. Gone.

**H.** Four more stacked JSDoc blocks, the class f8c59fc set out to fix:
a stale release doc contradicting the block beneath it, findUpgrade's
doc on findUpgrades, parseComposeLine's on createComposeStream, and
safeUpgradeWebSocket's on a type alias.

**7.** Patches are grouped by the work tree that owns each file and
committed per repository. One root for a set spanning two left the
others absolute in the diff header and `git apply --cached` rejected
everything.

**8.** An image the budget ran out on keeps its previous verdict instead
of an error entry; the caller replaces the map wholesale, so writing
`updateAvailable: null` downgraded a known update to unknown and dropped
it from the count.

**Unchanged items from the last pass, now done:** the two byte-identical
ternary branches collapsed, `compare()`'s shape-before-order rule
written down, and `commit.ts` routed through `hostPath` — which is what
made that helper's "every route goes through here" true rather than
aspirational.
fix(docker): three defects in the commit that fixed the last three
Some checks failed
Build and Deploy / verify (pull_request) Failing after 13s
Build and Deploy / build (pull_request) Has been skipped
658f67846a
Found by re-reading `7cbf97e` rather than by review, which is where the
base rate has been.

**A sync loop, and it is the worse half of what it replaced.** The
`abandoned` branch I added to JobFollow reported the run on every sync
with no `wasRunning` guard — while the branch six lines below it carries
exactly that guard, with a comment explaining why: onFinish reloads the
page, reloading re-syncs, "or the two call each other forever". An
abandoned run stays abandoned for as long as its task holds the claim,
which the same commit made deliberately long, so this would have spun
for the length of a compose run. Reported once now.

**"update 0 pinned images".** Grouping patches per repository matched a
group's changes by looking for `c.to` inside the rewritten line. That
only works when both spellings agree — and the edit now deliberately
preserves the operator's registry prefix while `c.to` carries the
engine's, so they routinely do not. A group could match nothing and
commit a message describing no change. The patch entries carry their
service and the grouping matches on that.

**A cosmetic one, fixed while there:** `hostPath` produced `//srv/...`
when the host root is "/", which is legal and shows up in every logged
path.
Owner

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/checkout had moved on while @v6 still meant whatever that repository decided it meant that morning.

So 06cd02f pins all four actions in build.yml to commit SHAs and all three base images — including COPY --from=denoland/deno:bin-2.5.6, which appears in no FROM line 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 --check resolves 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.

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/checkout` had moved on while `@v6` still meant whatever that repository decided it meant that morning. So `06cd02f` pins all four actions in `build.yml` to commit SHAs and all three base images — including `COPY --from=denoland/deno:bin-2.5.6`, which appears in no `FROM` line 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 --check` resolves 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.
fix(docker): two more of mine, and a test that would have caught one
Some checks failed
Build and Deploy / verify (pull_request) Failing after 12s
Build and Deploy / build (pull_request) Has been skipped
58a61427a5
Auditing my own last two commits, as they needed.

**The carry-forward defeated itself.** `...(before ?? fallback)` followed
by `error: before?.error ?? "ran out of time"` — and a verdict that was
fine has `error: null`, so `null ?? msg` is the msg. Every image the
budget skipped came back carrying an error over a perfectly good answer,
which is precisely the downgrade the change existed to prevent, wearing
a different hat. The previous entry is carried whole now.

The budget is injectable so that path is testable at all, and there is a
test for it: it fails against the code as it was an hour ago and passes
against this. That is the check I should have written when I first
touched this, and the reason this bug survived two passes.

**The swallowed answer had a narrower door left open.** While an answer
is in flight the derived fell through to the next free prompt if
`load()` dropped the answered entry mid-flight — putting a live, enabled
dialog on screen whose buttons hit the re-entrancy guard and did
nothing. While `answering` is set, the only prompt that may be shown is
the one being answered.

Checked and found correct while in there: `runTask` catches its own
POST, so an unawaited `answerConfirm` cannot leak a rejection; the
abandoned branch cannot loop now that `wasRunning` gates it; and
`hostPath` with a host root of "/" yields "/srv/..." rather than the
empty prefix I half-expected after trimming.
test(docker): run the pin plumbing against real git and real compose
Some checks failed
Build and Deploy / verify (pull_request) Failing after 16s
Build and Deploy / build (pull_request) Has been skipped
91a2d558d8
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.
Author
Owner

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. ALREADY is tested before the currentImage guard now, so the half-applied state its own comment describes is reachable. You were also right about why the existing test passed: it called rewriteServiceImage without currentImage, 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 config on a base+override pair, pre-fix resolves to postgres:16.1 — the pin silently does nothing — and post-fix to 16.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 busy is 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 canonicalRef change. Only the tag is taken from the new ref; the rest of the line keeps the operator's spelling. Tested.

F, G, Habandoned is read by JobFollow and the route reports stillClaimed; 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 has error: null, so null ?? "ran out of time" put an error on every carried-forward answer — the same downgrade wearing a different hat. Fixed in 58a6142, 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, and commit.ts routed through hostPath, 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:

  • The abandoned branch I added called onFinish with no wasRunning guard — six lines above the branch that has one, with a comment saying why. onFinish reloads, 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.
  • Per-repository grouping matched a group's changes by looking for c.to inside 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.
  • The carry-forward error above.
  • The residual swallowed-answer race.
  • hostPath emitting //srv/... when the host root is /.

Tested, not reasoned

91a2d55 runs the pin plumbing against real git: the generated patch through git apply --cached for 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 .git is 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 inside denoland/deno:2.5.6 rather than assuming.

251 tests locally, 245 + 4 ignored in the CI image. deno fmt --check, deno lint, deno task check all 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 .svelte file — testing it means extracting the logic to a .svelte.ts module 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-keyscan TOFU you spotted at build.yml:94 — is still yours to file, as offered.

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. `ALREADY` is tested before the `currentImage` guard now, so the half-applied state its own comment describes is reachable. You were also right about why the existing test passed: it called `rewriteServiceImage` without `currentImage`, 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 config` on a base+override pair, pre-fix resolves to `postgres:16.1` — the pin silently does nothing — and post-fix to `16.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 `busy` is 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 `canonicalRef` change. Only the tag is taken from the new ref; the rest of the line keeps the operator's spelling. Tested. **F, G, H** — `abandoned` is read by `JobFollow` and the route reports `stillClaimed`; 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 has `error: null`, so `null ?? "ran out of time"` put an error on every carried-forward answer — the same downgrade wearing a different hat. Fixed in `58a6142`, 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, and `commit.ts` routed through `hostPath`, 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: - The `abandoned` branch I added called `onFinish` with no `wasRunning` guard — six lines above the branch that has one, with a comment saying why. `onFinish` reloads, 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. - Per-repository grouping matched a group's changes by looking for `c.to` inside 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. - The carry-forward error above. - The residual swallowed-answer race. - `hostPath` emitting `//srv/...` when the host root is `/`. ## Tested, not reasoned `91a2d55` runs the pin plumbing against real git: the generated patch through `git apply --cached` for 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 `.git` is 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 inside `denoland/deno:2.5.6` rather than assuming. 251 tests locally, 245 + 4 ignored in the CI image. `deno fmt --check`, `deno lint`, `deno task check` all 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 `.svelte` file — testing it means extracting the logic to a `.svelte.ts` module 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-keyscan` TOFU you spotted at `build.yml:94` — is still yours to file, as offered.
Owner

Separate from the review items, and found while getting #11's workflow to run: the verify job added here has never passed, and cannot as written.

runs 175–186   twelve consecutive pull_request failures
before them    every push-to-main run green

The cause is the job's own container. container: denoland/deno:2.5.6 plus uses: actions/checkout@… cannot work: actions/checkout is a Node action and that image ships no node — nor git, nor curl. The job dies at

OCI runtime exec failed: exec: "node": executable file not found in $PATH

before 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:

  1. drop container:, run the job on the runner, and enter the pinned image per command. Note that docker run -v "$PWD":/w also fails here — the runner executes steps inside its own container, so $PWD is not a path the host daemon knows and the mount silently lands on nothing. docker cp works. .forgejo/deno.sh on #11 is that, with both reasons written down;
  2. or keep 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 verify job should not merge to main in a state where it fails on every pull request.

Separate from the review items, and found while getting #11's workflow to run: **the `verify` job added here has never passed, and cannot as written.** ``` runs 175–186 twelve consecutive pull_request failures before them every push-to-main run green ``` The cause is the job's own container. `container: denoland/deno:2.5.6` plus `uses: actions/checkout@…` cannot work: `actions/checkout` is a Node action and that image ships no `node` — nor `git`, nor `curl`. The job dies at ``` OCI runtime exec failed: exec: "node": executable file not found in $PATH ``` before 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: 1. drop `container:`, run the job on the runner, and enter the pinned image per command. Note that `docker run -v "$PWD":/w` **also** fails here — the runner executes steps inside its own container, so `$PWD` is not a path the host daemon knows and the mount silently lands on nothing. `docker cp` works. [.forgejo/deno.sh](https://git.imhof.cloud/OpsDeck/core/src/branch/ci/dependency-update-issues/.forgejo/deno.sh) on #11 is that, with both reasons written down; 2. or keep `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 `verify` job should not merge to main in a state where it fails on every pull request.
julian requested changes 2026-08-10 14:10:13 +02:00
Dismissed
julian left a comment

Fourth pass, against 91a2d55. This review replaces the one that was still sitting at 665bea2 — you asked for that twice and were right to.

I ran the tree rather than reading the claim: deno fmt --check 232 clean, deno lint 142 clean, deno task check all five entrypoints, deno task test 251 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 verify job takes main's deploy with it

I said last time that verify "should not merge to main in a state where it fails on every pull request". That undersells it. verify carries no if:, and build carries needs: verify. The job list for run #425 — this PR's head — is the whole argument:

verify   failure
build    skipped    needs: [verify]

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-on does not save it, because it is the obvious objection and I made it myself before reading the log. cth-ubuntu-latest maps to catthehacker's full image, which does have node — but container: overrides where steps execute, and runs-on only picks the machine. build has never had a container: 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. verify is the first job in this repository to set container:, and the log says exactly where the exec lands:

🐳  docker create image=denoland/deno:2.5.6 entrypoint=["tail" "-f" "/dev/null"]
🐳  docker cp    src=/data/.cache/act/b3/… dst=/var/run/act/actions/b3/…
🐳  docker exec  cmd=[node /var/run/act/actions/b3/…/dist/index.js]
OCI runtime exec failed: exec: "node": executable file not found in $PATH: unknown

docker exec into the deno container. The runner image's node is on the far side of that boundary. act_runner copies the action's dist/index.js in; 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:

  1. Drop 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 the docker cp dance, because the runner executes steps inside its own container, so docker run -v "$PWD":/w mounts a path the host daemon does not know and lands on nothing. .forgejo/deno.sh on #11 is that, with both reasons written down; say the word and I will port it across.
  2. Keep 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:242 returns the in-flight promise to a second caller. runTask at :414 awaits it immediately after the POST — and a request already in flight at that moment is one issued before the job existed. So jobs comes back without the new run: job.adopt(null), and starting[id] = false at :416 while 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 give runTask an uncoalesced call. My "single-flight already admits one request" is what let it through, so this one is mine.

2. ALREADY no longer proves the line names the image being pinned. pin.ts:160 returns it before the currentImage guard 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. File nginx:1.27, container httpd:1.25, pin to 1.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 :90 still describes a guard that branch no longer reaches. Comparing the repo parts before returning ALREADY restores it without undoing A.

3. The .. substring test is still live in the other guard. Round three fixed hostPath to normalise rather than substring-match, because env_file: ../shared/.env is legal and is what resolveEnvPath emits. compose_update.ts:77 still does path.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. :83 also builds ${prefix}${f} by hand, which makes mod.ts:121's "the single place a host path is built" aspirational again — and prefix is untrimmed, so a hostRoot with 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:317 checks the deadline before dequeuing. Nothing bounds an item already running, and listTags is up to 20 pages at TAG_TIMEOUT_MS each — 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 an AbortSignal.

5. retag/tagOf mangle a digest-pinned ref. pin.ts:241: lastIndexOf(":") on postgres@sha256:abc… lands inside the digest, so retagging produces postgres@sha256:16.4. parseImageRef then refuses anything with @, and mod.ts:1446 reports no 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:104 returns {digest: null, error: null} when the response is 200 without docker-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:199release() pushes onto job.lines directly, skipping the MAX_LINES trim record() applies. One line, so only theoretically unbounded.
  • ConfirmDialog.svelte:47 — the focus effect depends on open, busy, variant and both element refs, so it re-focuses on any of them changing, including busy going 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:145 vs :92 — for a four-component pin, listUpgrades().minor admits shared === 1 while withinScope("minor") requires fixed = 2, so upgradeOptions.minor can 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:215 and :82rootOf runs once per file for grouping and again inside commitOne, so two git rev-parse per file. Caching the group key covers it.
  • deno fmt joined 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:16 all now read Switch 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. withRepoLock and the staged-index refusal. restore.keep's first-wins against the base each hunk measures from. release()'s abandoned-versus-force semantics and JobFollow's two wasRunning guards. The phase carry-forward on both sides of the wire. compose_progress's rate differentiation, the DOWNLOAD_SETTLED rebase and pullState. canonicalReflibrary/ implicit on Hub only, ports and digests intact. nextPage's cross-host refusal. localImages's bare-:latest aliasing. update.ts is 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_CHANGES rests 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.

Fourth pass, against `91a2d55`. This review replaces the one that was still sitting at `665bea2` — you asked for that twice and were right to. I ran the tree rather than reading the claim: `deno fmt --check` 232 clean, `deno lint` 142 clean, `deno task check` all five entrypoints, `deno task test` **251 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 `verify` job takes main's deploy with it I said last time that `verify` "should not merge to main in a state where it fails on every pull request". That undersells it. `verify` carries no `if:`, and `build` carries `needs: verify`. The job list for run #425 — this PR's head — is the whole argument: ``` verify failure build skipped needs: [verify] ``` 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-on` does not save it, because it is the obvious objection and I made it myself before reading the log. `cth-ubuntu-latest` maps to catthehacker's full image, which does have node — but `container:` overrides where steps execute, and `runs-on` only picks the machine. `build` has never had a `container:` 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. `verify` is the first job in this repository to set `container:`, and the log says exactly where the exec lands: ``` 🐳 docker create image=denoland/deno:2.5.6 entrypoint=["tail" "-f" "/dev/null"] 🐳 docker cp src=/data/.cache/act/b3/… dst=/var/run/act/actions/b3/… 🐳 docker exec cmd=[node /var/run/act/actions/b3/…/dist/index.js] OCI runtime exec failed: exec: "node": executable file not found in $PATH: unknown ``` `docker exec` into the deno container. The runner image's node is on the far side of that boundary. act_runner copies the action's `dist/index.js` in; 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: 1. **Drop `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 the `docker cp` dance, because the runner executes steps inside its own container, so `docker run -v "$PWD":/w` mounts a path the host daemon does not know and lands on nothing. `.forgejo/deno.sh` on #11 is that, with both reasons written down; say the word and I will port it across. 2. **Keep `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:242` returns the in-flight promise to a second caller. `runTask` at `:414` awaits it immediately after the POST — and a request already in flight at that moment is one issued *before* the job existed. So `jobs` comes back without the new run: `job.adopt(null)`, and `starting[id] = false` at `:416` while 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 give `runTask` an uncoalesced call. My "single-flight already admits one request" is what let it through, so this one is mine. **2. `ALREADY` no longer proves the line names the image being pinned.** `pin.ts:160` returns it before the `currentImage` guard 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. File `nginx:1.27`, container `httpd:1.25`, pin to `1.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 `:90` still describes a guard that branch no longer reaches. Comparing the repo parts before returning `ALREADY` restores it without undoing A. **3. The `..` substring test is still live in the other guard.** Round three fixed `hostPath` to normalise rather than substring-match, because `env_file: ../shared/.env` is legal and is what `resolveEnvPath` emits. `compose_update.ts:77` still does `path.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. `:83` also builds `${prefix}${f}` by hand, which makes `mod.ts:121`'s "the single place a host path is built" aspirational again — and `prefix` is untrimmed, so a `hostRoot` with 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:317` checks the deadline before dequeuing. Nothing bounds an item already running, and `listTags` is up to 20 pages at `TAG_TIMEOUT_MS` each — 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 an `AbortSignal`. **5. `retag`/`tagOf` mangle a digest-pinned ref.** `pin.ts:241`: `lastIndexOf(":")` on `postgres@sha256:abc…` lands inside the digest, so retagging produces `postgres@sha256:16.4`. `parseImageRef` then refuses anything with `@`, and `mod.ts:1446` reports `no 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:104` returns `{digest: null, error: null}` when the response is 200 without `docker-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 onto `job.lines` directly, skipping the `MAX_LINES` trim `record()` applies. One line, so only theoretically unbounded. - `ConfirmDialog.svelte:47` — the focus effect depends on `open`, `busy`, `variant` and both element refs, so it re-focuses on any of them changing, including `busy` going 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:145` vs `:92` — for a four-component pin, `listUpgrades().minor` admits `shared === 1` while `withinScope("minor")` requires `fixed = 2`, so `upgradeOptions.minor` can 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:215` and `:82` — `rootOf` runs once per file for grouping and again inside `commitOne`, so two `git rev-parse` per file. Caching the group key covers it. - `deno fmt` joined 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:16` all now read `Switch 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. `withRepoLock` and the staged-index refusal. `restore.keep`'s first-wins against the base each hunk measures from. `release()`'s abandoned-versus-force semantics and `JobFollow`'s two `wasRunning` guards. The phase carry-forward on both sides of the wire. `compose_progress`'s rate differentiation, the `DOWNLOAD_SETTLED` rebase and `pullState`. `canonicalRef` — `library/` implicit on Hub only, ports and digests intact. `nextPage`'s cross-host refusal. `localImages`'s bare-`:latest` aliasing. `update.ts` is 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_CHANGES` rests 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.
ci: run verify on the runner, entering the pinned image per command
All checks were successful
Build and Deploy / verify (pull_request) Successful in 52s
Build and Deploy / build (pull_request) Has been skipped
4efffd5dbd
The verify job never ran a single check. `container: denoland/deno:2.5.6`
moves step execution into that image, actions/checkout is a Node action,
and the image ships no node — so every run of this PR died at checkout
with `exec: "node": executable file not found in $PATH`, and `build`
(needs: verify) was skipped with it. Merged as it was, the next push to
main would have built, pushed and deployed nothing until someone fixed
the workflow on main. Worse than the breakage: I added this job and
never read one of its runs; every "verify gates this" sentence in this
PR's history was written about a job that had only ever failed.

The fix is taken from ci/dependency-update-issues (#11), where it is
already green on this runner: .forgejo/deno.sh is copied verbatim from
that branch — credit to it for both halves of the trick. The job runs on
the runner, where node exists, and enters the pinned image per command.
The image is read from docker/Dockerfile's FROM line so there is exactly
one toolchain pin, and the workspace is docker-cp'd in because the
runner executes inside its own container, so a bind mount of $PWD would
name a path the host daemon does not have.

One chained `deno task ci` instead of a step per check: every deno.sh
call is a fresh container, so separate steps would throw away the
`deno install --frozen` three times over and re-download for each.

Verified by running the exact command locally through the exact helper:
exit 0, 247 passed, 4 ignored — the git-dependent tests skip inside the
image, which is their designed behaviour.
Both from the fourth review pass.

**loadJobs freshness.** runTask awaits loadJobs right after its POST, and
plain single-flight handed it whatever was already in flight — a request
begun before the job existed. The row was then released against a
snapshot without the new run, and projectBusy reads that list, so a
queued destructive confirmation could mount for a project that was in
fact running. The reviewer's own framing was exact: dropping the epoch
fixed ordering, but coalescing never guaranteed freshness. 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.

**ALREADY no longer vouched for the repository.** Moving the ALREADY
check above the currentImage guard (finding A) removed the only
comparison on that path, so `nginx:1.27` in the file answered "already
written" to a pin of httpd:1.27 — same tag, wrong repository. A
repo-only guard now sits before the ALREADY check; the full comparison
stays below it, because in the half-applied state the tags legitimately
differ. The regression test failed before the guard landed and passes
after — red first by accident, but red first.

Also: the update budget's comment now says what the code does (bounds
starts, does not interrupt a running item); release() applies the same
MAX_LINES cap as record(), which was the one other writer of job.lines;
and the two caveman directives deno fmt had reflowed into one run-on
sentence in five agent-instruction files are separate lines again,
backtick-wrapped so the formatter keeps its hands off.
fix(docker): composeEligibility normalises paths like its twin
All checks were successful
Build and Deploy / verify (pull_request) Successful in 48s
Build and Deploy / build (pull_request) Has been skipped
ef7944c49f
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.
Author
Owner

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: verify had never executed a single check, build was skipped with it on needs:, 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.sh copied 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 from docker/Dockerfile's FROM, so the toolchain pin stays singular. One deviation from your usage: a single chained deno task ci instead of a step per check, because each deno.sh call is a fresh container and separate steps would throw away the deno install --frozen three times over.

Verified in the order that was missing:

  • the exact command through the exact helper locally, before pushing: exit 0, 247 passed, 4 ignored (the git-dependent tests skip inside the image, by design);
  • run #430: verify success in 53s, log carries the real ok | 247 passed | 0 failed | 4 ignored, build skipped on the PR-only gate as intended;
  • run #431 (the next push): success in 48s.

The workflow comment now records why container: must not come back.

Findings 1 and 2

1, loadJobs freshness — taken, and your framing was the correct one: dropping the epoch fixed ordering, but coalescing never guaranteed freshness, and 02c2886 promoting jobs into projectBusy'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, ALREADY vouching 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 the ALREADY check; 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: composeEligibility normalises like hostPath now, the prefix is trimmed, and there are two tests (sub/../compose.yml accepted, /../etc/passwd refused 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, rootOf caching, 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() applies record()'s MAX_LINES cap, and the five agent-instruction files have their two directives on separate lines again, backtick-wrapped so the formatter keeps its hands off.

State

b92f637 and ef7944c are green on CI (#430, #431 — read, not assumed). 254 tests locally, 247 + 4 ignored in the image. Our build.yml delta 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.

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: `verify` had never executed a single check, `build` was skipped with it on `needs:`, 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.sh` copied 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 from `docker/Dockerfile`'s `FROM`, so the toolchain pin stays singular. One deviation from your usage: a single chained `deno task ci` instead of a step per check, because each `deno.sh` call is a fresh container and separate steps would throw away the `deno install --frozen` three times over. Verified in the order that was missing: - the exact command through the exact helper locally, before pushing: exit 0, 247 passed, 4 ignored (the git-dependent tests skip inside the image, by design); - **run #430**: `verify` success in 53s, log carries the real `ok | 247 passed | 0 failed | 4 ignored`, `build` skipped on the PR-only gate as intended; - **run #431** (the next push): success in 48s. The workflow comment now records why `container:` must not come back. ## Findings 1 and 2 **1, `loadJobs` freshness** — taken, and your framing was the correct one: dropping the epoch fixed ordering, but coalescing never guaranteed freshness, and `02c2886` promoting `jobs` into `projectBusy`'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, `ALREADY` vouching 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 the `ALREADY` check; 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: `composeEligibility` normalises like `hostPath` now, the prefix is trimmed, and there are two tests (`sub/../compose.yml` accepted, `/../etc/passwd` refused 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, `rootOf` caching, 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()` applies `record()`'s `MAX_LINES` cap, and the five agent-instruction files have their two directives on separate lines again, backtick-wrapped so the formatter keeps its hands off. ## State `b92f637` and `ef7944c` are green on CI (#430, #431 — read, not assumed). 254 tests locally, 247 + 4 ignored in the image. Our `build.yml` delta 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.
Merge remote-tracking branch 'origin/main' into integration/docker-overhaul
All checks were successful
Build and Deploy / verify (pull_request) Successful in 52s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m10s
903ae7cd86
julian requested changes 2026-08-10 20:49:22 +02:00
Dismissed
julian left a comment

Fifth pass, at 903ae7c (your integration/docker-overhaul with origin/main merged 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 verify success, #431 the push after, and #436/#437 on the merge head 903ae7c — read, not assumed. The blocker that had never once executed a check now executes them and passes. .forgejo/deno.sh copied from #11 is the right shape: on the runner, entering the pinned image per command, workspace docker cp'd in. fmt/lint/check clean 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 — verify runs fork-supplied code on the deploy runner, ungated

This PR gives build.yml a pull_request: trigger and a verify job (28 additions, the first container:-free CI job in the repo to shell into docker). verify has no if:. Its one step runs ./.forgejo/deno.sh task ci, and deno.sh drives the host docker daemon directly — docker create / cp / start / wait against whatever socket the runner exposes. On a pull_request event, actions/checkout lays down the PR head's copy of deno.sh and deno.json, so a pull request supplies the very script that then runs on cth-ubuntu-latest — the same runner that holds PACKAGE_TOKEN and DEPLOY_SSH_KEY on 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 —

if: >-
  github.event_name != 'pull_request' ||
  github.event.pull_request.head.repo.full_name == github.repository

A fork's pull request supplies its own copy of that script … daemon access is host root, on the same runner build.yml uses with PACKAGE_TOKEN and DEPLOY_SSH_KEY.

deps.yml deemed the guard necessary for a job that runs deno.sh. build.yml's verify runs deno.sh and 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: on verify, 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 what deps.yml chose not to rely on. Do the same here — defense in depth and internal consistency both point one way.

Confirmed closed from the fourth pass

  • The CI blockerverify on 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.
  • 1, loadJobs freshnessjobsTail/jobsQueued gives 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.
  • 2, ALREADY vs. the wrong repository — repo-only guard sits above the ALREADY check, full comparison stays below, and pin_test.ts has the nginx:1.27/httpd:1.27 regression test. Reordering A did remove the only comparison on that path; this puts one back at the right altitude.
  • 3, composeEligibility — normalises segments like hostPath, trims the prefix, two tests (sub/../compose.yml accepted, /../etc/passwd refused).
  • Budget comment now says "bounds starts, not work"; release() applies record()'s MAX_LINES cap; 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 test is 299 passed, 4 failed — the three commit_test.ts git probes and compose_update_test.ts's "a legal ../ path is accepted" eligibility case. Cause is path shape, not logic: rootOf returns C:/Users/…, the file paths carry \, file.startsWith(root + "/") misses so the diff --git header keeps an absolute path and git apply --cached rejects it; and makeTempDir() hands back C:\…, which fails startsWith("/"). 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 a Deno.build.os === "windows" skip (same spirit as the ignore: !hasGit gate already there), or normalise separators, so the tree is green where it's actually being reviewed.

b. checkUpdates can 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 fixed state.stacks order 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 get previous ?? "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.ts re-implements updates.ts's parse/bearer/paginate, and the copy lacks 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, which is the field-drift you and thisilike already patched in lockstep once; and the /container/:id/pin and /stack/:project/pin routes 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_CHANGES is the verify fork guard alone. Add the if: 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.

Fifth pass, at `903ae7c` (your `integration/docker-overhaul` with `origin/main` merged 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 `verify` success, #431 the push after, and #436/#437 on the merge head `903ae7c` — read, not assumed. The blocker that had never once executed a check now executes them and passes. `.forgejo/deno.sh` copied from #11 is the right shape: on the runner, entering the pinned image per command, workspace `docker cp`'d in. `fmt`/`lint`/`check` clean 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 — `verify` runs fork-supplied code on the deploy runner, ungated This PR gives `build.yml` a `pull_request:` trigger and a `verify` job (28 additions, the first `container:`-free CI job in the repo to shell into docker). `verify` has no `if:`. Its one step runs `./.forgejo/deno.sh task ci`, and `deno.sh` drives the **host** docker daemon directly — `docker create` / `cp` / `start` / `wait` against whatever socket the runner exposes. On a `pull_request` event, `actions/checkout` lays down the PR head's copy of `deno.sh` and `deno.json`, so a pull request supplies the very script that then runs on `cth-ubuntu-latest` — the same runner that holds `PACKAGE_TOKEN` and `DEPLOY_SSH_KEY` on 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 — ```yaml if: >- github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository ``` > A fork's pull request supplies its own copy of that script … daemon access is host root, on the same runner build.yml uses with PACKAGE_TOKEN and DEPLOY_SSH_KEY. `deps.yml` deemed the guard necessary for a job that runs `deno.sh`. `build.yml`'s `verify` runs `deno.sh` and 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:` on `verify`, 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 what `deps.yml` chose not to rely on. Do the same here — defense in depth and internal consistency both point one way. ### Confirmed closed from the fourth pass - **The CI blocker** — `verify` on 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. - **1, `loadJobs` freshness** — `jobsTail`/`jobsQueued` gives 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. - **2, `ALREADY` vs. the wrong repository** — repo-only guard sits above the `ALREADY` check, full comparison stays below, and `pin_test.ts` has the `nginx:1.27`/`httpd:1.27` regression test. Reordering A did remove the only comparison on that path; this puts one back at the right altitude. - **3, `composeEligibility`** — normalises segments like `hostPath`, trims the prefix, two tests (`sub/../compose.yml` accepted, `/../etc/passwd` refused). - Budget comment now says "bounds starts, not work"; `release()` applies `record()`'s `MAX_LINES` cap; 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 test` is **299 passed, 4 failed** — the three `commit_test.ts` git probes and `compose_update_test.ts`'s "a legal ../ path is accepted" eligibility case. Cause is path shape, not logic: `rootOf` returns `C:/Users/…`, the file paths carry `\`, `file.startsWith(root + "/")` misses so the `diff --git` header keeps an absolute path and `git apply --cached` rejects it; and `makeTempDir()` hands back `C:\…`, which fails `startsWith("/")`. 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 a `Deno.build.os === "windows"` skip (same spirit as the `ignore: !hasGit` gate already there), or normalise separators, so the tree is green where it's actually being reviewed. **b. `checkUpdates` can 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 fixed `state.stacks` order 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 get `previous ?? "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.ts` re-implements `updates.ts`'s parse/bearer/paginate, and the copy lacks 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, which is the field-drift you and thisilike already patched in lockstep once; and the `/container/:id/pin` and `/stack/:project/pin` routes 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_CHANGES` is the `verify` fork guard alone. Add the `if:` 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, test
run: ./.forgejo/deno.sh task ci
Owner

verify has no if:, and this step runs deno.sh, which drives the host docker daemon (docker create/cp/start/wait). On a pull_request event the checked-out deno.sh and deno.json are the PR head's copies, so a pull request supplies the script that runs here — on cth-ubuntu-latest, the runner that holds PACKAGE_TOKEN/DEPLOY_SSH_KEY on the next push to main.

deps.yml (merged in #11) guards the identical case:

if: >-
  github.event_name != 'pull_request' ||
  github.event.pull_request.head.repo.full_name == github.repository

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, and deps.yml already decided this script is not safe to run for an arbitrary pull request.

`verify` has no `if:`, and this step runs `deno.sh`, which drives the host docker daemon (`docker create`/`cp`/`start`/`wait`). On a `pull_request` event the checked-out `deno.sh` and `deno.json` are the PR head's copies, so a pull request supplies the script that runs here — on `cth-ubuntu-latest`, the runner that holds `PACKAGE_TOKEN`/`DEPLOY_SSH_KEY` on the next push to main. `deps.yml` (merged in #11) guards the identical case: ```yaml if: >- github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository ``` 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, and `deps.yml` already decided this script is not safe to run for an arbitrary pull request.
ci: gate verify to same-repository pull requests
All checks were successful
Build and Deploy / verify (pull_request) Successful in 55s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m1s
fd07ab1def
The step runs .forgejo/deno.sh, which drives the host docker daemon, and
on a pull_request event checkout lays down the PR head's own copy of
that script — so a fork's pull request supplies the code that then runs
on the runner holding PACKAGE_TOKEN and DEPLOY_SSH_KEY for the next push
to main. Daemon access is host root; secrets are withheld from fork runs,
so the exposure is the runner rather than the secrets directly.

deps.yml already carries this exact clause for a job that runs the same
script, and says why. Adding the pull_request trigger to build.yml
without it left the two workflows disagreeing about whether deno.sh is
safe to run for a pull request. Same clause, same reason.
Author
Owner

fd07ab1 adds the if: — the same clause deps.yml carries, 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 to build.yml so my verify job 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 — but deps.yml declining to rely on that setting is exactly the precedent, and two workflows in one repo disagreeing about whether deno.sh is 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:

  • a, the Windows failures. These are my tests and they are red on the box where this repo is actually reviewed, which makes them worse than the count suggested — "251 local / 245 + 4 ignored" only ever held on POSIX. Deno.build.os === "windows" skip alongside the existing ignore: !hasGit, same spirit.
  • b, tail starvation in 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.
  • c, the duplication. Two registry v2 clients with the same-host token guard missing from one, two tag parsers already drifted by a word, 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-ALREADY bug 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.

`fd07ab1` adds the `if:` — the same clause `deps.yml` carries, 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 to `build.yml` so my `verify` job 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 — but `deps.yml` declining to rely on that setting is exactly the precedent, and two workflows in one repo disagreeing about whether `deno.sh` is 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: - **a, the Windows failures.** These are my tests and they are red on the box where this repo is actually reviewed, which makes them worse than the count suggested — "251 local / 245 + 4 ignored" only ever held on POSIX. `Deno.build.os === "windows"` skip alongside the existing `ignore: !hasGit`, same spirit. - **b, tail starvation in `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. - **c, the duplication.** Two registry v2 clients with the same-host token guard missing from one, two tag parsers already drifted by a word, `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-`ALREADY` bug 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.
julian approved these changes 2026-08-10 21:35:34 +02:00
julian left a comment

Sixth pass, at fd07ab1. One commit since the fifth, ten lines, all comment plus the if: — 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 runs verify unconditionally.

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 verify from 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 on fd07ab1, both pull_request events. Those durations are executed checks, not an all-skipped pass — #430's verify ran 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 checkUpdates tail 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.yml are SHA-pinned with the # v6 trailing comments dep-check reads, all three image refs in docker/Dockerfile are digest-pinned — including the COPY --from bin ref that was in nobody's list of base images — and the ssh-keyscan TOFU is gone, replaced by a committed, reviewable known_hosts with 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 whether deno.sh is safe for a pull request. The follow-ups have owners (#12 here, pinning watched by dep-check). This is ready.

Sixth pass, at `fd07ab1`. One commit since the fifth, ten lines, all comment plus the `if:` — 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 runs `verify` unconditionally. 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 `verify` from 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 on `fd07ab1`, both `pull_request` events.** Those durations are executed checks, not an all-skipped pass — #430's `verify` ran 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 `checkUpdates` tail 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.yml` are SHA-pinned with the `# v6` trailing comments dep-check reads, all three image refs in `docker/Dockerfile` are digest-pinned — including the `COPY --from` bin ref that was in nobody's list of base images — and the `ssh-keyscan` TOFU is gone, replaced by a committed, reviewable `known_hosts` with 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 whether `deno.sh` is safe for a pull request. The follow-ups have owners (#12 here, pinning watched by dep-check). This is ready.
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
OpsDeck/core!10
No description provided.