Skip to content

test: repair route harness error rendering + stop AI-checker spawning real processes in tests#97

Merged
Ark0N merged 2 commits into
Ark0N:masterfrom
TeigenZhang:fix/route-test-harness-errors
May 25, 2026
Merged

test: repair route harness error rendering + stop AI-checker spawning real processes in tests#97
Ark0N merged 2 commits into
Ark0N:masterfrom
TeigenZhang:fix/route-test-harness-errors

Conversation

@TeigenZhang
Copy link
Copy Markdown
Contributor

Two fixes that make the route test suite correct and safe to run.

1. Render structured route errors in the test harness + fix stale assertions

Route helpers (findSessionOrFail → 404, parseBody → 400) throw structured { statusCode, body } errors that the production server converts via a global setErrorHandler. The route test harness built a bare Fastify instance without that handler, so thrown errors fell through to Fastify's default handler — yielding a {statusCode,error,message} body instead of {success:false,...}, while the tests still asserted the old implicit-200 behavior. 51 route tests across 7 files were red.

  • Extract the handler into src/web/route-error-handler.ts; the server and the test harness now install the identical handler (single source of truth — no production behavior change).
  • Correct stale assertions across the route test files: throw-based paths now assert 404 (unknown session) / 400 (invalid body); genuine in-handler return createErrorResponse(...) paths (200 + success:false) left untouched.

2. Stop the AI idle checker from spawning real processes in tests

respawn-controller.test.ts drives the AI idle checker, whose runCheck() spawns a real tmux new-session running claude -p. The AI-enabled tests only assert the ai_checking state transition (then cancel/stop), so the spawn leaked stray tmux sessions + claude processes on every run — the reason the full suite was unsafe to run inside a managed session. Mock node:child_process here (mirroring ai-idle-checker.test.ts), spreading the real module so exec stays intact for transitively-imported modules.

Test plan

  • tsc --noEmit clean
  • npm test -- test/routes/ → 305 passing (was 254/305)
  • npm test -- test/respawn-controller.test.ts → 162 passing, zero real tmux/claude spawned (verified via before/after tmux session diff)

🤖 Generated with Claude Code

Teigen added 2 commits May 25, 2026 20:15
…sertions

The route test harness built a bare Fastify instance without the production
global error handler (server.ts), so structured errors thrown by route helpers
(findSessionOrFail → 404, parseBody → 400) fell through to Fastify's default
handler — yielding a `{statusCode,error,message}` body instead of the
`{success:false,...}` shape, and the tests asserted the old implicit-200
behavior. 51 route tests across 7 files were red.

- Extract the handler into src/web/route-error-handler.ts; server.ts and the
  test harness now install the identical handler (single source of truth).
- Correct stale assertions across route test files: throw-based error paths
  now assert 404 (unknown session) / 400 (invalid body); genuine in-handler
  `return createErrorResponse(...)` paths (200 + success:false) left untouched.
- Reformat a few test files prettier flagged (pre-existing non-compliance).

Route suite: 307/307 passing (was 256/307). No production behavior change.
…cesses

respawn-controller.test.ts drives the AI idle checker (ai-checker-base), whose
runCheck() spawns a real `tmux new-session` running `claude -p`. The AI-enabled
tests only assert the ai_checking state transition (then cancel/stop), so the
spawn produced stray real tmux sessions and claude processes on every run — the
reason `npm test` (full suite) was unsafe to run inside a managed session.

Mock node:child_process here (mirroring ai-idle-checker.test.ts), spreading the
real module so `exec` stays intact for transitively-imported modules
(tmux-manager calls promisify(exec) at load). With this, the full non-mobile
suite runs without spawning any real tmux/claude.
@Ark0N
Copy link
Copy Markdown
Owner

Ark0N commented May 25, 2026

Thank you very much for this, @TeigenZhang — this is an excellent, well-documented contribution. 🙏

I reviewed it carefully and verified both claims locally:

  • Production change is a clean no-op refactor. The error handler is extracted verbatim into route-error-handler.ts and installed via installRouteErrorHandler() in both the server and the test harness — identical logic, single source of truth, no behavior change. 👍
  • Route harness fix confirmed: npm test -- test/routes/305 passing (was 254). The missing setErrorHandler was indeed why thrown findSessionOrFail/parseBody errors fell through to Fastify's default handler.
  • child_process mock confirmed safe: ran test/respawn-controller.test.ts (162 passing) with a before/after tmux list-sessions diff — zero real tmux/claude processes spawned. Spreading the real module to keep exec intact for tmux-manager's load-time promisify(exec) is exactly right.

CI (Typecheck & Lint) is green. Merging now — thanks again for making the suite both correct and safe to run! 🚀

@Ark0N Ark0N merged commit 1b652ce into Ark0N:master May 25, 2026
1 check passed
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