Follow-ups from #22: host path guards — the parts the approval left open #47

New issue

Closed

opened 2026-08-19 09:18:51 +02:00 by julian · 0 comments

julian commented

2026-08-19 09:18:51 +02:00

Owner

Non-blocking findings from @thisilike's approving review of #22 (review at f832cb2). None of these blocked the merge; all five are recorded here so they are not lost with the PR.

They are independent — take them separately or in one pass.

1. `hostFs.read`'s loud refusal reaches the operator by an accident of call placement

pin_apply.ts:59:

read: (file) => Deno.readTextFile(hostPath(file)),

hostPath throws synchronously, before the promise exists, so applyPin's read guard at pin_apply.ts:85-89 — which turns an unreadable file into "not here" — never attaches. That is what makes the new drive-under-mounted-root refusal visible.

Checked against the shipped code, both spellings:

shipped:       THREW -> drive path C:/Users/j/stack/compose.yml is only reachable wh...
async variant: returned -> {"ok":false,"reason":"no-image-line"}

The async variant is read: async (f) => await Deno.readTextFile(hostPath(f)) — a refactor with no intent behind it — and it silently converts the named refusal into no-image-line ("no compose file declares an image for this service"), on the write path the refusal exists to protect.

Fix: either a comment at hostFs saying the call placement is load-bearing, or narrow the catch to Deno.errors.NotFound so only a genuinely absent file becomes "not here". The second is the real fix.

2. `envFiles` is still outside the one definition

composeProject reads three sets of label paths (compose_update.ts:52-70). composeEligibility resolves and stats two of them (:96). envFiles is not resolved, not refused, not stat'd — and goes straight to the host compose CLI at compose_update.ts:158 as --env-file.

The guard's own docstring is "are the project's files really there … a stack whose file moved or died must fall back to the API updater instead of failing mid-run", and a missing --env-file fails the real update exactly the way a missing -f does.

Pre-existing, not a regression — but sharper after #22, because the same label (com.docker.compose.project.environment_file) now gets a dedicated normalising function in wantedEnvFiles for the stack page while staying raw and unchecked for the invocation. The working directory was added to the loop for precisely this argument; the env files are the third leg.

3. `effectiveHostRoot` widens the strategy to `string`

hostpath.ts:113 takes { host: { strategy: string | null } }, but HostStrategy = "chroot" | "nsfile" | "pidns" | "direct" is exported from @opsdeck/sdk (sdk/mod.ts:72) and this module already imports from it.

As typed, renaming a union member leaves the comparison compiling and silently always-false — and everything now rides on that one string equality. HostStrategy | null covers every row the test uses and costs an import. Same trade as deleting dir from CommitLines: make it structural.

4. The new README sentence stops one clause short

OPSDECK_HOST_ROOT=/ fixes the paths, and reads genuinely work after it. Anything through ctx.host still does not.

The probe's candidates are chroot / nsenter / nsenter / direct, and its functional test is itself a sh -c (hostexec.ts:135-141), with no Deno.build.os anywhere in host-exec. Without a POSIX sh on PATH the strategy stays null and moduleHostExec.run throws before spawning anything (hostexec.ts:236-238).

Concretely: on a Windows checkout the stack page works, and a pin dies at h.step("validate") (actions.ts:398, unguarded ctx.host.run) with "host command execution unavailable" — honestly reported, edit rolled back by the compensator, and entirely outside #22's diff. But README.md:48 is now the place the Windows setup is documented, so a clause saying host-exec (compose validate, the compose updater, the pin's git commit) needs a shell on PATH would stop the next person concluding the variable was the whole story.

5. Record that the working-directory stat's failure mode is graceful

composeEligibility stats the project working directory now — a POSIX behaviour change. A stack whose workingDir has gone away but whose compose files are still present was eligible and is now not.

The reasoning is right and #22's description owns the change. What is not written down anywhere is that the failure mode is graceful: the engine-API updater takes over, no run fails. Worth a line so nobody has to work that out from the diff.

Refs #20, #22.

Non-blocking findings from @thisilike's approving review of #22 ([review at `f832cb2`](https://git.imhof.cloud/OpsDeck/core/pulls/22#issuecomment-824)). None of these blocked the merge; all five are recorded here so they are not lost with the PR. They are independent — take them separately or in one pass. --- ### 1. `hostFs.read`'s loud refusal reaches the operator by an accident of call placement `pin_apply.ts:59`: ```ts read: (file) => Deno.readTextFile(hostPath(file)), ``` `hostPath` throws **synchronously**, before the promise exists, so `applyPin`'s read guard at `pin_apply.ts:85-89` — which turns an unreadable file into "not here" — never attaches. That is what makes the new drive-under-mounted-root refusal visible. Checked against the shipped code, both spellings: ``` shipped: THREW -> drive path C:/Users/j/stack/compose.yml is only reachable wh... async variant: returned -> {"ok":false,"reason":"no-image-line"} ``` The async variant is `read: async (f) => await Deno.readTextFile(hostPath(f))` — a refactor with no intent behind it — and it silently converts the named refusal into `no-image-line` ("no compose file declares an image for this service"), on the write path the refusal exists to protect. **Fix:** either a comment at `hostFs` saying the call placement is load-bearing, or narrow the catch to `Deno.errors.NotFound` so only a genuinely absent file becomes "not here". The second is the real fix. ### 2. `envFiles` is still outside the one definition `composeProject` reads three sets of label paths (`compose_update.ts:52-70`). `composeEligibility` resolves and stats two of them (`:96`). `envFiles` is not resolved, not refused, not stat'd — and goes straight to the host compose CLI at `compose_update.ts:158` as `--env-file`. The guard's own docstring is "are the project's files really there … a stack whose file moved or died must fall back to the API updater instead of failing mid-run", and a missing `--env-file` fails the real update exactly the way a missing `-f` does. Pre-existing, not a regression — but sharper after #22, because the *same label* (`com.docker.compose.project.environment_file`) now gets a dedicated normalising function in `wantedEnvFiles` for the stack page while staying raw and unchecked for the invocation. The working directory was added to the loop for precisely this argument; the env files are the third leg. ### 3. `effectiveHostRoot` widens the strategy to `string` `hostpath.ts:113` takes `{ host: { strategy: string | null } }`, but `HostStrategy = "chroot" | "nsfile" | "pidns" | "direct"` is exported from `@opsdeck/sdk` (`sdk/mod.ts:72`) and this module already imports from it. As typed, renaming a union member leaves the comparison compiling and silently always-false — and everything now rides on that one string equality. `HostStrategy | null` covers every row the test uses and costs an import. Same trade as deleting `dir` from `CommitLines`: make it structural. ### 4. The new README sentence stops one clause short `OPSDECK_HOST_ROOT=/` fixes the *paths*, and reads genuinely work after it. Anything through `ctx.host` still does not. The probe's candidates are `chroot` / `nsenter` / `nsenter` / `direct`, and its functional test is itself a `sh -c` (`hostexec.ts:135-141`), with no `Deno.build.os` anywhere in host-exec. Without a POSIX `sh` on `PATH` the strategy stays `null` and `moduleHostExec.run` throws before spawning anything (`hostexec.ts:236-238`). Concretely: on a Windows checkout the stack page works, and a pin dies at `h.step("validate")` (`actions.ts:398`, unguarded `ctx.host.run`) with "host command execution unavailable" — honestly reported, edit rolled back by the compensator, and entirely outside #22's diff. But `README.md:48` is now the place the Windows setup is documented, so a clause saying host-exec (compose validate, the compose updater, the pin's git commit) needs a shell on `PATH` would stop the next person concluding the variable was the whole story. ### 5. Record that the working-directory stat's failure mode is graceful `composeEligibility` stats the project working directory now — a POSIX behaviour change. A stack whose `workingDir` has gone away but whose compose files are still present was eligible and is now not. The reasoning is right and #22's description owns the change. What is not written down anywhere is that the failure mode is graceful: the engine-API updater takes over, no run fails. Worth a line so nobody has to work that out from the diff. --- Refs #20, #22.

julian referenced this issue from a commit

2026-08-19 10:04:11 +02:00

fix(docker): close the five host-path follow-ups #22 left open

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

2026-08-19 10:04:32 +02:00

fix(docker): close the five host-path follow-ups #22 left open #48

julian referenced this issue from a commit

2026-08-20 11:04:29 +02:00

fix(docker): the env-file answer stops depending on recreation order

julian referenced this issue from a commit

2026-08-21 00:55:50 +02:00

fix(docker): the newest labelled container answers for --env-file, alone