fix(test): strip the instance-selection env vars in test/setup.ts

`test/setup.ts` gives every test file a temp HOME so the suite cannot touch the
real Codeman tree, and strips the env vars that would leak past it — but the
list only covered auth and the gesture flag. The three vars
`src/config/instance.ts` derives the data dir and tmux socket from were missing,
and they reach past the temp HOME:

- **`CODEMAN_DATA_DIR` is the one that matters.** It is an ABSOLUTE override
  read in `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`.
- **`CODEMAN_INSTANCE`** moves the data dir to `~/.codeman-<name>` and the
  socket to `codeman-<name>`. Inside the temp HOME that is not data loss, but it
  silently changes the paths tests assert on — and `scripts/run-beta.sh` exports
  it, so any shell that has run a beta carries it.
- **`CODEMAN_TMUX_SOCKET`** renames the socket `resolveTmuxSocketName()`
  returns. `TmuxManager` no-ops its shell commands under vitest, so this is
  assertion drift rather than a stray `tmux -L` against prod — same class of
  leak, same one-line fix.

They are deleted in the setup file rather than in a hook because
`CODEMAN_INSTANCE` is captured into a module-level const the first time
`config/instance.ts` is imported; a `beforeEach` would already be too late.

`test/test-env-isolation.test.ts` pins the whole list in two halves, because the
obvious half is not enough: asserting the vars are unset passes trivially on a
machine that never set them, so a removed `delete` line would sail through on
almost every box and on CI. The static half reads `setup.ts` and asserts each
name is deleted there, which fails everywhere. An anti-drift check catches the
other direction — a var stripped in `setup.ts` but never given a reason in the
list — and is scoped to the strip section so the teardown's restores are not
mistaken for strips.

Verified by demonstrating the leak: with the `CODEMAN_DATA_DIR` line removed and
the var exported, the runtime assertion fails; with the line restored it passes.
Full suite: no new failures against an upstream/master baseline.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQkoi1cNegqVwZHgzx5SbJ
This commit is contained in:
Devvyn
2026-09-02 09:49:09 +08:00
co-authored by Claude Opus 5
parent 71ffbf18e4
commit 1125f7c1c5
2 changed files with 105 additions and 2 deletions
+32 -2
View File
@@ -5,8 +5,11 @@
* mode before application modules load. Tests therefore cannot touch the real * mode before application modules load. Tests therefore cannot touch the real
* Codeman state/cases tree or launch external tmux-backed agent sessions. * Codeman state/cases tree or launch external tmux-backed agent sessions.
* *
* This setup file strips shell-level auth configuration that can leak from a * This setup file strips shell-level configuration that can leak from a running
* running Codeman instance, then handles mock/timer cleanup between tests. * Codeman instance — auth (`CODEMAN_PASSWORD`/`CODEMAN_USERNAME`), the gesture
* flag, and the three INSTANCE-selection vars that would otherwise point the
* suite at a real data dir or tmux socket — then handles mock/timer cleanup
* between tests.
*/ */
import { mkdtempSync, rmSync } from 'node:fs'; import { mkdtempSync, rmSync } from 'node:fs';
@@ -39,6 +42,33 @@ delete process.env.CODEMAN_USERNAME;
// (test/server-index-title.test.ts) when the shell exports CODEMAN_GESTURE=1. // (test/server-index-title.test.ts) when the shell exports CODEMAN_GESTURE=1.
delete process.env.CODEMAN_GESTURE; delete process.env.CODEMAN_GESTURE;
// Instance selection is PROCESS-WIDE and is what `src/config/instance.ts` derives
// both the data dir and the tmux socket from, so a shell that exports any of these
// three reaches straight past the temp HOME above and undoes the isolation this
// file exists to provide:
//
// - CODEMAN_DATA_DIR is the dangerous one. It is an ABSOLUTE override read in
// `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.
// - CODEMAN_INSTANCE moves the data dir to `~/.codeman-<name>` and the socket to
// `codeman-<name>`. 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`
// exports it, so any shell that has run a beta carries it.
// - CODEMAN_TMUX_SOCKET renames the socket `resolveTmuxSocketName()` returns.
// `TmuxManager` no-ops its shell commands under vitest, so this is assertion
// drift rather than a stray `tmux -L` against prod — but it is the same class
// of leak and the same one-line fix.
//
// ⚠️ These must be deleted HERE rather than in a test, because `CODEMAN_INSTANCE`
// is captured into a module-level const the first time `config/instance.ts` is
// imported. A setup file runs before any application module loads; a beforeEach
// would already be too late.
delete process.env.CODEMAN_INSTANCE;
delete process.env.CODEMAN_DATA_DIR;
delete process.env.CODEMAN_TMUX_SOCKET;
afterEach(() => { afterEach(() => {
vi.clearAllMocks(); vi.clearAllMocks();
vi.useRealTimers(); vi.useRealTimers();
+73
View File
@@ -0,0 +1,73 @@
/**
* @fileoverview Pins the environment isolation `test/setup.ts` provides.
*
* The suite's hermeticity rests on a temp `HOME` plus a short list of env vars that are
* deleted before any application module loads. That list is easy to under-maintain: it grew
* once for auth (`CODEMAN_PASSWORD`/`CODEMAN_USERNAME`) and once for `CODEMAN_GESTURE`, both
* times only after a leak had already produced a confusing failure, and it was still missing
* the three INSTANCE-selection vars.
*
* Those three matter more than the ones already on the list, because `src/config/instance.ts`
* derives BOTH the data dir and the tmux socket from them, and `CODEMAN_DATA_DIR` is an
* absolute path that bypasses `HOME` entirely — so a developer who exports it has the suite
* reading and writing their real `state.json` rather than a throwaway tree.
*
* ⚠️ The runtime half of this file cannot fail on a machine where the vars were never set, so
* it is not enough on its own: a `delete` line removed from `setup.ts` would still pass here
* on almost every developer's box and on CI. The STATIC half is what actually guards the
* list — it reads `setup.ts` and asserts each name is deleted there, which fails wherever the
* suite runs. Both halves are deliberate; do not drop the static one as redundant.
*
* Port: none (pure, over process.env and one source file).
*/
import { describe, expect, it } from 'vitest';
import { readFileSync } from 'node:fs';
import { fileURLToPath } from 'node:url';
/** Every env var `setup.ts` must strip, with why it would otherwise leak. */
const STRIPPED_ENV_VARS: Array<[name: string, why: string]> = [
['CODEMAN_PASSWORD', 'auth from a running instance would make protected routes behave differently'],
['CODEMAN_USERNAME', 'same, and it changes which owner scoping resolves to'],
['CODEMAN_GESTURE', 'flips renderIndexHtml output and breaks byte-identity assertions'],
['CODEMAN_INSTANCE', 'moves the data dir to ~/.codeman-<name> and the tmux socket to codeman-<name>'],
['CODEMAN_DATA_DIR', 'ABSOLUTE override: bypasses the temp HOME and points the suite at a real data dir'],
['CODEMAN_TMUX_SOCKET', 'renames the socket resolveTmuxSocketName() returns'],
];
const SETUP_SOURCE = readFileSync(fileURLToPath(new URL('./setup.ts', import.meta.url)), 'utf-8');
/**
* Just the top-of-file STRIP section, cut at the first hook.
*
* The teardown below it restores HOME/USERPROFILE/VITEST/PLAYWRIGHT_BROWSERS_PATH with the
* same `delete` syntax, and those are the opposite of a strip — counting them would make the
* anti-drift check demand a reason for a var the suite deliberately puts back.
*/
const SETUP_STRIP_SECTION = SETUP_SOURCE.split(/^afterEach\(/m)[0];
describe('test environment isolation', () => {
it.each(STRIPPED_ENV_VARS)('%s is unset while the suite runs', (name) => {
expect(process.env[name], `${name} leaked into the test environment`).toBeUndefined();
});
it.each(STRIPPED_ENV_VARS)('setup.ts deletes %s (%s)', (name) => {
// The half that fails everywhere, not just on a machine that happens to export the var.
expect(SETUP_SOURCE, `setup.ts no longer deletes ${name}`).toContain(`delete process.env.${name};`);
});
it('runs against a throwaway HOME, not the real one', () => {
// The property every other test's isolation is built on: `~/.codeman` and `~/codeman-cases`
// both resolve under here, so a test that writes state cannot reach the developer's own.
const home = process.env.HOME ?? process.env.USERPROFILE;
expect(home).toBeTruthy();
expect(home).toContain('codeman-vitest-');
});
it('lists every name the setup file strips (anti-drift)', () => {
// Catches the other direction: a var added to setup.ts but never given a reason here, so
// the next person cannot tell whether it is load-bearing or left over.
const deleted = [...SETUP_STRIP_SECTION.matchAll(/delete process\.env\.([A-Z0-9_]+);/g)].map((m) => m[1]).sort();
expect(deleted).toEqual(STRIPPED_ENV_VARS.map(([name]) => name).sort());
});
});