Skip to content

fix(test): isolate route tests from the production ~/.codeman data dir - #356

Merged
Ark0N merged 4 commits into
Ark0N:masterfrom
timkjr:pr/test-isolation
Sep 4, 2026
Merged

fix(test): isolate route tests from the production ~/.codeman data dir#356
Ark0N merged 4 commits into
Ark0N:masterfrom
timkjr:pr/test-isolation

Conversation

@timkjr

Copy link
Copy Markdown

Problem

test/routes/session-routes-workspace-hooks.test.ts writes its h1/box/10.0.0.5 remote-host fixture into getDataDir()/remote-hosts.json:

awaitmkdir(getDataDir(),{recursive: true});awaitwriteFile(join(getDataDir(),'remote-hosts.json'),JSON.stringify([{id: 'h1',label: 'box',host: '10.0.0.5',username: 'dev'}]));

getDataDir() resolves via homedir()~/.codeman (INSTANCE_SUFFIX='' by default). Overriding HOME in test/setup.ts does not change os.homedir() on Linux — it reads /etc/passwd, not $HOME. So every full-suite run silently overwrites the production ~/.codeman/remote-hosts.json: user-defined remote hosts are wiped, the launch-case dropdown empties, and remote session creation breaks with "host not found".

Found live 2026-08-29: a bare suite run destroyed a production host registry. The test suite is tmux-safe by design, but this write path bypassed every safety.

Why the obvious fix doesn't work

Vitest 4.1.8 ignores the test.env config key (probe-confirmed: a worker still saw CODEMAN_DATA_DIR=undefined). Setting the env in test/setup.ts also doesn't propagate reliably. So the fix must happen inside the test itself, at call time — getDataDir() reads process.env.CODEMAN_DATA_DIR on every call, so vi.stubEnv works:

constfixtureDataDir=join(tmpdir(),`codeman-hook-fixture-${process.pid}`);vi.stubEnv('CODEMAN_DATA_DIR',fixtureDataDir);try{awaitmkdir(getDataDir(),{recursive: true});awaitwriteFile(/* fixture */);}finally{vi.unstubAllEnvs();}

Changes

  • test/routes/session-routes-workspace-hooks.test.ts: redirect the fixture write to a throwaway /tmp dir + finally unstub.
  • test/setup.ts: set CODEMAN_DATA_DIR to a temp dir as defense-in-depth, and clean it up.
  • config/vitest.config.ts + config/vitest.ci.config.ts: declare CODEMAN_DATA_DIR (inert in v4, correct in v5+ — harmless, and keeps intent documented).

Verification

  • md5sum ~/.codeman/remote-hosts.json before and after vitest run test/routes/session-routes-workspace-hooks.test.ts — identical (was being clobbered before).
  • Fixture lands in /tmp/codeman-hook-fixture-*, not prod.
  • 17/17 tests in the file pass.

Suggested rule

Any test that writes into getDataDir() MUST stub CODEMAN_DATA_DIR to a throwaway dir first. The suite's HOME override is not sufficient on Linux.

timkjr pushed a commit to timkjr/Codeman that referenced this pull request Aug 31, 2026
PR Ark0N#356 stopped the remote-hosts.json fixture write from clobbering prod.
Two holes in the same file remain:
1. The quick-start afterEach still ran rmSync(CASES_DIR, recursive).
CASES_DIR is join(homedir(), 'codeman-cases'), and on Linux builds
where os.homedir() reads /etc/passwd instead of $HOME it resolves to
the PROD case tree - so a full-suite run deleted the real
~/codeman-cases. Add a shared safeRmHomeTree() containment gate that
only deletes a path under the redirected test HOME.
2. setup.ts teardown did rmSync(process.env.CODEMAN_DATA_DIR ?? '') AFTER
restoring the env - if a pre-existing prod CODEMAN_DATA_DIR was set,
that deleted prod. Capture the throwaway dir in a const and clean that.
A broader test-isolation sweep (10 files: cli-skill-target, edge-cases,
integration-flows, operation-lightspeed, ralph-integration,
case-clone-routes, voice-routes, session-cleanup, sse-events,
sse-subscription-filter) also applies the same containment gates to every
per-case delete. It is intentionally NOT included here to keep this PR
skinny; it is identified and available on request.
@timkjr

Copy link
Copy Markdown
Author

Test-isolation sweep identified — available on request

While hardening this PR I found the same containment gap in a broader set of tests: per-case cleanup that does rmSync(join(homedir(), 'codeman-cases', ...)) (or writes into dataPath()) without the CODEMAN_DATA_DIR/HOME-override protection. On platforms where os.homedir() ignores $HOME, each of these can touch the real ~/codeman-cases or ~/.codeman during a bare suite run.

The affected files (identified, changes ready, deliberately not included here to keep this PR skinny):

  • test/cli-skill-target.test.ts (writes+deletes linked-cases.json)
  • test/edge-cases.test.ts, test/integration-flows.test.ts, test/session-cleanup.test.ts, test/ralph-integration.test.ts (per-case deletes)
  • test/operation-lightspeed.test.ts, test/sse-events.test.ts, test/sse-subscription-filter.test.ts (per-case deletes)
  • test/routes/case-clone-routes.test.ts (clone dirs under CASES_DIR)
  • test/routes/voice-routes.test.ts (writes+deletes ~/.claude/.credentials.json)

Each applies the same safeRmHomeTree containment gate (only delete a path under the redirected test HOME) that this PR adds. Happy to open a follow-up PR with that sweep if it's wanted.

@timkjr

Copy link
Copy Markdown
Author

Pushed a follow-up fix: the "never writes hooks for a remote attach" test was stubbing CODEMAN_DATA_DIR to a separate throwaway dir for its fixture write, but session-routes.ts's CODEMAN_CONFIG_DIR is a module-load-time constant frozen before the test runs — so the fixture landed somewhere the route handler could never read, the host lookup silently failed, and the test passed for the wrong reason (Fastify defaults an unset status to 200). Now writes straight to getDataDir(), matching the docker-hosts fixture convention already used elsewhere in this file. Verified it now exercises the actual success path. Still 6 files, still scoped to the original fix.

timkjr added 4 commits September 2, 2026 20:40
session-routes-workspace-hooks.test.ts wrote its h1/box/10.0.0.5 host
fixture into getDataDir()/remote-hosts.json. getDataDir() resolves via
homedir() → ~/.codeman (INSTANCE_SUFFIX='' by default), and overriding
HOME in test/setup.ts does NOT change os.homedir() on Linux — so every
full-suite run silently overwrote the PRODUCTION remote-hosts.json,
wiping user-defined remote hosts, emptying the launch-case dropdown and
breaking remote session creation (found live 2026-08-29).
The vitest v4 test.env config key is ignored (probe confirmed the
worker still saw CODEMAN_DATA_DIR=undefined), so the reliable fix is
stubbing the env inside the test: the fixture write now goes to a
throwaway /tmp dir via vi.stubEnv + finally unstub. Verified: prod
remote-hosts.json hash is identical before and after the suite run.
PR Ark0N#356 stopped the remote-hosts.json fixture write from clobbering prod.
Two holes in the same file remain:
1. The quick-start afterEach still ran rmSync(CASES_DIR, recursive).
CASES_DIR is join(homedir(), 'codeman-cases'), and on Linux builds
where os.homedir() reads /etc/passwd instead of $HOME it resolves to
the PROD case tree - so a full-suite run deleted the real
~/codeman-cases. Add a shared safeRmHomeTree() containment gate that
only deletes a path under the redirected test HOME.
2. setup.ts teardown did rmSync(process.env.CODEMAN_DATA_DIR ?? '') AFTER
restoring the env - if a pre-existing prod CODEMAN_DATA_DIR was set,
that deleted prod. Capture the throwaway dir in a const and clean that.
A broader test-isolation sweep (10 files: cli-skill-target, edge-cases,
integration-flows, operation-lightspeed, ralph-integration,
case-clone-routes, voice-routes, session-cleanup, sse-events,
sse-subscription-filter) also applies the same containment gates to every
per-case delete. It is intentionally NOT included here to keep this PR
skinny; it is identified and available on request.
…ads it
The "never writes hooks for a remote attach" test stubbed CODEMAN_DATA_DIR
to a separate throwaway dir just for this write, but session-routes.ts's
CODEMAN_CONFIG_DIR is a module-load-time constant frozen at test/setup.ts's
sandboxed dir before this test ever runs. The fixture landed somewhere the
route handler could never read, so the remote-host lookup silently failed
(NOT_FOUND) and the test passed for the wrong reason -- createErrorResponse
never sets reply.code(), so Fastify's default 200 made the NOT_FOUND branch
and the intended success branch indistinguishable by status code alone.
Write straight to getDataDir() instead, matching the docker-hosts fixture
convention already used elsewhere in this file. Verified the fix actually
exercises the success path (host resolves, 200 with a real session), not
just an accidental 200 from the error branch.
Ark0N#356 introduced safeRmHomeTree/isUnderTestHome to stop tests from deleting
the PRODUCTION ~/codeman-cases tree on platforms where os.homedir() ignores
the $HOME override -- but only applied it to the one file caught doing it
live. CASES_DIR has no CODEMAN_DATA_DIR-style env override at all, so every
other test file's raw rmSync(join(CASES_DIR, ...)) was the same unguarded
pattern, just not yet triggered.
Routes every CASES_DIR delete in these 10 files through safeRmHomeTree:
cli-skill-target, edge-cases, integration-flows, operation-lightspeed,
ralph-integration, routes/case-clone-routes, routes/voice-routes,
session-cleanup, sse-events, sse-subscription-filter.
Also fixes one instance in case-clone-routes.test.ts that mkdirSync'd then
rmSync'd a CASES_DIR path directly with no guard at all -- the exact
clobbering pattern Ark0N#356 exists to prevent, found by extending the sweep.
Held as a separate commit (and intended as a separate PR once Ark0N#356 merges)
rather than folding into Ark0N#356 -- keeps the already-checked skinny fix
reviewable on its own; this is the same bug class applied broadly, not new
functionality.
Verified: all 10 files pass (180 tests), npm run typecheck clean.
@Ark0N
Ark0N merged commit 9696078 into Ark0N:masterSep 4, 2026
2 checks passed
Ark0N pushed a commit that referenced this pull request Sep 4, 2026
#356 stopped a bare suite run from overwriting the production
`remote-hosts.json` by pointing `CODEMAN_DATA_DIR` at a throwaway dir, and it
gated every case-tree delete on the temp HOME. Both changes are right; the
explanation written next to them is not. It says `os.homedir()` reads
/etc/passwd rather than `$HOME` on Linux, which would mean the temp HOME in
test/setup.ts never worked. It does: libuv checks the env var before the passwd
entry (measured: `HOME=/tmp/x node -e 'console.log(os.homedir())'` prints
/tmp/x), and CLAUDE.md's testing section relies on exactly that.
What bypasses the temp HOME is `CODEMAN_DATA_DIR` itself. `getDataDir()` reads
it as an absolute override before it looks at `homedir()`, so one inherited from
the shell (a second instance, a beta run) sends the whole suite at the real data
dir. That is the case setup.ts now closes, and #371 names the same variable from
the other direction.
The comments in setup.ts, the `safeRmHomeTree` helper, the voice-routes and
case-clone tests now say that, and the containment gate is described as what it
is: defense in depth. CLAUDE.md's testing paragraph gets the same note so the
next reader does not chase a homedir() bug that does not exist.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qg6bcATm1pNNY4kQWGwzgu
@Ark0N

Ark0N commented Sep 4, 2026

Copy link
Copy Markdown
Owner

Merged, thank you. Both changes are right and they ship in today's release. One correction to the write-up so the next reader does not chase it: Node's os.homedir() does follow $HOME on Linux (libuv checks the env var before the passwd entry; HOME=/tmp/x node -e 'console.log(os.homedir())' prints /tmp/x here), so the temp HOME in test/setup.ts was already redirecting ~/.codeman and ~/codeman-cases for the suite. What bypasses it is an inherited CODEMAN_DATA_DIR, which getDataDir() reads as an absolute override before it ever looks at homedir(), and that is exactly what your setup.ts change now neutralises. I pushed a small follow-up (2e0129f) that says so in the comments and in CLAUDE.md, and keeps safeRmHomeTree described as the defense in depth it is. @opticon454's #371 names the same variable from the other direction and will need a small rebase over this.

@Ark0NArk0N mentioned this pull request Sep 4, 2026
Ark0N pushed a commit that referenced this pull request Sep 4, 2026
#356 and #371 fixed the same leak two ways. #356 pointed CODEMAN_DATA_DIR at a
second throwaway directory and cleaned it up in afterAll and on exit; #371
deletes the variable along with CODEMAN_INSTANCE and CODEMAN_TMUX_SOCKET, so
`getDataDir()` falls back to `homedir()`, which the temp HOME already redirects.
Merged as they were, setup.ts set the variable and deleted it a few lines
later, and the second directory was created for nothing.
The strip wins: same protection, one tree to clean up, and the isolation test
#371 adds pins the list statically. The extra directory, its restore and its
two rmSync calls go, the vitest config `env` entries that set the same variable
go (they were documented as inert and would now be contradicted by the setup
file either way), the two test comments that described the old mechanism are
reworded, and CLAUDE.md's testing paragraph names the three stripped variables
and why CODEMAN_INSTANCE has to be stripped in the setup file rather than a hook.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qg6bcATm1pNNY4kQWGwzgu
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@timkjr@Ark0N