From a2dcc91ddf3d876f4f5000869b3d20d65115613c Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Mon, 21 Sep 2026 21:23:39 +0800 Subject: [PATCH] fix(docker): add and correct the cross-checkout collision guard for Update-Codeman.sh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same guard as Start-Codeman.sh's own (docs/docker-self-update.md-adjacent incident, 2026-09-21): docker-compose.yaml hard-codes `name: codeman`, so a second checkout run without COMPOSE_PROJECT_NAME resolves to the SAME Compose project as any other checkout on the host. It has to live here too, not just in Start-Codeman.sh: this script's own --no-cache build and its `down`/`down --volumes` both run BEFORE the handoff at the bottom of the file, so Start-Codeman.sh's copy of the guard would only fire after this script's own destructive calls already ran — and its default `down --volumes` is more destructive than Start-Codeman.sh's own targeted refresh, clearing every named volume the resolved project has. Also fixes a real bug the same guard shipped with: under `set -o pipefail`, `grep -v` legitimately exits 1 when nothing survives the filter (the ordinary, no-collision case), and without `|| true` on the pipeline that non-zero status propagates through the command substitution and `set -e` aborts the WHOLE script at the guard — every time, collision or not. Caught only by actually executing the guard end-to-end against a stub `docker` (the existing smoke-test harness), never by a static text/regex check on the source; the stub's `config --format json` response was also fixed to pretty-print like real Compose does, since a compact one-liner silently resolved project_name to empty and exercised neither script's guard the way production output does. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GuHtuPiHXdykq9T6rKQJ9n --- docker/Update-Codeman.sh | 47 +++++++++++++++++++++++++++++ test/docker-entrypoint.test.ts | 54 ++++++++++++++++++++++++++++++++-- 2 files changed, 98 insertions(+), 3 deletions(-) diff --git a/docker/Update-Codeman.sh b/docker/Update-Codeman.sh index ba9fb2fd..cc318ed0 100644 --- a/docker/Update-Codeman.sh +++ b/docker/Update-Codeman.sh @@ -88,6 +88,53 @@ for override_file in "$override_yml" "$override_yaml"; do done compose_command=(docker compose --env-file "$env_file" "${compose_files[@]}") +# Same collision guard as Start-Codeman.sh, and load-bearing HERE rather than +# left to that script's own copy: this script's --no-cache build and its +# `down`/`down --volumes` (below) both run BEFORE the handoff at the bottom of +# this file, so Start-Codeman.sh's guard would only fire after the damage this +# one exists to prevent has already happened. docker-compose.yaml hard-codes +# `name: codeman`, so a second checkout run without COMPOSE_PROJECT_NAME +# resolves to the SAME Compose project as any other checkout on the host and +# operates on ITS containers and volumes — this script's default `down +# --volumes` makes that worse than Start-Codeman.sh's own targeted refresh, +# since it clears every named volume the resolved project has, not just the +# two this script means to. See Start-Codeman.sh's own guard for the full +# incident this is written against (2026-09-21). +project_name=$( + "${compose_command[@]}" config --format json 2>/dev/null | + sed -n 's/^[[:space:]]*"name":[[:space:]]*"\([^"]*\)".*$/\1/p' | head -n1 +) +if [[ -n "$project_name" ]]; then + # `|| true` on the pipeline's LAST command: under `set -o pipefail`, `grep -v` + # exits 1 when nothing survives the filter — the ordinary, no-collision case, + # since `docker ps` finds nothing at all on a first-ever deployment or a + # single matching (own) working_dir gets filtered out. Without it, that exit + # status propagates through the command substitution and `set -e` aborts the + # WHOLE script right here, every time, regardless of whether a collision + # actually exists — caught only by actually running this end-to-end (a + # static text/regex check on the source cannot see it). + other_working_dir=$( + docker ps -a --filter "label=com.docker.compose.project=$project_name" \ + --format '{{.Label "com.docker.compose.project.working_dir"}}' 2>/dev/null | + grep -v -F -x -- "$script_dir" | head -n1 || true + ) + if [[ -n "$other_working_dir" ]]; then + printf 'Error: Compose project "%s" is already in use by a DIFFERENT checkout:\n' "$project_name" >&2 + printf ' %s\n' "$other_working_dir" >&2 + printf 'This checkout is:\n' >&2 + printf ' %s\n' "$script_dir" >&2 + printf '\n' >&2 + printf 'docker-compose.yaml hard-codes `name: %s`, so two checkouts on the same host\n' "$project_name" >&2 + printf 'collide unless each one sets a distinct COMPOSE_PROJECT_NAME. Continuing would\n' >&2 + printf 'rebuild and stop the OTHER checkout'"'"'s running container and, by default,\n' >&2 + printf 'delete ALL of its named volumes.\n' >&2 + printf '\n' >&2 + printf 'Fix: export COMPOSE_PROJECT_NAME= before\n' >&2 + printf 'running this script, then retry.\n' >&2 + exit 1 + fi +fi + # Same owner-detection Start-Codeman.sh uses to derive PUID/PGID for its own # build — without it, the --no-cache build below gets Compose's untouched # default of 1000:1000, and on any host whose appdata owner differs (99:100 on diff --git a/test/docker-entrypoint.test.ts b/test/docker-entrypoint.test.ts index 8058ba34..c4a8cabe 100644 --- a/test/docker-entrypoint.test.ts +++ b/test/docker-entrypoint.test.ts @@ -192,6 +192,24 @@ describe('Update-Codeman.sh (the scripted major-update path — docker/README.md expect(updateScript).not.toMatch(/exec "\$script_dir\/Start-Codeman\.sh"/); }); + it('resolves the collision guard BEFORE the --no-cache build and the down, not after', () => { + // This script's own build/down run before the handoff to Start-Codeman.sh, + // so its copy of the guard has to be early here too - Start-Codeman.sh's + // copy alone would only catch the collision after this script's own + // destructive calls already ran. + const projectName = updateScript.indexOf('project_name=$('); + const guard = updateScript.indexOf('other_working_dir=$('); + const build = updateScript.indexOf('"${compose_command[@]}" build --no-cache'); + const down = updateScript.indexOf('"${compose_command[@]}" down'); + expect(projectName).toBeGreaterThan(-1); + expect(guard).toBeGreaterThan(projectName); + expect(guard).toBeLessThan(build); + expect(guard).toBeLessThan(down); + expect(updateScript).toMatch(/label=com\.docker\.compose\.project=\$project_name/); + expect(updateScript).toMatch(/\{\{\.Label "com\.docker\.compose\.project\.working_dir"\}\}/); + expect(updateScript).toMatch(/grep -v -F -x -- "\$script_dir"/); + }); + it('clears the named volumes by DEFAULT; --keep-volumes opts out to a plain `down`', () => { expect(updateScript).toMatch(/--keep-volumes\)\s*\n\s*keep_volumes=1/); expect(updateScript).toMatch(/"\$\{compose_command\[@\]\}" down --volumes/); @@ -270,7 +288,10 @@ describe('Update-Codeman.sh (the scripted major-update path — docker/README.md // artifact that has nothing to do with the scripts under test. const posix = (p: string) => p.replace(/\\/g, '/'); - function runSmokeTest(args: string[]): { status: number; stderr: string; log: string[] } { + function runSmokeTest( + args: string[], + extraEnv: Record = {} + ): { status: number; stderr: string; log: string[] } { const dir = mkdtempSync(join(tmpdir(), 'codeman-update-smoke-')); try { const dockerDir = join(dir, 'docker'); @@ -333,11 +354,26 @@ describe('Update-Codeman.sh (the scripted major-update path — docker/README.md ' exit 0', ' fi', ' if [[ " $* " == *" config "* && " $* " == *" --format json "* ]]; then', - ' echo \'{"name":"codeman"}\'', + // Real `docker compose config --format json` pretty-prints, so + // `"name"` starts its OWN line rather than sharing one with `{` - + // the sed extraction both scripts use anchors on that, and a + // compact one-liner here would silently resolve project_name to + // empty, exercising neither script's guard the way real Compose + // output does. + ' printf \'{\\n "name": "codeman"\\n}\\n\'', ' exit 0', ' fi', ' exit 0', 'fi', + // Mirrors the guard's own `docker ps -a --filter ... --format + // '{{.Label "com.docker.compose.project.working_dir"}}'` call. + // Empty by default (no collision) so the existing smoke tests above + // see no output here and proceed exactly as before; a test that + // wants to exercise the guard itself sets STUB_PS_WORKING_DIR. + 'if [[ "$1" == "ps" && -n "${STUB_PS_WORKING_DIR:-}" ]]; then', + ' echo "$STUB_PS_WORKING_DIR"', + ' exit 0', + 'fi', 'exit 0', ].join('\n'); const stubPath = join(binDir, 'docker'); @@ -351,7 +387,7 @@ describe('Update-Codeman.sh (the scripted major-update path — docker/README.md let stderr = ''; try { execFileSync('bash', [join(dockerDir, 'Update-Codeman.sh'), ...args], { - env: { ...process.env, PATH: `${binDir}:${process.env.PATH}`, CMDLOG: logPath }, + env: { ...process.env, PATH: `${binDir}:${process.env.PATH}`, CMDLOG: logPath, ...extraEnv }, encoding: 'utf-8', }); } catch (err) { @@ -399,6 +435,18 @@ describe('Update-Codeman.sh (the scripted major-update path — docker/README.md expect(downLine).toBeDefined(); expect(downLine).not.toContain('--volumes'); }); + + it('refuses BEFORE the --no-cache build when the resolved project belongs to a different checkout', () => { + // The whole reason this guard lives here rather than only in + // Start-Codeman.sh: this script's own build/down run before the handoff + // ever reaches that script's copy of the same check. + const { status, stderr, log } = runSmokeTest([], { STUB_PS_WORKING_DIR: '/some/other/checkout/docker' }); + expect(status).toBe(1); + expect(stderr).toMatch(/already in use by a DIFFERENT checkout/); + expect(stderr).toContain('/some/other/checkout/docker'); + expect(log.some((l) => l.includes('build --no-cache'))).toBe(false); + expect(log.some((l) => / down(\s|$)/.test(l))).toBe(false); + }); }); });