From 796d8cd4a5efaefc93e25376a1c9dde77e51d313 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 31 Oct 2026 18:14:13 +0000 Subject: [PATCH] fix(hooks): the timeline Stop hook bounds its stdin read (#3628) A host that writes the Stop payload and keeps stdin open held the hook for as long as the pipe stayed open. Read for at most 500 ms, stop at the first complete JSON payload, and await the repair before exiting 0. Reported in #3628 (shahab, mashtii85); the read is written fresh on main. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gsta208-stop-hook-stdin.md | 7 +++ hosts/claude/hooks/timeline-stop-hook.ts | 45 ++++++++++++---- scripts/lib/windows-curation.ts | 1 + test/timeline-stop-hook-stdin.test.ts | 65 ++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 10 deletions(-) create mode 100644 changelog.d/gsta208-stop-hook-stdin.md create mode 100644 test/timeline-stop-hook-stdin.test.ts 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);