diff --git a/.github/workflows/docker-build.yml b/.github/workflows/docker-build.yml index cbf8304..ba92cdc 100644 --- a/.github/workflows/docker-build.yml +++ b/.github/workflows/docker-build.yml @@ -16,6 +16,16 @@ on: # This file defines the build itself: change the docker build command, # a --build-context or a --build-arg and the verification must re-run. - .github/workflows/docker-build.yml + # Neither of these is a build input; both feed the post-build check. + # The checker itself, so an edit to it is exercised by a build rather + # than first running on someone else's PR — and docker-compose.yml, + # which the checker DERIVES the checked directory from: move the + # transcript mount without touching a Dockerfile and nothing would + # rebuild, so nothing would re-derive. Listed here, on the ~2 minute + # classic build, and deliberately NOT in nix-base.yml, where it would + # set off an hour-long two-arch build for the same answer. + - dev/check-image-mount-parents + - docker-compose.yml pull_request: branches: [main] paths: @@ -31,6 +41,16 @@ on: # This file defines the build itself: change the docker build command, # a --build-context or a --build-arg and the verification must re-run. - .github/workflows/docker-build.yml + # Neither of these is a build input; both feed the post-build check. + # The checker itself, so an edit to it is exercised by a build rather + # than first running on someone else's PR — and docker-compose.yml, + # which the checker DERIVES the checked directory from: move the + # transcript mount without touching a Dockerfile and nothing would + # rebuild, so nothing would re-derive. Listed here, on the ~2 minute + # classic build, and deliberately NOT in nix-base.yml, where it would + # set off an hour-long two-arch build for the same answer. + - dev/check-image-mount-parents + - docker-compose.yml jobs: build: @@ -45,3 +65,11 @@ jobs: --build-arg DEV_CONTAINER_WORKSPACE=/workspaces/src \ -t devcontainer-ci:test \ . + + # The image half of #106: tests/test-volume-chown-guard.sh can only read + # the Dockerfile text, so the fact that the built image really ships + # ~/.claude/projects writable by the container user is checkable only + # here. The checker derives both the directory and the user from + # docker-compose.yml and runs the mkdir the #106 report ran. + - name: Transcript bind parent ships writable + run: dev/check-image-mount-parents devcontainer-ci:test diff --git a/.github/workflows/nix-base.yml b/.github/workflows/nix-base.yml index c3f3a5d..170b466 100644 --- a/.github/workflows/nix-base.yml +++ b/.github/workflows/nix-base.yml @@ -46,6 +46,9 @@ on: # COPY without any COPY source changing. - .dockerignore - .github/workflows/nix-base.yml + # Not a build input: the post-build check. Listed so an edit to it is + # exercised by a build rather than first running on someone else's PR. + - dev/check-image-mount-parents pull_request: paths: - nix/base/** @@ -62,6 +65,9 @@ on: # COPY without any COPY source changing. - .dockerignore - .github/workflows/nix-base.yml + # Not a build input: the post-build check. Listed so an edit to it is + # exercised by a build rather than first running on someone else's PR. + - dev/check-image-mount-parents workflow_dispatch: env: @@ -120,6 +126,14 @@ jobs: -t nix-base-tail:ci . docker run --rm nix-base-tail:ci sh -c 'claude --version && test -x /usr/local/bin/setup-env' + # The image half of #106, per arch: the nix route pre-creates + # ~/.claude/projects in the tail build (the base ships the sibling home + # dirs but not this one), and only a built image can confirm it landed + # writable. Same checker as docker-build.yml; it derives the directory + # and the user from docker-compose.yml. + - name: Transcript bind parent ships writable + run: dev/check-image-mount-parents nix-base-tail:ci + - name: Log in to ghcr.io if: github.event_name != 'pull_request' uses: docker/login-action@v4 diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index ecc86dc..1ac6a67 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -15,7 +15,7 @@ repos: args: [--severity=warning] types: [file] files: >- - ^dev/(devcontainer|devcontainer-completions|init|new-worktree|setup-worktree|cleanup-worktree|check-gitignore-agents|hooks/post-checkout)$ + ^dev/(devcontainer|devcontainer-completions|init|new-worktree|setup-worktree|cleanup-worktree|check-gitignore-agents|check-image-mount-parents|hooks/post-checkout)$ # These pins are the source of truth for CI (the pre-commit job is the only # CI lint gate). Keep ruff/yamllint/hadolint in sync with the in-container diff --git a/CLAUDE.md b/CLAUDE.md index a736924..9a01cd4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -267,13 +267,65 @@ taxonomy and reads as `test:`. prevents is bounded because the glob, not the workflow, enumerates suites). - Container-side root logic lives in `lib/*.sh` and is INJECTED per run via `dc exec ... sh -c "$script" ` — a runtime input: no - rebuild, no fingerprint entry unless it is also COPYed - (`tests/test-volume-chown-guard.sh` pins the volume-perms driver line). + rebuild, no fingerprint entry unless it is also COPYed (test: + `tests/test-volume-chown-guard.sh` pins the volume-perms driver line). - `setup()` runs on EVERY cold start (gated only on `is_running`), so its steps must be idempotent and cheap. The named-volume chown is guarded by one owner+group stat, sound because `chown -R` is post-order: an interrupted walk leaves the mount point root-owned and retries next start - (`tests/test-volume-chown-guard.sh`). + (test: `tests/test-volume-chown-guard.sh`). +- Mount-point ownership repair is dispatched on the compose mount TYPE: a + `volume` gets the guarded recursive chown plus its ancestors, a `bind` gets + ancestors ONLY, anything else gets nothing. The dispatcher never routes a + bind INTO the recursive branch, and the ANCESTOR WALK never chowns a bind + mount point nor anything under one — it can reach both, + which `docker-compose.yml` really does arrange (`DEV_MAIN_GIT` nests inside + `DEV_MAIN_TREE`, both bound at their host paths) — though only when the HOST + checkout sits under the container home prefix, which is not exotic: this + repo's own main checkout is `/home/vscode/repos/github/devcontainer`. + Everything at or below a bind mount point is the host side. Binds cannot + simply be excluded from the walk instead: the daemon creates a bind target's + missing parents as root exactly as it does a volume's, and that is what left + `~/.claude/projects` unwritable (#106) + (test: `tests/test-volume-chown-guard.sh`). +- Those two sentences are about the DISPATCHER and the ANCESTOR WALK. Neither + says a bind is never recursed into, because it is: `chown -R` rooted at a + volume has no mount awareness and descends through any bind nested under + that volume — the transcript bind inside `claude-home`, i.e. the #106 + topology itself (`unguarded`: predates the ancestor-walk work and is latent + only incidentally — both images pre-create `~/.claude` user-owned and + `setup_claude` re-chowns the top every up, so the guard's precondition (a + volume root not already user-owned) is not normally met. Closing it means + replacing `chown -R` with a pruned `find`, which also rewrites the + post-order rationale the cold-start guard rests on — filed as #115 with the + options. `-xdev` is the tempting variant and is worse: it stops at every + filesystem boundary, changing behaviour for reasons unrelated to this + invariant. The suite pins the current, unsafe behaviour rather than + asserting the safe one). +- The chown driver reads mount points from `dc config`, so the compose query in + `mount_point_targets` decides what the lib ever sees; the query itself needs + docker, but the python snippet inside it is lifted out and run against a + synthetic config document (test: `tests/test-volume-chown-guard.sh`). +- BOTH image routes pre-create the transcript bind's parent + (`~/.claude/projects`) and chown it to the compose `user:`, so a FRESH + `claude-home` volume seeds it user-owned and the daemon never creates it as + root. This covers what the ancestor walk cannot — the walk runs in `setup()`, + so an entry path that never calls `dev/devcontainer` (VS Code "Reopen in + Container", #43) would get #106 back — but only for a fresh volume: an + existing one still needs the walk, a `clean` or a `reset`. The nix route + carries it in the tail build, not `nix/base/flake.nix`, because a fix in the + base reaches nobody until a republish AND a repin (#83) + (`ci:` docker-build.yml and nix-base.yml run `dev/check-image-mount-parents`, + which derives directory and owner from `docker-compose.yml` and runs #106's + own `mkdir` probe against the built image; `test: + tests/test-volume-chown-guard.sh` holds only what keeps that check reachable + — both workflows invoke it, it is executable, it does not hardcode what it + derives, and `docker-compose.yml` is a trigger path of docker-build.yml, or a + compose-side move of the mount rebuilds nothing. The suite deliberately does + not re-derive the fact from Dockerfile text: it did, and that parser had four + measured fail-opens. Where a `ci:` check can observe the artefact, a hermetic + restatement of the same fact is not a second guard but a second thing to get + wrong). - Lock discipline: per-worktree lock on fd 9, repo-scoped build lock on fd 8; any helper backgrounded inside the locked region must be spawned with `9>&-` or it holds the worktree lock forever (`test: diff --git a/Dockerfile b/Dockerfile index 673ad55..0eb1bb7 100644 --- a/Dockerfile +++ b/Dockerfile @@ -88,7 +88,15 @@ RUN printf '. /usr/local/lib/devcontainer/claude-code-token-env.sh\n' \ # No baked .credentials.json symlink: setup-claude seeds a private per-container # token into the claude-home volume from the :ro host profile store # (docs/adr/0001-devcontainer-private-token-isolation.md). Just create the dirs. -RUN mkdir -p /home/vscode/.cache /home/vscode/.ssh /home/vscode/.claude \ +# +# .claude/projects is here for a second reason: compose mounts the transcript +# bind at .claude/projects/, so the daemon creates the parent as root if +# the image does not ship it (#106). lib/volume-perms.sh repairs that after the +# fact, but only from setup(); shipping it here covers entry paths that never +# run dev/devcontainer at all. A FRESH claude-home volume seeds from this path +# — an existing one holding a root-owned projects/ still needs the walk, a +# clean or a reset. +RUN mkdir -p /home/vscode/.cache /home/vscode/.ssh /home/vscode/.claude/projects \ && chown -R vscode:vscode /home/vscode/.cache /home/vscode/.ssh /home/vscode/.claude \ && chmod 700 /home/vscode/.ssh diff --git a/dev/check-image-mount-parents b/dev/check-image-mount-parents new file mode 100755 index 0000000..9d0429f --- /dev/null +++ b/dev/check-image-mount-parents @@ -0,0 +1,97 @@ +#!/usr/bin/env bash +# CI check: the parent of the transcript bind must ship in a built image owned +# by the container user, so the daemon never has to create it as root (#106). +# +# Why this is not in tests/: it needs a built image, so it cannot live in the +# hermetic suite (tests/run-all globs tests/*.sh and would run it with no +# docker). tests/test-volume-chown-guard.sh section 10 reads the Dockerfile +# TEXT — that a mkdir exists and a later chown covers it — which is a proxy. +# This runs the actual failing operation against the actual image, so it is the +# `ci:` half of that invariant rather than a second textual restatement. +# +# The probe is the one from the #106 report: mkdir as the container user inside +# the directory. A stat comparison would pass on a dir owned by a same-named +# user with no write bit; only the mkdir answers the question that was asked. +# +# Both the directory and the user are DERIVED from docker-compose.yml — the +# transcript mount's target and the service's `user:` — so nothing here is a +# third encoding of either, and moving the mount moves this check with it. +# +# Usage: dev/check-image-mount-parents +set -euo pipefail + +image="${1:-}" +if [ -z "$image" ] || [ "$image" = "-h" ] || [ "$image" = "--help" ]; then + echo "usage: check-image-mount-parents " >&2 + [ -n "$image" ] || exit 2 + exit 0 +fi + +base="$(cd "$(dirname "$(readlink -f "$0")")/.." && pwd)" +compose="$base/docker-compose.yml" +[ -f "$compose" ] || { + echo "error: no docker-compose.yml at $compose" >&2 + exit 1 +} + +# The transcript mount is the one whose target interpolates +# DEV_CONTAINER_PROJECT_KEY: that variable is what makes the target a +# per-project subdirectory, so its parent is the directory the daemon creates. +target="$(awk '/^[[:space:]]*target:[[:space:]]/ && /DEV_CONTAINER_PROJECT_KEY/ {print $2}' "$compose")" +if [ "$(printf '%s' "$target" | grep -c . || true)" != 1 ]; then + echo "error: expected exactly one compose target interpolating" \ + "DEV_CONTAINER_PROJECT_KEY, got: ${target:-}" >&2 + exit 1 +fi +parent="${target%%\$\{*}" +parent="${parent%/}" +case "$parent" in + /*/?*) ;; + *) + echo "error: derived a nonsense parent directory from $compose: '$parent'" >&2 + exit 1 + ;; +esac + +user="$(awk '/^[[:space:]]*user:[[:space:]]/ {print $2; exit}' "$compose")" +[ -n "$user" ] || { + echo "error: no 'user:' in $compose — cannot tell who must own $parent" >&2 + exit 1 +} + +echo "checking $image: $parent must be writable by '$user' (derived from $(basename "$compose"))" + +# Pre-flight so infrastructure failures are not reported as the #106 diagnosis. +# A missing image, or a `--user` docker cannot resolve, exits non-zero from the +# probe below exactly as an unwritable directory does, and pointing a CI reader +# at the wrong file is the whole cost of a check like this being wrong. +docker image inspect "$image" >/dev/null 2>&1 || { + echo "::error::no such image: $image (this is a CI wiring problem, not a" \ + "#106 regression — the build step above did not produce this tag)" >&2 + exit 1 +} +if ! docker run --rm --user "$user" "$image" true 2>/dev/null; then + echo "::error::cannot run $image as '$user' at all — the image has no such" \ + "user, or no working entrypoint. A CI wiring problem, not a #106" \ + "regression." >&2 + exit 1 +fi + +docker run --rm "$image" sh -c "stat -c '%n %U:%G %a' '$parent'" || { + echo "::error::$image does not ship $parent at all. Compose mounts the" \ + "transcript bind inside it, so the daemon will create it as root and" \ + "the container user will not be able to create sibling project keys" \ + "(#106)." >&2 + exit 1 +} + +if ! docker run --rm --user "$user" "$image" \ + sh -c "mkdir '$parent/.mount-parent-probe' && rmdir '$parent/.mount-parent-probe'"; then + echo "::error::$parent is not writable by '$user' in $image. That is the" \ + "#106 failure: Claude cannot create a project key it was not given," \ + "including its memory directory. Fix the mkdir/chown in the" \ + "Dockerfile for this route." >&2 + exit 1 +fi + +echo "ok: $parent ships writable by '$user'" diff --git a/dev/devcontainer b/dev/devcontainer index 97bcfa3..15ff181 100755 --- a/dev/devcontainer +++ b/dev/devcontainer @@ -778,25 +778,35 @@ require_tty() { # this line would silently run without the host mounts. generate_host_mounts_override -# Named-volume mount targets from the merged compose config — these come -# up root-owned and need chowning before user-mode setup runs. Deriving +# Mount targets from the merged compose config, as `:` — these +# come up root-owned and need chowning before user-mode setup runs. Deriving # from compose means projects only declare volumes in compose.override.yml. -named_volume_targets() { +# +# Binds are emitted alongside volumes because only their ANCESTORS are repaired +# (lib/volume-perms.sh never DISPATCHES a bind into its recursive branch, and +# its ancestor walk never chowns a bind mount point or anything under one — but +# see #115 below for what that does not cover). A bind nested under a named +# volume — the +# transcript bind at ~/.claude/projects/, inside claude-home — makes the +# daemon create the intervening directories as root, and nothing else repairs +# them (#106). The recursion rooted at a volume is a separate matter: it still +# descends through a bind nested under that volume (#115). +mount_point_targets() { local cfg cfg="$(dc config --format=json)" || { - echo "error: 'docker compose config' failed — cannot derive named-volume mount targets" >&2 + echo "error: 'docker compose config' failed — cannot derive mount targets" >&2 return 1 } printf '%s' "$cfg" | python3 -c ' import json, sys cfg = json.load(sys.stdin) for v in cfg.get("services", {}).get("app", {}).get("volumes") or []: - if v.get("type") == "volume" and v.get("target"): - print(v["target"]) + if v.get("type") in ("volume", "bind") and v.get("target"): + print(v["type"] + ":" + v["target"]) ' } -# Chown the named-volume mount points to the container user. The logic lives in +# Chown the mount points to the container user. The logic lives in # lib/volume-perms.sh (unit-tested by tests/test-volume-chown-guard.sh) and is # injected into the container as the `sh -c` script with the mount points as # positional args — it runs in the container, but nothing COPYs it into the @@ -805,12 +815,12 @@ for v in cfg.get("services", {}).get("app", {}).get("volumes") or []: # Note this is called on EVERY cold start, not just the first (setup() is gated # only on `is_running`), which is why the recursive chown is guarded inside the # lib rather than run unconditionally. -chown_named_volume_targets() { +chown_mount_points() { local targets_out - # Command substitution, not a pipe: named_volume_targets fails loudly when + # Command substitution, not a pipe: mount_point_targets fails loudly when # `dc config` does, and `|| return 1` propagates that instead of letting the # downstream exit status mask it. - targets_out="$(named_volume_targets)" || return 1 + targets_out="$(mount_point_targets)" || return 1 local targets=() t while IFS= read -r t; do @@ -821,18 +831,18 @@ chown_named_volume_targets() { local script script="$(cat "$DEV_BASE_DIR/lib/volume-perms.sh")" || return 1 script="$script"' -dev_chown_volume_targets vscode vscode /home/vscode "$@"' +dev_chown_mount_points vscode vscode /home/vscode "$@"' # `sh -c