Skip to content

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

Merged
Ark0N merged 1 commit into
Ark0N:masterfrom
opticon454:fix/test-env-instance-isolation
Sep 4, 2026
Merged

fix(test): strip the instance-selection env vars in test/setup.ts#371
Ark0N merged 1 commit into
Ark0N:masterfrom
opticon454:fix/test-env-instance-isolation

Conversation

@opticon454

Copy link
Copy Markdown
Contributor

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.

`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
@opticon454

Copy link
Copy Markdown
Contributor Author

As found/mentioned in #347

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 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
@Ark0N
Ark0N merged commit 65d19c7 into Ark0N:master Sep 4, 2026
2 checks passed
@Ark0N

Ark0N commented Sep 4, 2026

Copy link
Copy Markdown
Owner

Merged, thank you. The static half of the test is the right call and it is what kept this honest once #356 landed next to it: that PR had answered the same leak by pointing CODEMAN_DATA_DIR at a second throwaway dir, so the plain merge set the variable and deleted it a few lines later. I resolved it in favour of your strip (72fd231): the second directory, its restores and the vitest config env entries are gone, getDataDir() lands under the temp HOME like everything else, and CLAUDE.md's testing paragraph now names the three variables and why CODEMAN_INSTANCE has to go in the setup file rather than a hook. Test-only, so it rides the next release rather than getting one of its own.

@opticon454
opticon454 deleted the fix/test-env-instance-isolation branch September 4, 2026 12:21
@Ark0N Ark0N mentioned this pull request Sep 4, 2026
Sign up for free to 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