diff --git a/CLAUDE.md b/CLAUDE.md index e9de12e5..4e887201 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -407,7 +407,7 @@ Raw `npx vitest` skips the config (and with it `setup.ts`); always use `npm test **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` env var, set automatically), `TmuxManager` no-ops ALL shell commands and becomes a pure in-memory mock — tests physically cannot create/kill/attach real tmux sessions (`IS_TEST_MODE` in `src/tmux-manager.ts`). Every docker IO path is no-op'd the same way. `Session` is test-gated too: instead of attaching a real tmux client, it spawns a raw-mode echo PTY (`TEST_PTY_SCRIPT` in `src/session.ts`), so integration tests get a live input/output loop that echoes each byte exactly once. `test/setup.ts` gives every test file a temporary `HOME`/`USERPROFILE` (all `homedir()`-derived state, `~/.codeman` and `~/codeman-cases` included, resolves into a per-file fixture; the Playwright browser cache path is preserved), and additionally strips `CODEMAN_PASSWORD`/`CODEMAN_USERNAME` (so auth state from the running instance can't leak into tests) and `CODEMAN_GESTURE` (a shell-exported gesture flag would flip render-injection assertions), and points `CODEMAN_DATA_DIR` at a throwaway dir (#356). ⚠️ That last one is what actually protects `~/.codeman`: `getDataDir()` reads `CODEMAN_DATA_DIR` as an ABSOLUTE override before it ever looks at `homedir()`, so one inherited from the shell (a second instance, a beta run) 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. 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`. ⚠️ Raw `npx vitest` without `--config` skips `setup.ts` and with it the temp-HOME isolation. +**Tmux safety**: under vitest (`VITEST` env var, set automatically), `TmuxManager` no-ops ALL shell commands and becomes a pure in-memory mock — tests physically cannot create/kill/attach real tmux sessions (`IS_TEST_MODE` in `src/tmux-manager.ts`). Every docker IO path is no-op'd the same way. `Session` is test-gated too: instead of attaching a real tmux client, it spawns a raw-mode echo PTY (`TEST_PTY_SCRIPT` in `src/session.ts`), so integration tests get a live input/output loop that echoes each byte exactly once. `test/setup.ts` gives every test file a temporary `HOME`/`USERPROFILE` (all `homedir()`-derived state, `~/.codeman` and `~/codeman-cases` included, resolves into a per-file fixture; the Playwright browser cache path is preserved), and additionally strips `CODEMAN_PASSWORD`/`CODEMAN_USERNAME` (so auth state from the running instance can't leak into tests) and `CODEMAN_GESTURE` (a shell-exported gesture flag would flip render-injection assertions), and strips the three instance-selection vars `CODEMAN_INSTANCE`/`CODEMAN_DATA_DIR`/`CODEMAN_TMUX_SOCKET` (#356/#371; `test/test-env-isolation.test.ts` pins the list, and its STATIC half reads setup.ts so a dropped `delete` fails everywhere rather than only on a box that exports the var). ⚠️ `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`. ⚠️ Raw `npx vitest` without `--config` skips `setup.ts` and with it the temp-HOME isolation. **Ports**: Pick unique ports manually, 3150+. Search `const PORT =` before adding new tests. Never 3000 (the live instance). diff --git a/config/vitest.ci.config.ts b/config/vitest.ci.config.ts index e0490680..0305838c 100644 --- a/config/vitest.ci.config.ts +++ b/config/vitest.ci.config.ts @@ -23,13 +23,6 @@ export default defineConfig({ include: ['test/**/*.test.ts'], exclude: [...configDefaults.exclude, ...NON_CI_TEST_GLOBS], setupFiles: ['./test/setup.ts'], - // SAFETY: force every worker's data dir away from prod `~/.codeman`. Route - // tests (e.g. session-routes-workspace-hooks) write remote-hosts.json into - // `getDataDir()`; without this a bare run clobbers the production host - // registry (found 2026-08-29). `/tmp` is fine here — the tree is throwaway. - env: { - CODEMAN_DATA_DIR: '/tmp/codeman-vitest-data', - }, fileParallelism: false, testTimeout: 30000, teardownTimeout: 60000, diff --git a/config/vitest.config.ts b/config/vitest.config.ts index 7b1317d4..578e6c4a 100644 --- a/config/vitest.config.ts +++ b/config/vitest.config.ts @@ -21,13 +21,6 @@ export default defineConfig({ environment: 'node', include: ['test/**/*.test.ts'], setupFiles: ['./test/setup.ts'], - // SAFETY: force every worker's data dir away from prod `~/.codeman`. Route - // tests (e.g. session-routes-workspace-hooks) write remote-hosts.json into - // `getDataDir()`; without this a bare run clobbers the production host - // registry (found 2026-08-29). `/tmp` is fine here — the tree is throwaway. - env: { - CODEMAN_DATA_DIR: '/tmp/codeman-vitest-data', - }, // Run test files sequentially to respect mux session limits // Individual tests within files still run in parallel where safe fileParallelism: false, diff --git a/test/cli-skill-target.test.ts b/test/cli-skill-target.test.ts index 66e720b2..e8db8525 100644 --- a/test/cli-skill-target.test.ts +++ b/test/cli-skill-target.test.ts @@ -43,10 +43,10 @@ beforeEach(() => { }); afterEach(() => { - // LINKED_CASES_FILE is dataPath('linked-cases.json') → CODEMAN_DATA_DIR, - // which test/setup.ts points at a throwaway /tmp dir, so a plain delete is - // safe here. Only homedir()-derived paths (CASES_DIR/LINKED_ROOT) need the - // containment gate. + // LINKED_CASES_FILE is dataPath('linked-cases.json'), which test/setup.ts + // sandboxes (temp HOME, and an inherited CODEMAN_DATA_DIR is stripped), so a + // plain delete is safe here. The case trees still go through the containment + // gate as defense in depth. rmSync(LINKED_CASES_FILE, { force: true }); safeRmHomeTree(CASES_DIR); safeRmHomeTree(LINKED_ROOT); diff --git a/test/routes/session-routes-workspace-hooks.test.ts b/test/routes/session-routes-workspace-hooks.test.ts index 94f0a7fb..82c55820 100644 --- a/test/routes/session-routes-workspace-hooks.test.ts +++ b/test/routes/session-routes-workspace-hooks.test.ts @@ -169,8 +169,9 @@ describe('POST /api/sessions workspace hooks', () => { // as a junk directory under the server cwd. statusLineTelemetry rides along: // applyStatusLineConfig mkdirs the same way and used to run for remote attaches. // SAFETY (2026-08-29): write straight to `getDataDir()` — `test/setup.ts` - // already sandboxes CODEMAN_DATA_DIR for the whole file (same convention as - // the docker-hosts fixtures below). A prior version of this test stubbed + // already sandboxes the data dir for the whole file (temp HOME, inherited + // CODEMAN_DATA_DIR stripped; same convention as the docker-hosts fixtures + // below). A prior version of this test stubbed // CODEMAN_DATA_DIR to a SEPARATE throwaway dir for just this write, but // `session-routes.ts`'s `CODEMAN_CONFIG_DIR` is a module-load-time constant // (frozen at the sandboxed dir before this test ever runs), so that fixture diff --git a/test/setup.ts b/test/setup.ts index 49c34f81..2bbb742d 100644 --- a/test/setup.ts +++ b/test/setup.ts @@ -21,9 +21,7 @@ const originalHome = process.env.HOME; const originalUserProfile = process.env.USERPROFILE; const originalVitest = process.env.VITEST; const originalPlaywrightBrowsersPath = process.env.PLAYWRIGHT_BROWSERS_PATH; -const originalCodemanDataDir = process.env.CODEMAN_DATA_DIR; const testHome = mkdtempSync(join(tmpdir(), 'codeman-vitest-')); -const testDataDir = join(tmpdir(), `codeman-vitest-data-${process.pid}`); if (originalPlaywrightBrowsersPath === undefined && originalHome) { process.env.PLAYWRIGHT_BROWSERS_PATH = @@ -37,17 +35,6 @@ process.env.HOME = testHome; process.env.USERPROFILE = testHome; process.env.VITEST = 'true'; -// SAFETY: `getDataDir()` is `process.env.CODEMAN_DATA_DIR || join(homedir(), '.codeman')`. -// The temp HOME above already redirects the second half (`os.homedir()` follows -// `$HOME`; libuv checks the env var before the passwd entry), but the first half -// is an ABSOLUTE override: a `CODEMAN_DATA_DIR` inherited from the shell (a -// second instance, a beta run) bypasses the temp HOME entirely, and a bare suite -// run then reads and writes the REAL data dir (found 2026-08-29: -// `session-routes-workspace-hooks.test.ts` overwrote the production -// `remote-hosts.json` with an `h1/box/10.0.0.5` fixture, wiping every user-defined -// remote host and emptying the launch case dropdown). Point it at a throwaway dir. -process.env.CODEMAN_DATA_DIR = testDataDir; - delete process.env.CODEMAN_PASSWORD; delete process.env.CODEMAN_USERNAME; // Gesture availability changes renderIndexHtml output (injects the @@ -64,7 +51,13 @@ delete process.env.CODEMAN_GESTURE; // `getDataDir()`, so it bypasses HOME entirely: a developer who exports it // (or a shell left over from `codeman web -d`) has the suite reading and // WRITING their real `state.json`, `users.json`, `intents.json` and -// `hook-secret` instead of a throwaway tree. +// `hook-secret` instead of a throwaway tree. Found live 2026-08-29 (#356): +// `session-routes-workspace-hooks.test.ts` overwrote a production +// `remote-hosts.json` with its `h1/box/10.0.0.5` fixture. `os.homedir()` +// itself DOES follow `$HOME`, so with this var gone `getDataDir()` lands +// under the temp HOME like everything else. (#356 first answered this by +// pointing the var at a second throwaway dir; deleting it is the same +// protection with one tree to clean up.) // - CODEMAN_INSTANCE moves the data dir to `~/.codeman-` and the socket to // `codeman-`. Inside the temp HOME that is not a data-loss risk, but it // silently changes the paths tests assert on — and `scripts/run-beta.sh` @@ -109,11 +102,7 @@ afterAll(async () => { if (originalPlaywrightBrowsersPath === undefined) delete process.env.PLAYWRIGHT_BROWSERS_PATH; else process.env.PLAYWRIGHT_BROWSERS_PATH = originalPlaywrightBrowsersPath; - if (originalCodemanDataDir === undefined) delete process.env.CODEMAN_DATA_DIR; - else process.env.CODEMAN_DATA_DIR = originalCodemanDataDir; - rmSync(testHome, { recursive: true, force: true }); - rmSync(testDataDir, { recursive: true, force: true }); }); // afterAll never fires for a fully-skipped test file (no tests execute), which @@ -121,5 +110,4 @@ afterAll(async () => { // with force is a no-op when afterAll already removed it. process.on('exit', () => { rmSync(testHome, { recursive: true, force: true }); - rmSync(testDataDir, { recursive: true, force: true }); });