From 6c3d0ec53648e25a2d5778080550abad87401b9b Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 6 Nov 2026 17:26:49 -0800 Subject: [PATCH] fix: the CEO review gate waits for Claude Code's transcript flush (GST-1701) (#4102) * test: failing-first cases for the CEO review gate reading a transcript before Claude Code flushes it (GST-1701) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: the CEO review gate waits for the transcript flush through #4100's shared settle helper (GST-1701) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/capy-gst-1701-ceo-gate-settle.md | 1 + docs/skill-gates.md | 2 ++ hosts/claude/hooks/autoplan-stop-gate.ts | 33 +++----------------- hosts/claude/hooks/ceo-review-gate.ts | 16 +++------- lib/skill-gates.ts | 29 +++++++++++++++++ test/ceo-review-gate.test.ts | 26 +++++++++++++++ 6 files changed, 67 insertions(+), 40 deletions(-) create mode 100644 changelog.d/capy-gst-1701-ceo-gate-settle.md diff --git a/changelog.d/capy-gst-1701-ceo-gate-settle.md b/changelog.d/capy-gst-1701-ceo-gate-settle.md new file mode 100644 index 000000000..c360b495c --- /dev/null +++ b/changelog.d/capy-gst-1701-ceo-gate-settle.md @@ -0,0 +1 @@ +**The /plan-ceo-review gate no longer judges a transcript Claude Code hasn't finished writing (GST-1701).** Claude Code can fire Stop about 200 ms before it flushes the turn's last transcript lines. The CEO review gate read the transcript once, so a finished review written outside the repository (a plan under `~/.claude/plans`) could be held because its Write wasn't on disk yet, and a run whose load hadn't landed could stop without being checked. The gate now waits, like the /autoplan gate, until the transcript holds the stop's final message (at most 2 seconds or half its deadline). Both gates share one helper, `settledTranscript` in `lib/skill-gates.ts`. diff --git a/docs/skill-gates.md b/docs/skill-gates.md index 8c49336f6..b32d3aeb9 100644 --- a/docs/skill-gates.md +++ b/docs/skill-gates.md @@ -69,6 +69,8 @@ Once a session loads /plan-ceo-review (the Skill tool, a typed `/plan-ceo-review Bash commands are never parsed: a review written with a heredoc or `tee` is found by the repository and ceo-plans scans. +Like the /autoplan gate, the hook first re-reads the transcript until it holds the stop's `last_assistant_message`, for at most 2 seconds or half its deadline (`settledTranscript` in `lib/skill-gates.ts`). Claude Code can fire Stop before it writes the turn's last lines, so a single read could miss the Write of a review kept outside the repository (a plan under `~/.claude/plans`) and hold a finished run, or miss the load itself and let an unfinished one through. + **The block.** Its reason starts with the `blocked via=stop` marker and says exactly what to write and where: a `## Findings` section ranking the top risks as a numbered list, in `GSTACK_CEO_REVIEW_OUTPUT`, else the last markdown file the run wrote, else the last plan file it read (never a file in a `skills/` directory, judged from the repository root so a project that itself sits under a `skills/` folder still counts, such as the review's own methodology sections; such a file never releases the gate either), plus each file checked and why it fell short. It never names the off switch. **The cap.** A run is blocked at most 3 times. The next final stop without ranked risks goes through with an `exhausted` marker and `exhausted_at` in the record. `stop_hook_active` (Claude Code sets it once a Stop hook has blocked the turn) does not skip the check; the cap ends the repeats. After `released` or `exhausted` the gate does nothing for the rest of that run; a new load starts a new run. diff --git a/hosts/claude/hooks/autoplan-stop-gate.ts b/hosts/claude/hooks/autoplan-stop-gate.ts index 831a9298f..0fdd4d99a 100755 --- a/hosts/claude/hooks/autoplan-stop-gate.ts +++ b/hosts/claude/hooks/autoplan-stop-gate.ts @@ -5,7 +5,7 @@ * SWITCH_DEFAULT). Registered through one shim: autoplan-stop-gate-stop (Stop). * * Stop: payload transcript_path (+ last_assistant_message), re-read until it - * holds that final message (at most 2 s; Stop can beat the CLI's flush) + * holds that final message (lib/skill-gates.ts settledTranscript) * -> the last /autoplan load (none: nothing) * -> a final turn? (lib/skill-contract.ts isFinalTurn; a question to a * human in an interactive session is never blocked) @@ -25,45 +25,20 @@ */ import { sessionKind } from './spawn-bin'; import { SKILL, checkRun, missingStep } from '../../../lib/autoplan-stop-gate'; -import { StateError, UnknownShape, isFinalTurn, parseTranscript, readTranscriptFile, sameSkill, skillLoads, turnEnd, withFinalMessage } from '../../../lib/skill-contract'; +import { isFinalTurn, sameSkill, skillLoads, turnEnd } from '../../../lib/skill-contract'; import { readSkillSwitch } from '../../../lib/skill-route'; import { - GateError, blockWith, hookRecord, note, pruneSessionDirs, readGateRecord, recordPath, remainingMs, runGateHook, sessionDir, systemMessage, + blockWith, hookRecord, note, pruneSessionDirs, readGateRecord, recordPath, remainingMs, runGateHook, sessionDir, settledTranscript, systemMessage, withSessionLock, writeGateRecord, type GatePayload, type HookOutput, } from '../../../lib/skill-gates'; const HOOK = 'autoplan-gate'; const MAX_BLOCKS = 3; const SESSION_KIND_TIMEOUT_MS = 1000; -const SETTLE_MS = 2000; -const SETTLE_POLL_MS = 50; - -/** - * The transcript once it holds the turn's final message. Claude Code can run - * Stop before it flushes the turn's last lines (the smoke run saw the scope - * call and a phase publication land about 200 ms after Stop fired), so a - * stale read would block a finished run. Re-read until the final text is - * there, for at most SETTLE_MS or half the deadline, then judge what exists. - */ -function settledTranscript(p: GatePayload) { - const until = Date.now() + Math.min(SETTLE_MS, remainingMs() / 2); - for (;;) { - const t = parseTranscript(readTranscriptFile(p.transcriptPath)); - const settled = withFinalMessage(t, p.raw.last_assistant_message); - if (settled === t || Date.now() >= until) return settled; - Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, SETTLE_POLL_MS); - } -} function handle(p: GatePayload): HookOutput | undefined { if (p.event && p.event !== 'Stop') return undefined; - let t; - try { - t = settledTranscript(p); - } catch (e) { - if (e instanceof StateError || e instanceof UnknownShape) throw new GateError('GATE_TRANSCRIPT', e.message); - throw e; - } + const { t } = settledTranscript(p.transcriptPath, p.raw.last_assistant_message); const load = skillLoads(t).filter(l => sameSkill(l.skill, SKILL)).pop(); if (!load) return undefined; if (!isFinalTurn(turnEnd(t), () => sessionKind(Math.min(SESSION_KIND_TIMEOUT_MS, remainingMs())))) return undefined; diff --git a/hosts/claude/hooks/ceo-review-gate.ts b/hosts/claude/hooks/ceo-review-gate.ts index 4f3850823..1e7a604df 100755 --- a/hosts/claude/hooks/ceo-review-gate.ts +++ b/hosts/claude/hooks/ceo-review-gate.ts @@ -8,6 +8,8 @@ * Stop: payload transcript_path (+ last_assistant_message) * SubagentStop: payload agent_transcript_path, sidechain records included, * so only a subagent that itself loaded /plan-ceo-review is gated + * -> re-read until it holds that final message (lib/skill-gates.ts + * settledTranscript; Stop can beat the CLI's flush) * -> the last /plan-ceo-review load (none: nothing) * -> Stop only: a final turn? (lib/skill-contract.ts isFinalTurn; a * question to a human in an interactive session is never blocked) @@ -26,10 +28,10 @@ import * as path from 'path'; import { runBin, sessionKind } from './spawn-bin'; import { SKILL, checkOutput } from '../../../lib/ceo-review-gate'; -import { StateError, UnknownShape, isFinalTurn, parseTranscript, readTranscriptFile, sameSkill, skillLoads, turnEnd, withFinalMessage } from '../../../lib/skill-contract'; +import { isFinalTurn, sameSkill, skillLoads, turnEnd } from '../../../lib/skill-contract'; import { readSkillSwitch } from '../../../lib/skill-route'; import { - GateError, blockWith, hookRecord, note, pruneSessionDirs, readGateRecord, recordPath, remainingMs, runGateHook, sessionDir, systemMessage, + GateError, blockWith, hookRecord, note, pruneSessionDirs, readGateRecord, recordPath, remainingMs, runGateHook, sessionDir, settledTranscript, systemMessage, withSessionLock, writeGateRecord, type GatePayload, type HookOutput, } from '../../../lib/skill-gates'; @@ -49,15 +51,7 @@ function handle(p: GatePayload): HookOutput | undefined { const sub = p.event === 'SubagentStop'; const file = sub ? p.raw.agent_transcript_path : p.transcriptPath; if (typeof file !== 'string' || !file) return undefined; - let raw: string; - let t; - try { - raw = readTranscriptFile(file); - t = withFinalMessage(parseTranscript(raw, { includeSidechain: sub }), p.raw.last_assistant_message); - } catch (e) { - if (e instanceof StateError || e instanceof UnknownShape) throw new GateError('GATE_TRANSCRIPT', e.message); - throw e; - } + const { raw, t } = settledTranscript(file, p.raw.last_assistant_message, { includeSidechain: sub }); const load = skillLoads(t).filter(l => sameSkill(l.skill, SKILL)).pop(); if (!load) return undefined; if (!sub && !isFinalTurn(turnEnd(t), () => sessionKind(Math.min(SESSION_KIND_TIMEOUT_MS, remainingMs())))) return undefined; diff --git a/lib/skill-gates.ts b/lib/skill-gates.ts index 35b491bf3..5736c7d60 100644 --- a/lib/skill-gates.ts +++ b/lib/skill-gates.ts @@ -16,6 +16,7 @@ import * as fs from 'fs'; import * as path from 'path'; import { atomicWriteSync } from './fs-atomic'; import { resolveStateRoot } from './state-root'; +import { StateError, UnknownShape, parseTranscript, readTranscriptFile, withFinalMessage, type Transcript } from './skill-contract'; import { appendHookTrace, readStdinBounded, type TraceHook } from './skill-route'; export type GateHook = 'investigate-gate' | 'ceo-review-gate' | 'autoplan-gate'; @@ -59,6 +60,34 @@ export class GateError extends Error { constructor(readonly code: string, why: string) { super(why); } } +const SETTLE_MS = 2000; +const SETTLE_POLL_MS = 50; + +/** + * A stop's transcript once it holds the payload's final message (raw text and + * parsed). Claude Code can run Stop before it flushes the turn's last lines (a + * smoke run saw a scope call and a phase publication land about 200 ms after + * Stop fired), so a stale read can hold a finished run or miss a run's load. + * Re-read until the final text is there, for at most SETTLE_MS or half the + * deadline, then judge what exists. A transcript the parser cannot read is + * GATE_TRANSCRIPT. + */ +export function settledTranscript(file: string, finalMessage: unknown, opts: { includeSidechain?: boolean } = {}): { raw: string; t: Transcript } { + const until = Date.now() + Math.min(SETTLE_MS, remainingMs() / 2); + try { + for (;;) { + const raw = readTranscriptFile(file); + const t = parseTranscript(raw, opts); + const settled = withFinalMessage(t, finalMessage); + if (settled === t || Date.now() >= until) return { raw, t: settled }; + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, SETTLE_POLL_MS); + } + } catch (e) { + if (e instanceof StateError || e instanceof UnknownShape) throw new GateError('GATE_TRANSCRIPT', e.message); + throw e; + } +} + export const cleanSessionId = (sid: string) => sid.replace(/[^A-Za-z0-9_-]/g, '').slice(0, 64) || 'unknown'; const markerValue = (v: string | number) => String(v).replace(/\s+/g, '_').slice(0, 240); diff --git a/test/ceo-review-gate.test.ts b/test/ceo-review-gate.test.ts index 711810dc7..f0df84607 100644 --- a/test/ceo-review-gate.test.ts +++ b/test/ceo-review-gate.test.ts @@ -12,6 +12,7 @@ import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as path from 'node:path'; +import { bunIsolationArgs } from '../lib/bun-isolation'; import { rankedRisksGap } from '../lib/ceo-review-gate'; import { ROOT, hookEnv, result, runHook, say, skillsDir, tmpDir, tool, user } from './helpers/skill-contract-fixture'; @@ -219,6 +220,31 @@ describe('final turns only', () => { expect(stop([...LOADED], { last_assistant_message: 'Which mode do you want: expansion or hold?' }).stdout).toBe(''); }); + test('a Stop that fires before the CLI flushes the turn waits for the final message: a review written outside the repo is released, not held', async () => { + const plan = put(path.join(work, 'plans', 'checkout.md'), RANKED); + const file = transcript(LOADED); + const payload = { session_id: 'sess-1', transcript_path: file, cwd: repo, hook_event_name: 'Stop', stop_hook_active: false, last_assistant_message: 'Review complete.' }; + const proc = Bun.spawn([process.execPath, ...bunIsolationArgs(), HOOK], { stdin: new Blob([JSON.stringify(payload)]), env: baseEnv(), stdout: 'pipe', timeout: 20_000 }); + await Bun.sleep(300); + fs.appendFileSync(file, `${[...wrote(plan), DONE].join('\n')}\n`); + const out = json(await new Response(proc.stdout).text()); + expect(await proc.exited).toBe(0); + expect(out.decision).toBeUndefined(); + expect(out.systemMessage).toMatch(/^GSTACK_GATE ceo-review-gate released session=sess-1 skill=plan-ceo-review file=checkout.md blocked=0$/m); + }); + + test('a Stop that fires before the CLI flushes the load itself still judges the run once the final message lands', async () => { + const file = transcript([user('review my plan')]); + const payload = { session_id: 'sess-1', transcript_path: file, cwd: repo, hook_event_name: 'Stop', stop_hook_active: false, last_assistant_message: 'Review complete.' }; + const proc = Bun.spawn([process.execPath, ...bunIsolationArgs(), HOOK], { stdin: new Blob([JSON.stringify(payload)]), env: baseEnv(), stdout: 'pipe', timeout: 20_000 }); + await Bun.sleep(300); + fs.appendFileSync(file, `${[...LOADED.slice(1), DONE].join('\n')}\n`); + const out = json(await new Response(proc.stdout).text()); + expect(await proc.exited).toBe(0); + expect(out?.decision).toBe('block'); + expect(out.reason.split('\n')[0]).toBe('GSTACK_GATE ceo-review-gate blocked session=sess-1 skill=plan-ceo-review via=stop files=none n=1'); + }); + test('no /plan-ceo-review load, or the switch off: no output and no log', () => { expect(stop([user('hi'), DONE]).stdout).toBe(''); expect(stop([...LOADED, DONE], {}, { GSTACK_CEO_REVIEW_GATE: 'off' }).stdout).toBe('');