From ef2001b1c114fd6efb321124b07d0c58ffce775d Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Sun, 11 Oct 2026 11:00:34 +0800 Subject: [PATCH 1/6] fix(test): make a bare vitest invocation use the isolated CI config `npx vitest run` with no --config found no config file, so test/setup.ts never ran and HOME was not redirected: the suite wrote fixtures (clis.json with a test entry, users.json, webhook.json, intents.json, .env, settings.json values) into the developer's real ~/.codeman, and a running Codeman then loaded them on its next restart. A root vitest.config.ts re-exports config/vitest.ci.config.ts, so the default invocation is the isolated one; npm test and the browser/mobile/perf scripts keep passing their own --config. The shared temp-HOME teardown also retries on ENOTEMPTY, which failed whole files intermittently after every test had passed. --- .changeset/b569450a.md | 5 +++++ test/setup.ts | 4 +++- test/test-env-isolation.test.ts | 13 +++++++++++++ vitest.config.ts | 8 ++++++++ 4 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 .changeset/b569450a.md create mode 100644 vitest.config.ts diff --git a/.changeset/b569450a.md b/.changeset/b569450a.md new file mode 100644 index 00000000..578f328d --- /dev/null +++ b/.changeset/b569450a.md @@ -0,0 +1,5 @@ +--- +"aicodeman": patch +--- + +Test suite: a bare `npx vitest run` now resolves the isolated CI config instead of running with no setup file (which wrote fixtures into the real `~/.codeman`), and the shared temp-HOME teardown retries on `ENOTEMPTY`. diff --git a/test/setup.ts b/test/setup.ts index 0ef7e35b..7a546103 100644 --- a/test/setup.ts +++ b/test/setup.ts @@ -121,7 +121,9 @@ afterAll(async () => { if (originalPlaywrightBrowsersPath === undefined) delete process.env.PLAYWRIGHT_BROWSERS_PATH; else process.env.PLAYWRIGHT_BROWSERS_PATH = originalPlaywrightBrowsersPath; - rmSync(testHome, { recursive: true, force: true }); + // maxRetries: a straggling writer (a server persisting on shutdown) can recreate a file between + // rmSync's readdir and rmdir, which fails the whole file with ENOTEMPTY after every test passed. + rmSync(testHome, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); }); // afterAll never fires for a fully-skipped test file (no tests execute), which diff --git a/test/test-env-isolation.test.ts b/test/test-env-isolation.test.ts index 9ea5e0c7..93cafc47 100644 --- a/test/test-env-isolation.test.ts +++ b/test/test-env-isolation.test.ts @@ -78,3 +78,16 @@ describe('test environment isolation', () => { expect(deleted).toEqual(STRIPPED_ENV_VARS.map(([name]) => name).sort()); }); }); + +describe('a bare vitest invocation is still isolated', () => { + // Without a root config, `npx vitest run` loads no setup file, so HOME is never redirected and + // the suite writes fixtures into the developer's real ~/.codeman. Pin that the root config + // exists and carries the setup file, rather than trusting everyone to pass --config. + it('has a root vitest.config.ts that loads test/setup.ts', () => { + const root = fileURLToPath(new URL('..', import.meta.url)); + const src = readFileSync(`${root}vitest.config.ts`, 'utf8'); + expect(src).toContain('./config/vitest.ci.config'); + const ci = readFileSync(`${root}config/vitest.ci.config.ts`, 'utf8'); + expect(ci).toContain("setupFiles: ['./test/setup.ts']"); + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts new file mode 100644 index 00000000..58fedb7a --- /dev/null +++ b/vitest.config.ts @@ -0,0 +1,8 @@ +/** + * Bare `vitest` / `npx vitest run` (no `--config`) resolves THIS file. Without it there is no + * config at all, so no `setupFiles`: `test/setup.ts` never runs, HOME is not redirected, and the + * suite reads and writes the developer's real `~/.codeman` (settings, clis.json, users.json, + * .env, ...). It points at the CI gate so the default invocation is always the isolated one; + * `npm test`, `test:browser`, `test:mobile` and `test:perf` still pass their own `--config`. + */ +export { default } from './config/vitest.ci.config'; From 686ce5ac02c6fae7bafeb15a8b5fef7d75c3ed09 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Sun, 11 Oct 2026 12:53:11 +0800 Subject: [PATCH 2/6] docs(test): a bare vitest run is isolated now, and why it mattered CLAUDE.md and Contributing said raw npx vitest skips the config; state what happened when it did (fixtures written into the real ~/.codeman) and that the root vitest.config.ts now makes a bare run isolated, with the test that pins it. --- CLAUDE.md | 2 +- docs/wiki/Contributing.md | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 207548c2..e6f2f9b2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -331,7 +331,7 @@ npm run test:all # literally everything; fails ~87 tests on a clean master ⚠️ **A file filter must match the runner.** `npm test -- test/mobile/keyboard.test.ts` matches nothing and exits GREEN having run zero tests, because the gate's config excludes that path — an excluded file needs its own runner (`npm run test:mobile -- `, `npm run test:browser -- `, `npm run test:perf -- `). Vitest treats "no files matched a filter" as success, so read the file count, not just the colour. -Raw `npx vitest` skips the config (and with it `setup.ts`); always use `npm test --` or pass `--config`. +A raw `npx vitest` used to find no config, so `setup.ts` never ran and the suite wrote fixtures into the REAL `~/.codeman` (a `clis.json` with a test entry, `users.json`, `webhook.json`, `.env`, settings values: it looks exactly like "my settings were reset"). The root `vitest.config.ts` now re-exports `config/vitest.ci.config.ts`, so a bare run is isolated too, and `test/test-env-isolation.test.ts` pins that. Still prefer `npm test --`, and pass `--config config/vitest.config.ts` (or the browser/mobile/perf configs) deliberately for the suites the gate leaves out. **Config**: Vitest with `globals: true`, `fileParallelism: false`. Timeout 30s, teardown 60s. `config/vitest.config.ts` is the everything-config behind `test:all`; `config/vitest.ci.config.ts` is the gate and derives its excludes from `config/test-suites.ts`, which is also what `vitest.browser.config.ts` and `vitest.perf.config.ts` derive their includes from — so the exclusions and the runners cannot drift apart. Keep shared options in sync across them. diff --git a/docs/wiki/Contributing.md b/docs/wiki/Contributing.md index 171203b7..ccb6b87c 100644 --- a/docs/wiki/Contributing.md +++ b/docs/wiki/Contributing.md @@ -53,7 +53,9 @@ something other than the checked-out HEAD, or when the tree has uncommitted chan checks would read. Skip it once with `CODEMAN_SKIP_PREPUSH=1 git push`; a `pre-push` hook of your own is never overwritten. -`npm test` runs the same suite CI runs, so a green run locally means a green run there. It +`npm test` runs the same suite CI runs, so a green run locally means a green run there. A bare +`npx vitest run` resolves the same isolated config (a root `vitest.config.ts`), so it never +touches your real `~/.codeman`; it did before that file existed. `npm test` also leaves out three suites that cannot pass on an arbitrary machine, each with its own command: `npm run test:browser` (Playwright, Chromium and a live server), `npm run test:mobile` (the same plus environment-specific screenshot baselines) and `npm run test:perf` (wall-clock From 1802bafc74cc554381c097d287bbb717ead010cf Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Sun, 11 Oct 2026 13:05:31 +0800 Subject: [PATCH 3/6] fix(test): keep removing the temp HOME until a straggling CLI goes quiet maxRetries alone was not enough: on a machine with a real agent CLI installed, a test's code path probes or spawns `claude`, which flushes its own ~/.claude.json and ~/.claude/backups/ into the temp HOME a moment after the test ends, so rmSync raced it and failed the whole file with ENOTEMPTY after every test had passed (about one run in three for session-routes-claude-model; CI has no CLI). Retry the removal for up to ~5s, and if it never goes quiet leave the folder to the exit backstop instead of failing a file that passed. 12 consecutive runs of that file: no failure. --- test/setup.ts | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/test/setup.ts b/test/setup.ts index 7a546103..e4f67be7 100644 --- a/test/setup.ts +++ b/test/setup.ts @@ -121,9 +121,20 @@ afterAll(async () => { if (originalPlaywrightBrowsersPath === undefined) delete process.env.PLAYWRIGHT_BROWSERS_PATH; else process.env.PLAYWRIGHT_BROWSERS_PATH = originalPlaywrightBrowsersPath; - // maxRetries: a straggling writer (a server persisting on shutdown) can recreate a file between - // rmSync's readdir and rmdir, which fails the whole file with ENOTEMPTY after every test passed. - rmSync(testHome, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + // A straggling writer can recreate files between rmSync's readdir and rmdir and fail the whole file + // with ENOTEMPTY after every test has passed. The usual culprit is a real agent CLI on a developer + // machine (the `claude` a test's code path probes or spawns) flushing its own `~/.claude.json` and + // `~/.claude/backups/` into the temp HOME a moment after the test ends; CI has no such CLI. Keep + // removing until it goes quiet, and if it never does, leave the temp dir to the `exit` backstop + // below: a leaked temp folder must not fail a test file that passed. + for (let attempt = 0; attempt < 20; attempt++) { + try { + rmSync(testHome, { recursive: true, force: true, maxRetries: 3, retryDelay: 50 }); + break; + } catch { + await new Promise((resolve) => setTimeout(resolve, 250)); + } + } }); // afterAll never fires for a fully-skipped test file (no tests execute), which From 710ed73585f3aa3141423543e6ef8dd7fc09003f Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Mon, 12 Oct 2026 16:58:16 +0800 Subject: [PATCH 4/6] fix(test): a real home is never "the test HOME" to the deletion guards safeRmHomeTree and isUnderTestHome answered "is this path under process.env.HOME?". With test/setup.ts skipped (a bare vitest) HOME is the developer's real home, so every real file passed, which is how the real linked-cases registry was deleted on 2026-10-10. They now also require HOME to be the throwaway codeman-vitest-* directory setup.ts creates, and the shared route harness throws when it is not, so every route test refuses to start against a real ~/.codeman rather than just the one file that checked. --- test/mocks/index.ts | 10 ++- test/mocks/test-helpers.ts | 79 ++++++++++++++------- test/routes/_route-test-utils.ts | 6 +- test/routes/case-custom-path-routes.test.ts | 4 +- test/test-home-guard.test.ts | 78 ++++++++++++++++++++ 5 files changed, 149 insertions(+), 28 deletions(-) create mode 100644 test/test-home-guard.test.ts diff --git a/test/mocks/index.ts b/test/mocks/index.ts index 2a696c2d..29bc0c8e 100644 --- a/test/mocks/index.ts +++ b/test/mocks/index.ts @@ -7,5 +7,13 @@ export { MockSession, createMockSession, terminalOutputs } from './mock-session.js'; export { MockStateStore } from './mock-state-store.js'; -export { waitForEvent, createDeferred, safeRmHomeTree, isUnderTestHome } from './test-helpers.js'; +export { + waitForEvent, + createDeferred, + safeRmHomeTree, + isUnderTestHome, + isSandboxedHome, + assertSandboxedHome, + TEST_HOME_PREFIX, +} from './test-helpers.js'; export { createMockRouteContext, type MockRouteContext } from './mock-route-context.js'; diff --git a/test/mocks/test-helpers.ts b/test/mocks/test-helpers.ts index edf13015..c930e0a2 100644 --- a/test/mocks/test-helpers.ts +++ b/test/mocks/test-helpers.ts @@ -3,7 +3,7 @@ */ import { rmSync } from 'node:fs'; -import { resolve } from 'node:path'; +import { basename, resolve } from 'node:path'; /** Wait for an EventEmitter to emit a specific event, with timeout */ export function waitForEvent( @@ -39,37 +39,68 @@ export function createDeferred(): { } /** - * Delete a directory tree, but ONLY when it lives inside the test HOME. - * - * SAFETY (2026-08-29): `test/setup.ts` redirects `process.env.HOME` to a - * throwaway dir and `os.homedir()` follows it, so a `rmSync(CASES_DIR, - * recursive)` normally lands inside the fixture. This gate is defense in depth - * for the day that stops being true (a test that runs outside setup.ts, an - * env override that anchors a path elsewhere): it refuses to delete anything - * not under the redirected `process.env.HOME`, so the failure mode is a - * leftover temp dir rather than a deleted PRODUCTION `~/codeman-cases`. - * Lexical `resolve()` is used because the leaf often does not exist and - * `realpathSync` would throw. + * Prefix of the throwaway HOME `test/setup.ts` creates (`mkdtempSync(join(tmpdir(), 'codeman-vitest-'))`). + * Pinned by test/test-env-isolation.test.ts, so renaming one without the other fails there. */ -export function safeRmHomeTree(path: string): void { +export const TEST_HOME_PREFIX = 'codeman-vitest-'; + +/** + * True only when `process.env.HOME` is the throwaway directory `test/setup.ts` made. + * + * ⚠️ This is what the helpers below hang on, and it exists because "is the path under HOME" + * is a vacuous question when setup never ran: a bare `vitest` invocation has no setupFiles, + * HOME is the developer's REAL home, and every real file is then "under the test HOME". That is + * how a bare run on 2026-10-10 deleted the real linked-cases registry through a guard that was + * written to prevent exactly that. The temp directory's name is the only thing a real home + * cannot have. + */ +export function isSandboxedHome(): boolean { const home = process.env.HOME; - if (!home) return; - const target = resolve(path); - const root = resolve(home); - if (target === root || target.startsWith(root + '/')) { - rmSync(target, { recursive: true, force: true }); + return !!home && basename(resolve(home)).startsWith(TEST_HOME_PREFIX); +} + +/** + * Throw unless the suite is running under `test/setup.ts`'s temp HOME. The shared route harness + * calls this, so every route test refuses to start against a real `~/.codeman`. + */ +export function assertSandboxedHome(what = 'this test'): void { + if (!isSandboxedHome()) { + throw new Error( + `${what} reads and writes Codeman's data files and must run under test/setup.ts's temp HOME ` + + `(HOME=${process.env.HOME ?? '(unset)'}). Run it with \`npm test\` or ` + + `\`npx vitest run --config config/vitest.ci.config.ts \`, never with a bare vitest.` + ); } } /** - * True when `path` resolves strictly inside `process.env.HOME` (or to it). - * Same rationale as `safeRmHomeTree`; use for guarded non-recursive deletes - * (single files like `linked-cases.json`) so they can never touch prod state. + * Delete a directory tree, but ONLY when it lives inside the test HOME. + * + * SAFETY (2026-08-29, tightened 2026-10-12): `test/setup.ts` redirects `process.env.HOME` to a + * throwaway dir and `os.homedir()` follows it, so a `rmSync(CASES_DIR, recursive)` normally + * lands inside the fixture. This gate is defense in depth for the day that stops being true + * (a test that runs outside setup.ts, an env override that anchors a path elsewhere): it + * refuses to delete anything unless HOME IS the throwaway dir ({@link isSandboxedHome}) and the + * target is under it, so the failure mode is a leftover temp dir rather than a deleted + * PRODUCTION `~/codeman-cases`. The first version only checked "under HOME", which a run + * without setup.ts satisfied for the user's real home. + * Lexical `resolve()` is used because the leaf often does not exist and + * `realpathSync` would throw. + */ +export function safeRmHomeTree(path: string): void { + if (!isUnderTestHome(path)) return; + rmSync(resolve(path), { recursive: true, force: true }); +} + +/** + * True when `path` resolves strictly inside the throwaway test HOME (or to it), and false + * whenever HOME is anything else. Same rationale as `safeRmHomeTree`; use for guarded + * non-recursive deletes (single files like `linked-cases.json`) so they can never touch + * prod state. */ export function isUnderTestHome(path: string): boolean { - const home = process.env.HOME; - if (!home) return false; + if (!isSandboxedHome()) return false; + const root = resolve(process.env.HOME as string); const target = resolve(path); - const root = resolve(home); return target === root || target.startsWith(root + '/'); } diff --git a/test/routes/_route-test-utils.ts b/test/routes/_route-test-utils.ts index 42a4c291..d254bfa0 100644 --- a/test/routes/_route-test-utils.ts +++ b/test/routes/_route-test-utils.ts @@ -6,7 +6,7 @@ */ import Fastify, { type FastifyInstance } from 'fastify'; import fastifyCookie from '@fastify/cookie'; -import { createMockRouteContext, type MockRouteContext } from '../mocks/index.js'; +import { assertSandboxedHome, createMockRouteContext, type MockRouteContext } from '../mocks/index.js'; import { installRouteErrorHandler } from '../../src/web/route-error-handler.js'; import { MAX_ROUTE_PARAM_LENGTH } from '../../src/web/route-helpers.js'; @@ -31,6 +31,10 @@ export async function createRouteTestHarness( registerFn: (app: FastifyInstance, ctx: any) => void, ctxOptions?: { sessionId?: string; authUser?: { username: string; role: 'admin' | 'user' } } ): Promise { + // Route tests write the linked-cases, docker and settings files. Refuse to build a harness against + // a real home (a bare vitest has no setupFiles), instead of finding out from a wiped registry. + assertSandboxedHome('A route test'); + const app = Fastify({ logger: false, routerOptions: { maxParamLength: MAX_ROUTE_PARAM_LENGTH } }); // Register cookie plugin — some routes access req.cookies diff --git a/test/routes/case-custom-path-routes.test.ts b/test/routes/case-custom-path-routes.test.ts index 9db507d1..b49d703e 100644 --- a/test/routes/case-custom-path-routes.test.ts +++ b/test/routes/case-custom-path-routes.test.ts @@ -24,12 +24,12 @@ import { basename, join } from 'node:path'; import { createRouteTestHarness } from './_route-test-utils.js'; import { registerCaseRoutes } from '../../src/web/routes/case-routes.js'; import { dataPath } from '../../src/config/instance.js'; -import { safeRmHomeTree } from '../mocks/index.js'; +import { isSandboxedHome, safeRmHomeTree } from '../mocks/index.js'; const LINKED = () => dataPath('linked-cases.json'); const CASES_DIR = () => join(homedir(), 'codeman-cases'); /** test/setup.ts's temp HOME (its prefix is pinned by test/test-env-isolation.test.ts). */ -const sandboxed = () => basename(homedir()).startsWith('codeman-vitest-'); +const sandboxed = () => isSandboxedHome(); let workDir = ''; const work = () => workDir; const linked = (): Record => (existsSync(LINKED()) ? JSON.parse(readFileSync(LINKED(), 'utf8')) : {}); diff --git a/test/test-home-guard.test.ts b/test/test-home-guard.test.ts new file mode 100644 index 00000000..6481df99 --- /dev/null +++ b/test/test-home-guard.test.ts @@ -0,0 +1,78 @@ +/** + * @fileoverview The test-only deletion guards must treat a REAL home as "not the test HOME". + * + * `safeRmHomeTree` and `isUnderTestHome` were written so a test could never delete production + * state, and they asked "is this path under `process.env.HOME`?". With `test/setup.ts` skipped + * (a bare `vitest`), HOME is the developer's real home, so that answer is "yes" for every real + * file, and on 2026-10-10 they approved wiping the real linked-cases registry. They now also + * require HOME to be the throwaway directory setup.ts made. + * + * Port: none. Everything happens in a temp tree; HOME is stubbed for the duration of a test. + */ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { + TEST_HOME_PREFIX, + assertSandboxedHome, + isSandboxedHome, + isUnderTestHome, + safeRmHomeTree, +} from './mocks/index.js'; +import { createRouteTestHarness } from './routes/_route-test-utils.js'; + +const made: string[] = []; +/** A stand-in for a developer's REAL home: a real directory with real files, not a sandbox name. */ +function fakeRealHome(): string { + const dir = mkdtempSync(join(tmpdir(), 'not-the-sandbox-')); + made.push(dir); + mkdirSync(join(dir, '.codeman')); + writeFileSync(join(dir, '.codeman', 'linked-cases.json'), '{"keep":"/me"}'); + return dir; +} + +afterEach(() => { + vi.unstubAllEnvs(); + for (const d of made.splice(0)) rmSync(d, { recursive: true, force: true }); +}); + +describe('test HOME guards', () => { + it('this suite really runs under the sandboxed HOME', () => { + expect(isSandboxedHome()).toBe(true); + expect(() => assertSandboxedHome()).not.toThrow(); + }); + + it('pins the prefix test/setup.ts creates', () => { + expect(TEST_HOME_PREFIX).toBe('codeman-vitest-'); + }); + + it('safeRmHomeTree does not delete anything under a real home', () => { + const home = fakeRealHome(); + vi.stubEnv('HOME', home); + safeRmHomeTree(join(home, '.codeman')); + expect(existsSync(join(home, '.codeman', 'linked-cases.json'))).toBe(true); + }); + + it('isUnderTestHome is false for a real home, even for a file inside it', () => { + const home = fakeRealHome(); + vi.stubEnv('HOME', home); + expect(isSandboxedHome()).toBe(false); + expect(isUnderTestHome(join(home, '.codeman', 'linked-cases.json'))).toBe(false); + }); + + it('still deletes inside the real sandbox', () => { + const inside = mkdtempSync(join(process.env.HOME as string, 'guard-')); + writeFileSync(join(inside, 'f'), 'x'); + expect(isUnderTestHome(inside)).toBe(true); + safeRmHomeTree(inside); + expect(existsSync(inside)).toBe(false); + }); + + it('the route harness refuses to start against a real home', async () => { + const home = fakeRealHome(); + vi.stubEnv('HOME', home); + await expect(createRouteTestHarness(() => undefined)).rejects.toThrow(/temp HOME/); + expect(existsSync(join(home, '.codeman', 'linked-cases.json'))).toBe(true); + }); +}); From 63108686967c25d15bc06c8fc09ad64ea2d9acea Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Thu, 22 Oct 2026 19:25:04 +0200 Subject: [PATCH 5/6] fix(test): bound the temp-HOME removal by a deadline, not an attempt count Merge-time fix for #660. afterAll runs under vitest's 10 s hookTimeout, and each failed removal attempt costs rmSync's own retry sleeps (50 + 100 + 150 ms) plus the 250 ms wait, so 20 attempts overran it: with a writer still filling $HOME/.claude/backups, the file failed with "Hook timed out in 10000ms" after its test passed, and the "leave it to the exit backstop" branch was never reached. The loop now stops after about 5 s. Checked with a temporary (uncommitted) test that leaves a child process writing there for 20 s: before, the file failed at 11 s with the hook timeout; after, it passes in under 6 s. Co-Authored-By: Claude Opus 5.5 (1M context) --- test/setup.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/test/setup.ts b/test/setup.ts index e4f67be7..01724de3 100644 --- a/test/setup.ts +++ b/test/setup.ts @@ -125,13 +125,18 @@ afterAll(async () => { // with ENOTEMPTY after every test has passed. The usual culprit is a real agent CLI on a developer // machine (the `claude` a test's code path probes or spawns) flushing its own `~/.claude.json` and // `~/.claude/backups/` into the temp HOME a moment after the test ends; CI has no such CLI. Keep - // removing until it goes quiet, and if it never does, leave the temp dir to the `exit` backstop - // below: a leaked temp folder must not fail a test file that passed. - for (let attempt = 0; attempt < 20; attempt++) { + // removing until it goes quiet, for at most about 5 s, and if it never does, leave the temp dir to + // the `exit` backstop below: a leaked temp folder must not fail a test file that passed. + // ⚠️ The bound is a deadline, not an attempt count: this hook runs under vitest's 10 s + // `hookTimeout`, and each failed attempt costs rmSync's own retry sleeps (50 + 100 + 150 ms) plus + // the wait below, so 20 attempts overran it and failed the file with "Hook timed out". + const deadline = Date.now() + 5_000; + for (;;) { try { rmSync(testHome, { recursive: true, force: true, maxRetries: 3, retryDelay: 50 }); break; } catch { + if (Date.now() >= deadline) break; await new Promise((resolve) => setTimeout(resolve, 250)); } } From 6d2763d50bc1fe1fc3bed12c017ef109c7fafaff Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Thu, 22 Oct 2026 19:25:52 +0200 Subject: [PATCH 6/6] docs(test): a bare vitest is isolated now, and the guards need the sandbox HOME Merge-time fix for #660. CLAUDE.md and architecture-invariants still said a raw npx vitest skips setup.ts, contradicting the updated Testing note: both now say it resolves the root vitest.config.ts (and why that file must stay). The architecture-invariants entry for safeRmHomeTree adds the sandbox-HOME requirement (isSandboxedHome, TEST_HOME_PREFIX), the same rule for isUnderTestHome, the route-harness assert and the 2026-10-10 incident. CLAUDE.md's repo-root preamble lists vitest.config.ts with its reason, so a tidy-up does not move it into config/ and reopen #640. The assertSandboxedHome JSDoc no longer claims every route test is covered: only the ones built on createRouteTestHarness are. Co-Authored-By: Claude Opus 5.5 (1M context) --- CLAUDE.md | 4 ++-- docs/architecture-invariants.md | 4 ++-- test/mocks/test-helpers.ts | 3 ++- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e6f2f9b2..a04e8e3c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -6,7 +6,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co > > **This file is in `.prettierignore` on purpose.** Prettier's markdown printer escapes underscores inside the glob-heavy paths used throughout (`agent-*.jsonl` became `agent-\_.jsonl`, collapsing backtick spans and corrupting a whole paragraph). Do not remove the ignore entry, and do not run `prettier --write` on it. > -> **Repo root is kept short on purpose** (the README sits below the file listing on GitHub). Config lives in `config/` (`eslint.config.js`, `knip.json`, the vitest configs), Prettier's config is the `"prettier"` key in `package.json`, and `SECURITY.md` is under `.github/`. Root-only files are the ones tools genuinely require there: `CLAUDE.md` + `AGENTS.md` (loaded from the root by Claude Code / Codex), `CHANGELOG.md` (changesets writes it next to `package.json`), `tsconfig.json`, `.editorconfig`, `.nvmrc`/`.npmrc`, `.prettierignore` (resolved relative to cwd), `LICENSE` (GitHub detection), `.dockerignore` (the build context is the repo root, so Docker resolves it there and nowhere else) and `install.sh` (its raw URL is the published install one-liner). `.claude-plugin/marketplace.json` is root-only for the same reason: `/plugin marketplace add Ark0N/Codeman` reads it from the repo root and nowhere else, which makes the repo its own plugin marketplace. The one plugin it lists is `plugins/codeman/` (manifest + README + a MIRROR of `skills/codeman/`), and ⚠️ the plugin is a small separate directory on purpose: `claude plugin install` copies the plugin root into its cache, and a plugin root that carries a `package.json` gets an **npm install** at install time (measured with the repo root as plugin root: 832 MB, 511 packages and this repo's postinstall on every installer's machine), while a symlink to `skills/codeman` would dangle in the copy. `skills/codeman/` stays the single source; `scripts/sync-plugin.mjs` mirrors it and writes `package.json`'s version into both manifests inside `version-packages`, and `test/plugin-manifest.test.ts` pins the byte-identity, the versions, the absence of a `package.json` in the plugin root and that no other component (`commands/`, `agents/`, `hooks/`, `.mcp.json`, `settings.json`) rides along. `npm run check:plugin` runs that drift check plus both strict validations with EXPLICIT paths (`plugins/codeman`, `.claude-plugin/marketplace.json`): a bare `.` argument copied out of prose reads as a full stop and gets dropped, which surfaces as `missing required argument 'path'`. Needs the `claude` CLI, so it is a local check, not a CI step. Don't relocate those. +> **Repo root is kept short on purpose** (the README sits below the file listing on GitHub). Config lives in `config/` (`eslint.config.js`, `knip.json`, the vitest configs), Prettier's config is the `"prettier"` key in `package.json`, and `SECURITY.md` is under `.github/`. Root-only files are the ones tools genuinely require there: `CLAUDE.md` + `AGENTS.md` (loaded from the root by Claude Code / Codex), `CHANGELOG.md` (changesets writes it next to `package.json`), `tsconfig.json`, `.editorconfig`, `.nvmrc`/`.npmrc`, `.prettierignore` (resolved relative to cwd), `vitest.config.ts` (a one-line re-export of `config/vitest.ci.config.ts`: vitest finds its config by walking up from cwd, so without it a bare `npx vitest` ran with no setup file and wrote into the real `~/.codeman`, #640), `LICENSE` (GitHub detection), `.dockerignore` (the build context is the repo root, so Docker resolves it there and nowhere else) and `install.sh` (its raw URL is the published install one-liner). `.claude-plugin/marketplace.json` is root-only for the same reason: `/plugin marketplace add Ark0N/Codeman` reads it from the repo root and nowhere else, which makes the repo its own plugin marketplace. The one plugin it lists is `plugins/codeman/` (manifest + README + a MIRROR of `skills/codeman/`), and ⚠️ the plugin is a small separate directory on purpose: `claude plugin install` copies the plugin root into its cache, and a plugin root that carries a `package.json` gets an **npm install** at install time (measured with the repo root as plugin root: 832 MB, 511 packages and this repo's postinstall on every installer's machine), while a symlink to `skills/codeman` would dangle in the copy. `skills/codeman/` stays the single source; `scripts/sync-plugin.mjs` mirrors it and writes `package.json`'s version into both manifests inside `version-packages`, and `test/plugin-manifest.test.ts` pins the byte-identity, the versions, the absence of a `package.json` in the plugin root and that no other component (`commands/`, `agents/`, `hooks/`, `.mcp.json`, `settings.json`) rides along. `npm run check:plugin` runs that drift check plus both strict validations with EXPLICIT paths (`plugins/codeman`, `.claude-plugin/marketplace.json`): a bare `.` argument copied out of prose reads as a full stop and gets dropped, which surfaces as `missing required argument 'path'`. Needs the `claude` CLI, so it is a local check, not a CI step. Don't relocate those. ## Quick Reference @@ -335,7 +335,7 @@ A raw `npx vitest` used to find no config, so `setup.ts` never ran and the suite **Config**: Vitest with `globals: true`, `fileParallelism: false`. Timeout 30s, teardown 60s. `config/vitest.config.ts` is the everything-config behind `test:all`; `config/vitest.ci.config.ts` is the gate and derives its excludes from `config/test-suites.ts`, which is also what `vitest.browser.config.ts` and `vitest.perf.config.ts` derive their includes from — so the exclusions and the runners cannot drift apart. Keep shared options in sync across them. -**Tmux safety**: under vitest (`VITEST`), `TmuxManager` no-ops ALL shell commands (`IS_TEST_MODE` in `src/tmux-manager.ts`), docker IO is no-op'd likewise, and `Session` spawns an echo PTY (`TEST_PTY_SCRIPT`) instead of attaching tmux. `test/setup.ts` gives each file a temp `HOME`/`USERPROFILE` and strips `CODEMAN_PASSWORD`/`CODEMAN_USERNAME`, `CODEMAN_GESTURE` and `CODEMAN_INSTANCE`/`CODEMAN_DATA_DIR`/`CODEMAN_TMUX_SOCKET` (pinned by `test/test-env-isolation.test.ts`). ⚠️ `CODEMAN_DATA_DIR` overrides the temp HOME, so never drop its strip; strip `CODEMAN_INSTANCE` in the setup file, never in a hook (captured at first import). ⚠️ Delete case trees only via `safeRmHomeTree()`. ⚠️ Raw `npx vitest` without `--config` skips `setup.ts` and its isolation. → [architecture-invariants#test-isolation-tmux-docker-and-home](docs/architecture-invariants.md#test-isolation-tmux-docker-and-home) +**Tmux safety**: under vitest (`VITEST`), `TmuxManager` no-ops ALL shell commands (`IS_TEST_MODE` in `src/tmux-manager.ts`), docker IO is no-op'd likewise, and `Session` spawns an echo PTY (`TEST_PTY_SCRIPT`) instead of attaching tmux. `test/setup.ts` gives each file a temp `HOME`/`USERPROFILE` and strips `CODEMAN_PASSWORD`/`CODEMAN_USERNAME`, `CODEMAN_GESTURE` and `CODEMAN_INSTANCE`/`CODEMAN_DATA_DIR`/`CODEMAN_TMUX_SOCKET` (pinned by `test/test-env-isolation.test.ts`). ⚠️ `CODEMAN_DATA_DIR` overrides the temp HOME, so never drop its strip; strip `CODEMAN_INSTANCE` in the setup file, never in a hook (captured at first import). ⚠️ Delete case trees only via `safeRmHomeTree()`. ⚠️ A bare `npx vitest` gets `setup.ts` only through the root `vitest.config.ts` (a re-export of the gate config): keep that file, and never point `--config` at a config without `setupFiles`. → [architecture-invariants#test-isolation-tmux-docker-and-home](docs/architecture-invariants.md#test-isolation-tmux-docker-and-home) **Ports**: every in-process test server binds port 0 (the one exception, `test/codex-predictive-echo.test.ts`, starts a separate lab server process on 3222). A test that constructs `WebServer` reads `server.boundPort` (mobile tests: `createTestServer()`, then `server.boundPort`); a raw `http`/`net`/Fastify/`ws` server listens on `0` and reads `address().port`. `test/test-ports-guard.test.ts` fails a `WebServer` built on any other port and a raw `listen` on a number or `…PORT` constant. A test that needs a port nothing listens on binds 0, reads it and closes (`closedPort()` in `test/daemon-control.test.ts`), never "the server's port + 1". Never 3000 (the live instance). diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index 896e8d73..3e056e49 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -1170,9 +1170,9 @@ The repair is a **chmod, not a rebuild**: the prebuilt binary is fine. `scripts/ ⚠️ `CODEMAN_DATA_DIR` is the one that matters: `getDataDir()` reads it as an ABSOLUTE override before it ever looks at `homedir()`, so one inherited from the shell (a second instance, a beta run, a shell left over from `codeman web -d`) bypasses the temp HOME entirely, and a bare suite run once overwrote the real `remote-hosts.json` with a route test's fixture. `os.homedir()` itself DOES follow `$HOME`, so the temp HOME is what redirects everything else; `CODEMAN_INSTANCE` must be stripped in the setup file and never in a hook, because `config/instance.ts` captures it into a module-level const on first import. -Tests that delete case trees go through `safeRmHomeTree()` (`test/mocks`), which refuses any path outside the temp HOME, so a wrong anchor leaves a temp dir behind instead of deleting `~/codeman-cases`. +Tests that delete case trees go through `safeRmHomeTree()` (`test/mocks`), which refuses any path outside the temp HOME, so a wrong anchor leaves a temp dir behind instead of deleting `~/codeman-cases`. ⚠️ It also refuses everything unless HOME IS the throwaway dir `test/setup.ts` made (`isSandboxedHome()`: its basename starts with `TEST_HOME_PREFIX`, `codeman-vitest-`, the one thing a real home cannot have). "Under HOME" alone is vacuous when setup never ran: on 2026-10-10 a bare `vitest` run had the developer's real home as HOME, every real file was "under the test HOME", and the guard written to prevent exactly that deleted the real linked-cases registry. `isUnderTestHome()` (for single-file deletes such as `linked-cases.json`) carries the same requirement, and the shared route harness `createRouteTestHarness()` calls `assertSandboxedHome()`, so a route test built on it refuses to start against a real `~/.codeman` (a route test that builds its own app from `createMockRouteContext()` is not covered by that). -⚠️ Raw `npx vitest` without `--config` skips `setup.ts` and with it the temp-HOME isolation. +⚠️ A bare `npx vitest` (no `--config`) resolves the root `vitest.config.ts`, which re-exports `config/vitest.ci.config.ts`, so it runs `setup.ts` and gets the temp HOME like `npm test`. Vitest looks for its config by walking up from cwd, so this holds from `test/` as well, while `packages/xterm-zerolag-input` keeps its own config. Before that file existed a bare run found no config, ran without the setup file, and wrote fixtures into the real `~/.codeman` (#640: a `clis.json` with a test entry, `users.json`, `webhook.json`, `.env`, settings values). `test/test-env-isolation.test.ts` pins it; a `--config` pointing at a config without `setupFiles` would still skip the isolation. ### install.sh: the public installer diff --git a/test/mocks/test-helpers.ts b/test/mocks/test-helpers.ts index c930e0a2..12c0a280 100644 --- a/test/mocks/test-helpers.ts +++ b/test/mocks/test-helpers.ts @@ -61,7 +61,8 @@ export function isSandboxedHome(): boolean { /** * Throw unless the suite is running under `test/setup.ts`'s temp HOME. The shared route harness - * calls this, so every route test refuses to start against a real `~/.codeman`. + * (`createRouteTestHarness`) calls this, so a route test built on it refuses to start against a real + * `~/.codeman`. A route test that builds its own app from `createMockRouteContext` is not covered. */ export function assertSandboxedHome(what = 'this test'): void { if (!isSandboxedHome()) {