test: isolate runtime state and PTY integration - #175
Merged
Conversation
This was referenced Jul 29, 2026
Lint111
marked this pull request as ready for review
July 29, 2026 17:06
Ark0N
approved these changes
Jul 31, 2026
Ark0N
left a comment
Owner
There was a problem hiding this comment.
Verdict: Approve. This fixes two real, verified safety holes in the test harness. I reproduced the author's validation and additionally ran the full CI sweep that hadn't been run yet (details below). Recommend merging once GitHub CI has run on the branch.
What it does (3 files, +81/-19)
src/session.ts: gates four real-tmux touchpoints behindIS_TEST_MODE(process.env.VITEST): skipssetManualWindowSizeandqueryTmuxWindowSize, skipsresizeWindowon resize, and replaces the PTY attach with a deterministic echo transport (node -e 'process.stdin.pipe(process.stdout);') so integration tests get a live input/output loop without touching the host tmux server.test/quick-start.test.ts: gives the suite its own tempHOMEand switches to a lazy dynamic import ofserver.js(the static import is now type-only), soCASES_DIRresolves into the temp home.test/setup.ts: global per-test-file tempHOME/USERPROFILE, an explicitVITESTmarker, Playwright browser-cache path preservation, and full restore plus temp-dir removal inafterAll.
The bugs it fixes are real (verified in source)
TmuxManagerhas ~20IS_TEST_MODEgates, butsetManualWindowSize()andresizeWindow()are not gated (src/tmux-manager.ts:3150,:3169), andgetAttachCommand()/getAttachArgs()return a realtmux -L <socket> attach-session -t <name>. So before this PR, any test that started an interactive session spawned a real tmux client against the live shared socket. The CLAUDE.md claim that "tests physically cannot interact with real tmux" was false for exactly these paths.- The old quick-start suite computed
CASES_DIR = join(homedir(), 'codeman-cases')and didrmSync(..., { recursive: true, force: true })on case names including the defaulttestcase, i.e. it deleted from the real cases tree. The lazy import matters becausesrc/web/route-helpers.ts:29computesCASES_DIRat module load, so redirectingHOMEafter import would silently not work. The PR sequences this correctly.
Verification (worktree at PR head bba3d80)
tsc --noEmit: pass.prettier --checkon all three files: pass.- Author's claimed validation reproduces: 23/23 across the two suites.
- Full
npm run test:cisweep: 187 files passed (1 skipped), 3786 tests passed (12 skipped), zero failures. This was the main open risk, since thesetup.tschange affects every suite. - Live
tmux -L codemansocket snapshot before vs. after the 3786-test sweep, on a machine with 4 live production sessions: byte-identical. The isolation demonstrably works. os.homedir()follows$HOMEon Linux (mechanism confirmed);USERPROFILEcovers Windows.- Cleanly mergeable, 7 commits behind master, zero drift in the touched files since its base.
Findings and suggestions (none blocking)
- Echo-PTY output is doubled. Empirically, writing
hello\ryields"hello\r\nhello\r\n": once from tty line-discipline echo, once from the pipe. Input without\rcomes back only via tty echo (canonical mode buffers the pipe until newline). Today's assertions (buffer.length > 0) are fine, but a future "output contains X exactly once" assertion will mysteriously fail. Suggestprocess.stdin.setRawMode(true); process.stdin.pipe(process.stdout);as the script, which gives exactly-once, unbuffered echo. - The gates arguably belong in
TmuxManager, notSession. The project convention is that the mux layer self-mocks underVITEST, and the root cause here is precisely the two ungated methods plus the real attach command in that layer. Gating there (and returning the echo command fromgetAttachCommand/Argsin test mode) would fix all callers, keepsession.tsfree of test branches, and make the documented invariant true again. Also,IS_TEST_MODEis now duplicated in a second file, a small drift risk. Reasonable follow-up rather than a rework; the shipped approach works and mirrors the existing pattern. - Residual hole: the direct-PTY fallback paths (
pty.spawn('claude', ...)atsrc/session.ts:1589and:2025,pty.spawn(shell, ...)at:1919) are still ungated. They are only reachable in tests if the mux path throws and falls back, so this is theoretical, but "tests cannot launch agent processes" still has that asterisk. - quick-start's private temp HOME is redundant in the normal path (setup.ts already provides one per file), but it is justified belt-and-braces: raw
npx vitestwithout--configskipssetup.tsentirely, and the in-file fixture keeps this suite safe even then. Keep it. Minor: itsORIGINAL_HOMEactually captures setup.ts's temp home, not the real one, so its "restore" is cosmetic; harmless since the worker exits. - One empty temp dir leaks per fully-skipped test file (setup's module-level
mkdtempSyncruns butafterAllnever fires when zero tests execute; observed exactly 1 leak matching the 1 skipped file). Trivial; aprocess.on('exit')cleanup would close it, or just accept it. - Windows nit: the Playwright fallback when
LOCALAPPDATAis unset should bejoin(originalHome, 'AppData', 'Local', 'ms-playwright'). Cosmetic, sinceLOCALAPPDATAis essentially always set and CI is Linux. - Docs follow-up: CLAUDE.md's Testing section still describes safety as "TmuxManager no-ops all shell commands" only; the new temp-HOME and echo-PTY behavior deserves a line there (the PR did properly update
setup.ts's fileoverview).
Risks
- No CI has run on this PR yet (workflows pending approval). My local run used the exact CI config, but the real gate should run before merge.
- Production behavior: unchanged. Every new branch is dead unless
VITESTis set, which the production service never sets. Security posture is a strict improvement: tests can no longer mutate the live tmux socket or delete real user directories.
Ark0N
pushed a commit
that referenced
this pull request
Jul 31, 2026
…abilize CI teardown Follow-ups from the PR #175/#176 reviews: - Rewake helper self-terminates on its own 6h deadline and when orphaned, instead of relying on Claude Code to reap the poller - Rewake marker versioned (V2) with a version-agnostic ownership prefix, so future script updates replace older handlers instead of duplicating them; regression test covers the V1 to V2 swap - HOOK_TIMEOUT_MS renamed to HOOK_TIMEOUT_SECONDS = 10: the hook timeout field is seconds (the CLI multiplies by 1000), so the curl hooks have effectively had a ~2.8h timeout since COD-54 - Test echo PTY switches to raw mode: each input byte echoes exactly once (tty line discipline doubled every line and buffered until Enter) - test/setup.ts: drain in-flight console-log rpc forwards before environment teardown (fixes the EnvironmentTeardownError that failed CI twice on the merge commit with all 3820 tests passing), clean the temp home on process exit (fully-skipped files leaked it), fix the Windows Playwright cache fallback path - test/webview-proxy.test.ts: stop naming the vitest environment directive in prose; vitest matches it inside comments and silently ran the whole file under the jsdom environment while the comment claimed node - CLAUDE.md: document the temp-HOME and echo-PTY test isolation Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
HOME.HOME/USERPROFILEand explicitVITESTmarker before application imports can resolve runtime paths.
state, then restore the caller environment and remove only the temporary
fixture directory during teardown.
tmux/agent geometry calls, so integration tests exercise input/output without
creating or controlling host sessions.
Root cause
The test harness did not reliably identify itself as a test environment and
allowed module-level paths to resolve from the developer's real home directory.
The quick-start suite could therefore clean up the real
~/codeman-cases/testcasedirectory, including the checkout running Vitest.Session integration tests could also launch real tmux/agent processes.
Scope
This is the test-infrastructure slice extracted from #173. It changes the
quick-start fixture, global test setup, and a
VITEST-guarded session adapter.Normal application execution is unchanged.
Validation
npm run test:ci -- test/quick-start.test.ts test/integration-flows.test.ts:2 files, 23 tests passed
git diff --check: clean