diff --git a/CLAUDE.md b/CLAUDE.md index f5edebec..b596bccf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -320,7 +320,7 @@ Raw `npx vitest` skips `config/vitest.config.ts`; always use `npm test --` or pa **Config**: Vitest with `globals: true`, `fileParallelism: false`. Timeout 30s, teardown 60s. `config/vitest.ci.config.ts` = same minus the browser/perf excludes — keep the two configs in sync when changing shared options. -**Tmux safety**: under vitest (`VITEST` env var, set automatically), `TmuxManager` no-ops ALL shell commands and becomes a pure in-memory mock — tests physically cannot create/kill/attach real tmux sessions (`IS_TEST_MODE` in `src/tmux-manager.ts`). Every docker IO path is no-op'd the same way. `test/setup.ts` additionally strips `CODEMAN_PASSWORD`/`CODEMAN_USERNAME` (so auth state from the running instance can't leak into tests) and `CODEMAN_GESTURE` (a shell-exported gesture flag would flip render-injection assertions). +**Tmux safety**: under vitest (`VITEST` env var, set automatically), `TmuxManager` no-ops ALL shell commands and becomes a pure in-memory mock — tests physically cannot create/kill/attach real tmux sessions (`IS_TEST_MODE` in `src/tmux-manager.ts`). Every docker IO path is no-op'd the same way. `Session` is test-gated too: instead of attaching a real tmux client, it spawns a raw-mode echo PTY (`TEST_PTY_SCRIPT` in `src/session.ts`), so integration tests get a live input/output loop that echoes each byte exactly once. `test/setup.ts` gives every test file a temporary `HOME`/`USERPROFILE` (all `homedir()`-derived state, `~/.codeman` and `~/codeman-cases` included, resolves into a per-file fixture; the Playwright browser cache path is preserved), and additionally strips `CODEMAN_PASSWORD`/`CODEMAN_USERNAME` (so auth state from the running instance can't leak into tests) and `CODEMAN_GESTURE` (a shell-exported gesture flag would flip render-injection assertions). ⚠️ Raw `npx vitest` without `--config` skips `setup.ts` and with it the temp-HOME isolation. **Ports**: Pick unique ports manually, 3150+. Search `const PORT =` before adding new tests. Never 3000 (the live instance). diff --git a/src/config/auth-config.ts b/src/config/auth-config.ts index 0fc4f1b3..9aa60482 100644 --- a/src/config/auth-config.ts +++ b/src/config/auth-config.ts @@ -31,5 +31,10 @@ export const AUTH_FAILURE_WINDOW_MS = 15 * 60 * 1000; // Hooks // ============================================================================ -/** Timeout for Claude Code hook curl commands (ms) */ -export const HOOK_TIMEOUT_MS = 10000; +/** + * Timeout for Claude Code hook curl commands, in SECONDS: the hook `timeout` + * field is seconds (the CLI multiplies by 1000). The predecessor constant + * `HOOK_TIMEOUT_MS = 10000` fed the same field, so those hooks effectively had a + * ~2.8-hour timeout; 10 seconds is the originally intended budget. + */ +export const HOOK_TIMEOUT_SECONDS = 10; diff --git a/src/hooks-config.ts b/src/hooks-config.ts index ca8b0a2f..9c33bf6a 100644 --- a/src/hooks-config.ts +++ b/src/hooks-config.ts @@ -18,7 +18,7 @@ * Hook categories: `Notification` (3 matchers), `Stop` (1), `TeammateIdle` (1), * `TaskCompleted` (1), `PostToolUse` (1 self-contained background Bash rewake) * - * @dependencies types (HookEventType), config/auth-config (HOOK_TIMEOUT_MS) + * @dependencies types (HookEventType), config/auth-config (HOOK_TIMEOUT_SECONDS) * @consumedby web/server (session creation), session-cli-builder (env setup) * * @module hooks-config @@ -29,7 +29,7 @@ import { readFile, writeFile, mkdir } from 'node:fs/promises'; import { join } from 'node:path'; import type { HookEventType } from './types.js'; -import { HOOK_TIMEOUT_MS } from './config/auth-config.js'; +import { HOOK_TIMEOUT_SECONDS } from './config/auth-config.js'; /** * Serializes read-modify-write access to a `settings.local.json` path. Every @@ -40,7 +40,19 @@ import { HOOK_TIMEOUT_MS } from './config/auth-config.js'; * are independent; the map self-prunes when a path's chain goes idle. */ const settingsWriteLocks = new Map>(); -const BACKGROUND_WAKE_MARKER = 'CODEMAN_BACKGROUND_REWAKE_V1'; +/** + * Version-agnostic ownership prefix: every rewake script version embeds a marker + * starting with this, and `isCodemanHookHandler` matches on the prefix. That way a + * version bump replaces the old handler instead of duplicating it (matching on the + * full versioned marker would disown every older script). + */ +const BACKGROUND_WAKE_MARKER_PREFIX = 'CODEMAN_BACKGROUND_REWAKE_V'; +/** + * Current script version. Bump the suffix whenever `generateBackgroundWakeScript` + * changes: `refreshStaleCodemanHooks` treats the absence of the CURRENT marker as + * stale, so healed cases pick up the new script on next launch. + */ +const BACKGROUND_WAKE_MARKER = `${BACKGROUND_WAKE_MARKER_PREFIX}2`; const BACKGROUND_WAKE_TIMEOUT_SECONDS = 6 * 60 * 60; /** @@ -51,11 +63,17 @@ const BACKGROUND_WAKE_TIMEOUT_SECONDS = 6 * 60 * 60; * record avoids injecting terminal input (which could submit a user's draft). * The helper is embedded in settings via `node -e`, so it has no script path * that can go stale after an install or plugin-cache cleanup. + * + * Self-terminating: Claude Code enforces the hook timeout, but the helper does not + * rely on it. It exits on its own deadline (same budget) and when orphaned + * (`ppid === 1`), so a dead session cannot leave a poller stat-ing the transcript + * forever. The ppid check misses subreaper setups; the deadline is the backstop. */ export function generateBackgroundWakeScript(): string { return [ "const fs = require('node:fs');", `const ${BACKGROUND_WAKE_MARKER} = true;`, + `const deadline = Date.now() + ${BACKGROUND_WAKE_TIMEOUT_SECONDS} * 1000;`, 'let input = {};', "try { input = JSON.parse(fs.readFileSync(0, 'utf8') || '{}'); } catch { process.exit(0); }", 'function findTaskId(value) {', @@ -99,6 +117,7 @@ export function generateBackgroundWakeScript(): string { ' }', '}', 'function poll() {', + ' if (Date.now() > deadline || process.ppid === 1) process.exit(0);', ' try {', ' const size = fs.statSync(transcriptPath).size;', " if (size < position) { position = 0; carry = ''; }", @@ -165,30 +184,30 @@ export function generateHooksConfig(): { hooks: Record } { Notification: [ { matcher: 'idle_prompt', - hooks: [{ type: 'command', command: curlCmd('idle_prompt'), timeout: HOOK_TIMEOUT_MS }], + hooks: [{ type: 'command', command: curlCmd('idle_prompt'), timeout: HOOK_TIMEOUT_SECONDS }], }, { matcher: 'permission_prompt', - hooks: [{ type: 'command', command: curlCmd('permission_prompt'), timeout: HOOK_TIMEOUT_MS }], + hooks: [{ type: 'command', command: curlCmd('permission_prompt'), timeout: HOOK_TIMEOUT_SECONDS }], }, { matcher: 'elicitation_dialog', - hooks: [{ type: 'command', command: curlCmd('elicitation_dialog'), timeout: HOOK_TIMEOUT_MS }], + hooks: [{ type: 'command', command: curlCmd('elicitation_dialog'), timeout: HOOK_TIMEOUT_SECONDS }], }, ], Stop: [ { - hooks: [{ type: 'command', command: curlCmd('stop'), timeout: HOOK_TIMEOUT_MS }], + hooks: [{ type: 'command', command: curlCmd('stop'), timeout: HOOK_TIMEOUT_SECONDS }], }, ], TeammateIdle: [ { - hooks: [{ type: 'command', command: curlCmd('teammate_idle'), timeout: HOOK_TIMEOUT_MS }], + hooks: [{ type: 'command', command: curlCmd('teammate_idle'), timeout: HOOK_TIMEOUT_SECONDS }], }, ], TaskCompleted: [ { - hooks: [{ type: 'command', command: curlCmd('task_completed'), timeout: HOOK_TIMEOUT_MS }], + hooks: [{ type: 'command', command: curlCmd('task_completed'), timeout: HOOK_TIMEOUT_SECONDS }], }, ], PostToolUse: [ @@ -212,7 +231,8 @@ export function generateHooksConfig(): { hooks: Record } { function isCodemanHookHandler(value: unknown): boolean { try { const serialized = JSON.stringify(value); - return serialized.includes('/api/hook-event') || serialized.includes(BACKGROUND_WAKE_MARKER); + // Prefix, not the versioned marker: older script versions must still be ours. + return serialized.includes('/api/hook-event') || serialized.includes(BACKGROUND_WAKE_MARKER_PREFIX); } catch { return false; } diff --git a/src/session.ts b/src/session.ts index 460358ac..604d9e9c 100644 --- a/src/session.ts +++ b/src/session.ts @@ -181,7 +181,12 @@ const DEFAULT_PTY_COLS = 120; const DEFAULT_PTY_ROWS = 40; const TMUX_DISPLAY_TIMEOUT_MS = 2000; const IS_TEST_MODE = !!process.env.VITEST; -const TEST_PTY_SCRIPT = 'process.stdin.pipe(process.stdout);'; +/** + * Echo transport for the test-mode PTY attach. Raw mode disables the tty line + * discipline, so each input byte flows back exactly once and immediately; without + * it, tty echo doubles every line and canonical buffering holds bytes until Enter. + */ +const TEST_PTY_SCRIPT = 'if (process.stdin.isTTY) process.stdin.setRawMode(true); process.stdin.pipe(process.stdout);'; /** Delay before the in-container Claude CLI version probe (lets the container start). */ const DOCKER_CLI_VERSION_PROBE_DELAY_MS = 3000; diff --git a/test/hook-secret-selfheal.test.ts b/test/hook-secret-selfheal.test.ts index 892bf32e..9af794be 100644 --- a/test/hook-secret-selfheal.test.ts +++ b/test/hook-secret-selfheal.test.ts @@ -127,7 +127,7 @@ describe('refreshStaleCodemanHooks', () => { const after = JSON.parse(readFileSync(settingsPath, 'utf-8')); expect(JSON.stringify(after.hooks)).toContain(SECRET_HEADER); - expect(JSON.stringify(after.hooks)).toContain('CODEMAN_BACKGROUND_REWAKE_V1'); + expect(JSON.stringify(after.hooks)).toContain('CODEMAN_BACKGROUND_REWAKE_V'); expect(JSON.stringify(after.hooks.Stop)).toContain('./notify-user.sh'); expect(after.hooks.PostToolUse).toEqual(expect.arrayContaining([customPostToolUse])); expect(after.hooks.CustomEvent).toEqual(customEvent); diff --git a/test/hooks-config.test.ts b/test/hooks-config.test.ts index ebb0b63d..e4cbd254 100644 --- a/test/hooks-config.test.ts +++ b/test/hooks-config.test.ts @@ -95,10 +95,10 @@ describe('generateHooksConfig', () => { expect(notifHooks[0].hooks[0].command).toContain('|| true'); }); - it('should set timeout to 10000ms', () => { + it('should set timeout to 10 seconds (hook timeout fields are seconds)', () => { const config = generateHooksConfig(); const notifHooks = config.hooks.Notification as Array<{ hooks: Array<{ timeout: number }> }>; - expect(notifHooks[0].hooks[0].timeout).toBe(10000); + expect(notifHooks[0].hooks[0].timeout).toBe(10); }); it('should include correct event names in curl payloads', () => { @@ -199,7 +199,40 @@ describe('writeHooksConfig', () => { const parsed = JSON.parse(readFileSync(settingsPath, 'utf-8')); expect(parsed.hooks.PostToolUse).toHaveLength(1); - expect(JSON.stringify(parsed.hooks.PostToolUse)).toContain('CODEMAN_BACKGROUND_REWAKE_V1'); + expect(JSON.stringify(parsed.hooks.PostToolUse)).toContain('CODEMAN_BACKGROUND_REWAKE_V'); + }); + + it('should replace an older rewake script version without duplicating it', async () => { + const claudeDir = join(testDir, '.claude'); + const settingsPath = join(claudeDir, 'settings.local.json'); + mkdirSync(claudeDir, { recursive: true }); + // Simulate a case healed by the previous release: current curls (secret present) + // plus a V1 rewake handler. The version bump must swap the handler in place. + const hooks = generateHooksConfig().hooks; + hooks.PostToolUse = [ + { + matcher: 'Bash', + hooks: [ + { + type: 'command', + command: 'node', + args: ['-e', 'const CODEMAN_BACKGROUND_REWAKE_V1 = true; process.exit(0);'], + asyncRewake: true, + timeout: 21600, + }, + ], + }, + ]; + writeFileSync(settingsPath, JSON.stringify({ hooks }, null, 2)); + + await refreshStaleCodemanHooks(testDir); + + const parsed = JSON.parse(readFileSync(settingsPath, 'utf-8')); + const serialized = JSON.stringify(parsed.hooks.PostToolUse); + expect(parsed.hooks.PostToolUse).toHaveLength(1); + expect(parsed.hooks.PostToolUse[0].hooks).toHaveLength(1); + expect(serialized).toContain('CODEMAN_BACKGROUND_REWAKE_V2'); + expect(serialized).not.toContain('CODEMAN_BACKGROUND_REWAKE_V1'); }); it('should not add rewake hooks to a user-owned hook configuration', async () => { @@ -841,7 +874,7 @@ describe('Hook Config Generation - Extended', () => { expect(hook.matcher).toBeDefined(); expect(hook.hooks).toHaveLength(1); expect(hook.hooks[0].type).toBe('command'); - expect(hook.hooks[0].timeout).toBe(10000); + expect(hook.hooks[0].timeout).toBe(10); expect(hook.hooks[0].command).toBeTruthy(); } }); diff --git a/test/setup.ts b/test/setup.ts index 59881fd6..edd11f51 100644 --- a/test/setup.ts +++ b/test/setup.ts @@ -25,7 +25,7 @@ if (originalPlaywrightBrowsersPath === undefined && originalHome) { process.platform === 'darwin' ? join(originalHome, 'Library', 'Caches', 'ms-playwright') : process.platform === 'win32' - ? join(process.env.LOCALAPPDATA || originalHome, 'ms-playwright') + ? join(process.env.LOCALAPPDATA || join(originalHome, 'AppData', 'Local'), 'ms-playwright') : join(originalHome, '.cache', 'ms-playwright'); } process.env.HOME = testHome; @@ -44,7 +44,14 @@ afterEach(() => { vi.useRealTimers(); }); -afterAll(() => { +afterAll(async () => { + // Let in-flight console-log rpc forwards drain before the worker environment + // tears down. On loaded CI runners the channel otherwise closes while the last + // "onUserConsoleLog" call is still pending, and that single unhandled + // EnvironmentTeardownError fails the run after every test has passed + // (observed twice on the PR #175/#176 merge commit; never locally). + await new Promise((resolve) => setTimeout(resolve, 50)); + if (originalHome === undefined) delete process.env.HOME; else process.env.HOME = originalHome; @@ -59,3 +66,10 @@ afterAll(() => { rmSync(testHome, { recursive: true, force: true }); }); + +// afterAll never fires for a fully-skipped test file (no tests execute), which +// would leak the temp home created above. The exit hook is the backstop; rmSync +// with force is a no-op when afterAll already removed it. +process.on('exit', () => { + rmSync(testHome, { recursive: true, force: true }); +}); diff --git a/test/webview-proxy.test.ts b/test/webview-proxy.test.ts index dfca625c..6815f889 100644 --- a/test/webview-proxy.test.ts +++ b/test/webview-proxy.test.ts @@ -487,7 +487,10 @@ describe('runtimeUrlShim', () => { * 404s on Codeman's own root while the dashboard's fetch-driven data loads fine. * * Node environment on purpose, like test/markdown-sanitizer.test.ts: a per-file - * `@vitest-environment jsdom` externalizes node builtins under vite. + * jsdom environment directive would externalize node builtins under vite. The + * directive is deliberately not written out here, even in prose: vitest scans + * comments for it, and naming it flipped this whole file to the jsdom + * environment while this comment claimed the opposite. */ describe('runtimeUrlShim DOM sinks', () => { const body = runtimeUrlShim(PREFIX)