From 76757e82e0ad8d71d647ac57e6bf73a2916e1d10 Mon Sep 17 00:00:00 2001 From: kewton Date: Fri, 9 Oct 2026 12:22:47 +0900 Subject: [PATCH] =?UTF-8?q?fix(verify):=20demo-video=20=E3=81=AE=20env-scr?= =?UTF-8?q?ipts=20=E3=83=86=E3=82=B9=E3=83=88=E3=81=8C=E5=AE=9F=E3=81=AE?= =?UTF-8?q?=20$HOME=20=E3=81=AB=E4=BD=9C=E6=A5=AD=E5=A0=B4=E6=89=80?= =?UTF-8?q?=E3=82=92=E4=BD=9C=E3=82=89=E3=81=AA=E3=81=84=E3=82=88=E3=81=86?= =?UTF-8?q?=E3=81=AB=E3=81=99=E3=82=8B=20(#3479)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 原因（(a)〜(c) のどれでもなく、(d) ワーカー自身のまだ動いていたテスト） 記録で確かめた: - wait-3395.log: env-clean は検証の先頭（1.0s）で走り、`.commandmate-demo-vitest-36552 [unattributed]`。 同じ run の tmux 行は「attributed to another worktree」を出しており、#2954 以降の帰属つきのサーバーだった（(b) ではない）。 work-evidence は commits=0 / uncommitted=56 で、ワーカーはまだ作業中だった。 - 3395 のワーカーの転写（~/.claude/projects/...-commandmate-issue-3395）: 07:36:30Z に `npx vitest run ... tests/unit/skills ...` を起動し、120 秒で背景に移った（bf0ktuuzb）。その出力は `Start at 16:37:22`・`Duration 161.67s`（16:40:04 JST まで）。tests/unit/skills は demo-video/env-scripts.test.ts を含む。 - 07:38:47Z に Stop hook が発火（背景のテストを待つ間にターンが終わった）→ wait が hook_stop で完了 → 検証開始。 つまり env-clean の時点で pid 36552 は生きており、cwd は 3395 の worktree 自身。コミットは 16:41:49。 - attributeHomeEntry を env-clean-gate.test.ts の形で直接呼んで確認（resolver を注入、サーバー等は起動せず）: cwd=3395 自身 → unattributed / cwd 不明 → unattributed / cwd=別 worktree → other。 attributeAnchor は自 worktree 内を self とし、home-entries では self も unattributed になる。設計どおりの判定だった。 「検証がコミットの前に走る」側は #3430（requireCommit）で塞がれている。 ## 直し方（第一案） env-scripts.test.ts の SCRATCH_HOME を `os.homedir()` の下から `realpath(os.tmpdir())` の下へ移した。 env-up.sh が要求するのは「状態ディレクトリが（子の）$HOME の下」と「$HOME がログインホームではない」の 2 つで、 子の $HOME は SCRATCH_HOME 自身。スタブのサーバーは validateDbPath を通らない（STUB_NAMESPACE の DB も better-sqlite3 で直接開く）。src/lib/db/db-path-resolver.ts と env-up.sh は変えていない（本番の制限は緩めない）。 realpath は macOS の /var → /private/var で、スクリプトの `pwd -P` と比較が揃うようにするため。 beforeAll の残骸の掃除（#3025）も実の HOME ではなく一時ディレクトリを対象にした（テストが実の HOME に一切触れない）。 テスト: - 陽性対照: env-scripts.test.ts「scratch location (Issue #3479)」— 直す前の実装で赤を確認 （HOME を scratchpad に差し替えて実行。isPathWithin(SCRATCH_HOME, os.homedir()) が true）。 - no-home-leftovers.test.ts: 子に TMPDIR も渡し、$HOME に何も作らないこと・一時ディレクトリに残骸が無いことを追加。 既存の it は消さず名前も変えていない。 - env-clean-gate.test.ts の「still counts a pid that is not alive」は変えずに緑（死んだ pid を other にする変更はしない）。 - env-clean-gate.ts は帰属のコードを変えず、コメントだけ「#3479 より古いチェックアウトのために残す」に直した。 orchestrate.md 3-4 の 2 か所（並行テストの demo-vitest、合図の前に始まった検証）を直した後の状態に合わせた。 tests/unit/tasks/orchestrate-*.test.ts は期待値を変えずに緑。 確認: HOME を scratchpad に差し替えて `npx vitest run tests/unit/skills/demo-video`（15 files / 770 passed、HOME は空のまま）、 `npx vitest run tests/unit/tasks tests/unit/verification tests/unit/guards tests/unit/docs`（84 files passed）、 npm run lint、npx tsc --noEmit、node scripts/changelog-fragments.mjs check。 ## 対になる場所を探した結果 - `commandmate-demo-vitest` を git grep: 作るのは env-scripts.test.ts だけ。stale-scratch.ts のコメントを直した。 - 同じく validateDbPath を理由に $HOME の下へ作るテスト: 他に無し（demo-video 配下を確認）。 - 本文に無い指摘: docs/design/task-contract.md:656 `deadDemoVitestEntries` の行が 「env-scripts.test.ts が死んだ pid の残骸を（$HOME で）掃除する（#3025）」と書いているが、#3479 から HOME は掃除しない（scope 外）。 - 本文に無い指摘: .claude/skills/demo-video/scripts/env-up.sh:8 のコメントは実際のデモ実行についての説明で今も正しい（変更不要）。 - 本文に無い指摘: 早すぎる hook_stop（背景のテストを待つ間のターン終了）で検証が作業中に始まる問題そのものは、 #3430 の requireCommit で不合格にはなるが、env-clean 以外のゲートも作業中のツリーを測る（scope 外）。 Co-Authored-By: Claude Opus 5.5 --- .claude/commands/orchestrate.md | 11 ++-- changelog.d/3479.md | 2 + src/lib/verification/env-clean-gate.ts | 11 ++-- .../skills/demo-video/env-scripts.test.ts | 51 ++++++++++++++----- .../demo-video/no-home-leftovers.test.ts | 37 ++++++++++---- tests/unit/skills/demo-video/stale-scratch.ts | 8 +-- 6 files changed, 85 insertions(+), 35 deletions(-) create mode 100644 changelog.d/3479.md diff --git a/.claude/commands/orchestrate.md b/.claude/commands/orchestrate.md index d9f1f56c3..828f27bcd 100644 --- a/.claude/commands/orchestrate.md +++ b/.claude/commands/orchestrate.md @@ -1110,9 +1110,11 @@ commandmatedev verify show "$RUN_ID" --json | jq '.gates[] | select(.status != " - 別プロセスの TCP listener（`-`） - ワーカーの最初のツール呼び出しより前の時刻が名前に入った `~/.commandmate-test-`（`+`） - 並行するワーカーのテストが一時的に作る `~/.commandmate-demo-vitest-`（`+`）も、このワーカー起因ではないことがある - （2026-09-28、2 本を並行した run で、互いのテストが作ったものを `env-clean` が違反に数えた。道具の側は #2954 で直す）。 - 直るまでは、`commandmatedev verify "$WT" --task "$TASK_ID" --gates env-clean` を再実行して、その項目が消えていれば合格として扱う。 + `~/.commandmate-demo-vitest-`（`+`）は、#3479 より前のコードの `env-scripts.test.ts` だけが作る + （#3479 から作業場所は OS の一時ディレクトリで、`$HOME` には作らない）。base が #3479 より古いブランチの + テストが並行で動いていると現れ、このワーカー起因ではないことがある（2026-09-28 の 2 本並行の run。別の worktree の + 生きたテストのものは #2954 で `other` に分かれる）。`[unattributed]` で残ったら、 + `commandmatedev verify "$WT" --task "$TASK_ID" --gates env-clean` を再実行して、その項目が消えていれば合格として扱う。 帰属は次の 3 つで確かめる: - ワーカーが実行したコマンド: `capture --prompts --limit 100` の `Run this command?` と、そこに書かれた `start with ''` @@ -1127,7 +1129,8 @@ commandmatedev verify show "$RUN_ID" --json | jq '.gates[] | select(.status != " **合図の前に始まった検証**（Antigravity 担当。3-3 の「完了の合図」）で `env-clean` だけが落ちたとき: - 違反は、ワーカー自身がまだ動かしていたもの（バックグラウンドのテスト実行の listener `[self]`、 - テストが作って後で消す `~/.commandmate-demo-vitest-*` など）であることが多い + テストが作って後で消す `$HOME` 直下のエントリなど）であることが多い。`~/.commandmate-demo-vitest-*` は + #3479 から作られない（#3395 はこの形で、ワーカー自身のまだ動いていたテストが作ったものだった） - 合図の後に `commandmatedev verify "$WT" --task "$TASK_ID" --gates env-clean` を再実行する （`work-evidence` と `scope` も一緒に走る） - 再実行が PASS で、かつ 3-3 の確認で「最後のコミットが検証の開始より前・作業ツリーに変更なし」なら、合格として扱う。 diff --git a/changelog.d/3479.md b/changelog.d/3479.md new file mode 100644 index 000000000..25e0089cf --- /dev/null +++ b/changelog.d/3479.md @@ -0,0 +1,2 @@ + +- **fix(verify): demo-video の env-scripts テストが作業場所を実の `$HOME` ではなく OS の一時ディレクトリに作るようにした** (#3479): `~/.commandmate-demo-vitest-` が実行中ずっと `$HOME` 直下にあり、同じマシンのどの契約検証の `env-clean` にも `+` として見えていた（#3395 では、検証の時点でまだ動いていたワーカー自身のテストのものが `[unattributed]` で不合格になった）。子の `$HOME` は作業場所そのもので、スタブのサーバーは DB パスの検証を通らないため、本番の制限（`validateDbPath`）は変えずに一時ディレクトリへ移した。テストの実行中・実行後に `$HOME` へ何も作らない diff --git a/src/lib/verification/env-clean-gate.ts b/src/lib/verification/env-clean-gate.ts index fdb883480..f259676ba 100644 --- a/src/lib/verification/env-clean-gate.ts +++ b/src/lib/verification/env-clean-gate.ts @@ -225,11 +225,12 @@ export const readLivePidCwd: PidCwdResolver = (pid) => { }; /** - * `tests/unit/skills/demo-video/env-scripts.test.ts` creates - * `~/.commandmate-demo-vitest-` for the length of its run — in the real - * `$HOME`, because `env-up.sh` refuses `/tmp` and `/var` (#2954). The name - * carries the pid of the test process that made it, which is the one `$HOME` - * entry whose owner can be established. + * `tests/unit/skills/demo-video/env-scripts.test.ts` created + * `~/.commandmate-demo-vitest-` for the length of its run, in the real + * `$HOME` (#2954). Since #3479 it makes the dir under the OS temp dir instead; + * this stays for checkouts that predate that. The name carries the pid of the + * test process that made it, which is the one `$HOME` entry whose owner can be + * established. */ const DEMO_VITEST_HOME_ENTRY = /^\.commandmate-demo-vitest-([1-9]\d*)$/; diff --git a/tests/unit/skills/demo-video/env-scripts.test.ts b/tests/unit/skills/demo-video/env-scripts.test.ts index 901aecad2..ac9036050 100644 --- a/tests/unit/skills/demo-video/env-scripts.test.ts +++ b/tests/unit/skills/demo-video/env-scripts.test.ts @@ -16,6 +16,7 @@ import os from 'node:os'; import path from 'node:path'; import { afterAll, afterEach, beforeAll, describe, expect, it } from 'vitest'; import { removeTempDir } from '@tests/helpers/temp-dir'; +import { isPathWithin } from '@/config/system-directories'; import { CLI_TOOL_IDS } from '@/lib/cli-tools/types'; import { claudeProjectSlug } from '@/lib/hooks/sources/claude/transcript'; import { findCodexRolloutPath } from '@/lib/hooks/sources/codex/history'; @@ -27,15 +28,22 @@ const SCRIPTS = path.join(REPO_ROOT, '.claude/skills/demo-video/scripts'); const ENV_UP = path.join(SCRIPTS, 'env-up.sh'); const ENV_DOWN = path.join(SCRIPTS, 'env-down.sh'); -// env-up refuses a state dir outside $HOME (validateDbPath rejects /tmp and -// /var as system directories), so the scratch dir has to live under $HOME. -// -// Issue #2380: env-up ALSO refuses to run with $HOME set to the login home, -// because it plants fake agent transcripts under `~/.claude/projects` and +// Issue #2380: env-up refuses to run with $HOME set to the login home, because +// it plants fake agent transcripts under `~/.claude/projects` and // `~/.codex/sessions`. So the scratch dir doubles as the HOME every script // here runs with: SCRATCH_HOME is `$HOME` for the child, and the demo state // dir sits inside it the way it would under /Users/Shared/cmdemo-home. -const SCRATCH_HOME = path.join(os.homedir(), `.commandmate-demo-vitest-${process.pid}`); +// +// Issue #3479: it lives under the OS temp dir, NOT the real $HOME. It used to +// be `~/.commandmate-demo-vitest-` because env-up requires the state dir +// to be under `$HOME` and a real demo's validateDbPath rejects /tmp and /var — +// but the `$HOME` env-up checks is the child's, which is SCRATCH_HOME itself, +// and the stub server never runs validateDbPath. In the real home the dir was +// a `$HOME` entry for every env-clean gate on the machine while a run was in +// flight (#3395 failed on its own still-running test's dir). The physical path +// (`/private/var/...` on macOS) so `pwd -P` in the scripts agrees with it. +const SCRATCH_BASE = fs.realpathSync(os.tmpdir()); +const SCRATCH_HOME = path.join(SCRATCH_BASE, `.commandmate-demo-vitest-${process.pid}`); const DEMO_HOME = path.join(SCRATCH_HOME, '.commandmate-demo'); const STATE_FILE = path.join(DEMO_HOME, 'state.env'); const STUB = path.join(SCRATCH_HOME, 'stub-server.js'); @@ -215,8 +223,9 @@ async function portListening(port: number): Promise { } beforeAll(async () => { - // Collect dirs left by runs that were killed before afterAll (#3025). - sweepStaleScratchDirs(os.homedir()); + // Collect dirs left by runs that were killed before afterAll (#3025). Only + // under the temp dir: this file no longer writes to, or sweeps, the real home. + sweepStaleScratchDirs(SCRATCH_BASE); TEST_PORT = await reserveDemoPortPair(); removeTempDir(SCRATCH_HOME); fs.mkdirSync(DEMO_HOME, { recursive: true }); @@ -286,11 +295,10 @@ afterEach(() => { fs.rmSync(REQUEST_LOG, { force: true }); }); -// The scratch dir has to sit under $HOME (env-up refuses anything else, because -// validateDbPath rejects /tmp and /var), which makes leaving it behind a leak -// into a real user's home: one directory per `npm run test:unit`, never -// collected. afterAll still runs when a test — or beforeAll — fails, and the -// removal is force/recursive so it cannot throw and mask that failure. +// Leaving the scratch dir behind is a leak: one directory per +// `npm run test:unit`, collected only by the next run's sweep. afterAll still +// runs when a test — or beforeAll — fails, and the removal is force/recursive +// so it cannot throw and mask that failure. // `no-home-leftovers.test.ts` runs this file in a child process and asserts the // directory is gone afterwards, so deleting this hook turns that test red. afterAll(() => { @@ -323,6 +331,23 @@ describe('demo port band', () => { }); }); +describe('scratch location (Issue #3479)', () => { + it('keeps the scratch dir out of the real home, so env-clean never sees it', () => { + // A dir under the real $HOME was a `$HOME` entry for every parallel + // worktree's env-clean gate, and for this worker's own gate while its tests + // were still running (#3395: `.commandmate-demo-vitest-36552 [unattributed]`). + expect(isPathWithin(SCRATCH_HOME, os.homedir())).toBe(false); + expect(isPathWithin(SCRATCH_HOME, LOGIN_HOME)).toBe(false); + // Negative control: the check does see a path that is in the home. + expect(isPathWithin(path.join(os.homedir(), '.commandmate-demo-vitest-1'), os.homedir())).toBe(true); + }); + + it('is still a real directory that env-up accepts as an isolated HOME', () => { + expect(fs.statSync(SCRATCH_HOME).isDirectory()).toBe(true); + expect(isPathWithin(DEMO_HOME, SCRATCH_HOME)).toBe(true); + }); +}); + describe('env-up refuses configurations that could reach a live instance', () => { it('refuses port 3000', () => { const result = run(ENV_UP, [], { ...stubEnv(), CM_DEMO_PORT: '3000' }); diff --git a/tests/unit/skills/demo-video/no-home-leftovers.test.ts b/tests/unit/skills/demo-video/no-home-leftovers.test.ts index 01826472f..02f35818d 100644 --- a/tests/unit/skills/demo-video/no-home-leftovers.test.ts +++ b/tests/unit/skills/demo-video/no-home-leftovers.test.ts @@ -1,16 +1,18 @@ /** - * The demo-video env tests must not leave anything in the user's home (#1553). + * The demo-video env tests must not leave anything in the user's home (#1553), + * and since #3479 must not create anything there at all. * - * `env-scripts.test.ts` has to put its scratch dir under $HOME — env-up.sh - * refuses anywhere else, because validateDbPath rejects /tmp and /var as system - * directories. That makes forgetting to remove it a leak into a real home - * directory: one `.commandmate-demo-vitest-` per `npm run test:unit`, with - * nothing to ever collect them. Eight had accumulated before this guard existed. + * `env-scripts.test.ts` used to put its scratch dir under $HOME, so forgetting + * to remove it was a leak into a real home directory: one + * `.commandmate-demo-vitest-` per `npm run test:unit`. Eight had + * accumulated before this guard existed. Even removed on time, the dir was a + * `$HOME` entry for every env-clean gate on the machine while the run lasted + * (#3479), so it now lives under the OS temp dir. * * A hook that runs last cannot be observed from inside its own file, so this - * test runs that file in a child process with `HOME` pointed at a scratch - * directory and inspects what survives. `os.homedir()` returns `$HOME` on - * POSIX, so the child's scratch dir lands where this test can see it. + * test runs that file in a child process with `HOME` and `TMPDIR` pointed at + * scratch directories and inspects what survives in each. `os.homedir()` and + * `os.tmpdir()` follow those variables on POSIX. * * @vitest-environment node */ @@ -38,6 +40,8 @@ const TARGET = 'tests/unit/skills/demo-video/env-scripts.test.ts'; const TEST_NAME = 'refuses to start on top of an existing state file'; const SCRATCH_HOME = fs.mkdtempSync(path.join(os.tmpdir(), 'demo-video-home-')); +/** The child's `os.tmpdir()`, where its scratch dir is now made (#3479). */ +const SCRATCH_TMP = fs.mkdtempSync(path.join(os.tmpdir(), 'demo-video-tmp-')); function leftovers(home: string): string[] { return fs.readdirSync(home).filter((entry) => entry.startsWith('.commandmate-demo-vitest-')); @@ -71,6 +75,7 @@ beforeAll(() => { env: { ...process.env, HOME: SCRATCH_HOME, + TMPDIR: SCRATCH_TMP, NODE_ENV: 'test', // Belt: keep the child's summary colourless wherever it runs, so local // and CI see the same bytes. Braces: `passedTestCount` strips ANSI anyway, @@ -84,6 +89,7 @@ beforeAll(() => { afterAll(() => { removeTempDir(SCRATCH_HOME); + removeTempDir(SCRATCH_TMP); }); describe('env-scripts.test.ts cleans up after itself', () => { @@ -99,6 +105,19 @@ describe('env-scripts.test.ts cleans up after itself', () => { expect(leftovers(SCRATCH_HOME)).toEqual([]); }); + it('creates nothing in $HOME at all (#3479)', () => { + // Not just "no demo-vitest dir": while the run lasts, anything in the real + // home is an entry every parallel worktree's env-clean gate can see. + expect(fs.readdirSync(SCRATCH_HOME)).toEqual([]); + }); + + it('leaves no .commandmate-demo-vitest-* directory behind in its temp dir (#3479)', () => { + // The probe case passing proves the scratch dir existed; env-scripts makes + // it under `os.tmpdir()`, which the child resolves to SCRATCH_TMP. Its own + // 'scratch location' case pins that it is not under the home. + expect(leftovers(SCRATCH_TMP)).toEqual([]); + }); + it('counts the child summary whether or not vitest coloured it', () => { // Colour is not reproducible on demand: vitest leaves the summary plain in // a local pipe *and* under a pty, and colours it on GitHub Actions. So the diff --git a/tests/unit/skills/demo-video/stale-scratch.ts b/tests/unit/skills/demo-video/stale-scratch.ts index f59739222..7c064da50 100644 --- a/tests/unit/skills/demo-video/stale-scratch.ts +++ b/tests/unit/skills/demo-video/stale-scratch.ts @@ -1,8 +1,8 @@ /** - * Sweeping of `.commandmate-demo-vitest-` scratch dirs left in $HOME (#3025). + * Sweeping of `.commandmate-demo-vitest-` scratch dirs left behind (#3025). * - * `env-scripts.test.ts` has to create its scratch dir under the real $HOME and - * removes it in `afterAll`. A killed or timed-out run never reaches `afterAll`, + * `env-scripts.test.ts` creates its scratch dir under the OS temp dir (the real + * $HOME until #3479) and removes it in `afterAll`. A killed or timed-out run never reaches `afterAll`, * so the dir stays behind. The next run collects the ones whose owner is gone. */ import fs from 'node:fs'; @@ -34,7 +34,7 @@ export function isStaleScratchDir(name: string): boolean { return pid !== null && !isPidAlive(pid); } -/** Remove stale scratch dirs directly under `home`; returns the names removed. */ +/** Remove stale scratch dirs directly under `home` (any base dir); returns the names removed. */ export function sweepStaleScratchDirs(home: string): string[] { const removed: string[] = []; for (const entry of fs.readdirSync(home, { withFileTypes: true })) {