fix(docker): make the path guards and commit tests hold on a Windows checkout #22
thisilike
julian
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/issue-20-windows-test-paths"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #20 — the four Windows-only test failures, taken the deliberate way the issue asked for: normalise, don't guard, so the tests answer whether
rootOf/hostPathactually work on Windows paths. Three review rounds later, the shape of the fix is: one module decides what a host path is —hostpath.tsowns the arithmetic, the platform switch, and the root.The arithmetic, once.
posixPath(fold\to/— only where\is a separator),resolvePathSegments(the one definition of "absolute": drive absolutes accepted,C:relativerefused, UNC refused by name,./..folded, escapes reported) andjoinHostPath(the one join) live inhostpath.ts;compose_update.ts,compose.tsandcommit.tsimport them. The platform switch is a parameter everywhere (windows = Deno.build.os === "windows"), so every Windows row runs on Linux CI — including an every-OS variant of the pure-backslash commit test, end to end through real git.The root, once — and the pairing it cannot serve is refused, not joined.
effectiveHostRoot(ctx): strategydirectmeans the process is already on the host, so the mount prefix is/no matter whatOPSDECK_HOST_ROOTsays;null(probe failed, or not yet run) keeps the configured root, which is right in a container with a broken chroot. On Windowsnullis the only reachable state — every probe candidate is a POSIX construct — so nothing can ever pair a drive label with/, and the earlier claim that "drive labels exist only where the strategy is direct" was enforced by nothing.joinHostPathnow refuses a drive absolute under a non-/root by name; the message saysOPSDECK_HOST_ROOT=/, and the README puts that variable besideDOCKER_HOSTin the Windows dev setup.makeHostPathtakes a thunk, because the strategy is probed afterregister()runs — which narrows the pre-probe window to sub-second and self-healing; it does not close it.The eligibility guard cannot make up its own root.
composeEligibilitytakes the ctx and derives the root viaeffectiveHostRootitself — a plain root-string parameter is exactly how a call site disagreed withhostPathlast round, and three call sites agreeing by convention is not a guarantee. It also now stats the working directory it was already resolving (compose is addressed with--project-directory, so a missing directory fails the real update as surely as a missing-ffile), and the join's drive refusal surfaces as the eligibility reason.commitLinesderives every directory itself. Thedirparameter is gone from the type: both callers used to compute it from the raw label with a/-onlylastIndexOf, which on a pure-backslash path ate the last character and turned every Windows pin into "not a work tree". Paths are normalised at the entry; a drive-root file asks git aboutC:/, not drive-relativeC:. "Nothing needed writing" has one spelling,NOTHING_TO_COMMIT.UNC is refused as UNC.
\\wsl$\…and\\server\share(Docker Desktop's WSL2 backend emits these) get their own union member and their own message — they are rooted, and refusing them as "relative" was a message the operator had to disbelieve before debugging. Folded, they would masquerade as/-rooted and stat somewhere else entirely.The stack page dedupes env files by resolved path — and the dedup is now
wantedEnvFiles()incompose.ts, a function with the platform switch threaded, instead of route-handler code a test could not reach. Keys areposixPath-normalised, so the label'sC:\…\stack\.envand the derived${projectDir}/.envare one entry, not two; label files stay required, the derived default stays optional.Temp dirs are canonicalised with
Deno.realPathin the tests: TMP is an 8.3 short name on Windows (C:\Users\JULIAN~1\…) while git prints the canonical long path, and macOS's/varis a symlink into/private— the root comparison could never match either way.What changed on POSIX. The fold and the drive parsing are gated to Windows, so
/srv/a\b/compose.ymlstays one segment on POSIX, a directory literally namedC:stays relative, and//srv/xstays accepted. Two POSIX behaviours did move, deliberately:composeEligibilitystats the resolved path rather than the raw concatenation (/srv/gone/../compose.ymlunder a mounted root now stats…/srv/compose.yml), and it stats the project working directory at all. Both are the guard agreeing with what an update would actually do.Verification — Windows checkout at this head: docker backend 215 passed / 0 failed / 0 ignored (the Windows-only twins run natively);
deno task check,deno fmt --check,deno lintclean. Server suite 101 passed / 1 failed:external_modules_test.ts"a module that loads after the app is built still gets its API routes", which fails identically on the parent commit with a stashed tree — environment, not this diff.🤖 Generated with Claude Code
Reviewed by applying the diff onto the base (
b11a279) in a clean worktree and running the suite plus probes against both sides of the change.The dedup itself is sound and it typechecks. But three of the claims in the description do not hold, and the Windows support this advertises stops short of working — a Windows checkout still cannot pin.
Numbers first, because they matter for how much this is verified: base is 303 passed / 0 failed on Linux, and the patched tree is 303 passed / 0 failed on Linux. The PR adds no test. Every behaviour introduced below — drive parsing, backslash folding, the
hostPathdrive join — is untested on every platform.Claims vs. what the code does
1. "POSIX behavior unchanged" — not true
compose_update.ts:82normalises backslashes unconditionally:\is a legal filename character on Linux. Comparing the newhostPathagainst the old one on the base:hostPathgates writes —mod.ts:1490,mod.ts:1687,restore.ts:55. So a Linux directory with a backslash in its name now has the operator's compose file written to a different path than the label names. It is not an escape (the folding and the escape check still run, and in fact run on more segments than before), but it is a silent retarget on the platform the product actually runs on, taken on for the benefit of a dev environment.The description's supporting sentence — "no POSIX code path gains a drive prefix" — is true and is the wrong half. The drive prefix is not what changes POSIX behaviour; the backslash replace is, and it is not mentioned.
Gate it:
Deno.build.os === "windows", or take the separator convention from the caller.2. "the join now handles it" — in one of the two guards
The description says the missing-separator case (
/host/rootC:/...) was folded in. It was, inhostPath.composeEligibilitystill concatenates raw, atcompose_update.ts:121:and this PR is precisely what makes drive paths reach that line — before, they were refused by
startsWith("/")one loop earlier. Observed:Same defect, same PR, sibling function. The stated premise is that the two guards drifting apart is how the last regression happened — unifying the check while leaving the two consumers to each make up their own mind about the result is that same drift with a new address.
(Pre-existing and worth noting separately:
composeEligibilitycomputes the resolved segments and then discards them, stating the unresolved path. Not introduced here.)3. "a Windows checkout is a supported dev environment" — pinning still will not commit on one
commitLinesnormalisesedits[].fileatcommit.ts:214. Both callers computedirfrom the raw label path first, with the same/-only assumption this PR set out to remove —mod.ts:1537andmod.ts:1754:On the pure-backslash path the description says Docker Desktop writes:
The last character is eaten, and the result is non-empty so the
|| "/"fallback never fires. That string goes tocommitOne→rootOf→git -C <that> rev-parse --show-toplevel, which fails, sorootis null and every pin returns "not a work tree": file rewritten, nothing committed.The suite cannot see this.
commit_test.ts:101passes the directory straight in asdir, and builds the file path with a forward-slash join (C:\Users\…\tmpX+/compose.yml), so the only shape under test is mixed-separator and the caller is never exercised. The comment added atcommit_test.ts:53-60says separators stay OS-native so the tests exercisecommitLines' own normalisation — for the temp-dir prefix that is true and it is what makes the root comparison match, but the file paths are/-joined, so the shape that actually breaks the product never appears.This is what makes it blocking rather than a nit: issue #20 offered guarding or normalising, and said normalising is worth it because it "tells us whether
rootOf/hostPathactually work on a Windows path". The tests now go green while the answer to that question is still no.Also
UNC / WSL paths are mangled rather than refused —
compose_update.ts:83. The drive regex does not match a UNC path, and after normalisation it starts with/, so it sails through as rooted:Docker Desktop with the WSL2 backend emits exactly these. A loud refusal became a quiet wrong path, in the case the change was written for. A
//-prefix check returning"relative"covers it.resolveEnvPathstill keeps its own idea of "absolute" —compose.ts:62:C:\…\prod.envis therefore treated as relative and joined onto the project dir, producingC:\Users\j\stack/C:\Users\j\prod.env, which then goes tohostPathviamod.ts:666→mod.ts:627. Third copy of the arithmetic this PR set out to unify, untouched by it.No test for
resolvePathSegments. It is newly exported specifically so both guards can share it — that is exactly the seam a table test belongs on, and roughly twenty lines of one would have caught findings 1, 2 and 4.hostPath's drive join is untestable as written, being a closure insideregister(); if that join was worth a description paragraph it is worth being reachable from a test.commit.ts:214reassigns theeditsparameter, and normalisesedits[].filebut not the siblingdirargument — so the single-group path handscommitOnean un-normaliseddirwhile the multi-group path (commit.ts:239, viadirOf) hands it a normalised one. Harmless today sincedironly feedsgit -C, but the comment says "Normalise once, here" and it is normalising one of the two path inputs.What holds up
The dedup is real: one definition, both guards call it, and the arithmetic is faithfully preserved. Escape semantics are intact —
/../etc/passwdis still refused with the exact message the test asserts, and../shared/.envis still accepted once resolved. The{ drive, segs } | "relative" | "escapes"union narrows cleanly at both call sites, andC:relativecorrectly stays refused.Deno.realPathon the temp dirs is the right fix for both the 8.3 short-name trap and the macOS/var→/privateone, and it is the kind of fix that keeps paying off.Blocking
resolvePathSegments— it is a write guard and it retargets legal POSIX paths.mod.ts:1537/mod.ts:1754— as shipped, this closes four red tests without making the behaviour they stand for work on Windows.The rest (eligibility's raw concatenation, UNC,
resolveEnvPath, the missing table test) I would like to see in the same pass, since they are all the same question — which code is allowed to decide what "absolute" means — and answering it in one place is the whole point of the change.All findings addressed in
09d2bfc, taken as the one question you framed them as:resolvePathSegmentsis now the only code that decides what "absolute" means, and the platform switch exists in exactly one place.Blocking 1 — unconditional backslash replace. The folding moved into an exported
posixPath(p, windows = Deno.build.os === "windows")and only runs on a Windows checkout. Drive parsing is gated the same way, so POSIX also does not gain the drive prefix for a directory literally namedC:. Your/srv/stacks/a\b/compose.ymlexample is pinned in the table test: one segment on POSIX, folded on Windows.Blocking 2 —
mod.ts:1537/mod.ts:1754.commitLineslost itsdirparameter entirely; it derives every directory from the normalised patched paths itself. Letting callers hand in a directory computed from the raw label is exactly the reintroduction path, so the signature no longer offers it. A Windows-only test now feeds the pure-backslash shape the routes actually pass (C:\...\compose.yml, no/anywhere) end to end through a real repository and asserts "committed" — the shape that madelastIndexOf("/")eat the last character.Eligibility's raw concatenation.
joinHostPathis exported and shared:hostPathbuilds the write path with it andcomposeEligibilitystats through it, so the read guard checks the exact path the write guard would produce. That also fixes the pre-existing discard-the-resolution defect you noted — eligibility now stats the resolved path, with a test where the..passes through a directory that does not exist (the OS cannot resolve that; the shared arithmetic can).UNC. Refused as
"relative"before drive parsing, on the Windows side of the switch —\\wsl$\...,\\server\share, and the//server/sharespelling are all in the table. POSIX//srv/xkeeps its old acceptance (empty segments fold away), since refusing it there would have been another POSIX behaviour change.resolveEnvPath. Delegates toresolvePathSegmentsinstead of keeping the thirdstartsWith("/")copy, soC:\...\prod.envon a Windows checkout stays absolute rather than being glued onto the project dir. An absolute-but-escaping ref is returned as-is and refused byhostPathat the read, same as before.Tests.
resolvePathSegments,posixPathandjoinHostPathhave table tests covering both platform settings — thewindowsparameter exists so every row runs on every OS, which is also what makes the drive join reachable without theregister()closure. Thecommit.tsparameter reassignment is gone andcommitOnereceives a normalised dir on the single-group path too.One incidental:
compose_update_test.tscarried two literal NUL bytes inside thejoin()strings on line 87 (pre-existing on main — the test passed because both sides joined with the same NUL, but git diffed the file as binary). They are spaces again, so this PR's test diff renders as text.deno task teston the Windows checkout: 310 passed / 0 failed (was 303).check,fmt,lintclean. Happy to split the NUL fix out if you'd rather keep it off this branch.Reviewed at
97249e0in a clean worktree. Basec768c9dis 436 passed / 0 failed on Linux; the head is 441 passed / 0 failed / 2 ignored, stable over three consecutive runs.deno task check,deno fmt --checkanddeno lintare clean, and the NUL bytes are gone (grep -Pc '\x00'→ 0).Both blocking findings from the last round are genuinely fixed, and fixed the right way — the
dirparameter is gone from theCommitLinestype, which is a structural guarantee rather than a test, and the platform switch really does live in one function with a parameter, so both answers run on both OSes. Details under "What holds up".One thing is still blocking, and it is the same question as last round — which code decides what a host path is — one level up from where this PR looked.
Blocking: the two guards share the arithmetic but not the host root
compose_update.ts:151now asserts:They do share the rule and the join. They are not handed the same
hostRoot. All threecomposeEligibilitycall sites pass a strategy-dependent root —actions.ts:208,actions.ts:280,actions.ts:565:while both
makeHostPathconstructions pass the configured one unconditionally —mod.ts:84andactions.ts:77:OPSDECK_HOST_ROOTdefaults to/host/root(server/src/config.ts:67), and the host-exec probe falls through todirectwhenever that mount is not usable (server/src/util/hostexec.ts:181-183, candidates["chroot", "nsfile", "pidns", "direct"]). So on any checkout running outside the container,strategyisdirectand the two guards resolve different paths for the same label:The second path is the one every read and every write actually uses:
mod.ts:557andmod.ts:606(the stack page's compose and env files),hostFs/makeFileRestorefor the pin writes (actions.ts:370,actions.ts:488), and the patch file atcommit.ts:169. All of them get ENOENT.Which makes the drive parsing this PR exists for dead on the only platform it is for.
README.md:48documents the Windows dev setup asDOCKER_HOST=tcp://localhost:2375and says nothing aboutOPSDECK_HOST_ROOT, so the default applies: a Windows checkout following the README reaches the new code path and still cannot read a compose file. It is not Windows-specific either —deno task starton Linux withoutOPSDECK_FAKE_DATAdiverges the same way; it is just invisible there because both sides fail on a path that looks plausible.compose_update_test.ts:178pins the broken half as expected output:By the standard this PR applies to UNC one screen earlier — refusing loudly beats a quiet wrong path — a drive absolute under a non-
/host root is precisely a quiet wrong path, and it is the combination the feature meets by default.Two ways out, and I do not mind which: give
makeHostPaththe same root the eligibility call sites compute (one helper, read wherestrategyis known — notemod.ts:84runs atregister(), so it has to be read lazily rather than snapshotted), or refuse a drive path whenhostRootis not/and put theOPSDECK_HOST_ROOT=/line in the README beside theDOCKER_HOSTone. What should not ship is a comment claiming the two agree while the one input that differs is the one that makes them disagree.Non-blocking
resolveEnvPath's output is not part of the one definition —mod.ts:583-598. Thewantedmap is documented as "deduped by resolved path" and is keyed on the raw string. On Windows the project label spells the default env fileC:\Users\j\stack\.env, whilemod.ts:591computes${projectDir}/.env→C:\Users\j\stack/.env. Different keys, one file, so the stack page lists it twice — and both entries read fine, becausehostPathfolds them to the same place. Keying onposixPath(...)is the whole fix. Same class as the three copies this PR deleted: a path decision taken outside the one definition.UNC is refused as
"relative"—compose_update.ts:107, surfacing athostpath.ts:18asrefusing relative compose path: \\wsl$\Ubuntu\…. Refusing it is right; calling an absolutely-rooted path relative is a message the operator has to disbelieve before they can debug it. A fourth union member, or just a different string at the throw.The arithmetic ended up in the compose-CLI runner.
hostpath.tsstill opens with "The single place a host path is built", and now imports both halves of that fromcompose_update.ts, whose own header is about runningdocker composeon the host.compose.ts— until now a parser with no imports at all — picks up the same dependency. Type-only transitively, so nothing breaks, butposixPath/resolvePathSegments/joinHostPathbelong inhostpath.tswithcompose_update.tsimporting them, not the reverse.The platform seam stops at the arithmetic.
posixPathandresolvePathSegmentstakewindowsso every row runs on every OS — exactly right — and thenmakeHostPath,makeCommitLines,composeEligibilityandresolveEnvPatheach readDeno.build.osimplicitly, so their Windows behaviour is only reachable on Windows. On this CI that is the 2 ignored tests, and one of them is the only regression guard for thelastIndexOf("/")bug that blocked the last round.makeHostPathandmakeCommitLinesare already factories taking arguments; threading the flag costs a parameter each and makes those rows run on Linux.dirOfon a drive-root file —commit.ts:236."C:/compose.yml".lastIndexOf("/")is 2, sodiris"C:"— on Windows that is the current directory on drive C:, notC:\, and it is non-empty so the|| "/"fallback cannot fire. The same shape as the bug this replaced, one drive letter narrower. An unlikely placement for a compose file; still oneif.The description is stale. It still describes the pre-
09d2bfcdesign: "POSIX behavior unchanged" with no mention of the OS gate that is what makes it true now,303 passed / 0 failed (was 299 / 4), and nothing about the removeddirparameter, the UNC refusal, orresolveEnvPathjoining the shared definition. The merge body is the record of why this landed; right now the follow-up comment is the accurate account and the description is not.What holds up
posixPath(p, windows)— folding gated to the platform where\is a separator, and drive parsing gated the same way, so a POSIX directory literally namedC:stays relative./srv/stacks/a\b/compose.ymlis one segment on POSIX and two on Windows, and both rows are in the table.CommitLineslost itsdirparameter, not just its bad callers. A caller cannot reintroduce a raw-label directory because the signature has nowhere to put one — better than the test would have been.joinHostPathis shared, so eligibility stats a resolved path instead of a raw concatenation. The..-through-a-nonexistent-directory test is the right test for it: the OS cannot answer that, only the arithmetic can.resolveEnvPathdelegates instead of keeping the third copy, and the absolute-but-escaping case is left tohostPath— which does refuse it, atmod.ts:606, inside atrythat reports the error rather than dropping the file.\\wsl$\,\\server\shareand the//server/sharespelling all in the table, and POSIX//srv/xdeliberately left accepted.Deno.realPathon the temp dirs — the fix for both the 8.3 short name and macOS/var, and the reason the root comparison can match at all.The last round unified the rule and the join; the two guards were still handed different ROOTS. Eligibility's call sites knew that strategy "direct" means the mount prefix is "/", while makeHostPath snapshotted the configured OPSDECK_HOST_ROOT at register() — same label, two paths, every read and write ENOENT on any checkout running outside the container. effectiveHostRoot(ctx) is now the one answer, and hostPath takes a thunk because the strategy is probed after register(). The rest of the review's findings, same question one level down: - posixPath / resolvePathSegments / joinHostPath move into hostpath.ts, which claims to be where host paths are defined; compose_update.ts imports them instead of exporting them. - UNC gets its own verdict ("unc") and its own refusal messages — calling a rooted path "relative" was a message the operator had to disbelieve before they could debug it. - makeCommitLines, composeEligibility and resolveEnvPath take the platform switch as a parameter, so the Windows rows run on Linux CI: the pure-backslash commit test — the only regression guard for the lastIndexOf bug — now has an every-OS variant through real git. - dirOf maps a drive-root file to "C:/", not drive-relative "C:" (the same shape as the bug it replaced, one letter narrower). - The stack page's env-file map keys on posixPath, so the label's C:\...\.env and the derived ${projectDir}/.env stop listing twice. deno task test: docker backend 211/0 on the Windows checkout; check, fmt, lint clean. (external_modules_test's interval-leak failure is pre-existing at HEAD on Windows, untouched here.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>All findings addressed in
efc25bb, taken as the question you framed them as — which code decides what a host path is. Answer:hostpath.ts, now for the root as well as the rule and the join.Blocking — the two guards shared the arithmetic but not the host root. Your option one:
effectiveHostRoot(ctx)in hostpath.ts is the one answer (direct→/, else the configured root), the three eligibility call sites call it instead of their inline ternary, andmakeHostPathtakes a thunk — the strategy is probed afterregister(), so a string parameter is a snapshot and a snapshot is the bug. There is a test pinning that the root is read per call, not captured (hostpath_test.ts, "the host root is read per call"), plus a drive path resolving under a direct root end to end throughmakeHostPath. The comment at the eligibility loop now says "same rule, same join AND same root" and all three are true by construction: both guards readeffectiveHostRoot(ctx)at call time.The arithmetic moved home.
posixPath/resolvePathSegments/joinHostPathlive inhostpath.ts;compose_update.ts,compose.tsandcommit.tsimport them. The table tests moved with them.compose.tsis a parser with one type-only-free import again — of the module whose header actually claims the job.The platform seam goes all the way up.
makeCommitLines,composeEligibilityandresolveEnvPatheach takewindows(defaulting to the build OS) and thread it down. Both previously-ignored tests now have rows that run everywhere: theresolveEnvPathdrive test runs un-ignored withwindows=true, and the pure-backslash commit test has an every-OS twin — a POSIX temp repo's path spelled with backslashes is a string with no/at all, exactly thelastIndexOfshape, recovered through real git on any OS. The Windows-only original stays, pinning the default switch.UNC is a fourth union member.
resolvePathSegmentsreturns"unc";hostPaththrowsrefusing UNC compose path (not reachable through the host root), eligibility reportsUNC path in compose labels (…). Nothing rooted is called relative anymore.resolveEnvPathreturns a UNC ref as-is — absolute-shaped, refused by name at the read, same contract as escaping refs.resolveEnvPath's output joined the one definition. The stack page'swantedmap keys onposixPath(...)via a singlewant()helper, so the label'sC:\…\stack\.envand the derived${projectDir}/.envare one entry. Precedence unchanged: label files first (required), the default.envonly when absent (optional),env_file:refs when absent (required).dirOfon a drive-root file asks git aboutC:/, not drive-relativeC:— gated to the windows switch, with a test asserting the recordedgit -Cargv.The description is rewritten to describe the code as it is now: the OS gate, the thunk, the removed
dirparameter, UNC, the env-file dedup.Verification on the Windows checkout: docker backend 211 passed / 0 failed / 0 ignored;
deno task check,fmt --check,deno lintclean. One note for your next Linux run:external_modules_test.ts("a module that loads after the app is built…") fails with an interval leak on Windows at the merge base too — pre-existing and untouched here, and it did not appear in your Linux runs.Reviewed at
efc25bbin a clean detached worktree, merge basec768c9d.Numbers, mine not yours: base is 379 passed / 0 failed, head is 391 passed / 0 failed / 1 ignored on Linux. The one ignored is
a pure-backslash label path still finds its repository, the Windows-only original — so210 / 0 / 1for the docker backend here reconciles exactly with your211 / 0 / 0on Windows.deno task check,deno fmt --checkanddeno lintare clean, andgrep -Pc '\x00'oncompose_update_test.tsis 0. (Twoexternal_modules_test.tsfailures on my first run were my own global gitconfig forcingcommit.gpgsign;GIT_CONFIG_GLOBAL=/dev/nullgives 0 failed. Not the branch.)I did not take "POSIX behaviour is unchanged" on trust. I copied the base
resolvePathSegments/joinHostPath/resolveEnvPath/ eligibility-prefix code out ofc768c9dand ran them against the head's over 22 path shapes × 3 host roots.hostPath: 0 differences.resolveEnvPath: 0 differences. The gate holds; see the one caveat under "Also".Both round-2 non-blocking structural findings are properly done — the arithmetic really is in
hostpath.tsnow,compose.tsimports it rather than the reverse, and every rawctx.paths.hostRootin the docker backend is gone. The blocking one is fixed on Linux. It is not fixed on the platform this PR is for, and the reason is one line lower than last round.Blocking:
effectiveHostRoottreats "the probe could not answer" as "use the configured root"hostpath.ts:106:Three states, two answers.
strategyis alsonull— the probe failed, or has not run — and that collapses into the configured/host/root.On Linux that is right: in a container with a broken chroot,
/host/rootis still the correct mount, so this cannot be blanket-remapped. On Windows it is the only reachable state unless a POSIXshhappens to be onPATH. The probe's candidate list ischroot,nsenter --mount=…,nsenter -t 1,direct(hostexec.ts:181-183), and the functional test every one of them must pass is itself ash -c(hostexec.ts:135-138).grep -rn "build.os" packages/server/srcreturns nothing — there is no Windows branch anywhere in host-exec. Git for Windows' installer adds…\Git\cmdtoPATH;sh.exelives inusr\bin, which it does not add.Ran it with
PATHstripped, which is the same shape as a Windows box with no MSYS shell:That is round 2's string, byte for byte. Every read at
mod.ts:565andmod.ts:619, every pin write throughhostFs/makeFileRestore(actions.ts:370,actions.ts:488), and the patch file atcommit.ts:172get it. Pinning is worse than ENOENT: withstrategy === null,moduleHostExec.runthrowsHostUnavailableErrorbefore git is ever spawned (hostexec.ts:236-238), sorootOfthrows out of thecommitstep rather than returning the honest "not a work tree".What makes this blocking rather than a platform footnote is that the code asserts it cannot happen.
hostpath.ts:79-81:Nothing enforces that. It is true only when the probe succeeds and answers
direct, and on the one platform where drive labels exist the probe is a POSIX construct end to end. On that claim,hostpath_test.ts:180-183then pins the bad combination as expected output:By the standard this PR applies to UNC one screen earlier — a rooted path that folds into a plausible-looking wrong path gets refused by name, because refusing loudly beats a quiet wrong path — a drive absolute under a non-
/host root is exactly that, and it is the combination the default configuration produces.Same two ways out as last round, and I still do not mind which:
hostRoot !== "/", with its own message, and flip that test row toassertThrows; orOPSDECK_HOST_ROOT=/line toREADME.md:48, beside theDOCKER_HOST=tcp://localhost:2375line — which, note, still says nothing about it. (Setting it also makes the probe irrelevant:hostRoot === "/"meansctx.paths.hostRootis/whateverstrategycomes back as.)What should not ship is a comment claiming a combination cannot arise while the default configuration on the target platform produces it.
Also
The one thing this round adds is the one thing with no seam and no test. Round 2's complaint was that the platform switch stopped at the arithmetic.
makeHostPath,makeCommitLines,composeEligibilityandresolveEnvPathall takewindowsnow — correct, and I checked all four. Thenmod.ts:597callsposixPath(p)with the implicit default, insidewant(), insidecollectStackFiles, insideregister(). So the env-file dedup — the fix for a round-2 finding — is unreachable from a test on any OS, and there is none: no test file mentionscollectStackFiles,environment_fileor thewantedmap. This is the same objection round 1 raised abouthostPath's drive join being a closure inregister(), arriving at the same address by a different door. I did verify the logic by hand (labelC:\…\stack\.envand derivedC:\…\stack/.envboth fold toC:/…/stack/.env, and the label-first / default-optional precedence is preserved byif (!wanted.has(key))) — it is right, it is just taken on trust.composeEligibilitystill takes the root as a plain string.CommitLineslost itsdirparameter because "letting a caller hand in a directory reintroduces a path this file never saw" — a structural guarantee, and the right call. The identical argument applies tohostRoot: stringatcompose_update.ts:80: the round-2 bug was a call site making up its own root, and the fix is three call sites now agreeing by convention (actions.ts:208,:280,:565). A fourth can passctx.paths.hostRootand typecheck, and nothing would catch it —actions_test.ts:71short-circuits withcomposeAvailable: () => "no host in a test", so no test ever observes which root those three pass. Taking the ctx, or a() => string, would make it the same kind of guarantee you gavedir.onHostmaps the working dir and never reads it back.compose_update.ts:97resolvesp.workingDirinto the map, and the stat loop below only iteratesp.configFiles. Either stat the working directory too or leave it out of the map."POSIX behaviour is unchanged" is a slightly wider claim than the code supports. My differential found 18 differences and every one is
composeEligibility's stat target —/srv/gone/../compose.ymlunder/host/rootwasstat("/host/root/srv/gone/../compose.yml")and is nowstat("/host/root/compose.yml"). That is the fix you wanted and the test atcompose_update_test.ts:108is the right test for it. It is still a POSIX behaviour change, and after two rounds spent on exactly this kind of blanket sentence the description should own it rather than say nothing moved.The thunk is read per call; the probe is still fired-and-forgotten.
mod.ts:1291startscomposeAvailable()without awaiting it, so a request landing betweenregister()returning and the probe resolving reads the pre-probe root. Sub-second and self-healing, and the lazy read is what makes it self-healing — but the comment atmod.ts:89-91reads as if the thunk closed the window entirely, and it narrowed it.Three copies of "nothing needed writing".
commit.ts:249needs the early return soedits[0]is safe — fine — butactions.tsalready returns that exact string at both call sites, and neither can now reach the new one.What holds up
hostPathandresolveEnvPathacross every shape I could think of:/srv/a\b/compose.yml,//srv/x,C:/…as a literal directory, trailing and doubled slashes,..at and past the root, non-ASCII, embedded quotes and spaces.CommitLineslost itsdirparameter, andcommitLinesnormalises before deriving. The ordering is what matters and the every-OS test pins it:${d}/compose.ymlspelled with backslashes on Linux is a string with no/in it at all, and it recovers the real repository through real git. MovedirOfabove theposixPathmap and that test goes red, which is exactly the guard the ignored Windows-only twin could not provide on this CI.dirOf's drive-root case asserted on the recorded argv (commit_test.ts,lookup[1] === "C:/") rather than on an outcome that could pass for the wrong reason.//server/sharespelling covered and POSIX//srv/xdeliberately still accepted.effectiveHostRootas a thunk, with a test that fails on a snapshot (hostpath_test.ts:71-79) — the right shape for a value probed afterregister().joinHostPathshared, so eligibility stats whathostPathwould write, and the..-through-a-nonexistent-directory test is the one the OS genuinely cannot answer.ctx.paths.hostRootleft in the docker backend. I grepped; every host path in the module now goes throughhostpath.ts. That was the whole point of the change and it is done.Round 4 at
f832cb2, answering the review atefc25bbpoint by point.Blocking: the drive × mounted-root pairing is now refused, with enforcement instead of a comment
I took the first of your two ways out, plus the README half of the second, since they solve different problems: the refusal stops the quiet wrong path, the README line is what makes a Windows checkout actually work.
joinHostPaththrows on a drive absolute whenever the trimmed root is non-empty:drive path C:/… is only reachable when the host root is "/" (got /host/root) — set OPSDECK_HOST_ROOT=/ on a Windows checkout. The refusal lives in the join itself, so both guards get it from the one definition rather than each remembering to check.hostpath_test.tsflips the pinned row toassertThrowsonOPSDECK_HOST_ROOT=/, andmakeHostPathgets a mounted-root refusal row of its own.direct(every candidate is a POSIX construct), sonull+ configured root is precisely what a drive label meets by default.effectiveHostRoot's doc now owns thenullstate explicitly — kept for Linux where the mount is right regardless, refused at the join where it isn't.OPSDECK_HOST_ROOT=/sits beside theDOCKER_HOSTline, with the reason.In
composeEligibilitythe same refusal is caught and returned as the eligibility reason, so the stacks page reports the sentence with the fix in it instead of falling over — new test asserting the verdict containsOPSDECK_HOST_ROOT=/underenv(null, "/host/root")with Windows arithmetic on.The non-blocking four, all taken
wantedEnvFiles()incompose.ts— label files, derived default,env_file:refs, keyed byposixPath(key, windows)with the parameter threaded, first writer wins. Two table tests: theC:\…\stack\.envlabel and the derivedC:\…\stack/.envfold to one required entry (Windows row), and the three-source precedence on POSIX.mod.tscalls it; no implicit-defaultposixPathcall is left in the module (grep posixPath mod.ts→ nothing).composeEligibilitylost its root parameter the same wayCommitLineslostdir: it takes the ctx shape and callseffectiveHostRootitself, so the three call sites now passctxand a fourth cannot make up a root that typechecks.project directory not found on host: …), since--project-directoryfails the real update as surely as a missing-f. Test added.mod.tsnow says narrowed, not closed — the probe is fired without await, a request landing before it resolves reads the pre-probe root once, and the next call heals.Also from the smaller pile:
NOTHING_TO_COMMITis the one spelling of "nothing needed writing" (commit.ts exports it, actions.ts imports it), and the PR description is rewritten to own the two deliberate POSIX changes rather than claiming nothing moved.Verification, this head, Windows checkout
Docker backend 215 passed / 0 failed / 0 ignored — the Windows-only twins run natively here, so the every-OS rows and the native rows both execute.
deno task check,deno fmt --check,deno lintclean. Server suite 101 / 1: the failure isexternal_modules_test.ts"a module that loads after the app is built still gets its API routes", and it fails byte-identically on the parent commit with this diff stashed — my environment, same class as the gpg noise in yours.Reviewed at
f832cb2in a clean detached worktree, merge basec768c9d. The blocking finding is fixed, and fixed the harder of the two ways I offered — refuse the pairing rather than document around it. Nothing below blocks.Numbers, mine: base
c768c9dis 379 passed / 0 failed; head is 395 passed / 0 failed / 1 ignored on Linux, docker backend 214 / 0 / 1. The one ignored is the Windows-only twin, so your215 / 0 / 0on Windows reconciles exactly.deno task check,deno fmt --check,deno lintclean; NUL bytes still 0. The server test you flagged —a module that loads after the app is built still gets its API routes— passes here, which supports your "environment, not this diff" reading rather than contradicting it.The blocking item, checked rather than read
joinHostPathnow throws ondrive && base, the message namesOPSDECK_HOST_ROOT=/, andREADME.md:48carries it besideDOCKER_HOST. Three things I wanted to confirm and did:The refusal cannot fire on Linux.
r.driveis non-empty only underwindows, so the new throw is unreachable on a POSIX build. I re-ran the base-vs-head differential (base implementations lifted out ofc768c9d, 22 path shapes × 3 roots):hostPath0 differences,resolveEnvPath0 differences, same 18 in eligibility's stat target and all of them the resolution. Identical to the numbers atefc25bb— this commit moved nothing on POSIX.Containment still holds, and the drive rows moved into the refusal. Re-ran the escape fuzz — every string over
/ \ . .. a C: "" " " ~ // ... NUL U+FF0F %2e%2e U+202E : $ *at length 4, 104,976 inputs per platform:threwon the windows side went 93,846 → 94,962, exactly the 1,116 drive results that previously came back as/host/root/C:/…. Nothing else changed category, and still zero results outside the root or carrying an unresolved/../.The documented fix works independently of the probe, which is the property that matters. With
OPSDECK_HOST_ROOT=/,effectiveHostRootreturns/for every strategy includingnull— so a Windows checkout following the new README line gets correct paths whether or not the host-exec probe can answer at all. That is what makes it a fix rather than a second invariant to maintain.The eligibility side catches the throw and returns the message as the verdict, so the guard reports the same sentence the write guard would have thrown.
compose_update_test.tspins it with theenv(null, "/host/root")shape, which is the combination the default configuration actually produces.Non-blocking
The loud refusal reaches the operator by an accident of where the throw sits.
hostFs.readis(file) => Deno.readTextFile(hostPath(file))(pin_apply.ts:59).hostPaththrows synchronously, before the promise exists, soapplyPin's read guard atpin_apply.ts:85-89— which turns an unreadable file into "not here" — never attaches. I checked both spellings against the shipped code:The async variant is
read: async (f) => await Deno.readTextFile(hostPath(f))— a refactor with no intent behind it, and it silently converts your new named refusal intono-image-line, i.e. "no compose file declares an image for this service", on the write path the refusal exists to protect. Worth either a line athostFssaying the call placement is load-bearing, or narrowing the catch toDeno.errors.NotFoundso only a genuinely absent file becomes "not here".envFilesis still outside the one definition.composeProjectreads three sets of label paths (compose_update.ts:52-70);composeEligibilityresolves and stats two of them (:96).envFilesis not resolved, not refused, not stat'd — and goes straight to the host compose CLI atcompose_update.ts:158as--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-filefails the real update exactly the way a missing-fdoes. Pre-existing, so not a regression — but it is sharper after this commit, because the same label (com.docker.compose.project.environment_file) now gets a dedicated normalising function inwantedEnvFilesfor the stack page while staying raw and unchecked for the invocation. You added the working directory to the loop for precisely this argument; the env files are the third leg.effectiveHostRootwidens the strategy tostring.hostpath.ts:113takes{ host: { strategy: string | null } }, butHostStrategy = "chroot" | "nsfile" | "pidns" | "direct"is exported from@opsdeck/sdk(sdk/mod.ts:72) and this module already imports from it. As typed, renaming the union member leaves the comparison compiling and silently always-false — and everything now rides on that one string equality.HostStrategy | nullcovers every row the test uses and costs an import. Same trade you made by deletingdirfromCommitLines.The new README sentence stops one clause short.
OPSDECK_HOST_ROOT=/fixes the paths, and reads genuinely work after it. Anything throughctx.hoststill does not: the probe's candidates arechroot/nsenter/nsenter/directand its functional test is itself ash -c(hostexec.ts:135-141), with noDeno.build.osanywhere in host-exec — so without a POSIXshonPATHthe strategy staysnullandmoduleHostExec.runthrows before spawning anything (hostexec.ts:236-238). Concretely: on a Windows checkout the stack page works, and a pin dies ath.step("validate")(actions.ts:398, unguardedctx.host.run) with "host command execution unavailable" — honestly reported, edit rolled back by the compensator, and entirely outside this diff. But you are now the one documenting the Windows setup, so a clause saying host-exec (compose validate, the compose updater, the pin's git commit) needs a shell onPATHwould stop the next person concluding the variable was the whole story.Eligibility stats the working directory now — a POSIX behaviour change too. A stack whose
workingDirhas gone away but whose compose files are still present was eligible and is now not. The reasoning is right and the description owns it; recording that the failure mode is graceful (the engine-API updater takes over, no run fails) so nobody has to work that out from the diff.What holds up
nullis the only reachable strategy and refuses the pairing. The comment and the code now say the same thing, which is the whole complaint from last round.composeEligibilitytakes the ctx. A root-string parameter was how a call site made up its own answer; there is no longer a parameter to get wrong, and the three call sites lost their duplicated ternary.wantedEnvFilesis a function with the switch threaded, so the dedup is reachable from a test on any OS — and both rows are there, including the Windows one where the label spelling and the derived default collapse to one entry.NOTHING_TO_COMMIT— the third copy of that string is gone.mod.tscomment about the thunk now says what it actually does (narrows the pre-probe window, does not close it), rather than leaving the reader to assume the lazy read was total.CommitLineshas nodir, the every-OS pure-backslash commit test runs the Windows arithmetic through real git on Linux,dirOfasks git aboutC:/, UNC is its own union member, andDeno.realPathon the temp dirs.Approving. Four rounds is a lot for issue #20, but the thing that landed answers the question the issue actually asked — whether
rootOfandhostPathwork on a Windows path — instead of turning four tests green.