diff --git a/autoplan/SKILL.md b/autoplan/SKILL.md index 94c570f6e..b366fdd4d 100644 --- a/autoplan/SKILL.md +++ b/autoplan/SKILL.md @@ -683,9 +683,9 @@ State what you examined and why nothing was flagged (1-2 sentences minimum). ``` Phase: `ceo|design|dx|eng`. Record accepted requirements here; no analysis/severity/verdict/consensus. No changes: `None: reason`. -`amend` copies atomically, checks exact retention, returns full Implementation; -None checks unchanged. Prior blocks immutable; state replacements in current block. -Reconcile all decisions with block/readback: +`amend` checks exact retention atomically; full readback; None unchanged. +Baseline edits: `create`'s `baselineEdits`. Prior blocks immutable; +state replacements in current block. Reconcile all decisions with readback. Transport ≠ approval/complete enumeration/correctness. --- diff --git a/autoplan/SKILL.md.tmpl b/autoplan/SKILL.md.tmpl index e050eb52a..861f8e24e 100644 --- a/autoplan/SKILL.md.tmpl +++ b/autoplan/SKILL.md.tmpl @@ -168,9 +168,9 @@ State what you examined and why nothing was flagged (1-2 sentences minimum). ``` Phase: `ceo|design|dx|eng`. Record accepted requirements here; no analysis/severity/verdict/consensus. No changes: `None: reason`. -`amend` copies atomically, checks exact retention, returns full Implementation; -None checks unchanged. Prior blocks immutable; state replacements in current block. -Reconcile all decisions with block/readback: +`amend` checks exact retention atomically; full readback; None unchanged. +Baseline edits: `create`'s `baselineEdits`. Prior blocks immutable; +state replacements in current block. Reconcile all decisions with readback. Transport ≠ approval/complete enumeration/correctness. --- diff --git a/autoplan/sections/ceo-phase.md b/autoplan/sections/ceo-phase.md index cbaa7c841..35f2fd6fa 100644 --- a/autoplan/sections/ceo-phase.md +++ b/autoplan/sections/ceo-phase.md @@ -1,6 +1,6 @@ -Before dispatch, fully Read `~/.claude/skills/gstack/plan-ceo-review/SKILL.md` + triggered sections; record successful start/end/total ranges; fill gaps to EOF. Load skip-listed sections; skip execution. +Before dispatch, fully Read `~/.claude/skills/gstack/plan-ceo-review/SKILL.md` and `sections/review-sections.md` beside it; log successful start/end/total; fill gaps to EOF. Skip-listed: load only. **Override rules:** - Mode selection: SELECTIVE EXPANSION diff --git a/autoplan/sections/ceo-phase.md.tmpl b/autoplan/sections/ceo-phase.md.tmpl index 773b2ba1f..f69723085 100644 --- a/autoplan/sections/ceo-phase.md.tmpl +++ b/autoplan/sections/ceo-phase.md.tmpl @@ -1,4 +1,4 @@ -Before dispatch, fully Read {{AUTOPLAN_REVIEW_FILE:plan-ceo-review}} + triggered sections; record successful start/end/total ranges; fill gaps to EOF. Load skip-listed sections; skip execution. +Before dispatch, fully Read {{AUTOPLAN_REVIEW_FILE:plan-ceo-review:with-sections}}; log successful start/end/total; fill gaps to EOF. Skip-listed: load only. **Override rules:** - Mode selection: SELECTIVE EXPANSION diff --git a/autoplan/sections/design-phase.md b/autoplan/sections/design-phase.md index 8e55f1d2a..0c1777a34 100644 --- a/autoplan/sections/design-phase.md +++ b/autoplan/sections/design-phase.md @@ -1,6 +1,6 @@ -Before dispatch, fully Read `~/.claude/skills/gstack/plan-design-review/SKILL.md` + triggered sections; record successful start/end/total ranges; fill gaps to EOF. Load skip-listed sections; skip execution. +Before dispatch, fully Read `~/.claude/skills/gstack/plan-design-review/SKILL.md` and `sections/review-sections.md` beside it; log successful start/end/total; fill gaps to EOF. Skip-listed: load only. **Override rules:** - Focus areas: all relevant dimensions (P1) diff --git a/autoplan/sections/design-phase.md.tmpl b/autoplan/sections/design-phase.md.tmpl index ce20f5ce1..c5266bcb6 100644 --- a/autoplan/sections/design-phase.md.tmpl +++ b/autoplan/sections/design-phase.md.tmpl @@ -1,4 +1,4 @@ -Before dispatch, fully Read {{AUTOPLAN_REVIEW_FILE:plan-design-review}} + triggered sections; record successful start/end/total ranges; fill gaps to EOF. Load skip-listed sections; skip execution. +Before dispatch, fully Read {{AUTOPLAN_REVIEW_FILE:plan-design-review:with-sections}}; log successful start/end/total; fill gaps to EOF. Skip-listed: load only. **Override rules:** - Focus areas: all relevant dimensions (P1) diff --git a/autoplan/sections/dx-phase.md b/autoplan/sections/dx-phase.md index 48285765d..779c741ee 100644 --- a/autoplan/sections/dx-phase.md +++ b/autoplan/sections/dx-phase.md @@ -1,6 +1,6 @@ -Before dispatch, fully Read `~/.claude/skills/gstack/plan-devex-review/SKILL.md` + triggered sections; record successful start/end/total ranges; fill gaps to EOF. Load skip-listed sections; skip execution. +Before dispatch, fully Read `~/.claude/skills/gstack/plan-devex-review/SKILL.md` and `sections/review-sections.md` beside it; log successful start/end/total; fill gaps to EOF. Skip-listed: load only. **Override rules:** - Mode selection: DX POLISH diff --git a/autoplan/sections/dx-phase.md.tmpl b/autoplan/sections/dx-phase.md.tmpl index 174dbdf96..03183d36b 100644 --- a/autoplan/sections/dx-phase.md.tmpl +++ b/autoplan/sections/dx-phase.md.tmpl @@ -1,4 +1,4 @@ -Before dispatch, fully Read {{AUTOPLAN_REVIEW_FILE:plan-devex-review}} + triggered sections; record successful start/end/total ranges; fill gaps to EOF. Load skip-listed sections; skip execution. +Before dispatch, fully Read {{AUTOPLAN_REVIEW_FILE:plan-devex-review:with-sections}}; log successful start/end/total; fill gaps to EOF. Skip-listed: load only. **Override rules:** - Mode selection: DX POLISH diff --git a/autoplan/sections/eng-phase.md b/autoplan/sections/eng-phase.md index 6dedc21b2..b585d82b2 100644 --- a/autoplan/sections/eng-phase.md +++ b/autoplan/sections/eng-phase.md @@ -1,6 +1,6 @@ -Before dispatch, fully Read `~/.claude/skills/gstack/plan-eng-review/SKILL.md` + triggered sections; record successful start/end/total ranges; fill gaps to EOF. Load skip-listed sections; skip execution. +Before dispatch, fully Read `~/.claude/skills/gstack/plan-eng-review/SKILL.md` and `sections/review-sections.md` beside it; log successful start/end/total; fill gaps to EOF. Skip-listed: load only. **Override rules:** - Scope challenge: never reduce (P2) diff --git a/autoplan/sections/eng-phase.md.tmpl b/autoplan/sections/eng-phase.md.tmpl index c9bd4d90a..d091277b3 100644 --- a/autoplan/sections/eng-phase.md.tmpl +++ b/autoplan/sections/eng-phase.md.tmpl @@ -1,4 +1,4 @@ -Before dispatch, fully Read {{AUTOPLAN_REVIEW_FILE:plan-eng-review}} + triggered sections; record successful start/end/total ranges; fill gaps to EOF. Load skip-listed sections; skip execution. +Before dispatch, fully Read {{AUTOPLAN_REVIEW_FILE:plan-eng-review:with-sections}}; log successful start/end/total; fill gaps to EOF. Skip-listed: load only. **Override rules:** - Scope challenge: never reduce (P2) diff --git a/bin/gstack-autoplan-snapshot.ts b/bin/gstack-autoplan-snapshot.ts index 0611fdb14..72a46e89b 100644 --- a/bin/gstack-autoplan-snapshot.ts +++ b/bin/gstack-autoplan-snapshot.ts @@ -181,14 +181,95 @@ function obligationState(plan: string, phase: string, prior: string) { return { bounds, implementation, recorded, applied, block }; } +// A small exact-replacement record explains baseline byte changes. It cannot +// establish who approved them or whether every decision was recorded correctly. +function baselineEditRecords(review: string) { + let fence: { char: string; length: number } | null = null; + const records = new Map }>(); + for (const raw of review.split(/(?<=\n)/)) { + const line = raw.replace(/\r?\n$/, ''); + const delimiter = /^ {0,3}(`{3,}|~{3,})(.*)$/.exec(line); + if (delimiter) { + const run = delimiter[1]!; + if (fence) { + if (run[0] === fence.char && run.length >= fence.length && !delimiter[2]!.trim()) fence = null; + } else if (run[0] !== '`' || !delimiter[2]!.includes('`')) fence = { char: run[0]!, length: run.length }; + } else if (!fence && /^$/.exec(line); + if (!marker || records.has(marker[1]!)) throw new Error('Malformed or duplicate baseline-edit record'); + const record = JSON.parse(marker[2]!); + // Canonical compact JSON also rejects duplicate keys and hidden extra fields. + if (!record || Object.keys(record).join(',') !== 'sourceSha256,replacements' || + !/^[a-f0-9]{64}$/.test(record.sourceSha256) || !Array.isArray(record.replacements) || + JSON.stringify(record) !== marker[2]) throw new Error('Expected exact compact baseline-edit JSON'); + for (const edit of record.replacements) { + if (!edit || Object.keys(edit).join(',') !== 'oldText,newText' || typeof edit.oldText !== 'string' || + !edit.oldText || typeof edit.newText !== 'string' || + [edit.oldText, edit.newText].some(value => Buffer.from(value).toString('utf8') !== value)) { + throw new Error('Baseline replacements require nonempty oldText and UTF-8 newText only'); + } + } + records.set(marker[1]!, record); + } + } + return records; +} + +function editedBaseline(review: string, phase: string, prior: string): string { + const record = baselineEditRecords(review).get(phase); + if (!record) return prior; + if (record.sourceSha256 !== sha256(prior)) throw new Error('Baseline-edit source SHA does not match immutable input'); + const protectedBlocks = [...acceptedBlocks(prior).values()]; + const spans = record.replacements.map(edit => { + const start = prior.indexOf(edit.oldText); + if (start < 0 || prior.indexOf(edit.oldText, start + 1) >= 0) throw new Error('Baseline oldText must occur exactly once'); + const end = start + edit.oldText.length; + if (protectedBlocks.some(block => start < block.end && end > block.start)) { + throw new Error('Baseline replacements cannot touch prior accepted blocks'); + } + return { ...edit, start, end }; + }).sort((a, b) => a.start - b.start); + for (let i = 1; i < spans.length; i++) { + if (spans[i]!.start < spans[i - 1]!.end) throw new Error('Baseline replacement anchors overlap'); + } + let result = prior; + for (const edit of spans.reverse()) result = result.slice(0, edit.start) + edit.newText + result.slice(edit.end); + if (baselineEditRecords(result).size) throw new Error('Baseline-edit metadata belongs only in Review record'); + // Edits may change requirements, never manufacture or hide retention structure. + const afterBlocks = acceptedBlocks(result); + if (afterBlocks.size !== protectedBlocks.length || protectedBlocks.some(block => afterBlocks.get(block.phase)?.raw !== block.raw)) { + throw new Error('Baseline replacements changed accepted-block structure'); + } + return result; +} + +function withAcceptedBlock(baseline: string, block: AcceptedBlock) { + const existing = acceptedBlocks(baseline).get(block.phase); + return block.none ? baseline : existing + ? baseline.slice(0, existing.start) + block.raw + block.newline + baseline.slice(existing.end) + : baseline + (baseline.endsWith('\n') ? '' : '\n') + '\n' + block.raw + block.newline; +} + +function expectedAmendment(plan: string, phase: string, prior: string, state: ReturnType) { + const baseline = editedBaseline(plan.slice(state.bounds.reviewStart), phase, prior); + const implementation = withAcceptedBlock(baseline, state.block); + if (state.block.none && baseline !== prior) throw new Error('None cannot authorize baseline replacements'); + return { baseline, implementation }; +} + /** The CLI close check adds exact recorded-obligation retention to the byte check. */ export function checkPhaseImplementation(phase: string, activePlan: string, snapshotPath: string, expected: string) { const checked = checkImplementation(phase, activePlan, snapshotPath, expected); - const state = obligationState(readFileSync(checked.activePlan, 'utf8'), phase, snapshotIdentity(phase, activePlan, snapshotPath).original); + const plan = readFileSync(checked.activePlan, 'utf8'); + const prior = snapshotIdentity(phase, activePlan, snapshotPath).original; + const state = obligationState(plan, phase, prior); if (state.block.none ? state.applied.has(phase) : state.applied.get(phase)?.raw !== state.block.raw) { throw new Error(`Accepted ${phase} obligations are not retained exactly in Implementation plan; run amend`); } if (state.block.none && expected !== 'unchanged') throw new Error('None requires unchanged with its recorded reason'); + if (state.implementation !== expectedAmendment(plan, phase, prior, state).implementation) { + throw new Error('Unrecorded Implementation rewrite; preserve immutable input or record exact baseline replacements'); + } return { ...checked, recordedObligations: { phase, sha256: sha256(state.block.raw), none: state.block.none }, limitation: 'Exact recorded text retained; approval, enumeration and semantic correctness still require review.' }; } @@ -201,11 +282,17 @@ export function amendImplementation(phase: string, activePlan: string, snapshotP // Reuse the unchanged snapshot identity checks without assuming current changes. checkImplementation(phase, source, snapshotPath, extractImplementationPlan(original) === prior ? 'unchanged' : 'changed'); const state = obligationState(original, phase, prior); + const planned = expectedAmendment(original, phase, prior, state); + const allowed = [prior, planned.baseline, planned.implementation]; + const current = state.applied.get(phase); + // The current phase may accumulate more obligations after an earlier amend. + // Only its block may differ; its surrounding baseline must still be exact. + if (current) allowed.push(withAcceptedBlock(prior, current), withAcceptedBlock(planned.baseline, current)); + if (!allowed.includes(state.implementation)) { + throw new Error('Unrecorded Implementation rewrite; no overwrite; preserve input or record exact baseline replacements'); + } if (state.block.none) return checkPhaseImplementation(phase, source, snapshotPath, 'unchanged'); - const existing = state.applied.get(phase); - const nextImplementation = existing - ? state.implementation.slice(0, existing.start) + state.block.raw + state.block.newline + state.implementation.slice(existing.end) - : state.implementation + (state.implementation.endsWith('\n') ? '' : '\n') + '\n' + state.block.raw + state.block.newline; + const nextImplementation = planned.implementation; const next = original.slice(0, state.bounds.start) + nextImplementation + original.slice(state.bounds.end); // Validate the assembled text before publishing, including its section boundary. const validated = obligationState(next, phase, prior); @@ -393,7 +480,10 @@ If the file cannot be fully read, report the read failure instead of a completed writeFileSync(snapshotPath, content, { flag: 'wx', mode: 0o444 }); writeFileSync(nativePromptPath, nativePrompt, { flag: 'wx', mode: 0o444 }); writeFileSync(join(directory, 'snapshot.json'), JSON.stringify(manifest) + '\n', { flag: 'wx', mode: 0o444 }); - return { ...manifest, nativePrompt }; + return { ...manifest, nativePrompt, baselineEdits: { + record: ``, + instructions: 'Optional: put one unfenced record in Review record only. Use this exact compact JSON shape; each replacement is {"oldText":"exact unique old span","newText":"replacement (empty deletes)"}. Bind sourceSha256 to this snapshot. Replacements must not overlap or touch accepted blocks. Leave all other Implementation bytes intact; amend also accepts the untouched snapshot baseline. Describe approved replacements in current accepted requirements. This verifies explained bytes, not approval or completeness.' + } }; } catch (error) { rmSync(directory, { recursive: true, force: true }); throw error; diff --git a/plan-design-review/sections/review-sections.md b/plan-design-review/sections/review-sections.md index ca405d275..4a654d764 100644 --- a/plan-design-review/sections/review-sections.md +++ b/plan-design-review/sections/review-sections.md @@ -211,6 +211,7 @@ If visual mockups were generated in Step 0.5, evaluate them against the AI slop Rate 0-10: Does the plan align with DESIGN.md? FIX TO 10: If DESIGN.md exists, annotate with specific tokens/components; when it has YAML front matter (the open DESIGN.md format), cite tokens by path (`{colors.primary}`, `{rounded.md}`) so the plan and the file share one vocabulary. If no DESIGN.md, flag the gap and recommend `/design-consultation`. Flag any new component — does it fit the existing vocabulary? +Before offering a token-alignment fix, check whether an earlier pass already approved that outcome. If so, apply the established tokens and update every stale gap/reference under that decision; changing the plan location or spelling out the same fix is not a new issue. Ask again only if new evidence exposes an unresolved requirement or tradeoff, and name it. An unapproved violation still needs its first individual decision. **STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. ### Pass 6: Responsive & Accessibility diff --git a/plan-design-review/sections/review-sections.md.tmpl b/plan-design-review/sections/review-sections.md.tmpl index 0bdd2a918..ea21fa328 100644 --- a/plan-design-review/sections/review-sections.md.tmpl +++ b/plan-design-review/sections/review-sections.md.tmpl @@ -76,6 +76,7 @@ If visual mockups were generated in Step 0.5, evaluate them against the AI slop Rate 0-10: Does the plan align with DESIGN.md? FIX TO 10: If DESIGN.md exists, annotate with specific tokens/components; when it has YAML front matter (the open DESIGN.md format), cite tokens by path (`{colors.primary}`, `{rounded.md}`) so the plan and the file share one vocabulary. If no DESIGN.md, flag the gap and recommend `/design-consultation`. Flag any new component — does it fit the existing vocabulary? +Before offering a token-alignment fix, check whether an earlier pass already approved that outcome. If so, apply the established tokens and update every stale gap/reference under that decision; changing the plan location or spelling out the same fix is not a new issue. Ask again only if new evidence exposes an unresolved requirement or tradeoff, and name it. An unapproved violation still needs its first individual decision. **STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. ### Pass 6: Responsive & Accessibility diff --git a/scripts/resolvers/composition.ts b/scripts/resolvers/composition.ts index 20800d412..5579c082a 100644 --- a/scripts/resolvers/composition.ts +++ b/scripts/resolvers/composition.ts @@ -56,7 +56,14 @@ export function generateAutoplanReviewFile(ctx: TemplateContext, args?: string[] if (!skill || !['plan-ceo-review', 'plan-design-review', 'plan-devex-review', 'plan-eng-review'].includes(skill)) { throw new Error('AUTOPLAN_REVIEW_FILE requires an autoplan review skill'); } - if (ctx.host === 'claude') return `\`${ctx.paths.skillRoot}/${skill}/SKILL.md\``; + const withSections = args?.[1] === 'with-sections'; + if ((args?.length ?? 0) > 2 || (args?.[1] !== undefined && !withSections)) { + throw new Error('AUTOPLAN_REVIEW_FILE only accepts with-sections'); + } + if (ctx.host === 'claude') { + const entry = `\`${ctx.paths.skillRoot}/${skill}/SKILL.md\``; + return withSections ? `${entry} and \`sections/review-sections.md\` beside it` : entry; + } const host = getHostConfig(ctx.host); const file = `gstack-${skill}/SKILL.md`; @@ -65,6 +72,7 @@ export function generateAutoplanReviewFile(ctx: TemplateContext, args?: string[] // Resolve from the discovered entrypoint's directory: GSTACK_ROOT is an // independently configurable runtime asset tree, not a skill registry. // This also preserves custom CODEX_HOME installations without guessing HOME. + // Other hosts inline the review sections into this full registry file. return `the sibling registry file \`../${file}\`, relative to the installed \`/autoplan\` SKILL.md directory (local: \`${local}\`; global: \`${global}\`${ctx.host === 'codex' ? ', or the corresponding skills directory under CODEX_HOME when configured' : ''})`; } diff --git a/test/autoplan-obligations.test.ts b/test/autoplan-obligations.test.ts index b909ee557..89c4c699b 100644 --- a/test/autoplan-obligations.test.ts +++ b/test/autoplan-obligations.test.ts @@ -8,6 +8,7 @@ import { createSnapshot, extractImplementationPlan } from '../bin/gstack-autopla const TOOL = join(import.meta.dir, '../bin/gstack-autoplan-snapshot.ts'); const captured = JSON.parse(readFileSync(join(import.meta.dir, 'fixtures/autoplan/t-ceo-omitted-obligations.json'), 'utf8')); +const lost = JSON.parse(readFileSync(join(import.meta.dir, 'fixtures/autoplan/u-ceo-original-loss.json'), 'utf8')); const owned: string[] = []; afterEach(() => { for (const dir of owned.splice(0)) rmSync(dir, { recursive: true, force: true }); }); @@ -44,6 +45,10 @@ test('whole recorded T obligations retain omitted guards and every nested verifi writeFileSync(f.active, captured.activeAtBoundary + accepted); const reviewBefore = readFileSync(f.active, 'utf8').split('## Review record\n')[1]; expect(invoke('check', 'ceo', f.active, f.snapshot.snapshotPath, 'changed').status).toBe(1); + const rewritten = readFileSync(f.active, 'utf8'); + expect(invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath).status).toBe(1); + expect(readFileSync(f.active, 'utf8')).toBe(rewritten); + writeFileSync(f.active, '## Implementation plan\n' + captured.initialImplementation + '## Review record\n' + reviewBefore); const amended = invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath); expect(amended.status, amended.stderr).toBe(0); const checked = invoke('check', 'ceo', f.active, f.snapshot.snapshotPath, 'changed'); @@ -66,6 +71,135 @@ test('whole recorded T obligations retain omitted guards and every nested verifi expect(readFileSync(f.restore, 'utf8')).toBe('Original restore bytes\n'); }); +const editRecord = (phase: string, sourceSha256: string, replacements: Array<{ oldText: string; newText: string }>) => + `\n`; + +test('actual U canonical block cannot conceal a rewritten original baseline at amend or check', () => { + const f = setup(lost.initialImplementation); + writeFileSync(f.active, lost.activeAfterAmend); + for (const command of ['amend', 'check']) { + const result = invoke(command, 'ceo', f.active, f.snapshot.snapshotPath, ...(command === 'check' ? ['changed'] : [])); + expect(result.status).toBe(1); + expect(result.stderr).toContain('Unrecorded Implementation rewrite'); + expect(readFileSync(f.active, 'utf8')).toBe(lost.activeAfterAmend); + } +}); + +test('unchanged U source retains every original byte when accepted requirements are appended', () => { + const f = setup(lost.initialImplementation); + appendRecord(f.active, block('ceo', '- Preserve each contract and add a loading state.\n Verify: reject cross-workspace requests.')); + const amended = invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath); + expect(amended.status, amended.stderr).toBe(0); + expect(JSON.parse(amended.stdout).implementation.startsWith(lost.initialImplementation)).toBe(true); + const next = createSnapshot('design', f.active, f.restore); + expect(readFileSync(next.snapshotPath, 'utf8').startsWith(lost.initialImplementation)).toBe(true); + expect(readFileSync(f.snapshot.sourceSnapshotPath, 'utf8')).toBe(lost.initialImplementation); +}); + +test('exact replacements and deletion preserve untouched CRLF/Unicode bytes and produce only effective blind input', () => { + const baseline = 'Keep café ✓.\r\nUse a blue button.\r\nObsolete behavior.\r\nKeep 日本語.\r\n'; + for (const alreadyEdited of [false, true]) { + const f = setup(baseline); + const replacements = [{ oldText: 'blue', newText: 'green' }, { oldText: 'Obsolete behavior.\r\n', newText: '' }]; + const edited = baseline.replace('blue', 'green').replace('Obsolete behavior.\r\n', ''); + if (alreadyEdited) writeFileSync(f.active, `## Implementation plan\n${edited}## Review record\n`); + appendRecord(f.active, editRecord('ceo', f.snapshot.sourceSha256, replacements) + + block('ceo', '- Replace blue with green; remove obsolete behavior.\n Verify: green renders; obsolete behavior is absent.')); + const result = invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath); + expect(result.status, result.stderr).toBe(0); + expect(JSON.parse(result.stdout).implementation.startsWith(edited)).toBe(true); + const first = readFileSync(f.active, 'utf8'); + expect(invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath).status).toBe(0); + expect(readFileSync(f.active, 'utf8')).toBe(first); + expect(invoke('check', 'ceo', f.active, f.snapshot.snapshotPath, 'changed').status).toBe(0); + const next = createSnapshot('design', f.active, f.restore); + expect(next.nativePrompt).not.toContain('autoplan-baseline-edits'); + expect(next.nativePrompt).not.toContain('Use a blue button.'); + expect(next.nativePrompt.endsWith(readFileSync(next.snapshotPath, 'utf8'))).toBe(true); + expect(readFileSync(next.snapshotPath, 'utf8').startsWith(edited)).toBe(true); + } +}); + +test('baseline edit record rejects stale source, ambiguous/overlapping anchors and malformed or quoted edits without writes', () => { + const f = setup('Keep owner permission.\nRepeat repeat.\n'); + const original = readFileSync(f.active, 'utf8'); + const accepted = block('ceo', '- Preserve scope.\n Verify: permission is checked.'); + const hash = f.snapshot.sourceSha256; + const good = editRecord('ceo', hash, [{ oldText: 'owner', newText: 'member' }]); + const bad = [ + editRecord('ceo', '0'.repeat(64), [{ oldText: 'owner', newText: 'member' }]), + editRecord('ceo', hash, [{ oldText: '', newText: 'inserted' }]), + editRecord('ceo', hash, [{ oldText: 'missing', newText: 'present' }]), + editRecord('ceo', hash, [{ oldText: 'e', newText: 'E' }]), + editRecord('ceo', hash, [{ oldText: 'owner', newText: 'member' }, { oldText: 'owner', newText: 'admin' }]), + editRecord('ceo', hash, [{ oldText: 'owner permission', newText: 'member' }, { oldText: 'permission', newText: 'scope' }]), + editRecord('ceo', hash, [{ oldText: 'owner', newText: '\ud800' }]), + good + good, good.replace('"replacements":', '"unknown":'), good.replace('ceo ', 'invalid '), + good.replace(' -->', ''), good.replace('"oldText":"owner"', '"oldText":"owner","extra":true'), + good.replace('"oldText":"owner"', '"oldText":"other","oldText":"owner"'), + editRecord('ceo', hash, [{ oldText: 'owner', newText: '\n' + good }]), + editRecord('ceo', hash, [{ oldText: 'owner', newText: 'owner\n## Review record\n' }]), + ]; + for (const record of bad) { + const plan = original + accepted + record; writeFileSync(f.active, plan); + expect(invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath).status, record).toBe(1); + expect(readFileSync(f.active, 'utf8')).toBe(plan); + } + for (const record of ['```html\n' + good + '```\n', '> ' + good, ' ' + good]) { + const plan = original.replace('owner', 'member') + accepted + record; writeFileSync(f.active, plan); + expect(invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath).status).toBe(1); + expect(readFileSync(f.active, 'utf8')).toBe(plan); + } +}); + +test('later exact baseline revisions preserve earlier accepted blocks and reject edits into them', () => { + const f = setup('Use blue.\n'); + const ceo = block('ceo', '- Preserve owner authorization.\n Verify: reject cross-user access.'); + appendRecord(f.active, ceo); + expect(invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath).status).toBe(0); + const design = createSnapshot('design', f.active, f.restore); + const original = readFileSync(f.active, 'utf8'); + const accepted = block('design', '- Replace the blue baseline with green.\n Verify: green keeps the authorized action.'); + for (const oldText of ['owner authorization', ceo, 'Use blue.\n\n' + ceo]) { + const plan = original + accepted + editRecord('design', design.sourceSha256, [{ oldText, newText: 'replacement' }]); + writeFileSync(f.active, plan); + expect(invoke('amend', 'design', f.active, design.snapshotPath).status).toBe(1); + expect(readFileSync(f.active, 'utf8')).toBe(plan); + } + writeFileSync(f.active, original + accepted + editRecord('design', design.sourceSha256, [{ oldText: 'blue', newText: 'green' }])); + const result = invoke('amend', 'design', f.active, design.snapshotPath); + expect(result.status, result.stderr).toBe(0); + expect(JSON.parse(result.stdout).implementation).toContain(ceo); + expect(JSON.parse(result.stdout).implementation.startsWith('Use green.\n')).toBe(true); + const next = createSnapshot('dx', f.active, f.restore); + expect(readFileSync(next.snapshotPath, 'utf8')).toContain('Preserve owner authorization.'); + expect(next.nativePrompt).not.toContain('sourceSha256'); +}); + +test('empty exact-edit list supports honest unchanged closure; declared edits cannot hide behind None', () => { + const f = setup(); const original = readFileSync(f.active, 'utf8'); + appendRecord(f.active, block('ceo', 'None: Existing baseline suffices.') + editRecord('ceo', f.snapshot.sourceSha256, [])); + const result = invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath); + expect(result.status, result.stderr).toBe(0); + expect(JSON.parse(result.stdout).changed).toBe(false); + writeFileSync(f.active, original + block('ceo', 'None: Existing baseline suffices.') + + editRecord('ceo', f.snapshot.sourceSha256, [{ oldText: 'dashboard', newText: 'inbox' }])); + const before = readFileSync(f.active, 'utf8'); + expect(invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath).status).toBe(1); + expect(readFileSync(f.active, 'utf8')).toBe(before); +}); + +test('create returns an exact current-source edit record without adding reviewer metadata to immutable payloads', () => { + const f = setup(); + expect(f.snapshot.baselineEdits.record).toBe(editRecord('ceo', f.snapshot.sourceSha256, []).trimEnd()); + expect(f.snapshot.baselineEdits.instructions).toContain('not approval or completeness'); + const manifest = readFileSync(join(f.snapshot.snapshotPath, '..', 'snapshot.json'), 'utf8'); + expect(manifest).not.toContain('baselineEdits'); + expect(readFileSync(f.snapshot.nativePromptPath, 'utf8')).toBe(f.snapshot.nativePrompt); + expect(f.snapshot.nativePrompt).not.toContain('autoplan-baseline-edits'); + expect(statSync(f.snapshot.sourceSnapshotPath).mode & 0o777).toBe(0o444); +}); + test('amend is idempotent and check rejects a dropped condition or verification line', () => { const f = setup(); const accepted = block('ceo', '- Disable while pending.\n Verify: two clicks fire one request.'); appendRecord(f.active, accepted); @@ -79,6 +213,27 @@ test('amend is idempotent and check rejects a dropped condition or verification expect(checked.stderr).toContain('not retained exactly'); }); +test('the current phase can grow its accepted block without permitting an unrelated baseline rewrite', () => { + const f = setup('Use blue.\nKeep ownership checks.\n'); + const first = block('ceo', '- Disable the action while pending.\n Verify: one request.'); + const second = block('ceo', '- Disable the action while pending.\n Verify: one request.\n- Replace blue with green.\n Verify: green renders.'); + appendRecord(f.active, first); + expect(invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath).status).toBe(0); + const current = readFileSync(f.active, 'utf8'); + const boundary = current.indexOf('## Review record\n'); + const revised = current.slice(0, boundary) + current.slice(boundary).replace(first, second) + + editRecord('ceo', f.snapshot.sourceSha256, [{ oldText: 'blue', newText: 'green' }]); + writeFileSync(f.active, revised.replace('Keep ownership checks.\n', '')); + const invalid = readFileSync(f.active, 'utf8'); + expect(invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath).status).toBe(1); + expect(readFileSync(f.active, 'utf8')).toBe(invalid); + writeFileSync(f.active, revised); + const result = invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath); + expect(result.status, result.stderr).toBe(0); + expect(JSON.parse(result.stdout).implementation).toBe('Use green.\nKeep ownership checks.\n\n' + second); + expect(invoke('check', 'ceo', f.active, f.snapshot.snapshotPath, 'changed').status).toBe(0); +}); + test('none requires a reason and unchanged implementation, without creating a fake amendment', () => { const f = setup(); appendRecord(f.active, block('ceo', 'None: All current requirements were retained.')); expect(invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath).status).toBe(0); diff --git a/test/autoplan-phase-order.test.ts b/test/autoplan-phase-order.test.ts index f4924db9f..5ca06001a 100644 --- a/test/autoplan-phase-order.test.ts +++ b/test/autoplan-phase-order.test.ts @@ -79,11 +79,12 @@ describe('autoplan phase execution checkpoints', () => { expect(intake).toContain('Do not prefetch future phase sections or review skills'); for (const phase of phases) { const section = read(`autoplan/sections/${phase}-phase.md.tmpl`); - expect(section).toMatch(/^Before dispatch, fully Read \{\{AUTOPLAN_REVIEW_FILE:plan-[a-z-]+\}\} \+ triggered sections/); + expect(section).toMatch(/^Before dispatch, fully Read \{\{AUTOPLAN_REVIEW_FILE:plan-[a-z-]+:with-sections\}\}/); const load = section.split('**Override rules:**')[0]!; - expect(load).toContain('record successful start/end/total ranges'); + expect(load).toContain('log successful start/end/total'); expect(load).toContain('fill gaps to EOF'); - expect(load).toContain('Load skip-listed sections; skip execution'); + expect(load).toContain('Skip-listed: load only'); + expect(section.indexOf(':with-sections}}')).toBeLessThan(section.indexOf('create ' + phase)); } }); diff --git a/test/autoplan-review-discovery.test.ts b/test/autoplan-review-discovery.test.ts index b3423b83a..b84bb55a2 100644 --- a/test/autoplan-review-discovery.test.ts +++ b/test/autoplan-review-discovery.test.ts @@ -69,6 +69,30 @@ describe('autoplan reads installed host methodology', () => { expect(loaded).toBe(fs.readFileSync(generatedReview, 'utf8')); expect(loaded).not.toContain('STALE FOREIGN HARNESS SKILL'); if (host.name === 'codex') expect(loaded).toContain('"outside_provider":"claude-code"'); + const phase = review === 'plan-devex-review' ? 'dx' : review.split('-')[1]!; + const phaseBody = host.name === 'claude' + ? fs.readFileSync(path.join(generatedRoot, 'autoplan', 'sections', `${phase}-phase.md`), 'utf8') + : body; + const directive = phaseBody.split('\n').find(line => line.startsWith('Before dispatch, fully Read ') + && (line.includes(`/${review}/SKILL.md`) || line.includes(`/gstack-${review}/SKILL.md`))); + expect(directive).toBeDefined(); + expect(phaseBody.indexOf(directive!)).toBeLessThan(phaseBody.indexOf(`create ${phase} `)); + // U's CEO loaded this section only after its child finished. Pin the + // concrete prerequisite, then resolve the rendered path in real + // copy/symlink installations; references alone are not its contents. + if (host.name === 'claude') { + expect(directive).toContain('and `sections/review-sections.md` beside it'); + const relative = directive!.match(/and `([^`]+)` beside it/)![1]!; + const sectionSource = path.join(generatedRoot, review, relative); + const sectionInstalled = path.resolve(path.dirname(entry), reference, '..', relative); + installFile(sectionSource, sectionInstalled, mode); + const section = fs.readFileSync(sectionInstalled, 'utf8'); + expect(section).toBe(fs.readFileSync(sectionSource, 'utf8')); + expect(section).toContain('## Review Sections'); + } else { + expect(directive).not.toContain('sections/review-sections.md'); + expect(loaded).toContain('## Review Sections'); + } } } }); @@ -82,6 +106,7 @@ describe('autoplan reads installed host methodology', () => { expect(generateAutoplanReviewFile({ ...ctx, model: 'gpt' }, [review])).toBe(generateAutoplanReviewFile({ ...ctx, model: 'claude' }, [review])); } expect(() => generateAutoplanReviewFile(ctx, ['../foreign'])).toThrow(); + expect(() => generateAutoplanReviewFile(ctx, ['plan-ceo-review', '../foreign'])).toThrow(); } }); diff --git a/test/ceo-completion-handoff.test.ts b/test/ceo-completion-handoff.test.ts index 1fc68e334..d548fb103 100644 --- a/test/ceo-completion-handoff.test.ts +++ b/test/ceo-completion-handoff.test.ts @@ -10,6 +10,7 @@ import currentHandoffs from './fixtures/ceo-completion-handoff-j-calls.json'; import kHandoffs from './fixtures/ceo-completion-handoff-k-calls.json'; import rCalls from './fixtures/ceo-completion-handoff-r-calls.json'; import tHandoff from './fixtures/ceo-completion-handoff-t-call.json'; +import uHandoff from './fixtures/ceo-completion-handoff-u-call.json'; type CapturedCall = typeof captures.cases[number]['calls'][number]; function nativeCall(record: CapturedCall, sessionId = 'native-capture'): NativePlanQuestionCall { @@ -23,6 +24,108 @@ function nativeCall(record: CapturedCall, sessionId = 'native-capture'): NativeP const handoff = () => nativeCall(captures.cases[0]!.calls.at(-1)!); const fingerprint = (call: NativePlanQuestionCall) => nativePlanCallFingerprint(call, 0, false); +describe('U completed CEO metadata navigation with scoped review explanations', () => { + const captured = () => structuredClone(uHandoff.calls.at(-1)!) as NativePlanQuestionCall; + const pending = (call: NativePlanQuestionCall) => { + const copy = structuredClone(call); copy.answered = false; delete copy.answers; delete copy.unansweredQuestionIndices; + return fingerprint(copy); + }; + test('the actual six-call stream retains two issues and selects the offered manual stop', () => { + const calls = structuredClone(uHandoff.calls) as NativePlanQuestionCall[]; + expect(replay(calls, false, ceoFirstReviewAUQ)).toMatchObject({ step0Count: 3, reviewCount: 2, administrativeCount: 1 }); + expect(isCeoCompletionHandoff(fingerprint(captured()))).toBe(true); + expect(pickCeoCompletionHandoff(pending(captured())) ?? 1).toBe(2); + expect(calls).toEqual(uHandoff.calls); + }); + test('native identity, completed answer and real option order remain required', () => { + const call = captured(); call.questions[0]!.options.reverse(); + expect(pickCeoCompletionHandoff(pending(call))).toBe(1); + expect(pickCeoCompletionHandoff(fingerprint(call))).toBeNull(); + expect(pickCeoCompletionHandoff({ ...pending(call), signature: 'foreign:call' })).toBeNull(); + expect(pickCeoCompletionHandoff({ ...pending(call), nativeCall: undefined })).toBeNull(); + for (const mutate of [ + (c: NativePlanQuestionCall) => { c.failed = true; }, + (c: NativePlanQuestionCall) => { delete c.failed; }, + (c: NativePlanQuestionCall) => { c.answered = false; }, + (c: NativePlanQuestionCall) => { c.unansweredQuestionIndices = [0]; }, + (c: NativePlanQuestionCall) => { delete c.unansweredQuestionIndices; }, + (c: NativePlanQuestionCall) => { c.answers = { [c.questions[0]!.question]: 'Fix one more issue first' }; }, + ]) { const c = captured(); mutate(c); expect(isCeoCompletionHandoff(fingerprint(c))).toBe(false); } + }); + test('unfinished, conditional, quoted and additional-work descriptions remain substantive', () => { + for (const extra of [ + 'Delete the failing regression test before Eng.', 'Remove the owner check before Eng.', + 'Change the guarantee to permit old results.', 'Rewrite the acceptance criteria before shipping.', + 'Repair the missing authorization test.', 'We may repair the missing authorization test.', + 'All findings become resolved after the tests pass.', 'There is an outstanding authorization gap.', + 'Should we add another test before Eng?', 'Stakes if we pick wrong: delete the owner check.', + 'No UI scope was detected, so the CEO review is not complete.', + ]) { + const c = captured(); c.questions[0]!.options[1]!.description += ' ' + extra; + expect(isCeoCompletionHandoff(fingerprint(c))).toBe(false); + expect(pickCeoCompletionHandoff(pending(c))).toBeNull(); + } + for (const [from, to] of [ + ['The CEO review is done.', 'The CEO review is not done.'], + ['The CEO review is done.', 'The CEO review is done if tests pass.'], + ['Two assertion spec gaps were caught and resolved.', 'Not all assertion spec gaps were resolved.'], + ['Two assertion spec gaps were caught and resolved.', 'Two assertion spec gaps remain unresolved.'], + ['No UI scope was detected, so a design review is not needed.', 'The CEO review is not needed.'], + ['No UI scope was detected, so a design review is not needed.', 'No UI scope was detected, so a design review is not complete.'], + ['Stakes if we pick wrong:', 'The CEO review is complete only if we pick correctly:'], + ]) { + const c = captured(); const q = c.questions[0]!; const old = q.question; q.question = old.replace(from!, to!); + c.answers = { [q.question]: c.answers![old]! }; + expect(isCeoCompletionHandoff(fingerprint(c))).toBe(false); + expect(pickCeoCompletionHandoff(pending(c))).toBeNull(); + } + for (const prefix of ['> ', '```text\n', 'Example: ']) { + const c = captured(); c.questions[0]!.options[1]!.description = prefix + c.questions[0]!.options[1]!.description; + expect(isCeoCompletionHandoff(fingerprint(c))).toBe(false); + } + for (const mutate of [ + (c: NativePlanQuestionCall) => { c.questions[0]!.question = 'Should we fix the missing authorization check?'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.header = 'Authorization gap'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.question += ' '; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options[1]!.label = 'Repair authorization before Eng'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options.push({ label: 'Add a new TODO' }); }, + ]) { const c = captured(); mutate(c); expect(isCeoCompletionHandoff(fingerprint(c))).toBe(false); expect(pickCeoCompletionHandoff(pending(c))).toBeNull(); } + }); + test('metadata headings cannot shelter an extra obligation or conditional completion', () => { + for (const extra of ['Delete the owner check.', 'Remove the failing regression.', 'Change the guarantee.', + 'Rewrite the acceptance criteria.', 'All decisions are resolved after the tests pass.', + 'Should we approve one more issue?', 'The CEO review is not complete.']) { + const c = captured(); const q = c.questions[0]!; const old = q.question; + q.question += ' ' + extra; c.answers = { [q.question]: c.answers![old]! }; + expect(isCeoCompletionHandoff(fingerprint(c))).toBe(false); + expect(pickCeoCompletionHandoff(pending(c))).toBeNull(); + } + }); + test('the retained pending Exit and report still require fresh substantive decisions', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ceo-u-handoff-')); const file = path.join(dir, 'plan.md'); + try { + fs.writeFileSync(file, uHandoff.reportContent); + fs.utimesSync(file, uHandoff.reportAtMs / 1000, uHandoff.reportAtMs / 1000); + const calls = structuredClone(uHandoff.calls) as NativePlanQuestionCall[]; + const transcript = { status: 'ready' as const, calls, assistantMessages: [], planReadyRequests: [{ + sessionId: uHandoff.pendingExit.sessionId, toolUseId: uHandoff.pendingExit.toolUseId, + timestamp: uHandoff.pendingExit.timestamp, failed: false, source: 'pre_tool_use' as const, + }] }; + const admin = new Set(calls.filter(c => isCeoCompletionHandoff(fingerprint(c))).map(c => `${c.sessionId}:${c.toolUseId}`)); + expect(hasNativePlanTerminal(transcript, file, uHandoff.startedAtMs, 'plan_ready')).toBe(false); + expect(hasNativePlanTerminal(transcript, file, uHandoff.startedAtMs, 'plan_ready', admin)).toBe(true); + transcript.planReadyRequests[0]!.failed = true; + expect(hasNativePlanTerminal(transcript, file, uHandoff.startedAtMs, 'plan_ready', admin)).toBe(false); + transcript.planReadyRequests[0]!.failed = false; + transcript.planReadyRequests[0]!.sessionId = 'foreign-session'; + expect(hasNativePlanTerminal(transcript, file, uHandoff.startedAtMs, 'plan_ready', admin)).toBe(false); + transcript.planReadyRequests[0]!.sessionId = uHandoff.pendingExit.sessionId; + calls[3]!.answeredAt = new Date(uHandoff.reportAtMs + 1000).toISOString(); + expect(hasNativePlanTerminal(transcript, file, uHandoff.startedAtMs, 'plan_ready', admin)).toBe(false); + } finally { fs.rmSync(dir, { recursive: true, force: true }); } + }); +}); + describe('T completed CEO next-review navigation', () => { const captured = () => structuredClone(tHandoff.calls.at(-1)!) as NativePlanQuestionCall; const pending = (call: NativePlanQuestionCall) => { diff --git a/test/ceo-posture-packet.test.ts b/test/ceo-posture-packet.test.ts new file mode 100644 index 000000000..6b113c0a1 --- /dev/null +++ b/test/ceo-posture-packet.test.ts @@ -0,0 +1,165 @@ +import { describe, expect, test } from 'bun:test'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { pathToFileURL } from 'node:url'; +import { nextCeoPostureContinuation } from './helpers/ceo-mode-option'; +import type { PlanCountTranscript } from './helpers/plan-count-transcript'; + +const selectedAt = Date.parse('2026-09-09T07:57:52Z'); +const questions = ['Autosave', 'View tabs', 'Deep links', 'Team views'].map(header => ({ + header, question: `Add ${header.toLowerCase()} to the plan?`, options: [{label:'Add'}, {label:'Defer'}], +})); +function transcript(count = 4, native = false): PlanCountTranscript { + return {status:'ready', assistantMessages:[], calls:[{ + sessionId:'mode-session', toolUseId:'mode', answered:true, failed:false, + answeredAt:'2026-09-09T07:57:54Z', unansweredQuestionIndices:[], + questions:[{header:'Review mode',question:'Which mode?',options:[{label:'HOLD SCOPE'},{label:'SCOPE EXPANSION'}]}], + answers:{'Which mode?':'SCOPE EXPANSION'}, + }, ...(native ? [{sessionId:'mode-session',toolUseId:'downstream',answered:false,failed:false,questions:structuredClone(questions.slice(0,count))}] : [])]}; +} +function screen(next: number, count = 4) { + const bar = '← ' + questions.slice(0,count).map((q,i)=>(i, started: boolean) => + nextCeoPostureContinuation(visible,t,'SCOPE EXPANSION',selectedAt,seen,started); + +describe('one CEO posture continuation means one complete native call',()=>{ + test('two and four tabs complete once with eager or delayed native metadata',()=>{ + for(const count of [2,4]) for(const native of [false,true]) { + const t=transcript(count,native),seen=new Set(); + for(let i=0;i0)).toBe('question'); + expect(step(screen(i,count),t,seen,true)).toBeNull(); + } + expect(step(screen(count,count),t,seen,true)).toBe('submission'); + expect(step(screen(count,count),t,seen,true)).toBeNull(); + expect(step(screen(0,count),t,seen,true)).toBeNull(); + expect(step('☐ New issue\nFix another issue?\n❯1.Fix\n2.Keep',t,seen,true)).toBeNull(); + } + }); + test('late native metadata binds the existing packet without replaying a tab',()=>{ + const seen=new Set(); + expect(step(screen(0),transcript(),seen,false)).toBe('question'); + expect(step(screen(0),transcript(4,true),seen,true)).toBeNull(); + expect(step(screen(1),transcript(4,true),seen,true)).toBe('question'); + const foreign=transcript(4,true);foreign.calls[1]!.toolUseId='foreign'; + expect(step(screen(2),foreign,seen,true)).toBeNull(); + expect(step(screen(2),transcript(4,true),seen,true)).toBe('question'); + }); + test('changed bars, skipped answers, foreign calls and incomplete panels cannot extend the budget',()=>{ + const invalid = [screen(0),screen(2),screen(4),screen(1).replace('Deep links','Other work'), + screen(1).replace('Tab/Arrow keys to navigate','navigate'), + '☐ New issue\nFix another issue?\n❯1.Fix\n2.Keep']; + for(const visible of invalid) { + const seen=new Set();expect(step(screen(0),transcript(),seen,false)).toBe('question'); + expect(step(visible,transcript(),seen,true)).toBeNull(); + } + for(const mutate of [ + (t:PlanCountTranscript)=>{t.calls[1]!.sessionId='foreign';}, + (t:PlanCountTranscript)=>{t.calls[1]!.questions[1]!.question='Unrelated request?';}, + (t:PlanCountTranscript)=>{t.calls[0]!.failed=true;}, + (t:PlanCountTranscript)=>{t.calls[0]!.answers={'Which mode?':'HOLD SCOPE'};}, + ]) { + const seen=new Set();expect(step(screen(0),transcript(),seen,false)).toBe('question'); + const t=transcript(4,true);mutate(t); + expect(step(screen(1),t,seen,true)).toBeNull(); + } + expect(step(screen(1),transcript(),new Set(),false)).toBeNull(); + expect(step(screen(0),transcript(),new Set(),true)).toBeNull(); + }); + test('late metadata at Submit must match every tab and remain pending in the selected session',()=>{ + const mutations: Array<(t: PlanCountTranscript)=>void> = [ + t=>{t.calls[1]!.sessionId='foreign';}, + t=>{t.calls[1]!.questions[0]!.header='Foreign header';}, + t=>{t.calls[1]!.questions[0]!.question='Unrelated request?';}, + t=>{t.calls[1]!.answered=true;}, + t=>{t.calls[1]!.failed=true;}, + ]; + for(const mutate of mutations) { + const seen=new Set(); + for(let i=0;i<4;i++)expect(step(screen(i),transcript(),seen,i>0)).toBe('question'); + const t=transcript(4,true);mutate(t); + expect(step(screen(4),t,seen,true)).toBeNull(); + } + const seen=new Set(); + for(let i=0;i<4;i++)expect(step(screen(i),transcript(),seen,i>0)).toBe('question'); + expect(step(screen(4),transcript(4,true),seen,true)).toBe('submission'); + }); + test('single-question continuation remains one question even if the caller flag is stale',()=>{ + const t=transcript(),seen=new Set(); + const one='☐ Architecture\nGuard the lookup?\n❯1.Add guard\n2.Defer'; + expect(step(one,t,seen,false)).toBe('question'); + expect(step(one.replace('lookup','cache'),t,seen,false)).toBeNull(); + expect(step(screen(0),t,seen,false)).toBeNull(); + }); +}); + +test.skipIf(process.platform==='win32')('real fake CLI flushes native posture only after all four tabs and Submit',async()=>{ + const dir=fs.mkdtempSync(path.join(os.tmpdir(),'ceo-posture-packet-')); + const fake=path.join(dir,'fake-claude'),worker=path.join(dir,'worker.ts'),events=path.join(dir,'events.jsonl'),result=path.join(dir,'result.json'); + fs.writeFileSync(fake,`#!${process.execPath}\n`+String.raw` +import fs from 'node:fs';import path from 'node:path'; +const item=JSON.parse(process.env.POSTURE_PACKET);const sid='mode-session'; +const file=path.join(process.env.CLAUDE_CONFIG_DIR,'projects','fixture',sid+'.jsonl');fs.mkdirSync(path.dirname(file),{recursive:true}); +const native=(role,content,extra={})=>fs.appendFileSync(file,JSON.stringify({cwd:process.cwd(),sessionId:sid,isSidechain:false,timestamp:new Date().toISOString(),message:{role,content},...extra})+'\n'); +const log=e=>fs.appendFileSync(item.events,JSON.stringify(e)+'\n'); +log({type:'start',pid:process.pid}); +native('assistant',[{type:'tool_use',id:'mode',name:'AskUserQuestion',input:{questions:[{header:'Mode',question:'Which mode?',options:[{label:'HOLD SCOPE'},{label:'SCOPE EXPANSION'}]}]}}]); +native('user',[{type:'tool_result',tool_use_id:'mode',content:'Answered'}],{toolUseResult:{answers:{'Which mode?':'SCOPE EXPANSION'}}}); +let index=0,done=false; +const render=()=>process.stdout.write('\x1b[2J\x1b[H'+item.screens[index].replaceAll('\n','\r\n')); +process.stdin.setRawMode?.(true);process.stdin.on('data',data=>{ + const input=data.toString();log({type:'input',input,index}); + if(done)throw Error('input after completion'); + if(index<4){if(input!=='1')throw Error('native shortcut must not queue Enter');index++;render();return;} + if(input!=='\r')throw Error('expected Submit'); + native('assistant',[{type:'text',text:'I will explore expansion opportunities that improve saved project views.'},{type:'tool_use',id:'downstream',name:'AskUserQuestion',input:{questions:item.questions}}]); + native('user',[{type:'tool_result',tool_use_id:'downstream',content:'Answered'}],{toolUseResult:{answers:Object.fromEntries(item.questions.map(q=>[q.question,'Add']))}}); + done=true;process.stdout.write('\x1b[2J\x1b[HPOSTURE_FLUSHED\r\n'); +});process.on('SIGINT',()=>process.exit(0));process.stdin.resume();render(); +`);fs.chmodSync(fake,0o755); + const module=(name:string)=>pathToFileURL(path.join(import.meta.dir,'helpers',name)).href; + fs.writeFileSync(worker,` +import {launchClaudePty,capturePlanCountQuestion,planCountQuestionInput} from ${JSON.stringify(module('claude-pty-runner.ts'))}; +import {readPlanCountTranscript} from ${JSON.stringify(module('plan-count-transcript.ts'))}; +import {nextCeoPostureContinuation,hasNativePostAnswerCeoPosture} from ${JSON.stringify(module('ceo-mode-option.ts'))}; +const start=Date.now(),seen=new Set();let started=false; +const session=await launchClaudePty({cwd:${JSON.stringify(dir)},timeoutMs:7000,observeScreen:true,env:{POSTURE_PACKET:${JSON.stringify(JSON.stringify({events,questions,screens:Array.from({length:5},(_,i)=>screen(i))}))}}}); +try { + await session.waitFor('Autosave',{timeoutMs:2000,pollMs:20}); + const read=()=>readPlanCountTranscript(session.hermeticConfigDir,${JSON.stringify(dir)}); + const before=hasNativePostAnswerCeoPosture(read(),'SCOPE EXPANSION',/expansion/,start); + while(Date.now()-start<5000){ + const t=read();if(hasNativePostAnswerCeoPosture(t,'SCOPE EXPANSION',/expansion/,start))break; + const visible=await session.currentScreen(); + const action=nextCeoPostureContinuation(visible,t,'SCOPE EXPANSION',start,seen,started,session.visibleText()); + if(action==='question'){started=true;const pending=t.calls.find(c=>!c.answered&&!c.failed);const fp=capturePlanCountQuestion(visible,new Set(),0,false,pending);session.send(planCountQuestionInput(visible,fp,1));} + else if(action==='submission')session.send('\\r'); + await Bun.sleep(25); + } + const t=read();await Bun.write(${JSON.stringify(result)},JSON.stringify({before,after:hasNativePostAnswerCeoPosture(t,'SCOPE EXPANSION',/expansion/,start),calls:t.calls})); +} finally {await session.close();} +`); + const child=Bun.spawn([process.execPath,worker],{env:{...process.env,BROWSE_TERMINAL_BINARY:fake,EVALS_HERMETIC:'1'},stdout:'pipe',stderr:'pipe'}); + const timer=setTimeout(()=>child.kill('SIGKILL'),10000); + try{ + const [code,out,err]=await Promise.all([child.exited,new Response(child.stdout).text(),new Response(child.stderr).text()]); + expect(code,out+err).toBe(0);const r=JSON.parse(fs.readFileSync(result,'utf8')); + expect(r.before).toBe(false);expect(r.after).toBe(true); + expect(r.calls).toHaveLength(2);expect(r.calls[1].unansweredQuestionIndices).toEqual([]); + const rows=fs.readFileSync(events,'utf8').trim().split('\n').map(s=>JSON.parse(s)); + expect(rows.filter(r=>r.type==='input').map(r=>r.input)).toEqual(['1','1','1','1','\r']); + expect(()=>process.kill(rows[0].pid,0)).toThrow(); + } finally { + clearTimeout(timer);child.kill('SIGKILL');await child.exited; + if(fs.existsSync(events))try{ + const pid=JSON.parse(fs.readFileSync(events,'utf8').split('\n')[0]!).pid; + const argv=process.platform==='linux'?fs.readFileSync('/proc/'+pid+'/cmdline','utf8').split('\0'):Bun.spawnSync(['ps','-p',String(pid),'-o','command='], { timeout: 1_000 }).stdout.toString().trim().split(/\s+/); + if(argv.includes(fake))process.kill(pid,'SIGKILL'); + }catch{} + fs.rmSync(dir,{recursive:true,force:true}); + } +},12000); diff --git a/test/ceo-section-loading-fixture.test.ts b/test/ceo-section-loading-fixture.test.ts index b4ac3bad9..3b4fba4be 100644 --- a/test/ceo-section-loading-fixture.test.ts +++ b/test/ceo-section-loading-fixture.test.ts @@ -5,6 +5,43 @@ import { hasStaleFillRaceFinding, } from './helpers/ceo-section-loading-fixture'; +describe('pre-write snapshot vocabulary in the actual U finding', () => { + const report = require('node:fs').readFileSync(require('node:path').join(import.meta.dir, 'fixtures/ceo-section-u-report.md'), 'utf8'); + const paragraph = report.slice(report.indexOf('After T4 the cache correctly reflects'), report.indexOf('**Recommended fix (auto-decided):**')).trim(); + + test('recognizes the exact delivered report and its complete asserted paragraph independently of the bad remedy', () => { + expect(paragraph).toContain('re-populates the cache with the pre-write snapshot'); + expect(paragraph).toContain('This violates the invariant:'); + expect(hasStaleFillRaceFinding(paragraph)).toBe(true); + expect(hasStaleFillRaceFinding(report)).toBe(true); + }); + + test.each(['pre-write snapshot', 'pre write snapshot', 'pre-write value', 'pre-write data', 'pre-write version'])('recognizes an old snapshot synonym: %s', value => { + expect(hasStaleFillRaceFinding(`An in-flight read re-populates the cache with the ${value} after write invalidation. A new reader sees it, violating the contract.`)).toBe(true); + }); + + test.each([ + 'A pending read returns the pre-write snapshot to its original caller; that return is permitted.', + 'An in-flight read re-populates the cache with the post-write snapshot after invalidation.', + 'The pre-write snapshot expires after 30 seconds. The LRU byte cap is adequate.', + 'If invalidation throws after a write, the cache retains the pre-write snapshot. Log the failure and bypass the cache.', + 'An in-flight read re-populates the cache with the pre-write snapshot after invalidation. This is allowed behavior for subsequent reads.', + 'An in-flight read re-populates the cache with the pre-write snapshot after invalidation. This is the accepted consistency model.', + 'An in-flight read re-populates the cache with the pre-write snapshot after invalidation, so a new reader receives that version. This is the accepted consistency model.', + 'An in-flight read re-populates the cache with the pre-write snapshot after invalidation. It is not a bug; no guard is required.', + 'An in-flight read cannot re-populate the cache with the pre-write snapshot after invalidation. No race remains.', + '> An in-flight read re-populates the pre-write snapshot after write invalidation; a new read sees it, violating the contract.', + '```text\nAn in-flight read re-populates the pre-write snapshot after write invalidation; a new read sees it, violating the contract.\n```', + '* An in-flight read re-populates the pre-write snapshot after invalidation.\n* Telemetry retry handling has a bug.', + ])('retains original-caller, freshness, dismissal and source boundaries: %s', value => { + expect(hasStaleFillRaceFinding(value)).toBe(false); + }); + + test('permission for the original caller still cannot excuse a later-reader violation', () => { + expect(hasStaleFillRaceFinding('The original pending caller may receive the pre-write snapshot; that return is permitted. However, an in-flight read refills the cache with the pre-write snapshot after write invalidation, so a new reader violates the contract. Guard cache fills with a generation token.')).toBe(true); + }); +}); + describe('CEO section-loading cache fixture', () => { test('the exact proposed wrapper retains a reproducible stale-fill race', async () => { let releaseRead!: (value: string) => void; diff --git a/test/design-completion-handoff-u.test.ts b/test/design-completion-handoff-u.test.ts new file mode 100644 index 000000000..90d0732a9 --- /dev/null +++ b/test/design-completion-handoff-u.test.ts @@ -0,0 +1,121 @@ +import { describe, expect, test } from 'bun:test'; +import { designStep0Boundary, nativePlanCallFingerprint, planCountQuestionPhase } from './helpers/claude-pty-runner'; +import { isDesignCompletionHandoff, isDesignCountFirstReview, pickDesignCountQuestion } from './helpers/design-count-review'; +import type { NativePlanQuestionCall } from './helpers/plan-count-transcript'; +import actual from './fixtures/design-handoff-u-calls.json'; + +const calls = () => structuredClone(actual) as NativePlanQuestionCall[]; +const handoff = () => calls().at(-1)!; +const fp = (call: NativePlanQuestionCall) => nativePlanCallFingerprint(call, 0, false); +function answer(call: NativePlanQuestionCall) { + call.answers = { [call.questions[0]!.question]: call.questions[0]!.options[0]!.label }; + return call; +} +function pending(call: NativePlanQuestionCall) { + call.answered = false; + delete call.answers; + delete call.unansweredQuestionIndices; + return call; +} + +describe('Design completed recap before its required review handoff', () => { + test('the exact U calls preserve seven issues and classify only the eighth navigation call separately', () => { + const input = calls(); + const before = structuredClone(input); + let reviewStarted = false; + const counts = { setup: 0, review: 0, administrative: 0 }; + for (const call of input) { + const phase = planCountQuestionPhase(fp(call), reviewStarted, designStep0Boundary, + isDesignCountFirstReview, undefined, isDesignCompletionHandoff); + reviewStarted = phase.reviewStarted; + if (phase.administrative) counts.administrative++; + else if (phase.preReview) counts.setup++; + else counts.review++; + } + expect(counts).toEqual({ setup: 0, review: 7, administrative: 1 }); + expect(input.slice(0, 7).every(call => !isDesignCompletionHandoff(fp(call)))).toBe(true); + expect(input).toEqual(before); + }); + + test('the pending exact menu chooses its actual manual option in either order', () => { + for (const reverse of [false, true]) { + const call = pending(handoff()); + if (reverse) call.questions[0]!.options.reverse(); + expect(pickDesignCountQuestion(fp(call), fp(call))).toBe(reverse ? 1 : 2); + expect(isDesignCompletionHandoff(fp(call))).toBe(false); + expect(pickDesignCountQuestion(fp(call), { ...fp(call), signature: 'foreign:request' })).toBeNull(); + } + }); + + test('completed recap facts vary without changing the native closed-review decision', () => { + for (const recap of [ + '7 decisions resolved, 6 implementation tasks added, 0 deferred.', + 'All findings resolved. 6 tasks recorded. No deferred issues.', + 'The design review recorded accessibility and form-layout requirements. 7 issues addressed.', + 'This review has approved responsive layout constraints. Zero unresolved decisions.', + ]) { + const call = handoff(); + call.questions[0]!.question = `Design review complete (6/10 → 9/10). ${recap} Engineering Review is the required shipping gate. What next? `; + expect(isDesignCompletionHandoff(fp(answer(call)))).toBe(true); + const active = fp(pending(call)); + expect(pickDesignCountQuestion(active, active)).toBe(2); + } + }); + + test('a closed prefix never hides unfinished work, another decision, source claims or new instructions', () => { + const invalid = [ + 'One contrast gap remains.', '7 decisions unresolved.', '1 deferred issue.', + 'The review is not complete.', 'The review will be complete after contrast is fixed.', + 'The plan claims that all issues are resolved.', 'The design review added a task; configure the missing states.', + 'The design review added a task. Configure the missing states.', + 'The design review added a task and then delete the validation.', + 'The design review added a task — remove the accessibility check.', + 'The design review added a task. Should we fix its contrast?', + 'The design review added a task if the user approves it.', + 'The design review added a task but the contrast is still missing.', + ]; + for (const text of invalid) { + const call = handoff(); + call.questions[0]!.question = `Design review complete. ${text} Eng Review is the required shipping gate. What next? `; + expect(isDesignCompletionHandoff(fp(answer(call)))).toBe(false); + const active = fp(pending(call)); + expect(pickDesignCountQuestion(active, active)).toBeNull(); + } + }); + + test('offered action descriptions cannot smuggle new work or conditional closure', () => { + for (const extra of [ + ' Configure a new layout.', ' Remove the missing test.', ' Pick the unresolved color.', + ' Then implement the spinner.', ' The review is incomplete.', + ' Once contrast is fixed, all decisions are resolved.', + ' Please fix the contrast before proceeding.', + ]) { + const call = handoff(); + call.questions[0]!.options[1]!.description += extra; + expect(isDesignCompletionHandoff(fp(call))).toBe(false); + const active = fp(pending(call)); + expect(pickDesignCountQuestion(active, active)).toBeNull(); + } + }); + + test('native identity, complete offered answers and the exact binary menu remain required', () => { + const mutations: Array<(call: NativePlanQuestionCall) => void> = [ + c => { c.failed = true; }, c => { c.answered = false; }, + c => { c.unansweredQuestionIndices = [0]; }, c => { delete c.unansweredQuestionIndices; }, + c => { c.answers = {}; }, c => { c.answers = { [c.questions[0]!.question]: 'repair another issue' }; }, + c => { c.questions[0]!.header = 'Contrast'; }, c => { c.questions[0]!.multiSelect = true; }, + c => { c.questions[0]!.question = c.questions[0]!.question.replace('plan-design-review-next-step', 'plan-ceo-review-next-step'); }, + c => { c.questions[0]!.question += ' '; }, + c => { c.questions[0]!.options.push({ label: 'Run /plan-ceo-review first' }); }, + c => { c.questions[0]!.options.push(structuredClone(c.questions[0]!.options[1]!)); }, + c => { c.questions[0]!.options[1]!.label += ' and fix contrast'; }, + c => { c.questions[0]!.question = c.questions[0]!.question.replace('Eng review is the required shipping gate.', 'Eng review is optional.'); }, + ]; + for (const mutate of mutations) { + const call = handoff(); mutate(call); + expect(isDesignCompletionHandoff(fp(call))).toBe(false); + } + expect(isDesignCompletionHandoff({ ...fp(handoff()), nativeCall: undefined })).toBe(false); + expect(isDesignCompletionHandoff({ ...fp(handoff()), signature: 'foreign:request' })).toBe(false); + }); +}); diff --git a/test/devex-count-fixture.test.ts b/test/devex-count-fixture.test.ts index 8185678b5..43338a011 100644 --- a/test/devex-count-fixture.test.ts +++ b/test/devex-count-fixture.test.ts @@ -1,3 +1,7 @@ +import capturedURetry from './fixtures/devex-count-u-retry-calls.json'; +import capturedU from './fixtures/devex-count-u-calls.json'; + + import { describe, expect, test } from 'bun:test'; import type { AskUserQuestionFingerprint } from './helpers/claude-pty-runner'; import capturedL from './fixtures/devex-review-l-calls.json'; @@ -372,3 +376,122 @@ describe('T native documentation follow-up decisions', () => { } }); }); + +describe('U completed first-pass contract decisions', () => { + const calls = () => structuredClone(capturedU.calls) as NativePlanQuestionCall[]; + const fp = (call: NativePlanQuestionCall) => nativePlanCallFingerprint(call, 0, true); + const change = (call: NativePlanQuestionCall, transform: (s: string) => string) => { + const q = call.questions[0]!; const answer = call.answers![q.question]!; + q.question = transform(q.question); call.answers = {[q.question]: answer}; return call; + }; + test('all five actual seed decisions count once, without mutating evidence', () => { + const actual = calls(); const before = structuredClone(actual); + expect(actual.map(call => isDevexReviewIssue(fp(call)))).toEqual([true, true, true, true, true]); + expect(actual).toEqual(before); + }); + for (const index of [0, 1]) { + test(`decision ${index + 1} requires complete native identity and an offered answer`, () => { + expect(isDevexReviewIssue(fp(calls()[index]!))).toBe(true); + for (const mutate of [ + (c: NativePlanQuestionCall) => { c.answered = false; }, + (c: NativePlanQuestionCall) => { c.failed = true; }, + (c: NativePlanQuestionCall) => { delete c.failed; }, + (c: NativePlanQuestionCall) => { c.unansweredQuestionIndices = [0]; }, + (c: NativePlanQuestionCall) => { delete c.unansweredQuestionIndices; }, + (c: NativePlanQuestionCall) => { c.answers = {}; }, + (c: NativePlanQuestionCall) => { c.answers = {[c.questions[0]!.question]: 'Foreign answer'}; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.multiSelect = true; }, + (c: NativePlanQuestionCall) => { c.questions.push(structuredClone(c.questions[0]!)); }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options.push(structuredClone(c.questions[0]!.options[0]!)); }, + ]) { const c = calls()[index]!; mutate(c); expect(isDevexReviewIssue(fp(c))).toBe(false); } + const foreign = fp(calls()[index]!); foreign.signature = 'foreign:tool'; expect(isDevexReviewIssue(foreign)).toBe(false); + const ui = fp(calls()[index]!); delete ui.nativeCall; expect(isDevexReviewIssue(ui)).toBe(false); + }); + test(`decision ${index + 1} cannot borrow issue words for setup or quoted examples`, () => { + for (const transform of [ + (s: string) => '> ' + s, + (s: string) => '```text\n' + s + '\n```', + (s: string) => s.replace(/Pass 1 \(Getting Started\):/, 'Pass 1 (Getting Started): It is false that'), + (s: string) => s.replace(/]+>/, ''), + (s: string) => s + ' ', + ]) expect(isDevexReviewIssue(fp(change(calls()[index]!, transform)))).toBe(false); + const c = calls()[index]!; c.questions[0]!.header = 'Review mode'; expect(isDevexReviewIssue(fp(c))).toBe(false); + }); + test(`decision ${index + 1} keeps a distinct accepted or deferred decision independent of option order`, () => { + const c = calls()[index]!; const q = c.questions[0]!; + q.options.reverse(); expect(isDevexReviewIssue(fp(c))).toBe(true); + c.answers = {[q.question]: q.options[0]!.label}; expect(isDevexReviewIssue(fp(c))).toBe(true); + }); + } + test('resolved or negated first-run contracts and pure navigation do not count', () => { + for (const transform of [ + (s: string) => s.replace("doesn't ship", 'already ships'), + (s: string) => s.replace('quickstart points to', 'quickstart no longer points to'), + (s: string) => s.replace('Should we fix the quickstart path in the plan?', 'Should we begin the review?'), + (s: string) => s.replace('Should we fix', 'Should we not fix'), + ]) expect(isDevexReviewIssue(fp(change(calls()[0]!, transform)))).toBe(false); + for (const transform of [ + (s: string) => s.replace('makes that unreachable', 'makes that reachable'), + (s: string) => s.replace('makes that unreachable', 'does not make that unreachable'), + (s: string) => s.replace('The plan retains the gate.', 'The plan already skips the gate.'), + (s: string) => s.replace('How should this plan handle the contradiction?', 'Should we begin the review?'), + ]) expect(isDevexReviewIssue(fp(change(calls()[1]!, transform)))).toBe(false); + }); +}); + + +describe('U demo timing decision after completed measurements', () => { + const captured = () => structuredClone(capturedURetry[0]!) as NativePlanQuestionCall; + const fp = (c: NativePlanQuestionCall) => nativePlanCallFingerprint(c, 0, true); + function replace(c: NativePlanQuestionCall, from: string, to: string) { + const q = c.questions[0]!; const old = q.question; q.question = old.replace(from, to); + if (c.answers) c.answers = { [q.question]: c.answers[old]! }; + return c; + } + test('all five actual completed calls are independent issue decisions', () => { + const calls = structuredClone(capturedURetry) as NativePlanQuestionCall[]; + expect(calls.map(c => isDevexReviewIssue(fp(c)))).toEqual([true, true, true, true, true]); + expect(calls).toEqual(capturedURetry); + const c = captured(); c.questions[0]!.options.reverse(); + expect(isDevexReviewIssue(fp(c))).toBe(true); + c.answers = { [c.questions[0]!.question]: c.questions[0]!.options[0]!.label }; + expect(isDevexReviewIssue(fp(c))).toBe(true); // Deferring the repair is still this decision. + }); + test('timings are compared instead of pinning the observed minutes', () => { + let c = captured(); + for (const [from,to] of [['<2 min','<3 min'],['under 2 minutes','under 3 minutes'],['blocks for 5 minutes','blocks for 4 minutes'],['measured TTHW of 6 minutes','measured TTHW of 5 minutes']]) c=replace(c,from!,to!); + expect(isDevexReviewIssue(fp(c))).toBe(true); + for (const [from,to] of [['blocks for 5 minutes','blocks for 1 minutes'],['measured TTHW of 6 minutes','measured TTHW of 4 minutes'],['under 2 minutes','under 9 minutes']]) + expect(isDevexReviewIssue(fp(replace(captured(),from!,to!)))).toBe(false); + }); + test('setup, negated, quoted and merely hypothetical timing claims remain outside the new arm', () => { + for (const [from,to] of [ + ['should it bypass the mandatory CI check to reach the <2 min TTHW target?', 'which TTHW target should we confirm?'], + ['ELI10: The agreed onboarding target is under 2 minutes', 'Example: ELI10: The agreed onboarding target is under 2 minutes'], + ['ELI10: The agreed onboarding target is under 2 minutes', '> ELI10: The agreed onboarding target is under 2 minutes'], + ['ELI10: The agreed onboarding target is under 2 minutes', '```text\nELI10: The agreed onboarding target is under 2 minutes'], + ['Today `python -m evalkit.demo` blocks', 'Today `python -m evalkit.demo` no longer blocks'], + ['Today `python -m evalkit.demo` blocks', 'It is false that `python -m evalkit.demo` blocks'], + ['Today `python -m evalkit.demo` blocks', 'If `python -m evalkit.demo` blocks'], + ['giving a measured TTHW of 6 minutes', 'giving a measured TTHW of 6 minutes only if the optional slow simulation is enabled'], + ['giving a measured TTHW of 6 minutes', 'giving a measured TTHW of 6 minutes only in a hypothetical example'], + ['devex-demo-ci-bypass', 'plan-devex-review-tthw-tier'], + ]) expect(isDevexReviewIssue(fp(replace(captured(),from!,to!)))).toBe(false); + const c = captured(); c.questions[0]!.header = 'TTHW target'; expect(isDevexReviewIssue(fp(c))).toBe(false); + }); + test('the new measured branch requires one complete matched native decision', () => { + for (const mutate of [ + (c: NativePlanQuestionCall) => { c.failed = true; }, + (c: NativePlanQuestionCall) => { delete c.failed; }, + (c: NativePlanQuestionCall) => { c.answered = false; }, + (c: NativePlanQuestionCall) => { delete c.unansweredQuestionIndices; }, + (c: NativePlanQuestionCall) => { c.unansweredQuestionIndices = [0]; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.multiSelect = true; }, + (c: NativePlanQuestionCall) => { c.questions.push(structuredClone(c.questions[0]!)); }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options.push(structuredClone(c.questions[0]!.options[0]!)); }, + (c: NativePlanQuestionCall) => { c.answers = { [c.questions[0]!.question]: 'unoffered answer' }; }, + ]) { const c=captured(); mutate(c); expect(isDevexReviewIssue(fp(c))).toBe(false); } + expect(isDevexReviewIssue({...fp(captured()), signature:'foreign:call'})).toBe(false); + expect(isDevexReviewIssue({...fp(captured()), nativeCall:undefined})).toBe(false); + }); +}); diff --git a/test/fixtures/autoplan/u-ceo-original-loss.json b/test/fixtures/autoplan/u-ceo-original-loss.json new file mode 100644 index 000000000..d67b9bc3a --- /dev/null +++ b/test/fixtures/autoplan/u-ceo-original-loss.json @@ -0,0 +1,6 @@ +{ + "sourceRun": "ship-source-u-full-paid-20260909-0740", + "initialImplementation": "# Plan: User Dashboard Page\n\n## Context\nWe're shipping a new user dashboard at `/dashboard` showing recent activity,\nnotifications panel, and quick-action buttons. Users land here after login.\n\n## UI Scope\n- New React page component `UserDashboard.tsx` at `src/pages/`\n- Three new sub-components: `ActivityFeed`, `NotificationsPanel`, `QuickActions`\n- Tailwind CSS for layout, mobile-first responsive (breakpoints: sm/md/lg)\n- Empty state, loading skeleton, error state for each panel\n- Hover states + focus-visible outlines on every interactive element\n- Modal dialog for \"Mark all as read\" on notifications panel\n- Toast notification system for action feedback\n\n## Backend\n- New REST endpoint `GET /api/dashboard` returns `{ activity, notifications, quickActions }`\n- Backed by existing PostgreSQL tables; no schema changes\n\n## Out of scope\n- Dark mode (separate plan)\n- Personalization / customization (separate plan)\n\n## Existing product and application contracts\n\nThis is the existing single-role member workspace, not a new product or a new\nonboarding flow. Members currently visit three separate pages after login to\nresume work, check alerts, and inspect recent changes. In the team's last task\nwalkthrough, finding the next item took a median 75 seconds. The dashboard's\nsuccess measure is login-to-first-completed-task time, targeting 45 seconds,\nwith completed-task rate and permission-error rate as guardrails. Existing\nanalytics records login, action start, action completion, and permission errors;\nthe new page still needs its own exposure and interaction instrumentation.\n\nActivity is the immutable audit history of workspace changes. Notifications are\nmember-specific alerts with persistent read state; acknowledging an alert does\nnot alter audit history. The existing action registry supplies three actions\n(create an item, resume assigned work, invite a member), with stable IDs, labels,\nroute targets, and server-side eligibility predicates. These are links into\nexisting workflows; action ranking and a new configuration service do not exist.\n\nThe application already uses cookie sessions and workspace membership middleware.\nIts request context supplies the authenticated member and workspace IDs. Existing\nrepository methods apply both IDs where appropriate; callers do not accept a\nworkspace ID from query parameters. Mutations already require CSRF tokens. The\nnew dashboard endpoint must compose these methods and follow the same boundaries;\nits handler, authorization integration, and failure paths have not been written.\n\nExisting list methods return the latest 20 records plus a cursor and have indexed\nworkspace/member and created-at access paths. The existing full activity and\nnotification pages own older-page navigation. The member-scoped bulk-read API is\nidempotent and marks only notifications at or before the supplied snapshot time,\nso later arrivals remain unread. Existing HTTP clients expose typed unauthenticated,\nforbidden, validation, retryable-service, and network errors. Each dashboard panel\nstill needs to map these results to its loading, empty, error, retry, and success\nstates; the aggregate endpoint's response composition and partial-failure behavior\nremain new implementation work. No schema migration or new mutation API is needed.\n\nThe app already has Tailwind spacing/color/type tokens, a responsive page shell,\nbuttons, links, and a dialog primitive with focus trapping, Escape dismissal, and\nfocus return. These primitives do not implement any dashboard panel, confirmation\nflow, or toast system. The new modal and toast feedback must also work with keyboard\nand screen readers; existing accessibility policy requires named controls, a live\nregion for nonblocking feedback, sufficient contrast, and reduced-motion support.\nThe dashboard still needs its own layout, content hierarchy, mobile behavior, and\nstate-specific copy at sm/md/lg breakpoints.\n\nVitest, React Testing Library, and Playwright already run in CI. Existing fixtures\ncover authenticated members, another workspace, empty lists, and service failures;\nthere are no dashboard-specific tests yet. Existing staging feature flags and\nrequest/error metrics support a member-cohort rollout and rollback to the current\nlanding page. The dashboard's rollout criteria, endpoint performance checks,\ninteraction tests, and accessibility verification must be specified and added.\n\nAll dashboard screen, panel, aggregate-endpoint, modal, and toast work listed above\nis new. The existing contracts describe dependencies to reuse, not completed work\nor prior approval of an implementation approach.\n", + "activeAfterAmend": "# Plan: User Dashboard Page\n\n## Implementation plan\n\n### Original plan\n\n**Context:** Shipping a new user dashboard at `/dashboard` — activity feed, notifications panel, quick-action buttons. Users land here after login. Success metric: login-to-first-completed-task median from 75 seconds to 45 seconds.\n\n**UI Scope:**\n- `UserDashboard.tsx` at `src/pages/`\n- Sub-components: `ActivityFeed`, `NotificationsPanel`, `QuickActions`\n- Tailwind CSS, mobile-first (sm/md/lg breakpoints)\n- Loading skeleton, empty state, error state per panel\n- Hover + focus-visible on interactive elements\n- Modal: \"Mark all as read\" on notifications panel\n- Toast notification system for action feedback\n\n**Backend:**\n- `GET /api/dashboard` → `{ activity, notifications, quickActions }`\n- Existing PostgreSQL tables; no schema changes\n\n**Out of scope:** dark mode, personalization.\n\n### Amendments from CEO review (A1-A14)\n\n**A1 — Partial-failure contract (REQUIRED before implementation):**\nResponse schema:\n```\n{\n activity: ActivityData | null,\n notifications: NotificationData | null,\n quickActions: QuickActionData | null,\n panelErrors: {\n activity?: { code: string, message: string },\n notifications?: { code: string, message: string },\n quickActions?: { code: string, message: string }\n }\n}\n```\nEach panel renders based on its own data/error field independently.\n\n**A2 — Parallel backend queries:** Handler uses Promise.all (or equivalent) for all three data sources. Serial execution prohibited.\n\n**A3 — Rollout criteria (pre-specified, required before feature flag enables):**\n1. p95 GET /api/dashboard latency < 300ms (staging verification)\n2. Zero WCAG 2.1 AA violations (keyboard + screen reader paths)\n3. Permission-error rate at or below current baseline\n4. Login-to-first-completed-task median < 50s in first cohort (abort if > 65s)\n\n**A4 — Double-click guard:** \"Mark all as read\" confirm button disables immediately on click; re-enables on API response.\n\n**A5 — Toast system:**\n- Use existing installed toast library if present; otherwise install `sonner` (not hand-rolled)\n- Toast container at AppShell level (not inside UserDashboard) — `role=\"status\"` / `aria-live=\"polite\"`\n- Max 3 visible toasts; FIFO replacement; 4-second auto-dismiss\n- Suppress animations via `@media (prefers-reduced-motion)`\n\n**A6 — PanelWrapper:** Shared component handling loading skeleton, empty state, error state + retry button, success slot. All three panels use it.\n\n**A7 — QuickActions empty state:** When all 3 actions ineligible, render defined empty state (e.g., \"No actions available\").\n\n**A8 — Panel order:** QuickActions → NotificationsPanel → ActivityFeed (mobile and lg breakpoint).\n\n**A9 — Content rendering:** All user-generated content in ActivityFeed and NotificationsPanel rendered as plaintext (React's default `{value}`). No `dangerouslySetInnerHTML`.\n\n**A10 — QuickActions service layer:** Data fetching via `fetchQuickActions()` service function, not direct action registry import. Enables future ranking service substitution.\n\n**A11 — Analytics events (required):**\n- `dashboard_viewed` on page mount\n- `panel_loaded {panel, duration_ms, status}` per panel\n- `quick_action_clicked {action_id}`\n- `notification_marked_read {count}` on bulk-read success\n- `panel_error {panel, error_code}`\n\n**A12 — Alert:** Dashboard endpoint p95 > 500ms for 5 minutes → existing alert channel.\n\n**A13 — Runbook:** 1-page operational runbook: what to check when endpoint is slow/erroring, how to disable feature flag, escalation path.\n\n**A14 — Required tests:**\n- Playwright: login → all panels render (happy path)\n- Playwright: one panel API fails → other two panels still render\n- RTL: \"Mark all as read\" modal — confirm / cancel / error states\n- RTL: double-click guard on confirm button\n- RTL: all panel states (loading / empty / error)\n- Playwright: keyboard navigation through all panels (Tab, Enter, Escape)\n- Playwright: reduced-motion mode (no skeleton animations)\n- Vitest: GET /api/dashboard partial failure response structure\n- Playwright staging: p95 latency verification check\n\n**Deferred to TODOS.md:**\n- State-aware redirect for members with in-progress work (P2)\n- Notification badge in page `` (P3)\n- Keyboard shortcut for bulk read (P3)\n- Action ranking service integration (P2)\n- Validate action registry coverage vs. post-login analytics frequency (P1 — do before implementation if analytics accessible)\n\n---\n\n\n<!-- autoplan-accepted:ceo -->\n- A1: Partial-failure contract for GET /api/dashboard — per-panel status fields in response; each panel renders independently\n- A2: Parallel backend query execution required (Promise.all or equivalent)\n- A3: Rollout criteria pre-specified: p95 < 300ms staging, WCAG 2.1 AA clean, error rate baseline, median < 50s first cohort\n- A4: Double-click guard on Mark all as read confirm button\n- A5: Toast system: existing library or sonner; AppShell-level; max 3; FIFO; 4s dismiss; reduced-motion; ARIA live region\n- A6: PanelWrapper shared component for panel states\n- A7: QuickActions empty state when all actions ineligible\n- A8: Panel order: QuickActions → NotificationsPanel → ActivityFeed\n- A9: Plaintext rendering for user-generated content (no dangerouslySetInnerHTML)\n- A10: QuickActions data via service layer for future ranking decoupling\n- A11: 5 analytics events specified\n- A12: Dashboard p95 alert (> 500ms / 5min → existing alert channel)\n- A13: 1-page operational runbook for dashboard endpoint failures\n- A14: 9 required test specifications (Playwright + RTL + Vitest)\n<!-- /autoplan-accepted:ceo -->\n## Review record\n\n### CEO Review (Phase 1) — SELECTIVE EXPANSION\n\n**Mode:** SELECTIVE EXPANSION (auto-selected by /autoplan)\n**Dual voices:** Claude subagent completed; Codex disabled (codex_reviews=disabled)\n\n#### Pre-Review System Audit\nFixture repo — single plan file, one commit, no application code. Plan evaluated on specification quality only. No design doc, TODOS.md, or CLAUDE.md.\n\n#### 0A: Premise Challenge\n\n| Premise | Status |\n|---|---|\n| Navigation fragmentation causes 75s median | ASSUMED — root cause (load time vs. cognitive load) not validated |\n| Three quick actions cover top post-login behaviors | ASSUMED — action registry designed for eligibility, not frequency |\n| No schema changes needed | VALID |\n| Existing dialog primitive handles modal | VALID |\n| Feature flag supports rollout | VALID |\n| Partial-failure behavior can be decided during implementation | PROBLEMATIC — now required upfront (A1) |\n| Rollout criteria can be specified later | WRONG — queued as User Challenge for Final Gate |\n\n**User Challenge (queued for Final Gate):** Both CEO subagent and primary review flagged that root cause of 75-second median is unvalidated. If bottleneck is task ambiguity (not page count), dashboard may not move the needle. Does not block implementation; surfaced for user judgment at Final Gate.\n\n#### 0B: Existing Code Leverage Map\n\n| Sub-problem | Existing code |\n|---|---|\n| Auth / session | Cookie sessions + workspace membership middleware |\n| Activity data | Existing activityRepo.list() (20 records + cursor, indexed) |\n| Notification data | Existing notificationRepo.list() (20 records + cursor, indexed) |\n| Quick actions | Existing action registry (3 actions, eligibility predicates) |\n| HTTP error types | Typed: unauthenticated, forbidden, validation, retryable, network |\n| UI primitives | Tailwind tokens, responsive page shell, buttons, links, dialog |\n| Bulk-read | Existing idempotent mark-read API (snapshot-time scoped) |\n| Tests | Vitest + RTL + Playwright + existing fixtures |\n| Rollout | Existing feature flag + request/error metrics |\n\nNew (no existing equivalent): dashboard layout, aggregate endpoint, PanelWrapper, Toast system, dashboard analytics, rollout criteria.\n\n#### 0C: Dream State\n\n```\nCURRENT STATE THIS PLAN 12-MONTH IDEAL\n3 pages after login --> /dashboard (all 3 --> State-aware redirect\n(activity, notifs, panels). Static + personalized actions\nwork). 75s median. quick actions. + < 30s median\n 45s target.\n```\n\n#### 0C-bis: Implementation Alternatives\n\n```\nAPPROACH A: Aggregate Endpoint (the plan)\n Effort: M | Risk: Med | Single request; one loading state\n Con: Single point of failure; partial-failure contract needed (A1 fixes)\n\nAPPROACH B: Per-Panel Independent Fetch\n Effort: M | Risk: Low | Better fault isolation; natural progressive load\n Con: 3 requests vs 1; waterfall risk if not parallelized\n \nRECOMMENDATION: Both valid. Approach A retained (plan spec). Partial-failure\ncontract now required by A1. Logging as Taste Decision.\n```\n\n#### 0D: SELECTIVE EXPANSION Analysis\n\nComplexity: 8-10 files — at threshold but justified.\n\nState-aware redirect (10x reframe from subagent): ~1d human / ~30min CC. DEFERRED — outside blast radius, requires new routing logic; validate after first cohort data.\n\nDelight opportunities deferred: badge in `<title>`, optimistic bulk-read, keyboard shortcuts, \"last updated\" timestamps, skeleton-to-content transitions. All to TODOS.md.\n\n#### 0E: Temporal Interrogation\n\n```\nHOUR 1: Partial-failure schema + Toast placement (AppShell) — decide before any code\nHOUR 2-3: Parallel queries; Mark-all-read optimistic vs. confirmed; query parallelism\nHOUR 4-5: CSRF token for bulk-read; toast copy strings needed\nHOUR 6+: Rollout criteria verification; Playwright reduced-motion test\n```\n\n#### Section 1: Architecture\n\n```\n Browser\n │ GET /dashboard (feature-flagged)\n ▼\n UserDashboard.tsx\n ├── QuickActions.tsx ──────────────────┐\n ├── NotificationsPanel.tsx ────────── │── GET /api/dashboard\n │ └── [Mark all as read modal] │ ├── activityRepo.list() ─┐\n └── ActivityFeed.tsx ────────────────┘ ├── notifRepo.list() ─ │─ Promise.all\n └── actionRegistry() ─┘\n ▼\n PostgreSQL (existing)\n AppShell\n └── ToastProvider (new, root-level)\n```\n\n**Findings (auto-decided):**\n- Single-point-of-failure: → A1 (partial-failure contract). IN SCOPE.\n- Serial queries: → A2 (parallel). IN SCOPE.\n- Rollback: feature flag flip < 1 minute. GOOD.\n\n#### Section 2: Error & Rescue Map\n\n```\nMETHOD | WHAT CAN GO WRONG | EXCEPTION\n────────────────────────|─────────────────────────────|──────────────────\nGET /api/dashboard | One panel query timeout | QueryTimeoutError\n | All panels fail | (cascading)\n | Unhandled exception | UnhandledError ← GAP\nPOST /bulk-read | CSRF stale (403) | CSRFError\n | Auth expired (401) | AuthError\n | Network error | NetworkError\n\nEXCEPTION | RESCUED? | ACTION | USER SEES\n───────────────────|──────────|──────────────────────────────|───────────────────\nQueryTimeoutError | YES* | panelErrors field in response| Panel error + retry\nUnhandledError | NO ← GAP | — | 500 page ← BAD\nCSRFError | YES | Toast + modal close | \"Session expired\"\nAuthError | YES | Redirect /login | Login redirect\nNetworkError | YES | Error toast | \"Failed, try again\"\n```\n*Requires partial-failure contract (A1). GAP fixed by adding error middleware requirement.\n\n#### Section 3: Security\n\n| Threat | Mitigated? |\n|---|---|\n| Workspace ID injection via query param | YES — middleware provides IDs; contracts prohibit query param |\n| Direct object reference (cross-member data) | YES — existing repo methods scope to workspace+member |\n| CSRF on bulk-read | YES — CSRF tokens required for mutations |\n| XSS via activity/notification content | NO ← GAP → A9 (plaintext rendering) |\n\n#### Section 4: Interaction Edge Cases\n\n| Interaction | Edge Case | Status |\n|---|---|---|\n| Mark all read | Double-click confirm | GAP → A4 |\n| Mark all read | Navigation during request | OK (idempotent, silent complete) |\n| Notifications | New notif after snapshot | HANDLED (snapshot-time scoped API) |\n| Toast | Multiple simultaneous | GAP → A5 (max 3, FIFO) |\n| QuickActions | All actions ineligible | GAP → A7 |\n| Dashboard | Session expires mid-load | HANDLED (AuthError → redirect) |\n\n#### Section 5: Code Quality\n\n- DRY gap: 3 panels × 4 states without shared component → A6 (PanelWrapper)\n- Toast: don't hand-roll → A5 (use existing library or sonner)\n- QuickActions: direct registry import → A10 (service layer for future decoupling)\n\n#### Section 6: Tests\n\nNew things requiring tests:\n```\nNEW UX FLOWS: dashboard load, panel states (loading/empty/error/success),\n mark-all-read modal, quick action navigation, toast\nNEW DATA FLOWS: GET /api/dashboard (full + partial failure),\n POST /bulk-read\nNEW CODEPATHS: feature flag enabled/disabled, per-panel error/success/empty,\n double-click guard, auth expired during load\n```\n\nTest gaps → A14 (9 test specifications required).\n\n**2am Friday test:** Playwright full load + one panel failure + mark-all-read + keyboard nav + toast.\n\n#### Section 7: Performance\n\n| Concern | Fix |\n|---|---|\n| 3 serial DB queries = sum of latencies | A2: parallel execution required |\n| p95 latency target unspecified | A3: < 300ms in staging |\n| Existing indexes on workspace+member+created_at | No new indexes needed |\n\n#### Section 8: Observability\n\n5 analytics events missing → A11. Alert missing → A12. Runbook missing → A13.\n\nStructured logging requirement: each panel error logs `{panel, error_code, workspace_id, member_id, timestamp}`.\n\n#### Section 9: Deployment\n\n- No DB migration. GOOD.\n- Feature flag: existing system supports member-cohort rollout.\n- Rollback: flag flip < 1 minute.\n- Rollout criteria: CRITICAL GAP → A3.\n- Post-deploy: monitor p95 + panel error rate in first 15 minutes.\n\n#### Section 10: Long-Term Trajectory\n\n- Toast system: use library (A5) prevents tech debt.\n- QuickActions decoupling (A10): enables ranking service without UI changes.\n- PanelWrapper (A6): reusable by future dashboard-like features.\n- Reversibility: 4/5 (feature flag makes rollback trivial; no DB changes).\n\n#### Section 11: Design & UX\n\nPanel order: QuickActions first → A8 (information hierarchy for success metric).\n\n```\nLogin ──► QuickActions (action intent) ──► Resume work ──► Goal achieved\n │\n ├── NotificationsPanel (3 unread) ──► [optional: mark all read]\n │\n └── ActivityFeed (ambient context)\n```\n\nAccessibility gaps → A5 (live region for toast), A8 (tab order), A5 (reduced-motion).\n\nState coverage gaps: QuickActions empty → A7; partial dashboard failure → A1.\n\n---\n\n### CEO Dual Voices\n\n**CLAUDE SUBAGENT (independent CEO review):**\n1. Critical: Rollout criteria not specified\n2. High: Root cause of 75s median not validated\n3. High: Partial-failure contract deferred\n4. Medium: Action registry not validated vs. behavior frequency\n5. Medium: State-aware redirect not evaluated\n6. Medium: Alternatives section absent\n7. Medium: Toast + modal not connected to success metric\n\n**Codex:** disabled (codex_reviews=disabled)\n\n```\nCEO DUAL VOICES — CONSENSUS TABLE:\n═══════════════════════════════════════════════════════════════\n Dimension Claude Codex Consensus\n ──────────────────────────────────── ─────── ─────── ─────────\n 1. Premises valid? PARTIAL N/A N/A\n 2. Right problem to solve? MOSTLY N/A N/A\n 3. Scope calibration correct? YES N/A N/A\n 4. Alternatives sufficiently explored? NO N/A N/A\n 5. Competitive/market risks covered? YES N/A N/A\n 6. 6-month trajectory sound? MOSTLY N/A N/A\n═══════════════════════════════════════════════════════════════\nOutside disabled — 6 Consensus cells N/A, never CONFIRMED.\n```\n\n---\n\n### Required Outputs\n\n#### NOT in scope\n- Dark mode (existing out-of-scope)\n- Personalization (existing out-of-scope)\n- State-aware post-login redirect (DEFER — outside blast radius)\n- Notification badge in page `<title>` (DEFER)\n- Optimistic Mark-all-read (optional — implementer choice)\n- Action ranking service (DEFER — A10 decouples for future)\n- Keyboard shortcut for bulk read (DEFER)\n- Multi-page notification navigation (existing full notification page owns)\n\n#### What already exists\nSee Section 0B. Reusable: auth middleware, list methods, HTTP error types, UI primitives, dialog primitive, bulk-read API, test infrastructure, feature flags.\n\n#### Dream State Delta\nThis plan consolidates 3 pages → 1. Leaves open: state-aware routing, personalized actions, sub-30s median. Correct scope for v1.\n\n#### Error & Rescue Registry\nSee Section 2. 5 paths mapped, 1 critical gap (unhandled exception → 500) addressed by adding error middleware requirement.\n\n#### Failure Modes Registry\n```\nCODEPATH | FAILURE MODE | RESCUED? | TEST? | USER SEES? | LOGGED?\n──────────────────────|─────────────────────|──────────|──────-|─────────────────|────────\nGET /api/dashboard | One panel timeout | YES (A1) | YES(A14)| Panel error | YES\nGET /api/dashboard | All panels fail | YES (A1) | YES(A14)| All errors | YES\nGET /api/dashboard | Unhandled exception | YES* | NO→A14 | 500 page | YES*\nPOST /bulk-read | CSRF 403 | YES | NO→A14 | Toast+close | YES\nPOST /bulk-read | Auth 401 | YES | NO→A14 | Login redirect| YES\nActivityFeed | XSS via content | YES (A9) | NO→A14 | Plaintext | N/A\nNotifPanel | XSS via content | YES (A9) | NO→A14 | Plaintext | N/A\n```\n*Requires error middleware addition (addressed).\n\n**CRITICAL GAPS identified: 3 (all addressed by amendments A1, A9, error middleware)**\n\n---\n\n### CEO Completion Summary\n\n```\n+====================================================================+\n| MEGA PLAN REVIEW (CEO) — COMPLETION SUMMARY |\n+====================================================================+\n| Mode selected | SELECTIVE EXPANSION |\n| System Audit | Fixture only — plan specification evaluated |\n| Step 0 | SELECTIVE EXPANSION; premise challenge; |\n| | approach A retained + partial-failure added |\n| Section 1 (Arch) | 2 findings → A1, A2 |\n| Section 2 (Errors) | 5 paths mapped, 1 gap → error middleware |\n| Section 3 (Security) | 2 XSS gaps → A9 |\n| Section 4 (Data/UX) | 3 unhandled edge cases → A4, A5, A7 |\n| Section 5 (Quality) | 2 issues → A5, A6, A10 |\n| Section 6 (Tests) | 9 test gaps → A14 |\n| Section 7 (Perf) | 1 issue → A2; p95 target → A3 |\n| Section 8 (Observ) | 5 events missing → A11; alert → A12 |\n| Section 9 (Deploy) | Rollout criteria gap → A3; runbook → A13 |\n| Section 10 (Future) | Reversibility 4/5; decoupling → A10 |\n| Section 11 (Design) | Panel order → A8; a11y gaps → A5 |\n+--------------------------------------------------------------------+\n| NOT in scope | 8 items written |\n| What already exists | Written |\n| Dream state delta | Written |\n| Error/rescue registry | 5 paths, 1 gap addressed |\n| Failure modes | 7 rows, 3 critical gaps addressed |\n| TODOS.md updates | 5 items deferred |\n| Scope proposals | 0 accepted cherry-picks |\n| CEO plan | Not written (no accepted expansions) |\n| Outside voice | Claude subagent: 7 findings; Codex disabled |\n| Lake Score | 14/14 chose complete option |\n| Amendments | A1-A14 (14 requirements added) |\n| Unresolved decisions | 1 (root cause of 75s median) |\n+====================================================================+\n```\n\n<!-- autoplan-accepted:ceo -->\n- A1: Partial-failure contract for GET /api/dashboard — per-panel status fields in response; each panel renders independently\n- A2: Parallel backend query execution required (Promise.all or equivalent)\n- A3: Rollout criteria pre-specified: p95 < 300ms staging, WCAG 2.1 AA clean, error rate baseline, median < 50s first cohort\n- A4: Double-click guard on Mark all as read confirm button\n- A5: Toast system: existing library or sonner; AppShell-level; max 3; FIFO; 4s dismiss; reduced-motion; ARIA live region\n- A6: PanelWrapper shared component for panel states\n- A7: QuickActions empty state when all actions ineligible\n- A8: Panel order: QuickActions → NotificationsPanel → ActivityFeed\n- A9: Plaintext rendering for user-generated content (no dangerouslySetInnerHTML)\n- A10: QuickActions data via service layer for future ranking decoupling\n- A11: 5 analytics events specified\n- A12: Dashboard p95 alert (> 500ms / 5min → existing alert channel)\n- A13: 1-page operational runbook for dashboard endpoint failures\n- A14: 9 required test specifications (Playwright + RTL + Vitest)\n<!-- /autoplan-accepted:ceo -->\n\n---\n\n## Decision Audit Trail\n\n<!-- AUTONOMOUS DECISION LOG -->\n\n| # | Phase | Decision | Classification | Principle | Rationale | Rejected |\n|---|-------|----------|----------------|-----------|-----------|----------|\n| 1 | CEO | Mode: SELECTIVE EXPANSION | Mechanical | P6 | Feature iteration on existing workspace; autoplan pre-selects | EXPANSION, HOLD, REDUCTION |\n| 2 | CEO | Approach A retained + partial-failure contract required | Taste | P1+P5 | Both A and B valid; A is plan-specified; A1 addresses key concern | Approach B, Approach C |\n| 3 | CEO | A1: Partial-failure contract → IN SCOPE | Mechanical | P1+P2 | Critical gap; in blast radius; < 1d CC | Defer to implementation |\n| 4 | CEO | A2: Parallel queries → IN SCOPE | Mechanical | P1+P7 | Serial adds 200-400ms; correctness not taste | Accept serial |\n| 5 | CEO | A3: Rollout criteria → IN SCOPE | Mechanical | P1 | Critical governance gap; cannot enable flag safely without | Defer |\n| 6 | CEO | A4: Double-click guard → IN SCOPE | Mechanical | P1+P5 | Interaction correctness; idempotent API | Ignore |\n| 7 | CEO | A5: Toast at AppShell, use library | Mechanical | P4+P5 | DRY; existing library > hand-rolled; placement is correctness | Hand-roll |\n| 8 | CEO | A6: PanelWrapper → IN SCOPE | Mechanical | P4+P5 | 3 panels × 4 states = DRY without shared component | Separate per-panel |\n| 9 | CEO | A7: QuickActions empty state → IN SCOPE | Mechanical | P1 | Edge case; in blast radius | Omit |\n| 10 | CEO | A8: Panel order (QuickActions first) | Mechanical | P5 | Information hierarchy for success metric | Keep original order |\n| 11 | CEO | A9: Plaintext rendering (XSS) | Mechanical | P1+P3 | Security correctness; React default | Allow HTML |\n| 12 | CEO | A10: QuickActions service layer | Mechanical | P5 | Enables future ranking; explicit boundary | Direct import |\n| 13 | CEO | A11-A13: Observability + alert + runbook | Mechanical | P1+P8 | Observability is scope | Post-launch |\n| 14 | CEO | A14: Test specifications → IN SCOPE | Mechanical | P1+P6 | Concrete specs reduce ambiguity | Vague \"add tests\" |\n| 15 | CEO | State-aware redirect → DEFER | Mechanical | P3 | Outside blast radius; new routing logic | Include in plan |\n| 16 | CEO | Root cause of 75s median → USER CHALLENGE | User Challenge | — | Both models flagged; queued for Final Gate | Auto-decide |\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 | 14 amendments added; 1 User Challenge queued for gate |\n| Outside Review | Claude subagent (Codex disabled) | Independent 2nd opinion | 1 | issues_found [subagent-only] | 7 findings; Codex: disabled |\n| Eng Review | `/plan-eng-review` | Architecture & tests (required) | 0 | — | Not yet run |\n| Design Review | `/plan-design-review` | UI/UX gaps | 0 | — | Not yet run |\n| DX Review | `/plan-devex-review` | Developer experience gaps | 0 | — | Not yet run |\n\n**OUTSIDE COVERAGE:** CEO phase — Claude subagent completed (source: in-host); Codex: disabled. Consensus N/A (outside disabled). Design phase: not yet run. DX phase: not yet run. Eng phase: not yet run.\n\n**VERDICT:** CEO review complete with amendments. Eng review required before implementation.\n\n**UNRESOLVED DECISIONS:**\n- User Challenge: Root cause of 75-second median not validated — both models flagged that the dashboard may solve the wrong bottleneck if cognitive load (task ambiguity) rather than navigation fragmentation is the real cause. Your original direction (build the dashboard) stands unless you explicitly change it at the Final Gate.\n", + "note": "Actual U closed CEO after replacing the original with a summary; raw operator-cancelled/incomplete outcome is unchanged." +} diff --git a/test/fixtures/ceo-completion-handoff-u-call.json b/test/fixtures/ceo-completion-handoff-u-call.json new file mode 100644 index 000000000..a6d3e4fb9 --- /dev/null +++ b/test/fixtures/ceo-completion-handoff-u-call.json @@ -0,0 +1,189 @@ +{ + "calls": [ + { + "sessionId": "62db3fe0-af3a-479e-8e23-d045cf3d8be6", + "toolUseId": "toolu_014aEaQBfpSJq2ECNfE2f81b", + "questions": [ + { + "question": "D1 \u2014 Run /office-hours before the CEO review? <gstack-qid:plan-ceo-review-office-hours-offer>\n\nNo design doc found for this branch. `/office-hours` produces a structured problem statement, premise challenge, and explored alternatives \u2014 it gives this review much sharper input. Takes about 10 minutes. That said, this plan is narrowly scoped test coverage (2 unit tests), so the problem statement is already clear from the code.\n\nStakes if we pick wrong: /office-hours on a clearly-scoped test plan costs 10 minutes for minimal gain; skipping it on a murkier plan leaves the review shallow.", + "header": "Prerequisites", + "multiSelect": false, + "options": [ + { + "label": "Skip \u2014 proceed with standard review (Recommended)", + "description": "The plan scope is explicit: two unit tests for processPayment(). The problem statement needs no further clarification. Proceed directly to the CEO review." + }, + { + "label": "Run /office-hours first", + "description": "Run the office-hours skill to produce a structured design doc, then pick up the review. Best when the problem is still being defined \u2014 less useful here." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 \u2014 Run /office-hours before the CEO review? <gstack-qid:plan-ceo-review-office-hours-offer>\n\nNo design doc found for this branch. `/office-hours` produces a structured problem statement, premise challenge, and explored alternatives \u2014 it gives this review much sharper input. Takes about 10 minutes. That said, this plan is narrowly scoped test coverage (2 unit tests), so the problem statement is already clear from the code.\n\nStakes if we pick wrong: /office-hours on a clearly-scoped test plan costs 10 minutes for minimal gain; skipping it on a murkier plan leaves the review shallow.": "Skip \u2014 proceed with standard review (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:48:07.582Z" + }, + { + "sessionId": "62db3fe0-af3a-479e-8e23-d045cf3d8be6", + "toolUseId": "toolu_015pRmP4ca6cL2zrzWZhJHgr", + "questions": [ + { + "question": "D2 \u2014 Which implementation approach for the test coverage plan? <gstack-qid:plan-ceo-review-approach-selection>\n\nELI10: We need unit tests for processPayment(). The plan names exactly 2 tests. Do we write precisely those 2, or do we also cover adjacent gaps at the processPayment() level that the adapter suite can't catch (nil receipt, zero-amount charge, idempotency)?\n\nStakes if we pick wrong: Too narrow = we fill the named gap but leave real failure modes untested; too broad = scope creep on what was scoped as a targeted fix.\n\nRecommendation: A because the adapter suite already covers the failure taxonomy broadly; the named gap is specific and the plan is correctly scoped. Surface the adjacents as TODOS if needed.\n\nCompleteness: A=7/10, B=9/10", + "header": "Approach", + "multiSelect": false, + "options": [ + { + "label": "A) Minimal viable \u2014 exactly the 2 named tests (Recommended)", + "description": "Write one happy-path test (successful Stripe charge \u2192 correct receipt) and one 502 test (retry fires once, then fails clean). Use existing factory + virtual sleeper. Completeness: 7/10 \u2014 covers the stated gap; broader failure taxonomy lives in the adapter suite." + }, + { + "label": "B) Complete unit coverage \u2014 2 named tests + adjacent gaps", + "description": "The 2 named tests, plus: nil/zero amount guard, receipt field completeness assertion, idempotency-key collision, and concurrent call behavior. Completeness: 9/10 \u2014 covers processPayment() orchestration edge cases that the adapter suite cannot reach." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 \u2014 Which implementation approach for the test coverage plan? <gstack-qid:plan-ceo-review-approach-selection>\n\nELI10: We need unit tests for processPayment(). The plan names exactly 2 tests. Do we write precisely those 2, or do we also cover adjacent gaps at the processPayment() level that the adapter suite can't catch (nil receipt, zero-amount charge, idempotency)?\n\nStakes if we pick wrong: Too narrow = we fill the named gap but leave real failure modes untested; too broad = scope creep on what was scoped as a targeted fix.\n\nRecommendation: A because the adapter suite already covers the failure taxonomy broadly; the named gap is specific and the plan is correctly scoped. Surface the adjacents as TODOS if needed.\n\nCompleteness: A=7/10, B=9/10": "A) Minimal viable \u2014 exactly the 2 named tests (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:49:09.726Z" + }, + { + "sessionId": "62db3fe0-af3a-479e-8e23-d045cf3d8be6", + "toolUseId": "toolu_014awuScc1tYtsTh3VNuR3qX", + "questions": [ + { + "question": "D3 \u2014 Which review mode should govern this plan? <gstack-qid:plan-ceo-review-mode-selection>\n\nELI10: The plan is small (2 unit tests, clear scope, existing infrastructure). Do we review it as-is with maximum rigor, or do we also surface cherry-pick expansion opportunities (more test cases, test organization improvements) for you to opt into individually?\n\nStakes if we pick wrong: Hold Scope misses expansion opportunities that are easy to add now; Selective Expansion adds noise to a plan that's already correctly scoped.\n\nRecommendation: C (Hold Scope) because this is test gap-fill on a well-understood function. The scope is correct; the job is to make the 2 tests bulletproof.\n\nNote: options differ in kind, not coverage \u2014 no completeness score.", + "header": "Review mode", + "multiSelect": false, + "options": [ + { + "label": "C) Hold Scope \u2014 make the 2 tests bulletproof (Recommended)", + "description": "Accept the plan\u2019s scope. Review with maximum rigor: failure modes, assertion completeness, infrastructure reuse correctness, edge cases within each test, naming and isolation." + }, + { + "label": "B) Selective Expansion \u2014 hold scope + surface cherry-picks", + "description": "Hold the 2-test baseline, but surface adjacent test cases (nil receipt, idempotency, concurrent calls) as individual opt-in decisions. Good if you want to see what else is easy to add." + }, + { + "label": "A) Scope Expansion \u2014 dream big", + "description": "Push the test suite toward its platonic ideal: property-based testing, mutation testing targets, full failure taxonomy. Best when the test suite is the product; less suited here." + }, + { + "label": "D) Scope Reduction \u2014 strip to essentials", + "description": "Find the minimum tests that prove processPayment() works. Likely a single parametrized test. Only useful if the 2-test plan feels over-specified." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 \u2014 Which review mode should govern this plan? <gstack-qid:plan-ceo-review-mode-selection>\n\nELI10: The plan is small (2 unit tests, clear scope, existing infrastructure). Do we review it as-is with maximum rigor, or do we also surface cherry-pick expansion opportunities (more test cases, test organization improvements) for you to opt into individually?\n\nStakes if we pick wrong: Hold Scope misses expansion opportunities that are easy to add now; Selective Expansion adds noise to a plan that's already correctly scoped.\n\nRecommendation: C (Hold Scope) because this is test gap-fill on a well-understood function. The scope is correct; the job is to make the 2 tests bulletproof.\n\nNote: options differ in kind, not coverage \u2014 no completeness score.": "C) Hold Scope \u2014 make the 2 tests bulletproof (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:49:25.761Z" + }, + { + "sessionId": "62db3fe0-af3a-479e-8e23-d045cf3d8be6", + "toolUseId": "toolu_01DriS8PRtw2uYpL8uZ6HBnY", + "questions": [ + { + "question": "D4 \u2014 Finding 1 (Sections 1+2+6): \"fails clean\" is undefined in the 502 test spec. <gstack-qid:plan-ceo-review-finding-1-fails-clean>\n\nELI10: The plan says the 502 test should \"assert retry-with-backoff fires once, then fails clean.\" \"Fires once\" is concrete (verify stripe_mock.calls.count == 2). But \"fails clean\" has no assertion. Does processPayment() raise an exception? Return an error object? Return nil? Without specifying this, the implementer either has to read production code to guess, or writes an assertion that might accidentally pass even if the function misbehaves.\n\nStakes if we pick wrong: A test that doesn't specify what wrong result it rejects has low defensive value. A missed assertion here could let a silent-swallow bug through.\n\nRecommendation: A because naming the error contract makes the test actually fail when the behavior breaks. The plan should specify the expected failure signal even without knowing the exact exception class \u2014 at minimum, \"assert no receipt is returned\" plus \"assert the exception class or error indicator.\"\n\nNote: options differ in kind, not coverage \u2014 no completeness score.", + "header": "Finding 1", + "multiSelect": false, + "options": [ + { + "label": "A) Add failure assertion spec to the plan (Recommended)", + "description": "Add to the plan: the 502 test should assert (a) stripe_mock.calls.count == 2, AND (b) the failure mode \u2014 e.g. 'raises PaymentError or equivalent', or 'returns an error result with no receipt emitted'. The exact class is left to implementation, but the contract is stated." + }, + { + "label": "B) Leave it to implementation", + "description": "Trust the implementer to read the production code and write appropriate assertions. Acceptable if the team knows the error contract well \u2014 lower plan completeness, higher implementation risk." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 \u2014 Finding 1 (Sections 1+2+6): \"fails clean\" is undefined in the 502 test spec. <gstack-qid:plan-ceo-review-finding-1-fails-clean>\n\nELI10: The plan says the 502 test should \"assert retry-with-backoff fires once, then fails clean.\" \"Fires once\" is concrete (verify stripe_mock.calls.count == 2). But \"fails clean\" has no assertion. Does processPayment() raise an exception? Return an error object? Return nil? Without specifying this, the implementer either has to read production code to guess, or writes an assertion that might accidentally pass even if the function misbehaves.\n\nStakes if we pick wrong: A test that doesn't specify what wrong result it rejects has low defensive value. A missed assertion here could let a silent-swallow bug through.\n\nRecommendation: A because naming the error contract makes the test actually fail when the behavior breaks. The plan should specify the expected failure signal even without knowing the exact exception class \u2014 at minimum, \"assert no receipt is returned\" plus \"assert the exception class or error indicator.\"\n\nNote: options differ in kind, not coverage \u2014 no completeness score.": "A) Add failure assertion spec to the plan (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:51:38.199Z" + }, + { + "sessionId": "62db3fe0-af3a-479e-8e23-d045cf3d8be6", + "toolUseId": "toolu_01MYWY5gZzeZYCHfdbsF1myg", + "questions": [ + { + "question": "D5 \u2014 Finding 2 (Section 5+6): \"correct receipt\" is underspecified in the happy path test. <gstack-qid:plan-ceo-review-finding-2-correct-receipt>\n\nELI10: The plan says the happy path test should \"assert correct receipt is generated.\" But \"correct\" isn't defined. A test that just checks `receipt != nil` would pass this description and would also pass if the receipt had the wrong amount, wrong charge_id, or wrong status. The test would then give false confidence \u2014 it would pass in CI while the receipt is actually wrong.\n\nStakes if we pick wrong: A loose assertion on receipt correctness is low-value test coverage. A future bug that puts wrong data in the receipt would sail through CI.\n\nRecommendation: A because field-level assertions are what makes a correctness test actually test correctness. The plan should name at minimum: receipt.charge_id matches the Stripe mock's charge id, and receipt.amount matches the input charge params.\n\nNote: options differ in kind, not coverage \u2014 no completeness score.", + "header": "Finding 2", + "multiSelect": false, + "options": [ + { + "label": "A) Add field-level assertion spec to the plan (Recommended)", + "description": "Add to the plan: the happy path test should assert receipt fields against the mock\u2019s charge response \u2014 e.g. receipt.charge_id == stripe_mock.last_charge.id, receipt.amount == params[:amount], receipt.status == \"succeeded\". Exact field names left to implementation." + }, + { + "label": "B) Leave it to implementation", + "description": "Trust the implementer to define \u201ccorrect\u201d. Lower plan completeness; acceptable if the team has a shared definition of a valid receipt and won\u2019t write a nil-check and call it done." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D5 \u2014 Finding 2 (Section 5+6): \"correct receipt\" is underspecified in the happy path test. <gstack-qid:plan-ceo-review-finding-2-correct-receipt>\n\nELI10: The plan says the happy path test should \"assert correct receipt is generated.\" But \"correct\" isn't defined. A test that just checks `receipt != nil` would pass this description and would also pass if the receipt had the wrong amount, wrong charge_id, or wrong status. The test would then give false confidence \u2014 it would pass in CI while the receipt is actually wrong.\n\nStakes if we pick wrong: A loose assertion on receipt correctness is low-value test coverage. A future bug that puts wrong data in the receipt would sail through CI.\n\nRecommendation: A because field-level assertions are what makes a correctness test actually test correctness. The plan should name at minimum: receipt.charge_id matches the Stripe mock's charge id, and receipt.amount matches the input charge params.\n\nNote: options differ in kind, not coverage \u2014 no completeness score.": "A) Add field-level assertion spec to the plan (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:51:50.237Z" + }, + { + "sessionId": "62db3fe0-af3a-479e-8e23-d045cf3d8be6", + "toolUseId": "toolu_0148EaJFEcMSPAYQCEHZPJom", + "questions": [ + { + "question": "D6 \u2014 What's next after this CEO review? <gstack-qid:plan-ceo-review-next-steps>\n\nELI10: The CEO review is done. Two assertion spec gaps were caught and resolved. The plan is now ready for the Eng Review, which is the required gate before shipping. No UI scope was detected, so a design review is not needed.\n\nStakes if we pick wrong: skipping the eng review means shipping without an architecture + code quality pass. For test code this is lower risk than production code, but the eng review also validates that the test infrastructure is used correctly.\n\nRecommendation: A because eng review is the required shipping gate, and this plan is ready for it.", + "header": "Next step", + "multiSelect": false, + "options": [ + { + "label": "A) Run /plan-eng-review next (Recommended)", + "description": "Required gate. Validates architecture, test infrastructure usage, code quality, and that the 2-test plan will be implementable without hidden issues." + }, + { + "label": "B) Skip \u2014 I\u2019ll handle reviews manually", + "description": "Proceed to implementation without the eng review. Lower confidence that the test infrastructure is wired correctly, but acceptable for low-risk test coverage work." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D6 \u2014 What's next after this CEO review? <gstack-qid:plan-ceo-review-next-steps>\n\nELI10: The CEO review is done. Two assertion spec gaps were caught and resolved. The plan is now ready for the Eng Review, which is the required gate before shipping. No UI scope was detected, so a design review is not needed.\n\nStakes if we pick wrong: skipping the eng review means shipping without an architecture + code quality pass. For test code this is lower risk than production code, but the eng review also validates that the test infrastructure is used correctly.\n\nRecommendation: A because eng review is the required shipping gate, and this plan is ready for it.": "A) Run /plan-eng-review next (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:55:38.740Z" + } + ], + "reportContent": "# CEO Review: Payment Processing \u2014 Test Coverage\n\n**Mode:** HOLD SCOPE\n**Approach:** A (minimal viable \u2014 exactly the 2 named tests)\n**Branch:** main\n\n## Context\n\n`processPayment()` is a critical revenue path with zero unit tests. The Stripe adapter suite covers network-layer failure modes (timeouts, 402, 429, 502\u2192success recovery). The gap is orchestration logic inside `processPayment()` itself: receipt assembly on success, retry wiring on failure. This review adds two unit tests using existing infrastructure (payment test factory, virtual sleeper) to close that gap.\n\n---\n\n## Amended Plan\n\n### Test 1 \u2014 Happy Path\n\n**Trigger:** successful Stripe charge\n\n**Setup:**\n- Use payment test factory (configured with valid charge params)\n- Stripe mock returns a successful charge response\n\n**Assertions:**\n- `receipt.charge_id == stripe_mock.last_charge.id`\n- `receipt.amount == params[:amount]`\n- `receipt.status == \"succeeded\"` (or equivalent success indicator)\n- Receipt-builder was called exactly once\n\n**Rationale for field-level assertions:** \"correct receipt\" as a nil-check has no defensive value. A bug that produces a receipt with wrong charge_id or wrong amount would pass. These three fields constitute minimum correctness.\n\n### Test 2 \u2014 502 Error Path\n\n**Trigger:** Stripe returns 502 on both attempts (max_retries=1 \u2192 exactly two charge attempts)\n\n**Setup:**\n- Use payment test factory (max_retries=1, Stripe mock configured to return 502 on all calls)\n- Virtual sleeper injected (records backoff delay, no real wait)\n\n**Assertions:**\n- `stripe_mock.calls.count == 2` (retry fired exactly once)\n- Failure mode: assert the error contract \u2014 either:\n - `expect { processPayment(params) }.to raise_error(PaymentError)` (or the project-specific exception class), OR\n - if processPayment() returns an error object: assert `result.success? == false` and `result.receipt == nil`\n- No receipt is emitted (receipt-builder not called, or result carries no receipt)\n\n**Rationale for failure assertion:** \"fails clean\" is not an assertion. Without specifying the error contract, the test cannot distinguish a function that raises correctly from one that silently swallows the error and returns nil.\n\n---\n\n## Required Outputs\n\n### NOT in Scope\n\n- Nil/zero amount guard at processPayment() level \u2014 the adapter suite covers this; no evidence of a gap at the orchestration level\n- Idempotency-key collision test \u2014 out of scope per HOLD SCOPE mode\n- Concurrent call behavior \u2014 out of scope\n- Property-based testing for charge params \u2014 out of scope\n- Backoff interval assertion via virtual sleeper \u2014 the plan asserts retry count (2 calls); whether to additionally assert sleeper.recorded_delays.count == 1 is left to implementation discretion\n\n### What Already Exists\n\n| Sub-problem | Existing coverage | Reused? |\n|---|---|---|\n| Network timeouts | Stripe adapter suite | Yes (retained) |\n| Card declines (402) | Stripe adapter suite | Yes (retained) |\n| Rate limits (429) | Stripe adapter suite | Yes (retained) |\n| 502\u2192success recovery | Stripe adapter suite | Yes (retained) |\n| Receipt-builder failure | Receipt-builder regression tests | Yes (retained) |\n| Test factory setup | Payment test factory (max_retries=1, mock call history) | Yes (new tests use it) |\n| Virtual sleeper | Existing virtual sleeper | Yes (new 502 test uses it) |\n\n### Dream State Delta\n\n```\nCURRENT STATE THIS PLAN 12-MONTH IDEAL\nprocessPayment() --> +2 unit tests: --> Full unit coverage:\nzero unit tests happy path (receipt happy/sad/nil/idempotency/\n field assertions) + concurrent + property-based\n 502 retry (failure for charge params + mutation\n contract assertion) testing targets\n```\n\nThis plan moves from zero to the two most critical correctness checks. The 12-month ideal would add orchestration-level edge cases; that work is deferred (no evidenced gap now).\n\n---\n\n## Error & Rescue Registry (Section 2)\n\n| Method/codepath | What can go wrong | Exception class | Rescued? | Rescue action | User sees |\n|---|---|---|---|---|---|\n| processPayment() \u2014 502 retries exhausted | Stripe returns 502 twice | PaymentError (or project equivalent) | Yes (retry logic) | Raise after max_retries | Error response to caller |\n\n*Note: the test must assert this rescue \u2014 the plan now specifies this (Finding 1 remedy).*\n\n---\n\n## Failure Modes Registry\n\n| Codepath | Failure mode | Rescued? | Test? | User sees | Logged? |\n|---|---|---|---|---|---|\n| processPayment() happy path | Stripe mock success | N/A | YES (Test 1) | Receipt | N/A |\n| processPayment() 502 \u00d7 2 | Retry exhausted | YES | YES (Test 2) | Error response | Per production code |\n| processPayment() receipt fields wrong | Receipt assembly bug | N/A | YES (field assertions in Test 1) | Wrong receipt | N/A |\n\nNo CRITICAL GAPS remain after Finding 1 and Finding 2 remedies are applied.\n\n---\n\n## Diagrams\n\n### System Architecture\n\n```\n +---------------------+\n | processPayment() |\n +---------------------+\n | |\n v v\n +---------+ +-----------------+\n | Stripe | | receipt-builder |\n | adapter | +-----------------+\n +---------+\n |\n (mock in tests)\n +---------+\n | payment |\n | test |\n | factory |\n +---------+\n |\n +---------+\n | virtual |\n | sleeper |\n +---------+\n```\n\n### Data Flow \u2014 Happy Path\n\n```\n charge_params \u2500\u2500\u25b6 processPayment() \u2500\u2500\u25b6 Stripe mock (success) \u2500\u2500\u25b6 receipt-builder \u2500\u2500\u25b6 receipt\n \u2502 \u2502\n \u2514\u2500\u2500 params[:amount] \u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500 receipt.amount (asserted)\n \u2514\u2500\u2500 mock.last_charge.id \u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500 receipt.charge_id (asserted)\n```\n\n### Data Flow \u2014 502 Path\n\n```\n charge_params \u2500\u2500\u25b6 processPayment() \u2500\u2500\u25b6 Stripe mock (502) \u2500\u2500\u25b6 [backoff via virtual sleeper]\n [call 1] \u2500\u2500\u25b6 Stripe mock (502) \u2500\u2500\u25b6 raise PaymentError\n [call 2]\n stripe_mock.calls.count == 2 (asserted)\n exception raised (asserted)\n no receipt emitted (asserted)\n```\n\n### State Machine \u2014 502 Retry\n\n```\n INITIAL \u2500\u2500\u25b6 ATTEMPT_1 \u2500\u2500\u25b6 502_RECEIVED \u2500\u2500\u25b6 BACKOFF \u2500\u2500\u25b6 ATTEMPT_2 \u2500\u2500\u25b6 502_RECEIVED \u2500\u2500\u25b6 FAILED\n [virtual sleeper records delay]\n```\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: ~15min / CC: ~2min)** \u2014 processPayment-test \u2014 Specify failure assertion for 502 test: exception class or error contract\n - Surfaced by: Section 6 \u2014 \"fails clean\" undefined; implementer cannot write correct assertion without knowing error contract\n - Files: `tests/payment/process_payment_test.*`\n - Verify: 502 test asserts either `raise_error(PaymentError)` or `result.success? == false AND result.receipt == nil`\n\n- [ ] **T2 (P1, human: ~10min / CC: ~2min)** \u2014 processPayment-test \u2014 Specify field-level assertions for happy path: receipt.charge_id, receipt.amount, receipt.status\n - Surfaced by: Section 5+6 \u2014 \"correct receipt\" underspecified; nil-check passes but has zero defensive value\n - Files: `tests/payment/process_payment_test.*`\n - Verify: happy path test asserts at least 3 receipt fields against mock response values\n\n---\n\n## Completion Summary\n\n```\n+====================================================================+\n| MEGA PLAN REVIEW \u2014 COMPLETION SUMMARY |\n+====================================================================+\n| Mode selected | HOLD SCOPE |\n| Approach selected | A \u2014 exactly the 2 named tests |\n| System Audit | Clean repo, 1 commit, no TODOs |\n| Step 0 | HOLD SCOPE, approach A, no expansions |\n| Section 1 (Arch) | 1 finding (fails clean ambiguity) |\n| Section 2 (Errors) | 1 error path mapped, 0 GAPS (after remedy) |\n| Section 3 (Security)| 0 issues found |\n| Section 4 (Data/UX) | 0 unhandled edge cases |\n| Section 5 (Quality) | 1 finding (correct receipt underspecified) |\n| Section 6 (Tests) | Diagram produced, 2 gaps (both resolved) |\n| Section 7 (Perf) | 0 issues found (unit tests, no I/O) |\n| Section 8 (Observ) | 0 gaps found |\n| Section 9 (Deploy) | 0 risks (tests only) |\n| Section 10 (Future) | Reversibility: 5/5, 0 debt items |\n| Section 11 (Design) | SKIPPED (no UI scope) |\n+--------------------------------------------------------------------+\n| NOT in scope | written (5 items) |\n| What already exists | written (7 items reused) |\n| Dream state delta | written |\n| Error/rescue registry| 1 method, 0 CRITICAL GAPS |\n| Failure modes | 3 total, 0 CRITICAL GAPS |\n| TODOS.md updates | 0 items (HOLD SCOPE \u2014 no evidenced gaps) |\n| Scope proposals | 0 (HOLD SCOPE mode) |\n| CEO plan | skipped (HOLD SCOPE) |\n| Outside voice | disabled (codex_reviews=disabled) |\n| Diagrams produced | 4 (architecture, happy-path flow, 502 flow, state machine) |\n| Stale diagrams found | 0 |\n| Unresolved decisions | 0 |\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_found | mode: HOLD_SCOPE, 0 critical gaps, 2 assertion spec gaps resolved |\n| Outside Review | disabled | Independent 2nd opinion | 0 | disabled | codex_reviews=disabled |\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 \u2014 outside voice skipped. Re-enable: `gstack-config set codex_reviews enabled`.\n\n**VERDICT:** CEO REVIEWED \u2014 2 assertion spec gaps identified and resolved (failure contract for 502 test, field-level assertions for happy path). Eng review required before shipping.\n\nNO UNRESOLVED DECISIONS\n", + "reportAtMs": 1788940503130.361, + "startedAtMs": 1788940023465, + "pendingExit": { + "sessionId": "62db3fe0-af3a-479e-8e23-d045cf3d8be6", + "toolUseId": "toolu_014FEdWvpJErR8gVHCLUgEhf", + "timestamp": "2026-09-09T07:56:06.298Z", + "failed": false + } +} diff --git a/test/fixtures/ceo-preview-u-call.json b/test/fixtures/ceo-preview-u-call.json new file mode 100644 index 000000000..ed9dcf40e --- /dev/null +++ b/test/fixtures/ceo-preview-u-call.json @@ -0,0 +1,35 @@ +{ + "sessionId": "167fb463-80f5-474a-b4e1-4df63585c69f", + "toolUseId": "toolu_0158a1GLhaRxX4cMtMVsemBT", + "questions": [ + { + "question": "D1 \u2014 Which implementation approach should this plan use? <gstack-qid:plan-ceo-approach-selection>", + "header": "Approach", + "options": [ + { + "label": "A) Current plan as-is", + "description": "Ship the handler as described: raw SQL fragment, inline email with no rescue, no new tests, N+1 order query. Smallest diff. Completeness: 3/10 \u2014 SQL injection is explicit in the plan text, email failures cause ambiguous HTTP 500 after payment commits.", + "preview": "APPROACH A: Current Plan\n Risk: HIGH\n Pros: - Smallest diff\n - Minimal new code\n Cons: - SQL injection (plan explicitly states no sanitization)\n - Email exception after DB commit \u2192 HTTP 500\n \u2192 Stripe retries \u2192 dedup blocks \u2192 email silently lost\n - N+1 query on orders\n - Zero test coverage on payment path" + }, + { + "label": "B) Parameterized queries + email rescue + tests + batch query (recommended)", + "description": "Same handler class, same existing contracts, but: (1) parameterized DB lookup eliminates SQL injection, (2) email failure caught and logged, not rethrown, (3) basic tests for happy path + edge cases, (4) batch order query. Completeness: 9/10.", + "preview": "APPROACH B: Correct Implementation\n Risk: LOW\n Pros: - Eliminates SQL injection entirely\n - Email failure becomes a warning, not a 500\n - Tests catch regressions before production\n - Batch query is correct under any load\n Cons: - More LOC than Approach A\n - Requires decision on where to catch email exceptions\n Reuses: All existing guards, DB client, mail client,\n parameterized query helpers" + }, + { + "label": "C) Approach B + async queue for email fan-out", + "description": "Same as B but email goes through an async job queue. Handler returns HTTP 200 immediately after DB commit; email delivery is decoupled. Completeness: 10/10 \u2014 but larger scope, requires confirmed job infrastructure.", + "preview": "APPROACH C: Queue-Backed Notifications\n Risk: MED (depends on existing job infrastructure)\n Pros: - Email failure never affects webhook response code\n - Retry logic for email in the queue, not via Stripe replay\n - Most resilient long-term architecture\n Cons: - Larger diff if no job system confirmed\n - Introduces at-least-once email delivery question\n - More moving parts to operate" + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 \u2014 Which implementation approach should this plan use? <gstack-qid:plan-ceo-approach-selection>": "A) Current plan as-is" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:51:53.687Z" +} diff --git a/test/fixtures/ceo-preview-u-screen.txt b/test/fixtures/ceo-preview-u-screen.txt new file mode 100644 index 000000000..ee4598aa1 --- /dev/null +++ b/test/fixtures/ceo-preview-u-screen.txt @@ -0,0 +1,39 @@ + No design doc was found for this branch. Per the skill workflow: + +● User answered Claude's questions: + ⎿  · No design doc found for this branch. `/office-hours` produces a structured problem statement, premise challenge, + and explored alternatives — it gives this review much sharper input. Takes ~10 minutes. Want to run it first? → + Skip — proceed with standard review + +● Proceeding with standard review. Now Step 0C-bis — the plan's implementation approach selection is mandatory before + mode selection. + + The plan proposes building a StripePaymentWebhookHandler. I've analyzed three possible approaches: + + Premise: The plan's Database Access section explicitly says userId goes "directly into a raw SQL fragment" — and the + contracts section confirms the adapter "performs no SQL-format validation" and "does not cast, escape, or SQL-sanitize + it." This is a deliberate architectural choice in the current plan that makes it unshippable as written. + +──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── + ☐ Approach + +│ D1 — Which implementation approach should this plan use? <gstack-qid:plan-ceo-approach-selection> + +❯ 1. A) Current plan as-is ┌────────────────────────────────────────────────────────────────────────┐ + 2. B) Parameterized queries │ APPROACH A: Current Plan │ + + email rescue + tests + │ Risk: HIGH │ + batch query (recommended) │ Pros: - Smallest diff │ + 3. C) Approach B + async │ - Minimal new code │ + queue for email fan-out │ Cons: - SQL injection (plan explicitly states no sanitization) │ + │ - Email exception after DB commit → HTTP 500 │ + │ → Stripe retries → dedup blocks → email silently lost │ + │ - N+1 query on orders │ + │ - Zero test coverage on payment path │ + └────────────────────────────────────────────────────────────────────────┘ + + Notes: press n to add notes + +──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── + Chat about this + +Enter to select · ↑/↓ to navigate · n to add notes · Esc to cancel diff --git a/test/fixtures/ceo-section-u-report.md b/test/fixtures/ceo-section-u-report.md new file mode 100644 index 000000000..cbaa51123 --- /dev/null +++ b/test/fixtures/ceo-section-u-report.md @@ -0,0 +1,1037 @@ +# Plan: cache profile summaries in one process + +## Measured problem and accepted scope +The existing profile-summary service has one active process. A one-week trace +shows repeated reads of about 900 hot keys: DB CPU is 70%, with read p95 120 ms. +Add a process-local LRU wrapper to the existing repository. Acceptance targets +are at least 60% cache hits, DB CPU below 50%, and read p95 below 60 ms, with the +existing error-rate and correctness SLOs unchanged. This is an internal backend +change with no UI, API, schema, pricing, or developer onboarding change. + +## Existing contracts retained +- All reads and writes use this repository in the same process; there are no + external DB writers. Multi-process operation remains unsupported and startup + rejects that configuration while caching is enabled. +- Authentication and authorization run before repository access. Keys encode + the authenticated tenant ID and validated profile ID without ambiguity. + Values are immutable profile-summary DTOs; secrets and cache keys are never + logged. Cached results cannot bypass authorization. +- The existing LRU adapter supports 1000 entries, a 16 MiB byte cap, and a + 30-second TTL. Recorded hot data fits those limits. Absent records use a + distinct sentinel with a 10-second TTL; undefined means a cache miss. +- Cache operations are synchronous and atomic in the single JS event loop. + On any cache failure the existing adapter bypasses the cache until an empty + cache is reinitialized; repository errors keep the current typed API error + mapping. The existing per-key + single-flight wrapper coalesces simultaneous misses and releases on failure. +- A read already in progress when a write commits may return its earlier DB + snapshot to that caller. Every read begun after that write completes must + observe the committed version. TTL expiry is not a substitute for this rule. + +## Proposed wrapper integration +Keep the current read-through repository interface and shared adapters. These +are the complete new read/write ordering rules; no additional version checks or +coordination between a cache fill and a write are proposed: + +```javascript +async function readProfile(key) { + const cached = cache.get(key); + if (cached !== undefined) return cached; + const value = await repository.read(key); + cache.set(key, value); + return value; +} + +async function writeProfile(key, update) { + const saved = await repository.write(key, update); + cache.delete(key); + return saved; +} +``` + +## Verification and rollout +Existing repository contract tests cover tenant isolation, key validation, +absence, DB failures, and authorization. New wrapper tests cover hit/miss, +eviction and byte limits, TTL, adapter-failure fallback, successful-write +invalidation, failed-write preservation, and concurrent-miss coalescing. +The rollout uses the existing runtime feature flag: enable for 10% of keys, +then 50%, then all keys after one healthy hour at each stage. Monitor hit/miss, +eviction, cache bytes, fallback errors, DB CPU, and read p95 without raw IDs. +On error-rate or latency regression, disable the flag immediately; both reads +and writes bypass the cache while disabled, and enabling creates an empty cache. +Cold starts remain within the existing DB capacity. The service owner monitors +the rollout and records the results against the acceptance targets. + +## Out of scope +Distributed caching, cross-process coherence, prewarming, changing consistency +semantics, or adding new product surfaces. The repository interface preserves a +future replacement path without introducing a general cache framework now. + +--- + +## CEO REVIEW — HOLD SCOPE + +*Mode: HOLD SCOPE. Scope is accepted as stated; review focuses on correctness, +security, observability, and deployment risk. No expansions surfaced.* + +### Step 0 — Scope Confirmation + +**0A. Premise Challenge** + +The problem is measured, not hypothetical. A one-week trace showing 900 hot keys, +70% DB CPU, and p95 120 ms is solid evidence. The acceptance targets (≥60% hits, +DB CPU <50%, p95 <60 ms) are concrete and verifiable. The direct path — process- +local LRU wrapping the existing repository — is correct. Doing nothing leaves the +DB pressure growing as the hot-key set grows. + +**0B. Existing Code Leverage** + +- The existing LRU adapter is reused without modification. +- The existing single-flight wrapper is reused. +- The existing runtime feature flag is the rollout mechanism. +- The existing repository interface is preserved unchanged. + +Nothing is being rebuilt that already exists. The wrapper is purely additive. + +**0C. Dream State Mapping** + +``` +CURRENT STATE THIS PLAN 12-MONTH IDEAL +DB CPU 70% ---> Process-local LRU ---> DB CPU <50% +p95 120 ms wrapper ---> p95 <60 ms +900 repeated reads Feature-flag rollout ---> ≥60% cache hits + 10% → 50% → 100% Stable operation + Distributed cache + as next step if + scale requires +``` + +The 12-month ideal is essentially this plan's success state. The plan explicitly +defers distributed caching, which is the correct next evolution if process-count +scales. + +**0C-bis. Implementation Approaches (auto-decided)** + +Three approaches evaluated: + +``` +APPROACH A: Inline cache logic in repository methods + Summary: Add cache calls directly inside existing read/write methods. + Effort: S + Risk: Low + Pros: Fewest files; no new layer + Cons: Harder to test independently; toggle coupling; harder to replace + Reuses: Existing adapter + +APPROACH B: Wrapper layer (the plan's approach) [SELECTED] + Summary: Thin wrapper module owns cache interaction; repository unchanged. + Effort: S + Risk: Low + Pros: Independently testable; clean separation; future-replaceable + Cons: One extra layer (trivial in practice) + Reuses: Existing adapter, single-flight, feature flag + +APPROACH C: External cache (Redis) + Summary: Move caching to Redis for cross-process coherence. + Effort: XL + Risk: High + Pros: Cross-process safe + Cons: Out of scope; network latency; cross-process coherence complexity + Reuses: None of the existing infrastructure +``` + +**RECOMMENDATION: Approach B** — matches "engineered enough" preference: clean +separation, independently testable, preserves future replacement path. The wrapper +is thin enough that the extra layer costs nothing. Auto-decided: proceed with the +plan's approach. + +**0D. HOLD SCOPE Analysis** + +1. *Complexity check:* The plan touches approximately 1 new module (the wrapper) + and 1 new test file. No new services or classes beyond the wrapper. No + complexity smell. +2. *Minimum set:* The plan is already scoped to wrapper + tests + rollout + monitoring. Nothing extraneous. +3. *Stated invariants:* The ordering rule ("every read begun after write completes + must observe the committed version") is a hard requirement. Several sections + below check the implementation against it. + +**0E. Temporal Interrogation** + +``` +HOUR 1 (foundations): What is the exact cache key format? The plan asserts + "without ambiguity" but doesn't show the format. + Implementer needs to specify and document it. + +HOUR 2-3 (core logic): What happens when cache.delete throws during a + successful write? The code lets the exception + propagate, misleading the caller. Needs handling. + +HOUR 4-5 (integration): The in-flight single-flight read can call cache.set + AFTER a concurrent write's cache.delete, re-populating + the cache with stale data. This violates the ordering + invariant. Needs a guard before cache.set. + +HOUR 6+ (tests/polish): The rollout "healthy hour" criterion is qualitative. + What numeric thresholds govern advancement? + What clock mocking strategy handles TTL tests? +``` + +--- + +### Section 1: Architecture Review + +**System architecture:** + +``` + ┌─────────────────────────────────────────────────┐ + │ Profile-Summary Service Process │ + │ │ + │ Caller ──▶ AuthZ/AuthN ──▶ CacheWrapper │ + │ ├── LRU Adapter │ + │ │ (1000 keys, │ + │ │ 16 MiB, 30s) │ + │ │ │ + │ │ miss │ + │ ▼ │ + │ SingleFlight │ + │ │ │ + │ ▼ │ + │ Repository ──▶ Database │ + │ │ + │ Startup guard: rejects multi-process config │ + │ Feature flag: bypasses wrapper when disabled │ + └─────────────────────────────────────────────────┘ + + Coupling delta: CacheWrapper is a new dependency on top of Repository. + No other coupling changes. +``` + +**Data flow — readProfile (all four paths):** + +``` + key ──▶ cache.get(key) + │ + ├─ HIT ──────────────────────────────────▶ return value [HAPPY] + │ + ├─ FAIL ──▶ adapter bypass → treat as miss [ERROR → bypass] + │ + └─ MISS ──▶ await repository.read(key) + │ + ├─ ERROR ──▶ propagate; sf releases [ERROR] + │ + └─ OK ──▶ cache.set(key, value) ──▶ return value + │ + └─ FAIL ──▶ bypass; return value anyway +``` + +**Data flow — writeProfile (all four paths):** + +``` + key, update ──▶ await repository.write(key, update) + │ + ├─ ERROR ──▶ cache NOT modified; propagate [ERROR] + │ + └─ OK ──▶ cache.delete(key) ──▶ return saved + │ + └─ FAIL ──▶ exception propagates! + caller sees write failure + even though write succeeded + ← GAP (see Finding 2) +``` + +**State machine — LRU adapter:** + +``` + [ACTIVE] ──▶ cache.get/set/delete ──▶ [ACTIVE] + │ + └─ adapter failure ──▶ [BYPASS] ──▶ empty cache reinit ──▶ [ACTIVE] + │ + └─ feature flag off ──▶ [DISABLED] + [DISABLED] ──▶ flag on ──▶ [ACTIVE] (empty cache) +``` + +**CRITICAL FINDING 1: Post-write stale cache re-population** + +The proposed code has a correctness gap in the concurrent read-during-write +scenario. The single-threaded JS event loop does not prevent this interleaving: + +``` +TIMELINE readProfile(k) writeProfile(k) Cache[k] +───────────────────────────────────────────────────────────────────── +T1 cache.get(k) → miss — undefined +T2 sf: await repo.read(k) — undefined +T3 [awaiting at yield point] repo.write resolves v0 (old) +T4 [awaiting] cache.delete(k) undefined ✓ +T5 repo.read returns v0 return saved undefined +T6 cache.set(k, v0) ← STALE — v0 (BAD!) +T7 return v0 — v0 (BAD!) +``` + +After T4 the cache correctly reflects the new write. At T6, the in-flight read +re-populates the cache with the pre-write snapshot. Reads begun after T7 (after +the write completes) will get v0 from cache until TTL expiry (up to 30 seconds). +This violates the invariant: "every read begun after that write completes must +observe the committed version." + +**Recommended fix (auto-decided):** Add a re-check guard before cache.set in the +read path. Because no await occurs between the re-check and cache.set (single JS +event loop), this is safe: + +```javascript +async function readProfile(key) { + const cached = cache.get(key); + if (cached !== undefined) return cached; + const value = await repository.read(key); + // Guard: only cache if write did not invalidate the key during our read + if (cache.get(key) === undefined) { + cache.set(key, value); + } + return value; +} +``` + +This requires that the absent-record sentinel is a distinct non-undefined value +(confirmed by the plan: "undefined means a cache miss"). When the write's +cache.delete ran at T4, cache.get(k) at T6 returns undefined → guard fires → we +do not re-populate. The read still returns v0 (which is allowed per the invariant +for reads in-flight during the write). Reads begun after the write see a miss and +fetch the committed version from DB. + +**Single points of failure:** +- Cache adapter: covered by bypass fallback. +- DB: covered by existing repository error handling. +- Feature flag: disabling bypasses cache entirely; no SPOF in the flag path. + +**Rollback posture:** Flag flip. Seconds. Both read and write bypass the cache +when disabled. Enabling creates an empty cache (no stale data on re-enable). + +**Production failure scenario:** +- Cache adapter crashes mid-operation: bypass mode activates; DB load spikes + temporarily to pre-cache levels. Not a service outage, but a load event. +- DB timeout during a miss: repository error propagates; single-flight releases; + no cache poisoning (the failed read does not call cache.set). + +*Section 1 findings: 1 CRITICAL GAP (post-write stale re-population).* + +--- + +### Section 2: Error & Rescue Map + +**Error/Rescue Registry:** + +``` +METHOD/CODEPATH | WHAT CAN GO WRONG | EXCEPTION CLASS +---------------------------|--------------------------------|------------------- +readProfile(key) | cache.get adapter failure | CacheAdapterError + | repository.read typed failure | RepositoryError + | cache.set adapter failure | CacheAdapterError +writeProfile(key, update) | repository.write typed failure | RepositoryError + | cache.delete adapter failure | CacheAdapterError ← GAP + +EXCEPTION CLASS | RESCUED? | RESCUE ACTION | USER SEES +-----------------------------|----------|------------------------------|------------------ +CacheAdapterError (get) | Y | Bypass; treat as miss | Transparent +RepositoryError (read) | Y | Propagate typed error | Typed API error +CacheAdapterError (set) | Y | Bypass; return value | Transparent +RepositoryError (write) | Y | Propagate typed error | Typed API error +CacheAdapterError (delete) | N ← GAP | None — exception propagates | Spurious write + | | | failure (BAD) +``` + +**CRITICAL FINDING 2: cache.delete failure exposes a successful write as failed** + +In the proposed writeProfile: +```javascript +const saved = await repository.write(key, update); +cache.delete(key); // if this throws, caller sees write failure +return saved; +``` + +If the LRU adapter's delete operation throws, the exception bubbles out of +`writeProfile`. The write succeeded (data is in the DB) but the caller receives +an error. This breaks the typed API contract and may trigger spurious retries, +causing double-writes. + +**Recommended fix (auto-decided):** Wrap cache.delete in a try/catch. Cache +failure in the delete path is non-fatal — the adapter's bypass mechanism will +handle ongoing degradation, and the next read will miss (fetching the fresh DB +value after TTL or on the next miss): + +```javascript +async function writeProfile(key, update) { + const saved = await repository.write(key, update); + try { + cache.delete(key); + } catch { + // Adapter handles bypass; write already committed to DB + } + return saved; +} +``` + +*Section 2 findings: 1 CRITICAL GAP (cache.delete failure during write).* + +--- + +### Section 3: Security & Threat Model + +**Attack surface expansion:** None. Process-local in-memory cache has no network +surface. No new endpoints, no new parameters, no new file paths. + +**Authorization:** AuthZ runs before repository access. Cached values are DTOs +scoped by the key encoding. Cached results cannot bypass authorization. + +**Cross-tenant isolation:** Cache keys encode tenant ID + profile ID. A tenant A +key cannot collide with tenant B unless the key construction is ambiguous. + +**FINDING 3 (documentation): Key format not specified** + +The plan asserts keys are "without ambiguity" but does not document the format. +For example, if the key is `${tenantId}:${profileId}` and tenant IDs can contain +`:`, collisions are possible and cross-tenant data could leak. + +Threat: Low likelihood (internal service, key construction is controlled). +Impact: High (cross-tenant data leakage if collision occurs). + +**Recommended fix (auto-decided):** Document the exact key format in the plan and +in a code comment. The format must be collision-resistant (e.g., delimiter +escaping, URL encoding, or a structured separator that tenant/profile IDs cannot +contain). + +**PII in cache:** Profile summary DTOs are already in application memory. The +cache holds the same data already in the running process — no additional PII +exposure beyond what the repository already holds. Correct to avoid logging raw +IDs. + +**Secrets:** No new secrets. Existing env-var patterns unchanged. + +**Dependency risk:** No new npm packages. Existing LRU adapter reused. + +**Injection vectors:** Cache keys are constructed internally from authenticated, +validated IDs. No user-supplied raw strings reach cache key construction. + +*Section 3 findings: 1 WARNING (key format documentation).* + +--- + +### Section 4: Data Flow & Interaction Edge Cases + +**This is a pure backend change. No user-visible interactions exist. The UX edge +case table is N/A.** + +**Data flow diagram — full shadow paths:** + +``` + readProfile(key): + + INPUT ──▶ [key null?] ──▶ validated upstream (plan states this) + │ + ▼ + cache.get(key) + │ + ├─ HIT ──▶ return value [OK] + │ + ├─ FAIL ──▶ bypass mode; proceed as miss [OK] + │ + └─ MISS (undefined) + │ + ▼ + [await repo.read(key)] ← yield point; writes can run here + │ + ├─ ERROR ──▶ propagate; sf releases [OK] + │ + └─ OK: value + │ + ▼ + [cache.get(key) === undefined?] ← ADDED GUARD (Finding 1) + │ + ├─ YES (key was deleted by write) ──▶ skip set [OK] + │ + └─ NO ──▶ cache.set(key, value) + │ + ├─ FAIL ──▶ bypass; return value [OK] + └─ OK ──▶ return value [OK] +``` + +**Async ordering — read-write interleaving (the critical case):** + +With the guard added (Finding 1), the race is closed: + +``` +T1: readProfile miss → sf: await repo.read +T2: writeProfile → repo.write completes → cache.delete(k) [cache: undefined] +T3: readProfile resumes, value = v0 +T4: guard: cache.get(k) === undefined → true → skip cache.set ✓ +T5: readProfile returns v0 (allowed: read was in-flight during write) +T6: new readProfile → cache miss → repo.read → gets committed version ✓ +``` + +Invariant satisfied: reads begun after T2 (write completes) fetch from DB and +get the committed version. + +**Concurrent miss coalescing + write interleaving:** + +``` +T1: read1(k) → miss → sf: await repo.read +T2: read2(k) → sf coalesces with read1 +T3: write(k) → repo.write completes → cache.delete(k) +T4: read1 resumes → guard: cache.get(k) === undefined → skip cache.set +T5: sf releases → read1 returns v0; read2 returns v0 (both in-flight during write) +T6: read3(k) → cache miss → repo.read → committed version ✓ +``` + +The guard correctly handles the coalesced case too, because the sf coalescence +means only one cache.set attempt occurs (for the primary flight) — and the guard +blocks it if the key was deleted. + +**Absent record path:** + +``` +T1: readProfile(k) → miss → repo.read(k) → returns absent-sentinel +T2: guard: cache.get(k) === undefined → true (sentinel not yet set) → cache.set(k, sentinel) with 10s TTL +T3: subsequent reads within 10s → HIT → return sentinel quickly +``` + +Ensure the absent-sentinel is a distinct non-undefined value (confirmed by plan). + +*Section 4 findings: 0 new gaps (Findings 1 and 2 from Sections 1 and 2 cover +the identified issues here).* + +--- + +### Section 5: Code Quality Review + +**Organization:** Single wrapper module; clean separation from repository. +Appropriate pattern, consistent with existing adapter layer. + +**DRY:** No duplication. Cache logic exists only in the wrapper. Repository +unchanged. + +**Naming:** `readProfile`, `writeProfile` — imperative, domain-aligned. Correct. + +**Error handling patterns:** Two gaps identified (Findings 1 and 2 above). The +wrapper code as proposed does not handle either. + +**Missing edge cases in the proposed implementation:** + +1. Post-write stale re-population (Finding 1 — CRITICAL). +2. cache.delete failure during successful write (Finding 2 — CRITICAL). +3. `undefined` value guard: the plan relies on `cache.get(key) === undefined` + as the miss sentinel. If the repository ever returns `undefined` as a + legitimate value (not the absent-sentinel), the hit/miss detection breaks. + The plan says values are "immutable profile-summary DTOs" and absent records + use "a distinct sentinel" — this constraint must be enforced by the repository + layer and documented as an invariant the wrapper depends on. + +**Over-engineering check:** The wrapper is appropriately thin. No unnecessary +abstractions. + +**Under-engineering check:** The two critical gaps (Findings 1 and 2) represent +under-engineering — not handling known edge cases of async + cache interactions. + +**Cyclomatic complexity:** readProfile: 1 branch. writeProfile: 0 branches. +Complexity is minimal and appropriate. + +*Section 5 findings: 0 new (existing findings from Sections 1-2 cover the gaps).* + +--- + +### Section 6: Test Review + +**New codepaths introduced:** + +``` +NEW DATA FLOWS: + 1. Cache hit — readProfile returns cached value without DB call + 2. Cache miss — readProfile reads from DB and populates cache + 3. Absent record cached — sentinel returned and cached with 10s TTL + 4. Absent record expired — TTL expiry triggers re-fetch + 5. Cache failure read — adapter fails, bypass, DB read proceeds + 6. Cache failure write — adapter delete fails, write still succeeds (after fix) + 7. Successful-write invalidation — writeProfile writes then deletes cache key + 8. Failed-write preservation — write fails, cache NOT modified + 9. Concurrent-miss coalescing — single-flight prevents duplicate DB reads + 10. Eviction by count — cache exceeds 1000 entries → LRU eviction + 11. Eviction by bytes — cache exceeds 16 MiB → LRU eviction + 12. TTL expiry (hot key) — cached value expires after 30s + 13. TTL expiry (sentinel) — absent sentinel expires after 10s + 14. Feature flag disabled — all reads/writes bypass cache + 15. Post-write stale guard — concurrent write during in-flight read (FINDING 1) + +NEW ERROR/RESCUE PATHS: + - cache.get failure → bypass mode (test: adapter mock throws → bypass activated) + - cache.set failure → bypass; value still returned + - cache.delete failure → caught; write return value unaffected (after fix) + - repository.read failure → propagate; single-flight releases cleanly + - repository.write failure → propagate; cache unmodified +``` + +**Per-codepath test assessment:** + +| Codepath | Exists in plan? | Missing test | +|----------|-----------------|--------------| +| Cache hit | Y | — | +| Cache miss | Y | — | +| Absent record | Y | — | +| Adapter failure fallback | Y | — | +| Write invalidation | Y | — | +| Failed-write preservation | Y | — | +| Concurrent miss coalescing | Y | — | +| Eviction (count + bytes) | Y | — | +| TTL expiry | Y | — | +| Feature flag disabled | **N** | Test: flag off → cache.get never called | +| **Post-write stale guard (Finding 1)** | **N — CRITICAL** | See below | +| **cache.delete failure (Finding 2)** | **? (unclear)** | Test: delete throws → writeProfile returns saved | + +**Critical missing test — concurrent-read-during-write:** + +```javascript +it('does not re-populate cache with stale data when write invalidates during read', async () => { + // Pre-populate cache with v0 + cache.set('k', v0); + // Start a read that will pause at the await + const readPromise = readProfile('k'); + // While read is in-flight, complete a write + await writeProfile('k', newData); + // The write should have cleared the cache + expect(cache.get('k')).toBe(undefined); + // Let the read complete + const readResult = await readPromise; + // The read returns v0 (acceptable — it was in-flight during the write) + expect(readResult).toBe(v0); + // The cache must NOT be re-populated with v0 + expect(cache.get('k')).toBe(undefined); + // A fresh read must get the committed version + const freshResult = await readProfile('k'); + expect(freshResult).toBe(newData); +}); +``` + +**Test ambition check:** +- "Ship at 2am?" — The concurrent-read-during-write test above. Not in the plan. +- "Hostile QA test?" — Same test. Also: cache.delete throws during successful write → caller should NOT see a write failure. +- "Chaos test?" — Interleaved reads and writes at high frequency verifying no stale cache entry persists for more than TTL after a write. + +**Test pyramid:** Unit tests for hit/miss/eviction/TTL + integration tests for +full flow. Appropriate for a single-process in-memory cache. + +**Flakiness risk:** TTL tests depending on real wall-clock time will be flaky. +Use clock mocking (e.g., fake timers, dependency-injected clock) for TTL tests. +The plan does not mention this; it should be specified. + +*Section 6 findings: 3 test gaps (feature flag, post-write guard, cache.delete +failure path). The post-write guard test is CRITICAL.* + +--- + +### Section 7: Performance Review + +**Memory usage:** +- LRU: bounded at 1000 entries and 16 MiB. 900 hot keys fit comfortably. +- No memory leak risk — LRU eviction prevents unbounded growth. +- Worst-case steady-state: 16 MiB additional heap per process. + +**N+1 queries:** Cache reduces DB reads. No new query patterns introduced. + +**Database indexes:** No new queries. Existing index coverage unchanged. + +**Connection pool:** No new connections. Cache is in-memory. DB connections +are only opened on cache misses. + +**Slow paths (estimated):** + +| Path | Estimated p99 | +|------|---------------| +| Cache hit | < 1 ms (in-memory LRU lookup) | +| Cache miss with DB read | existing p99 (cache adds ~1 ms overhead) | +| Bypass mode | existing p99 (no overhead) | + +**FINDING 4 (operational): Rollout advancement criterion is qualitative** + +The plan says "enable for 10% of keys, then 50%, then all keys after one healthy +hour at each stage." "Healthy" is undefined. The acceptance targets (DB CPU <50%, +p95 <60 ms, ≥60% cache hits) are stated but not tied to the rollout gate criteria. + +**Recommended (auto-decided):** Define explicit gate criteria in the plan. Example: +"Advance when cache hit rate ≥50% AND fallback error rate <0.1% AND p95 <80 ms +AND no error-rate regression, sustained over the preceding hour." + +*Section 7 findings: 1 WARNING (undefined rollout gate criterion).* + +--- + +### Section 8: Observability & Debuggability Review + +**The plan specifies monitoring:** hit/miss, eviction, cache bytes, fallback +errors, DB CPU, read p95 — all without raw IDs. This is correct. + +**Gaps:** + +**FINDING 5 (observability): Metric names, alert thresholds, and "healthy" gate +criteria not defined** + +The plan lists what to monitor but not: +1. Specific metric names (how to query them in the observability stack) +2. Alert thresholds (e.g., "alert when fallback error rate > 1%") +3. Dashboard panels (what does day-1 monitoring look like?) +4. Quantitative gate criteria for rollout advancement (links to Finding 4) +5. Runbook: "Cache fallback rate spiked — what do I do?" + +Without metric names, the monitors cannot be set up before rollout, which +violates "observability is scope, not afterthought." + +**Recommended (auto-decided):** Add an observability spec to the plan listing: +- Metric names for hit count, miss count, eviction count, byte usage, fallback + error count, DB CPU, read p95 +- Alert thresholds for fallback error rate and p95 regression +- Runbook stub: fallback spike → check adapter health → disable flag if + error rate sustained > threshold → investigate adapter failure mode + +**Debuggability:** "If a bug is reported 3 weeks post-ship..." — The plan specifies +logging hit/miss and errors without raw IDs. Structured log lines at entry (key +hash, not raw key), exit (HIT/MISS/BYPASS), and on cache failure should give +enough context to reconstruct behavior. The plan does not specify log levels or +format; this should be added. + +*Section 8 findings: 1 WARNING (observability spec incomplete).* + +--- + +### Section 9: Deployment & Rollout Review + +**Migration safety:** No DB migrations. Pure in-process change. Zero downtime. + +**Feature flag:** Existing runtime feature flag is the deployment gating +mechanism. Correct. + +**Rollout order:** Flag disabled by default. Enable at 10% → 50% → 100%. + +**Rollback:** +1. Disable feature flag. (Seconds.) +2. Reads and writes bypass cache immediately. +3. Re-enable when ready — starts with empty cache. +4. No data migration, no schema rollback. + +**Deploy-time risk:** Old code and new code run simultaneously only during the +deploy itself. Since the cache is disabled by default, there is no cache-state +risk during the deploy window. + +**FINDING 6 (deployment): Multi-process detection relies only on startup check** + +The plan says "startup rejects that configuration while caching is enabled." This +covers process boot. It does not cover: +- A process crash and respawn in an environment that has since added a second + process (e.g., autoscaling event between restarts) +- Docker/k8s restart policies that could instantiate multiple replicas + +The check runs once at startup; it does not re-validate at runtime. + +**Recommended (auto-decided):** Document this as a known constraint: the startup +check is a single-point guard. If the deployment environment is changed (e.g., +replica count increased), the operator must disable caching before scaling to +multiple processes. Add this to the runbook stub. + +**Post-deploy verification:** + +The plan mentions monitoring targets. The first-5-minutes check should include: +- Cache hit rate > 0% (cache is being used) +- Fallback error rate = 0% (adapter is healthy) +- p95 not elevated (no regression) +- DB CPU not elevated vs. baseline + +*Section 9 findings: 1 WARNING (multi-process check robustness).* + +--- + +### Section 10: Long-Term Trajectory Review + +**Technical debt introduced:** Minimal. The wrapper is thin, testable, and +flag-controlled. No locking-in debt. + +**Path dependency:** The plan explicitly preserves "a future replacement path +without introducing a general cache framework now." Distributed cache (Redis) +or a different LRU implementation can replace this without changing the +repository interface. + +**Knowledge concentration:** The concurrent-read-during-write ordering rule is +subtle. It must be documented in a code comment in the wrapper, not just in this +plan. A new engineer who reads only the code needs to understand why the +post-write guard exists. + +**Reversibility:** 5/5 — Feature flag makes this completely reversible. Disabling +returns the system to identical behavior as pre-cache with an empty-cache +clean state. + +**Ecosystem fit:** In-process LRU in a Node.js single-threaded event loop is +idiomatic. The pattern is standard. + +**1-year question:** The ordering invariant and the post-write guard are the only +non-obvious elements. With a code comment explaining the invariant and the guard's +purpose, a new engineer would understand the design. Without those comments, the +guard looks like dead code and may be "cleaned up" incorrectly. + +**Dream state delta:** After this plan ships successfully: +- DB CPU: ~45% (estimated from ≥60% cache hit rate on 900 hot keys) +- p95: ~50 ms (estimated) +- Headroom for user growth before DB becomes the bottleneck + +*Section 10 findings: 0 new. Documentation of the ordering invariant in code is +implied by Finding 1's fix.* + +--- + +### Section 11: Design & UX Review + +**SKIPPED — no UI scope detected.** + +The plan states: "This is an internal backend change with no UI, API, schema, +pricing, or developer onboarding change." Section 11 does not apply. + +--- + +## Required Outputs + +### NOT in scope + +| Item | Rationale | +|------|-----------| +| Distributed cache (Redis) | Out of scope; single-process constraint | +| Cross-process coherence | Not needed in single-process model | +| Cache prewarming | Complexity not justified by 900-key set | +| Changing consistency semantics | Out of scope; read-during-write window accepted | +| New product surfaces | Backend-only change | + +### What already exists + +| Component | Status | +|-----------|--------| +| LRU adapter (1000 entries, 16 MiB, 30s TTL) | Exists; reused unchanged | +| Single-flight per-key coalescing wrapper | Exists; reused unchanged | +| Runtime feature flag for rollout | Exists; reused unchanged | +| Repository contract tests (tenant isolation, key validation, absence, DB failures, auth) | Exists; unchanged | +| Typed API error mapping | Exists; preserved | + +### Dream state delta + +``` +THIS PLAN LEAVES US AT: + DB CPU: ~45% (from ~70%) — target met if ≥60% hit rate + p95: ~50 ms (from 120 ms) — target met + Cache: Process-local; no cross-process coherence + +REMAINING GAP TO 12-MONTH IDEAL: + Distributed caching (if process count scales) + Prewarming strategies (if cold-start spikes become a concern) + Full observability stack integration (metric names, dashboards, runbooks) +``` + +### Error & Rescue Registry + +``` +METHOD/CODEPATH | WHAT CAN GO WRONG | EXCEPTION CLASS +---------------------------|--------------------------------|------------------- +readProfile(key) | cache.get adapter failure | CacheAdapterError + | repository.read typed failure | RepositoryError + | cache.set adapter failure | CacheAdapterError +writeProfile(key, update) | repository.write typed failure | RepositoryError + | cache.delete adapter failure | CacheAdapterError ← GAP + +EXCEPTION CLASS | RESCUED? | RESCUE ACTION | USER SEES +-----------------------------|----------|---------------------------------|------------------ +CacheAdapterError (get) | Y | Bypass; treat as miss | Transparent +RepositoryError (read) | Y | Propagate typed error | Typed API error +CacheAdapterError (set) | Y | Bypass; return value | Transparent +RepositoryError (write) | Y | Propagate typed error | Typed API error +CacheAdapterError (delete) | N ← GAP | None — propagates as write fail | Spurious error +``` + +### Failure Modes Registry + +``` +CODEPATH | FAILURE MODE | RESCUED? | TEST? | USER SEES? | LOGGED? +----------------------|----------------------------------|----------|-------|----------------------|-------- +readProfile | cache.get fails | Y | Y | transparent | Y +readProfile | repo.read fails | Y | Y | typed API error | Y +readProfile | cache.set fails | Y | Y | transparent | Y +readProfile | stale re-pop after write | N ← GAP | N | stale data (silent) | N ← CRITICAL +writeProfile | repo.write fails | Y | Y | typed API error | Y +writeProfile | cache.delete fails | N ← GAP | ? | false write failure | N ← CRITICAL +writeProfile | write succeeds, delete succeeds | Y | Y | transparent | Y +sf coalescing | read fails while coalesced | Y | Y | propagated error | Y +adapter bypass | bypass active, reads continue | Y | Y | transparent | Y +``` + +### Diagrams + +**System architecture:** See Section 1. + +**Data flow (including shadow paths):** See Section 1 and Section 4. + +**State machine:** See Section 1. + +**Error flow:** + +``` + Error in readProfile: + cache.get throws ──▶ bypass mode ──▶ treat as miss ──▶ repo.read + repo.read throws ──▶ propagate ──▶ sf releases ──▶ caller gets RepositoryError + cache.set throws ──▶ bypass mode ──▶ value still returned to caller + + Error in writeProfile (proposed fix applied): + repo.write throws ──▶ propagate ──▶ cache not modified ──▶ caller gets error + cache.delete throws ──▶ caught ──▶ bypass mode activates ──▶ caller gets saved +``` + +**Deployment sequence:** + +``` + 1. Deploy new code (cache disabled by default) + 2. Verify: no errors, p95 stable, DB CPU stable + 3. Enable flag at 10% + 4. Wait 1 healthy hour (gate criteria: see Finding 4) + 5. Advance to 50% + 6. Wait 1 healthy hour + 7. Advance to 100% + 8. Record results vs acceptance targets +``` + +**Rollback flowchart:** + +``` + Error/regression detected + │ + ▼ + Disable feature flag (seconds) + │ + ▼ + Cache bypassed; empty on re-enable + │ + ▼ + Investigate: adapter failure? Race? Load? + │ + ├─ Adapter issue ──▶ fix adapter ──▶ re-enable + └─ Race issue ──▶ apply guards (Finding 1/2) ──▶ re-enable +``` + +### Stale Diagram Audit + +No existing ASCII diagrams found in plan or noted codebase files. N/A. + +--- + +## Implementation Tasks + +Synthesized from this review's findings. Each task derives from a specific +finding above. Run with Claude Code; checkbox as you ship. + +- [ ] **T1 (P1, human: ~1h / CC: ~5min)** — CacheWrapper — Add post-write stale re-population guard + - Surfaced by: Section 1 (Finding 1), Section 4 — concurrent-read-during-write race violates ordering invariant + - Files: `cacheWrapper.js` (or equivalent wrapper module) + - Code: add `if (cache.get(key) === undefined)` guard before `cache.set(key, value)` in readProfile + - Verify: new concurrent-read-during-write test (see T3); existing tests pass + +- [ ] **T2 (P1, human: ~30min / CC: ~5min)** — CacheWrapper — Handle cache.delete failure in writeProfile + - Surfaced by: Section 2 (Finding 2) — delete failure propagates as spurious write error + - Files: `cacheWrapper.js` + - Code: wrap `cache.delete(key)` in try/catch in writeProfile; return saved regardless + - Verify: test — delete throws → writeProfile resolves with saved value + +- [ ] **T3 (P1, human: ~1h / CC: ~10min)** — Tests — Add concurrent-read-during-write test + - Surfaced by: Section 6 — no test covers the post-write stale re-population scenario + - Files: `cacheWrapper.test.js` + - Test: pause read at await, complete write, resume read, assert cache not re-populated with stale value + - Verify: test fails without T1 fix, passes with it + +- [ ] **T4 (P2, human: ~30min / CC: ~5min)** — Docs/Security — Document exact cache key format + - Surfaced by: Section 3 (Finding 3) — "without ambiguity" asserted but format not specified + - Files: plan doc + code comment in key construction logic + - Specify: exact format, collision-resistance guarantee, characters that tenant/profile IDs may not contain + +- [ ] **T5 (P2, human: ~30min / CC: ~5min)** — Rollout — Define quantitative gate criteria for rollout advancement + - Surfaced by: Section 7 (Finding 4) and Section 8 (Finding 5) + - Files: plan doc + runbook + - Add: explicit numeric thresholds (hit rate, fallback error rate, p95, DB CPU) that define "healthy hour" + +- [ ] **T6 (P2, human: ~20min / CC: ~5min)** — Observability — Add metric names, alert thresholds, and runbook stub + - Surfaced by: Section 8 (Finding 5) — metric names and alert thresholds not defined + - Files: plan doc + observability config (metrics definitions, alert rules) + - Include: metric names for hit/miss/eviction/bytes/fallback/p95/DB CPU; alert on fallback spike and p95 regression + +- [ ] **T7 (P3, human: ~20min / CC: ~5min)** — Deployment — Document multi-process constraint in runbook + - Surfaced by: Section 9 (Finding 6) — startup check does not cover post-startup process scaling + - Files: runbook + - Add: "disable caching before scaling to multiple replicas; re-enable only after confirming single-process config" + +- [ ] **T8 (P2, human: ~30min / CC: ~5min)** — Tests — Add clock-mocked TTL tests + - Surfaced by: Section 6 — TTL tests depending on real wall-clock time are flaky + - Files: `cacheWrapper.test.js` + - Use fake timers / dependency-injected clock for 30s and 10s TTL expiry tests + +- [ ] **T9 (P2, human: ~20min / CC: ~5min)** — Tests — Add feature-flag-disabled test + - Surfaced by: Section 6 — no test verifies that flag-off bypasses cache entirely + - Files: `cacheWrapper.test.js` + - Test: flag disabled → cache.get never called → reads go directly to repository + +--- + +``` ++====================================================================+ +| MEGA PLAN REVIEW — COMPLETION SUMMARY | ++====================================================================+ +| Mode selected | HOLD SCOPE | +| System Audit | Skipped (per run instructions) | +| Step 0 | HOLD SCOPE confirmed; Approach B selected; | +| | ordering invariant identified as critical | +| Section 1 (Arch) | 1 CRITICAL GAP (post-write stale re-pop) | +| Section 2 (Errors) | 4 error paths mapped, 1 CRITICAL GAP | +| | (cache.delete failure) | +| Section 3 (Security)| 1 WARNING (key format not documented) | +| Section 4 (Data/UX) | 0 new gaps (Findings 1-2 cover) | +| Section 5 (Quality) | 0 new (covered by Sections 1-2) | +| Section 6 (Tests) | Diagram produced, 3 gaps | +| Section 7 (Perf) | 1 WARNING (rollout gate criterion undefined) | +| Section 8 (Observ) | 1 WARNING (metric names/alerts/runbook) | +| Section 9 (Deploy) | 1 WARNING (multi-process check robustness) | +| Section 10 (Future) | Reversibility: 5/5, debt items: 0 | +| Section 11 (Design) | SKIPPED (no UI scope) | ++--------------------------------------------------------------------+ +| NOT in scope | written (5 items) | +| What already exists | written | +| Dream state delta | written | +| Error/rescue registry| 5 methods, 1 CRITICAL GAP | +| Failure modes | 9 total, 2 CRITICAL GAPS | +| TODOS.md updates | 0 items proposed (all addressed in tasks) | +| Scope proposals | 0 (HOLD SCOPE) | +| CEO plan | skipped (HOLD SCOPE) | +| Outside voice | disabled (codex_reviews: disabled) | +| Lake Score | N/A (HOLD SCOPE, no coverage options) | +| Diagrams produced | 6 (architecture, data flow, state machine, | +| | error flow, deployment, rollback) | +| Stale diagrams found | 0 | +| Unresolved decisions | 0 | ++====================================================================+ +``` + +### Unresolved Decisions + +None. All findings were auto-decided in HOLD SCOPE mode as per run instructions. +All decisions favor the more complete, conservative fix option. + +--- + +## GSTACK REVIEW REPORT + +| Review | Trigger | Why | Runs | Status | Findings | +|--------|---------|-----|------|--------|----------| +| CEO Review | `/plan-ceo-review` | Scope & strategy | 1 | issues_open | HOLD SCOPE, 2 critical gaps, 5 warnings | +| Outside Review | disabled | Independent 2nd opinion | 0 | disabled | — | +| Eng Review | `/plan-eng-review` | Architecture & tests (required) | 0 | — | — | +| Design Review | `/plan-design-review` | UI/UX gaps | 0 | — | — | +| DX Review | `/plan-devex-review` | Developer experience gaps | 0 | — | — | + +**OUTSIDE COVERAGE:** codex_reviews: disabled. Outside review step skipped per configuration. No outside review was run or attempted. Re-enable: `gstack-config set codex_reviews enabled`. + +**VERDICT:** CEO Review run (HOLD SCOPE). 2 critical gaps found (post-write stale re-population; cache.delete failure propagation). eng review required before shipping. + +NO UNRESOLVED DECISIONS diff --git a/test/fixtures/design-handoff-u-calls.json b/test/fixtures/design-handoff-u-calls.json new file mode 100644 index 000000000..71733281a --- /dev/null +++ b/test/fixtures/design-handoff-u-calls.json @@ -0,0 +1,234 @@ +[ + { + "sessionId": "ab2c7ff1-970e-40ec-84b1-59560fcd165a", + "toolUseId": "toolu_01MxehX219oH2HnxmXrNBL52", + "questions": [ + { + "question": "D1 \u2014 Pass 2/3/5: The plan identifies that saving takes 2-5 seconds with no feedback (frozen page). DESIGN.MD fully specifies the fix: inline spinner beside \"Saving\u2026\" inside the disabled Save button, aria-busy=true, reduced-motion support. The plan names this as a gap but adds no implementation task. Should we add it? <gstack-qid:plan-design-review-save-spinner>", + "header": "Save spinner", + "multiSelect": false, + "options": [ + { + "label": "Add implementation task (Recommended)", + "description": "Add a concrete task: apply DESIGN.MD pending-state pattern \u2014 spinner + 'Saving\u2026' in disabled button, aria-busy=true, prefers-reduced-motion support. Fix: ~30 min CC / ~2h human. Resolves Issues 2.1, 3 (step 6 emotional arc), and 5.5 in one task." + }, + { + "label": "Leave as gap description", + "description": "Keep the gap named but untasked. Developer reads the plan gap section and decides implementation during build. Risk: the pattern is non-trivial and may ship without reduced-motion support." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 \u2014 Pass 2/3/5: The plan identifies that saving takes 2-5 seconds with no feedback (frozen page). DESIGN.MD fully specifies the fix: inline spinner beside \"Saving\u2026\" inside the disabled Save button, aria-busy=true, reduced-motion support. The plan names this as a gap but adds no implementation task. Should we add it? <gstack-qid:plan-design-review-save-spinner>": "Add implementation task (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:53:07.541Z" + }, + { + "sessionId": "ab2c7ff1-970e-40ec-84b1-59560fcd165a", + "toolUseId": "toolu_01AZ2KWABJtBzHYDoPAhQo14", + "questions": [ + { + "question": "D2 \u2014 Pass 5: Visual Hierarchy. Save, Reset, Cancel, and Export are all rendered identically \u2014 same size, weight, and color. Nothing signals which is primary. DESIGN.MD specifies: Save is the only filled primary action (#1d4ed8 bg, white text); Reset, Cancel, Export are neutral ghost buttons. The plan names this gap but adds no implementation task. Should we add it? <gstack-qid:plan-design-review-primary-button>", + "header": "Primary button", + "multiSelect": false, + "options": [ + { + "label": "Add implementation task (Recommended)", + "description": "Add task: Apply Button primary variant (#1d4ed8 fill, white text) to Save; ghost variant to Reset, Cancel, Export. Fix aligns with DESIGN.MD exactly. ~15 min CC / ~1h human. Resolves Issue 5.1." + }, + { + "label": "Leave as gap description", + "description": "Keep gap named but untasked. Risk: developer ships all four buttons identical, user has no visual cue which action is primary \u2014 most critical UX gap on this page." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 \u2014 Pass 5: Visual Hierarchy. Save, Reset, Cancel, and Export are all rendered identically \u2014 same size, weight, and color. Nothing signals which is primary. DESIGN.MD specifies: Save is the only filled primary action (#1d4ed8 bg, white text); Reset, Cancel, Export are neutral ghost buttons. The plan names this gap but adds no implementation task. Should we add it? <gstack-qid:plan-design-review-primary-button>": "Add implementation task (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:53:15.554Z" + }, + { + "sessionId": "ab2c7ff1-970e-40ec-84b1-59560fcd165a", + "toolUseId": "toolu_01A5JA2y1jfNUU6d78Ma6G5B", + "questions": [ + { + "question": "D3 \u2014 Pass 5: Spacing. The plan reports 16px spacing appearing in a third location, alongside 24px (field groups) and 32px (sections) which match DESIGN.MD. But the plan doesn't say WHERE the 16px appears. DESIGN.MD's scale has no 16px slot: sections=32px, field groups=24px, label-to-input=8px. How should we handle this? <gstack-qid:plan-design-review-spacing-16px>", + "header": "Spacing fix", + "multiSelect": false, + "options": [ + { + "label": "Add task to locate and correct 16px spacing (Recommended)", + "description": "Add task: audit spacing in the header action group and between InlineStatus and first fieldset \u2014 most likely candidates for the 16px outlier. Replace with the correct DESIGN.MD value (8px, 24px, or 32px depending on context). ~20 min CC / ~30 min human." + }, + { + "label": "Leave location ambiguous", + "description": "Keep gap named without specifying location. Developer hunts during implementation. Risk: they fix the wrong instance, or the 16px inconsistency survives because its location was never pinned." + }, + { + "label": "Escalate \u2014 need clarification on exact location", + "description": "Pause for the user to identify which spacing element uses 16px. Safest if you know where it is. Adds one more conversation round before work begins." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 \u2014 Pass 5: Spacing. The plan reports 16px spacing appearing in a third location, alongside 24px (field groups) and 32px (sections) which match DESIGN.MD. But the plan doesn't say WHERE the 16px appears. DESIGN.MD's scale has no 16px slot: sections=32px, field groups=24px, label-to-input=8px. How should we handle this? <gstack-qid:plan-design-review-spacing-16px>": "Add task to locate and correct 16px spacing (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:53:25.574Z" + }, + { + "sessionId": "ab2c7ff1-970e-40ec-84b1-59560fcd165a", + "toolUseId": "toolu_018Cx5mafCD7jxsaibDvoWa1", + "questions": [ + { + "question": "D4 \u2014 Pass 5/6: Error Color \u2014 WCAG AA failure. The current error style (red text on light pink background) has ~3:1 contrast. WCAG AA requires 4.5:1 for normal-weight text. This fails both accessibility law and DESIGN.MD, which specifies: error.text #991b1b on error.surface #fef2f2, with an icon and explicit text. The plan names the gap but adds no implementation task. Should we add it? <gstack-qid:plan-design-review-error-contrast>", + "header": "Error contrast", + "multiSelect": false, + "options": [ + { + "label": "Add implementation task (Recommended)", + "description": "Add task: Set error text to #991b1b on #fef2f2 background, add icon + explicit error text label. Fixes both the WCAG AA violation and DESIGN.MD alignment. ~15 min CC / ~1h human. Resolves Issues 5.3 and 6.1." + }, + { + "label": "Leave as gap description", + "description": "Keep gap named but untasked. Risk: WCAG AA violation ships \u2014 legal and accessibility liability, plus poor experience for users with low vision or in challenging lighting." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 \u2014 Pass 5/6: Error Color \u2014 WCAG AA failure. The current error style (red text on light pink background) has ~3:1 contrast. WCAG AA requires 4.5:1 for normal-weight text. This fails both accessibility law and DESIGN.MD, which specifies: error.text #991b1b on error.surface #fef2f2, with an icon and explicit text. The plan names the gap but adds no implementation task. Should we add it? <gstack-qid:plan-design-review-error-contrast>": "Add implementation task (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:53:35.599Z" + }, + { + "sessionId": "ab2c7ff1-970e-40ec-84b1-59560fcd165a", + "toolUseId": "toolu_012vDse8oLT3o1hM7CqHd1sn", + "questions": [ + { + "question": "D5 \u2014 Pass 5: Typography. Three font sizes are used (14px, 16px, 18px) across form labels. DESIGN.MD specifies two roles only: 16px body/form labels/helper text; 20px section headings. The 14px violates the Universal Rule (body text must be \u226516px) and WCAG readability. The 18px conflicts with section heading size (20px) creating a muddled hierarchy. Should we add a consolidation task? <gstack-qid:plan-design-review-typography-sizes>", + "header": "Typography", + "multiSelect": false, + "options": [ + { + "label": "Add implementation task (Recommended)", + "description": "Add task: audit all form label and helper text sizes; replace 14px with 16px and 18px with 16px or 20px depending on element role. Verify no occurrence of 14px or 18px ships. ~20 min CC / ~1h human. Resolves Issue 5.4 and the <16px universal rule violation." + }, + { + "label": "Leave as gap description", + "description": "Keep gap named but untasked. Risk: 14px text ships (below minimum; violates Universal Rules), and the muddled 18px/20px near-match confuses hierarchy for sighted users and may confuse screen reader users too." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D5 \u2014 Pass 5: Typography. Three font sizes are used (14px, 16px, 18px) across form labels. DESIGN.MD specifies two roles only: 16px body/form labels/helper text; 20px section headings. The 14px violates the Universal Rule (body text must be \u226516px) and WCAG readability. The 18px conflicts with section heading size (20px) creating a muddled hierarchy. Should we add a consolidation task? <gstack-qid:plan-design-review-typography-sizes>": "Add implementation task (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:53:43.615Z" + }, + { + "sessionId": "ab2c7ff1-970e-40ec-84b1-59560fcd165a", + "toolUseId": "toolu_01Mkr9iX2J2GTch2YMHxuTU7", + "questions": [ + { + "question": "D6 \u2014 Pass 6: Reduced-motion save behavior. The plan says 'Respect reduced motion' and DESIGN.MD says the spinner has 'reduced-motion support' \u2014 but neither specifies what the user actually sees when prefers-reduced-motion: reduce is active. An animated spinner that ignores reduced-motion ships a broken accessibility experience. Which behavior should be specified? <gstack-qid:plan-design-review-reduced-motion>", + "header": "Reduced motion", + "multiSelect": false, + "options": [ + { + "label": "Static spinner + 'Saving\u2026' text (Recommended)", + "description": "Under prefers-reduced-motion: reduce, render the spinner as a static (non-animating) icon beside 'Saving\u2026'. Users still get visual feedback but no motion. Matches what most spinner libraries produce under reduced-motion." + }, + { + "label": "'Saving\u2026' text only, no spinner", + "description": "Hide the spinner entirely under reduced-motion; show 'Saving\u2026' text alone inside the disabled button. Simpler, zero animation artifacts, but removes the visual progress indicator entirely." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D6 \u2014 Pass 6: Reduced-motion save behavior. The plan says 'Respect reduced motion' and DESIGN.MD says the spinner has 'reduced-motion support' \u2014 but neither specifies what the user actually sees when prefers-reduced-motion: reduce is active. An animated spinner that ignores reduced-motion ships a broken accessibility experience. Which behavior should be specified? <gstack-qid:plan-design-review-reduced-motion>": "Static spinner + 'Saving\u2026' text (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:53:51.630Z" + }, + { + "sessionId": "ab2c7ff1-970e-40ec-84b1-59560fcd165a", + "toolUseId": "toolu_01KkdZitbTn6SvNCpZpKA2rq", + "questions": [ + { + "question": "D7 \u2014 Pass 7: InlineStatus layout stability. The persistent status area transitions between three states: blank (before first save), 'Unsaved changes', and 'Saved at HH:mm'. If no minimum height is reserved, the content below it (Profile fieldset) will shift up/down each time the text appears or disappears. Neither the plan nor DESIGN.MD specifies whether a min-height should hold the space. What should the plan specify? <gstack-qid:plan-design-review-inline-status-height>", + "header": "Status height", + "multiSelect": false, + "options": [ + { + "label": "Reserve min-height equal to one text line (Recommended)", + "description": "Add spec: InlineStatus container has min-height: 1 line-height (approximately 24px). Blank state renders empty space; text transitions without layout shift. Matches the 'Unsaved changes' line height as the stable baseline." + }, + { + "label": "Allow layout shift (no min-height)", + "description": "No min-height; content shifts when status text appears. Simpler CSS. Risk: noticeable layout jump on first edit or save \u2014 may feel broken, especially combined with the focus ring on the Save button." + }, + { + "label": "Defer \u2014 decide during implementation", + "description": "Leave this unspecified; let the developer decide during build. Risk: inconsistent between future form pages if not codified." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D7 \u2014 Pass 7: InlineStatus layout stability. The persistent status area transitions between three states: blank (before first save), 'Unsaved changes', and 'Saved at HH:mm'. If no minimum height is reserved, the content below it (Profile fieldset) will shift up/down each time the text appears or disappears. Neither the plan nor DESIGN.MD specifies whether a min-height should hold the space. What should the plan specify? <gstack-qid:plan-design-review-inline-status-height>": "Reserve min-height equal to one text line (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:54:07.678Z" + }, + { + "sessionId": "ab2c7ff1-970e-40ec-84b1-59560fcd165a", + "toolUseId": "toolu_01QeeStZr3jXcFNm2spvvn3S", + "questions": [ + { + "question": "Design review complete (6/10 \u2192 9/10). 7 decisions resolved, 6 implementation tasks added, 0 deferred. The design review added significant interaction specs (spinner pending-state, reduced-motion behavior, layout stability) that have architectural implications. Eng review is the required shipping gate. What next? <gstack-qid:plan-design-review-next-step>", + "header": "Next step", + "multiSelect": false, + "options": [ + { + "label": "Run /plan-eng-review next (Recommended)", + "description": "Eng review is the required gate before shipping. This design review added spinner behavior, reduced-motion spec, and InlineStatus layout constraints \u2014 architectural implications worth validating before implementation starts." + }, + { + "label": "Skip \u2014 handle next steps manually", + "description": "Exit plan mode and proceed on your own. You have 6 concrete implementation tasks ready to build from." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "Design review complete (6/10 \u2192 9/10). 7 decisions resolved, 6 implementation tasks added, 0 deferred. The design review added significant interaction specs (spinner pending-state, reduced-motion behavior, layout stability) that have architectural implications. Eng review is the required shipping gate. What next? <gstack-qid:plan-design-review-next-step>": "Run /plan-eng-review next (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:57:37.838Z" + } +] diff --git a/test/fixtures/devex-count-u-calls.json b/test/fixtures/devex-count-u-calls.json new file mode 100644 index 000000000..54e07c9c0 --- /dev/null +++ b/test/fixtures/devex-count-u-calls.json @@ -0,0 +1,170 @@ +{ + "sourceRun": "ship-source-u-full-paid-20260909-0740", + "status": "Captured first attempt; historical raw2setup3review floor failure remains unchanged", + "calls": [ + { + "sessionId": "5ce81f31-e588-4876-9e18-b6c6c352d1f1", + "toolUseId": "toolu_01BMxq3Rbo8Pf84iMiswFJAZ", + "questions": [ + { + "question": "D1 \u2014 Pass 1 (Getting Started): The README quickstart points to a file that doesn't ship in the package. Should we fix the quickstart path in the plan? <gstack-qid:plan-devex-quickstart-missing-file>", + "header": "Quickstart fix", + "multiSelect": false, + "options": [ + { + "label": "Fix README to use python -m evalkit.demo (Recommended)", + "description": "ELI10: The README says to run `python examples/first_eval.py` \u2014 but docs/package-contents.txt confirms that file is absent from the published package and the release archive. Every developer who follows the README quickstart hits a file-not-found error before seeing a single line of output. The fix is to update the quickstart to use `python -m evalkit.demo`, which IS shipped and produces the documented score output. Completeness: 10/10. \u2705 Immediate fix; closes a hard first-run failure before it ships. \u2705 The demo module is already described in the README and produces the correct output. \u274c Requires a coordinated README edit as part of the release plan. Recommendation: Fix this \u2014 a broken quickstart on the first command violates Zero Friction at T0 and will produce immediate negative word-of-mouth." + }, + { + "label": "Add examples/first_eval.py to the package", + "description": "Ship the missing file so the README quickstart works as written. Completeness: 9/10. \u2705 Keeps the README as-is, no doc changes needed. \u2705 Offers two distinct entry points (file + module). \u274c Requires authoring and publishing a new file, which may widen the release scope. \u274c The plan says package contents are settled; this adds to them." + }, + { + "label": "Document the gap, leave for post-beta", + "description": "Acknowledge the broken path in the plan, add a note to fix it post-beta. Completeness: 3/10. \u2705 Zero change to release scope. \u274c Ships a known broken quickstart \u2014 the very first thing an ML engineer tries will fail. \u274c Contradicts DX POLISH posture." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 \u2014 Pass 1 (Getting Started): The README quickstart points to a file that doesn't ship in the package. Should we fix the quickstart path in the plan? <gstack-qid:plan-devex-quickstart-missing-file>": "Fix README to use python -m evalkit.demo (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:49:47.733Z" + }, + { + "sessionId": "5ce81f31-e588-4876-9e18-b6c6c352d1f1", + "toolUseId": "toolu_01AzrEXtn1SgaosyQHGWB7nx", + "questions": [ + { + "question": "D2 \u2014 Pass 1 (Getting Started): The benchmarks set a <2 min TTHW target, but the mandatory 5-minute CI gate makes that unreachable. The plan retains the gate. How should this plan handle the contradiction? <gstack-qid:plan-devex-tthw-ci-gate>", + "header": "TTHW gate", + "multiSelect": false, + "options": [ + { + "label": "Add --local flag to skip CI gate for demo only (Recommended)", + "description": "ELI10: benchmarks.md says the agreed target is under 2 minutes, but the 5-minute CI wait is mandatory with no skip path. A Python ML engineer running `python -m evalkit.demo` for the first time waits 5 minutes for a CI check on bundled sample data that never changes \u2014 the wait is pointless for a demo. The fix: add a `--local` or `--offline` mode to `evalkit.demo` that uses the bundled mock transport and skips the CI gate. Completeness: 10/10. \u2705 Achieves the <2 min target for the demo/first-run experience the plan actually promises. \u2705 The current contracts already document a mock transport and offline sample data. \u2705 CI gate stays required for real evaluations \u2014 no behavior change for production use. \u274c Requires a plan addition (one flag, demo module only), slightly widens scope. Recommendation: Add the flag \u2014 a 5-minute wait on bundled sample data is indefensible friction; the mock transport already exists.", + "preview": "# What the demo would do with --local\npython -m evalkit.demo --local\n# Skips CI gate, uses bundled mock transport\n# Output in ~2s:\n example 1: score=0.80\n example 2: score=1.00\n overall: score=0.90" + }, + { + "label": "Revise target to 6 min to match reality", + "description": "Update the plan's TTHW target to reflect the actual 6-minute measured time (including the CI gate). Completeness: 5/10. \u2705 Honest; no scope change. \u274c The benchmark showed competitors at 2-4 min; accepting 6 min leaves EvalKit in Red Flag tier. \u274c The target was the agreed goal \u2014 revising it down is a regression, not a fix." + }, + { + "label": "Accept the contradiction, note in plan as known gap", + "description": "Keep the <2 min target in the plan but document the CI gate as a known TTHW blocker. Completeness: 2/10. \u2705 Zero scope change. \u274c Ships a plan with an internally inconsistent TTHW claim. \u274c The ML engineer arrives at minute 5 of a '2 minute' demo and abandons." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 \u2014 Pass 1 (Getting Started): The benchmarks set a <2 min TTHW target, but the mandatory 5-minute CI gate makes that unreachable. The plan retains the gate. How should this plan handle the contradiction? <gstack-qid:plan-devex-tthw-ci-gate>": "Add --local flag to skip CI gate for demo only (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:50:07.775Z" + }, + { + "sessionId": "5ce81f31-e588-4876-9e18-b6c6c352d1f1", + "toolUseId": "toolu_01Y9EfNisbCNCJgixcQdbc9x", + "questions": [ + { + "question": "D3 \u2014 Pass 2 (API Design): run_eval and run_batch take the same two arguments in reversed order. Should the plan fix this? <gstack-qid:plan-devex-api-arg-order>", + "header": "API arg order", + "multiSelect": false, + "options": [ + { + "label": "Standardize to consistent order with keyword-only enforcement (Recommended)", + "description": "ELI10: docs/api.md defines `run_eval(dataset, evaluator)` and `run_batch(evaluator, dataset)` \u2014 the same two arguments, but swapped. This is a classic footgun: an ML engineer who learns one function will pass args to the other in the wrong order and get silently wrong results or a runtime type error. The fix: pick one order (dataset first, then evaluator) and enforce keyword arguments in both functions \u2014 `run_eval(*, dataset, evaluator)`. The plan note says 'neither function requires keyword arguments', which is exactly the problem. Completeness: 10/10. \u2705 Eliminates silent misuse; the two functions now share a single mental model. \u2705 Keyword enforcement makes the wrong call a NameError, not a subtle bug. \u274c Breaking change for any positional callers \u2014 but this is a beta, the right moment for it. Recommendation: Fix now \u2014 this is a beta; swapping arg order post-GA is a major breaking change.", + "preview": "# Before (confusing)\nresult = run_eval(my_dataset, my_evaluator) # dataset first\nresult = run_batch(my_evaluator, my_dataset) # evaluator first\n\n# After (consistent, keyword-enforced)\nresult = run_eval(dataset=my_dataset, evaluator=my_evaluator)\nresult = run_batch(dataset=my_dataset, evaluator=my_evaluator)" + }, + { + "label": "Add runtime validation with a clear error", + "description": "Keep the inconsistent order but add a runtime check that raises if the types are wrong. Completeness: 6/10. \u2705 No breaking change to positional order. \u274c Relies on type mismatch being detectable at runtime \u2014 won't catch cases where dataset and evaluator have compatible types. \u274c Does not fix the underlying design inconsistency." + }, + { + "label": "Document the order prominently, leave the API as-is", + "description": "Add a docstring/README warning about the reversed order. Completeness: 4/10. \u2705 Zero code change. \u274c Puts the burden on the developer to remember an arbitrary exception. \u274c 'This API is surprising but documented' is still a DX failure \u2014 the Pit of Success principle means the right usage should be the obvious usage." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 \u2014 Pass 2 (API Design): run_eval and run_batch take the same two arguments in reversed order. Should the plan fix this? <gstack-qid:plan-devex-api-arg-order>": "Standardize to consistent order with keyword-only enforcement (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:50:31.820Z" + }, + { + "sessionId": "5ce81f31-e588-4876-9e18-b6c6c352d1f1", + "toolUseId": "toolu_01Qb1UdjTVfGTk2uczoVFKyi", + "questions": [ + { + "question": "D4 \u2014 Pass 3 (Error Messages): AuthError(\"request failed\") gives the developer nothing to act on. Should the plan fix this message? <gstack-qid:plan-devex-auth-error-message>", + "header": "Auth error msg", + "multiSelect": false, + "options": [ + { + "label": "Fix to include cause + actionable fix (Recommended)", + "description": "ELI10: When a developer sets the wrong API key, they get `AuthError: request failed`. That message tells them nothing: not that the key is wrong, not what to check, not where to get a valid key. The ML engineer will stare at it and Google 'evalkit AuthError request failed' \u2014 and find nothing, because it could be anything. The fix is three lines of message: what happened (authentication rejected), why (invalid key), what to do (set EVALKIT_API_KEY to a valid key from the dashboard). Completeness: 10/10. \u2705 Eliminates the most common first-run failure mode with a self-service fix. \u2705 No API surface change \u2014 same exception class, better message. \u274c Requires updating the message string in the SDK (noted as separately maintained, but the plan can specify the new message).", + "preview": "# Before\nAuthError: request failed\n\n# After\nAuthError: Authentication failed. Your EVALKIT_API_KEY was rejected.\nSet a valid key: export EVALKIT_API_KEY=<key>\nGet your key at: evalkit.io/dashboard/keys" + }, + { + "label": "Add an error code, keep brief message", + "description": "Raise `AuthError(code='auth_invalid', message='request failed')` with a structured error code. Completeness: 7/10. \u2705 Machine-readable for tools/integrations. \u274c Still says 'request failed' to the human \u2014 doesn't tell them the key is wrong or how to fix it." + }, + { + "label": "Retain the current message", + "description": "The plan already documents this gap; accept it for beta. Completeness: 1/10. \u2705 Zero change. \u274c The plan explicitly notes this message has no cause or fix instruction. DX POLISH means fixing it, not documenting it." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 \u2014 Pass 3 (Error Messages): AuthError(\"request failed\") gives the developer nothing to act on. Should the plan fix this message? <gstack-qid:plan-devex-auth-error-message>": "Fix to include cause + actionable fix (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:50:45.849Z" + }, + { + "sessionId": "5ce81f31-e588-4876-9e18-b6c6c352d1f1", + "toolUseId": "toolu_01BM4QEudSA79eLa5KCeBqEt", + "questions": [ + { + "question": "D5 \u2014 Pass 5 (Upgrade Path): Client.evaluate() is removed immediately in v2 with no alias, no deprecation warning, and no migration guide. Should the plan fix this? <gstack-qid:plan-devex-v1-v2-migration>", + "header": "v1\u2192v2 migration", + "multiSelect": false, + "options": [ + { + "label": "Add deprecated alias + migration guide (Recommended)", + "description": "ELI10: docs/api.md says v1's `Client.evaluate()` is immediately replaced by v2's `Client.run()` with no transition path. A developer upgrading from v1 gets `AttributeError: 'Client' object has no attribute 'evaluate'` with no hint that they should call `run()` instead. For a beta release targeting existing evaluators, this is upgrade-fear territory. The fix is two things: (1) a `Client.evaluate()` alias that calls `run()` and emits a `DeprecationWarning` with the migration instruction in the message, and (2) a one-page migration guide in the docs. Completeness: 9/10. \u2705 v1 code continues to work with a clear signal to upgrade. \u2705 Lowers the cost of adoption for existing users. \u274c Small scope addition \u2014 one alias, one doc page.", + "preview": "# With deprecated alias\nimport warnings\nclass Client:\n def evaluate(self, *args, **kwargs):\n warnings.warn(\n \"evaluate() is deprecated. Use run() instead. \"\n \"See: evalkit.io/docs/migrate-v1-v2\",\n DeprecationWarning, stacklevel=2\n )\n return self.run(*args, **kwargs)" + }, + { + "label": "Migration guide only, no alias", + "description": "Add a v1-to-v2 migration guide doc but do not add a compatibility alias. Completeness: 7/10. \u2705 Documents the change clearly. \u274c v1 code still breaks immediately on upgrade \u2014 the developer must read the guide before they can run anything." + }, + { + "label": "Retain immediate hard break, document in changelog", + "description": "Keep the current plan (no alias, no guide) and note the breaking change prominently in the changelog. Completeness: 3/10. \u2705 Zero code change. \u274c Upgrade-fear is the #1 reason developers freeze on old versions. An undocumented AttributeError in a v2 beta will generate support tickets and negative reviews." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D5 \u2014 Pass 5 (Upgrade Path): Client.evaluate() is removed immediately in v2 with no alias, no deprecation warning, and no migration guide. Should the plan fix this? <gstack-qid:plan-devex-v1-v2-migration>": "Add deprecated alias + migration guide (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T07:50:59.874Z" + } + ] +} diff --git a/test/fixtures/devex-count-u-retry-calls.json b/test/fixtures/devex-count-u-retry-calls.json new file mode 100644 index 000000000..ba59a4390 --- /dev/null +++ b/test/fixtures/devex-count-u-retry-calls.json @@ -0,0 +1,152 @@ +[ + { + "sessionId": "d9bdeb45-8f66-49c5-b58c-ab4adc09e697", + "toolUseId": "toolu_01TwTKAQSnw7D7EUJX9EhKct", + "questions": [ + { + "question": "D1 \u2014 EvalKit demo command: should it bypass the mandatory CI check to reach the <2 min TTHW target?\n\nELI10: The agreed onboarding target is under 2 minutes (from benchmarks.md). Today `python -m evalkit.demo` blocks for 5 minutes waiting for a CI check, giving a measured TTHW of 6 minutes \u2014 Red Flag tier vs. Competitor A's 2 minutes. The SDK already ships mock transport and offline sample data (documented as working in current-contracts.md). Using those for the demo path would bring demo TTHW to roughly 30\u201360 seconds \u2014 Champion tier \u2014 with zero new infrastructure. Real evaluations (non-demo code paths) keep the full CI gate.\n\nStakes if we pick wrong: Retaining the CI gate on the demo means the TTHW target is mathematically unreachable. An ML engineer who runs the demo in a 5-minute evaluation window hits the wait and moves on to a competitor.\n\nRecommendation: A because the mock transport already exists, the fix is confined to demo.py, and it's the only way to meet the agreed <2 min target.\n\nCompleteness: A=9/10, B=4/10\n\n<gstack-qid:devex-demo-ci-bypass>", + "header": "Demo CI gate", + "multiSelect": false, + "options": [ + { + "label": "Fix: demo uses mock transport (Recommended)", + "description": "demo.py routes through the existing mock transport so it never triggers the CI check. TTHW drops from 6 min to ~60 sec. Live evaluations keep the full gate. Engineering effort: edit demo.py to use mock transport (already ships).", + "preview": "# Resulting onboarding sequence:\npip install evalkit==2.0.0b1\nexport EVALKIT_API_KEY=...\npython -m evalkit.demo\n# => prints scores in ~60 seconds (no CI wait)\n# => TTHW: ~90 sec total (Champion tier)" + }, + { + "label": "Retain CI gate, accept 6 min TTHW", + "description": "Keep the demo identical to a real evaluation. TTHW stays at 6 minutes \u2014 below the <2 min target. Accept that the demo experience matches a slow first real evaluation rather than a fast preview.", + "preview": "# Resulting onboarding sequence:\npip install evalkit==2.0.0b1\nexport EVALKIT_API_KEY=...\npython -m evalkit.demo\n# => Waiting for CI check: 30s elapsed of 300s\n# => ... (4.5 more minutes) ...\n# => prints scores\n# => TTHW: ~6 min total (Red Flag tier)" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 \u2014 EvalKit demo command: should it bypass the mandatory CI check to reach the <2 min TTHW target?\n\nELI10: The agreed onboarding target is under 2 minutes (from benchmarks.md). Today `python -m evalkit.demo` blocks for 5 minutes waiting for a CI check, giving a measured TTHW of 6 minutes \u2014 Red Flag tier vs. Competitor A's 2 minutes. The SDK already ships mock transport and offline sample data (documented as working in current-contracts.md). Using those for the demo path would bring demo TTHW to roughly 30\u201360 seconds \u2014 Champion tier \u2014 with zero new infrastructure. Real evaluations (non-demo code paths) keep the full CI gate.\n\nStakes if we pick wrong: Retaining the CI gate on the demo means the TTHW target is mathematically unreachable. An ML engineer who runs the demo in a 5-minute evaluation window hits the wait and moves on to a competitor.\n\nRecommendation: A because the mock transport already exists, the fix is confined to demo.py, and it's the only way to meet the agreed <2 min target.\n\nCompleteness: A=9/10, B=4/10\n\n<gstack-qid:devex-demo-ci-bypass>": "Fix: demo uses mock transport (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T08:03:03.304Z" + }, + { + "sessionId": "d9bdeb45-8f66-49c5-b58c-ab4adc09e697", + "toolUseId": "toolu_01RH4Fwn3A3vReLZuPpjVmVm", + "questions": [ + { + "question": "D2 \u2014 Broken README quickstart: remove the reference to examples/first_eval.py?\n\nELI10: The README tells developers to run `python examples/first_eval.py` as the quickstart. That file does not exist in the published package or the examples archive (confirmed in docs/package-contents.txt). Every developer who follows the README literally hits `No such file or directory` on their first try. The demo command (`python -m evalkit.demo`) is the real onboarding path and works correctly. The README reference to examples/first_eval.py is an orphaned pointer that should be removed.\n\nStakes if we pick wrong: Keeping the broken reference means the first thing an ML engineer does after `pip install` is hit a Python error. First impressions are irreversible \u2014 they close the tab.\n\nRecommendation: A (remove the reference) because the demo command is the settled onboarding path and there is nothing to add the missing file to.\n\nCompleteness: A=9/10, B=5/10\n\n<gstack-qid:devex-readme-quickstart-fix>", + "header": "Quickstart fix", + "multiSelect": false, + "options": [ + { + "label": "Remove broken reference (Recommended)", + "description": "Delete the `python examples/first_eval.py` line from README. The demo command becomes the sole onboarding entry point. No file to create or ship." + }, + { + "label": "Bundle examples/first_eval.py in the package", + "description": "Create and add examples/first_eval.py to the published package so the README reference resolves. Requires authoring, testing, and packaging a new file for a path that duplicates what the demo command already covers." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 \u2014 Broken README quickstart: remove the reference to examples/first_eval.py?\n\nELI10: The README tells developers to run `python examples/first_eval.py` as the quickstart. That file does not exist in the published package or the examples archive (confirmed in docs/package-contents.txt). Every developer who follows the README literally hits `No such file or directory` on their first try. The demo command (`python -m evalkit.demo`) is the real onboarding path and works correctly. The README reference to examples/first_eval.py is an orphaned pointer that should be removed.\n\nStakes if we pick wrong: Keeping the broken reference means the first thing an ML engineer does after `pip install` is hit a Python error. First impressions are irreversible \u2014 they close the tab.\n\nRecommendation: A (remove the reference) because the demo command is the settled onboarding path and there is nothing to add the missing file to.\n\nCompleteness: A=9/10, B=5/10\n\n<gstack-qid:devex-readme-quickstart-fix>": "Remove broken reference (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T08:03:15.347Z" + }, + { + "sessionId": "d9bdeb45-8f66-49c5-b58c-ab4adc09e697", + "toolUseId": "toolu_013uLGaW6EBsh3AELNhQs8Bc", + "questions": [ + { + "question": "D3 \u2014 Inconsistent positional argument order between run_eval and run_batch: fix or require keyword args?\n\nELI10: The two core evaluation functions have their arguments in reversed order: `run_eval(dataset, evaluator)` but `run_batch(evaluator, dataset)`. A developer who learns the order from `run_eval` and then calls `run_batch` the same way silently swaps evaluator and dataset \u2014 no error, just wrong results. The reversal is described as 'intentional in the current draft' in docs/api.md, but both functions do the same logical thing (run an evaluator over a dataset), so there is no semantic reason for different ordering.\n\nStakes if we pick wrong: Silent transposition bugs are the worst kind \u2014 the code runs, the scores look plausible, and the developer ships incorrect evaluations without knowing it. ML engineers in particular expect evaluation results to be reproducible and deterministic.\n\nRecommendation: A (standardize the order) because both functions have the same semantics; inconsistency here has no upside and creates a pit of failure rather than success.\n\nCompleteness: A=9/10, B=7/10, C=6/10\n\n<gstack-qid:devex-api-arg-order>", + "header": "API arg order", + "multiSelect": false, + "options": [ + { + "label": "Standardize: (evaluator, dataset) everywhere (Recommended)", + "description": "Align both run_eval and run_batch to the same positional order. Evaluator first (the tool), dataset second (the input). Breaking change within beta, but fixing it now costs less than fixing after GA.", + "preview": "# Consistent API:\nrun_eval(evaluator, dataset) # was: (dataset, evaluator)\nrun_batch(evaluator, dataset) # unchanged\n\n# One mental model, no transposition bugs" + }, + { + "label": "Require keyword-only arguments for both", + "description": "Add a bare * to both function signatures to forbid positional calls. Developers must write run_eval(evaluator=e, dataset=d) explicitly. Catches the confusion at the call site but requires all existing callers to update their code.", + "preview": "# Keyword-only API:\nrun_eval(*, evaluator, dataset)\nrun_batch(*, evaluator, dataset)\n\n# Any positional call raises TypeError immediately:\nrun_eval(my_eval, my_data) # TypeError: takes 0 positional args" + }, + { + "label": "Document the reversal prominently, keep as-is", + "description": "Add a warning box to the API reference noting that run_eval and run_batch have reversed argument order. No code change. Relies on developers reading the docs before calling the second function." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 \u2014 Inconsistent positional argument order between run_eval and run_batch: fix or require keyword args?\n\nELI10: The two core evaluation functions have their arguments in reversed order: `run_eval(dataset, evaluator)` but `run_batch(evaluator, dataset)`. A developer who learns the order from `run_eval` and then calls `run_batch` the same way silently swaps evaluator and dataset \u2014 no error, just wrong results. The reversal is described as 'intentional in the current draft' in docs/api.md, but both functions do the same logical thing (run an evaluator over a dataset), so there is no semantic reason for different ordering.\n\nStakes if we pick wrong: Silent transposition bugs are the worst kind \u2014 the code runs, the scores look plausible, and the developer ships incorrect evaluations without knowing it. ML engineers in particular expect evaluation results to be reproducible and deterministic.\n\nRecommendation: A (standardize the order) because both functions have the same semantics; inconsistency here has no upside and creates a pit of failure rather than success.\n\nCompleteness: A=9/10, B=7/10, C=6/10\n\n<gstack-qid:devex-api-arg-order>": "Standardize: (evaluator, dataset) everywhere (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T08:03:31.404Z" + }, + { + "sessionId": "d9bdeb45-8f66-49c5-b58c-ab4adc09e697", + "toolUseId": "toolu_01MdArqW9eNA8om1uWfQyEvU", + "questions": [ + { + "question": "D4 \u2014 AuthError(\"request failed\"): add cause, env var name, and key URL?\n\nELI10: When a developer passes an invalid or missing API key, the SDK raises `AuthError(\"request failed\")`. That message names the symptom (request failed) but tells the developer nothing about what went wrong or how to fix it. They don't know: is it a network issue? A bad key? A missing env var? Is the key in the right environment variable? Where do they get a new key? This is documented as the retained behavior in docs/api.md. A Stripe-quality error would name the cause and point directly to the fix.\n\nStakes if we pick wrong: Every new developer who missets the env var (very common) sees a cryptic error and has to guess or search. An ML engineer evaluating the SDK for their team during a 30-minute slot hits this wall and concludes the SDK has rough error handling \u2014 accurate, and damaging.\n\nRecommendation: A because naming the env var and linking to key retrieval reduces a multi-minute debug loop to a 5-second fix, and it's a one-line change.\n\nNote: options differ in kind, not coverage \u2014 no completeness score.\n\n<gstack-qid:devex-autherror-message>", + "header": "AuthError msg", + "multiSelect": false, + "options": [ + { + "label": "Improve: name cause, env var, fix URL (Recommended)", + "description": "Change to: AuthError(\"invalid API key (EVALKIT_API_KEY not set or rejected). Get your key at app.evalkit.com/keys.\") Names what went wrong, which env var to check, and where to fix it.", + "preview": "# Before:\nAuthError(\"request failed\")\n\n# After:\nAuthError(\n \"invalid API key (EVALKIT_API_KEY not set or rejected). \"\n \"Get your key at app.evalkit.com/keys.\"\n)" + }, + { + "label": "Keep retained message, add to docs only", + "description": "Leave AuthError(\"request failed\") unchanged (as documented in api.md) and add a troubleshooting section to the docs explaining what it means. Developer must leave the terminal to find the fix." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 \u2014 AuthError(\"request failed\"): add cause, env var name, and key URL?\n\nELI10: When a developer passes an invalid or missing API key, the SDK raises `AuthError(\"request failed\")`. That message names the symptom (request failed) but tells the developer nothing about what went wrong or how to fix it. They don't know: is it a network issue? A bad key? A missing env var? Is the key in the right environment variable? Where do they get a new key? This is documented as the retained behavior in docs/api.md. A Stripe-quality error would name the cause and point directly to the fix.\n\nStakes if we pick wrong: Every new developer who missets the env var (very common) sees a cryptic error and has to guess or search. An ML engineer evaluating the SDK for their team during a 30-minute slot hits this wall and concludes the SDK has rough error handling \u2014 accurate, and damaging.\n\nRecommendation: A because naming the env var and linking to key retrieval reduces a multi-minute debug loop to a 5-second fix, and it's a one-line change.\n\nNote: options differ in kind, not coverage \u2014 no completeness score.\n\n<gstack-qid:devex-autherror-message>": "Improve: name cause, env var, fix URL (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T08:03:47.461Z" + }, + { + "sessionId": "d9bdeb45-8f66-49c5-b58c-ab4adc09e697", + "toolUseId": "toolu_01DnamAcLJS8iVvtBFsShu5B", + "questions": [ + { + "question": "D5 \u2014 v1\u2192v2 breaking change: add Client.evaluate compatibility alias and DeprecationWarning?\n\nELI10: Version 1 of the SDK exposes `Client.evaluate()`. Version 2 removes it immediately and replaces it with `Client.run()`. Any developer who installed the v1 beta and upgrades to v2 will hit `AttributeError: 'Client' object has no attribute 'evaluate'` the moment their code runs \u2014 with no explanation of what changed or how to fix it. No alias, no warning, no migration note. The fix is two things: (1) keep `Client.evaluate` as a deprecated alias that calls `Client.run` and prints a DeprecationWarning, so existing code doesn't break immediately; (2) add one paragraph to the changelog explaining the rename and telling developers to update their call sites.\n\nStakes if we pick wrong: Any team already using the v1 beta ships a hard AttributeError to production on upgrade. An SDK that breaks silently on upgrade loses trust permanently \u2014 especially for ML engineers who need reproducible results.\n\nRecommendation: A because the alias costs ~3 lines and gives existing callers a safe upgrade window. It's the difference between 'this SDK is polished' and 'this SDK broke my code'.\n\nCompleteness: A=9/10, B=5/10\n\n<gstack-qid:devex-v2-migration>", + "header": "v1\u2192v2 migration", + "multiSelect": false, + "options": [ + { + "label": "Add alias + DeprecationWarning + changelog note (Recommended)", + "description": "Client.evaluate = Client.run with DeprecationWarning on call. Existing v1 code continues to work with a warning. Changelog paragraph explains the rename and instructs users to update call sites before the alias is removed in v2.1.", + "preview": "# In client.py:\ndef evaluate(self, *args, **kwargs):\n import warnings\n warnings.warn(\n \"Client.evaluate() renamed to Client.run(). \"\n \"Update call sites; alias removed in v2.1.\",\n DeprecationWarning, stacklevel=2\n )\n return self.run(*args, **kwargs)\n\n# In CHANGELOG.md:\n## v2.0.0b1\n### Breaking changes\n- `Client.evaluate()` renamed to `Client.run()`.\n A compatibility alias exists and will be removed in v2.1.\n Update call sites: s/client.evaluate(/client.run(/" + }, + { + "label": "Ship hard break as-is, document in release notes", + "description": "Keep the immediate removal. Add a prominent release-notes section explaining that Client.evaluate() is gone and Client.run() replaces it. Developers must read release notes before upgrading or their code breaks on import." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D5 \u2014 v1\u2192v2 breaking change: add Client.evaluate compatibility alias and DeprecationWarning?\n\nELI10: Version 1 of the SDK exposes `Client.evaluate()`. Version 2 removes it immediately and replaces it with `Client.run()`. Any developer who installed the v1 beta and upgrades to v2 will hit `AttributeError: 'Client' object has no attribute 'evaluate'` the moment their code runs \u2014 with no explanation of what changed or how to fix it. No alias, no warning, no migration note. The fix is two things: (1) keep `Client.evaluate` as a deprecated alias that calls `Client.run` and prints a DeprecationWarning, so existing code doesn't break immediately; (2) add one paragraph to the changelog explaining the rename and telling developers to update their call sites.\n\nStakes if we pick wrong: Any team already using the v1 beta ships a hard AttributeError to production on upgrade. An SDK that breaks silently on upgrade loses trust permanently \u2014 especially for ML engineers who need reproducible results.\n\nRecommendation: A because the alias costs ~3 lines and gives existing callers a safe upgrade window. It's the difference between 'this SDK is polished' and 'this SDK broke my code'.\n\nCompleteness: A=9/10, B=5/10\n\n<gstack-qid:devex-v2-migration>": "Add alias + DeprecationWarning + changelog note (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T08:04:03.536Z" + } +] diff --git a/test/gen-skill-docs.test.ts b/test/gen-skill-docs.test.ts index 29f613074..44e6db272 100644 --- a/test/gen-skill-docs.test.ts +++ b/test/gen-skill-docs.test.ts @@ -1880,6 +1880,20 @@ describe('DESIGN_HARD_RULES resolver', () => { }); describe('Design approval reconciliation', () => { + test('token alignment reuses the approved outcome while preserving first decisions and new tradeoffs', () => { + const section = fs.readFileSync(path.join(ROOT, 'plan-design-review/sections/review-sections.md'), 'utf8'); + const pass = extractMarkdownSection(section, '### Pass 5: Design System Alignment'); + const stop = pass.indexOf('**STOP.**'); + expect(stop).toBeGreaterThan(0); + const beforeQuestion = pass.slice(0, stop); + expect(beforeQuestion).toContain('check whether an earlier pass already approved that outcome'); + expect(beforeQuestion).toContain('apply the established tokens and update every stale gap/reference under that decision'); + expect(beforeQuestion).toContain('changing the plan location or spelling out the same fix is not a new issue'); + expect(beforeQuestion).toContain('Ask again only if new evidence exposes an unresolved requirement or tradeoff, and name it'); + expect(beforeQuestion).toContain('An unapproved violation still needs its first individual decision'); + expect(pass.slice(stop)).toContain('AskUserQuestion once per issue. Do NOT batch. Recommend + WHY.'); + }); + test('loaded review section distinguishes individual issue decisions from navigation', () => { const section = fs.readFileSync(path.join(ROOT, 'plan-design-review/sections/review-sections.md'), 'utf8'); expect(section).toContain('Complete one decision cycle per unresolved finding'); diff --git a/test/helpers/ceo-completion-handoff.ts b/test/helpers/ceo-completion-handoff.ts index fe70dbd12..a8ef5b116 100644 --- a/test/helpers/ceo-completion-handoff.ts +++ b/test/helpers/ceo-completion-handoff.ts @@ -35,6 +35,35 @@ function metadataClosedReviewNavigation(declaration: string, context: string): b closedNavigationContext(context); } +/** Scope/risk explanations can contain "if" and "not" without reopening CEO work. */ +function explainedMetadataNavigation(declaration: string, descriptions: string[]): boolean { + const question = declaration.replace(/<gstack-qid:[^>]+>/gi, '').trim(); + if (!isMetadataNavigationQuestion(question)) return false; + const sentences = [question.split('\n').slice(1).join('\n'), ...descriptions] + .flatMap(text => text.trim().split(/[.!](?:\s+|$)/).map(sentence => sentence.trim()).filter(Boolean)); + const resolved = /^(?:\d+|one|two|three|four|five|six|seven|eight|nine|ten) assertion spec gaps were caught and resolved$/i; + const noDesignScope = /^No UI scope was detected, so a design review is not needed$/i; + const stakes = /^Stakes if we pick wrong: skipping the eng review means shipping without an architecture \+ code quality pass$/i; + // Validate every whole sentence before discounting the two inert phrases. + // Additional repair, conditional closure, or a different review denial is + // substantive even when it follows a valid metadata heading or recap. + if (sentences.filter(sentence => resolved.test(sentence)).length !== 1 || + !sentences.every(sentence => resolved.test(sentence) || noDesignScope.test(sentence) || stakes.test(sentence) || + /^ELI10: The CEO review is (?:done|complete|cleared|clean)$/i.test(sentence) || + /^The plan is now ready for the Eng Review, which is the required gate before shipping$/i.test(sentence) || + /^For test code this is lower risk than production code, but the eng review also validates that the test infrastructure is used correctly$/i.test(sentence) || + /^Recommendation: [A-Z] because eng review is the required shipping gate, and this plan is ready for it$/i.test(sentence) || + /^Required gate$/i.test(sentence) || + /^Validates architecture, test infrastructure usage, code quality, and that the \d+-test plan will be implementable without hidden issues$/i.test(sentence) || + /^Proceed to implementation without the eng review$/i.test(sentence) || + /^Lower confidence that the test infrastructure is wired correctly, but acceptable for low-risk test coverage work$/i.test(sentence))) return false; + const normalized = [question.split('\n')[0]!, ...sentences + .filter(sentence => !noDesignScope.test(sentence)) + .map(sentence => stakes.test(sentence) ? sentence.replace(/^Stakes if we pick wrong:/i, 'Stakes:') : sentence)] + .join('\n'); + return metadataClosedReviewNavigation(question, normalized); +} + /** A direct Eng/manual choice can put its unconditional CEO recap in a native description. */ function describedEngNavigation(question: string, descriptions: string[], context: string): boolean { if (!/^Run\s+\/plan-eng-review\s+(?:next|now)\s*\((?:the\s+)?required(?:\s+shipping)?\s+gate\),?\s+or\s+handle\s+reviews\s+manually\?$/i.test(question)) return false; @@ -134,7 +163,9 @@ function manualHandoffIndex(fp: AskUserQuestionFingerprint): number | null { 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 metadataCompletion = Boolean(id) && metadataClosedReviewNavigation(declaration, gateContext); + const metadataCompletion = Boolean(id) && (metadataClosedReviewNavigation(declaration, gateContext) || + (call.failed === false && q.options.length === 2 && + explainedMetadataNavigation(declaration, q.options.map(option => option.description ?? '')))); if (isMetadataNavigationQuestion(questionText) && /\n[ \t]*ELI10:/i.test(questionText) && !metadataCompletion) return null; const describedEngCompletion = q.options.length === 2 && describedEngNavigation(questionText, q.options.map(option => option.description ?? ''), gateContext); diff --git a/test/helpers/ceo-mode-option.ts b/test/helpers/ceo-mode-option.ts index a35d49f6b..576cf5db8 100644 --- a/test/helpers/ceo-mode-option.ts +++ b/test/helpers/ceo-mode-option.ts @@ -180,10 +180,25 @@ export function hasNativePostAnswerCeoPosture( }); } +type PosturePacket = { headers: string[]; screens: string[]; next: number; nativeId?: string; submitted: boolean }; +const postureContinuations = new WeakMap<Set<string>, { modeId: string; packet?: PosturePacket }>(); + +/** The complete native bar binds delayed-JSONL tabs to one bounded AUQ. */ +function posturePacketBar(visible: string): { headers: string[]; answered: boolean[] } | null { + const bars = [...visible.matchAll(/←([^\r\n]+)✔\s*Submit\s*→/g)]; + const bar = bars.at(-1); + if (!bar) return null; + const tabs = [...bar[1]!.matchAll(/([☐☒])\s*([^☐☒]+)/g)]; + if (tabs.length < 2 || tabs.length > 4 || bar[1]!.slice(0, tabs[0]!.index).trim()) return null; + const headers = tabs.map(tab => tab[2]!.trim().replace(/\s+/g, ' ')); + if (headers.some(header => !header) || new Set(headers).size !== headers.length) return null; + return { headers, answered: tabs.map(tab => tab[1] === '☒') }; +} + /** * Claude can defer persisting assistant prose until the next AUQ resolves. - * Permit one fresh downstream question to finish that turn, after the native - * mode answer is confirmed. A mode redraw or permission is never that question. + * Permit one fresh downstream call, including its remaining tabs and Submit. + * Native completion still supplies all posture evidence; UI only drives input. */ export function nextCeoPostureContinuation( visible: string, @@ -193,11 +208,62 @@ export function nextCeoPostureContinuation( seenQuestions: Set<string>, alreadyContinued: boolean, completionHistory = visible, -): 'permission' | 'question' | null { +): 'permission' | 'question' | 'submission' | null { const pending = transcript.calls.find(call => !call.answered && !call.failed); const permission = pending && matchesNativePlanQuestion(visible, pending) ? null : ceoPermissionAction(visible, seenQuestions, completionHistory); if (permission !== null) return permission === 'grant' ? 'permission' : null; - if (alreadyContinued || !nativeCeoModeAnswer(transcript, targetMode, selectionStartedAt)) return null; + const selected = nativeCeoModeAnswer(transcript, targetMode, selectionStartedAt); + if (!selected) return null; + const modeId = `${selected.sessionId}:${selected.toolUseId}`; + const state = postureContinuations.get(seenQuestions); + if (state && state.modeId !== modeId) return null; + const bar = posturePacketBar(visible); + const packet = state?.packet; + if (state || alreadyContinued) { + if (!packet || packet.submitted || !bar || + JSON.stringify(bar.headers) !== JSON.stringify(packet.headers) || + !bar.answered.every((answered, i) => answered === (i < packet.next))) return null; + const sameHeaders = (call: NativePlanQuestionCall) => JSON.stringify(call.questions.map(q => + q.header.trim().replace(/\s+/g, ' '))) === JSON.stringify(packet.headers); + const recorded = transcript.calls.slice(transcript.calls.indexOf(selected) + 1).find(sameHeaders); + const call = packet.nativeId + ? transcript.calls.find(call => `${call.sessionId}:${call.toolUseId}` === packet.nativeId) + : pending ?? recorded; + if (packet.nativeId && !call) return null; + if (call) { + if (call.sessionId !== selected.sessionId || call.answered || call.failed || + !sameHeaders(call) || (pending && pending !== call) || + !packet.screens.every((screen, i) => capturePlanCountQuestion( + screen, new Set(), 0, false, call)?.nativeQuestionIndex === i)) return null; + packet.nativeId = `${call.sessionId}:${call.toolUseId}`; + } + if (packet.next === packet.headers.length) { + if (planCountSubmissionInput(visible) !== '\r') return null; + packet.submitted = true; + return 'submission'; + } + } else if (bar && bar.answered.some(Boolean)) return null; + // With native metadata present, require the same call and exact displayed + // tab. Without it, the complete bar, footer and ordered answered transitions + // are required; another menu cannot spend this call's remaining tab budget. + if (bar) { + if (!/Enter\s*to\s*select\s*·\s*Tab\/Arrow\s*keys\s*to\s*navigate\s*·\s*Esc\s*to\s*cancel/i.test(visible)) return null; + if (pending && (pending.sessionId !== selected.sessionId || + !matchesNativePlanQuestion(visible, pending) || + JSON.stringify(pending.questions.map(q => q.header.trim().replace(/\s+/g, ' '))) !== JSON.stringify(bar.headers))) return null; + } const action = nextCeoModeNavigation(visible, targetMode, seenQuestions, pending, completionHistory); - return action.kind === 'question' ? 'question' : null; + if (action.kind !== 'question') return null; + if (bar) { + const current = packet ?? { headers: bar.headers, screens: [], next: 0, submitted: false }; + if (action.question.nativeCall) { + const nativeId = `${action.question.nativeCall.sessionId}:${action.question.nativeCall.toolUseId}`; + if ((current.nativeId && current.nativeId !== nativeId) || action.question.nativeQuestionIndex !== current.next) return null; + current.nativeId = nativeId; + } + current.screens.push(visible); + current.next++; + postureContinuations.set(seenQuestions, { modeId, packet: current }); + } else postureContinuations.set(seenQuestions, { modeId }); + return 'question'; } diff --git a/test/helpers/ceo-section-loading-fixture.ts b/test/helpers/ceo-section-loading-fixture.ts index fc4746878..d87ecf048 100644 --- a/test/helpers/ceo-section-loading-fixture.ts +++ b/test/helpers/ceo-section-loading-fixture.ts @@ -170,7 +170,7 @@ function hasProseStaleFillFinding(report: string): boolean { const normalize = (text: string) => text.replace(/[*_`]/g, '').replace(/\s+/g, ' ').trim(); return blocks.some((block, index) => { const text = normalize(block); - const stale = /\b(?:stale|outdated)\b|\bold(?:er)?\s+(?:value|data|result|version|snapshot)\b/i.test(text); + const stale = /\b(?:stale|outdated)\b|\b(?:old(?:er)?|pre[- ]write)\s+(?:value|data|result|version|snapshot)\b/i.test(text); const inFlight = /\b(?:race|racing|concurrent|concurrency|in[- ]flight|pending)\b/i.test(text); const read = /\b(?:read|fetch)\w*\b/i.test(text); const fillPattern = /\b(?:fill|refill|repopulat|populat|insert|stor)\w*\b|\bcache\.set\b|\bcache(?:s|d)?\s+(?:the|an?|old|stale|same)\s+(?:\w+\s+){0,2}(?:value|data|result|snapshot)\b/i; @@ -213,7 +213,7 @@ function hasProseStaleFillFinding(report: string): boolean { const previousClaim = claims[claimIndex - 1] ?? ''; const explicitStaleSubject = /^(?:this|the|an?)\s+(?:bounded\s+)?(?:inconsistency|staleness|stale[- ](?:read|fill)|stale\s+(?:read|fill|refill))(?:\s+(?:window|behavior|behaviour|race|consequence))?$/i.test(subject); const impliedStaleSubject = /^this$/i.test(subject) - && /\b(?:stale|outdated|old(?:er)?\s+(?:value|snapshot|data))\b/i.test(previousClaim) + && /\b(?:stale|outdated|old(?:er)?\s+(?:value|snapshot|data)|pre[- ]write\s+(?:value|data|result|version|snapshot))\b/i.test(previousClaim) && /\b(?:read|fetch|fill|refill|repopulat)\w*\b/i.test(previousClaim) && !/\b(?:must|shall|requires?|violat\w*|not|cannot|can't)\b/i.test(previousClaim); const acceptedStaleModel = Boolean(modelDeclaration) && (explicitStaleSubject || impliedStaleSubject); @@ -225,7 +225,7 @@ function hasProseStaleFillFinding(report: string): boolean { if (!dismissal) continue; const originalCaller = /\b(?:original|already[- ]pending)\s+(?:pending\s+)?(?:caller|reader|request)\b|\bpending\s+caller\b/i.test(claim); const onlyEarlierReturn = originalCaller && /\b(?:return|receiv|observ)\w*\b/i.test(claim) - && /\b(?:old|earlier|previous)\s+(?:snapshot|value|result|version)\b/i.test(claim) + && /\b(?:old|earlier|previous|pre[- ]write)\s+(?:snapshot|value|result|version)\b/i.test(claim) && !fillPattern.test(claim) && !/\b(?:next|later|subsequent|new|fresh)\s+(?:read\w*|request\w*|caller\w*)\b/i.test(claim); if (!(onlyEarlierReturn && subsequentRead && (violation || remedy))) return false; } diff --git a/test/helpers/claude-pty-runner.ts b/test/helpers/claude-pty-runner.ts index b633dbf53..12154b3d0 100644 --- a/test/helpers/claude-pty-runner.ts +++ b/test/helpers/claude-pty-runner.ts @@ -1261,7 +1261,7 @@ function planCountPermissionMenu(visible: string): { // The native question panel has its own header and navigation footer. // Its actual finding may discuss file creation or permission policy. if (header && !/❯\s*[1-9]\./.test(before.slice(header.index)) && - /Enter\s*to\s*select\s*·\s*↑\/↓\s*to\s*navigate\s*·\s*Esc\s*to\s*cancel/i.test(menu)) return null; + /Enter\s*to\s*select\s*·\s*↑\/↓\s*to\s*navigate\s*·\s*(?:n\s*to\s*add\s*notes\s*·\s*)?Esc\s*to\s*cancel/i.test(menu)) return null; // Anchor redraw identity to its question line; prior tool output and // the preceding menu's footer must not change a permission signature. const prompt = [...before.matchAll(/^[^\n]*\?[^\n]*$/gm)].at(-1)?.[0].trim() @@ -1289,7 +1289,7 @@ function matchesClippedNativeQuestion(visible: string, call: NativePlanQuestionC if (suffix.split('\n').filter(line => line.trim()).length < 2 || displayed.length < 160 || displayed.length > native.length || !native.endsWith(displayed)) return false; const menu = visible.slice(cursor.index); - const footer = /Enter\s*to\s*select\s*·\s*(?:↑\/↓\s*to\s*navigate|Tab\/Arrow\s*keys\s*to\s*navigate)\s*·\s*Esc\s*to\s*cancel/i.exec(menu); + const footer = /Enter\s*to\s*select\s*·\s*(?:↑\/↓\s*to\s*navigate(?:\s*·\s*n\s*to\s*add\s*notes)?|Tab\/Arrow\s*keys\s*to\s*navigate)\s*·\s*Esc\s*to\s*cancel/i.exec(menu); if (!footer || !/^[\s│┃─━└┘]*$/.test(menu.slice(footer.index + footer[0].length))) return false; const options = parseNumberedOptions(visible); const offered = options.slice(0, question.options.length); @@ -1335,7 +1335,7 @@ export function planCountQuestionInput(visible: string, fp: AskUserQuestionFinge const packet = /←[^\r\n]*[☐☒][^\r\n]*✔\s*Submit\s*→[\s\S]*❯\s*1\./.test(visible) && /Enter\s*to\s*select\s*·\s*Tab\/Arrow\s*keys\s*to\s*navigate\s*·\s*Esc\s*to\s*cancel/i.test(visible); const single = /(?:^|[\r\n])[\t │┃]*[☐□][^\r\n]+[\s\S]*❯\s*1\./.test(visible) && - /Enter\s*to\s*select\s*·\s*↑\/↓\s*to\s*navigate\s*·\s*Esc\s*to\s*cancel/i.test(visible); + /Enter\s*to\s*select\s*·\s*↑\/↓\s*to\s*navigate\s*·\s*(?:n\s*to\s*add\s*notes\s*·\s*)?Esc\s*to\s*cancel/i.test(visible); const panel = packet || single; return panel ? String(index) : `${index}\r`; } @@ -1357,9 +1357,10 @@ export function matchesNativePlanQuestion(visible: string, call: NativePlanQuest const identity = question.question.match(/<gstack-qid:[^>]+>/i)?.[0] ?? question.question; if (!compact(before.slice(header.index)).includes(compact(identity))) return false; // Preserve the captured damaged-option path when the native panel's - // complete footer is intact. With a damaged footer, require the full + // complete footer is intact, including the optional native preview notes key. + // With a damaged footer, require the full // question and every offered label instead of guessing from keywords. - if (/Enter\s*to\s*select\s*·\s*↑\/↓\s*to\s*navigate\s*·\s*Esc\s*to\s*cancel/i.test(tail.slice(cursor.index))) return true; + if (/Enter\s*to\s*select\s*·\s*↑\/↓\s*to\s*navigate\s*·\s*(?:n\s*to\s*add\s*notes\s*·\s*)?Esc\s*to\s*cancel/i.test(tail.slice(cursor.index))) return true; const displayed = before.slice(header.index).replace(/^[\t ─━┄┅┈┉┌┐└┘├┤┬┴┼│┃☐□■]+/gm, ''); const options = parseNumberedOptions(visible); return compact(displayed) === compact(`${question.header} ${question.question}`) && diff --git a/test/helpers/design-count-review.ts b/test/helpers/design-count-review.ts index eb19a8e49..63d69d4ec 100644 --- a/test/helpers/design-count-review.ts +++ b/test/helpers/design-count-review.ts @@ -45,6 +45,34 @@ export function isDesignCountFirstReview(fp: AskUserQuestionFingerprint): boolea }); } +/** A closed recap may explain why Eng is next; it cannot request another fix. */ +function closedDesignGateRecap(tail: string, descriptions: string[]): boolean { + const navigation = /\bWhat(?:['’]s)?\s+next\?\s*<gstack-qid:[a-z0-9-]+>\s*$/i.exec(tail); + if (!navigation) return false; + const body = tail.slice(0, navigation.index).trim(); + const gate = /^(?:Eng(?:ineering)? Review is (?:the )?required (?:shipping gate|gate before shipping))[.!]?$/i; + const sentences = (text: string) => text.split(/[.!]\s+|[.!]$/).map(s => s.trim()).filter(Boolean); + const recap = (text: string): boolean => { + // Each count describes completed or explicitly absent work. A positive + // deferred/open count is not a closed review, regardless of its title. + const count = /^(?:(?:\d+|all)\s+(?:design\s+)?(?:decisions|findings|issues)\s+(?:(?:are|were)\s+)?(?:resolved|approved|addressed|closed)|\d+\s+(?:implementation\s+)?tasks\s+(?:(?:are|were)\s+)?(?:added|recorded|ready)|(?:no|zero|0)\s+(?:deferred(?:\s+(?:decisions|findings|issues|tasks|items))?|(?:unresolved|open|pending|outstanding)\s+(?:decisions|findings|issues|tasks|items)))$/i; + if (text.split(/,\s*(?:and\s+)?|\s+and\s+/i).every(part => count.test(part))) return true; + // Only a declarative completed-review subject can introduce explanatory + // content. Separate clauses, questions and conditional/future work fail. + if (!/^(?:The|This)\s+(?:design\s+)?review\s+(?:has\s+)?(?:added|recorded|approved|addressed|specified|covered|resolved)\s+\S/i.test(text)) return false; + if (/[;?<>]|\b(?:if|unless|until|once|when|should|must|need|needs|will|would|could|please|then|also|still|missing|unresolved)\b|\b(?:and|but)\s+(?:first\s+)?(?:do|add|fix|repair|implement|resolve|decide|configure|remove|delete|pick|choose)\b/i.test(text)) return false; + const clauses = text.split(/\s+[—–]\s+/); + return clauses.length <= 2 && (clauses.length === 1 || /^(?:architectural|engineering|implementation)\s+(?:implications|considerations|details)\b/i.test(clauses[1]!)); + }; + const parts = sentences(body); + if (parts.filter(part => gate.test(part)).length !== 1 || + !parts.every(part => gate.test(part) || recap(part))) return false; + return descriptions.every(description => sentences(description).every(part => + gate.test(part) || recap(part) || + /^Exit plan mode and proceed on your own$/i.test(part) || + /^You have \d+ (?:concrete )?(?:implementation )?tasks ready to build from$/i.test(part))); +} + function designHandoff(fp: AskUserQuestionFingerprint): { manualIndex: number | null } | null { const call = fp.nativeCall; if (!call || call.failed || call.questions.length !== 1 || @@ -60,7 +88,9 @@ function designHandoff(fp: AskUserQuestionFingerprint): { manualIndex: number | const completed = /^Design\s+review\s+(?:is\s+)?complete(?:[.!]|\s+\((?:\d+(?:\.\d+)?(?:\/10)?\s*(?:→|->|to)\s*)?\d+(?:\.\d+)?\/10(?:,\s*\d+\s+decisions?(?:\s+(?:made|added))?)?\)[.!])(?:\s|$)/i.exec(declaration); if (!completed) return null; const requiredGateOffer = /^The required next gate is Eng(?:ineering)? Review\s*[—–-]\s*want me to run it now\?\s*<gstack-qid:[a-z0-9-]+>\s*$/i.test(declaration.slice(completed[0].length).trim()); - const requiredGateQuestion = requiredGateOffer || /^(?:\d+ implementation tasks ready\.\s*)?Eng(?:ineering)? Review is the required shipping gate\.\s*What next\?\s*<gstack-qid:[a-z0-9-]+>\s*$/i.test(declaration.slice(completed[0].length).trim()); + const closedRecap = q.options.length === 2 && closedDesignGateRecap( + declaration.slice(completed[0].length).trim(), q.options.map(option => option.description ?? '')); + const requiredGateQuestion = requiredGateOffer || closedRecap || /^(?:\d+ implementation tasks ready\.\s*)?Eng(?:ineering)? Review is the required shipping gate\.\s*What next\?\s*<gstack-qid:[a-z0-9-]+>\s*$/i.test(declaration.slice(completed[0].length).trim()); // The offered Eng action can carry the required-gate declaration while the // closed question asks only what is next. Its descriptions remain part of // the decision, so they cannot conceal a new repair or conditional closure. diff --git a/test/helpers/devex-count-fixture.ts b/test/helpers/devex-count-fixture.ts index 4b2c6bfb3..4afbc3f78 100644 --- a/test/helpers/devex-count-fixture.ts +++ b/test/helpers/devex-count-fixture.ts @@ -247,7 +247,20 @@ function answeredContractRepair(fp: AskUserQuestionFingerprint): boolean { q.options.filter(o => o.label === call.answers?.[q.question]).length !== 1 || administrativeQuestion(q.header, q.question, q.options)) return false; const ids = [...q.question.matchAll(/<gstack-qid:([^>]+)>/gi)]; - if (ids.length !== 1 || (q.question.match(/<gstack-qid/gi)?.length ?? 0) !== 1 || + if (ids.length !== 1 || (q.question.match(/<gstack-qid/gi)?.length ?? 0) !== 1) return false; + if (ids[0]![1] === 'devex-demo-ci-bypass') { + // A demo is a first result too. Require an affirmative measured timing + // contradiction and a direct bypass decision, not benchmark confirmation. + if (call.failed !== false || !/^Demo CI gate$/i.test(q.header.trim()) || + /(?:^|\n)[ \t]*(?:>|`{3}|~{3}|example:)/im.test(q.question)) return false; + const headline = /^D\s*\d+\s*[—–:-]\s*[a-z][a-z0-9 -]{0,60} demo command: should it bypass the mandatory CI check to reach the <(\d+(?:\.\d+)?) min TTHW target\?$/i.exec(q.question.split('\n')[0]!.trim()); + const timing = /^ELI10:\s*The agreed onboarding target is under (\d+(?:\.\d+)?) minutes(?: \([^\n)]+\))?\.\s+Today `[^`\n]+` blocks for (\d+(?:\.\d+)?) minutes waiting for a CI check, giving a measured TTHW of (\d+(?:\.\d+)?) minutes(?: [—–-] Red Flag tier vs\. Competitor [A-Z]['’]s \d+(?:\.\d+)? minutes)?\.(?:\s|$)/im.exec(q.question); + if (!headline || !timing) return false; + const [target, wait, measured] = timing.slice(1).map(Number); + return [target, wait, measured].every(n => Number.isFinite(n) && n! > 0) && + Number(headline[1]) === target && wait! >= target! && measured! >= wait!; + } + if ( !/^plan-devex-(?:review-)?[a-z0-9-]+$/i.test(ids[0]![1]!) || /(?:^|-)(?:mode|setup|scope|routing|prerequisite|next-steps?)(?:-|$)/i.test(ids[0]![1]!)) return false; const body = q.question.replace(/<gstack-qid:[^>]+>/i, '').trim().replace(/\s+/g, ' '); @@ -258,7 +271,15 @@ function answeredContractRepair(fp: AskUserQuestionFingerprint): boolean { const conflictingGate = /^(?:The )?plan targets TTHW\b[^.!?]*\bbut retains a mandatory\b[^.!?]*\bCI gate with no skip path\b/i.test(statement) && /\b(?:these are mutually exclusive|these contradict each other)\b/i.test(body) && /\bhow should (?:the plan|we) resolve (?:this|it)\?$/i.test(body); - return absentPackageFile || conflictingGate; + // A first-run decision may describe shipment, or compare the measured gate + // directly with the benchmark. Require the complete affirmative claim and + // its repair question; setup/quoted/negated recaps still fail above/below. + const completedNative = call.answered === true && call.failed === false; + const unshippedQuickstart = completedNative && + /^(?:The )?(?:README )?quickstart points to a file that doesn['’]t ship in the (?:published )?package\. Should we fix the quickstart path in the plan\?$/i.test(statement); + const unreachableBenchmark = completedNative && + /^(?:The )?benchmarks set an? <\d+(?:\.\d+)? min TTHW target, but the mandatory \d+(?:\.\d+)?[- ]minute CI gate makes that unreachable\. The plan retains the gate\. How should this plan handle the contradiction\?$/i.test(statement); + return absentPackageFile || conflictingGate || unshippedQuickstart || unreachableBenchmark; } /** A batched native call remains one decision; the caller owns call-ID deduplication. */ diff --git a/test/helpers/plan-count-transcript.ts b/test/helpers/plan-count-transcript.ts index ec8d756c6..91ed140d5 100644 --- a/test/helpers/plan-count-transcript.ts +++ b/test/helpers/plan-count-transcript.ts @@ -106,7 +106,7 @@ export function readPlanCountTranscript(configDir: string, cwd: string): PlanCou throw new Error('conflicting question metadata for one tool call'); } if (!prior) calls.set(key, { sessionId: record.sessionId, toolUseId: block.id, - questions: block.input.questions, answered: false }); + questions: block.input.questions, answered: false, failed: false }); } else if (record.message.role === 'user' && block.type === 'tool_result' && typeof block.tool_use_id === 'string') { const ready = planReadyRequests.get(`${record.sessionId}:${block.tool_use_id}`); diff --git a/test/helpers/touchfiles-data.ts b/test/helpers/touchfiles-data.ts index c2c54eb4a..67a7b2fdd 100644 --- a/test/helpers/touchfiles-data.ts +++ b/test/helpers/touchfiles-data.ts @@ -139,7 +139,7 @@ export const E2E_TOUCHFILES: Record<string, string[]> = { // Each one tests behavior the SDK harness can't observe (rendered TTY, // numbered-option lists, multi-phase ordering, idempotency state echo). 'auq-format-gate': ['plan-ceo-review/**', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completeness-section.ts', 'scripts/resolvers/preamble.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/helpers/llm-judge.ts', 'test/skill-e2e-ask-user-question-format-compliance.test.ts'], - 'plan-ceo-mode-routing': ['plan-ceo-review/**', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble.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/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/ceo-mode-option.ts', 'test/ceo-mode-option.test.ts', 'test/pty-option-selection.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/skill-e2e-plan-ceo-mode-routing.test.ts', 'test/ceo-mode-posture-native.test.ts', 'test/fixtures/ceo-hold-posture-l.json', 'test/ceo-mode-labels-native.test.ts', 'test/fixtures/ceo-mode-labels-l.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/ceo-mode-prerequisite.test.ts', 'test/fixtures/ceo-mode-prerequisite-o-calls.json', 'test/fixtures/ceo-mode-prerequisite-q-call.json'], + 'plan-ceo-mode-routing': ['plan-ceo-review/**', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble.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/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/ceo-mode-option.ts', 'test/ceo-mode-option.test.ts', 'test/pty-option-selection.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/skill-e2e-plan-ceo-mode-routing.test.ts', 'test/ceo-mode-posture-native.test.ts', 'test/fixtures/ceo-hold-posture-l.json', 'test/ceo-mode-labels-native.test.ts', 'test/fixtures/ceo-mode-labels-l.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/ceo-mode-prerequisite.test.ts', 'test/fixtures/ceo-mode-prerequisite-o-calls.json', 'test/fixtures/ceo-mode-prerequisite-q-call.json', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-screen.txt', 'test/ceo-posture-packet.test.ts'], 'plan-design-with-ui-scope': ['plan-design-review/**', 'test/fixtures/plans/ui-heavy-feature.md', 'test/helpers/claude-pty-runner.ts', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/skill-e2e-plan-design-with-ui.test.ts'], 'ship-idempotency-pty': ['ship/**', 'bin/gstack-next-version', 'bin/gstack-version-bump', 'scripts/resolvers/sections.ts', 'lib/worktree.ts', 'test/helpers/claude-pty-runner.ts', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/skill-e2e-ship-idempotency.test.ts'], 'tpa-present': ['scripts/resolvers/third-party-actions.ts', 'ship/SKILL.md.tmpl', 'ship/sections/apple-release.md.tmpl', 'scripts/gen-skill-docs.ts', 'test/helpers/session-runner.ts', 'test/skill-e2e-third-party-actions.test.ts'], @@ -148,22 +148,22 @@ export const E2E_TOUCHFILES: Record<string, string[]> = { 'tpa-absent-darwin': ['scripts/resolvers/third-party-actions.ts', 'ship/SKILL.md.tmpl', 'ship/sections/apple-release.md.tmpl', 'scripts/gen-skill-docs.ts', 'test/helpers/session-runner.ts', 'test/skill-e2e-third-party-actions.test.ts'], 'tpa-apple-ban': ['scripts/resolvers/third-party-actions.ts', 'ship/SKILL.md.tmpl', 'ship/sections/apple-release.md.tmpl', 'scripts/gen-skill-docs.ts', 'test/helpers/session-runner.ts', 'test/skill-e2e-third-party-actions.test.ts'], 'ship-section-loading': ['ship/**', 'scripts/resolvers/sections.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/session-runner-tools.test.ts', 'test/skill-e2e-ship-section-loading.test.ts'], - 'plan-ceo-section-loading': ['plan-ceo-review/**', 'test/skill-ceo-section-ordering.test.ts', 'scripts/resolvers/sections.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/session-runner-tools.test.ts', 'test/helpers/ceo-section-loading-fixture.ts', 'test/ceo-section-loading-fixture.test.ts', 'test/skill-e2e-plan-ceo-review-section-loading.test.ts', 'test/fixtures/ceo-section-loading-l-report.md', 'test/fixtures/ceo-section-loading-q-report.md', 'test/fixtures/ceo-section-r-rejected-report.md', 'test/fixtures/ceo-section-s-trace-report.md'], + 'plan-ceo-section-loading': ['plan-ceo-review/**', 'test/skill-ceo-section-ordering.test.ts', 'scripts/resolvers/sections.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/session-runner-tools.test.ts', 'test/helpers/ceo-section-loading-fixture.ts', 'test/ceo-section-loading-fixture.test.ts', 'test/skill-e2e-plan-ceo-review-section-loading.test.ts', 'test/fixtures/ceo-section-loading-l-report.md', 'test/fixtures/ceo-section-loading-q-report.md', 'test/fixtures/ceo-section-r-rejected-report.md', 'test/fixtures/ceo-section-s-trace-report.md', 'test/fixtures/ceo-section-u-report.md'], // Data-driven behavioral guard for the 'plan'/'prompt' carves (eng, design, // devex, office-hours + future PR2 carves). One file iterating CARVE_GUARDS; // the selector sets GSTACK_CARVE_SKILL=<name> to scope cost to the changed // skill (D-CODEX A). Touching the registry/helper or sections.ts runs all. - 'carve-section-loading': ['scripts/resolvers/composition.ts', 'test/autoplan-review-discovery.test.ts', 'test/autoplan-phase-order.test.ts', 'design-html/**', 'design-shotgun/**', 'qa/**', 'browse/**', 'retro/**', 'autoplan/**', 'spec/**', 'setup-gbrain/**', 'review/**', 'codex/**', 'land-and-deploy/**', 'plan-eng-review/**', 'plan-design-review/**', 'plan-devex-review/**', 'office-hours/**', 'document-release/**', 'design-consultation/**', 'cso/**', 'test/helpers/carve-guards.ts', 'scripts/resolvers/sections.ts', 'scripts/resolvers/redact-doc.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/session-runner-tools.test.ts', 'test/carve-section-loading.test.ts', 'bin/gstack-autoplan-snapshot.ts', 'test/autoplan-snapshot.test.ts', 'test/autoplan-init.test.ts', 'test/autoplan-obligations.test.ts', 'test/fixtures/autoplan/t-ceo-omitted-obligations.json'], - 'autoplan-chain-pty': ['scripts/resolvers/composition.ts', 'test/autoplan-review-discovery.test.ts', 'test/autoplan-phase-order.test.ts', 'autoplan/**', 'plan-ceo-review/**', 'plan-design-review/**', 'plan-eng-review/**', 'plan-devex-review/**', 'test/fixtures/plans/autoplan-dashboard.md', 'test/autoplan-chain-fixture.test.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'bin/gstack-config', '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/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/autoplan-setup-question.ts', 'test/autoplan-setup-question.test.ts', 'test/helpers/autoplan-phase-observer.ts', 'test/autoplan-phase-observer.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/skill-e2e-autoplan-chain.test.ts', 'bin/gstack-autoplan-snapshot.ts', 'test/autoplan-snapshot.test.ts', 'test/autoplan-init.test.ts', 'test/autoplan-obligations.test.ts', 'test/fixtures/autoplan/t-ceo-omitted-obligations.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/helpers/eval-budgets.ts', 'test/autoplan-eval-budget.test.ts', 'test/eval-budgets-policy.test.ts', 'test/eval-detach-timeout-floor.test.ts', 'scripts/test-paid-shards.ts', '.github/workflows/evals-periodic.yml', 'test/fixtures/autoplan-routing-n-screen.txt', 'test/autoplan-routing-o.test.ts', 'test/fixtures/autoplan-routing-o-screen.txt', 'test/autoplan-setup-packet-o.test.ts', 'test/fixtures/autoplan-setup-packet-o-screen.txt', 'test/fixtures/autoplan-setup-packet-o-call.json'], + 'carve-section-loading': ['scripts/resolvers/composition.ts', 'test/autoplan-review-discovery.test.ts', 'test/autoplan-phase-order.test.ts', 'design-html/**', 'design-shotgun/**', 'qa/**', 'browse/**', 'retro/**', 'autoplan/**', 'spec/**', 'setup-gbrain/**', 'review/**', 'codex/**', 'land-and-deploy/**', 'plan-eng-review/**', 'plan-design-review/**', 'plan-devex-review/**', 'office-hours/**', 'document-release/**', 'design-consultation/**', 'cso/**', 'test/helpers/carve-guards.ts', 'scripts/resolvers/sections.ts', 'scripts/resolvers/redact-doc.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/session-runner-tools.test.ts', 'test/carve-section-loading.test.ts', 'bin/gstack-autoplan-snapshot.ts', 'test/autoplan-snapshot.test.ts', 'test/autoplan-init.test.ts', 'test/autoplan-obligations.test.ts', 'test/fixtures/autoplan/t-ceo-omitted-obligations.json', 'test/fixtures/autoplan/u-ceo-original-loss.json'], + 'autoplan-chain-pty': ['scripts/resolvers/composition.ts', 'test/autoplan-review-discovery.test.ts', 'test/autoplan-phase-order.test.ts', 'autoplan/**', 'plan-ceo-review/**', 'plan-design-review/**', 'plan-eng-review/**', 'plan-devex-review/**', 'test/fixtures/plans/autoplan-dashboard.md', 'test/autoplan-chain-fixture.test.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'bin/gstack-config', '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/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/autoplan-setup-question.ts', 'test/autoplan-setup-question.test.ts', 'test/helpers/autoplan-phase-observer.ts', 'test/autoplan-phase-observer.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/skill-e2e-autoplan-chain.test.ts', 'bin/gstack-autoplan-snapshot.ts', 'test/autoplan-snapshot.test.ts', 'test/autoplan-init.test.ts', 'test/autoplan-obligations.test.ts', 'test/fixtures/autoplan/t-ceo-omitted-obligations.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/helpers/eval-budgets.ts', 'test/autoplan-eval-budget.test.ts', 'test/eval-budgets-policy.test.ts', 'test/eval-detach-timeout-floor.test.ts', 'scripts/test-paid-shards.ts', '.github/workflows/evals-periodic.yml', 'test/fixtures/autoplan-routing-n-screen.txt', 'test/autoplan-routing-o.test.ts', 'test/fixtures/autoplan-routing-o-screen.txt', 'test/autoplan-setup-packet-o.test.ts', 'test/fixtures/autoplan-setup-packet-o-screen.txt', 'test/fixtures/autoplan-setup-packet-o-call.json', 'test/fixtures/autoplan/u-ceo-original-loss.json'], // Per-finding AskUserQuestion count + review-report-at-bottom assertion. // 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/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/ceo-completion-handoff.ts', 'test/ceo-completion-handoff.test.ts', 'test/ceo-count-s-terminals.test.ts', 'test/fixtures/ceo-count-s-paired.json', '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/plan-count-timeout.test.ts', 'test/plan-count-navigation-r.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', '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', 'test/fixtures/ceo-handoff-n-calls.json', 'test/ceo-completion-handoff-o.test.ts', 'test/fixtures/ceo-completion-handoff-o-call.json', 'test/plan-count-empty-review.test.ts', 'test/fixtures/ceo-count-s-distinct.json', 'test/fixtures/plan-count-design-questionless-report.md', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json', 'test/helpers/ceo-approach-pick.ts', 'test/ceo-approach-pick.test.ts', 'test/fixtures/ceo-approach-q-call.json', 'test/fixtures/ceo-approach-r-call.json', 'test/fixtures/ceo-approach-r-distinct-call.json', 'test/fixtures/ceo-approach-q-paired-call.json', 'test/fixtures/ceo-completion-handoff-q-call.json', 'test/fixtures/ceo-completion-handoff-r-calls.json', 'test/fixtures/ceo-completion-handoff-t-call.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json'], - '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/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.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/plan-count-timeout.test.ts', 'test/plan-count-navigation-r.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', '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', 'test/plan-count-empty-review.test.ts', 'test/fixtures/ceo-count-s-distinct.json', 'test/fixtures/plan-count-design-questionless-report.md', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json', 'test/eng-devex-s-count.test.ts', 'test/fixtures/eng-devex-s-first-calls.json', 'test/fixtures/eng-devex-s-retry-calls.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/eng-first-review-t.test.ts', 'test/fixtures/eng-batching-t-calls.json'], - '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/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.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/plan-count-timeout.test.ts', 'test/plan-count-navigation-r.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', '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', 'test/fixtures/design-review-n-calls.json', 'test/design-completion-handoff-scored.test.ts', 'test/fixtures/design-handoff-n-calls.json', 'test/fixtures/design-handoff-q-calls.json', 'test/plan-count-empty-review.test.ts', 'test/fixtures/ceo-count-s-distinct.json', 'test/fixtures/plan-count-design-questionless-report.md', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json'], - '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/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.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/plan-count-timeout.test.ts', 'test/plan-count-navigation-r.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/helpers/devex-count-fixture.ts', 'test/devex-count-fixture.test.ts', 'test/skill-e2e-plan-devex-finding-count.test.ts', 'test/fixtures/dx-prerequisite-r-call.json', 'test/fixtures/devex-review-l-calls.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/fixtures/devex-review-n-calls.json', 'test/plan-count-empty-review.test.ts', 'test/fixtures/ceo-count-s-distinct.json', 'test/fixtures/plan-count-design-questionless-report.md', 'test/devex-output-o.test.ts', 'test/fixtures/devex-review-o-calls.json', 'test/fixtures/devex-output-o-retry-call.json', 'test/devex-setup-remedy-o.test.ts', 'test/fixtures/devex-review-o-retry-calls.json', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json', 'test/eng-devex-s-count.test.ts', 'test/fixtures/eng-devex-s-first-calls.json', 'test/fixtures/eng-devex-s-retry-calls.json', 'test/fixtures/devex-review-t-calls.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json'], + '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/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/ceo-completion-handoff.ts', 'test/ceo-completion-handoff.test.ts', 'test/ceo-count-s-terminals.test.ts', 'test/fixtures/ceo-count-s-paired.json', '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/plan-count-timeout.test.ts', 'test/plan-count-navigation-r.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', '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', 'test/fixtures/ceo-handoff-n-calls.json', 'test/ceo-completion-handoff-o.test.ts', 'test/fixtures/ceo-completion-handoff-o-call.json', 'test/plan-count-empty-review.test.ts', 'test/fixtures/ceo-count-s-distinct.json', 'test/fixtures/plan-count-design-questionless-report.md', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json', 'test/helpers/ceo-approach-pick.ts', 'test/ceo-approach-pick.test.ts', 'test/fixtures/ceo-approach-q-call.json', 'test/fixtures/ceo-approach-r-call.json', 'test/fixtures/ceo-approach-r-distinct-call.json', 'test/fixtures/ceo-approach-q-paired-call.json', 'test/fixtures/ceo-completion-handoff-q-call.json', 'test/fixtures/ceo-completion-handoff-r-calls.json', 'test/fixtures/ceo-completion-handoff-t-call.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-screen.txt', 'test/fixtures/ceo-completion-handoff-u-call.json'], + '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/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.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/plan-count-timeout.test.ts', 'test/plan-count-navigation-r.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', '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', 'test/plan-count-empty-review.test.ts', 'test/fixtures/ceo-count-s-distinct.json', 'test/fixtures/plan-count-design-questionless-report.md', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json', 'test/eng-devex-s-count.test.ts', 'test/fixtures/eng-devex-s-first-calls.json', 'test/fixtures/eng-devex-s-retry-calls.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/eng-first-review-t.test.ts', 'test/fixtures/eng-batching-t-calls.json', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-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/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.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/plan-count-timeout.test.ts', 'test/plan-count-navigation-r.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', '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', 'test/fixtures/design-review-n-calls.json', 'test/design-completion-handoff-scored.test.ts', 'test/fixtures/design-handoff-n-calls.json', 'test/fixtures/design-handoff-q-calls.json', 'test/plan-count-empty-review.test.ts', 'test/fixtures/ceo-count-s-distinct.json', 'test/fixtures/plan-count-design-questionless-report.md', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-screen.txt', 'test/design-completion-handoff-u.test.ts', 'test/fixtures/design-handoff-u-calls.json'], + '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/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.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/plan-count-timeout.test.ts', 'test/plan-count-navigation-r.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/helpers/devex-count-fixture.ts', 'test/devex-count-fixture.test.ts', 'test/skill-e2e-plan-devex-finding-count.test.ts', 'test/fixtures/dx-prerequisite-r-call.json', 'test/fixtures/devex-review-l-calls.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/fixtures/devex-review-n-calls.json', 'test/plan-count-empty-review.test.ts', 'test/fixtures/ceo-count-s-distinct.json', 'test/fixtures/plan-count-design-questionless-report.md', 'test/devex-output-o.test.ts', 'test/fixtures/devex-review-o-calls.json', 'test/fixtures/devex-output-o-retry-call.json', 'test/devex-setup-remedy-o.test.ts', 'test/fixtures/devex-review-o-retry-calls.json', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json', 'test/eng-devex-s-count.test.ts', 'test/fixtures/eng-devex-s-first-calls.json', 'test/fixtures/eng-devex-s-retry-calls.json', 'test/fixtures/devex-review-t-calls.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-screen.txt', 'test/fixtures/devex-count-u-calls.json', 'test/fixtures/devex-count-u-retry-calls.json'], // Gate-tier reviewCount-floor counterparts. Catch the May 2026 transcript // bug (model wrote a plan-mode plan and ExitPlanMode'd without firing any @@ -180,7 +180,7 @@ export const E2E_TOUCHFILES: Record<string, string[]> = { // a model fires one AUQ then batches the rest into a "## Decisions to // confirm" plan write. runPlanSkillFloorCheck cannot detect that shape // (it exits on first AUQ); runPlanSkillCounting can. - 'plan-eng-multi-finding-batching': ['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', 'scripts/resolvers/review.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/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/plan-count-timeout.test.ts', 'test/plan-count-navigation-r.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/fixtures/forcing-finding-seeds.ts', 'test/skill-e2e-plan-eng-multi-finding-batching.test.ts', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/eng-devex-s-count.test.ts', 'test/fixtures/eng-devex-s-first-calls.json', 'test/fixtures/eng-devex-s-retry-calls.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/eng-first-review-t.test.ts', 'test/fixtures/eng-batching-t-calls.json'], + 'plan-eng-multi-finding-batching': ['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', 'scripts/resolvers/review.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/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/plan-count-timeout.test.ts', 'test/plan-count-navigation-r.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/fixtures/forcing-finding-seeds.ts', 'test/skill-e2e-plan-eng-multi-finding-batching.test.ts', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/eng-devex-s-count.test.ts', 'test/fixtures/eng-devex-s-first-calls.json', 'test/fixtures/eng-devex-s-retry-calls.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/eng-first-review-t.test.ts', 'test/fixtures/eng-batching-t-calls.json', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-screen.txt'], 'plan-ceo-split-overflow': ['bin/gstack-config', 'plan-ceo-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'bin/gstack-question-preference', '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/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/plan-count-timeout.test.ts', 'test/plan-count-navigation-r.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/fixtures/forcing-finding-seeds.ts', 'test/skill-e2e-plan-ceo-split-overflow.test.ts', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json'], 'brain-privacy-gate': ['bin/gstack-skill-start', 'bin/gstack-skill-end', 'scripts/resolvers/preamble/generate-brain-sync-block.ts', 'scripts/resolvers/preamble.ts', 'bin/gstack-brain-sync', 'bin/gstack-artifacts-init', 'bin/gstack-config', 'test/helpers/agent-sdk-runner.ts', 'test/skill-e2e-brain-privacy-gate.test.ts'], @@ -365,7 +365,7 @@ export const E2E_TOUCHFILES: Record<string, string[]> = { // Autoplan - 'autoplan-dual-voice': ['scripts/resolvers/composition.ts', 'test/autoplan-review-discovery.test.ts', 'test/autoplan-phase-order.test.ts', 'autoplan/**', 'codex/**', 'bin/gstack-codex-probe', 'scripts/resolvers/review.ts', 'scripts/resolvers/design.ts', 'test/skill-e2e-autoplan-dual-voice.test.ts', 'bin/gstack-autoplan-snapshot.ts', 'test/autoplan-snapshot.test.ts', 'test/autoplan-init.test.ts', 'test/autoplan-obligations.test.ts', 'test/fixtures/autoplan/t-ceo-omitted-obligations.json'], + 'autoplan-dual-voice': ['scripts/resolvers/composition.ts', 'test/autoplan-review-discovery.test.ts', 'test/autoplan-phase-order.test.ts', 'autoplan/**', 'codex/**', 'bin/gstack-codex-probe', 'scripts/resolvers/review.ts', 'scripts/resolvers/design.ts', 'test/skill-e2e-autoplan-dual-voice.test.ts', 'bin/gstack-autoplan-snapshot.ts', 'test/autoplan-snapshot.test.ts', 'test/autoplan-init.test.ts', 'test/autoplan-obligations.test.ts', 'test/fixtures/autoplan/t-ceo-omitted-obligations.json', 'test/fixtures/autoplan/u-ceo-original-loss.json'], // Multi-provider benchmark adapters — live API smoke against real claude/codex/gemini CLIs 'benchmark-providers-live': ['bin/gstack-model-benchmark', 'test/helpers/providers/**', 'test/helpers/benchmark-runner.ts', 'test/helpers/pricing.ts', 'test/skill-e2e-benchmark-providers.test.ts'], diff --git a/test/plan-count-preview-footer.test.ts b/test/plan-count-preview-footer.test.ts new file mode 100644 index 000000000..f88b164f0 --- /dev/null +++ b/test/plan-count-preview-footer.test.ts @@ -0,0 +1,121 @@ +import {describe, test, expect} from 'bun:test'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import {pathToFileURL} from 'node:url'; +import {capturePlanCountQuestion, matchesNativePlanQuestion, nativePlanCallFingerprint, planCountQuestionInput} from './helpers/claude-pty-runner'; +import {pickCeoCountQuestion} from './helpers/ceo-approach-pick'; +import type {NativePlanQuestionCall} from './helpers/plan-count-transcript'; +import completed from './fixtures/ceo-preview-u-call.json'; + +const screen = fs.readFileSync(path.join(import.meta.dir, 'fixtures/ceo-preview-u-screen.txt'), 'utf8'); +// Project an unanswered call for regression execution. The actual U capture +// completed with option A; its 621ms pending state was not retained by observers. +function pending(): NativePlanQuestionCall { + const {answers, answeredAt, unansweredQuestionIndices, ...call} = structuredClone(completed); + return {...call, answered:false, failed:false}; +} + +describe('native question with preview and notes footer', () => { + test('the captured panel binds complete native labels and selects the offered recommendation', () => { + const call = pending(); + expect(matchesNativePlanQuestion(screen, call)).toBe(true); + const fp = capturePlanCountQuestion(screen, new Set(), 0, true, call)!; + expect(fp.nativeCall).toBe(call); + expect(fp.options.map(o => o.label)).toEqual(call.questions[0]!.options.map(o => o.label)); + expect(pickCeoCountQuestion(nativePlanCallFingerprint(call, 0, true), fp)).toBe(2); + expect(planCountQuestionInput(screen, fp, 2)).toBe('2'); + expect(completed.answers[completed.questions[0]!.question]).toBe('A) Current plan as-is'); + }); + + test('late native metadata changes neither input protocol nor historical coverage', () => { + const seen = new Set<string>(); + const fp = capturePlanCountQuestion(screen, seen, 0, true)!; + expect(fp.nativeCall).toBeUndefined(); + expect(pickCeoCountQuestion(fp)).toBeNull(); + expect(planCountQuestionInput(screen, fp, 2)).toBe('2'); + expect(capturePlanCountQuestion(screen, seen, 1, true, pending())).toBeNull(); + }); + + test('a notes footer cannot bind another question or authorize a different action', () => { + for (const different of [ + screen.replace('☐ Approach', '☐ Other'), + screen.replace('<gstack-qid:plan-ceo-approach-selection>', '<gstack-qid:foreign>'), + screen.replace('navigate · n to add notes', 'navigate · n to run a command'), + screen.replace('n to add notes · ', 'n to add notes · n to add notes · '), + screen.replace(' · Esc to cancel', ''), + ]) { + const call = pending(); + expect(matchesNativePlanQuestion(different, call)).toBe(false); + const fp = capturePlanCountQuestion(different, new Set(), 0, true, call)!; + expect(fp.nativeCall).toBeUndefined(); + expect(pickCeoCountQuestion(nativePlanCallFingerprint(call, 0, true), fp)).toBeNull(); + } + expect(matchesNativePlanQuestion(screen.replace(' · n to add notes', ''), pending())).toBe(true); + }); +}); + +test.skipIf(process.platform === 'win32')('real fake CLI chooses B from a native preview menu and sends no trailing Enter', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'count-preview-')); + const fake = path.join(dir, 'fake-claude'); + const worker = path.join(dir, 'worker.ts'); + const events = path.join(dir, 'events.jsonl'); + const output = path.join(dir, 'output.json'); + fs.writeFileSync(fake, `#!${process.execPath}\n` + String.raw` +import fs from 'node:fs';import path from 'node:path'; +const item=JSON.parse(process.env.PREVIEW_CASE);const log=e=>fs.appendFileSync(item.events,JSON.stringify(e)+'\n'); +const sid='preview-'+process.pid;const nativePath=path.join(process.env.CLAUDE_CONFIG_DIR,'projects',sid,sid+'.jsonl');fs.mkdirSync(path.dirname(nativePath),{recursive:true}); +const native=(role,content,extra={})=>fs.appendFileSync(nativePath,JSON.stringify({cwd:process.cwd(),sessionId:sid,isSidechain:false,timestamp:new Date().toISOString(),message:{role,content},...extra})+'\n'); +native('assistant',[{type:'text',text:'Fixture started.'}]);log({type:'start',pid:process.pid,cwd:process.cwd()}); +let stage='startup'; +process.stdin.setRawMode?.(true);process.stdin.on('data',data=>{ + const input=data.toString();log({type:'input',stage,input}); + if(stage==='startup'){ + stage='question';native('assistant',[{type:'tool_use',name:'AskUserQuestion',id:'preview',input:{questions:[item.question]}}]); + process.stdout.write('\x1b[2J\x1b[H'+item.screen.replaceAll('\n','\r\n'));return; + } + if(stage!=='question'){log({type:'unexpected',input});return;} + stage='done'; + const choice=item.question.options[Number(input[0])-1]?.label; + native('user',[{type:'tool_result',tool_use_id:'preview',content:'Answered'}],{toolUseResult:{answers:{[item.question.question]:choice}}}); + log({type:'choice',choice,input}); + const q={header:'Finding',question:'Apply the repair?',options:[{label:'Fix'},{label:'Keep'}]}; + native('assistant',[{type:'tool_use',name:'AskUserQuestion',id:'finding',input:{questions:[q]}}]); + native('user',[{type:'tool_result',tool_use_id:'finding',content:'Answered'}],{toolUseResult:{answers:{[q.question]:'Fix'}}}); + process.stdout.write('\x1b[2J\x1b[HDone.\r\n'); +});process.on('SIGINT',()=>process.exit(0));process.stdin.resume(); +`); + fs.chmodSync(fake, 0o755); + const runner=pathToFileURL(path.join(import.meta.dir,'helpers/claude-pty-runner.ts')).href; + const picker=pathToFileURL(path.join(import.meta.dir,'helpers/ceo-approach-pick.ts')).href; + fs.writeFileSync(worker, `import {runPlanSkillCounting} from ${JSON.stringify(runner)};import {pickCeoCountQuestion} from ${JSON.stringify(picker)}; +const result=await runPlanSkillCounting({skillName:'plan-ceo-review',slashCommand:'/plan-ceo-review',followUpPrompt:'Review this fixture.',isLastStep0AUQ:()=>true,pickAUQ:pickCeoCountQuestion,reviewCountCeiling:1,timeoutMs:26000,env:{PREVIEW_CASE:${JSON.stringify(JSON.stringify({events, screen, question:completed.questions[0]}))}}});await Bun.write(${JSON.stringify(output)},JSON.stringify(result));`); + const child=Bun.spawn([process.execPath,worker], {env:{...process.env,BROWSE_TERMINAL_BINARY:fake,EVALS_HERMETIC:'1'},stdout:'pipe',stderr:'pipe'}); + const timer=setTimeout(()=>child.kill('SIGKILL'),30000); + try { + const [code,out,err]=await Promise.all([child.exited,new Response(child.stdout).text(),new Response(child.stderr).text()]); + expect(code,out+err).toBe(0); + const result=JSON.parse(fs.readFileSync(output,'utf8')); + expect(result.outcome,JSON.stringify(result)).toBe('ceiling_reached'); + expect(result.step0Count).toBe(1); + expect(result.reviewCount).toBe(1); + const rows=fs.readFileSync(events,'utf8').trim().split('\n').map(line=>JSON.parse(line)); + expect(rows.filter(row=>row.type==='input').map(row=>row.input)).toEqual(['/plan-ceo-review\r','2']); + expect(rows.find(row=>row.type==='choice').choice).toBe(completed.questions[0]!.options[1]!.label); + expect(rows.some(row=>row.type==='unexpected')).toBe(false); + expect(()=>process.kill(rows[0].pid,0)).toThrow(); + expect(fs.existsSync(rows[0].cwd)).toBe(false); + } finally { + clearTimeout(timer);child.kill('SIGKILL');await child.exited; + if(fs.existsSync(events)) { + const first=JSON.parse(fs.readFileSync(events,'utf8').split('\n')[0]!); + try { + const argv = process.platform === 'linux' + ? fs.readFileSync('/proc/' + first.pid + '/cmdline', 'utf8').split('\0') + : Bun.spawnSync(['ps', '-p', String(first.pid), '-o', 'command='], { timeout: 1_000 }).stdout.toString().trim().split(/\s+/); + if (argv.includes(fake)) process.kill(first.pid, 'SIGKILL'); + } catch { /* owned child already closed */ } + } + fs.rmSync(dir,{recursive:true,force:true}); + } +},35000); diff --git a/test/skill-e2e-plan-ceo-mode-routing.test.ts b/test/skill-e2e-plan-ceo-mode-routing.test.ts index 293a2cf48..05be63b97 100644 --- a/test/skill-e2e-plan-ceo-mode-routing.test.ts +++ b/test/skill-e2e-plan-ceo-mode-routing.test.ts @@ -209,6 +209,7 @@ describeE2E('/plan-ceo-review mode routing (gate)', () => { if (continuation !== null) { if (continuation === 'question') continuedQuestion = true; if (continuation === 'permission') await selectPtyNumberedOption(session, 1); + else if (continuation === 'submission') session.send('\r'); else { const pending = transcript.calls.find(call => !call.answered && !call.failed); const question = capturePlanCountQuestion(currentInput, new Set(), 0, false, pending)!;