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 })) {