mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
fix(hooks,test): harden background rewake, fix hook timeout units, stabilize 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 commit is contained in:
@@ -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).
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
+30
-10
@@ -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<string, Promise<unknown>>();
|
||||
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<string, unknown[]> } {
|
||||
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<string, unknown[]> } {
|
||||
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;
|
||||
}
|
||||
|
||||
+6
-1
@@ -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;
|
||||
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
});
|
||||
|
||||
+16
-2
@@ -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 });
|
||||
});
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user