From d0cd5c26b4f6ae53316600b66a09a0dbd44ea094 Mon Sep 17 00:00:00 2001 From: Joshua Yanchar Date: Wed, 19 Aug 2026 11:49:23 -0700 Subject: [PATCH 1/3] fix(devcontainer): mount the cache volumes at the roots the tools use Both named volumes missed most of what they were meant to persist, and neither failed loudly. Measured in a container built from current main: ~/.cache/bazel mounted (ud-bazel-cache) ~/.cache/go-build container-local, rebuilt every recreate ~/.cache/bazelisk container-local -- a fresh 61M bazel download each time ~/.cache/pre-commit, ~/.cache/uv container-local /go/pkg (mod 544M + sumdb) container-local ~/go mounted (ud-go-cache), written by nothing Two separate bugs. `ud-bazel-cache` targeted one directory *inside* the cache root, leaving its four siblings ephemeral. `ud-go-cache` targeted `~/go`, but `features/go` bakes GOPATH=/go into the image ENV; running the image with no volumes at all shows no `~/go` and no `/go/pkg`, so that mount was writing nowhere while the real Go state was discarded. Two volumes now, each at the root of what it persists: `ud-cache` -> `~/.cache` and `ud-go-pkg-cache` -> `/go/pkg`. Roots rather than per-tool directories, because per-tool only persists what someone remembered to enumerate -- which is exactly what drifted here, with `bazelisk` missing and nothing noticing. Not all of `/go`: `bin/` there is image content (the feature builds ten tools at image-build time) and Docker seeds a volume from the image only while the volume is empty, so a volume over `/go` would freeze `bin/` at first-mount contents and a later feature bump would install tools nobody sees. Not `/go/pkg/mod` either: the checksum-db cache is its sibling at `/go/pkg/sumdb`, and the root-owned mountpoint parent breaks `go install` with `open /go/pkg/sumdb/...: no such file or directory`. `$GOPATH/pkg` is the boundary that holds -- `pkg/` derived, `bin/` artifact. post-create.sh's chown follows the new targets and is guarded on current ownership: unconditional `chown -R` over a warm `~/.cache` is minutes of re-asserting what is already correct, and on `/go` it would flatten the go feature's `vscode:golang` group. Verified in the devcontainer. Sentinels written in one container survived `devcontainer up --remove-existing-container` in both volumes, alongside 1.9G go-build, 61M bazelisk, 13M pre-commit, 544M mod and 12K sumdb. `/go/bin` is unshadowed -- golint/goplay/revive/staticcheck keep the image's Aug 18 date next to the six post-create installs' Aug 19 -- and `/go` keeps `vscode:golang 2775`. A probe with fresh volumes on the bare image confirms both targets arrive root-owned and unwritable by `vscode`, so the chown is load-bearing for each. `bazel test //...` is 22/22 and `pre-commit run --all-files` passes. Co-Authored-By: Claude Opus 5 --- .claude/CLAUDE.md | 48 ++++++++++ .devcontainer/devcontainer.json | 9 +- .devcontainer/post-create.sh | 21 +++-- .devcontainer/test_devcontainer_config.py | 108 +++++++++++++++++++++- README.md | 4 +- 5 files changed, 179 insertions(+), 11 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 74e7ea7..95df877 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -280,3 +280,51 @@ in the "Language toolchain SDKs" group alongside the `MODULE.bazel` and `setup-p symlinks `python3`/`python` onto PATH. The CI `Devcontainer` job caches the built image in GHCR (`imageName`/`cacheFrom`, `push: filter` seeds it on pushes to main), so the feature layers are reused across runs rather than rebuilt cold; this needs the workflow's `packages: write`. + +## Devcontainer cache volumes + +Two named volumes carry derived state across a container rebuild, and both are mounted at a +cache **root** rather than at a per-tool directory: + +- `ud-cache` → `/home/vscode/.cache` +- `ud-go-pkg-cache` → `/go/pkg` (`$GOPATH/pkg`; `features/go` bakes `GOPATH=/go` into the image + ENV, so this is not under `$HOME` at all) + +**A mount too narrow loses caches silently.** It only persists the tools someone remembered to +enumerate, and nothing fails when one is missed — the container just rebuilds that cache every +time, which reads as "devcontainers are slow" rather than as a bug. That had already happened: +the mount was `~/.cache/bazel`, so its four siblings — `go-build` (967M), `bazelisk` (61M), +`pre-commit` (13M), `uv` — were rebuilt on every recreate. Narrow mounts also break outright +when the omitted sibling is not optional: mounting `/go/pkg/mod` rather than `/go/pkg` leaves +the checksum-db cache (`/go/pkg/sumdb`) out, and Docker creates the `/go/pkg` mountpoint parent +root-owned, so the first `go install` fails on `open /go/pkg/sumdb/…: no such file or +directory`. + +**A mount too wide shadows image content, also silently.** Docker seeds a named volume from the +image only while the volume is empty; after that the volume wins. So a volume over all of `/go` +would freeze `/go/bin` at whatever the image held on first mount, and a later `features/go` bump +would install tools nobody ever sees. `/go/bin` is image content — the feature builds ten tools +there at image-build time — while `/go/pkg` does not exist in the image at all, because the +feature purges the module cache afterwards. So `pkg/` is the derived half of GOPATH and `bin/` +is the artifact half, and only `pkg/` is mounted. `post-create.sh` reinstalls its six pinned +tools over the image's copies on every create, which is why persisting `/go/bin` would buy +nothing even without the shadowing. + +Nothing under either mount should stay ephemeral: every entry is content-addressed or +key-validated by its own tool, and CI builds cold, so a stale local cache can't reach `main`. + +Two couplings, both asserted by `.devcontainer/test_devcontainer_config.py`: + +- **post-create.sh must chown every volume target.** Docker attaches a volume root-owned unless + the image has a directory at the target to seed ownership from, and it has one at neither of + these — so a mount added without a chown entry is unwritable by `remoteUser`. The + chown is guarded on current ownership: warm, `~/.cache` is tens of GB, and recursing it on + every rebuild is minutes of re-asserting what is already right. +- **`$GOPATH/pkg` is not derivable from anything in this repo** — `GOPATH` is the go feature's + own value, baked into the image ENV — so the test pins it as a constant. + +The volumes are per-host, not per-worktree, and shared by every checkout of this repo. That is +fine for both: Bazel namespaces its output base by workspace path, and the module cache is +content-addressed. It does mean a mount-target change is visible from other worktrees' running +containers, which still mount the old volume at the old path — a volume's content is shared, its +mount point is per-container. diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index e5a9bde..8e661cd 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -44,8 +44,13 @@ // meta/devcontainer-base/scripts/ carry the per-step commentary. "initializeCommand": ".devcontainer/initialize.sh", "mounts": [ - "source=ud-bazel-cache,target=/home/vscode/.cache/bazel,type=volume", - "source=ud-go-cache,target=/home/vscode/go,type=volume", + // A volume per cache *root* — the whole of ~/.cache, not one tool's directory inside it. + // See ".claude/CLAUDE.md", "Devcontainer cache volumes". + "source=ud-cache,target=/home/vscode/.cache,type=volume", + // $GOPATH/pkg — the whole derived half of GOPATH (mod + sumdb), which the go feature puts + // at /go. Not all of /go: `bin/` there is image content a volume would shadow. Not + // /go/pkg/mod either: that leaves sumdb out and its root-owned parent breaks `go install`. + "source=ud-go-pkg-cache,target=/go/pkg,type=volume", // Bind the symlink (a static, workspace-relative SOURCE) to the static // container path the shared plumbing points the host-absolute path at. Docker // follows the symlink host-side, so this resolves to wherever the real git diff --git a/.devcontainer/post-create.sh b/.devcontainer/post-create.sh index 403f2fa..5166e9f 100755 --- a/.devcontainer/post-create.sh +++ b/.devcontainer/post-create.sh @@ -17,12 +17,21 @@ PLUMBING_WORKSPACE="$(cd "$_dc_here/.." && pwd)" \ PLUMBING_DIR="$_dc_here/.git-plumbing" \ /usr/local/bin/devcontainer-plumbing post-create -# Make the named-volume mounts writable by the non-root user. Docker attaches volumes -# root-owned on first mount, and the .cache parent of the bazel mount inherits that, so -# the chown covers .cache itself. postCreateCommand reruns on every rebuild, so this -# self-heals UID drift if remoteUser later changes (assuming the new user has sudo). If -# chown fails loudly here, recover with `docker volume rm ud-bazel-cache ud-go-cache`. -sudo chown -R "$(id -u):$(id -g)" "$HOME/.cache" "$HOME/go" +# Make the named-volume mounts writable by the non-root user: Docker attaches a volume +# root-owned unless the image has a directory at the target for it to seed ownership from. +# Every volume target in devcontainer.json needs an entry here — one without comes up +# unwritable — which test_devcontainer_config.py asserts. postCreateCommand reruns on every +# rebuild, so this self-heals UID drift if remoteUser later changes (assuming the new user has +# sudo). If chown fails loudly here, recover with `docker volume rm ud-cache ud-go-pkg-cache`. +# The image has a directory at neither target, so both arrive root-owned on a fresh volume. +# +# Guarded rather than unconditional: warm, these hold tens of GB, and recursing them on every +# rebuild spends minutes re-asserting ownership that is already correct. +for volume_target in "$HOME/.cache" /go/pkg; do + if [ "$(stat -c '%u' "$volume_target")" != "$(id -u)" ]; then + sudo chown -R "$(id -u):$(id -g)" "$volume_target" + fi +done # Install golangci-lint. Version is pinned and tracked by Renovate (see renovate.json). # renovate: datasource=github-releases depName=golangci/golangci-lint diff --git a/.devcontainer/test_devcontainer_config.py b/.devcontainer/test_devcontainer_config.py index 03a2792..e485033 100644 --- a/.devcontainer/test_devcontainer_config.py +++ b/.devcontainer/test_devcontainer_config.py @@ -1,6 +1,6 @@ """Tests for the wiring between devcontainer.json, the Dockerfile, the base image, and .vscode. -Four couplings live across those files and none of them fails loudly: +Five couplings live across those files and none of them fails loudly: - The `BASE_IMAGE` override has an exact working shape. Every nearby shape either breaks every local `devcontainer up` (an empty `--build-arg` overriding the Dockerfile default) @@ -12,9 +12,14 @@ in Syndic/.dotfiles, which has no such copy. - An extension this repo configures is installed by devcontainer.json. Settings for an absent extension bind to nothing, so the feature is missing with no error anywhere. + - Each cache volume is mounted at a cache *root*, and post-create.sh chowns exactly the set + of volume targets. Too narrow a target leaves the siblings ephemeral, too wide a one + shadows image content with a stale volume copy, and a target with no chown entry comes up + root-owned. The rationale for the first three is in meta/devcontainer-base/README.md, "Consuming the -image". +image"; for the mounts — which are this repo's, not the base image's — it is in +.claude/CLAUDE.md, "Devcontainer cache volumes". The parsing helpers are pure so they can be exercised directly, same split as the shell tests in this directory. """ @@ -47,6 +52,13 @@ _PLUMBING_COMMAND = "/usr/local/bin/devcontainer-plumbing" _BASE_REPOSITORY = "ghcr.io/syndic/unnatural_designs-devcontainer-base" +# The cache roots the volumes persist. `$GOPATH/pkg` is derivable from nothing in this repo — +# `features/go` bakes GOPATH=/go into the image ENV — so this constant is the pin. None of its +# neighbours belongs here: `~/go` is not GOPATH and nothing writes it; `/go` holds image-built +# tools in `bin/` that a volume would shadow; `/go/pkg/mod` omits the sibling `sumdb` cache. +_GOPATH_PKG = "/go/pkg" +_XDG_CACHE_HOME = "~/.cache" + _LOCAL_ENV_RE = re.compile( r"\A\$\{localEnv:(?P[A-Za-z_][A-Za-z0-9_]*)(?::(?P.*))?\}\Z" ) @@ -122,6 +134,38 @@ def parse_local_env(value: str) -> tuple[str, str | None]: return match.group("var"), match.group("default") +def parse_mount(spec: str) -> dict[str, str]: + """Split one `source=…,target=…,type=…` mount string into its fields. + + Valueless flags (`readonly`) are real mount syntax and come back mapped to `""`. + """ + fields = {} + for part in spec.split(","): + key, _, value = part.partition("=") + fields[key.strip()] = value.strip() + return fields + + +def chown_targets(text: str) -> list[str]: + """The paths post-create.sh makes writable, read off its `for … in ; do` header. + + Reads the loop list rather than the `chown` line so a future edit cannot make the two + disagree. Paths come back as written, `$HOME` included — see `expand_home`. + """ + match = re.search(r"^for volume_target in (?P.+?); do$", text, re.MULTILINE) + if not match: + raise ValueError("post-create.sh has no `for volume_target in …; do` loop") + return [path.strip().strip('"') for path in match.group("paths").split()] + + +def expand_home(path: str, home: str) -> str: + """Resolve `~` / `$HOME` against the *container's* home, never this runner's.""" + for prefix in ("~", "$HOME", "${HOME}"): + if path == prefix or path.startswith(prefix + "/"): + return home + path[len(prefix) :] + return path + + def dockerfile_instructions(text: str) -> list[tuple[str, str]]: """Ordered instruction *heads* — `(FIRST_WORD, rest)` per non-comment, non-blank line. @@ -455,6 +499,66 @@ def test_the_last_stage_is_the_overridable_one(self): self.assertEqual(image, "${BASE_IMAGE}") +class TestMountParsing(unittest.TestCase): + def test_fields_are_split(self): + self.assertEqual( + parse_mount("source=ud-cache,target=/home/vscode/.cache,type=volume"), + {"source": "ud-cache", "target": "/home/vscode/.cache", "type": "volume"}, + ) + + def test_valueless_flag_is_kept(self): + # `readonly` is how the allowed_signers bind spells itself; rejecting it would make + # the volume filter below throw on a perfectly valid mount list. + self.assertEqual(parse_mount("type=bind,readonly")["readonly"], "") + + def test_chown_targets_are_read_off_the_loop(self): + self.assertEqual( + chown_targets('prelude\nfor volume_target in "$HOME/.cache" /go; do\nbody\n'), + ["$HOME/.cache", "/go"], + ) + + def test_chown_targets_requires_the_loop(self): + with self.assertRaises(ValueError): + chown_targets("sudo chown -R vscode /home/vscode/.cache\n") + + def test_home_expansion_uses_the_given_home(self): + self.assertEqual(expand_home("$HOME/.cache", "/home/vscode"), "/home/vscode/.cache") + self.assertEqual(expand_home("~/.cache", "/home/vscode"), "/home/vscode/.cache") + self.assertEqual(expand_home("/go", "/home/vscode"), "/go") + + def test_home_expansion_is_path_segment_wise(self): + # `~foo` is another user's home to a shell, not a subdirectory of ours. + self.assertEqual(expand_home("~foo/.cache", "/home/vscode"), "~foo/.cache") + + +class TestCacheVolumes(unittest.TestCase): + """The volumes that carry derived state across a container rebuild.""" + + @classmethod + def setUpClass(cls): + cls.config = json.loads(strip_jsonc(_DEVCONTAINER_JSON.read_text(encoding="utf-8"))) + cls.home = f"/home/{cls.config['remoteUser']}" + cls.targets = [ + parse_mount(spec)["target"] + for spec in cls.config["mounts"] + if parse_mount(spec).get("type") == "volume" + ] + + def test_the_volume_targets_are_the_two_cache_roots(self): + # Exactly the roots, and the failure is silent in both directions. Too narrow — + # `~/.cache/bazel`, which this replaces — persists one tool and leaves every sibling + # (go-build, bazelisk, uv, pre-commit) ephemeral. Too wide — `/go` rather than the + # `pkg/` inside it — shadows image content with a stale volume copy. + self.assertCountEqual(self.targets, [expand_home(_XDG_CACHE_HOME, self.home), _GOPATH_PKG]) + + def test_post_create_chowns_exactly_the_volume_targets(self): + # A volume whose target the image has no directory for arrives root-owned, so a mount + # added without a chown entry is simply unwritable by remoteUser. + script = (_HERE / "post-create.sh").read_text(encoding="utf-8") + chowned = [expand_home(path, self.home) for path in chown_targets(script)] + self.assertCountEqual(chowned, self.targets) + + class TestHooksCallTheInstalledCommand(unittest.TestCase): def test_hooks_use_the_image_path(self): for hook in _HOOKS: diff --git a/README.md b/README.md index 5b2886b..5c66fb4 100644 --- a/README.md +++ b/README.md @@ -37,7 +37,9 @@ Container_ from the Command Palette. First build takes a few minutes; subsequent [`shellcheck`](https://www.shellcheck.net) (shell lint). All Python tools (`ruff`, `ty`, `pre-commit`) are installed via `uv tool install` at image build time, so the devcontainer has a single Python package manager (uv) and no `pip install --user` in post-create. Named volumes -(`ud-bazel-cache`, `ud-go-cache`) preserve the Bazel and Go caches across container rebuilds. +preserve the two cache roots across container rebuilds: `ud-cache` (`~/.cache` — Bazel, +bazelisk, `go build`, uv, pre-commit) and `ud-go-pkg-cache` (`$GOPATH/pkg` — the Go module and +checksum-db caches, i.e. `/go/pkg`). **Base image**: all of that is layered on top of [`meta/devcontainer-base/`](meta/devcontainer-base/README.md)'s published image, which this repo From 96680c5f6d55c7b4836f5ed897d291ed47e6cfba Mon Sep 17 00:00:00 2001 From: Joshua Yanchar Date: Wed, 19 Aug 2026 15:34:38 -0700 Subject: [PATCH 2/3] fix(devcontainer): close the chown coupling and narrow the guard claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on the three points raised. The chown test asserted only what the loop iterates over. `chown_targets` matched the `for volume_target in …; do` header alone, so a body that chowned a literal -- or stopped chowning entirely -- kept returning the right paths and kept the test green, while the container came up with /go/pkg root-owned and the first `go install` died on permission denied. The docstring had the rationale backwards too: reading only the header is what lets the header and the body disagree. It now matches the loop as a unit, header through `done`, and requires the body to chown `"$volume_target"` on one line -- a `chown` somewhere and the variable mentioned somewhere later is not a chown of it. Both bodies the review named now raise, and the guarded real body still parses, which is its own test since the chown is neither the first nor the only line. The ownership guard tested `%u` while the chown sets `$(id -u):$(id -g)`, so a GID-only drift was skipped. Now `%u:%g`, which costs nothing: `sudo chgrp root /go/pkg` in a live container makes the old test skip and the new one fire. The guard reads the mount root, so it cannot heal a root-owned entry left *inside* an otherwise-correct tree -- a tool run under sudo in here, or a `chown -R` interrupted partway, which visits pre-order and so fixes the root first. Keeping the guard, since a full walk costs roughly what it saves, but the comment claimed a self-heal it only partly delivers; it now says which case it heals, which it does not, and how to recover by hand. The narrower statement made the earlier self-heal clause redundant, so that paragraph is merged rather than left to drift against it. CLAUDE.md said go-build was 967M, a figure inherited from the superseded attempt; this PR measured 1.9G in both the gap table and the persistence output, and the doc is the copy that outlives the PR. Verified: both review mutations fail the test and the real script passes; recreated the container, sentinels and all five caches survived, /go/bin still unshadowed and /go still vscode:golang 2775; `bazel test //...` 22/22; `pre-commit run --all-files` passes. Co-Authored-By: Claude Opus 5 --- .claude/CLAUDE.md | 2 +- .devcontainer/post-create.sh | 20 +++++--- .devcontainer/test_devcontainer_config.py | 60 ++++++++++++++++++++--- 3 files changed, 65 insertions(+), 17 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 95df877..95218b3 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -293,7 +293,7 @@ cache **root** rather than at a per-tool directory: **A mount too narrow loses caches silently.** It only persists the tools someone remembered to enumerate, and nothing fails when one is missed — the container just rebuilds that cache every time, which reads as "devcontainers are slow" rather than as a bug. That had already happened: -the mount was `~/.cache/bazel`, so its four siblings — `go-build` (967M), `bazelisk` (61M), +the mount was `~/.cache/bazel`, so its four siblings — `go-build` (1.9G), `bazelisk` (61M), `pre-commit` (13M), `uv` — were rebuilt on every recreate. Narrow mounts also break outright when the omitted sibling is not optional: mounting `/go/pkg/mod` rather than `/go/pkg` leaves the checksum-db cache (`/go/pkg/sumdb`) out, and Docker creates the `/go/pkg` mountpoint parent diff --git a/.devcontainer/post-create.sh b/.devcontainer/post-create.sh index 5166e9f..a85f0ed 100755 --- a/.devcontainer/post-create.sh +++ b/.devcontainer/post-create.sh @@ -18,17 +18,21 @@ PLUMBING_WORKSPACE="$(cd "$_dc_here/.." && pwd)" \ /usr/local/bin/devcontainer-plumbing post-create # Make the named-volume mounts writable by the non-root user: Docker attaches a volume -# root-owned unless the image has a directory at the target for it to seed ownership from. -# Every volume target in devcontainer.json needs an entry here — one without comes up -# unwritable — which test_devcontainer_config.py asserts. postCreateCommand reruns on every -# rebuild, so this self-heals UID drift if remoteUser later changes (assuming the new user has -# sudo). If chown fails loudly here, recover with `docker volume rm ud-cache ud-go-pkg-cache`. -# The image has a directory at neither target, so both arrive root-owned on a fresh volume. +# root-owned unless the image has a directory at the target for it to seed ownership from, and +# it has one at neither of these, so both arrive root-owned on a fresh volume. Every volume +# target in devcontainer.json needs an entry here — one without comes up unwritable — which +# test_devcontainer_config.py asserts. If chown fails loudly here (it needs sudo), recover with +# `docker volume rm ud-cache ud-go-pkg-cache`. # # Guarded rather than unconditional: warm, these hold tens of GB, and recursing them on every -# rebuild spends minutes re-asserting ownership that is already correct. +# rebuild spends minutes re-asserting ownership that is already correct. The guard reads the +# mount root only, so what it heals is a volume that arrives wholly root-owned — a fresh one, +# or one outliving a remoteUser change. It does NOT heal a root-owned entry left *inside* an +# otherwise-correct tree (a tool run under sudo in here, or an interrupted chown -R, which +# visits pre-order and so fixes the root first); recover from that by hand with +# `sudo chown -R "$(id -u):$(id -g)" `. for volume_target in "$HOME/.cache" /go/pkg; do - if [ "$(stat -c '%u' "$volume_target")" != "$(id -u)" ]; then + if [ "$(stat -c '%u:%g' "$volume_target")" != "$(id -u):$(id -g)" ]; then sudo chown -R "$(id -u):$(id -g)" "$volume_target" fi done diff --git a/.devcontainer/test_devcontainer_config.py b/.devcontainer/test_devcontainer_config.py index e485033..786c938 100644 --- a/.devcontainer/test_devcontainer_config.py +++ b/.devcontainer/test_devcontainer_config.py @@ -63,6 +63,16 @@ r"\A\$\{localEnv:(?P[A-Za-z_][A-Za-z0-9_]*)(?::(?P.*))?\}\Z" ) +# post-create.sh's ownership loop, split into the paths it names and the body that has to act +# on them. `.+?`/`.*?` are lazy so the first `done` at column 0 closes the loop. +_CHOWN_LOOP_RE = re.compile( + r"^for volume_target in (?P.+?); do\n(?P.*?)^done$", + re.MULTILINE | re.DOTALL, +) +# Deliberately line-bound (no DOTALL): the chown and the path it takes must be one command, +# not a `chown` on one line and the variable mentioned on some later one. +_CHOWN_OF_LOOP_VAR_RE = re.compile(r'\bchown\b.*"\$volume_target"') + def scan_outside_comments(text: str): """Yield `(char, in_string)` for every character of `text` that is not in a comment. @@ -147,14 +157,20 @@ def parse_mount(spec: str) -> dict[str, str]: def chown_targets(text: str) -> list[str]: - """The paths post-create.sh makes writable, read off its `for … in ; do` header. + """The paths post-create.sh actually makes writable. + + Both halves of the loop are checked, because either on its own is satisfiable without the + other: the header names the paths, and the body has to chown the loop variable. A body that + chowns a literal — or that stops chowning at all — leaves a volume root-owned with the + header still reading exactly right, which is the failure this helper exists to catch. - Reads the loop list rather than the `chown` line so a future edit cannot make the two - disagree. Paths come back as written, `$HOME` included — see `expand_home`. + Paths come back as written, `$HOME` included — see `expand_home`. """ - match = re.search(r"^for volume_target in (?P.+?); do$", text, re.MULTILINE) + match = _CHOWN_LOOP_RE.search(text) if not match: - raise ValueError("post-create.sh has no `for volume_target in …; do` loop") + raise ValueError("post-create.sh has no `for volume_target in …; do … done` loop") + if not _CHOWN_OF_LOOP_VAR_RE.search(match.group("body")): + raise ValueError('the volume_target loop body does not chown "$volume_target"') return [path.strip().strip('"') for path in match.group("paths").split()] @@ -499,6 +515,14 @@ def test_the_last_stage_is_the_overridable_one(self): self.assertEqual(image, "${BASE_IMAGE}") +# Loop fixtures for the chown_targets tests: one header, a body swapped per case. +_LOOP_PATHS = ["$HOME/.cache", "/go/pkg"] + + +def _loop(body: str) -> str: + return f'prelude\nfor volume_target in "$HOME/.cache" /go/pkg; do\n{body}\ndone\ntail\n' + + class TestMountParsing(unittest.TestCase): def test_fields_are_split(self): self.assertEqual( @@ -512,15 +536,35 @@ def test_valueless_flag_is_kept(self): self.assertEqual(parse_mount("type=bind,readonly")["readonly"], "") def test_chown_targets_are_read_off_the_loop(self): - self.assertEqual( - chown_targets('prelude\nfor volume_target in "$HOME/.cache" /go; do\nbody\n'), - ["$HOME/.cache", "/go"], + self.assertEqual(chown_targets(_loop('sudo chown -R x "$volume_target"')), _LOOP_PATHS) + + def test_chown_targets_tolerates_a_guarded_body(self): + # The real body wraps the chown in an ownership test, so the match cannot require the + # chown to be the loop's only — or first — line. + body = ( + ' if [ "$(stat -c \'%u\' "$volume_target")" != "$(id -u)" ]; then\n' + ' sudo chown -R "$(id -u):$(id -g)" "$volume_target"\n' + " fi" ) + self.assertEqual(chown_targets(_loop(body)), _LOOP_PATHS) def test_chown_targets_requires_the_loop(self): with self.assertRaises(ValueError): chown_targets("sudo chown -R vscode /home/vscode/.cache\n") + def test_chown_targets_requires_the_body_to_chown_the_loop_variable(self): + # The header alone is not the coupling: these two bodies iterate the right paths and + # still leave a volume root-owned. + for body in (' sudo chown -R x "$HOME/.cache"', " echo skipped"): + with self.subTest(body=body), self.assertRaises(ValueError): + chown_targets(_loop(body)) + + def test_chown_targets_requires_the_chown_and_the_path_on_one_line(self): + # A `chown` and a stray mention of the variable further down is not a chown of it. + body = ' sudo chown -R x /some/other/path\n echo "$volume_target"' + with self.assertRaises(ValueError): + chown_targets(_loop(body)) + def test_home_expansion_uses_the_given_home(self): self.assertEqual(expand_home("$HOME/.cache", "/home/vscode"), "/home/vscode/.cache") self.assertEqual(expand_home("~/.cache", "/home/vscode"), "/home/vscode/.cache") From 26314ff7dd136cf7797dcd0f70455456d396da62 Mon Sep 17 00:00:00 2001 From: Joshua Yanchar Date: Wed, 19 Aug 2026 16:37:29 -0700 Subject: [PATCH 3/3] test(devcontainer): make the chown assertion read shell, not raw text Review follow-up. `_CHOWN_OF_LOOP_VAR_RE` ran against the raw loop body, so a chown that had been *commented out* still satisfied it. Reproduced against the committed code before fixing: PASSES real body PASSES commented-out chown PASSES chown other path, same line caught chown literal caught no chown at all The realistic trigger is the one post-create.sh now sends people into: a root-owned entry inside ~/.cache, someone comments the chown out to test a hypothesis by hand, and does not restore it. `chown_targets` returns both paths, the coupling test passes, and the next fresh create comes up with both volumes root-owned -- the failure the body assertion exists to catch. Comment lines are stripped before the match now, per the suggestion. Also closed the second row, which the review flagged as contrived and worth leaving: `[^;&|\n]` bounds the gap between `chown` and the path, so the path has to be an argument of the chown rather than a mention on a line it happens to share. It is one character class, and without it the assertion does not mean what its name says. The `\n` in that class is load-bearing and cost a red test to find. A negated character class matches newlines even without DOTALL, so the first cut -- `[^;&|]` -- silently un-did the line-bound property the comment above it is careful about, and test_chown_targets_requires_the_chown_and_the_path_on_one_line caught it. The comment now says why the `\n` is not redundant. Four tests: the commented-out body, a chained command per separator, and -- guarding the other direction -- a chown whose path is followed by `|| true`, which is still a chown of that path and must keep passing. Verified: every row of the review matrix now resolves as intended, the real post-create.sh still parses to both targets, `bazel test //...` 22/22, `pre-commit run --all-files` passes. Co-Authored-By: Claude Opus 5 --- .devcontainer/test_devcontainer_config.py | 36 ++++++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/.devcontainer/test_devcontainer_config.py b/.devcontainer/test_devcontainer_config.py index 786c938..9721d10 100644 --- a/.devcontainer/test_devcontainer_config.py +++ b/.devcontainer/test_devcontainer_config.py @@ -69,9 +69,14 @@ r"^for volume_target in (?P.+?); do\n(?P.*?)^done$", re.MULTILINE | re.DOTALL, ) -# Deliberately line-bound (no DOTALL): the chown and the path it takes must be one command, -# not a `chown` on one line and the variable mentioned on some later one. -_CHOWN_OF_LOOP_VAR_RE = re.compile(r'\bchown\b.*"\$volume_target"') +# The path has to be an argument of the chown itself, not merely a mention on the same line or +# somewhere below it — hence `[^;&|\n]`, which bounds the gap at both a command separator and a +# newline. The `\n` is not redundant with the absent DOTALL: `.` stops at a newline but a +# negated character class does not. Comment lines are stripped before this runs, so a chown +# that has been commented out — the realistic way this regresses, while someone is testing an +# ownership hypothesis by hand — does not read as one. +_CHOWN_OF_LOOP_VAR_RE = re.compile(r'\bchown\b[^;&|\n]*"\$volume_target"') +_SHELL_COMMENT_LINE_RE = re.compile(r"^\s*#.*$", re.MULTILINE) def scan_outside_comments(text: str): @@ -169,7 +174,8 @@ def chown_targets(text: str) -> list[str]: match = _CHOWN_LOOP_RE.search(text) if not match: raise ValueError("post-create.sh has no `for volume_target in …; do … done` loop") - if not _CHOWN_OF_LOOP_VAR_RE.search(match.group("body")): + body = _SHELL_COMMENT_LINE_RE.sub("", match.group("body")) + if not _CHOWN_OF_LOOP_VAR_RE.search(body): raise ValueError('the volume_target loop body does not chown "$volume_target"') return [path.strip().strip('"') for path in match.group("paths").split()] @@ -565,6 +571,28 @@ def test_chown_targets_requires_the_chown_and_the_path_on_one_line(self): with self.assertRaises(ValueError): chown_targets(_loop(body)) + def test_chown_targets_ignores_a_commented_out_chown(self): + # The realistic regression: someone comments the chown out while testing an ownership + # hypothesis by hand — the situation post-create.sh's own guard comment sends them + # into — and doesn't restore it. Matched against raw text this reads as a chown. + body = ' # sudo chown -R "$(id -u):$(id -g)" "$volume_target"' + with self.assertRaises(ValueError): + chown_targets(_loop(body)) + + def test_chown_targets_requires_the_path_to_be_an_argument_of_the_chown(self): + # Same line is not enough: the path has to belong to the chown, not to a command + # chained after it. + for separator in ("&&", ";", "|"): + body = f' sudo chown -R x /other {separator} echo "$volume_target"' + with self.subTest(separator=separator), self.assertRaises(ValueError): + chown_targets(_loop(body)) + + def test_chown_targets_accepts_a_chown_with_a_trailing_clause(self): + # The bound is on what sits *between* `chown` and the path; a `|| true` after it is + # still a chown of that path, and must not be rejected. + body = ' sudo chown -R x "$volume_target" || true' + self.assertEqual(chown_targets(_loop(body)), _LOOP_PATHS) + def test_home_expansion_uses_the_given_home(self): self.assertEqual(expand_home("$HOME/.cache", "/home/vscode"), "/home/vscode/.cache") self.assertEqual(expand_home("~/.cache", "/home/vscode"), "/home/vscode/.cache")