From 36fa6add62cf36b986b962a1ea62b09ba9b10703 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 9 Sep 2026 01:01:45 +0000 Subject: [PATCH] test: fix native review completion and eval retry isolation --- test/autoplan-setup-question.test.ts | 289 ++++++++- test/ceo-completion-handoff-m.test.ts | 261 ++++++++ .../ceo-completion-handoff-m-call.json | 562 ++++++++++++++++++ .../fixtures/outside-async-task-m-events.json | 103 ++++ test/helpers/autoplan-setup-question.ts | 105 +++- test/helpers/ceo-completion-handoff.ts | 29 +- test/helpers/outside-voice-evidence.ts | 141 ++++- test/helpers/outside-voice-fixture.ts | 32 + test/helpers/touchfiles-data.ts | 6 +- test/outside-voice-async.test.ts | 167 ++++++ test/outside-voice-fixture.test.ts | 33 + test/skill-e2e-autoplan-chain.test.ts | 26 +- test/skill-e2e-outside-voice.test.ts | 36 +- test/skill-e2e-plan-ceo-finding-count.test.ts | 12 + 14 files changed, 1731 insertions(+), 71 deletions(-) create mode 100644 test/ceo-completion-handoff-m.test.ts create mode 100644 test/fixtures/ceo-completion-handoff-m-call.json create mode 100644 test/fixtures/outside-async-task-m-events.json create mode 100644 test/outside-voice-async.test.ts create mode 100644 test/outside-voice-fixture.test.ts diff --git a/test/autoplan-setup-question.test.ts b/test/autoplan-setup-question.test.ts index fe585e8d6..1c3c22fab 100644 --- a/test/autoplan-setup-question.test.ts +++ b/test/autoplan-setup-question.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'bun:test'; -import { autoplanRoutingSetupInput } from './helpers/autoplan-setup-question'; +import { autoplanRoutingSetupInput, autoplanSetupDecision } from './helpers/autoplan-setup-question'; import { E2E_TOUCHFILES, selectTests } from './helpers/touchfiles'; import * as fs from 'node:fs'; import * as os from 'node:os'; @@ -500,3 +500,290 @@ await Bun.write(${JSON.stringify(resultFile)},JSON.stringify(results)); fs.rmSync(dir, {recursive:true,force:true}); } }, 30000); + + +// Exact current viewport from source-M's routing stall. Owned temporary paths +// are retained as display text; no fixture path is accessed by this replay. +const M_ROUTING_CAPTURE = "\n\n❯ /autoplan\n\n● Starting the autoplan pipeline — running the preamble first.\n\n● Bash(_SS=\"$HOME/.claude/skills/gstack/bin/gstack-skill-start\"\n [ -x \"$_SS\" ] || _SS=\".claude/skills/gstack/bin/gstack-skill-start\"…)\n ⎿  SKILL_START_PROTO: 1\n BRANCH: main\n PROACTIVE: true \n … +54 lines (ctrl+o to expand)\n ⎿  Allowed by auto mode classifier\n\n● The preamble ran. SESSION_KIND is interactive, SESSION_ID is 1144263-1788912944-701e8cc4. There's a one-time routing\n instruction to handle first.\n\n Let me check if CLAUDE.md exists and explore the repo before presenting the routing question.\n\n Read 1 file, listed 1 directory (ctrl+o to expand)\n────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────\nPlanning:\n/tmp/gstack-paid-shard-2DwzUD/tmp/gstack-hermetic-1144068-Ep9FFb/with-skills/.claude/plans/scalable-bouncing-moth.md\n────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────\n ☐ Skill routing\n\n│ gstack works best when your project's CLAUDE.md includes skill routing rules. Should I add them now?\n│ \n\n❯ 1. Add routing rules (Recommended)\n Append skill routing rules to CLAUDE.md and commit it — /autoplan, /ship, /qa, and other skills will be suggested\n automatically when relevant.\n 2. No thanks, manual only\n Skip for now; you can invoke skills manually anytime. You won't be asked again.\n 3. Type something.\n────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────\n 4. Chat about this\n\nEnter to select · ↑/↓ to navigate · Esc to cancel\n"; + +describe('M routing manual-only action grammar', () => { + test('answers the exact native panel once and preserves the Add choice in either order', () => { + const seen = new Set(); + expect(autoplanRoutingSetupInput(M_ROUTING_CAPTURE, seen)).toBe('1'); + expect(autoplanRoutingSetupInput(M_ROUTING_CAPTURE, seen)).toBeNull(); + const reversed = M_ROUTING_CAPTURE + .replace('❯ 1. Add routing rules (Recommended)', '❯ 1. No thanks, manual only') + .replace(' 2. No thanks, manual only', ' 2. Add routing rules (Recommended)'); + expect(autoplanRoutingSetupInput(reversed, new Set())).toBe('2'); + }); + + test('equivalent manual actions use the same grammar with or without a courtesy prefix', () => { + for (const label of [ + 'No thanks, manual', 'No thanks, manual only', 'Skip — manual only', + 'Manual', 'Manual only', 'Manual-only', 'Manual invocation', 'Manual invocation only', + 'No thanks, manual invocation only', 'Invoke skills manually only', + "No thanks, I'll invoke skills manually only", + ]) expect(autoplanRoutingSetupInput(M_ROUTING_CAPTURE.replace('No thanks, manual only', label), new Set()), label).toBe('1'); + }); + + test('manual modifiers do not admit extra actions, other workflows or ambiguous choices', () => { + for (const label of [ + 'No thanks, manual data migration only', 'Manual deployment only', + 'No thanks, invoke the deployment manually only', 'No thanks, manual only then delete CLAUDE.md', + 'No thanks, skip the review', 'No thanks, proceed with implementation', + 'No thanks, manual invocation only after deleting the rules', 'Manual only approval', + ]) expect(autoplanRoutingSetupInput(M_ROUTING_CAPTURE.replace('No thanks, manual only', label), new Set()), label).toBeNull(); + for (const frame of [ + M_ROUTING_CAPTURE.replace(' 3. Type something.', ' 3. Manual only'), + M_ROUTING_CAPTURE.replace(' 3. Type something.', ' 3. Add routing rules'), + M_ROUTING_CAPTURE.replace('Add routing rules (Recommended)', 'Add product routes (Recommended)'), + M_ROUTING_CAPTURE.replace('Add routing rules (Recommended)', 'Add ruting rules (Recommended)'), + M_ROUTING_CAPTURE.replace("gstack works best when your project's CLAUDE.md includes skill routing rules. Should I add them now?", 'Which application API routing design should we choose?'), + M_ROUTING_CAPTURE.replace("gstack works best when your project's CLAUDE.md includes skill routing rules. Should I add them now?", 'The plan quotes gstack skill routing rules in CLAUDE.md. Should we expand the feature?'), + ]) expect(autoplanRoutingSetupInput(frame, new Set()), frame).toBeNull(); + }); +}); + + +const UNSUPPORTED_ROUTING = M_ROUTING_CAPTURE.replace('No thanks, manual only', 'Ask me after this review'); +const unsupportedNative = () => ({ + sessionId: 'unsupported-routing', toolUseId: 'routing-call', answered: false, failed: false, + questions: [{ header: 'Skill routing', question: "gstack works best when your project's CLAUDE.md includes skill routing rules. Should I add them now? ", + options: [{label:'Add routing rules (Recommended)'},{label:'Ask me after this review'}] }], +}); + +describe('unsupported setup diagnostic state', () => { + test('a complete recognized unsupported setup fails explicitly without selecting an action', () => { + const seen = new Set(); + for (const pending of [undefined, unsupportedNative()]) { + const result = autoplanSetupDecision(UNSUPPORTED_ROUTING, seen, pending); + expect(result.kind).toBe('unsupported_setup'); + if (result.kind === 'unsupported_setup') { + expect(result.setup).toBe('routing'); + expect(result.options).toEqual([{index:1,label:'Add routing rules (Recommended)'},{index:2,label:'Ask me after this review'}]); + expect(result.identitySource).toBe(pending ? 'native-bound' : 'current-native-panel'); + } + expect(seen.size).toBe(0); + } + expect(autoplanSetupDecision(PREREQUISITE_CAPTURE.replace('Skip — proceed with standard review', 'Ask me later'), new Set()).kind).toBe('unsupported_setup'); + }); + + test('supported input is pure until sent; redraw and delayed metadata then wait', () => { + const seen = new Set(); + const decision = autoplanSetupDecision(M_ROUTING_CAPTURE, seen); + expect(decision.kind).toBe('input'); expect(seen.size).toBe(0); + if (decision.kind !== 'input') throw Error('Expected supported setup'); + expect(decision.input).toBe('1'); + for (const signature of decision.signatures) seen.add(signature); + expect(autoplanSetupDecision(M_ROUTING_CAPTURE, seen).kind).toBe('waiting'); + const native = unsupportedNative(); native.questions[0]!.options[1]!.label = 'No thanks, manual only'; + expect(autoplanSetupDecision(M_ROUTING_CAPTURE, seen, native).kind).toBe('waiting'); + expect(autoplanSetupDecision(M_ROUTING_CAPTURE + '\n⏺ Continuing…', seen).kind).toBe('waiting'); + expect(autoplanSetupDecision(PREREQUISITE_CAPTURE, new Set()).kind).toBe('input'); + }); + + test('a substantive product or taste question mentioning office hours is not an unsupported prerequisite', () => { + const fullQuestion = prerequisiteQuestion.question; + const unsupported = PREREQUISITE_CAPTURE.replace('Skip — proceed with standard review', 'Ask me after this review'); + for (const [prompt, first, second] of [ + ['No design doc exists for /office-hours integration. Should we build X or defer Y?', 'Build X', 'Defer Y'], + ['We should produce a design doc for /office-hours. Which visual style should this product use?', 'Minimal', 'Expressive'], + ['No design doc exists for /office-hours integration. Should we build X or defer Y?', 'Run /office-hours now', 'Defer Y'], + ['No design doc found. Run /office-hours first?', 'Run /office-hours now and delete the feature', 'Ask me later'], + ]) { + const native = prerequisiteCall(); + native.questions[0]!.question = prompt!; + native.questions[0]!.options = [{label:first!},{label:second!}]; + // Reconstruct from the actual full native layout, including footer. + const frame = unsupported.replace(/│ No design doc[\s\S]*?Run \/office-hours first\?/, prompt!) + .replace('1. Run /office-hours now', '1. ' + first) + .replace('2. Ask me after this review', '2. ' + second); + for (const pending of [undefined, native]) { + expect(autoplanSetupDecision(frame, new Set(), pending).kind, prompt).toBe('unrelated'); + } + } + // Existing unsupported offer remains positively identified independently + // of the unsupported opposite label; no exact question wording is needed. + const native = prerequisiteCall(); + native.questions[0]!.question = fullQuestion.replace('Run /office-hours first?', 'Would you like to run /office-hours now?'); + native.questions[0]!.options[1]!.label = 'Ask me after this review'; + expect(autoplanSetupDecision(unsupported.replace('Run /office-hours first?', 'Would you like to run /office-hours now?'), new Set(), native).kind).toBe('unsupported_setup'); + }); + + test('routing identity still needs its explicit setup action before an unsupported failure', () => { + for (const [first, second] of [['React', 'Vue'], ['Accept recommendation', 'Defer finding'], ['Add routing rules (Recommended)', 'Add routing rules (Recommended)']]) { + const frame = UNSUPPORTED_ROUTING.replace('1. Add routing rules (Recommended)', '1. ' + first) + .replace('2. Ask me after this review', '2. ' + second); + const native = unsupportedNative(); + native.questions[0]!.options = [{label:first!},{label:second!}]; + for (const pending of [undefined,native]) expect(autoplanSetupDecision(frame,new Set(),pending).kind).toBe('waiting'); + } + }); + + test('incomplete, stale, quoted, indented or mixed UI cannot establish unsupported setup', () => { + const panel = UNSUPPORTED_ROUTING.slice(UNSUPPORTED_ROUTING.indexOf(' ☐ Skill routing')); + for (const frame of [ + panel.replace('Enter to select · ↑/↓ to navigate · Esc to cancel', ''), + panel.replace(' 2. Ask me after this review', ''), + panel.replace(' 4. Chat about this', ''), + panel.replace('❯ 1.', ' 1.'), + panel.replace(' 2.', '❯ 2.'), + panel.replace('1. Add', '1. [ ] Add'), + panel.replace(' ☐ Skill routing', '← ☐ Skill routing ✔ Submit →'), + panel + '\n⏺ Continuing the review now.', + panel + '\n❯ 1. Different menu\n 2. Other choice', + 'Example panel:\n' + panel, + 'Quoted source:\n' + panel, + '```text\n' + panel, + '~~~~text\n```\n' + panel, + panel.split('\n').map(line => ' ' + line).join('\n'), + panel.split('\n').map(line => '> ' + line).join('\n'), + ]) expect(autoplanSetupDecision(frame, new Set()).kind, frame).not.toBe('unsupported_setup'); + expect(autoplanSetupDecision('```text\nearlier code\n```\n' + panel, new Set()).kind).toBe('unsupported_setup'); + const product = panel.replace("gstack works best when your project's CLAUDE.md includes skill routing rules. Should I add them now?", 'Which product API router should we use?'); + expect(autoplanSetupDecision(product, new Set()).kind).toBe('unrelated'); + }); + + test('mismatched, failed, answered, empty and multi-question metadata cannot diagnose this panel', () => { + for (const mutate of [ + (call: ReturnType) => { call.failed = true; }, + (call: ReturnType) => { call.answered = true; }, + (call: ReturnType) => { call.questions = []; }, + (call: ReturnType) => { call.questions.push(structuredClone(call.questions[0]!)); }, + (call: ReturnType) => { Object.assign(call.questions[0]!, {multiSelect:true}); }, + (call: ReturnType) => { call.questions[0]!.header = 'Other question'; }, + (call: ReturnType) => { call.questions[0]!.question = 'Different question '; }, + (call: ReturnType) => { call.questions[0]!.options[1]!.label = 'Different choice'; }, + (call: ReturnType) => { call.questions[0]!.options[1]!.label = 'No thanks, manual only'; }, + ]) { + const native = unsupportedNative(); mutate(native); + expect(autoplanSetupDecision(UNSUPPORTED_ROUTING, new Set(), native).kind).not.toBe('unsupported_setup'); + } + }); + + test('a supported native question clipped by the actual viewport preserves its existing input policy', async () => { + const {createPtyScreen} = await import('./helpers/pty-screen'); + const {matchesNativePlanQuestion} = await import('./helpers/claude-pty-runner'); + const native = unsupportedNative(); + native.questions[0]!.question += '\n' + Array.from({length:41}, (_,i) => + `Routing context line ${i+1}: keep current project conventions and existing commands.`).join('\n'); + native.questions[0]!.options[1]!.label = 'No thanks, invoke manually'; + const frame = `☐ Skill routing\n${native.questions[0]!.question}\n❯ 1. Add routing rules (Recommended)\n 2. No thanks, invoke manually\n 3. Type something.\n 4. Chat about this\nEnter to select · ↑/↓ to navigate · Esc to cancel`; + const screen = await createPtyScreen(120,40); + try { + screen.write(frame.replace(/\n/g,'\r\n')); + const visible = await screen.read(); + expect(visible).not.toContain('☐ Skill routing'); + expect(matchesNativePlanQuestion(visible,native)).toBe(true); + const seen = new Set(); + const decision = autoplanSetupDecision(visible,seen,native); + expect(decision.kind).toBe('input'); + if (decision.kind !== 'input') throw new Error('Expected supported native input'); + expect(decision.input).toBe('1'); + expect(seen.size).toBe(0); + for (const signature of decision.signatures) seen.add(signature); + expect(autoplanSetupDecision(visible,seen,native).kind).toBe('waiting'); + } finally { await screen.dispose(); } + }); +}); + +test.skipIf(process.platform === 'win32')('real PTY unsupported setup fails after ready with zero input and durable parsed evidence', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-unsupported-setup-')); + const fake = path.join(dir, 'fake-claude'); + const worker = path.join(dir, 'worker.ts'); + const resultFile = path.join(dir, 'result.json'); + const cases = [false, true].map(early => ({ + name: early ? 'early' : 'deferred', early, cwd: path.join(dir, early ? 'early' : 'deferred'), + events: path.join(dir, early ? 'early.jsonl' : 'deferred.jsonl'), + evalDir: path.join(dir, early ? 'early-artifacts' : 'deferred-artifacts'), + frame: UNSUPPORTED_ROUTING, native: unsupportedNative(), + })); + for (const item of cases) fs.mkdirSync(item.cwd); + fs.writeFileSync(fake, `#!${process.execPath}\n` + String.raw` +import * as fs from 'node:fs'; +import * as path from 'node:path'; +const item=JSON.parse(process.env.SETUP_DIAGNOSTIC_CASE); +const event=value=>fs.appendFileSync(item.events,JSON.stringify(value)+'\n'); +event({kind:'startup',pid:process.pid}); +if(item.early){ + const folder=path.join(process.env.CLAUDE_CONFIG_DIR,'projects','fixture');fs.mkdirSync(folder,{recursive:true}); + fs.writeFileSync(path.join(folder,item.name+'.jsonl'),JSON.stringify({type:'assistant',sessionId:item.name,isSidechain:false,cwd:process.cwd(),timestamp:new Date().toISOString(),message:{role:'assistant',content:[{type:'tool_use',id:'setup',name:'AskUserQuestion',input:{questions:item.native.questions}}]}})+'\n'); +} +process.stdin.setRawMode?.(true); +process.stdin.on('data',data=>event({kind:'input',data:data.toString()})); +process.stdout.write('\x1b[2J\x1b[H'+item.frame); +process.on('SIGINT',()=>process.exit(0));process.stdin.resume(); +`); + fs.chmodSync(fake, 0o755); + const url = (name: string) => pathToFileURL(path.resolve(import.meta.dir, 'helpers', name)).href; + fs.writeFileSync(worker, ` +import * as fs from 'node:fs'; +import {launchClaudePty} from ${JSON.stringify(url('claude-pty-runner.ts'))}; +import {autoplanSetupDecision,autoplanRoutingSetupInput} from ${JSON.stringify(url('autoplan-setup-question.ts'))}; +import {readPlanCountTranscript} from ${JSON.stringify(url('plan-count-transcript.ts'))}; +import {createPlanCountSnapshotWriter} from ${JSON.stringify(url('plan-count-artifacts.ts'))}; +const results=[]; +for(const item of ${JSON.stringify(cases)}){ + const session=await launchClaudePty({cwd:item.cwd,observeScreen:true,timeoutMs:20000,env:{SETUP_DIAGNOSTIC_CASE:JSON.stringify(item)}}); + const result={name:item.name,config:session.hermeticConfigDir}; + try{ + await session.waitFor('Enter to select',{timeoutMs:10000,pollMs:20}); + const viewport=await session.currentScreen(); + const native=readPlanCountTranscript(session.hermeticConfigDir,item.cwd); + const pending=native.calls.find(call=>!call.answered&&!call.failed); + if(Boolean(pending)!==item.early)throw Error('Readiness did not establish expected metadata state'); + result.legacyInput=autoplanRoutingSetupInput(viewport,new Set(),pending); + const decision=autoplanSetupDecision(viewport,new Set(),pending); + if(decision.kind==='input')throw Error('Unexpected guessed input'); + if(decision.kind!=='unsupported_setup')throw Error('Expected unsupported_setup, got '+decision.kind); + const save=createPlanCountSnapshotWriter({EVALS_RUN_ID:item.name,GSTACK_EVAL_DIR:item.evalDir}); + Object.assign(result,save({skillName:'autoplan',cwd:item.cwd,claudeConfigDir:session.hermeticConfigDir,raw:session.rawOutput(),visible:session.visibleText(),viewport, + observation:{state:'unsupported_setup',unsupportedSetup:decision,native,retention:'UI and parsed metadata only; full parent JSONL not guaranteed.'}})); + throw Error('UNSUPPORTED_SETUP_DIAGNOSTIC: '+decision.prompt); + }catch(error){result.failed=true;result.error=String(error);} + finally{await session.close();fs.rmSync(item.cwd,{recursive:true,force:true});} + results.push(result); +} +await Bun.write(${JSON.stringify(resultFile)},JSON.stringify(results)); +process.exitCode=results.some(result=>result.failed)?1:0; +`); + const child = Bun.spawn([process.execPath, worker], { + env: { ...process.env, BROWSE_TERMINAL_BINARY: fake, EVALS_HERMETIC: '1' }, stdout: 'pipe', stderr: 'pipe', + }); + const killer = setTimeout(() => child.kill('SIGKILL'), 25000); + try { + const [exit, stdout, stderr] = await Promise.all([child.exited, new Response(child.stdout).text(), new Response(child.stderr).text()]); + expect(exit, stdout + stderr).toBe(1); + const results = JSON.parse(fs.readFileSync(resultFile, 'utf8')); + expect(results.length).toBe(2); + for (const [index, result] of results.entries()) { + const item = cases[index]!; + expect(result.failed).toBe(true); + expect(result.error).toContain('UNSUPPORTED_SETUP_DIAGNOSTIC:'); + expect(result.legacyInput).toBeNull(); + expect(result.artifactError).toBeUndefined(); + const artifact = JSON.parse(fs.readFileSync(path.join(result.artifactDir, 'observation.json'), 'utf8')); + expect(artifact.state).toBe('unsupported_setup'); + expect(artifact.native.calls.length).toBe(item.early ? 1 : 0); + expect(artifact.retention).toContain('full parent JSONL not guaranteed'); + expect(fs.readFileSync(path.join(result.artifactDir, 'terminal.screen.log'), 'utf8')).toContain('Ask me after this review'); + expect(fs.readFileSync(path.join(result.artifactDir, 'terminal.raw.log'), 'utf8')).toContain('routing-injection'); + expect(fs.existsSync(item.cwd)).toBe(false); + expect(fs.existsSync(result.config)).toBe(false); + const events = fs.readFileSync(item.events, 'utf8').trim().split('\n').map(line => JSON.parse(line)); + expect(events.filter(event => event.kind === 'input')).toEqual([]); + expect(() => process.kill(events[0].pid, 0)).toThrow(); + } + } finally { + clearTimeout(killer); child.kill('SIGKILL'); + for (const item of cases) { + if (!fs.existsSync(item.events)) continue; + const pid = JSON.parse(fs.readFileSync(item.events, 'utf8').split('\n')[0]!).pid; + if (process.platform === 'linux') { + try { if (fs.readFileSync('/proc/' + pid + '/cmdline', 'utf8').split('\0').includes(fake)) process.kill(pid, 'SIGKILL'); } + catch { /* already reaped or PID no longer belongs to this fixture */ } + } + } + fs.rmSync(dir, {recursive:true,force:true}); + } +}, 30000); diff --git a/test/ceo-completion-handoff-m.test.ts b/test/ceo-completion-handoff-m.test.ts new file mode 100644 index 000000000..898f0984c --- /dev/null +++ b/test/ceo-completion-handoff-m.test.ts @@ -0,0 +1,261 @@ +import { describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { capturePlanCountQuestion, ceoFirstReviewAUQ, ceoStep0Boundary, hasNativePlanTerminal, + nativePlanCallFingerprint, planCountQuestionPhase } from './helpers/claude-pty-runner'; +import { isCeoCompletionHandoff, pickCeoCompletionHandoff } from './helpers/ceo-completion-handoff'; +import type { NativePlanQuestionCall } from './helpers/plan-count-transcript'; +import captured from './fixtures/ceo-completion-handoff-m-call.json'; + +const calls = () => structuredClone(captured.calls) as NativePlanQuestionCall[]; +const handoff = () => calls().at(-1)!; +const fingerprint = (call: NativePlanQuestionCall) => nativePlanCallFingerprint(call, 0, false); + +function reanswer(call: NativePlanQuestionCall) { + call.answers = { [call.questions[0]!.question]: call.questions[0]!.options[0]!.label }; + return call; +} + +describe('CEO completion described by a native navigation choice', () => { + test('the exact seven-call session preserves three setup and three finding decisions', () => { + let reviewStarted = false; + const counts = { setup: 0, review: 0, administrative: 0 }; + const original = calls(); + for (const call of original) { + const phase = planCountQuestionPhase(fingerprint(call), reviewStarted, ceoStep0Boundary, + ceoFirstReviewAUQ, undefined, isCeoCompletionHandoff); + reviewStarted = phase.reviewStarted; + if (phase.administrative) counts.administrative++; + else if (phase.preReview) counts.setup++; + else counts.review++; + } + expect(counts).toEqual({ setup: 3, review: 3, administrative: 1 }); + expect(original).toEqual(calls()); + expect(original.slice(3, -1).map(call => isCeoCompletionHandoff(fingerprint(call)))).toEqual([false, false, false]); + }); + + test('the active pending handoff selects the actual manual option in either order', () => { + for (const reverse of [false, true]) { + const call = handoff(); + call.answered = false; + delete call.answers; + delete call.unansweredQuestionIndices; + const q = call.questions[0]!; + if (reverse) q.options.reverse(); + const screen = `☐ ${q.header}\n${q.question}\n❯ 1. ${q.options[0]!.label}\n 2. ${q.options[1]!.label}\nEnter to select · ↑/↓ to navigate · Esc to cancel`; + const active = capturePlanCountQuestion(screen, new Set(), 0, false, call)!; + expect(active.nativeCall?.toolUseId).toBe(call.toolUseId); + expect(pickCeoCompletionHandoff(fingerprint(call), active)).toBe(reverse ? 1 : 2); + expect(isCeoCompletionHandoff(active)).toBe(false); + expect(pickCeoCompletionHandoff(capturePlanCountQuestion(screen, new Set(), 0, false)!)).toBeNull(); + expect(pickCeoCompletionHandoff(fingerprint(call), { ...active, signature: 'another:call' })).toBeNull(); + } + }); + + test('completion placement is independent of the next-step wording and option order', () => { + const call = handoff(); + const q = call.questions[0]!; + q.question = 'D9 — Next steps: The review is done. Where should we go next? '; + q.header = 'Next review'; + q.options[0]!.description = 'Eng review is the required shipping gate.'; + for (const description of [ + 'CEO review found 3 specification gaps (all resolved). Continue manually.', + 'The CEO review identified gaps; all findings are resolved. Continue manually.', + 'CEO review is complete with 0 unresolved decisions. Continue manually.', + ]) { + q.options[1]!.description = description; + expect(isCeoCompletionHandoff(fingerprint(reanswer(call)))).toBe(true); + } + }); + + test('conditional, unfinished, quoted and non-CEO recaps cannot supply completion', () => { + for (const description of [ + 'Eng review is the required shipping gate. CEO review found 3 gaps (all resolved after adding tests).', + 'Eng review is the required shipping gate. If CEO review found 3 gaps (all resolved), continue.', + 'Eng review is the required shipping gate. CEO review found 3 gaps (all resolved); one gap remains.', + 'Eng review is the required shipping gate. CEO review found 3 gaps (all resolved). There is an unresolved test issue.', + 'Eng review is the required shipping gate. The document says "CEO review found 3 gaps (all resolved)."', + 'Eng review is the required shipping gate. Design review found 3 gaps (all resolved).', + 'Eng review is the required shipping gate. CEO review found 3 gaps.', + 'Eng review is the required shipping gate. CEO review found 3 specification gaps (not all resolved).', + 'Eng review is the required shipping gate. CEO review did not find all gaps resolved.', + 'Eng review is the required shipping gate. CEO review found 3 gaps (all resolved). Also add a new test before proceeding.', + 'Eng review is the required shipping gate. CEO review found 3 gaps (all resolved). Please fix the new missing authorization check before proceeding.', + ]) { + const call = handoff(); + call.questions[0]!.options[0]!.description = description; + expect(isCeoCompletionHandoff(fingerprint(call)), description).toBe(false); + call.answered = false; + expect(pickCeoCompletionHandoff(fingerprint(call))).toBeNull(); + } + }); + + test('native identity, completion, required gate and exclusively administrative choices remain necessary', () => { + for (const mutate of [ + (call: NativePlanQuestionCall) => { call.questions[0]!.question = 'Review complete only after fixing tests. What next? '; }, + (call: NativePlanQuestionCall) => { call.questions[0]!.question = 'Should we finish reviewing? '; }, + (call: NativePlanQuestionCall) => { call.questions[0]!.question += ' '; }, + (call: NativePlanQuestionCall) => { call.questions[0]!.question = ' ' + call.questions[0]!.question; }, + (call: NativePlanQuestionCall) => { call.questions[0]!.header = 'TODO decision'; }, + (call: NativePlanQuestionCall) => { call.questions[0]!.options[1]!.label = 'Add another TODO'; }, + (call: NativePlanQuestionCall) => { call.questions[0]!.options[0]!.label += ' and fix the missing test'; }, + (call: NativePlanQuestionCall) => { for (const option of call.questions[0]!.options) option.description = option.description?.replaceAll('required', 'optional'); }, + (call: NativePlanQuestionCall) => { call.questions.push(calls()[3]!.questions[0]!); }, + (call: NativePlanQuestionCall) => { call.questions[0]!.multiSelect = true; }, + (call: NativePlanQuestionCall) => { call.failed = true; }, + (call: NativePlanQuestionCall) => { call.unansweredQuestionIndices = [0]; }, + (call: NativePlanQuestionCall) => { call.answered = false; }, + ]) { + const call = handoff(); + mutate(call); + expect(isCeoCompletionHandoff(fingerprint(reanswer(call)))).toBe(false); + } + const addedWork = handoff(); + addedWork.answers = { [addedWork.questions[0]!.question]: 'First add another payment test' }; + expect(isCeoCompletionHandoff(fingerprint(addedWork))).toBe(false); + }); + + test('the actual report and Exit order permits only the administrative freshness exception', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ceo-native-handoff-')); + const report = path.join(dir, 'plan.md'); + try { + fs.writeFileSync(report, captured.report.content); + const reportAt = Date.parse(captured.report.successfulResult.timestamp) / 1000; + fs.utimesSync(report, reportAt, reportAt); + const transcript = { status: 'ready' as const, calls: calls(), assistantMessages: [], + planReadyRequests: structuredClone(captured.planReadyRequests) }; + const administrative = new Set(transcript.calls.filter(call => isCeoCompletionHandoff(fingerprint(call))) + .map(call => `${call.sessionId}:${call.toolUseId}`)); + const startedAt = Date.parse('2026-09-09T00:15:27Z'); + expect(administrative.size).toBe(1); + expect(hasNativePlanTerminal(transcript, report, startedAt, 'plan_ready')).toBe(false); + expect(hasNativePlanTerminal(transcript, report, startedAt, 'plan_ready', administrative)).toBe(true); + transcript.planReadyRequests[0]!.failed = true; + expect(hasNativePlanTerminal(transcript, report, startedAt, 'plan_ready', administrative)).toBe(false); + transcript.planReadyRequests[0]!.failed = false; + transcript.calls.push({ ...structuredClone(transcript.calls[3]!), toolUseId: 'new-test-obligation', + answeredAt: captured.calls.at(-1)!.answeredAt }); + expect(hasNativePlanTerminal(transcript, report, startedAt, 'plan_ready', administrative)).toBe(false); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); +}); + +describe('native next-review navigation with a resolved CEO recap', () => { + const retryCalls = () => structuredClone(captured.distinctRetry.calls) as NativePlanQuestionCall[]; + const retryHandoff = () => retryCalls().at(-1)!; + + test('the captured retry preserves its four actual findings and the unchanged mechanical band', () => { + let started = false; + const counts = { setup: 0, review: 0, administrative: 0 }; + const original = retryCalls(); + for (const call of original) { + const phase = planCountQuestionPhase(fingerprint(call), started, ceoStep0Boundary, + ceoFirstReviewAUQ, undefined, isCeoCompletionHandoff); + started = phase.reviewStarted; + if (phase.administrative) counts.administrative++; + else if (phase.preReview) counts.setup++; + else counts.review++; + } + expect(counts).toEqual({ setup: 4, review: 4, administrative: 1 }); + expect(original).toEqual(retryCalls()); + // The transcript contains four individual findings. The unasked dispatcher + // remedy remains a separate workflow-quality limitation, never a fifth call. + expect(original.slice(4, -1).every(call => !isCeoCompletionHandoff(fingerprint(call)))).toBe(true); + }); + + test('actual offered manual navigation still requires the matching pending native question', () => { + for (const reverse of [false, true]) { + const call = retryHandoff(); + call.answered = false; + delete call.answers; + const q = call.questions[0]!; + if (reverse) q.options.reverse(); + const screen = `☐ ${q.header}\n${q.question}\n❯ 1. ${q.options[0]!.label}\n 2. ${q.options[1]!.label}\nEnter to select · ↑/↓ to navigate · Esc to cancel`; + const active = capturePlanCountQuestion(screen, new Set(), 0, false, call)!; + expect(active.nativeCall?.toolUseId).toBe(call.toolUseId); + expect(pickCeoCompletionHandoff(fingerprint(call), active)).toBe(reverse ? 1 : 2); + expect(isCeoCompletionHandoff(active)).toBe(false); + expect(pickCeoCompletionHandoff(capturePlanCountQuestion(screen, new Set(), 0, false)!)).toBeNull(); + } + }); + + test('partial, conditional, quoted or still-open recaps never establish this navigation boundary', () => { + for (const recap of [ + 'This CEO review resolved some security bugs.', + 'This CEO review resolved most security bugs.', + 'This CEO review resolved all but one security bugs.', + 'This CEO review resolved two of three security bugs.', + 'This CEO review only resolved the security bugs.', + 'This CEO review did not resolve the security bugs.', + 'If this CEO review resolved the security bugs, continue.', + 'The document says "This CEO review resolved the security bugs."', + 'This CEO review resolved the security bugs. One issue remains unresolved.', + 'This CEO review resolved the security bugs; validation of that remedy is still pending.', + 'This CEO review resolved the security bugs. Please add a new test first.', + 'This CEO review will resolve the security bugs.', + ]) { + const call = retryHandoff(); + call.questions[0]!.options[0]!.description = 'Eng review is the required shipping gate. ' + recap; + expect(isCeoCompletionHandoff(fingerprint(call)), recap).toBe(false); + call.answered = false; + expect(pickCeoCompletionHandoff(fingerprint(call))).toBeNull(); + } + for (const question of [ + 'Should we add a missing authorization test as the next step after this CEO review?', + 'The CEO review did not finish. What is the next step after this CEO review?', + 'Can you first fix the missing authorization check as the next step after this CEO review?', + 'If the CEO review finishes, what is the next step after this CEO review?', + 'Example: What is the next step after this CEO review?', + ]) { + const call = retryHandoff(); + call.questions[0]!.question = question + ' '; + reanswer(call); + expect(isCeoCompletionHandoff(fingerprint(call)), question).toBe(false); + call.answered = false; + expect(pickCeoCompletionHandoff(fingerprint(call)), question).toBeNull(); + } + for (const mutate of [ + (call: NativePlanQuestionCall) => { call.questions[0]!.question = 'Choose a fix for the missing test '; }, + (call: NativePlanQuestionCall) => { call.questions[0]!.question = call.questions[0]!.question.replace('plan-ceo-next-step', 'plan-ceo-test-gap'); }, + (call: NativePlanQuestionCall) => { call.questions[0]!.question = call.questions[0]!.question.replace(/ ]+>/, ''); }, + (call: NativePlanQuestionCall) => { call.questions[0]!.header = 'TODO'; }, + (call: NativePlanQuestionCall) => { call.questions[0]!.options[1]!.label = 'Add a missing receipt assertion'; }, + (call: NativePlanQuestionCall) => { call.questions.push(retryCalls()[4]!.questions[0]!); }, + (call: NativePlanQuestionCall) => { call.failed = true; }, + (call: NativePlanQuestionCall) => { call.unansweredQuestionIndices = [0]; }, + ]) { + const call = retryHandoff(); + mutate(call); + expect(isCeoCompletionHandoff(fingerprint(reanswer(call)))).toBe(false); + } + }); + + test('the final native report edit precedes handoff and still covers every real answer', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ceo-retry-handoff-')); + const report = path.join(dir, 'plan.md'); + try { + fs.writeFileSync(report, captured.distinctRetry.reportContent); + const reportAt = Date.parse(captured.distinctRetry.reportUpdate.at(-1)!.timestamp) / 1000; + fs.utimesSync(report, reportAt, reportAt); + const transcript = { status: 'ready' as const, calls: retryCalls(), assistantMessages: [], + planReadyRequests: structuredClone(captured.distinctRetry.planReadyRequests) }; + const administrative = new Set(transcript.calls.filter(call => isCeoCompletionHandoff(fingerprint(call))) + .map(call => `${call.sessionId}:${call.toolUseId}`)); + const startedAt = Date.parse('2026-09-09T00:23:30Z'); + expect(administrative.size).toBe(1); + expect(hasNativePlanTerminal(transcript, report, startedAt, 'plan_ready')).toBe(false); + expect(hasNativePlanTerminal(transcript, report, startedAt, 'plan_ready', administrative)).toBe(true); + transcript.planReadyRequests[0]!.failed = true; + expect(hasNativePlanTerminal(transcript, report, startedAt, 'plan_ready', administrative)).toBe(false); + transcript.planReadyRequests[0]!.failed = false; + transcript.calls.push({ ...structuredClone(transcript.calls[4]!), toolUseId: 'new-independent-finding', + answeredAt: transcript.calls.at(-1)!.answeredAt }); + expect(hasNativePlanTerminal(transcript, report, startedAt, 'plan_ready', administrative)).toBe(false); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/test/fixtures/ceo-completion-handoff-m-call.json b/test/fixtures/ceo-completion-handoff-m-call.json new file mode 100644 index 000000000..689c2856f --- /dev/null +++ b/test/fixtures/ceo-completion-handoff-m-call.json @@ -0,0 +1,562 @@ +{ + "sourceHead": "084f75297dfe33530baa0cce33b6ec60faf0cc99", + "artifact": "plan-ceo-review-1788912957495-fB0byY", + "nativeSha256": "275cf3bfb1f78699abef1570c04e037833ab28c939c5db5e500e20b0efb47e77", + "rawCounts": { + "setup": 3, + "review": 4, + "administrative": 0 + }, + "calls": [ + { + "sessionId": "5036f6f6-8360-4f76-bea8-417f5c727b32", + "toolUseId": "toolu_013RgeeqwFtbTgK5PST95ywa", + "questions": [ + { + "question": "D1 \u2014 No design doc found. Run /office-hours first, or proceed with the standard review? ", + "header": "Prereq skill", + "multiSelect": false, + "options": [ + { + "label": "Skip \u2014 proceed with review (Recommended)", + "description": "The plan is already well-scoped (2 specific tests, documented infrastructure). /office-hours would add little here. Proceed directly to the CEO review." + }, + { + "label": "Run /office-hours first", + "description": "Produces a structured problem statement, premise challenge, and explored alternatives \u2014 gives the review sharper input. Takes ~10 min. We'll pick up the review right after." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 \u2014 No design doc found. Run /office-hours first, or proceed with the standard review? ": "Skip \u2014 proceed with review (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T00:17:45.716Z" + }, + { + "sessionId": "5036f6f6-8360-4f76-bea8-417f5c727b32", + "toolUseId": "toolu_01Gxc4Dzcfghy2XJ44dwociF", + "questions": [ + { + "question": "D2 \u2014 Which implementation approach for the test coverage? ", + "header": "Approach", + "multiSelect": false, + "options": [ + { + "label": "A) Minimal (Recommended)", + "description": "Exactly 2 tests as written: happy path + 502 retry exhaustion. Uses existing factory and virtual sleeper. Completeness: 7/10. Effort: human ~30 min / CC ~5 min. Closes the stated gap cleanly." + }, + { + "label": "B) Expanded matrix", + "description": "Add 402 (card decline) and 429 (rate limit) function-level tests alongside the 2 planned. Already infrastructure-supported \u2014 same factory, same mocks. Completeness: 9/10. Effort: human ~45 min / CC ~8 min. Closes the full function-level gap while scope is open." + }, + { + "label": "C) Table-driven / parameterized", + "description": "Restructure as parameterized tests covering all error paths. DRY and scales well. May require factory extensions. Completeness: 9/10. Effort: human ~1 hr / CC ~15 min. Cleaner structure, higher initial complexity." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 \u2014 Which implementation approach for the test coverage? ": "A) Minimal (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T00:18:27.821Z" + }, + { + "sessionId": "5036f6f6-8360-4f76-bea8-417f5c727b32", + "toolUseId": "toolu_01YAqKUDJyGUgSLjFWNdLxC9", + "questions": [ + { + "question": "D3 \u2014 Which review mode for this plan? ", + "header": "Review mode", + "multiSelect": false, + "options": [ + { + "label": "HOLD SCOPE (Recommended)", + "description": "The 2 tests are the scope. Review for maximum rigor: assertion correctness, edge cases, test isolation, naming, factory usage. No expansions surfaced." + }, + { + "label": "SELECTIVE EXPANSION", + "description": "Hold the 2-test baseline, but surface the 402/429 expansion opportunities individually so you can cherry-pick. You decide what gets added." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 \u2014 Which review mode for this plan? ": "HOLD SCOPE (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T00:18:37.843Z" + }, + { + "sessionId": "5036f6f6-8360-4f76-bea8-417f5c727b32", + "toolUseId": "toolu_0145EK1Vb4mBrAEd1vsHFnC9", + "questions": [ + { + "question": "Finding 1 (Section 2) \u2014 What does 'fails clean' mean for the 502 test assertion? The plan says 'fails clean' but doesn't specify what the test should assert after retries are exhausted. ", + "header": "502 assertion", + "multiSelect": false, + "options": [ + { + "label": "A) Assert a specific exception is raised (Recommended)", + "description": "Test asserts processPayment() raises a specific exception class (e.g., PaymentError, StripeError, or similar) when 502 retries are exhausted. Update the plan to name the exception class. Verifiable: the test fails if the wrong exception or no exception is raised." + }, + { + "label": "B) Assert an error return value", + "description": "Test asserts processPayment() returns a failure object/nil/false. Update the plan to specify the return shape. Verifiable: test fails if success is returned." + }, + { + "label": "C) Leave as-is \u2014 implementer decides", + "description": "Keep 'fails clean' as the spec. The implementer matches whatever the existing behavior is. Risk: the test may silently pass on wrong behavior if the assertion is too loose." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "Finding 1 (Section 2) \u2014 What does 'fails clean' mean for the 502 test assertion? The plan says 'fails clean' but doesn't specify what the test should assert after retries are exhausted. ": "A) Assert a specific exception is raised (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T00:19:38.037Z" + }, + { + "sessionId": "5036f6f6-8360-4f76-bea8-417f5c727b32", + "toolUseId": "toolu_01GNKjDq8UJMakRW1jCmNFhe", + "questions": [ + { + "question": "Finding 2 (Section 4) \u2014 'Assert correct receipt is generated' is underspecified. What should the happy path test actually assert about the receipt? ", + "header": "Receipt spec", + "multiSelect": false, + "options": [ + { + "label": "A) Assert specific receipt fields (Recommended)", + "description": "Test verifies at minimum: receipt amount matches charge amount, and customer/transaction ID is present and non-nil. Update plan to name the fields. Makes the test fail on wrong data, not just on nil receipt." + }, + { + "label": "B) Assert receipt is non-nil only", + "description": "Test checks receipt is returned (not nil/false). Simpler, but doesn't catch data corruption or wrong-amount bugs. Completeness: 5/10 as a correctness test." + }, + { + "label": "C) Assert receipt matches a snapshot/fixture", + "description": "Test compares the full receipt against a known-good fixture. Catches all field regressions but brittle to non-functional changes (formatting, field order). Best if receipt schema is stable." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "Finding 2 (Section 4) \u2014 'Assert correct receipt is generated' is underspecified. What should the happy path test actually assert about the receipt? ": "A) Assert specific receipt fields (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T00:19:56.074Z" + }, + { + "sessionId": "5036f6f6-8360-4f76-bea8-417f5c727b32", + "toolUseId": "toolu_01Cs55t94c1ENEJx9KUupDEj", + "questions": [ + { + "question": "Finding 3 (Section 6) \u2014 The virtual sleeper records backoff duration. Should the 502 test assert the sleep duration was correct? ", + "header": "Backoff assert", + "multiSelect": false, + "options": [ + { + "label": "A) Assert sleep count only \u2014 not duration (Recommended)", + "description": "Test verifies virtual_sleeper.calls.length == 1 (exactly one sleep occurred between the two charge attempts). Catches missing or extra retries without brittleness around the specific timing formula." + }, + { + "label": "B) Assert sleep count AND expected duration", + "description": "Test verifies virtual_sleeper.calls == [expected_backoff_ms]. Catches broken backoff formula too, but couples the test to the timing implementation. Update plan to specify the expected duration." + }, + { + "label": "C) Don\u2019t assert the sleeper at all", + "description": "Two charge attempts (from Stripe mock call history) are sufficient proof of retry. Skip the sleeper assertion entirely. Risk: if retry fires without sleeping, test still passes." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "Finding 3 (Section 6) \u2014 The virtual sleeper records backoff duration. Should the 502 test assert the sleep duration was correct? ": "A) Assert sleep count only \u2014 not duration (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T00:20:12.131Z" + }, + { + "sessionId": "5036f6f6-8360-4f76-bea8-417f5c727b32", + "toolUseId": "toolu_01DNHtxrJdk1mb8cYyfmBh9B", + "questions": [ + { + "question": "Review complete. What's the next step? ", + "header": "Next step", + "multiSelect": false, + "options": [ + { + "label": "A) Run /plan-eng-review next (Recommended)", + "description": "Eng review is the required shipping gate. It covers architecture, code quality, tests, and performance. CEO review found 3 specification gaps (all resolved); eng review validates the implementation plan is sound." + }, + { + "label": "B) Skip \u2014 I'll handle reviews manually", + "description": "Proceed without running eng review. Note: eng review is the required gate before shipping." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "Review complete. What's the next step? ": "A) Run /plan-eng-review next (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T00:24:14.771Z" + } + ], + "report": { + "id": "toolu_01Ndio4NgC8czFMD2XqEaaZ1", + "timestamp": "2026-09-09T00:23:12.466Z", + "path": "/tmp/gstack-paid-shard-Q1hRY6/tmp/gstack-e2e-plan-ceo-paired-lEplIi/gstack-test-plan-ceo-paired.md", + "content": "# Plan Review: Payment Processing \u2014 Test Coverage\n\n## Context\n\n`processPayment()` is a revenue-critical function with zero function-level unit tests. The Stripe adapter suite and receipt-builder regression tests cover their own layers, but nothing verifies that `processPayment()` itself correctly integrates those layers. This review closes that gap for the two most critical paths: the success path (correctness) and the 502 retry exhaustion path (graceful degradation).\n\n## Review Summary\n\n- **Mode:** HOLD SCOPE\n- **Approach:** Minimal \u2014 exactly 2 tests as specified, using existing test infrastructure\n- **Findings:** 3 specification gaps resolved (exception class, receipt fields, backoff assertion)\n- **Critical gaps:** 0\n\n---\n\n## Step 0 Decisions\n\n- **Implementation approach:** A (Minimal) \u2014 exactly 2 tests reusing existing factory, virtual sleeper, and Stripe mock call history\n- **Mode:** HOLD SCOPE \u2014 the plan's scope is correct; make it bulletproof\n\n---\n\n## Section 1: Architecture\n\nTest-only change. No production architecture changes.\n\n```\nEXISTING ARCHITECTURE (unchanged)\n processPayment() \u2500\u2500\u2192 StripeAdapter \u2500\u2500\u2192 Stripe API\n \u2502 \u2502\n \u25bc \u25bc\n ReceiptBuilder retry/backoff\n\nTEST ADDITIONS (new)\n HappyPathTest \u2500\u2500\u2192 payment_factory(max_retries=1)\n \u2502\u2500\u2500 Stripe mock (200 success)\n \u2502\u2500\u2500 virtual_sleeper (should NOT be called)\n \u2500\u2500\u2192 assert receipt.amount, receipt.customer_id, receipt.transaction_id\n\n 502Test \u2500\u2500\u2192 payment_factory(max_retries=1)\n \u2502\u2500\u2500 Stripe mock (502 \u2192 502)\n \u2502\u2500\u2500 virtual_sleeper.calls.length == 1\n \u2500\u2500\u2192 assert Stripe mock called exactly 2 times\n \u2500\u2500\u2192 assert specific exception class raised\n```\n\nNo new coupling, no single points of failure. Rollback: trivial (delete test file).\n\nNo issues.\n\n---\n\n## Section 2: Error & Rescue Map\n\n```\nCODEPATH | WHAT CAN GO WRONG | EXCEPTION CLASS\n---------------------|--------------------------------|---------------------------\nprocessPayment() | Stripe 502 (retries exhausted) | \n | Stripe 200 success | N/A\n\nFAILURE | RESCUED? | RESCUE ACTION | TESTED?\n----------------|----------|--------------------------------|--------\n502 exhaustion | Y | retry once (max_retries=1), | YES (test 2)\n | | then raise specific exception |\n```\n\n**Resolved (Finding 1):** \"fails clean\" is clarified as: the test asserts a specific named exception class is raised when 502 retries are exhausted. The implementer must inspect the production `processPayment()` error handling to find the correct exception class name and use it explicitly in the test assertion.\n\nNo critical gaps.\n\n---\n\n## Section 3: Security\n\nTest code only. No new endpoints, credentials, or data access. No issues.\n\n---\n\n## Section 4: Data Flow & Interaction Edge Cases\n\n```\nHAPPY PATH:\n payment_params \u2500\u2500\u2192 processPayment() \u2500\u2500\u2192 StripeAdapter mock (200)\n \u2502\n \u25bc\n ReceiptBuilder \u2500\u2500\u2192 receipt\n \u2502\n ASSERT: receipt.amount == charge_amount\n ASSERT: receipt.customer_id is present\n ASSERT: receipt.transaction_id is present\n\n502 ERROR PATH:\n payment_params \u2500\u2500\u2192 processPayment() \u2500\u2500\u2192 StripeAdapter mock (502)\n \u2502 \u2502\n \u25bc \u25bc\n virtual_sleeper.sleep() \u2500\u2500\u25b6 StripeAdapter mock (502)\n \u2502\n RETRIES EXHAUSTED (max_retries=1: 2 total attempts)\n \u2502\n ASSERT: Stripe mock call_history.length == 2\n ASSERT: virtual_sleeper.calls.length == 1\n ASSERT: raises SpecificExceptionClass\n```\n\n**Resolved (Finding 2):** \"assert correct receipt is generated\" is clarified as: assert `receipt.amount == charge_amount`, `receipt.customer_id` is present and non-nil, and `receipt.transaction_id` is present and non-nil. This catches data corruption and wrong-amount bugs, not just nil receipt.\n\nNo unhandled edge cases within scope.\n\n---\n\n## Section 5: Code Quality\n\nNo implementation code to review. Two tests are deliberately separate (plan correctly notes: success path is correctness, failure path is graceful degradation). No DRY violations, no over-engineering. No issues.\n\n---\n\n## Section 6: Test Review\n\n```\nNEW CODEPATHS:\n \u251c\u2500\u2500 processPayment() happy path\n \u2502 Type: Unit\n \u2502 \u2705 Planned\n \u2502 Happy path: charge succeeds \u2192 receipt returned with correct fields\n \u2502 Failure path: N/A (this test covers success only)\n \u2502 Hostile QA test: assert exact field values, not just non-nil\n \u2514\u2500\u2500 processPayment() 502 retry exhaustion\n Type: Unit\n \u2705 Planned\n Happy path: N/A (this test covers failure only)\n Failure path: 502 \u00d7 2 \u2192 exception raised, 2 Stripe calls made, 1 sleep recorded\n Hostile QA test: would assert retry fired with WRONG count \u2192 catches off-by-one in max_retries\n\nNEW ERROR/RESCUE PATHS:\n \u2514\u2500\u2500 502 exhaustion \u2192 specific exception raised \u2705 (resolved via Finding 1)\n```\n\n**Resolved (Finding 3):** The 502 test must assert `virtual_sleeper.calls.length == 1` (exactly one sleep between the two charge attempts). This catches the case where retry fires without sleeping (broken backoff), or fires more than once (max_retries misconfigured).\n\nTest pyramid: 2 unit tests. Correct level for this scope.\nFlakiness risk: None \u2014 virtual sleeper eliminates timing dependency; Stripe mock is deterministic.\n\n---\n\n## Section 7: Performance\n\nTest code only. Virtual sleeper eliminates real delays. No issues.\n\n---\n\n## Section 8: Observability\n\nTest code only. Test failure output + Stripe mock call history provide sufficient debug signal. No issues.\n\n---\n\n## Section 9: Deployment\n\nTest code only. No migrations, feature flags, or deployment sequence. No issues.\n\n---\n\n## Section 10: Long-Term Trajectory\n\n- **Reversibility:** 5/5\n- **Technical debt introduced:** None \u2014 tests reduce debt\n- **1-year readability:** Two focused tests with named assertions are self-documenting\n- **Dream state delta:** This plan covers 2 of 6 expected function-level scenarios. 402, 429, timeout, and receipt error paths remain adapter-level only.\n\n---\n\n## Section 11: Design/UX\n\nSKIPPED \u2014 no UI scope.\n\n---\n\n## Required Outputs\n\n### NOT in Scope\n- 402 (card decline) function-level test \u2014 covered at adapter level; expansion item\n- 429 (rate limit) function-level test \u2014 covered at adapter level; expansion item\n- Timeout function-level test \u2014 expansion item\n- Receipt builder error function-level test \u2014 covered by existing regression tests\n- Parameter validation tests for nil/empty inputs \u2014 out of scope per plan\n\n### What Already Exists\n| Existing helper | Covers | Reused? |\n|----------------|--------|---------|\n| Stripe adapter suite | network timeouts, 402, 429, 502\u2192success | Yes (unchanged) |\n| Receipt-builder regression tests | receipt builder failures | Yes (unchanged) |\n| Payment test factory | max_retries=1, Stripe mock call history | Yes (both tests) |\n| Virtual sleeper | records backoff, no real delays | Yes (502 test) |\n\n### Dream State Delta\n```\nCURRENT STATE THIS PLAN 12-MONTH IDEAL\nNo function-level \u2500\u2500\u2192 Happy path: \u2705 \u2500\u2500\u2192 Happy path: \u2705\ntests for 502 retry: \u2705 502 retry: \u2705\nprocessPayment() 402 decline: \u2717\n 429 rate limit: \u2717\n Timeout: \u2717\n Receipt error: \u2717\n```\n\n### Error & Rescue Registry\n```\nCODEPATH | EXCEPTION CLASS | RESCUED? | ACTION | USER SEES\n-----------------|--------------------------|----------|------------------|-----------\nprocessPayment() | StripeError/PaymentError | Y | retry\u00d71, raise | caller handles\n(502 exhaustion) | (name from prod code) | | |\n | ReceiptBuilderError | Y | existing tests | existing behavior\n```\n\n### Failure Modes Registry\n```\nCODEPATH | FAILURE MODE | RESCUED? | TEST? | USER SEES? | LOGGED?\n---------------------|-------------------|----------|-------|------------|--------\nprocessPayment() | 502 \u00d7 2 | Y | YES | via caller | (assumed)\nprocessPayment() | success path | N/A | YES | receipt | (assumed)\nprocessPayment() | 402 decline | Y | no | via caller | adapter-lvl\nprocessPayment() | 429 rate limit | Y | no | via caller | adapter-lvl\nprocessPayment() | timeout | ? | no | ? | ?\n```\n\nNo CRITICAL GAPS in the accepted scope.\n\n---\n\n## Implementation Tasks\n\nSynthesized from this review's findings. Each task derives from a specific finding above.\n\n- [ ] **T1 (P1, human: ~30min / CC: ~5min)** \u2014 processPayment tests \u2014 Identify and name the exception class raised on 502 retry exhaustion\n - Surfaced by: Section 2 \u2014 \"fails clean\" is underspecified; test must assert a specific exception class\n - Files: `payment/test_process_payment.py` (or equivalent)\n - Verify: Run the 502 test; confirm it fails if processPayment() does NOT raise the named exception\n\n- [ ] **T2 (P1, human: ~30min / CC: ~5min)** \u2014 processPayment tests \u2014 Assert specific receipt fields in happy path test\n - Surfaced by: Section 4 \u2014 \"correct receipt generated\" is underspecified; existence check alone cannot catch data corruption\n - Files: `payment/test_process_payment.py`\n - Assertions: `receipt.amount == charge_amount`, `receipt.customer_id` is present and non-nil, `receipt.transaction_id` is present and non-nil\n - Verify: Temporarily mutate the mock to return wrong amount; confirm test fails\n\n- [ ] **T3 (P1, human: ~30min / CC: ~5min)** \u2014 processPayment tests \u2014 Assert virtual_sleeper.calls.length == 1 in 502 test\n - Surfaced by: Section 6 \u2014 virtual sleeper records backoff; test should verify exactly one sleep occurred between the two charge attempts\n - Files: `payment/test_process_payment.py`\n - Assertions: `virtual_sleeper.calls.length == 1` AND `stripe_mock.call_history.length == 2`\n - Verify: Remove the sleep assertion; confirm test still passes; restore it; confirm it now catches a hypothetical \"retry without sleep\"\n\n---\n\n## Completion Summary\n\n```\n+====================================================================+\n| MEGA PLAN REVIEW \u2014 COMPLETION SUMMARY |\n+====================================================================+\n| Mode selected | HOLD SCOPE |\n| System Audit | 1-commit fixture repo, no TODOs, no design |\n| | doc, no handoff |\n| Step 0 | Approach A (minimal 2 tests), HOLD SCOPE |\n| Section 1 (Arch) | 0 issues found |\n| Section 2 (Errors) | 1 gap resolved (exception class named) |\n| Section 3 (Security)| 0 issues found |\n| Section 4 (Data/UX) | 1 gap resolved (receipt fields specified) |\n| Section 5 (Quality) | 0 issues found |\n| Section 6 (Tests) | Diagram produced, 1 gap resolved (backoff) |\n| Section 7 (Perf) | 0 issues found |\n| Section 8 (Observ) | 0 gaps found |\n| Section 9 (Deploy) | 0 risks flagged |\n| Section 10 (Future) | Reversibility: 5/5, debt items: 0 |\n| Section 11 (Design) | SKIPPED (no UI scope) |\n+--------------------------------------------------------------------+\n| NOT in scope | written (5 items) |\n| What already exists | written (4 helpers reused) |\n| Dream state delta | written (4 of 6 paths remain) |\n| Error/rescue registry| 2 paths mapped, 0 CRITICAL GAPS |\n| Failure modes | 5 total, 0 CRITICAL GAPS in scope |\n| TODOS.md updates | 0 items (no evidenced gaps beyond findings) |\n| Scope proposals | 0 proposed, 0 accepted (HOLD SCOPE) |\n| CEO plan | skipped (HOLD SCOPE) |\n| Outside voice | skipped (codex_reviews disabled) |\n| Diagrams produced | 3 (architecture, happy path flow, 502 flow) |\n| Stale diagrams found | 0 (no diagrams in existing code) |\n| Unresolved decisions | 0 |\n+====================================================================+\n```\n\n---\n\n## GSTACK REVIEW REPORT\n\n| Review | Trigger | Why | Runs | Status | Findings |\n|--------|---------|-----|------|--------|----------|\n| CEO Review | `/plan-ceo-review` | Scope & strategy | 1 | clean | mode: HOLD_SCOPE, 0 critical gaps |\n| Outside Review | disabled | Independent 2nd opinion | 0 | disabled | \u2014 |\n| Eng Review | `/plan-eng-review` | Architecture & tests (required) | 0 | \u2014 | \u2014 |\n| Design Review | `/plan-design-review` | UI/UX gaps | 0 | \u2014 | \u2014 |\n| DX Review | `/plan-devex-review` | Developer experience gaps | 0 | \u2014 | \u2014 |\n\n**OUTSIDE COVERAGE:** codex_reviews disabled. No outside review run. Re-enable: `gstack-config set codex_reviews enabled`.\n\n**VERDICT:** CEO CLEARED \u2014 3 specification gaps resolved, 0 critical gaps. Eng review required before shipping.\n\nNO UNRESOLVED DECISIONS\n", + "successfulResult": { + "timestamp": "2026-09-09T00:23:13.042Z", + "isError": false, + "content": "File created successfully at: /tmp/gstack-paid-shard-Q1hRY6/tmp/gstack-e2e-plan-ceo-paired-lEplIi/gstack-test-plan-ceo-paired.md (file state is current in your context \u2014 no need to Read it back)" + } + }, + "planReadyRequests": [ + { + "sessionId": "5036f6f6-8360-4f76-bea8-417f5c727b32", + "toolUseId": "toolu_01HLTX6zXpbrSuRWv1u3hKAR", + "timestamp": "2026-09-09T00:24:23.441Z", + "failed": false + } + ], + "distinctRetry": { + "calls": [ + { + "sessionId": "89297a6b-6a00-4308-8802-75c287f45046", + "toolUseId": "toolu_013YFPiqij263H7pMNfX8hau", + "questions": [ + { + "question": "Add gstack skill routing rules to this project's CLAUDE.md? ", + "header": "Routing rules", + "multiSelect": false, + "options": [ + { + "label": "Add routing rules (recommended)", + "description": "Appends skill routing rules to CLAUDE.md so gstack automatically suggests the right skill for each task type (plan review, QA, ship, etc.) without needing to type the slash command." + }, + { + "label": "No thanks, invoke manually", + "description": "Skip routing rules. You'll invoke skills manually with slash commands each time." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "Add gstack skill routing rules to this project's CLAUDE.md? ": "Add routing rules (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T00:27:11.829Z" + }, + { + "sessionId": "89297a6b-6a00-4308-8802-75c287f45046", + "toolUseId": "toolu_01GbewHUBvWHxXvUVdRGNf1e", + "questions": [ + { + "question": "D2 \u2014 No design doc found for this branch. Run /office-hours first, or proceed directly to the plan review? ", + "header": "office-hours?", + "multiSelect": false, + "options": [ + { + "label": "Skip \u2014 review the plan as-is (recommended)", + "description": "You already have PLAN.md written. Proceed with the CEO review directly. Best when the problem is well-understood and the plan is ready for scrutiny." + }, + { + "label": "Run /office-hours first", + "description": "Run /office-hours to produce a structured problem statement and explored alternatives before reviewing. Useful when you want sharper input but adds ~10 minutes upfront." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 \u2014 No design doc found for this branch. Run /office-hours first, or proceed directly to the plan review? ": "Skip \u2014 review the plan as-is (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T00:27:33.912Z" + }, + { + "sessionId": "89297a6b-6a00-4308-8802-75c287f45046", + "toolUseId": "toolu_01PZULs2ng16FwGgSHrH91ZP", + "questions": [ + { + "question": "D3 \u2014 Which implementation approach for the Stripe webhook handler? ", + "header": "Approach", + "multiSelect": false, + "options": [ + { + "label": "A) Minimal Secure Fix (recommended)", + "description": "Parameterized queries, email error handling, basic tests, N+1 fix. Smallest complete-secure diff. Retains the namespace-separation architecture. Human ~1 day / CC ~15 min. Completeness: 7/10." + }, + { + "label": "B) Ideal Architecture", + "description": "Async email via job queue, comprehensive tests at every layer, batch queries, WebhookDispatcher evaluation. Production-grade from day one. Human ~3 days / CC ~45 min. Completeness: 10/10." + }, + { + "label": "C) Integrated via WebhookDispatcher", + "description": "Route through existing WebhookDispatcher instead of bypassing it. Fixes all security and testing gaps while staying close to existing patterns. Human ~1.5 days / CC ~20 min. Completeness: 8/10." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 \u2014 Which implementation approach for the Stripe webhook handler? ": "A) Minimal Secure Fix (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T00:28:12.095Z" + }, + { + "sessionId": "89297a6b-6a00-4308-8802-75c287f45046", + "toolUseId": "toolu_015J3yUJCKkX2QCgyjj83xP5", + "questions": [ + { + "question": "D4 \u2014 Review mode for the Stripe webhook handler plan? ", + "header": "Review mode", + "multiSelect": false, + "options": [ + { + "label": "HOLD SCOPE \u2014 make it bulletproof (recommended)", + "description": "The plan has a critical SQL injection bug and missing tests. Hold scope and review with maximum rigor: security, error paths, edge cases, observability. No expansions surfaced.", + "preview": "Mode: HOLD SCOPE\nPlan has SQL injection. Make it secure,\ntested, and correct. No expansions." + }, + { + "label": "SELECTIVE EXPANSION \u2014 fix + cherry-pick improvements", + "description": "Hold the minimal-secure baseline but also surface expansion opportunities (async email, richer observability) as individual opt-in decisions.", + "preview": "Mode: SELECTIVE EXPANSION\nFix critical bugs, then surface optional\nimprovements one at a time." + }, + { + "label": "SCOPE EXPANSION \u2014 dream bigger", + "description": "Push scope up. Envision a 10x better webhook processing system. Each expansion is an explicit opt-in decision.", + "preview": "Mode: SCOPE EXPANSION\nRethink the whole webhook architecture.\nWhat would a 10x system look like?" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 \u2014 Review mode for the Stripe webhook handler plan? ": "HOLD SCOPE \u2014 make it bulletproof (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T00:28:22.136Z" + }, + { + "sessionId": "89297a6b-6a00-4308-8802-75c287f45046", + "toolUseId": "toolu_01KG7LpP7rxLGnHChP5Yz5if", + "questions": [ + { + "question": "D5 \u2014 Section 2: Email fires inline with no exception handling, creating a data-integrity risk ", + "header": "Email errors", + "multiSelect": false, + "options": [ + { + "label": "A) Catch email exceptions, log, continue (recommended)", + "description": "Wrap the email send in try/catch. On failure: log userId, event ID, and the exception with full context. Return HTTP 200. The webhook succeeds; email is best-effort. No duplicate DB writes. Verify: test that a simulated email failure does not trigger HTTP 500 or a second DB update. Human ~30min / CC ~5min." + }, + { + "label": "B) Remove inline email; add TODOS.md item for async email", + "description": "Ship the webhook handler now without the notification email. Add a TODOS.md item to wire up async email separately. Eliminates the error entirely; no email at launch. Human ~15min / CC ~5min." + }, + { + "label": "C) Async email via job queue", + "description": "Enqueue the email as a background job after DB commit. Email failures don't affect webhook processing or dedup recording. Correct architecture, but expands scope beyond Approach A. Human ~1 day / CC ~20min." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D5 \u2014 Section 2: Email fires inline with no exception handling, creating a data-integrity risk ": "A) Catch email exceptions, log, continue (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T00:32:18.928Z" + }, + { + "sessionId": "89297a6b-6a00-4308-8802-75c287f45046", + "toolUseId": "toolu_01GytSPkm6VPeam3qPaQqAL4", + "questions": [ + { + "question": "D6 \u2014 Section 3: SQL injection \u2014 userId from Stripe metadata is interpolated directly into raw SQL ", + "header": "SQL injection", + "multiSelect": false, + "options": [ + { + "label": "A) Parameterized query (recommended)", + "description": "Replace raw SQL interpolation with a bind parameter: `WHERE id = ?` (or `:userId`). The DB driver handles escaping. One-line fix. Verify: test with userId = \"1 OR 1=1\" and confirm it returns no results (no match). Completeness: 9/10.", + "preview": "-- Before (UNSAFE)\nSELECT * FROM users WHERE id = '#{userId}'\n\n-- After (safe)\nSELECT * FROM users WHERE id = ?\n-- bind: [userId]" + }, + { + "label": "B) ORM-safe lookup method", + "description": "Replace raw SQL with the ORM's safe finder (e.g. User.find_by(id: userId) in Rails, or equivalent). The ORM handles parameterization automatically. Same safety as A; abstraction is cleaner. Completeness: 9/10.", + "preview": "# Before (UNSAFE)\ndb.execute(\"SELECT * FROM users WHERE id = '#{userId}'\")\n\n# After (safe ORM)\nUser.find_by(id: userId)" + }, + { + "label": "C) Input validation + parameterized query", + "description": "Validate userId is a safe format (numeric, UUID) before executing ANY SQL. Reject and log if invalid. Plus use parameterized query. Defense in depth: two independent layers. Slightly more code but maximum safety. Completeness: 10/10.", + "preview": "# Validate first\nraise InvalidParam unless userId.match?(/\\A\\d+\\z/)\n# Then parameterized query\nUser.find_by(id: userId)" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D6 \u2014 Section 3: SQL injection \u2014 userId from Stripe metadata is interpolated directly into raw SQL ": "A) Parameterized query (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T00:32:43.068Z" + }, + { + "sessionId": "89297a6b-6a00-4308-8802-75c287f45046", + "toolUseId": "toolu_01G6ZjfH55xcHTTBf5MWwAgS", + "questions": [ + { + "question": "D7 \u2014 Section 6: Zero tests planned for new payment processing code ", + "header": "Test coverage", + "multiSelect": false, + "options": [ + { + "label": "A) Minimum viable integration test suite (recommended)", + "description": "Add 6-8 integration tests targeting the webhook endpoint directly: happy path, unknown user, email failure (no duplicate), duplicate event delivery (idempotency), SQL injection rejection, DB failure (returns 500). Verify tests pass before ship. Human ~2h / CC ~15min. Completeness: 7/10." + }, + { + "label": "B) Unit tests for the handler class only", + "description": "Test StripePaymentWebhookHandler in isolation with mocked DB and email. Faster to write but won't catch integration issues between the handler and the existing guards (dedup, lock). Human ~1h / CC ~10min. Completeness: 6/10." + }, + { + "label": "C) Unit tests + integration tests (full coverage)", + "description": "Both unit tests for the handler class and integration tests through the full endpoint. Covers the most ground; this is what we'd want at 2am on a Friday. Human ~4h / CC ~30min. Completeness: 9/10." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D7 \u2014 Section 6: Zero tests planned for new payment processing code ": "A) Minimum viable integration test suite (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T00:33:03.193Z" + }, + { + "sessionId": "89297a6b-6a00-4308-8802-75c287f45046", + "toolUseId": "toolu_01NmaABLks71Q4V4cVvC6bZz", + "questions": [ + { + "question": "D8 \u2014 Section 7: N+1 query \u2014 handler fetches each order in a loop ", + "header": "N+1 query", + "multiSelect": false, + "options": [ + { + "label": "A) Fix now: replace loop with batch query (recommended)", + "description": "Replace the per-order DB fetch loop with a single batch query (e.g. WHERE user_id = ? or using an ORM include/preload). One-line change. Adds the DB index check: confirm the user lookup column is indexed. Human ~30min / CC ~5min." + }, + { + "label": "B) Defer to TODOS.md", + "description": "Log the N+1 as a known performance issue. At current payment volume it likely doesn't matter. Fix it when query time shows up in monitoring. Risk: if orders-per-user is large or payment volume is high, this degrades under load." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D8 \u2014 Section 7: N+1 query \u2014 handler fetches each order in a loop ": "A) Fix now: replace loop with batch query (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T00:33:23.321Z" + }, + { + "sessionId": "89297a6b-6a00-4308-8802-75c287f45046", + "toolUseId": "toolu_0198YPJhdTFWmJqGPqVNxsrD", + "questions": [ + { + "question": "D9 \u2014 What's the next step after this CEO review? ", + "header": "Next step", + "multiSelect": false, + "options": [ + { + "label": "Run /plan-eng-review next (recommended)", + "description": "Eng Review is the required shipping gate. It covers architecture, code quality, test adequacy, and performance. This CEO review resolved the security and correctness bugs; eng review validates the implementation approach. Run it before writing any code." + }, + { + "label": "Skip \u2014 handle reviews manually", + "description": "Take the implementation tasks (T1-T6) from this review and implement directly. Run /plan-eng-review separately when ready." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D9 \u2014 What's the next step after this CEO review? ": "Run /plan-eng-review next (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T00:37:26.587Z" + } + ], + "reportContent": "# CEO Plan Review: Payment Processing Integration\n\n**Date:** 2026-09-09 \n**Mode:** HOLD SCOPE \n**Approach:** A \u2014 Minimal Secure Fix \n**Branch:** main \n**Reviewer:** gstack /plan-ceo-review\n\n---\n\n## Context\n\nThe plan adds a `StripePaymentWebhookHandler` to process `payment_intent.succeeded` events\nfrom Stripe. On success it looks up the user, updates the user record, and fires a notification\nemail. Several existing guards (signature verification, event dedup, per-user transaction lock)\nare retained unchanged.\n\n**Why this review:** The plan has a critical SQL injection vulnerability, no email error\nhandling, and zero tests. All three must be resolved before this ships.\n\n---\n\n## Step 0: Premise Challenge & Approach\n\n### 0A. Premise\nWebhook-driven payment confirmation is the right shape for this problem. Stripe's event model\nis the industry standard; building against it is correct. The \"do nothing\" cost is real: users\ndon't get payment confirmations and their records don't update.\n\n### 0B. Existing Code Leverage\nThe plan correctly retains existing guards:\n- Ingress middleware: Stripe signature verification (retained)\n- Event-type filter: only `payment_intent.succeeded` routed here (retained)\n- Payload adapter: exposes `event.data.object.metadata.user_id` as `request.params.userId`\n- Dedup guard: by Stripe event ID (retained)\n- Per-user transaction lock (retained)\n- Lookup-result guard: unknown/deleted users \u2192 HTTP 200 + log (retained)\n- Ingress logging: event IDs, outcomes, durations, alerts (retained)\n- Feature flag + rollback path (retained)\n\n**WebhookDispatcher bypass:** The plan introduces `StripePaymentWebhookHandler` outside the\nexisting `WebhookDispatcher` module. Rationale given: \"clean namespace separation.\" Approach A\nretains this decision; it is a WARNING but not a correctness bug.\n\n### 0C. Dream State (12-month arc)\n```\nCURRENT STATE THIS PLAN (as written) 12-MONTH IDEAL\nNo webhook handler ---> Handler with SQL injection --> Secure handler,\n no email error handling async email,\n no tests, N+1 query full test coverage,\n batch queries\n```\n\n### Implementation Alternatives Considered\n| Approach | Effort | Coverage |\n|----------|--------|----------|\n| A: Minimal Secure Fix (SELECTED) | S | 7/10 |\n| B: Ideal Architecture (async email, batch queries) | L | 10/10 |\n| C: Integrate via WebhookDispatcher | M | 8/10 |\n\n### 0D. Complexity Check (HOLD SCOPE)\nThe plan touches: `StripePaymentWebhookHandler` (new class), DB access layer, email\nnotification. Small footprint \u2014 no complexity smell.\n\n---\n\n## Architecture Diagram\n\n```\nStripe Webhook POST /webhook\n \u2502\n \u25bc\n[Ingress Middleware]\n \u251c\u2500 Verify Stripe signature (raw body) \u2500\u2500\u2500\u2500 invalid \u2192 400\n \u251c\u2500 Log event ID + timestamp\n \u2514\u2500 Filter: payment_intent.succeeded? \u2500\u2500 other types \u2192 200 ack\n \u2502\n \u25bc\n[Event Dedup Guard]\n \u251c\u2500 Already processed? \u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500 yes \u2192 200 ack\n \u2514\u2500 No \u2192 proceed\n \u2502\n \u25bc\n[Per-User Transaction Lock] (serialize per userId)\n \u2502\n \u25bc\n[StripePaymentWebhookHandler] \u25c4\u2500\u2500 NEW CLASS (bypasses WebhookDispatcher)\n \u2502\n \u251c\u2500 DB lookup: raw SQL with request.params.userId \u25c4\u2500\u2500 \u26a0\ufe0f SQL INJECTION\n \u2502 \u251c\u2500 User not found \u2192 200 ack + log\n \u2502 \u2514\u2500 User found \u2192\n \u2502 \u251c\u2500 Update user record (in transaction)\n \u2502 \u251c\u2500 Fire notification email \u25c4\u2500\u2500 \u26a0\ufe0f NO ERROR HANDLING\n \u2502 \u2502 \u2514\u2500 Email failure \u2192 exception propagates \u2192 HTTP 500\n \u2502 \u2514\u2500 (return HTTP 200 if email succeeds)\n \u2502\n \u2514\u2500 DB exception \u2192 propagates to ingress \u2192 HTTP 500 \u2192 Stripe retries\n \u2502\n \u25bc\n[Dedup Guard records completion] \u25c4\u2500\u2500 only after DB transaction commits\n (email fires BEFORE this records)\n```\n\n**Data flow shadow paths for userId:**\n- Happy path: valid userId \u2192 parameterized query \u2192 user found \u2192 update + email\n- Nil path: userId is null \u2192 raw SQL with NULL \u2192 behavior undefined / injection risk\n- Empty path: userId is `\"\"` \u2192 raw SQL with empty string \u2192 SQL syntax error or injection\n- Malicious path: userId is `\"1 OR 1=1\"` \u2192 raw SQL \u2192 table scan / data exfiltration\n\n---\n\n## Section 1: Architecture Review\n\n**Architecture diagram:** produced above.\n\n**WebhookDispatcher bypass:** WARNING \u2014 Approach A retains this; justification (\"namespace\nseparation\") is weak but not fatal. The new handler runs in its own namespace and the existing\nguards still wrap it correctly. No decision required \u2014 this is the accepted scope.\n\n**Email-dedup ordering (DATA INTEGRITY RISK):**\nThe email fires inline BEFORE the dedup guard records completion. If email throws an exception:\n- The dedup guard has NOT recorded the event as complete\n- The ingress wrapper returns HTTP 500\n- Stripe retries the event\n- On retry, dedup sees the event as unprocessed\n- The user record gets updated AGAIN (duplicate payment processing)\n\nThis is a data integrity bug, not just an error handling gap. Fix: either (a) wrap email in\ntry/catch and swallow/log failures, or (b) move dedup recording before the email call.\nBoth are addressed by adding error handling on the email leg (Section 2 finding).\n\n**Rollback posture:** Covered \u2014 existing feature flag + documented rollback.\n\n**Single points of failure:** Email service. If email provider is down, every webhook event\nfails and retries (with duplicate DB writes on each retry). Fix: error handling on email.\n\n---\n\n## Section 2: Error & Rescue Map\n\n### Failure table\n\n| Method/Codepath | What can go wrong | Exception class |\n|-----------------|-------------------|-----------------|\n| DB lookup (raw SQL) | SQL injection (attacker input) | DB executes injected SQL |\n| DB lookup | DB connection failure | DBConnectionError |\n| DB lookup | User not found | (handled: existing lookup-result guard) |\n| DB update | Transaction conflict | DeadlockError / LockTimeout |\n| DB update | Connection failure | DBConnectionError |\n| Email send | SMTP/provider failure | SMTPError / NetworkError |\n| Email send | Invalid address | AddressError |\n| Email send | Provider rate limit | RateLimitError |\n\n| Exception class | Rescued? | Rescue action | User sees |\n|-----------------|----------|---------------|-----------|\n| DBConnectionError (lookup) | YES | Propagates to ingress \u2192 HTTP 500 \u2192 Stripe retries | Nothing (retry transparent) |\n| DBConnectionError (update) | YES | Propagates to ingress \u2192 HTTP 500 \u2192 Stripe retries | Nothing (retry transparent) |\n| DeadlockError | YES | Propagates to ingress \u2192 HTTP 500 \u2192 Stripe retries | Nothing (retry transparent) |\n| SMTPError | NO \u2190 **CRITICAL GAP** | \u2014 | Duplicate payment processing or silent email loss |\n| NetworkError (email) | NO \u2190 **CRITICAL GAP** | \u2014 | Same as above |\n| AddressError | NO \u2190 **CRITICAL GAP** | \u2014 | Same as above |\n| Injected SQL | NO \u2190 **CRITICAL GAP** | Attacker controls DB query | Data exfiltration / corruption |\n\n---\n\n## Section 3: Security & Threat Model\n\n### SQL Injection (CRITICAL)\n\n| Threat | Likelihood | Impact | Mitigated? |\n|--------|-----------|--------|-----------|\n| SQL injection via userId | HIGH | HIGH | NO |\n\nThe plan explicitly states: \"The adapter forwards that external string unchanged. It does not\ncast, escape, or SQL-sanitize it; a valid signature does not make it safe for SQL.\" Then\nimmediately proposes: \"The new endpoint reads `request.params.userId` directly into a raw SQL\nfragment for the lookup query.\"\n\nA valid Stripe signature proves the payload came from Stripe but NOT that `userId` is safe.\nAny actor who can create Stripe payment intents (which includes internal systems and potentially\nexternal attackers if the payment intent creation API has any vulnerability) can set `userId`\nto an arbitrary string, including `\"; DROP TABLE users; --\"`.\n\n**Fix:** Use parameterized queries. One line change.\n\n---\n\n## Section 4: Data Flow & Interaction Edge Cases\n\n```\nuserId (from Stripe metadata)\n \u2502\n \u251c\u2500 VALIDATION: None \u25c4\u2500\u2500 \u26a0\ufe0f GAP (no length check, no type check, no SQL sanitization)\n \u2502\n \u25bc\nRaw SQL fragment \u25c4\u2500\u2500 \u26a0\ufe0f SQL INJECTION VECTOR\n \u2502\n \u251c\u2500 nil input \u2192 SQL error (or \"IS NULL\" match hitting wrong user if not guarded)\n \u251c\u2500 empty string \u2192 SQL error or empty-match\n \u251c\u2500 injected SQL \u2192 attacker controls DB\n \u2514\u2500 valid userId \u2192 user lookup\n \u2502\n \u25bc\n User found?\n \u251c\u2500 No \u2192 HTTP 200 + log (handled)\n \u2514\u2500 Yes \u2192\n \u25bc\n DB update (in transaction)\n \u2502\n \u25bc\n Email send (inline, no try/catch)\n \u251c\u2500 Success \u2192 HTTP 200\n \u2514\u2500 Failure \u2192 exception \u2192 HTTP 500 \u2192 Stripe retries \u2192 DUPLICATE DB UPDATE\n```\n\n**Webhook ordering edge case:**\nIf Stripe delivers the same event twice concurrently (network duplicate), the per-user\ntransaction lock serializes them. The second delivery hits the dedup guard and is blocked.\nThis is handled.\n\n**Email-before-dedup edge case (DATA INTEGRITY):**\nIf email fails after DB commit but before dedup records: Stripe retries \u2192 dedup not yet\nrecorded \u2192 duplicate DB update. Addressed by email error handling fix.\n\n---\n\n## Section 5: Code Quality Review\n\nThe plan describes behavior rather than showing code, so this evaluates plan-level quality.\n\n- **Under-engineering:** Glaring \u2014 raw SQL with unsanitized input, no error handling, no tests.\n- **Over-engineering:** None visible.\n- **Naming:** `StripePaymentWebhookHandler` is clear and appropriate.\n- **DRY concern:** Bypassing WebhookDispatcher creates a parallel webhook handling path. Future\n webhook handlers may replicate this pattern. Not a blocker at current scale.\n- **Missing defensive checks:** No validation of userId before SQL. No length/type check. No\n null guard.\n\n---\n\n## Section 6: Test Review\n\n### New flows introduced\n\n| New thing | Test type needed | In plan? | Gap |\n|-----------|-----------------|----------|-----|\n| StripePaymentWebhookHandler happy path (valid event, known user) | Integration | NO | CRITICAL GAP |\n| Unknown/deleted user \u2192 200 + log | Integration | NO | CRITICAL GAP |\n| SQL injection attempt (malicious userId) | Security test | NO | CRITICAL GAP |\n| Email send failure \u2192 user record still updated, no duplicate | Integration | NO | CRITICAL GAP |\n| DB lookup failure \u2192 HTTP 500 \u2192 retriable | Integration | NO | CRITICAL GAP |\n| DB update failure \u2192 HTTP 500 \u2192 retriable | Integration | NO | CRITICAL GAP |\n| Duplicate Stripe delivery \u2192 dedup blocks second | Integration | NO | CRITICAL GAP |\n| Concurrent deliveries \u2192 per-user lock serializes | Integration | NO | CRITICAL GAP |\n\n**Plan says:** \"None planned. We'll rely on the existing integration suite catching regressions.\"\n\nThis is not acceptable for payment processing code. The existing integration suite doesn't know\nabout this new handler and cannot catch regressions in it.\n\n**Minimum required test specs:**\n```\ndescribe StripePaymentWebhookHandler do\n it \"updates user record on valid payment_intent.succeeded\"\n it \"returns HTTP 200 for unknown user without updating anything\"\n it \"handles email failure without duplicate DB write\"\n it \"is idempotent (duplicate event delivery produces no duplicate update)\"\n it \"uses parameterized queries (rejects SQL injection attempt)\"\n it \"returns HTTP 500 on DB failure so Stripe retries\"\nend\n```\n\n---\n\n## Section 7: Performance Review\n\n**N+1 query (WARNING):**\n\"Each webhook lookup hits the database for the user, then fetches each order in a loop.\"\n\nFor a user with N orders, this is N+1 DB queries per webhook event. At low volume this is\nfine. At scale (high payment volume \u00d7 many orders per user), this creates database load.\n\n**Fix:** Replace order loop with `WHERE user_id = ? JOIN orders` or a batch `WHERE id IN (?)`.\nEstimated effort: human ~30 min / CC ~5 min.\n\n**DB index check:**\nThe user lookup query (by `userId` from Stripe metadata) must hit an indexed column. If\n`users.stripe_customer_id` or `users.id` is the lookup key, those are likely indexed. But if\nthe raw SQL is doing `WHERE some_column = ''` on an unindexed column, every webhook\nis a full table scan.\n\n**Connection pool:** No new connections beyond the DB lookup and update. No concern.\n\n---\n\n## Section 8: Observability & Debuggability Review\n\n**What's already there (retained):**\n- Ingress wrapper logs: event IDs, outcomes, durations\n- Alerts for failed webhook processing\n- DB lookup/update exceptions propagate to ingress logging\n\n**Gaps:**\n- No structured log line at the START of handler execution (before DB lookup) \u2014 if DB hangs,\n there's no log showing the handler was entered\n- No log of which userId was looked up (important for debugging \"why didn't user X get\n updated?\") \u2014 though this may be in the existing ingress logs\n- No metric for email send success/failure rate\n- No runbook for \"email send keeps failing and all webhooks are stuck retrying\"\n\nFor HOLD SCOPE, the observability from the existing ingress wrapper is probably sufficient for\nMVP. The critical gap is that email failures are currently invisible \u2014 covered by the error\nhandling fix in Section 2.\n\n---\n\n## Section 9: Deployment & Rollout Review\n\n**Feature flag:** Existing. Good.\n**Rollback:** Documented, tested. Good.\n**DB migrations:** None required for this change. Good.\n**Rollout order:** Single service change; no ordering concern.\n**Deploy-time risk window:** Old handler and new handler never run simultaneously (feature flag\ngates it). Clean cutover.\n\n**Post-deploy verification:**\n- Send a test Stripe webhook event (Stripe CLI: `stripe trigger payment_intent.succeeded`)\n- Verify user record updates\n- Verify notification email sends\n- Check ingress logs for any errors\n\nNo issues for HOLD SCOPE mode.\n\n---\n\n## Section 10: Long-Term Trajectory Review\n\n**Technical debt introduced (as currently written):**\n- SQL injection: critical security debt\n- No tests: all regressions invisible\n- N+1 query: performance debt at scale\n- Email inline with no handling: data integrity debt\n\n**If all fixes applied (Approach A):**\n- Debt introduced: WebhookDispatcher bypass creates a parallel webhook pattern. If a second\n handler is added later, it may replicate this pattern rather than routing through the\n dispatcher. Reversibility: 4/5 \u2014 easy to refactor into dispatcher later if needed.\n- Knowledge concentration: the bypass rationale (\"namespace separation\") should be documented\n in the class, not just the plan.\n- The 1-year question: A new engineer sees `StripePaymentWebhookHandler` outside the\n dispatcher. Without a comment explaining why, they'll wonder if it's a mistake.\n\n**Reversibility of the fixes:** 5/5 \u2014 parameterized queries, error handling, tests. All\nindividually revertable, none locked in.\n\n---\n\n## Section 11: Design & UX Review\n\nNo UI scope detected. SKIPPED.\n\n---\n\n## Outside Voice \u2014 Independent Plan Challenge\n\nCodex review skipped (codex_reviews disabled). Re-enable: `gstack-config set codex_reviews enabled`.\n\n---\n\n## Decisions Made\n\n| # | Section | Finding | Decision |\n|---|---------|---------|---------|\n| D5 | Section 2 | Email inline with no exception handling \u2192 data integrity risk | **A: Catch exceptions, log with context, return HTTP 200** |\n| D6 | Section 3 | SQL injection via userId in raw SQL fragment | **A: Parameterized query (`WHERE id = ?`, bind userId)** |\n| D7 | Section 6 | Zero tests for new payment processing code | **A: 6-8 integration tests (happy path, unknown user, email failure, idempotency, SQL injection rejection, DB failure)** |\n| D8 | Section 7 | N+1 query \u2014 fetches each order in a loop | **A: Fix now \u2014 batch query, confirm lookup column indexed** |\n\n---\n\n## Implementation Tasks\n\nSynthesized from this review's findings. Each task derives from a specific finding above.\nRun with Claude Code or Codex; checkbox as you ship.\n\n- [ ] **T1 (P1, human: ~30min / CC: ~5min)** \u2014 StripePaymentWebhookHandler \u2014 Replace raw SQL userId interpolation with parameterized query\n - Surfaced by: Section 3 \u2014 SQL injection: userId from Stripe metadata interpolated into raw SQL\n - Files: `app/handlers/stripe_payment_webhook_handler.rb`\n - Verify: test with `userId = \"1 OR 1=1\"` \u2014 must return no match; test with valid userId \u2014 must find user\n\n- [ ] **T2 (P1, human: ~30min / CC: ~5min)** \u2014 StripePaymentWebhookHandler \u2014 Wrap email send in try/catch; log userId + event ID + exception; return HTTP 200\n - Surfaced by: Section 2 \u2014 email failure causes HTTP 500, triggering duplicate DB writes on Stripe retry\n - Files: `app/handlers/stripe_payment_webhook_handler.rb`\n - Verify: simulate email failure \u2014 webhook returns HTTP 200, DB update not duplicated on re-delivery\n\n- [ ] **T3 (P1, human: ~2h / CC: ~15min)** \u2014 StripePaymentWebhookHandler \u2014 Add 6-8 integration tests\n - Surfaced by: Section 6 \u2014 zero tests; existing suite cannot catch regressions in new handler\n - Files: `spec/handlers/stripe_payment_webhook_handler_spec.rb` (or equivalent)\n - Tests required:\n - `it \"updates user record on valid payment_intent.succeeded\"`\n - `it \"returns HTTP 200 for unknown user without updating anything\"`\n - `it \"handles email failure without HTTP 500 or duplicate DB write\"`\n - `it \"is idempotent (duplicate event delivery produces no duplicate update)\"`\n - `it \"rejects SQL injection attempt in userId\"`\n - `it \"returns HTTP 500 on DB failure so Stripe retries\"`\n\n- [ ] **T4 (P2, human: ~30min / CC: ~5min)** \u2014 StripePaymentWebhookHandler \u2014 Replace per-order DB fetch loop with single batch query\n - Surfaced by: Section 7 \u2014 N+1 query\n - Files: `app/handlers/stripe_payment_webhook_handler.rb`\n - Verify: confirm 2 DB queries total (user lookup + orders batch) regardless of order count; confirm user lookup column is indexed\n\n- [ ] **T5 (P2, human: ~5min / CC: ~1min)** \u2014 StripePaymentWebhookHandler \u2014 Add comment explaining WebhookDispatcher bypass rationale\n - Surfaced by: Section 10 \u2014 bypass creates pattern debt for future engineers\n - Files: `app/handlers/stripe_payment_webhook_handler.rb`\n - Verify: comment explains \"bypasses WebhookDispatcher for namespace separation; this handler is scoped to StripePaymentWebhook namespace specifically\"\n\n- [ ] **T6 (P2, human: ~2min / CC: ~1min)** \u2014 CLAUDE.md \u2014 Add gstack skill routing rules and commit (post-plan-mode)\n - Surfaced by: GSTACK_INSTRUCTION routing-injection (user opted in)\n - Files: `CLAUDE.md`\n - Verify: routing rules appended; `git commit -m \"chore: add gstack skill routing rules to CLAUDE.md\"`\n\n---\n\n## NOT in Scope\n\n| Item | Rationale |\n|------|-----------|\n| Async email via job queue | Scope expansion beyond Approach A; best-effort sync email with error handling is sufficient for now |\n| WebhookDispatcher integration | Approach A explicitly retains namespace separation; can refactor later if a second handler is added |\n| Input format validation on userId | Defense in depth; parameterized query prevents injection; format validation is an optional enhancement |\n| Full unit + integration test suite | Approach A targets minimum viable (6-8 integration tests); expand if time permits |\n\n---\n\n## What Already Exists\n\n| Existing component | What it does | Plan reuses it? |\n|-------------------|-------------|----------------|\n| Ingress middleware (Stripe signature verify) | Verifies HMAC signature against raw body; rejects 400 on failure | YES |\n| Event-type filter | Routes only `payment_intent.succeeded` to this handler | YES |\n| Payload adapter | Exposes `event.data.object.metadata.user_id` as `request.params.userId` | YES |\n| Dedup guard (by Stripe event ID) | Prevents duplicate processing | YES |\n| Per-user transaction lock | Serializes concurrent updates for same user | YES |\n| Lookup-result guard | Returns HTTP 200 + log for unknown/deleted users | YES |\n| Ingress logging | Event IDs, outcomes, durations, failure alerts | YES |\n| Feature flag + rollback | Handler on/off flag; documented rollback | YES |\n| WebhookDispatcher | Existing webhook routing module | NO (bypassed by design) |\n\n---\n\n## Dream State Delta\n\n```\nTHIS PLAN (as written) AFTER ALL FIXES (T1-T5) 12-MONTH IDEAL\nSQL injection in userId ---> Parameterized query --> Async email,\nNo email error handling ---> catch + log + HTTP 200 batch queries,\nNo tests ---> 6-8 integration tests full test suite,\nN+1 order query ---> Batch query observability\n metrics/alerts\n```\n\nThis plan, once fixed, is production-safe and ships the right thing. The 12-month ideal\nadds async email and richer observability \u2014 those are TODOS.md items for a future sprint.\n\n---\n\n## Failure Modes Registry\n\n| Codepath | Failure mode | Rescued? | Test? | User sees | Logged? |\n|---------|-------------|---------|------|----------|--------|\n| DB lookup (raw SQL) | SQL injection | NO \u2190 **CRITICAL GAP** (fix: T1) | NO (fix: T3) | Silent data compromise | NO |\n| DB lookup | DB connection failure | YES (\u2192 HTTP 500 \u2192 Stripe retries) | NO (fix: T3) | Nothing (transparent) | YES (ingress) |\n| DB update | Transaction conflict | YES (\u2192 HTTP 500 \u2192 Stripe retries) | NO (fix: T3) | Nothing (transparent) | YES (ingress) |\n| Email send | SMTP/provider failure | NO \u2190 **CRITICAL GAP** (fix: T2) | NO (fix: T3) | Duplicate DB write | NO |\n| Email send | Network error | NO \u2190 **CRITICAL GAP** (fix: T2) | NO (fix: T3) | Duplicate DB write | NO |\n\nAfter T1 + T2 + T3 applied: all CRITICAL GAPsresolved.\n\n---\n\n```\n+====================================================================+\n| MEGA PLAN REVIEW \u2014 COMPLETION SUMMARY |\n+====================================================================+\n| Mode selected | HOLD SCOPE |\n| Approach selected | A \u2014 Minimal Secure Fix |\n| System Audit | Clean repo, 1 commit, PLAN.md only |\n| Step 0 | HOLD SCOPE; Approach A; WebhookDispatcher |\n| | bypass retained; all existing guards kept |\n| Section 1 (Arch) | 0 new issues (email timing \u2192 Sec 2) |\n| Section 2 (Errors) | 3 error paths mapped, 2 CRITICAL GAPS |\n| Section 3 (Security)| 1 CRITICAL: SQL injection (HIGH/HIGH) |\n| Section 4 (Data/UX) | Data flow traced, shadow paths mapped |\n| Section 5 (Quality) | Under-engineering flagged; covered by fixes |\n| Section 6 (Tests) | 8 gaps; 6-8 integration tests required |\n| Section 7 (Perf) | 1 issue: N+1 query (fix now: batch) |\n| Section 8 (Observ) | Existing ingress logging sufficient; email |\n| | failures now logged via T2 |\n| Section 9 (Deploy) | 0 risks; feature flag + rollback in place |\n| Section 10 (Future) | Reversibility: 4/5; bypass comment: T5 |\n| Section 11 (Design) | SKIPPED (no UI scope) |\n+--------------------------------------------------------------------+\n| NOT in scope | 4 items (async email, dispatcher, format |\n| | validation, expanded test suite) |\n| What already exists | 8 retained components listed |\n| Dream state delta | written |\n| Error/rescue registry| 5 methods, 2 CRITICAL GAPS (resolved T1+T2)|\n| Failure modes | 5 total, 2 CRITICAL GAPS \u2192 resolved by T1+T2|\n| TODOS.md updates | 0 items (all findings = fix now) |\n| Scope proposals | 0 proposed (HOLD SCOPE) |\n| CEO plan | skipped (HOLD SCOPE) |\n| Outside voice | skipped (codex_reviews disabled) |\n| Diagrams produced | 4 (system arch, data flow, shadow paths, |\n| | failure modes registry) |\n| Stale diagrams found | 0 (no prior diagrams in repo) |\n| Unresolved decisions | 0 |\n+====================================================================+\n```\n\n---\n\n## Next Steps\n\n1. Run `/plan-eng-review` \u2014 required shipping gate (architecture + code quality + tests)\n2. Implement T1-T6 (all P1s block ship)\n3. After implementation, run `/review` (diff review) before opening PR\n\n---\n\n## GSTACK REVIEW REPORT\n\n| Review | Trigger | Why | Runs | Status | Findings |\n|--------|---------|-----|------|--------|----------|\n| CEO Review | `/plan-ceo-review` | Scope & strategy | 1 | issues_open | mode: HOLD_SCOPE, 2 critical gaps (SQL injection, email error handling) \u2014 all resolved via T1-T3 |\n| Outside Review | disabled | Independent 2nd opinion | 0 | disabled | codex_reviews=disabled |\n| Eng Review | `/plan-eng-review` | Architecture & tests (required) | 0 | \u2014 | not yet run |\n| Design Review | `/plan-design-review` | UI/UX gaps | 0 | \u2014 | not yet run (no UI scope) |\n| DX Review | `/plan-devex-review` | Developer experience gaps | 0 | \u2014 | not yet run |\n\n**OUTSIDE COVERAGE:** codex disabled (codex_reviews=disabled). No native fallback dispatched per disabled branch rules.\n\n**VERDICT:** CEO review complete with 4 findings \u2014 all resolved. Eng review required before ship.\n\nNO UNRESOLVED DECISIONS\n", + "reportUpdate": [ + { + "timestamp": "2026-09-09T00:36:22.639Z", + "id": "toolu_01URf23uR37vCphgbewjWMa6", + "name": "Edit", + "input": { + "replace_all": false, + "file_path": "/tmp/gstack-paid-shard-Q1hRY6/tmp/gstack-e2e-plan-ceo-dM1qj1/gstack-test-plan-ceo.md", + "old_string": "## Outside Voice \u2014 Independent Plan Challenge\n\n", + "new_string": "## Outside Voice \u2014 Independent Plan Challenge\n\nCodex review skipped (codex_reviews disabled). Re-enable: `gstack-config set codex_reviews enabled`.\n\n---\n\n## Decisions Made\n\n| # | Section | Finding | Decision |\n|---|---------|---------|---------|\n| D5 | Section 2 | Email inline with no exception handling \u2192 data integrity risk | **A: Catch exceptions, log with context, return HTTP 200** |\n| D6 | Section 3 | SQL injection via userId in raw SQL fragment | **A: Parameterized query (`WHERE id = ?`, bind userId)** |\n| D7 | Section 6 | Zero tests for new payment processing code | **A: 6-8 integration tests (happy path, unknown user, email failure, idempotency, SQL injection rejection, DB failure)** |\n| D8 | Section 7 | N+1 query \u2014 fetches each order in a loop | **A: Fix now \u2014 batch query, confirm lookup column indexed** |\n\n---\n\n## Implementation Tasks\n\nSynthesized from this review's findings. Each task derives from a specific finding above.\nRun with Claude Code or Codex; checkbox as you ship.\n\n- [ ] **T1 (P1, human: ~30min / CC: ~5min)** \u2014 StripePaymentWebhookHandler \u2014 Replace raw SQL userId interpolation with parameterized query\n - Surfaced by: Section 3 \u2014 SQL injection: userId from Stripe metadata interpolated into raw SQL\n - Files: `app/handlers/stripe_payment_webhook_handler.rb`\n - Verify: test with `userId = \"1 OR 1=1\"` \u2014 must return no match; test with valid userId \u2014 must find user\n\n- [ ] **T2 (P1, human: ~30min / CC: ~5min)** \u2014 StripePaymentWebhookHandler \u2014 Wrap email send in try/catch; log userId + event ID + exception; return HTTP 200\n - Surfaced by: Section 2 \u2014 email failure causes HTTP 500, triggering duplicate DB writes on Stripe retry\n - Files: `app/handlers/stripe_payment_webhook_handler.rb`\n - Verify: simulate email failure \u2014 webhook returns HTTP 200, DB update not duplicated on re-delivery\n\n- [ ] **T3 (P1, human: ~2h / CC: ~15min)** \u2014 StripePaymentWebhookHandler \u2014 Add 6-8 integration tests\n - Surfaced by: Section 6 \u2014 zero tests; existing suite cannot catch regressions in new handler\n - Files: `spec/handlers/stripe_payment_webhook_handler_spec.rb` (or equivalent)\n - Tests required:\n - `it \"updates user record on valid payment_intent.succeeded\"`\n - `it \"returns HTTP 200 for unknown user without updating anything\"`\n - `it \"handles email failure without HTTP 500 or duplicate DB write\"`\n - `it \"is idempotent (duplicate event delivery produces no duplicate update)\"`\n - `it \"rejects SQL injection attempt in userId\"`\n - `it \"returns HTTP 500 on DB failure so Stripe retries\"`\n\n- [ ] **T4 (P2, human: ~30min / CC: ~5min)** \u2014 StripePaymentWebhookHandler \u2014 Replace per-order DB fetch loop with single batch query\n - Surfaced by: Section 7 \u2014 N+1 query\n - Files: `app/handlers/stripe_payment_webhook_handler.rb`\n - Verify: confirm 2 DB queries total (user lookup + orders batch) regardless of order count; confirm user lookup column is indexed\n\n- [ ] **T5 (P2, human: ~5min / CC: ~1min)** \u2014 StripePaymentWebhookHandler \u2014 Add comment explaining WebhookDispatcher bypass rationale\n - Surfaced by: Section 10 \u2014 bypass creates pattern debt for future engineers\n - Files: `app/handlers/stripe_payment_webhook_handler.rb`\n - Verify: comment explains \"bypasses WebhookDispatcher for namespace separation; this handler is scoped to StripePaymentWebhook namespace specifically\"\n\n- [ ] **T6 (P2, human: ~2min / CC: ~1min)** \u2014 CLAUDE.md \u2014 Add gstack skill routing rules and commit (post-plan-mode)\n - Surfaced by: GSTACK_INSTRUCTION routing-injection (user opted in)\n - Files: `CLAUDE.md`\n - Verify: routing rules appended; `git commit -m \"chore: add gstack skill routing rules to CLAUDE.md\"`\n\n---\n\n## NOT in Scope\n\n| Item | Rationale |\n|------|-----------|\n| Async email via job queue | Scope expansion beyond Approach A; best-effort sync email with error handling is sufficient for now |\n| WebhookDispatcher integration | Approach A explicitly retains namespace separation; can refactor later if a second handler is added |\n| Input format validation on userId | Defense in depth; parameterized query prevents injection; format validation is an optional enhancement |\n| Full unit + integration test suite | Approach A targets minimum viable (6-8 integration tests); expand if time permits |\n\n---\n\n## What Already Exists\n\n| Existing component | What it does | Plan reuses it? |\n|-------------------|-------------|----------------|\n| Ingress middleware (Stripe signature verify) | Verifies HMAC signature against raw body; rejects 400 on failure | YES |\n| Event-type filter | Routes only `payment_intent.succeeded` to this handler | YES |\n| Payload adapter | Exposes `event.data.object.metadata.user_id` as `request.params.userId` | YES |\n| Dedup guard (by Stripe event ID) | Prevents duplicate processing | YES |\n| Per-user transaction lock | Serializes concurrent updates for same user | YES |\n| Lookup-result guard | Returns HTTP 200 + log for unknown/deleted users | YES |\n| Ingress logging | Event IDs, outcomes, durations, failure alerts | YES |\n| Feature flag + rollback | Handler on/off flag; documented rollback | YES |\n| WebhookDispatcher | Existing webhook routing module | NO (bypassed by design) |\n\n---\n\n## Dream State Delta\n\n```\nTHIS PLAN (as written) AFTER ALL FIXES (T1-T5) 12-MONTH IDEAL\nSQL injection in userId ---> Parameterized query --> Async email,\nNo email error handling ---> catch + log + HTTP 200 batch queries,\nNo tests ---> 6-8 integration tests full test suite,\nN+1 order query ---> Batch query observability\n metrics/alerts\n```\n\nThis plan, once fixed, is production-safe and ships the right thing. The 12-month ideal\nadds async email and richer observability \u2014 those are TODOS.md items for a future sprint.\n\n---\n\n## Failure Modes Registry\n\n| Codepath | Failure mode | Rescued? | Test? | User sees | Logged? |\n|---------|-------------|---------|------|----------|--------|\n| DB lookup (raw SQL) | SQL injection | NO \u2190 **CRITICAL GAP** (fix: T1) | NO (fix: T3) | Silent data compromise | NO |\n| DB lookup | DB connection failure | YES (\u2192 HTTP 500 \u2192 Stripe retries) | NO (fix: T3) | Nothing (transparent) | YES (ingress) |\n| DB update | Transaction conflict | YES (\u2192 HTTP 500 \u2192 Stripe retries) | NO (fix: T3) | Nothing (transparent) | YES (ingress) |\n| Email send | SMTP/provider failure | NO \u2190 **CRITICAL GAP** (fix: T2) | NO (fix: T3) | Duplicate DB write | NO |\n| Email send | Network error | NO \u2190 **CRITICAL GAP** (fix: T2) | NO (fix: T3) | Duplicate DB write | NO |\n\nAfter T1 + T2 + T3 applied: all CRITICAL GAPsresolved.\n\n---\n\n```\n+====================================================================+\n| MEGA PLAN REVIEW \u2014 COMPLETION SUMMARY |\n+====================================================================+\n| Mode selected | HOLD SCOPE |\n| Approach selected | A \u2014 Minimal Secure Fix |\n| System Audit | Clean repo, 1 commit, PLAN.md only |\n| Step 0 | HOLD SCOPE; Approach A; WebhookDispatcher |\n| | bypass retained; all existing guards kept |\n| Section 1 (Arch) | 0 new issues (email timing \u2192 Sec 2) |\n| Section 2 (Errors) | 3 error paths mapped, 2 CRITICAL GAPS |\n| Section 3 (Security)| 1 CRITICAL: SQL injection (HIGH/HIGH) |\n| Section 4 (Data/UX) | Data flow traced, shadow paths mapped |\n| Section 5 (Quality) | Under-engineering flagged; covered by fixes |\n| Section 6 (Tests) | 8 gaps; 6-8 integration tests required |\n| Section 7 (Perf) | 1 issue: N+1 query (fix now: batch) |\n| Section 8 (Observ) | Existing ingress logging sufficient; email |\n| | failures now logged via T2 |\n| Section 9 (Deploy) | 0 risks; feature flag + rollback in place |\n| Section 10 (Future) | Reversibility: 4/5; bypass comment: T5 |\n| Section 11 (Design) | SKIPPED (no UI scope) |\n+--------------------------------------------------------------------+\n| NOT in scope | 4 items (async email, dispatcher, format |\n| | validation, expanded test suite) |\n| What already exists | 8 retained components listed |\n| Dream state delta | written |\n| Error/rescue registry| 5 methods, 2 CRITICAL GAPS (resolved T1+T2)|\n| Failure modes | 5 total, 2 CRITICAL GAPS \u2192 resolved by T1+T2|\n| TODOS.md updates | 0 items (all findings = fix now) |\n| Scope proposals | 0 proposed (HOLD SCOPE) |\n| CEO plan | skipped (HOLD SCOPE) |\n| Outside voice | skipped (codex_reviews disabled) |\n| Diagrams produced | 4 (system arch, data flow, shadow paths, |\n| | failure modes registry) |\n| Stale diagrams found | 0 (no prior diagrams in repo) |\n| Unresolved decisions | 0 |\n+====================================================================+\n```\n\n---\n\n## Next Steps\n\n1. Run `/plan-eng-review` \u2014 required shipping gate (architecture + code quality + tests)\n2. Implement T1-T6 (all P1s block ship)\n3. After implementation, run `/review` (diff review) before opening PR\n\n---\n\n## GSTACK REVIEW REPORT\n\n| Review | Trigger | Why | Runs | Status | Findings |\n|--------|---------|-----|------|--------|----------|\n| CEO Review | `/plan-ceo-review` | Scope & strategy | 1 | issues_open | mode: HOLD_SCOPE, 2 critical gaps (SQL injection, email error handling) \u2014 all resolved via T1-T3 |\n| Outside Review | disabled | Independent 2nd opinion | 0 | disabled | codex_reviews=disabled |\n| Eng Review | `/plan-eng-review` | Architecture & tests (required) | 0 | \u2014 | not yet run |\n| Design Review | `/plan-design-review` | UI/UX gaps | 0 | \u2014 | not yet run (no UI scope) |\n| DX Review | `/plan-devex-review` | Developer experience gaps | 0 | \u2014 | not yet run |\n\n**OUTSIDE COVERAGE:** codex disabled (codex_reviews=disabled). No native fallback dispatched per disabled branch rules.\n\n**VERDICT:** CEO review complete with 4 findings \u2014 all resolved. Eng review required before ship.\n\nNO UNRESOLVED DECISIONS\n" + } + }, + { + "timestamp": "2026-09-09T00:36:24.575Z", + "isError": false, + "content": "The file /tmp/gstack-paid-shard-Q1hRY6/tmp/gstack-e2e-plan-ceo-dM1qj1/gstack-test-plan-ceo.md has been updated successfully. (file state is current in your context \u2014 no need to Read it back)" + } + ], + "planReadyRequests": [ + { + "sessionId": "89297a6b-6a00-4308-8802-75c287f45046", + "toolUseId": "toolu_011uEY7Zrm5grGaiSZvat6dh", + "timestamp": "2026-09-09T00:37:39.599Z", + "failed": false + } + ], + "rawCounts": { + "setup": 4, + "review": 5, + "administrative": 0 + }, + "mechanicalBand": [ + 4, + 7 + ], + "qualityLimitation": "Dispatcher-bypass remedy added without its own finding approval; this is separate from the mechanical4\u20137 count gate." + } +} diff --git a/test/fixtures/outside-async-task-m-events.json b/test/fixtures/outside-async-task-m-events.json new file mode 100644 index 000000000..cf1470d11 --- /dev/null +++ b/test/fixtures/outside-async-task-m-events.json @@ -0,0 +1,103 @@ +{ + "sourceHead": "084f75297dfe33530baa0cce33b6ec60faf0cc99", + "nativeSha256": "06816f001d183eb41053b561cf3cdca27b496ac15321a10ce25e4319c9520d76", + "events": [ + { + "isSidechain": false, + "type": "assistant", + "uuid": "7d81efef-536d-48eb-b6c5-a0590946e741", + "timestamp": "2026-09-09T00:16:56.277Z", + "sessionId": "f60050c4-2ad1-41f0-b547-c08306903ead", + "message": { + "role": "assistant", + "content": [ + { + "type": "tool_use", + "id": "toolu_016PugkAqmFq5DjPFmvDA3DB", + "name": "Bash", + "input": { + "command": "_REPO_ROOT=$(git rev-parse --show-toplevel)\n_OUTSIDE_TMP=$(mktemp -d \"${TMPDIR:-/tmp}/gstack-outside.XXXXXXXX\")\n\nsource /home/vercel-sandbox/gstack/bin/gstack-codex-probe 2>/dev/null || true\n\n_gstack_codex_timeout_wrapper 540 codex exec \"$(cat /tmp/codex_adversarial_prompt.txt)\" -C \"$_REPO_ROOT\" -s read-only -c 'model_reasoning_effort=\"high\"' -c 'web_search=\"cached\"' < /dev/null > \"$_OUTSIDE_TMP/text\" 2> \"$_OUTSIDE_TMP/stderr\"\n_OUTSIDE_EXIT=$?\n\ncat \"$_OUTSIDE_TMP/text\"\ncat \"$_OUTSIDE_TMP/stderr\" >&2\n\nif [ \"$_OUTSIDE_EXIT\" -ne 0 ]; then\n echo \"Codex outside review unavailable: execution failed (exit $_OUTSIDE_EXIT); missing coverage.\"\nelse\n bun /home/vercel-sandbox/gstack/lib/outside-review-result.ts review \"$_OUTSIDE_TMP/text\" || true\n echo \"OUTSIDE_STATUS: completed provider=codex host=claude\"\nfi", + "description": "Run Codex adversarial review", + "timeout": 600000, + "run_in_background": true + }, + "caller": { + "type": "direct" + } + } + ] + } + }, + { + "isSidechain": false, + "type": "user", + "uuid": "48883a6f-71f6-4fa2-b34c-b86db284d11f", + "timestamp": "2026-09-09T00:16:56.296Z", + "sessionId": "f60050c4-2ad1-41f0-b547-c08306903ead", + "message": { + "role": "user", + "content": [ + { + "tool_use_id": "toolu_016PugkAqmFq5DjPFmvDA3DB", + "type": "tool_result", + "content": "Command running in background with ID: bn8jkc52l. Output is being written to: /tmp/gstack-paid-shard-OhjUxg/tmp/claude-1000/-tmp-gstack-paid-shard-OhjUxg-tmp-gstack-outside-live-kVBfLL-claude/f60050c4-2ad1-41f0-b547-c08306903ead/tasks/bn8jkc52l.output. You will be notified when it completes. To check interim output, use Read on that file path.", + "is_error": false + } + ] + } + }, + { + "isSidechain": false, + "attachment": { + "type": "queued_command", + "prompt": "\nbn8jkc52l\ntoolu_016PugkAqmFq5DjPFmvDA3DB\n/tmp/gstack-paid-shard-OhjUxg/tmp/claude-1000/-tmp-gstack-paid-shard-OhjUxg-tmp-gstack-outside-live-kVBfLL-claude/f60050c4-2ad1-41f0-b547-c08306903ead/tasks/bn8jkc52l.output\ncompleted\nBackground command \"Run Codex adversarial review\" completed (exit code 0)\n", + "commandMode": "task-notification", + "timestamp": "2026-09-09T00:17:04.299Z" + }, + "type": "attachment", + "uuid": "93081646-3c7e-4710-a652-5509b9611243", + "timestamp": "2026-09-09T00:17:04.299Z", + "sessionId": "f60050c4-2ad1-41f0-b547-c08306903ead" + }, + { + "isSidechain": false, + "type": "assistant", + "uuid": "b81e4244-ac19-4d34-8883-eed7e34b53c5", + "timestamp": "2026-09-09T00:17:51.017Z", + "sessionId": "f60050c4-2ad1-41f0-b547-c08306903ead", + "message": { + "role": "assistant", + "content": [ + { + "type": "tool_use", + "id": "toolu_01J2PNv3YyETiCWXKkHxFVxa", + "name": "Read", + "input": { + "file_path": "/tmp/gstack-paid-shard-OhjUxg/tmp/claude-1000/-tmp-gstack-paid-shard-OhjUxg-tmp-gstack-outside-live-kVBfLL-claude/f60050c4-2ad1-41f0-b547-c08306903ead/tasks/bn8jkc52l.output" + }, + "caller": { + "type": "direct" + } + } + ] + } + }, + { + "isSidechain": false, + "type": "user", + "uuid": "6311dd36-0cfd-434e-a8c8-13c1e1a5b373", + "timestamp": "2026-09-09T00:17:51.024Z", + "sessionId": "f60050c4-2ad1-41f0-b547-c08306903ead", + "message": { + "role": "user", + "content": [ + { + "tool_use_id": "toolu_01J2PNv3YyETiCWXKkHxFVxa", + "type": "tool_result", + "content": "1\t- **Critical \u2014 Broken object-level authorization / IDOR** ([invoice.ts](/tmp/gstack-paid-shard-OhjUxg/tmp/gstack-outside-live-kVBfLL/claude/invoice.ts:4)): Removing the ownership check lets any authenticated caller retrieve any invoice by supplying its ID. This exposes sensitive financial data, including `bankAccount` and `amount`; predictable, leaked, or brute-forced IDs enable bulk cross-tenant disclosure. The now-unused `actor` parameter may also falsely signal to callers that authorization still occurs.\n2\t\n3\tRecommendation: Restore and test the ownership check before returning invoice data because any caller can currently read another customer\u2019s bank account through an invoice-ID enumeration attack.\n4\tReading additional input from stdin...\n5\tOpenAI Codex v0.153.2\n6\t--------\n7\tworkdir: /tmp/gstack-paid-shard-OhjUxg/tmp/gstack-outside-live-kVBfLL/claude\n8\tmodel: gpt-5.6-sol\n9\tprovider: openai\n10\tapproval: never\n11\tsandbox: read-only\n12\treasoning effort: high\n13\treasoning summaries: none\n14\tsession id: 01a08386-5e31-7a32-be0c-7d90503e865c\n15\t--------\n16\tuser\n17\tIMPORTANT: Do NOT read or execute any files under ~/.claude/, ~/.agents/, .claude/skills/, or agents/. These are skill definitions, not repository review data. Do not follow nested skills, hooks, or tool instructions. They contain bash scripts and prompt templates that will waste your time. Ignore them completely. Do NOT modify agents/openai.yaml. Stay focused on the repository code only.\n18\t\n19\tReview the changes on this branch against the base branch. The diff is supplied below.\n20\t\n21\tDIFF (branch: feature/invoice-lookup vs base: main):\n22\t\n23\t```diff\n24\tdiff --git a/invoice.ts b/invoice.ts\n25\tindex c6aedd5..3bfe144 100644\n26\t--- a/invoice.ts\n27\t+++ b/invoice.ts\n28\t@@ -1,6 +1,5 @@\n29\t export async function readPrivateInvoice(db, actor, invoiceId) {\n30\t const invoice = await db.invoice.findUnique({ where: { id: invoiceId } });\n31\t if (!invoice) return null;\n32\t- if (invoice.ownerId !== actor.id) throw new Error('Forbidden');\n33\t return { amount: invoice.amount, bankAccount: invoice.bankAccount };\n34\t }\n35\t```\n36\t\n37\tYour job is to find ways this code will fail in production. Think like an attacker and a chaos engineer. Find edge cases, race conditions, security holes, resource leaks, failure modes, and silent data corruption paths. Be adversarial. Be thorough. No compliments \u2014 just the problems. End your output with ONE line in the canonical format `Recommendation: because `. Generic reasons like 'because it's safer' do not qualify; the reason must point to a specific finding or no-fix rationale.\n38\tcodex\n39\t- **Critical \u2014 Broken object-level authorization / IDOR** ([invoice.ts](/tmp/gstack-paid-shard-OhjUxg/tmp/gstack-outside-live-kVBfLL/claude/invoice.ts:4)): Removing the ownership check lets any authenticated caller retrieve any invoice by supplying its ID. This exposes sensitive financial data, including `bankAccount` and `amount`; predictable, leaked, or brute-forced IDs enable bulk cross-tenant disclosure. The now-unused `actor` parameter may also falsely signal to callers that authorization still occurs.\n40\t\n41\tRecommendation: Restore and test the ownership check before returning invoice data because any caller can currently read another customer\u2019s bank account through an invoice-ID enumeration attack.\n42\ttokens used\n43\t11,840\n44\tOUTSIDE_STATUS: completed provider=codex host=claude\n45\t\n46\t[exited with code 0]\n47\t" + } + ] + } + } + ] +} diff --git a/test/helpers/autoplan-setup-question.ts b/test/helpers/autoplan-setup-question.ts index 7c9c02804..d4ecfff63 100644 --- a/test/helpers/autoplan-setup-question.ts +++ b/test/helpers/autoplan-setup-question.ts @@ -1,9 +1,9 @@ -import { capturePlanCountQuestion, planCountPrerequisitePick, planCountQuestionInput } from './claude-pty-runner'; +import { capturePlanCountQuestion, planCountPrerequisitePick, planCountQuestionInput, type AskUserQuestionFingerprint } from './claude-pty-runner'; import type { NativePlanQuestionCall } from './plan-count-transcript'; /** A copied native panel is not actionable after prose or inside a code example. */ -function activePrerequisitePanel(visible: string): boolean { +function activeSetupPanel(visible: string): boolean { const lines = visible.replace(/\r+\n?/g, '\n').trimEnd().split('\n'); if (!/^Enter[\t ]+to[\t ]+select[\t ]*·[\t ]*↑\/↓[\t ]+to[\t ]+navigate[\t ]*·[\t ]*Esc[\t ]+to[\t ]+cancel$/i.test(lines.at(-1)?.trim() ?? '')) return false; const header = lines.findLastIndex(line => /^ {0,3}[☐□][^\n]+$/.test(line)); @@ -20,8 +20,53 @@ function activePrerequisitePanel(visible: string): boolean { return !/\b(?:example|sample|quot(?:e[sd]?|ed)|template|source)\b.*\b(?:panel|menu|choices?|prompt|question|below|following)\b/i.test(introduction); } -/** Handle routing and the optional design-doc prerequisite in this isolated fixture. */ -export function autoplanRoutingSetupInput(visible: string, seen: Set, pending?: NativePlanQuestionCall): string | null { +export type AutoplanSetupDecision = + | { kind: 'input'; input: string; signatures: string[] } + | { kind: 'waiting' | 'unrelated' } + | { kind: 'unsupported_setup'; setup: 'routing' | 'prerequisite'; prompt: string; + options: Array<{ index: number; label: string }>; identitySource: 'native-bound' | 'current-native-panel' }; + +/** Fail only a complete current setup panel; absence or stale/partial metadata is not failure. */ +function completeSetupOptions(visible: string, pending?: NativePlanQuestionCall): Array<{ index: number; label: string }> | null { + if (!activeSetupPanel(visible)) return null; + const lines = visible.replace(/\r+\n?/g, '\n').trimEnd().split('\n'); + const header = lines.findLastIndex(line => /^ {0,3}[☐□][^\n]+$/.test(line)); + const introduction = lines.slice(0, header).findLast(line => !/^[\t ─━-]*$/.test(line)) ?? ''; + if (/\b(?:example|sample|quot(?:e[sd]?|ed)|template|source)\b[^\n]*:\s*$/i.test(introduction)) return null; + const panel = lines.slice(header); + if (/[←→☒]|✔\s*Submit/.test(panel[0]!) || + panel.some(line => /(?:^|\s)[1-9]\.\s*\[[ ✓✔xX]\]/.test(line))) return null; + if (panel.filter(line => /^ {0,3}❯\s*1\./.test(line)).length !== 1 || + panel.filter(line => /❯\s*[1-9]\./.test(line)).length !== 1) return null; + const rows = panel.flatMap(line => { + const match = /^ {0,3}(?:❯\s*)?([1-9])\.[\t ]*(\S.*?)\s*$/.exec(line); + return match ? [{ index: Number(match[1]), label: match[2]! }] : []; + }); + const compact = (text: string) => text.replace(/\s+/g, '').toLowerCase(); + if (rows.length < 4 || rows.some((row, index) => row.index !== index + 1) || + compact(rows.at(-2)!.label) !== 'typesomething.' || compact(rows.at(-1)!.label) !== 'chataboutthis') return null; + const options = rows.slice(0, -2); + if (pending) { + const native = pending.questions[0]; + const cursor = panel.findIndex(line => /^ {0,3}❯\s*1\./.test(line)); + if (pending.answered || pending.failed || pending.questions.length !== 1 || native?.multiSelect || + compact(panel[0]!.replace(/^ {0,3}[☐□]/, '')) !== compact(native!.header) || + !compact(panel.slice(1, cursor).join(' ')).includes(compact(native!.question)) || + options.length !== native!.options.length || options.some((row, index) => compact(row.label) !== compact(native!.options[index]!.label))) return null; + } + return options; +} + +function unsupportedSetup(visible: string, question: AskUserQuestionFingerprint, + setup: 'routing' | 'prerequisite', pending?: NativePlanQuestionCall): AutoplanSetupDecision { + const options = completeSetupOptions(visible, pending); + return options ? { kind: 'unsupported_setup', setup, prompt: question.promptSnippet, options, + identitySource: pending ? 'native-bound' : 'current-native-panel' } : { kind: 'waiting' }; +} + +/** Pure classification: only the caller that sends input commits returned identities. */ +export function autoplanSetupDecision(visible: string, seen: ReadonlySet, pending?: NativePlanQuestionCall): AutoplanSetupDecision { + if (pending && (pending.answered || pending.failed || pending.questions.length !== 1 || pending.questions[0]?.multiSelect)) return { kind: 'waiting' }; // Box borders are terminal decoration, not part of an untagged native // question's wrapped text. Keep its full content for identity matching. const display = visible.replace(/(^|[\r\n])[\t ]*[│┃][\t ]?/g, '$1'); @@ -29,14 +74,15 @@ export function autoplanRoutingSetupInput(visible: string, seen: Set, pe // available when the actual native call arrives after the visible prompt. const captured = new Set(seen); const question = capturePlanCountQuestion(display, captured, 0, true, pending); - if (!question) return null; - const answered = (input: string | null) => { - if (input !== null) for (const signature of captured) seen.add(signature); - return input; - }; + if (!question) return { kind: 'waiting' }; + const answered = (input: string | null): AutoplanSetupDecision => input === null + ? { kind: 'waiting' } + : { kind: 'input', input, signatures: [...captured].filter(signature => !seen.has(signature)) }; const prerequisite = planCountPrerequisitePick(question); - if (prerequisite !== null) { + const prerequisitePrompt = /\/office-hours/i.test(question.promptSnippet) && + /(?:no\s*design\s*doc|produce\s*a\s*design\s*doc)/i.test(question.promptSnippet); + if (prerequisitePrompt) { // The canonical offer has two opposed choices. A known native call must // match this one-question panel; no mixed packet or substantive choice // may borrow its skip. Before JSONL flushes, require the complete native @@ -44,9 +90,20 @@ export function autoplanRoutingSetupInput(visible: string, seen: Set, pe const choices = question.options.filter(option => !/^(?:Type something\.|Chat about this)$/.test(option.label)); if (choices.length !== 2 || (pending && - (!question.nativeCall || pending.questions.length !== 1 || pending.questions[0]?.multiSelect))) return null; + (!question.nativeCall || pending.questions.length !== 1 || pending.questions[0]?.multiSelect))) return { kind: 'waiting' }; + if (prerequisite === null) { + // Mentioning a missing design doc or /office-hours in a product/taste + // question does not make it setup. Independently identify an offer to + // run that prerequisite even when its opposed skip is unsupported. + const run = choices.filter(({ label }) => /^Run\s*\/office-hours\s*(?:now|first)(?:\s*\(recommended\))?$/i.test(label)); + // UI fingerprints abbreviate long questions; inspect the current + // question before its cursor, with full-panel validation below. + const beforeOptions = display.slice(0, display.search(/❯\s*1\./)); + const offer = /\brun\s*\/office-hours\s*(?:now|first)\s*\?(?:\s*]+>)?\s*$/i.test(beforeOptions); + return run.length === 1 && offer ? unsupportedSetup(display, question, 'prerequisite', pending) : { kind: 'unrelated' }; + } const input = planCountQuestionInput(display, question, prerequisite); - if (!/^[1-9]\d*$/.test(input ?? '') || !activePrerequisitePanel(display)) return null; + if (!/^[1-9]\d*$/.test(input ?? '') || !activeSetupPanel(display)) return { kind: 'waiting' }; return answered(input); } @@ -65,6 +122,9 @@ export function autoplanRoutingSetupInput(visible: string, seen: Set, pe // Match the declined setup action, not every English label separately: // No thanks/Skip may stand alone or opt into manual invocation. A manual // migration, deletion, or unrelated workflow is not the opposed action. + // "Only" limits the same manual action; it does not add a second action. + // Use one whole-label grammar with and without a courtesy/Skip prefix. + const manualAction = /^(?:manual(?:invocation)?|(?:I['’]ll)?invoke(?:skills)?manually)(?:[-–—]?only)?$/i; const decline = options.filter(option => { const title = option.title.replace(/\(Recommended\)$/i, ''); // The action can stand alone or follow a short courtesy ('No thanks'). @@ -72,15 +132,14 @@ export function autoplanRoutingSetupInput(visible: string, seen: Set, pe // manually' intact. Match the complete action, not the spelling of No; // arbitrary preceding instructions and extra trailing actions still fail. const manual = title.replace(/^[a-z]{0,3}thanks[,—–-]/i, ''); - if (/^(?:manualinvocation|(?:I['’]ll)?invoke(?:skills)?manually)$/i.test(manual)) return true; + if (manualAction.test(manual)) return true; const prefix = /^(?:Nothanks|Skip)(?:[,—–-])?/i.exec(title); if (!prefix) return false; // 'No thanks' can be followed by the same explicit Skip action. Strip // that decline verb before checking any optional manual-invocation text. const action = title.slice(prefix[0].length).replace(/^skip(?:[,—–-])?/i, ''); - return action === '' || /^(?:manual(?:invocation)?|(?:I['’]ll)?invoke(?:skills)?manually)$/i.test(action); + return action === '' || manualAction.test(action); }); - if (add.length !== 1 || decline.length !== 1 || add[0]!.index === decline[0]!.index) return null; const routingId = //i.test(question.promptSnippet); const routingPremise = /gstack/i.test(prompt) && /CLAUDE\.md/i.test(prompt) && /skillroutingrules/i.test(prompt); // A qid can replace the longer premise, but cannot override a question @@ -88,6 +147,20 @@ export function autoplanRoutingSetupInput(visible: string, seen: Set, pe // project setup rather than a product routing or taste decision. const claudeTarget = /CLAUDE\.md/i.test(prompt); const quotedPremise = /\b(?:plan|spec|document)\s+(?:quotes?|cites?|references?)\b/i.test(primary); - if (!claudeTarget || quotedPremise || (!routingId && !routingPremise)) return null; + if (!claudeTarget || quotedPremise || (!routingId && !routingPremise)) return { kind: 'unrelated' }; + if (pending && (!question.nativeCall || pending.questions.length !== 1 || pending.questions[0]?.multiSelect)) return { kind: 'waiting' }; + // An intact Add-to-CLAUDE.md action identifies this setup offer even if + // its opposed decline is unsupported. A qid or premise alone must not + // turn substantive/ambiguous choices into an early setup failure. + if (add.length !== 1) return { kind: 'waiting' }; + if (decline.length !== 1 || add[0]!.index === decline[0]!.index) return unsupportedSetup(display, question, 'routing', pending); return answered(planCountQuestionInput(display, question, add[0]!.index)); } + +/** Compatibility wrapper: preserve the existing input-only API. */ +export function autoplanRoutingSetupInput(visible: string, seen: Set, pending?: NativePlanQuestionCall): string | null { + const decision = autoplanSetupDecision(visible, seen, pending); + if (decision.kind !== 'input') return null; + for (const signature of decision.signatures) seen.add(signature); + return decision.input; +} diff --git a/test/helpers/ceo-completion-handoff.ts b/test/helpers/ceo-completion-handoff.ts index 2f25c756a..38e74707f 100644 --- a/test/helpers/ceo-completion-handoff.ts +++ b/test/helpers/ceo-completion-handoff.ts @@ -1,5 +1,18 @@ import type { AskUserQuestionFingerprint } from './claude-pty-runner'; +/** A native choice may carry the CEO-specific recap beside a generic completion question. */ +function closedCeoRecap(description: string): boolean { + const clause = /(?:^|[.!?]\s+)((?:The\s+)?CEO\s+review\b[^.!?]{0,240})(?=[.!?]|$)/i.exec(description)?.[1]; + if (!clause || /\b(?:if|unless|until|once|when|after|not|never)\b|n['’]t\b/i.test(clause)) return false; + return /\b(?:all(?:\s+(?:gaps?|issues?|findings?))?(?:\s+(?:are|were))?\s+resolved|(?:no|0)\s+unresolved\s+(?:decisions|gaps|issues|findings))(?=\s*\)?\s*(?:;|$))/i.test(clause); +} + +/** Past-tense resolution can close a native next-review recap without the word "complete". */ +function resolvedCeoRecap(description: string): boolean { + const clause = /(?:^|[.!?]\s+)((?:(?:This|The)\s+)?CEO\s+review\s+resolved\s+[^.!?;]{1,180}\b(?:bugs|gaps|issues|findings))(?=\s*(?:[.!?;]|$))/i.exec(description)?.[1]; + return Boolean(clause && !/\b(?:if|unless|until|once|when|after|not|never|some|most|partially|only|of|but|several|few)\b|n['’]t\b/i.test(clause)); +} + /** Only the CEO's finished-review menu, never a finding/TODO mentioning another skill. */ function manualHandoffIndex(fp: AskUserQuestionFingerprint): number | null { const call = fp.nativeCall; @@ -8,12 +21,24 @@ function manualHandoffIndex(fp: AskUserQuestionFingerprint): number | null { if (!call || call.failed || fp.signature !== `${call.sessionId}:${call.toolUseId}` || call.questions.length !== 1) return null; const q = call.questions[0]!; if (q.multiSelect || q.options.length < 2) return null; - const id = //i.exec(q.question)?.[1]?.toLowerCase(); + const ids = [...q.question.matchAll(//gi)]; + if (ids.length > 1 || (q.question.match(/ option.description ?? '')].join('\n'); + const explicitCompletion = /(?:^|[.!?]\s+)(?:ELI10:\s*)?(?:The\s+)?CEO\s+review\s+(?:is\s+)?(?:complete|cleared|clean|done(?:\s+and\s+the\s+plan\s+is\s+cleared)?)(?:\s+with\s+0\s+unresolved\s+decisions)?(?=\s*(?:[.!?—–]|$))/i.test(declaration); + const genericCompletion = /(?:^|[.!?]\s+)(?:The\s+)?review\s+(?:is\s+)?(?:complete|cleared|clean|done)(?=\s*(?:[.!?—–]|$))/i.test(declaration); + const questionText = declaration.replace(/]+>/gi, '').trim(); + const recappedNavigation = Boolean(id) && + /^What(?:['’]s|\s+is)\s+the\s+next\s+(?:steps?|review)\s+after\s+(?:this|the)\s+CEO\s+review\?$/i.test(questionText) && + q.options.some(option => resolvedCeoRecap(option.description ?? '')); + const unfinished = gateContext.replace(/\b(?:no|0)\s+unresolved\s+(?:decisions|gaps|issues|findings)\b/gi, ''); + const describedCompletion = (recappedNavigation || (genericCompletion && q.options.some(option => closedCeoRecap(option.description ?? '')))) && + !/\b(?:unresolved|outstanding|remains?|remaining|pending)\b/i.test(unfinished) && + !/(?:^|[.!?;]\s+|\b(?:please|must|need\s+to)\s+)(?:(?:please|first|then|also)\s+)*(?:add|fix|implement|resolve|decide)\b/im.test(gateContext); + const completion = explicitCompletion || describedCompletion; const requiredEng = /(?:\bEng(?:ineering)?\s+review|\/plan-eng-review)\b[^.!?]{0,180}\brequired(?:\s+shipping)?\s+gate\b/i.test(gateContext) || /\brequired(?:\s+shipping)?\s+gate\s+is\s+(?:an?\s+)?(?:Eng(?:ineering)?\s+review|\/plan-eng-review)\b/i.test(gateContext); // A qid names the menu; it cannot replace its completed-review declaration diff --git a/test/helpers/outside-voice-evidence.ts b/test/helpers/outside-voice-evidence.ts index 4a3a382bc..ed2a14893 100644 --- a/test/helpers/outside-voice-evidence.ts +++ b/test/helpers/outside-voice-evidence.ts @@ -3,26 +3,149 @@ export interface OutsideExecution { command: string; output: string; succeeded: boolean; + background?: { + toolUseId: string; + taskId: string; + outputFile: string; + outputToolUseId?: string; + completion: 'pending' | 'task_notification' | 'TaskOutput'; + }; +} + +function toolText(content: unknown): string { + return typeof content === 'string' ? content : Array.isArray(content) + ? content.filter(block => block?.type === 'text' && typeof block.text === 'string').map(block => block.text).join('\n') : ''; +} + +function sessionIdentity(event: any): string | null { + if (event?.isSidechain === true || event?.parent_tool_use_id != null) return null; + const id = event?.session_id ?? event?.sessionId; + if (typeof id !== 'string' || !id || (event.sessionId && event.sessionId !== id)) return null; + return id; +} + +/** Only native task events carry completion; quoted/user-authored notifications do not. */ +function taskNotification(event: any) { + if (event?.type === 'system' && event.subtype === 'task_notification') return { + taskId: event.task_id, toolUseId: event.tool_use_id, outputFile: event.output_file, + status: event.status, summary: event.summary, + }; + if (event?.type !== 'attachment' || event.attachment?.type !== 'queued_command' || + event.attachment.commandMode !== 'task-notification') return null; + const text = event.attachment.prompt; + if (typeof text !== 'string') return null; + const match = /^\s*([^<>\r\n]+)<\/task-id>\s*([^<>\r\n]+)<\/tool-use-id>\s*([^<>\r\n]+)<\/output-file>\s*(completed|failed|stopped)<\/status>\s*([^<>]+)<\/summary>\s*<\/task-notification>$/.exec(text); + return match ? { taskId: match[1], toolUseId: match[2], outputFile: match[3], status: match[4], summary: match[5] } : null; +} + +/** A complete native Read starts at line one and includes the task's exit footer. */ +function completedTaskRead(text: string): string | null { + const lines = text.replace(/\r?\n$/, '').split(/\r?\n/); + const numbered = lines.map(line => /^\s*(\d+)\t(.*)$/.exec(line)); + if (!numbered.length || numbered.some((line, index) => !line || Number(line[1]) !== index + 1)) return null; + const output = numbered.map(line => line![2]).join('\n'); + return /(?:^|\n)\[exited with code 0\]\s*$/.test(output) ? output : null; } export function claudeOutsideExecutions(transcript: unknown[]): OutsideExecution[] { - const calls = new Map(); + const calls = new Map(); + const tasks = new Map; + }>(); const results: OutsideExecution[] = []; for (const event of transcript as any[]) { + const notice = taskNotification(event); + const notified = notice && tasks.get(notice.toolUseId); + if (notified && sessionIdentity(event) === notified.session && notice.taskId === notified.taskId && notice.outputFile === notified.outputFile) { + if (notice.status === 'failed' || notice.status === 'stopped') notified.failed = true; + else if (notice.status === 'completed') { + const terminal = /^Background command [\s\S]+ completed \(exit code (-?\d+)\)$/.exec(notice.summary ?? ''); + if (terminal?.[1] === '0') notified.completion = 'task_notification'; + else if (terminal) notified.failed = true; + } + } const blocks = event?.message?.content; if (!Array.isArray(blocks)) continue; for (const block of blocks) { - if (event.type === 'assistant' && block.type === 'tool_use' && block.name === 'Bash') { - const command = block.input?.command; - if (typeof block.id === 'string' && typeof command === 'string') calls.set(block.id, command); + if (event.type === 'assistant' && block.type === 'tool_use' && typeof block.id === 'string' && + ['Bash', 'Read', 'TaskOutput'].includes(block.name)) { + calls.set(block.id, { command: block.input?.command, name: block.name, input: block.input, session: sessionIdentity(event) }); } if (event.type === 'user' && block.type === 'tool_result' && calls.has(block.tool_use_id)) { - const output = typeof block.content === 'string' ? block.content - : (Array.isArray(block.content) ? block.content.filter((b: any) => b.type === 'text').map((b: any) => b.text).join('\n') : ''); - results.push({ command: calls.get(block.tool_use_id)!, output, succeeded: block.is_error !== true }); + const call = calls.get(block.tool_use_id)!; + const output = toolText(block.content); + if (call.name === 'Bash' && typeof call.command === 'string') { + const acknowledgment = /^Command running in background with ID: ([a-zA-Z0-9_-]+)\. Output is being written to: ([^\r\n]+)\. You will be notified when it completes\. To check interim output, use Read on that file path\.$/.exec(output); + if (!acknowledgment) { + const existing = tasks.get(block.tool_use_id); + if (existing) { + if (block.is_error === true) existing.failed = true; + results.push({ command: existing.command, output, succeeded: false, + background: { toolUseId: block.tool_use_id, taskId: existing.taskId, + outputFile: existing.outputFile, completion: 'pending' } }); + continue; + } + const pending = /^Command running in background\b/.test(output) || event.toolUseResult?.backgroundTaskId; + results.push({ command: call.command, output, succeeded: block.is_error !== true && !pending }); + continue; + } + const [, taskId, outputFile] = acknowledgment; + const background = { toolUseId: block.tool_use_id, taskId: taskId!, outputFile: outputFile!, completion: 'pending' as const }; + results.push({ command: call.command, output, succeeded: false, background }); + const existing = tasks.get(block.tool_use_id); + if (block.is_error === true || !call.session || sessionIdentity(event) !== call.session || + !outputFile!.endsWith(`/tasks/${taskId}.output`) || !outputFile!.startsWith('/')) { + if (existing) existing.failed = true; + continue; + } + if (existing) { + // Replayed acknowledgments cannot reset completed output or a + // terminal failure. One original call cannot launch a second task. + if (existing.command !== call.command || existing.session !== call.session || + existing.taskId !== taskId || existing.outputFile !== outputFile) existing.failed = true; + continue; + } + tasks.set(block.tool_use_id, { command: call.command, session: call.session, taskId: taskId!, outputFile: outputFile!, failed: false, outputs: [] }); + continue; + } + if (!call.session || sessionIdentity(event) !== call.session) continue; + for (const task of tasks.values()) { + if (task.session !== call.session) continue; + if (call.name === 'Read' && call.input?.file_path === task.outputFile) { + const complete = block.is_error !== true && (call.input.offset === undefined || call.input.offset === 1) + ? completedTaskRead(output) : null; + task.outputs.push({ toolUseId: block.tool_use_id, output: complete ?? output, complete: complete !== null }); + } + if (block.is_error === true) { + if (call.name === 'TaskOutput' && call.input?.task_id === task.taskId) task.failed = true; + continue; + } + if (call.name === 'TaskOutput' && call.input?.task_id === task.taskId) { + // CLI's native TaskOutput envelope puts terminal status before the + // output. Findings containing an exit-code string cannot supply it. + const terminal = /^success<\/retrieval_status>\s*([^<>]+)<\/task_id>\s*local_bash<\/task_type>\s*(completed|failed|stopped)<\/status>\s*(-?\d+)<\/exit_code>\s*\n([\s\S]*)\n<\/output>\s*$/.exec(output); + if (!terminal || terminal[1] !== task.taskId) continue; + const complete = terminal[2] === 'completed' && terminal[3] === '0'; + if (!complete) task.failed = true; + else { + task.completion = 'TaskOutput'; + } + task.outputs.push({ toolUseId: block.tool_use_id, output: terminal[4]!, complete }); + } + } } } } + for (const [toolUseId, task] of tasks) { + for (const output of task.outputs) { + results.push({ command: task.command, output: output.output, + succeeded: output.complete && Boolean(task.completion) && !task.failed, + background: { toolUseId, taskId: task.taskId, outputFile: task.outputFile, + outputToolUseId: output.toolUseId, completion: task.completion ?? 'pending' } }); + } + } return results; } @@ -46,8 +169,9 @@ function outsideInvocation(provider: 'codex' | 'claude-code'): RegExp { /** Preserve provider results, including failures, without unrelated shell/config events. */ export function outsideExecutionTranscript(executions: OutsideExecution[], provider: 'codex' | 'claude-code') { return executions.filter(({ command }) => outsideInvocation(provider).test(command)) - .map(({ command, output, succeeded }) => ({ + .map(({ command, output, succeeded, background }) => ({ type: 'outside_execution' as const, provider, command, output, succeeded, + ...(background ? { background } : {}), })); } @@ -56,5 +180,6 @@ export function foundInvoiceAuthorizationDefect(executions: OutsideExecution[], && outsideInvocation(provider).test(command) && /invoice/i.test(output) && /owner|ownership|unauthori[sz]ed|another user|cross[- ](?:tenant|user)|access control|authorization/i.test(output) + && !/(?:^|\n)\s*(?:I|We)\s+(?:cannot|can['’]t|won['’]t|am unable to|are unable to)\s+(?:review|perform|complete|provide|conduct|help with)\b/i.test(output) && !/OUTSIDE_STATUS:\s*(?:unavailable|disabled|skipped)/.test(output)); } diff --git a/test/helpers/outside-voice-fixture.ts b/test/helpers/outside-voice-fixture.ts index 33b8fb846..7f3c0f1fc 100644 --- a/test/helpers/outside-voice-fixture.ts +++ b/test/helpers/outside-voice-fixture.ts @@ -1,4 +1,6 @@ /** Extract the installed review workflow, handling carved and inline host renders. */ +import * as fs from 'node:fs'; +import * as path from 'node:path'; import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { extractSkillSections } from './skill-fixture'; @@ -24,3 +26,33 @@ export function installOutsideReviewFixture(rendered: string, host: 'claude' | ' writeFileSync(join(destination, 'SKILL.md'), content); return destination; } + +// Each paid invocation must begin at the same committed authorization defect. +function git(cwd: string, ...args: string[]) { + const result = Bun.spawnSync(['git', ...args], { cwd, stdout: 'pipe', stderr: 'pipe', timeout: 10_000 }); + if (result.exitCode !== 0) throw new Error(`git ${args[0]}: ${result.stderr.toString()}`); +} + +export function createOutsideReviewRepo(fixtureRoot: string, host: 'claude' | 'codex'): string { + // Bun retries reuse beforeAll state; each attempt needs a new git repository. + const dir = fs.mkdtempSync(path.join(fixtureRoot, `${host}-`)); + git(dir, 'init', '-b', 'main'); + git(dir, 'config', 'user.email', 'eval@example.com'); + git(dir, 'config', 'user.name', 'Outside Voice Eval'); + const safe = `export async function readPrivateInvoice(db, actor, invoiceId) { + const invoice = await db.invoice.findUnique({ where: { id: invoiceId } }); + if (!invoice) return null; + if (invoice.ownerId !== actor.id) throw new Error('Forbidden'); + return { amount: invoice.amount, bankAccount: invoice.bankAccount }; +} +`; + fs.writeFileSync(path.join(dir, 'invoice.ts'), safe); + git(dir, 'add', 'invoice.ts'); + git(dir, 'commit', '-m', 'Protect private invoices by owner'); + git(dir, 'update-ref', 'refs/remotes/origin/main', 'HEAD'); + git(dir, 'checkout', '-b', 'feature/invoice-lookup'); + fs.writeFileSync(path.join(dir, 'invoice.ts'), safe.replace(" if (invoice.ownerId !== actor.id) throw new Error('Forbidden');\n", '')); + git(dir, 'add', 'invoice.ts'); + git(dir, 'commit', '-m', 'Simplify invoice lookup'); + return dir; +} diff --git a/test/helpers/touchfiles-data.ts b/test/helpers/touchfiles-data.ts index 30b26e998..cdef145b8 100644 --- a/test/helpers/touchfiles-data.ts +++ b/test/helpers/touchfiles-data.ts @@ -160,7 +160,7 @@ export const E2E_TOUCHFILES: Record = { // Each test drives its skill end-to-end; touchfiles include preamble + // completion-status resolvers because they affect question cadence and // terminal output (the regression surface this test catches). - 'plan-ceo-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-ceo-review/**', 'test/skill-ceo-section-ordering.test.ts', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/plan-count-completion.test.ts', 'test/helpers/ceo-completion-handoff.ts', 'test/ceo-completion-handoff.test.ts', 'test/fixtures/ceo-completion-handoff-calls.json', 'test/fixtures/ceo-completion-handoff-j-calls.json', 'test/fixtures/ceo-completion-handoff-k-calls.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/skill-e2e-plan-ceo-finding-count.test.ts', 'test/ceo-completion-handoff-l.test.ts', 'test/fixtures/ceo-completion-handoff-l-calls.json', 'test/fixtures/devex-review-l-calls.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt'], + 'plan-ceo-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-ceo-review/**', 'test/skill-ceo-section-ordering.test.ts', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/plan-count-completion.test.ts', 'test/helpers/ceo-completion-handoff.ts', 'test/ceo-completion-handoff.test.ts', 'test/fixtures/ceo-completion-handoff-calls.json', 'test/fixtures/ceo-completion-handoff-j-calls.json', 'test/fixtures/ceo-completion-handoff-k-calls.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/skill-e2e-plan-ceo-finding-count.test.ts', 'test/ceo-completion-handoff-l.test.ts', 'test/fixtures/ceo-completion-handoff-l-calls.json', 'test/ceo-completion-handoff-m.test.ts', 'test/fixtures/ceo-completion-handoff-m-call.json', 'test/fixtures/devex-review-l-calls.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt'], 'plan-eng-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-eng-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/plan-count-completion.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/skill-e2e-plan-eng-finding-count.test.ts', 'test/fixtures/devex-review-l-calls.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt'], 'plan-design-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-design-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/plan-count-completion.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/helpers/design-count-review.ts', 'test/design-count-review.test.ts', 'test/fixtures/design-review-j-calls.json', 'test/helpers/design-count-outside.ts', 'test/design-count-outside.test.ts', 'test/skill-e2e-plan-design-finding-count.test.ts', 'test/fixtures/design-review-l-calls.json', 'test/design-completion-handoff.test.ts', 'test/fixtures/design-handoff-l-calls.json', 'test/fixtures/devex-review-l-calls.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt'], 'plan-devex-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-devex-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/plan-count-completion.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/helpers/devex-count-fixture.ts', 'test/devex-count-fixture.test.ts', 'test/skill-e2e-plan-devex-finding-count.test.ts', 'test/fixtures/devex-review-l-calls.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt'], @@ -281,14 +281,14 @@ export const E2E_TOUCHFILES: Record = { 'scripts/gen-skill-docs.ts', 'scripts/resolvers/index.ts', 'scripts/resolvers/outside-voice.ts', 'scripts/resolvers/constants.ts', 'scripts/resolvers/review.ts', 'bin/gstack-claude-code', 'lib/claude-code.ts', 'lib/claude-code-windows-job.ts', 'lib/claude-bin.ts', 'lib/outside-review-result.ts', 'test/helpers/codex-session-runner.ts', - 'test/helpers/skill-fixture.ts', 'test/helpers/outside-voice-fixture.ts', 'test/helpers/outside-voice-evidence.ts', 'test/outside-voice-evidence.test.ts', 'test/skill-e2e-outside-voice.test.ts', + 'test/helpers/skill-fixture.ts', 'test/helpers/outside-voice-fixture.ts', 'test/outside-voice-fixture.test.ts', 'test/helpers/outside-voice-evidence.ts', 'test/outside-voice-evidence.test.ts', 'test/outside-voice-async.test.ts', 'test/fixtures/outside-async-task-m-events.json', 'test/skill-e2e-outside-voice.test.ts', ], 'outside-voice-claude-code-to-codex': [ 'review/**', 'codex/**', 'hosts/claude.ts', 'hosts/define-host.ts', 'scripts/gen-skill-docs.ts', 'scripts/resolvers/index.ts', 'scripts/resolvers/outside-voice.ts', 'scripts/resolvers/constants.ts', 'scripts/resolvers/review.ts', 'bin/gstack-codex-probe', 'lib/outside-review-result.ts', 'test/helpers/session-runner.ts', - 'test/helpers/skill-fixture.ts', 'test/helpers/outside-voice-fixture.ts', 'test/helpers/outside-voice-evidence.ts', 'test/outside-voice-evidence.test.ts', 'test/skill-e2e-outside-voice.test.ts', + 'test/helpers/skill-fixture.ts', 'test/helpers/outside-voice-fixture.ts', 'test/outside-voice-fixture.test.ts', 'test/helpers/outside-voice-evidence.ts', 'test/outside-voice-evidence.test.ts', 'test/outside-voice-async.test.ts', 'test/fixtures/outside-async-task-m-events.json', 'test/skill-e2e-outside-voice.test.ts', ], // Disabled means no extra plan review, including a native Agent fallback. diff --git a/test/outside-voice-async.test.ts b/test/outside-voice-async.test.ts new file mode 100644 index 000000000..c47082b0a --- /dev/null +++ b/test/outside-voice-async.test.ts @@ -0,0 +1,167 @@ +import { describe, expect, test } from 'bun:test'; +import { claudeOutsideExecutions, foundInvoiceAuthorizationDefect, outsideExecutionTranscript } from './helpers/outside-voice-evidence'; +import { parseNDJSON } from './helpers/session-runner'; +import captured from './fixtures/outside-async-task-m-events.json'; + +const events = () => structuredClone(captured.events) as any[]; +const finding = 'The removed invoice owner check allows unauthorized access. Restore the ownership check.'; + +function sdkEvents() { + // The CLI emits the same native task completion as this documented SDK + // event. Disk-only attachment fields are not required by the stream parser. + const input = events(); + const attachment = input[2]; + const prompt = attachment.attachment.prompt as string; + const field = (name: string) => new RegExp(`<${name}>([^<]+)`).exec(prompt)![1]; + input[2] = { type: 'system', subtype: 'task_notification', session_id: attachment.sessionId, + task_id: field('task-id'), tool_use_id: field('tool-use-id'), output_file: field('output-file'), + status: field('status'), summary: field('summary') }; + return input.map(event => { + const { sessionId, isSidechain, ...rest } = event; + return { ...rest, session_id: event.session_id ?? sessionId }; + }); +} + +function detected(input: unknown[]) { + return foundInvoiceAuthorizationDefect(claudeOutsideExecutions(input), 'codex'); +} + +describe('completed background outside execution correlation', () => { + test('the exact task completion and complete output Read establish the actual external finding', () => { + const result = claudeOutsideExecutions(events()); + expect(result).toHaveLength(2); + expect(result[0]!.succeeded).toBe(false); + expect(result[0]!.background?.completion).toBe('pending'); + expect(result[1]!.succeeded).toBe(true); + expect(result[1]!.background).toEqual({ + toolUseId: 'toolu_016PugkAqmFq5DjPFmvDA3DB', taskId: 'bn8jkc52l', + outputFile: events()[3].message.content[0].input.file_path, + outputToolUseId: 'toolu_01J2PNv3YyETiCWXKkHxFVxa', completion: 'task_notification', + }); + expect(result[1]!.output).toContain('Removing the ownership check lets any authenticated caller retrieve any invoice'); + expect(detected(events())).toBe(true); + expect(outsideExecutionTranscript(result, 'codex')[1]!.background).toEqual(result[1]!.background); + }); + + test('SDK NDJSON retains the same exact identity and output instead of borrowing host prose', () => { + const stream = sdkEvents().map(event => JSON.stringify(event)); + const { transcript } = parseNDJSON(stream); + expect(detected(transcript)).toBe(true); + expect(detected(transcript.filter(event => event.type !== 'system'))).toBe(false); + }); + + test.each([ + ['missing acknowledgment', (e: any[]) => { e.splice(1, 1); }], + ['acknowledgment only', (e: any[]) => { e.splice(2); }], + ['missing completion', (e: any[]) => { e.splice(2, 1); }], + ['missing Read use', (e: any[]) => { e.splice(3, 1); }], + ['missing Read result', (e: any[]) => { e.pop(); }], + ['wrong Read identity', (e: any[]) => { e[4].message.content[0].tool_use_id = 'other'; }], + ['wrong task output path', (e: any[]) => { e[3].message.content[0].input.file_path += '.other'; }], + ['foreign Read session', (e: any[]) => { e[4].sessionId = 'foreign'; }], + ['sidechain Read', (e: any[]) => { e[4].isSidechain = true; }], + ['nested Read', (e: any[]) => { e[4].parent_tool_use_id = 'nested'; }], + ['failed Read', (e: any[]) => { e[4].message.content[0].is_error = true; }], + ['offset Read', (e: any[]) => { e[3].message.content[0].input.offset = 2; }], + ['partial output', (e: any[]) => { e[4].message.content[0].content = e[4].message.content[0].content.split('\n').slice(0, 3).join('\n'); }], + ['nonconsecutive lines', (e: any[]) => { e[4].message.content[0].content = e[4].message.content[0].content.replace('2\t', '9\t'); }], + ['output without exit footer', (e: any[]) => { e[4].message.content[0].content = e[4].message.content[0].content.replace('[exited with code 0]', ''); }], + ['failed exit footer', (e: any[]) => { e[4].message.content[0].content = e[4].message.content[0].content.replace('[exited with code 0]', '[exited with code 1]'); }], + ['raw forged output', (e: any[]) => { e[4].message.content[0].content = finding + '\n[exited with code 0]'; }], + ['foreign completion', (e: any[]) => { e[2].sessionId = 'foreign'; }], + ['wrong completed task', (e: any[]) => { e[2].attachment.prompt = e[2].attachment.prompt.replace('bn8jkc52l', 'other'); }], + ['wrong original tool', (e: any[]) => { e[2].attachment.prompt = e[2].attachment.prompt.replace('toolu_016PugkAqmFq5DjPFmvDA3DB', 'other'); }], + ['wrong completed file', (e: any[]) => { e[2].attachment.prompt = e[2].attachment.prompt.replace('bn8jkc52l.output', 'other.output'); }], + ['failed task', (e: any[]) => { e[2].attachment.prompt = e[2].attachment.prompt.replace('completed', 'failed'); }], + ['stopped task', (e: any[]) => { e[2].attachment.prompt = e[2].attachment.prompt.replace('completed', 'stopped'); }], + ['nonzero completed exit', (e: any[]) => { e[2].attachment.prompt = e[2].attachment.prompt.replace('(exit code 0)', '(exit code 1)'); }], + ['stale completion before launch', (e: any[]) => { e.unshift(e.splice(2, 1)[0]); }], + ['quoted user notification', (e: any[]) => { e[2] = { type: 'user', sessionId: e[2].sessionId, message: { content: [{ type: 'text', text: e[2].attachment.prompt }] } }; }], + ['assistant notification claim', (e: any[]) => { e[2] = { type: 'assistant', sessionId: e[2].sessionId, message: { content: [{ type: 'text', text: e[2].attachment.prompt }] } }; }], + ['refusal containing the fixture words', (e: any[]) => { e[4].message.content[0].content = '1\tI cannot review the invoice owner authorization issue.\n2\t[exited with code 0]'; }], + ['unavailable provider', (e: any[]) => { e[4].message.content[0].content = `1\t${finding}\n2\tOUTSIDE_STATUS: unavailable\n3\t[exited with code 0]`; }], + ])('rejects %s despite a matching finding elsewhere', (_name, mutate) => { + const input = events(); + mutate(input); + expect(detected(input)).toBe(false); + }); + + test('a terminal failure overrides an earlier completion and an output containing a finding', () => { + for (const terminal of [ + { status: 'failed', summary: 'Background command failed (exit code 1)' }, + { status: 'completed', summary: 'Background command "Run Codex adversarial review" completed (exit code 1)' }, + ]) { + const input = sdkEvents(); + input.push({ ...input[2], ...terminal }); + expect(detected(input)).toBe(false); + } + }); + + test('replayed acknowledgments preserve task state and reject conflicting launch identities', () => { + const original = sdkEvents(); + const failed = { ...original[2], status: 'failed', summary: 'Background command failed (exit code 1)' }; + expect(detected([ + ...original.slice(0, 3), failed, original[1], ...original.slice(2), + ])).toBe(false); + expect(detected([...original, original[1]])).toBe(true); + + const plain = structuredClone(original[1]); + plain.message.content[0].content = finding; + expect(detected([...original.slice(0, 2), plain])).toBe(false); + const retained = claudeOutsideExecutions([...original, plain]); + expect(retained.filter(result => result.succeeded)).toHaveLength(1); + expect(retained.find(result => result.output === finding)?.succeeded).toBe(false); + plain.message.content[0].is_error = true; + plain.message.content[0].content = 'The background launch failed.'; + expect(detected([...original, plain])).toBe(false); + + for (const alter of [ + (ack: any) => { ack.message.content[0].content = ack.message.content[0].content.replaceAll('bn8jkc52l', 'other-task'); }, + (ack: any) => { ack.session_id = 'foreign'; }, + (ack: any) => { ack.message.content[0].is_error = true; }, + ]) { + const acknowledgment = structuredClone(original[1]); + alter(acknowledgment); + expect(detected([...original, acknowledgment])).toBe(false); + } + }); + + test('partial and failed exact-path Reads remain diagnostic output without earning completion', () => { + for (const fail of [false, true]) { + const input = events(); + input[4].message.content[0].content = finding; + input[4].message.content[0].is_error = fail; + const retained = outsideExecutionTranscript(claudeOutsideExecutions(input), 'codex'); + expect(retained.at(-1)!.output).toBe(finding); + expect(retained.at(-1)!.succeeded).toBe(false); + expect(detected(input)).toBe(false); + } + }); + + test('an unsupported background acknowledgment cannot pass as a foreground result', () => { + const input = events().slice(0, 2); + input[1].message.content[0].content += '\n' + finding; + expect(detected(input)).toBe(false); + }); + + test('TaskOutput requires the exact launched task and native completed exit-zero envelope', () => { + const input = sdkEvents().slice(0, 2); + const session_id = input[0].session_id; + const result = `success\n\nbn8jkc52l\n\nlocal_bash\n\ncompleted\n\n0\n\n\n${finding}\n`; + input.push({ type: 'assistant', session_id, message: { content: [{ type: 'tool_use', name: 'TaskOutput', id: 'task-output', input: { task_id: 'bn8jkc52l' } }] } }); + input.push({ type: 'user', session_id, message: { content: [{ type: 'tool_result', tool_use_id: 'task-output', content: result }] } }); + expect(detected(input)).toBe(true); + for (const content of [ + result.replace('>success<', '>not_ready<'), result.replace('>completed<', '>running<'), + result.replace('>0<', '>1<'), result.replace('>bn8jkc52l<', '>other<'), + result.replace('>local_bash<', '>local_agent<'), finding + result, + ]) { + const invalid = structuredClone(input); + invalid[3].message.content[0].content = content; + expect(detected(invalid)).toBe(false); + } + const invalid = structuredClone(input); + invalid[3].message.content[0].is_error = true; + expect(detected(invalid)).toBe(false); + }); +}); diff --git a/test/outside-voice-fixture.test.ts b/test/outside-voice-fixture.test.ts new file mode 100644 index 000000000..6ec967377 --- /dev/null +++ b/test/outside-voice-fixture.test.ts @@ -0,0 +1,33 @@ +import { test, expect } from 'bun:test'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { createOutsideReviewRepo } from './helpers/outside-voice-fixture'; + +function git(cwd: string, ...args: string[]): string { + const result = Bun.spawnSync(['git', ...args], { cwd, stdout: 'pipe', stderr: 'pipe', timeout: 10_000 }); + if (result.exitCode !== 0) throw new Error(result.stderr.toString()); + return result.stdout.toString().trim(); +} + +for (const host of ['claude', 'codex'] as const) { + test(`${host} retries use independent committed fixtures with the same one-line defect`, () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'outside-retry-')); + try { + const first = createOutsideReviewRepo(root, host); + fs.writeFileSync(path.join(first, 'review-scratch.txt'), 'first attempt evidence'); + const second = createOutsideReviewRepo(root, host); + expect(second).not.toBe(first); + expect(fs.realpathSync(path.join(second, '.git'))).not.toBe(fs.realpathSync(path.join(first, '.git'))); + expect(fs.readFileSync(path.join(first, 'review-scratch.txt'), 'utf8')).toBe('first attempt evidence'); + expect(fs.existsSync(path.join(second, 'review-scratch.txt'))).toBe(false); + for (const repo of [first, second]) { + expect(git(repo, 'branch', '--show-current')).toBe('feature/invoice-lookup'); + expect(git(repo, 'diff', '--numstat', 'origin/main...HEAD')).toBe('0\t1\tinvoice.ts'); + expect(git(repo, 'show', 'origin/main:invoice.ts')).toContain('invoice.ownerId !== actor.id'); + expect(git(repo, 'show', 'HEAD:invoice.ts')).not.toContain('invoice.ownerId !== actor.id'); + } + expect(git(second, 'status', '--porcelain')).toBe(''); + } finally { fs.rmSync(root, { recursive: true, force: true }); } + }); +} diff --git a/test/skill-e2e-autoplan-chain.test.ts b/test/skill-e2e-autoplan-chain.test.ts index 72768c5b5..9fe61d734 100644 --- a/test/skill-e2e-autoplan-chain.test.ts +++ b/test/skill-e2e-autoplan-chain.test.ts @@ -30,7 +30,7 @@ import { isNumberedOptionListVisible, selectPtyNumberedOption, } from './helpers/claude-pty-runner'; -import { autoplanRoutingSetupInput } from './helpers/autoplan-setup-question'; +import { autoplanSetupDecision, type AutoplanSetupDecision } from './helpers/autoplan-setup-question'; import { autoplanPhaseCompletions, type AutoplanPhaseHit } from './helpers/autoplan-phase-observer'; import { readPlanCountTranscript, type PlanCountTranscript } from './helpers/plan-count-transcript'; import { createPlanCountSnapshotWriter } from './helpers/plan-count-artifacts'; @@ -81,7 +81,8 @@ describeE2E('/autoplan native chain ordering (periodic)', () => { let hits: AutoplanPhaseHit[] = []; let transcript: PlanCountTranscript = { status: 'missing', calls: [], assistantMessages: [] }; - let outcome: 'chain_complete' | 'plan_ready' | 'timeout' | 'exited' = 'timeout'; + let outcome: 'chain_complete' | 'plan_ready' | 'timeout' | 'exited' | 'unsupported_setup' = 'timeout'; + let unsupportedSetup: Extract | null = null; let evidence = ''; let viewport = ''; let fullSessionEvidence = ''; @@ -99,7 +100,8 @@ describeE2E('/autoplan native chain ordering (periodic)', () => { artifacts = saveSnapshot({ skillName: 'autoplan', cwd: tempDir, claudeConfigDir: session.hermeticConfigDir, raw: session.rawOutput(), visible: session.visibleText(), viewport, - observation: { state, hits, native: transcript, exitCode: session.exitCode() }, + observation: { state, hits, native: transcript, exitCode: session.exitCode(), unsupportedSetup, + retention: 'Current raw/visible/viewport and parsed native metadata only; full parent JSONL retention is not guaranteed.' }, }); }; @@ -147,13 +149,20 @@ describeE2E('/autoplan native chain ordering (periodic)', () => { // prerequisite. Keep the supplied plan and continue its full // review; taste decisions remain autoplan's responsibility. The helper // deduplicates the complete question before returning an input. - const setupInput = autoplanRoutingSetupInput(visible, seenSetupQuestions, transcript.calls.find(call => !call.answered && !call.failed)); - if (setupInput !== null) { - if (setupInput.includes('\r')) await selectPtyNumberedOption(session, Number(setupInput.trim())); - else session.send(setupInput); + const setup = autoplanSetupDecision(visible, seenSetupQuestions, transcript.calls.find(call => !call.answered && !call.failed)); + if (setup.kind === 'input') { + if (setup.input.includes('\r')) await selectPtyNumberedOption(session, Number(setup.input.trim())); + else session.send(setup.input); + for (const signature of setup.signatures) seenSetupQuestions.add(signature); await Bun.sleep(2000); continue; } + if (setup.kind === 'unsupported_setup') { + outcome = 'unsupported_setup'; + unsupportedSetup = setup; + evidence = viewport.slice(-3000); + break; + } // Terminal: Phase 3 (Eng) seen — chain reached the required end. if (hits.some(h => h.phase === 3)) { @@ -182,10 +191,11 @@ describeE2E('/autoplan native chain ordering (periodic)', () => { } finally { await session.close(); } } - if (outcome === 'exited' || outcome === 'timeout') { + if (outcome === 'exited' || outcome === 'timeout' || outcome === 'unsupported_setup') { throw new Error( `autoplan chain test FAILED: outcome=${outcome}, exitCode=${exitCode}, hits=${JSON.stringify(hits)}\n` + `Native transcript: ${transcript.status}; artifacts=${JSON.stringify(artifacts)}\n` + + (unsupportedSetup ? `Unsupported setup: ${JSON.stringify(unsupportedSetup)}; no input sent. Artifacts contain UI and parsed metadata, not guaranteed full parent JSONL.\n` : '') + `--- post-command evidence (last 3KB) ---\n${diagnosticTail(evidence)}\n` + `--- full-session visible tail, including startup (last 3KB) ---\n${fullSessionEvidence}`, ); diff --git a/test/skill-e2e-outside-voice.test.ts b/test/skill-e2e-outside-voice.test.ts index 75e44288f..2d20e637d 100644 --- a/test/skill-e2e-outside-voice.test.ts +++ b/test/skill-e2e-outside-voice.test.ts @@ -7,7 +7,7 @@ import { e2eTierEnabled } from './helpers/e2e-gate'; import { runSkillTest } from './helpers/session-runner'; import { runCodexSkill } from './helpers/codex-session-runner'; import { EvalCollector } from './helpers/eval-store'; -import { installOutsideReviewFixture } from './helpers/outside-voice-fixture'; +import { createOutsideReviewRepo, installOutsideReviewFixture } from './helpers/outside-voice-fixture'; import { claudeOutsideExecutions, codexOutsideExecutions, foundInvoiceAuthorizationDefect, outsideExecutionTranscript } from './helpers/outside-voice-evidence'; import { CAPTURE_LONG_MS } from './helpers/eval-budgets'; @@ -24,36 +24,6 @@ const describeLive = enabled ? describe : describe.skip; let fixtureRoot: string; let rendered: string; -function git(cwd: string, ...args: string[]) { - const result = Bun.spawnSync(['git', ...args], { cwd, stdout: 'pipe', stderr: 'pipe', timeout: 10_000 }); - if (result.exitCode !== 0) throw new Error(`git ${args[0]}: ${result.stderr.toString()}`); -} - -function createRepo(host: 'claude' | 'codex'): string { - const dir = path.join(fixtureRoot, host); - fs.mkdirSync(dir, { recursive: true }); - git(dir, 'init', '-b', 'main'); - git(dir, 'config', 'user.email', 'eval@example.com'); - git(dir, 'config', 'user.name', 'Outside Voice Eval'); - const safe = `export async function readPrivateInvoice(db, actor, invoiceId) { - const invoice = await db.invoice.findUnique({ where: { id: invoiceId } }); - if (!invoice) return null; - if (invoice.ownerId !== actor.id) throw new Error('Forbidden'); - return { amount: invoice.amount, bankAccount: invoice.bankAccount }; -} -`; - fs.writeFileSync(path.join(dir, 'invoice.ts'), safe); - git(dir, 'add', 'invoice.ts'); - git(dir, 'commit', '-m', 'Protect private invoices by owner'); - git(dir, 'update-ref', 'refs/remotes/origin/main', 'HEAD'); - git(dir, 'checkout', '-b', 'feature/invoice-lookup'); - fs.writeFileSync(path.join(dir, 'invoice.ts'), safe.replace(" if (invoice.ownerId !== actor.id) throw new Error('Forbidden');\n", '')); - git(dir, 'add', 'invoice.ts'); - git(dir, 'commit', '-m', 'Simplify invoice lookup'); - return dir; -} - - function prompt(skill: string): string { return `Run the installed ${skill} skill on this branch. The base branch is main (origin/main exists locally). Complete its adversarial review workflow and report the outside provider's actual findings separately from the native review. Review only; leave source files unchanged. All reviewer calls in this skill are authorized. If a native subagent tool is unavailable, report that and continue the independent outside CLI pass. Do not claim success if the outside CLI cannot complete.`; } @@ -74,7 +44,7 @@ describeLive('Installed workflows dispatch outside their host harness', () => { }); testIfSelected('outside-voice-codex-to-claude-code', async () => { - const repo = createRepo('codex'); + const repo = createOutsideReviewRepo(fixtureRoot, 'codex'); const skillDir = installOutsideReviewFixture(rendered, 'codex', repo, ROOT); const result = await runCodexSkill({ skillDir, skillName: 'gstack-review', cwd: repo, prompt: prompt('gstack-review'), @@ -95,7 +65,7 @@ describeLive('Installed workflows dispatch outside their host harness', () => { }); testIfSelected('outside-voice-claude-code-to-codex', async () => { - const repo = createRepo('claude'); + const repo = createOutsideReviewRepo(fixtureRoot, 'claude'); installOutsideReviewFixture(rendered, 'claude', repo, ROOT); const env: Record = {}; // Admit only Codex authentication/config location, never the operator's diff --git a/test/skill-e2e-plan-ceo-finding-count.test.ts b/test/skill-e2e-plan-ceo-finding-count.test.ts index e3e8b79c2..44593326f 100644 --- a/test/skill-e2e-plan-ceo-finding-count.test.ts +++ b/test/skill-e2e-plan-ceo-finding-count.test.ts @@ -82,6 +82,9 @@ const planCeo5Findings = (planPath: string) => [ 'The existing payload adapter exposes `event.data.object.metadata.user_id`', 'as `request.params.userId`. This params object is the parsed body-data map,', 'not URL query/path parameters; all users share one webhook URL.', + 'The adapter acknowledges missing, nil, or empty user_id metadata with', + 'HTTP 200 and an event-correlated warning before invoking this handler.', + 'For every nonempty external string it performs no SQL-format validation.', 'The adapter forwards that external string unchanged. It does not cast,', 'escape, or SQL-sanitize it; a valid signature does not make it safe for SQL.', 'The existing webhook event guard deduplicates deliveries by Stripe event ID,', @@ -92,12 +95,21 @@ const planCeo5Findings = (planPath: string) => [ 'HTTP 200, logs the event, and stops before user updates or email fan-out.', 'The ingress wrapper already logs event IDs, outcomes, and durations, with', 'alerts for failed webhook processing. Those controls remain in place.', + 'The existing DB and mail clients attach the adapter user ID and event ID', + 'to outcome traces, including update success and email delivery success or', + 'failure. These shared clients rethrow exceptions unchanged; tracing does', + 'not rescue email errors or change the inline email call below.', 'DB lookup/update exceptions propagate to that ingress wrapper, which logs', 'the failure and returns HTTP 500 so Stripe retries the event. The existing', 'event-ID dedup guard records completion only after the database transaction', 'commits; failed or rolled-back database attempts remain retryable.', 'The deployment already has a handler feature flag and a documented, tested', 'rollback to the prior handler; this change uses that existing rollout path.', + 'That documented manual rollout checklist already requires a staging', + 'payment-event replay for this handler and verification of the user update,', + 'email delivery, and correlated outcome trace before enabling it broadly.', + 'This is manual deployment verification, not automated handler regression', + 'coverage; no new automated tests are planned in the Tests section below.', '', '## Architecture', "We're adding a new `StripePaymentWebhookHandler` class that will handle Stripe webhooks.",