diff --git a/changelog.d/gsta208-stop-hook-stdin.md b/changelog.d/gsta208-stop-hook-stdin.md new file mode 100644 index 000000000..67adda05b --- /dev/null +++ b/changelog.d/gsta208-stop-hook-stdin.md @@ -0,0 +1,7 @@ +**The timeline Stop hook returns within its 2-second budget even when the host keeps stdin open.** The hook read its payload with a blocking read that waits for end of input, so a host that writes the Stop payload and leaves the pipe open held the hook for as long as the pipe stayed open. A local run with the pipe held for 8 seconds took 8 seconds, past the hook's 2-second budget and on the way to Claude Code's 15-second hook window (#3628, reported on Windows). Rewritten from PR #3628 by shahab (mashtii85). + +### Fixed +- **`hosts/claude/hooks/timeline-stop-hook.ts`** reads stdin for at most 500 ms and stops at the first complete JSON payload, then repairs the timeline as before. The hook still exits 0 on every path. + +### Tests +- `test/timeline-stop-hook-stdin.test.ts` (runs on windows-latest): a complete payload on a pipe that never closes returns within budget and still writes the repair; an open pipe with no data returns within budget. Both failed before the fix (10 s, killed). diff --git a/hosts/claude/hooks/timeline-stop-hook.ts b/hosts/claude/hooks/timeline-stop-hook.ts index a71255200..d5b2d42e4 100755 --- a/hosts/claude/hooks/timeline-stop-hook.ts +++ b/hosts/claude/hooks/timeline-stop-hook.ts @@ -59,6 +59,8 @@ import { readVersionedSlugCache } from '../../../lib/bin-context'; import { logHookError as sharedLogHookError } from './hook-log'; const DEADLINE_MS = 2000; +/** A host may keep stdin open after its payload (#3628); never wait for EOF past this. */ +const STDIN_DEADLINE_MS = 500; const REPAIR_SLUG_TIMEOUT_MS = 30_000; const LOCK_TTL_MS = 60_000; const MISS_TTL_MS = 5 * 60_000; @@ -161,9 +163,35 @@ function slugOverride(): string | undefined { return slug && slug !== '.' && slug !== '..' ? slug : undefined; } -function readStopCwd(): string { +/** The Stop payload: returns at EOF, at the first complete JSON object, or at the deadline. */ +function readStdinBounded(ms: number): Promise { + return new Promise((resolve) => { + let buf = ''; + const done = () => { + clearTimeout(timer); + process.stdin.removeAllListeners('data'); + process.stdin.destroy(); + resolve(buf); + }; + const timer = setTimeout(done, ms); + process.stdin.setEncoding('utf8'); + process.stdin.on('data', (chunk: string) => { + buf += chunk; + try { + JSON.parse(buf); + done(); + } catch { + // incomplete payload: keep reading until EOF or the deadline + } + }); + process.stdin.on('end', done); + process.stdin.on('error', done); + }); +} + +async function readStopCwd(): Promise { try { - const stdin = fs.readFileSync(0, 'utf8'); + const stdin = await readStdinBounded(STDIN_DEADLINE_MS); if (stdin.trim()) { const payload = JSON.parse(stdin) as { cwd?: string }; if (payload.cwd && fs.existsSync(payload.cwd)) return payload.cwd; @@ -209,12 +237,12 @@ function backgroundRepair(cwd: string): void { }); } -function main(): void { +async function main(): Promise { if (process.argv[2] === '--repair' && process.argv[3]) { backgroundRepair(process.argv[3]); return; } - const cwd = readStopCwd(); + const cwd = await readStopCwd(); const slug = slugOverride() ?? readVersionedSlugCache(resolveStateRoot(), cwd); if (!slug) { spawnRepair(cwd); @@ -318,9 +346,6 @@ function repairTimeline(slug: string, deadline: number): void { } } -try { - main(); -} catch (err) { - logHookError(`unexpected: ${err instanceof Error ? (err.stack ?? err.message) : String(err)}`); -} -process.exit(0); +main() + .catch((err) => logHookError(`unexpected: ${err instanceof Error ? (err.stack ?? err.message) : String(err)}`)) + .finally(() => process.exit(0)); diff --git a/scripts/lib/windows-curation.ts b/scripts/lib/windows-curation.ts index b1d0fd291..c2e86a868 100644 --- a/scripts/lib/windows-curation.ts +++ b/scripts/lib/windows-curation.ts @@ -341,6 +341,7 @@ export const KNOWN_WINDOWS_SAFE: Array<{ file: string; reason: string }> = [ { file: 'test/setup-hook-syntax-gate.test.ts', reason: '#3088/#3100: runs bin/gstack-hook-check and the setup hook sections through explicit bash with forward-slash paths; never executes a shebang. Its Bun drive-path spelling is the Windows bug it guards' }, { file: 'test/bun-config-shell-bins.test.ts', reason: '#3415: runs the record bins through lib/script-invocation.ts (Git Bash on Windows) with MSYS path conversion off, the case it guards; its bun and cygpath stand-ins and simulated-OSTYPE cases are skipIf win32' }, { file: 'test/path-spelling.test.ts', reason: 'sources bin/gstack-path-spelling.sh through explicit bash and checks lib/path-spelling.ts; on Windows it exercises the real Git Bash spellings' }, + { file: 'test/timeline-stop-hook-stdin.test.ts', reason: '#3628: the hook runs through Bun argv with a stdin pipe held open; the return-within-budget timing must meet real Windows process startup' }, { file: 'test/state-root-parity.test.ts', reason: 'runs the bash twin and lib/state-root.ts over an env table with PATH empty; no shebang execution, raw-string comparison is platform-neutral' }, { file: 'test/generator-eexist.test.ts', reason: 'E4: runs the generators through Bun argv with the Windows EEXIST emulation preload; no shebang execution' }, diff --git a/test/timeline-stop-hook-stdin.test.ts b/test/timeline-stop-hook-stdin.test.ts new file mode 100644 index 000000000..00db0e523 --- /dev/null +++ b/test/timeline-stop-hook-stdin.test.ts @@ -0,0 +1,65 @@ +/** + * #3628: a host that keeps the Stop hook's stdin open after writing its payload + * held the hook for as long as the pipe stayed open (fs.readFileSync(0) waits + * for EOF), well past the hook's 2 s budget and toward the host's 15 s RPC + * window. The hook must return within its budget and still repair the timeline. + * + * Windows-safe by construction: the hook runs through Bun argv + * (process.execPath), state lives under os.tmpdir(). + */ +import { afterEach, beforeEach, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { bunIsolationArgs } from '../lib/bun-isolation'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const HOOK_TS = path.join(ROOT, 'hosts', 'claude', 'hooks', 'timeline-stop-hook.ts'); +const SLUG = 'stop-hook-stdin'; +const BUDGET_MS = 2_000; +const STARTUP_ALLOWANCE_MS = process.platform === 'win32' ? 3_000 : 1_500; + +let home = '', project = '', timeline = ''; + +beforeEach(() => { + home = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-stop-stdin-home-'))); + project = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-stop-stdin-proj-'))); + fs.mkdirSync(path.join(home, 'projects', SLUG), { recursive: true }); + timeline = path.join(home, 'projects', SLUG, 'timeline.jsonl'); + fs.writeFileSync(timeline, JSON.stringify({ skill: 'review', event: 'started', session: '1-1' }) + '\n'); +}); + +afterEach(() => { + fs.rmSync(home, { recursive: true, force: true }); + fs.rmSync(project, { recursive: true, force: true }); +}); + +async function runHookHoldingStdin(payload: string | null): Promise<{ code: number | null; ms: number }> { + const started = performance.now(); + const child = Bun.spawn([process.execPath, ...bunIsolationArgs(), HOOK_TS], { + stdin: 'pipe', stdout: 'ignore', stderr: 'ignore', + env: { ...process.env, GSTACK_HOME: home, GSTACK_STATE_ROOT: home, GSTACK_PROJECT_SLUG: SLUG }, + }); + if (payload !== null) { child.stdin.write(payload); child.stdin.flush(); } + const kill = setTimeout(() => child.kill(), 10_000); + const code = await child.exited; + clearTimeout(kill); + const ms = performance.now() - started; + try { child.stdin.end(); } catch { /* already closed */ } + return { code, ms }; +} + +const repairs = () => fs.readFileSync(timeline, 'utf8').split('\n').filter(Boolean).map(l => JSON.parse(l)).filter(e => e.source === 'stop-hook'); + +test('a complete payload on a pipe that never closes: returns within budget and repairs (#3628)', async () => { + const r = await runHookHoldingStdin(JSON.stringify({ hook_event_name: 'Stop', cwd: project })); + expect(r.code).toBe(0); + expect(r.ms).toBeLessThan(BUDGET_MS + STARTUP_ALLOWANCE_MS); + expect(repairs()).toHaveLength(1); +}, 20_000); + +test('an open pipe with no data: returns within budget (#3628)', async () => { + const r = await runHookHoldingStdin(null); + expect(r.code).toBe(0); + expect(r.ms).toBeLessThan(BUDGET_MS + STARTUP_ALLOWANCE_MS); +}, 20_000);