diff --git a/autoplan/sections/ceo-phase.md b/autoplan/sections/ceo-phase.md index 1f18dd59b..a4a433364 100644 --- a/autoplan/sections/ceo-phase.md +++ b/autoplan/sections/ceo-phase.md @@ -1,6 +1,6 @@ -Before dispatch, fully Read `methodologyPath` from `bun "" methodology ceo "" ""`; log successful start/end/total; fill gaps to EOF. Skip-listed: load only. +Before dispatch, Read `methodologyPath` from `bun "" methodology ceo "" ""`; log successful ranges/total to EOF. Skip-listed: load only. **Override rules:** - Mode selection: SELECTIVE EXPANSION @@ -20,7 +20,7 @@ Before dispatch, fully Read `methodologyPath` from `bun "" method **Bind this phase's input:** Run; use returned `snapshotPath` as `` for both voices: ```bash -bun "" create ceo "" "" +bun "" create ceo "" "" "" ``` Fresh `Implementation plan` only; excludes `Review record`. diff --git a/autoplan/sections/ceo-phase.md.tmpl b/autoplan/sections/ceo-phase.md.tmpl index f69723085..26831343c 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:with-sections}}; log successful start/end/total; fill gaps to EOF. Skip-listed: load only. +Before dispatch, Read {{AUTOPLAN_REVIEW_FILE:plan-ceo-review:with-sections}}; log successful ranges/total to EOF. Skip-listed: load only. **Override rules:** - Mode selection: SELECTIVE EXPANSION @@ -18,7 +18,7 @@ Before dispatch, fully Read {{AUTOPLAN_REVIEW_FILE:plan-ceo-review:with-sections **Bind this phase's input:** Run; use returned `snapshotPath` as `` for both voices: ```bash -bun "" create ceo "" "" +bun "" create ceo "" "" "" ``` Fresh `Implementation plan` only; excludes `Review record`. diff --git a/autoplan/sections/design-phase.md b/autoplan/sections/design-phase.md index 0e75e99f6..ef1b13d7c 100644 --- a/autoplan/sections/design-phase.md +++ b/autoplan/sections/design-phase.md @@ -1,6 +1,6 @@ -Before dispatch, fully Read `methodologyPath` from `bun "" methodology design "" ""`; log successful start/end/total; fill gaps to EOF. Skip-listed: load only. +Before dispatch, Read `methodologyPath` from `bun "" methodology design "" ""`; log successful ranges/total to EOF. Skip-listed: load only. **Override rules:** - Focus areas: all relevant dimensions (P1) @@ -11,7 +11,7 @@ Before dispatch, fully Read `methodologyPath` from `bun "" method **Bind this phase's input:** Run; use returned `snapshotPath` as `` for both voices: ```bash -bun "" create design "" "" +bun "" create design "" "" "" ``` Fresh `Implementation plan` only; excludes `Review record`. diff --git a/autoplan/sections/design-phase.md.tmpl b/autoplan/sections/design-phase.md.tmpl index c5266bcb6..1192371a0 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:with-sections}}; log successful start/end/total; fill gaps to EOF. Skip-listed: load only. +Before dispatch, Read {{AUTOPLAN_REVIEW_FILE:plan-design-review:with-sections}}; log successful ranges/total to EOF. Skip-listed: load only. **Override rules:** - Focus areas: all relevant dimensions (P1) @@ -9,7 +9,7 @@ Before dispatch, fully Read {{AUTOPLAN_REVIEW_FILE:plan-design-review:with-secti **Bind this phase's input:** Run; use returned `snapshotPath` as `` for both voices: ```bash -bun "" create design "" "" +bun "" create design "" "" "" ``` Fresh `Implementation plan` only; excludes `Review record`. diff --git a/autoplan/sections/dx-phase.md b/autoplan/sections/dx-phase.md index 1c11c3986..8de205995 100644 --- a/autoplan/sections/dx-phase.md +++ b/autoplan/sections/dx-phase.md @@ -1,6 +1,6 @@ -Before dispatch, fully Read `methodologyPath` from `bun "" methodology dx "" ""`; log successful start/end/total; fill gaps to EOF. Skip-listed: load only. +Before dispatch, Read `methodologyPath` from `bun "" methodology dx "" ""`; log successful ranges/total to EOF. Skip-listed: load only. **Override rules:** - Mode selection: DX POLISH @@ -15,7 +15,7 @@ Before dispatch, fully Read `methodologyPath` from `bun "" method **Bind this phase's input:** Run; use returned `snapshotPath` as `` for both voices: ```bash -bun "" create dx "" "" +bun "" create dx "" "" "" ``` Fresh `Implementation plan` only; excludes `Review record`. diff --git a/autoplan/sections/dx-phase.md.tmpl b/autoplan/sections/dx-phase.md.tmpl index 03183d36b..5f83a64f1 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:with-sections}}; log successful start/end/total; fill gaps to EOF. Skip-listed: load only. +Before dispatch, Read {{AUTOPLAN_REVIEW_FILE:plan-devex-review:with-sections}}; log successful ranges/total to EOF. Skip-listed: load only. **Override rules:** - Mode selection: DX POLISH @@ -13,7 +13,7 @@ Before dispatch, fully Read {{AUTOPLAN_REVIEW_FILE:plan-devex-review:with-sectio **Bind this phase's input:** Run; use returned `snapshotPath` as `` for both voices: ```bash -bun "" create dx "" "" +bun "" create dx "" "" "" ``` Fresh `Implementation plan` only; excludes `Review record`. diff --git a/autoplan/sections/eng-phase.md b/autoplan/sections/eng-phase.md index 1086265fc..7a61a2edd 100644 --- a/autoplan/sections/eng-phase.md +++ b/autoplan/sections/eng-phase.md @@ -1,6 +1,6 @@ -Before dispatch, fully Read `methodologyPath` from `bun "" methodology eng "" ""`; log successful start/end/total; fill gaps to EOF. Skip-listed: load only. +Before dispatch, Read `methodologyPath` from `bun "" methodology eng "" ""`; log successful ranges/total to EOF. Skip-listed: load only. **Override rules:** - Scope challenge: never reduce (P2) @@ -8,7 +8,7 @@ Before dispatch, fully Read `methodologyPath` from `bun "" method **Bind this phase's input:** Run; use returned `snapshotPath` as `` for both voices: ```bash -bun "" create eng "" "" +bun "" create eng "" "" "" ``` Fresh `Implementation plan` only; excludes `Review record`. diff --git a/autoplan/sections/eng-phase.md.tmpl b/autoplan/sections/eng-phase.md.tmpl index d091277b3..5dac04932 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:with-sections}}; log successful start/end/total; fill gaps to EOF. Skip-listed: load only. +Before dispatch, Read {{AUTOPLAN_REVIEW_FILE:plan-eng-review:with-sections}}; log successful ranges/total to EOF. Skip-listed: load only. **Override rules:** - Scope challenge: never reduce (P2) @@ -6,7 +6,7 @@ Before dispatch, fully Read {{AUTOPLAN_REVIEW_FILE:plan-eng-review:with-sections **Bind this phase's input:** Run; use returned `snapshotPath` as `` for both voices: ```bash -bun "" create eng "" "" +bun "" create eng "" "" "" ``` Fresh `Implementation plan` only; excludes `Review record`. diff --git a/bin/gstack-autoplan-snapshot.ts b/bin/gstack-autoplan-snapshot.ts index 20eae3e5a..c00ca74b0 100644 --- a/bin/gstack-autoplan-snapshot.ts +++ b/bin/gstack-autoplan-snapshot.ts @@ -5,7 +5,7 @@ import { linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, realpathSync import { basename, dirname, isAbsolute, join } from 'node:path'; const PHASES = ['ceo', 'design', 'dx', 'eng']; -const sha256 = (text: string) => createHash('sha256').update(text).digest('hex'); +const sha256 = (text: string | Buffer) => createHash('sha256').update(text).digest('hex'); // Exact terms from Autoplan's existing Phase 0 DX trigger. Count occurrences, // not a subjective reinterpretation of whether an API is internal or external. @@ -489,13 +489,10 @@ function phaseName(phase: string): string { return phase; } -/** One complete current-phase load target, not evidence that an agent read it. */ -export function prepareMethodology(phase: string, skillFile: string, restorePath: string) { +function methodologyContent(phase: string, skillFile: string) { phaseName(phase); const skill = `plan-${phase === 'dx' ? 'devex' : phase}-review`; if (!isAbsolute(skillFile) || basename(skillFile) !== 'SKILL.md') throw new Error('Expected an absolute installed SKILL.md path'); - const restore = realpathSync(restorePath); - if (!statSync(restore).isFile()) throw new Error('Expected the existing restore-point file'); const readPart = (file: string) => { const resolved = realpathSync(file); if (!statSync(resolved).isFile()) throw new Error('Methodology source must be a regular file'); @@ -540,10 +537,19 @@ export function prepareMethodology(phase: string, skillFile: string, restorePath startByte, endByte: startByte + part.bytes.length }; }); const content = Buffer.concat(chunks); + return { content, sources }; +} + +/** One complete current-phase load target, not evidence that an agent read it. */ +export function prepareMethodology(phase: string, skillFile: string, restorePath: string) { + const restore = realpathSync(restorePath); + if (!statSync(restore).isFile()) throw new Error('Expected the existing restore-point file'); + const { content, sources } = methodologyContent(phase, skillFile); const directory = mkdtempSync(join(dirname(restore), `autoplan-${phase}-methodology-`)); try { const methodologyPath = join(directory, 'methodology.md'); - const manifest = { phase, methodologyPath, sha256: sha256(content.toString('utf8')), bytes: content.length, + const manifest = { phase, methodologyPath, restorePath: restore, restoreSha256: sha256(readFileSync(restore)), + sha256: sha256(content.toString('utf8')), bytes: content.length, lines: content.toString('utf8').split('\n').length, sources, instruction: 'Read methodologyPath completely before phase snapshot creation or dispatch; log successful ranges through EOF. Apply the existing Autoplan skip list and overrides. This artifact supplies exact methodology, not proof of reading or execution.' }; writeFileSync(methodologyPath, content, { flag: 'wx', mode: 0o444 }); @@ -555,11 +561,46 @@ export function prepareMethodology(phase: string, skillFile: string, restorePath } } -export function createSnapshot(phase: string, activePlan: string, restorePath: string) { +/** Require preparation, never treat a supplied artifact/hash as proof of reading. */ +function requireMethodology(phase: string, restore: string, methodologyPath: string) { + if (typeof methodologyPath !== 'string' || !isAbsolute(methodologyPath) || basename(methodologyPath) !== 'methodology.md') { + throw new Error('Expected METHODOLOGY_PATH from methodology PHASE SKILL_FILE RESTORE_PATH; Read it completely before create'); + } + const directory = dirname(methodologyPath); + const manifestPath = join(directory, 'methodology.json'); + if (realpathSync(methodologyPath) !== methodologyPath || dirname(directory) !== dirname(restore) || + !basename(directory).startsWith(`autoplan-${phase}-methodology-`)) { + throw new Error('Methodology artifact does not belong to this phase and restore directory'); + } + for (const file of [methodologyPath, manifestPath]) { + const stat = lstatSync(file); + if (!stat.isFile() || (process.platform !== 'win32' && (stat.mode & 0o777) !== 0o444)) { + throw new Error('Expected immutable regular methodology files'); + } + } + const manifestBytes = readFileSync(manifestPath); + const manifest = JSON.parse(manifestBytes.toString('utf8')); + if (manifest.phase !== phase || manifest.methodologyPath !== methodologyPath || manifest.restorePath !== restore || + manifest.restoreSha256 !== sha256(readFileSync(restore)) || !Array.isArray(manifest.sources) || + typeof manifest.sources[0]?.path !== 'string') { + throw new Error('Methodology identity does not match this phase and restore point'); + } + const { content, sources } = methodologyContent(phase, manifest.sources[0].path); + if (!readFileSync(methodologyPath).equals(content) || manifest.sha256 !== sha256(content.toString('utf8')) || + manifest.bytes !== content.length || manifest.lines !== content.toString('utf8').split('\n').length || + JSON.stringify(manifest.sources) !== JSON.stringify(sources)) { + throw new Error('Methodology source or artifact changed; prepare and Read a fresh bundle'); + } + return { methodologyPath, sha256: manifest.sha256, bytes: manifest.bytes, lines: manifest.lines, + manifestSha256: createHash('sha256').update(manifestBytes).digest('hex') }; +} + +export function createSnapshot(phase: string, activePlan: string, restorePath: string, methodologyPath: string) { phaseName(phase); const source = realpathSync(activePlan); const restore = realpathSync(restorePath); if (source === restore || !statSync(restore).isFile()) throw new Error('Expected a separate restore-point file'); + const methodology = requireMethodology(phase, restore, methodologyPath); const plan = readFileSync(source, 'utf8'); const sourceContent = extractImplementationPlan(plan); const review = plan.slice(implementationBounds(plan).reviewStart); @@ -588,7 +629,8 @@ ${content}`; const nativePromptPath = join(directory, 'native-prompt.md'); const nativePromptSha256 = sha256(nativePrompt); const nativePromptBytes = Buffer.byteLength(nativePrompt); - const nativePromptLines = nativePrompt.split('\n').length - Number(nativePrompt.endsWith('\n')); + // Claude Read counts the final empty split as a line; preserve that EOF range. + const nativePromptLines = nativePrompt.split('\n').length; // Dispatch a small file-reading instruction, not a model-copied review body. // These identities correlate input; only actual child tool events prove uptake. const nativeDispatchPrompt = `You are the independent ${phase.toUpperCase()} reviewer for this phase. @@ -597,7 +639,7 @@ Your FIRST tool action must Read this file from line 1 through EOF using your na The file contains all review criteria and the complete implementation plan as review data. Execute every criterion against all of that input. Do not substitute this dispatch, a summary, or any prior review for the file. Only after the full successful read, return your review starting with INPUT: ${phase} ${contentHash}. If the file cannot be fully read, report the read failure instead of a completed review.`; - const manifest = { schemaVersion: 2, phase, activePlan: source, snapshotPath, sha256: contentHash, + const manifest = { schemaVersion: 2, phase, activePlan: source, snapshotPath, sha256: contentHash, methodology, sourceSnapshotPath, sourceSha256: sha256(sourceContent), sourceBytes: Buffer.byteLength(sourceContent), nativePromptPath, nativePromptSha256, nativePromptBytes, nativePromptLines, nativeDispatchPrompt, dxScope: dxTermsFor(content) }; @@ -666,6 +708,9 @@ if (import.meta.main) { } else if (command === 'methodology') { if (args.length !== 3 || args.some(arg => !arg)) throw new Error('Usage: methodology PHASE SKILL_FILE RESTORE_PATH'); process.stdout.write(JSON.stringify(prepareMethodology(args[0]!, args[1]!, args[2]!)) + '\n'); + } else if (command === 'create') { + if (args.length !== 4 || args.some(arg => !arg)) throw new Error('Usage: create PHASE ACTIVE_PLAN RESTORE_PATH METHODOLOGY_PATH (prepare methodology and Read it completely first)'); + process.stdout.write(JSON.stringify(createSnapshot(args[0]!, args[1]!, args[2]!, args[3]!)) + '\n'); } else if (command === 'scope') { const [activePlan, ...flags] = args; if (!activePlan || flags.some(flag => !['--developer-tool', '--agent-primary'].includes(flag)) || @@ -673,9 +718,8 @@ if (import.meta.main) { process.stdout.write(JSON.stringify(detectDxScope(activePlan, flags.includes('--developer-tool'), flags.includes('--agent-primary'))) + '\n'); } else { const [phase, active, location, expected, ...extra] = args; - if (!phase || !active || !location || extra.length || (['create', 'amend'].includes(command) && expected)) throw new Error('Usage: create PHASE ACTIVE_PLAN RESTORE_PATH | amend PHASE ACTIVE_PLAN SNAPSHOT_PATH | check PHASE ACTIVE_PLAN SNAPSHOT_PATH changed|unchanged'); - const result = command === 'create' ? createSnapshot(phase, active, location) - : command === 'amend' ? amendImplementation(phase, active, location) + if (!phase || !active || !location || extra.length || (command === 'amend' && expected)) throw new Error('Usage: amend PHASE ACTIVE_PLAN SNAPSHOT_PATH | check PHASE ACTIVE_PLAN SNAPSHOT_PATH changed|unchanged'); + const result = command === 'amend' ? amendImplementation(phase, active, location) : command === 'check' && expected ? checkPhaseImplementation(phase, active, location, expected) : (() => { throw new Error('Expected create, amend or check command'); })(); process.stdout.write(JSON.stringify(result) + '\n'); diff --git a/scripts/resolvers/composition.ts b/scripts/resolvers/composition.ts index f03846c95..5bfc52c4e 100644 --- a/scripts/resolvers/composition.ts +++ b/scripts/resolvers/composition.ts @@ -60,13 +60,13 @@ export function generateAutoplanReviewFile(ctx: TemplateContext, args?: string[] 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\``; + // Every host prepares an explicit bound artifact before create. Inline hosts + // supply one complete source file; Claude supplies main plus its carved section. + if (withSections) { const phase = skill === 'plan-devex-review' ? 'dx' : skill.split('-')[1]!; - return withSections - ? `\`methodologyPath\` from \`bun "" methodology ${phase} "" ""\`` - : entry; + return `\`methodologyPath\` from \`bun "" methodology ${phase} "" ""\``; } + if (ctx.host === 'claude') return `\`${ctx.paths.skillRoot}/${skill}/SKILL.md\``; const host = getHostConfig(ctx.host); const file = `gstack-${skill}/SKILL.md`; diff --git a/test/autoplan-init.test.ts b/test/autoplan-init.test.ts index 89bc0cc4b..8f7961829 100644 --- a/test/autoplan-init.test.ts +++ b/test/autoplan-init.test.ts @@ -4,6 +4,7 @@ import { existsSync, linkSync, mkdirSync, mkdtempSync, readFileSync, readdirSync import { tmpdir } from 'node:os'; import { join, resolve } from 'node:path'; import { spawnSync } from 'node:child_process'; +import { prepareMethodology } from '../bin/gstack-autoplan-snapshot'; const ROOT = resolve(import.meta.dir, '..'); const TOOL = join(ROOT, 'bin/gstack-autoplan-snapshot.ts'); @@ -19,6 +20,7 @@ function fixture() { return { dir, source, active, restore }; } function cli(...args: string[]) { + if (args[0] === 'create' && args.length === 4) args.push(prepareMethodology(args[1]!, join(ROOT, `plan-${args[1] === 'dx' ? 'devex' : args[1]}-review`, 'SKILL.md'), args[3]!).methodologyPath); return spawnSync(process.execPath, [TOOL, ...args], { cwd: ROOT, encoding: 'utf8', timeout: 10_000, maxBuffer: 1024 * 1024, }); diff --git a/test/autoplan-obligations.test.ts b/test/autoplan-obligations.test.ts index d8f1e7033..d50d5031b 100644 --- a/test/autoplan-obligations.test.ts +++ b/test/autoplan-obligations.test.ts @@ -4,12 +4,15 @@ import { chmodSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { spawnSync } from 'node:child_process'; -import { createSnapshot, extractImplementationPlan } from '../bin/gstack-autoplan-snapshot'; +import { createSnapshot, prepareMethodology, extractImplementationPlan } from '../bin/gstack-autoplan-snapshot'; 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 dangling = JSON.parse(readFileSync(join(import.meta.dir, 'fixtures/autoplan/v-ceo-dangling-references.json'), 'utf8')); +function methodology(phase: string, restore: string) { + return prepareMethodology(phase, join(import.meta.dir, '..', `plan-${phase === 'dx' ? 'devex' : phase}-review`, 'SKILL.md'), restore).methodologyPath; +} const owned: string[] = []; afterEach(() => { for (const dir of owned.splice(0)) rmSync(dir, { recursive: true, force: true }); }); @@ -25,7 +28,7 @@ function setup(body = 'Build the dashboard.\n') { const active = join(dir, 'plan.md'); const restore = join(dir, 'restore.md'); writeFileSync(active, `## Implementation plan\n${body}## Review record\n`); writeFileSync(restore, 'Original restore bytes\n'); - const snapshot = createSnapshot('ceo', active, restore); + const snapshot = createSnapshot('ceo', active, restore, methodology('ceo', restore)); return { dir, active, restore, snapshot }; } const block = (phase: string, body: string) => `\n${body}\n\n`; @@ -45,7 +48,7 @@ for (const command of ['amend', 'check']) { test('actual V dangling references cannot create the next blind input even if close was skipped', () => { const f = setup(dangling.initialImplementation); writeFileSync(f.active, dangling.activeAfterAmend); - expect(() => createSnapshot('design', f.active, f.restore)).toThrow('Review-record-only Section 6'); + expect(() => createSnapshot('design', f.active, f.restore, methodology('design', f.restore))).toThrow('Review-record-only Section 6'); expect(readFileSync(f.active, 'utf8')).toBe(dangling.activeAfterAmend); expect(readFileSync(f.restore, 'utf8')).toBe('Original restore bytes\n'); }); @@ -91,7 +94,7 @@ test('inline adopted tests and instrumentation instead of transporting V review- const result = invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath); expect(result.status, result.stderr).toBe(0); expect(invoke('check', 'ceo', f.active, f.snapshot.snapshotPath, 'changed').status).toBe(0); - const next = createSnapshot('design', f.active, f.restore); + const next = createSnapshot('design', f.active, f.restore, methodology('design', f.restore)); const input = readFileSync(next.snapshotPath, 'utf8'); expect(input.startsWith(dangling.initialImplementation)).toBe(true); for (const term of ['T-S6-1', 'T-S6-12', 'dashboard.page.loaded', 'api.dashboard.partial_failure_rate', 'dashboard_fetch_start', 'snapshot_time']) { @@ -123,7 +126,7 @@ test('reference checks preserve external, unresolved, ambiguous, quoted and sati appendRecord(f.active, (example.review || '### Section 6: Tests\nRun coverage.\n') + block('ceo', example.body)); const result = invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath); expect(result.status, example.body + result.stderr).toBe(0); - expect(() => createSnapshot('design', f.active, f.restore)).not.toThrow(); + expect(() => createSnapshot('design', f.active, f.restore, methodology('design', f.restore))).not.toThrow(); } }); @@ -131,10 +134,10 @@ test('new reference checks do not bind a later phase to an earlier phase review const f = setup('Existing external contract uses tests in Section 6.\n'); appendRecord(f.active, '### Section 6: CEO tests\nOriginal review.\n' + block('ceo', '- Keep all authorization checks.')); expect(invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath).status).toBe(0); - const design = createSnapshot('design', f.active, f.restore); + const design = createSnapshot('design', f.active, f.restore, methodology('design', f.restore)); appendRecord(f.active, block('design', '- Run tests in Section 6 before rollout.')); expect(invoke('amend', 'design', f.active, design.snapshotPath).status).toBe(0); - expect(() => createSnapshot('dx', f.active, f.restore)).not.toThrow(); + expect(() => createSnapshot('dx', f.active, f.restore, methodology('dx', f.restore))).not.toThrow(); }); test('actual T changed-only plan cannot close with accepted obligations only in its review', () => { @@ -167,7 +170,7 @@ test('whole recorded T obligations retain omitted guards and every nested verifi 'Screen reader: live region', 'Reduced-motion', 'session expiry mid-page-load', 'RTL test with mixed panel results']) { expect(result.implementation).toContain(detail); } - const next = createSnapshot('design', f.active, f.restore); + const next = createSnapshot('design', f.active, f.restore, methodology('design', f.restore)); expect(readFileSync(next.sourceSnapshotPath, 'utf8')).toContain(accepted); expect(readFileSync(next.snapshotPath, 'utf8')).toContain(captured.acceptedObligations.trimEnd()); expect(readFileSync(next.snapshotPath, 'utf8')).not.toContain('autoplan-accepted:'); @@ -198,7 +201,7 @@ test('unchanged U source retains every original byte when accepted requirements 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); + const next = createSnapshot('design', f.active, f.restore, methodology('design', f.restore)); expect(readFileSync(next.snapshotPath, 'utf8').startsWith(lost.initialImplementation)).toBe(true); expect(readFileSync(f.snapshot.sourceSnapshotPath, 'utf8')).toBe(lost.initialImplementation); }); @@ -219,7 +222,7 @@ test('exact replacements and deletion preserve untouched CRLF/Unicode bytes and 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); + const next = createSnapshot('design', f.active, f.restore, methodology('design', 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); @@ -264,7 +267,7 @@ test('later exact baseline revisions preserve earlier accepted blocks and reject 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 design = createSnapshot('design', f.active, f.restore, methodology('design', 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]) { @@ -278,7 +281,7 @@ test('later exact baseline revisions preserve earlier accepted blocks and reject 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); + const next = createSnapshot('dx', f.active, f.restore, methodology('dx', f.restore)); expect(readFileSync(next.snapshotPath, 'utf8')).toContain('Preserve owner authorization.'); expect(next.nativePrompt).not.toContain('sourceSha256'); }); @@ -379,7 +382,7 @@ test('phase/path/snapshot identity still rejects before any amendment', () => { test('later phases retain prior registered obligations and cannot erase them with None', () => { const f = setup(); const ceo = block('ceo', '- Handle network failure.\n Verify: offer retry.'); 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 design = createSnapshot('design', f.active, f.restore, methodology('design', f.restore)); const next = block('design', '- Show a named error control.\n Verify: keyboard reaches retry.'); appendRecord(f.active, next); expect(invoke('amend', 'design', f.active, design.snapshotPath).status).toBe(0); @@ -398,7 +401,7 @@ test('UTF-8 and CRLF requirements survive exact copying and a repeated no-change appendRecord(f.active, accepted); expect(invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath).status).toBe(0); expect(extractImplementationPlan(readFileSync(f.active, 'utf8'))).toContain(accepted); - const again = createSnapshot('ceo', f.active, f.restore); + const again = createSnapshot('ceo', f.active, f.restore, methodology('ceo', f.restore)); expect(invoke('amend', 'ceo', f.active, again.snapshotPath).status).toBe(0); expect(invoke('check', 'ceo', f.active, again.snapshotPath, 'unchanged').status).toBe(0); }); @@ -422,7 +425,7 @@ test('changing both earlier copies cannot erase the authorization obligation fro const original = block('ceo', '- Preserve owner authorization.\n Verify: reject cross-user access.'); appendRecord(f.active, original); expect(invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath).status).toBe(0); - const design = createSnapshot('design', f.active, f.restore); + const design = createSnapshot('design', f.active, f.restore, methodology('design', f.restore)); const changed = readFileSync(f.active, 'utf8').replaceAll(original, block('ceo', '- Permit cross-user access.\n Verify: cross-user access succeeds.')) + block('design', '- Label the owner control.\n Verify: accessible name.'); @@ -441,7 +444,7 @@ test('blind projection preserves UTF-8/CRLF bodies and fenced examples, while bi const body = '- Preserve café ✓ and 日本語.\r\n Verify: the last condition survives.\r\n'; appendRecord(f.active, '\r\n' + body + '\r\n'); expect(invoke('amend', 'ceo', f.active, f.snapshot.snapshotPath).status).toBe(0); - const next = createSnapshot('design', f.active, f.restore); + const next = createSnapshot('design', f.active, f.restore, methodology('design', f.restore)); const transport = readFileSync(next.snapshotPath, 'utf8'); expect(transport).toContain(example); expect(transport).toContain(body); diff --git a/test/autoplan-phase-order.test.ts b/test/autoplan-phase-order.test.ts index 88be35420..f34264f31 100644 --- a/test/autoplan-phase-order.test.ts +++ b/test/autoplan-phase-order.test.ts @@ -79,12 +79,13 @@ 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-]+:with-sections\}\}/); + expect(section).toMatch(/^Before dispatch, Read \{\{AUTOPLAN_REVIEW_FILE:plan-[a-z-]+:with-sections\}\}/); const load = section.split('**Override rules:**')[0]!; - expect(load).toContain('log successful start/end/total'); - expect(load).toContain('fill gaps to EOF'); + expect(load).toContain('log successful ranges/total'); + expect(load).toContain('to EOF'); expect(load).toContain('Skip-listed: load only'); expect(section.indexOf(':with-sections}}')).toBeLessThan(section.indexOf('create ' + phase)); + expect(section).toContain(`create ${phase} "" "" ""`); } }); diff --git a/test/autoplan-review-discovery.test.ts b/test/autoplan-review-discovery.test.ts index 1ad34157b..f395dfd21 100644 --- a/test/autoplan-review-discovery.test.ts +++ b/test/autoplan-review-discovery.test.ts @@ -40,6 +40,13 @@ function assertBundle(result: ReturnType, expected: string[] expect(actual.length).toBe(part.bytes); } if (process.platform !== 'win32') expect(fs.statSync(manifest.methodologyPath).mode & 0o777).toBe(0o444); + const active = path.join(path.dirname(manifest.restorePath), 'active.md'); + fs.writeFileSync(active, '## Implementation plan\nKeep every requirement.\n## Review record\n'); + const created = spawnSync(process.execPath, [path.join(ROOT, 'bin/gstack-autoplan-snapshot.ts'), 'create', manifest.phase, active, manifest.restorePath, manifest.methodologyPath], { + encoding: 'utf8', timeout: 10_000, + }); + expect(created.status, created.stderr).toBe(0); + expect(JSON.parse(created.stdout).methodology.sha256).toBe(manifest.sha256); return manifest; } @@ -97,10 +104,12 @@ describe('autoplan reads installed host methodology', () => { 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 ') + const directive = phaseBody.split('\n').find(line => line.startsWith('Before dispatch, Read ') && (line.includes(`methodology ${phase} `) || line.includes(`/${review}/SKILL.md`) || line.includes(`/gstack-${review}/SKILL.md`))); expect(directive).toBeDefined(); expect(phaseBody.indexOf(directive!)).toBeLessThan(phaseBody.indexOf(`create ${phase} `)); + expect(directive).toContain(`methodology ${phase} `); + expect(phaseBody).toContain(`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. diff --git a/test/autoplan-snapshot.test.ts b/test/autoplan-snapshot.test.ts index 7f90f8cde..a08b94862 100644 --- a/test/autoplan-snapshot.test.ts +++ b/test/autoplan-snapshot.test.ts @@ -4,7 +4,7 @@ import { chmodSync, copyFileSync, mkdirSync, mkdtempSync, readFileSync, realpath import { tmpdir } from 'node:os'; import { join, resolve } from 'node:path'; import { spawnSync } from 'node:child_process'; -import { checkImplementation, createSnapshot, extractImplementationPlan } from '../bin/gstack-autoplan-snapshot'; +import { checkImplementation, createSnapshot, prepareMethodology, extractImplementationPlan } from '../bin/gstack-autoplan-snapshot'; import { generateAutoplanSnapshotTool } from '../scripts/resolvers/composition'; import { HOST_PATHS, type TemplateContext } from '../scripts/resolvers/types'; import { ALL_HOST_CONFIGS } from '../hosts'; @@ -12,6 +12,9 @@ import { E2E_TOUCHFILES } from './helpers/touchfiles'; const ROOT = resolve(import.meta.dir, '..'); const TOOL = join(ROOT, 'bin/gstack-autoplan-snapshot.ts'); +function methodology(phase: string, restore: string) { + return prepareMethodology(phase, join(import.meta.dir, '..', `plan-${phase === 'dx' ? 'devex' : phase}-review`, 'SKILL.md'), restore).methodologyPath; +} const owned: string[] = []; function fixture() { const dir = mkdtempSync(join(tmpdir(), 'gstack-snapshot-test-')); owned.push(dir); @@ -23,10 +26,113 @@ function fixture() { return { dir, active, restore, body, plan }; } function cli(...args: string[]) { + if (args[0] === 'create' && args.length === 4) args.push(methodology(args[1]!, args[3]!)); return spawnSync(process.execPath, [TOOL, ...args], { encoding: 'utf8', timeout: 10_000 }); } afterEach(() => { for (const dir of owned.splice(0)) rmSync(dir, { recursive: true, force: true }); }); + +describe('methodology preparation is a required snapshot input', () => { + test('actual Y three-argument create cannot return a native dispatch for any phase', () => { + const f = fixture(); + const before = readdirSync(f.dir).sort(); + for (const phase of ['ceo', 'design', 'dx', 'eng']) { + const result = spawnSync(process.execPath, [TOOL, 'create', phase, f.active, f.restore], { + encoding: 'utf8', timeout: 10_000, + }); + expect(result.status, phase).toBe(1); + expect(result.stdout).toBe(''); + expect(result.stderr).toContain('METHODOLOGY_PATH'); + expect(() => createSnapshot(phase, f.active, f.restore, undefined as unknown as string)).toThrow('METHODOLOGY_PATH'); + } + expect(readdirSync(f.dir).sort()).toEqual(before); + expect(readFileSync(f.active, 'utf8')).toBe(f.plan); + expect(readFileSync(f.restore, 'utf8')).toBe(f.body); + }); + + test('main-only, foreign-phase and foreign-restore artifacts fail before publication', () => { + const f = fixture(); + const otherRestore = join(f.dir, 'other-restore.md'); writeFileSync(otherRestore, f.body); + const candidates = [join(ROOT, 'plan-ceo-review/SKILL.md'), methodology('design', f.restore), methodology('ceo', otherRestore)]; + for (const candidate of candidates) { + const before = readdirSync(f.dir).sort(); + expect(() => createSnapshot('ceo', f.active, f.restore, candidate)).toThrow(); + expect(readdirSync(f.dir).sort()).toEqual(before); + } + expect(readFileSync(f.active, 'utf8')).toBe(f.plan); + }); + + test('altered manifest identities and bundle bytes cannot authorize snapshot publication', () => { + for (const kind of ['phase', 'restore', 'hash', 'lines', 'source-offset', 'source-hash', 'bundle']) { + const f = fixture(); const method = methodology('ceo', f.restore); + const manifestPath = join(method, '..', 'methodology.json'); + const manifest = JSON.parse(readFileSync(manifestPath, 'utf8')); + if (kind === 'bundle') { + chmodSync(method, 0o600); writeFileSync(method, readFileSync(method, 'utf8') + '\n'); chmodSync(method, 0o444); + } else { + if (kind === 'phase') manifest.phase = 'eng'; + if (kind === 'restore') manifest.restoreSha256 = '0'.repeat(64); + if (kind === 'hash') manifest.sha256 = '0'.repeat(64); + if (kind === 'lines') manifest.lines--; + if (kind === 'source-offset') manifest.sources[0].startByte++; + if (kind === 'source-hash') manifest.sources[0].sha256 = '0'.repeat(64); + chmodSync(manifestPath, 0o600); writeFileSync(manifestPath, JSON.stringify(manifest)); chmodSync(manifestPath, 0o444); + } + const before = readdirSync(f.dir).sort(); + expect(() => createSnapshot('ceo', f.active, f.restore, method), kind).toThrow(); + expect(readdirSync(f.dir).sort()).toEqual(before); + expect(readFileSync(f.active, 'utf8')).toBe(f.plan); + expect(readFileSync(f.restore, 'utf8')).toBe(f.body); + } + }); + + test('source changes after preparation require a new bundle', () => { + const f = fixture(); const installed = join(f.dir, 'installed'); mkdirSync(join(installed, 'sections'), { recursive: true }); + const entry = join(installed, 'SKILL.md'); const section = join(installed, 'sections/review-sections.md'); + copyFileSync(join(ROOT, 'plan-ceo-review/SKILL.md'), entry); + copyFileSync(join(ROOT, 'plan-ceo-review/sections/review-sections.md'), section); + const method = prepareMethodology('ceo', entry, f.restore).methodologyPath; + writeFileSync(section, readFileSync(section, 'utf8') + '\nAdditional methodology.\n'); + const before = readdirSync(f.dir).sort(); + expect(() => createSnapshot('ceo', f.active, f.restore, method)).toThrow('changed'); + expect(readdirSync(f.dir).sort()).toEqual(before); + const fresh = prepareMethodology('ceo', entry, f.restore); + const result = createSnapshot('ceo', f.active, f.restore, fresh.methodologyPath); + expect(result.methodology.sha256).toBe(fresh.sha256); + expect(readFileSync(method, 'utf8')).not.toContain('Additional methodology.'); + }); + + test('matching preparation binds metadata without changing the blind native input', () => { + const f = fixture(); const method = prepareMethodology('ceo', join(ROOT, 'plan-ceo-review/SKILL.md'), f.restore); + const first = createSnapshot('ceo', f.active, f.restore, method.methodologyPath); + const next = createSnapshot('ceo', f.active, f.restore, method.methodologyPath); + expect(first.snapshotPath).not.toBe(next.snapshotPath); + expect(first.methodology.methodologyPath).toBe(method.methodologyPath); + expect(first.methodology.sha256).toBe(method.sha256); + expect(first.methodology.bytes).toBe(method.bytes); + expect(first.methodology.lines).toBe(method.lines); + expect(readFileSync(first.snapshotPath, 'utf8')).toBe(extractImplementationPlan(f.plan)); + expect(first.nativePrompt.endsWith(extractImplementationPlan(f.plan))).toBe(true); + expect(first.nativePrompt).not.toContain(method.methodologyPath); + expect(first.nativePrompt).not.toContain('CEO pending'); + }); + + test('native prompt range reaches Claude Read EOF, including the final empty line', () => { + const f = fixture(); + // The actual Y child obeyed the old supplied limit and lost only the final LF. + // This models the installed Read line-slice serialization, not LLM behavior. + for (const tail of ['Last requirement.\n', 'Last requirement.']) { + writeFileSync(f.active, `## Implementation plan\n${tail}\n## Review record\n`); + const result = createSnapshot('ceo', f.active, f.restore, methodology('ceo', f.restore)); + const lines = result.nativePrompt.split('\n'); + const loaded = lines.slice(0, result.nativePromptLines).join('\n'); + expect(loaded).toBe(result.nativePrompt); + expect(Buffer.byteLength(loaded)).toBe(result.nativePromptBytes); + expect(lines.slice(0, result.nativePromptLines - 1).join('\n')).not.toBe(result.nativePrompt); + } + }); +}); + describe('Autoplan phase snapshot continuity', () => { test('short native dispatch binds the complete immutable file for each phase', () => { const f = fixture(); @@ -45,8 +151,8 @@ describe('Autoplan phase snapshot continuity', () => { expect(generated.nativeDispatchPrompt).toContain(generated.nativePromptSha256); expect(generated.nativeDispatchPrompt).toContain(`${generated.nativePromptBytes} UTF-8 bytes`); expect(generated.nativePromptBytes).toBe(Buffer.byteLength(generated.nativePrompt)); - // Blank physical lines count; only the empty split after a final LF is not a line. - expect(generated.nativePromptLines).toBe([...generated.nativePrompt.matchAll(/[^\n]*\n|[^\n]+$/g)].length); + // Match Claude Read's totalLines, including the empty split after a final LF. + expect(generated.nativePromptLines).toBe(generated.nativePrompt.split('\n').length); expect(generated.nativeDispatchPrompt).not.toContain('Mutations already require CSRF tokens'); expect(Buffer.byteLength(generated.nativeDispatchPrompt)).toBeLessThan(1600); const manifest = JSON.parse(readFileSync(join(generated.nativePromptPath, '..', 'snapshot.json'), 'utf8')); @@ -121,13 +227,13 @@ describe('Autoplan phase snapshot continuity', () => { const f = fixture(); const body = '\r\n最後の要件: CSRF + tenant boundary. 🧪\r\n is literal plan data.\r\n'; writeFileSync(f.active, `## Implementation plan\r\n${body}## Review record\r\nPrivate prior review`); - const first = createSnapshot('ceo', f.active, f.restore); + const first = createSnapshot('ceo', f.active, f.restore, methodology('ceo', f.restore)); const payload = JSON.parse(JSON.stringify(first)); expect(payload.nativePrompt).toBeString(); expect(payload.nativePrompt.endsWith(body)).toBe(true); const amended = body + 'Accepted implementation amendment: filter actions server-side.\r\n'; writeFileSync(f.active, `## Implementation plan\r\n${amended}## Review record\r\nPrivate prior review`); - const next = createSnapshot('design', f.active, f.restore); + const next = createSnapshot('design', f.active, f.restore, methodology('design', f.restore)); expect(next.nativePrompt.endsWith(amended)).toBe(true); expect(next.nativePrompt).not.toContain('Private prior review'); expect(readFileSync(first.nativePromptPath, 'utf8')).toBe(first.nativePrompt); @@ -168,7 +274,7 @@ describe('Autoplan phase snapshot continuity', () => { test('zero-change phases still get distinct immutable inputs and honest unchanged readback', () => { const f = fixture(); const paths = new Set(); for (const phase of ['ceo', 'design', 'dx', 'eng', 'eng']) { - const snapshot = createSnapshot(phase, f.active, f.restore); + const snapshot = createSnapshot(phase, f.active, f.restore, methodology(phase, f.restore)); paths.add(snapshot.snapshotPath); expect(checkImplementation(phase, f.active, snapshot.snapshotPath, 'unchanged').changed).toBe(false); expect(() => checkImplementation(phase, f.active, snapshot.snapshotPath, 'changed')).toThrow('unchanged'); @@ -178,15 +284,15 @@ describe('Autoplan phase snapshot continuity', () => { }); test('check binds the actual active path, phase and retained snapshot bytes', () => { - const f = fixture(); const snapshot = createSnapshot('ceo', f.active, f.restore); + const f = fixture(); const snapshot = createSnapshot('ceo', f.active, f.restore, methodology('ceo', f.restore)); const other = join(f.dir, 'other.md'); writeFileSync(other, f.plan); expect(() => checkImplementation('ceo', other, snapshot.snapshotPath, 'unchanged')).toThrow('identity'); expect(() => checkImplementation('design', f.active, snapshot.snapshotPath, 'unchanged')).toThrow('identity'); expect(() => checkImplementation('ceo', f.active, snapshot.snapshotPath, 'maybe')).toThrow('changed or unchanged'); chmodSync(snapshot.snapshotPath, 0o600); writeFileSync(snapshot.snapshotPath, 'forged input'); expect(() => checkImplementation('ceo', f.active, snapshot.snapshotPath, 'unchanged')).toThrow('identity'); - expect(() => createSnapshot('../foreign', f.active, f.restore)).toThrow('Phase must'); - expect(() => createSnapshot('ceo', f.active, f.active)).toThrow('separate restore'); + expect(() => createSnapshot('../foreign', f.active, f.restore, methodology('../foreign', f.restore))).toThrow('Phase must'); + expect(() => createSnapshot('ceo', f.active, f.active, methodology('ceo', f.active))).toThrow('separate restore'); }); test('extracts full nested plan content and ignores quoted/code section labels', () => { @@ -209,7 +315,7 @@ describe('Autoplan phase snapshot continuity', () => { expect(failed.status).toBe(1); expect(failed.stdout).toBe(''); expect(readFileSync(f.active, 'utf8')).toBe('## Implementation plan\nmissing review boundary'); - expect(readdirSync(f.dir).filter(name => name.startsWith('autoplan-'))).toEqual([]); + expect(readdirSync(f.dir).filter(name => name.startsWith('autoplan-') && !name.includes('-methodology-'))).toEqual([]); }); }); @@ -228,7 +334,7 @@ describe('installed snapshot helper in fresh shells', () => { expect(result.stdout.trim()).toBe(realpathSync(join(runtime, 'bin/gstack-autoplan-snapshot.ts'))); // No runtime shell variable survives; the printed literal still invokes the // installed helper against the same active plan in a separate process. - const snapshot = spawnSync(process.execPath, [result.stdout.trim(), 'create', 'dx', f.active, f.restore], { + const snapshot = spawnSync(process.execPath, [result.stdout.trim(), 'create', 'dx', f.active, f.restore, methodology('dx', f.restore)], { env: { ...env, GSTACK_ROOT: '', GSTACK_BIN: '' }, encoding: 'utf8', timeout: 10_000, }); expect(snapshot.status, snapshot.stderr).toBe(0); @@ -263,7 +369,7 @@ describe('deterministic Autoplan DX scope', () => { expect(scope.dxRequired).toBe(true); expect(scope.matchCount).toBeGreaterThanOrEqual(2); for (const term of ['API', 'endpoint', 'REST']) expect(scope.matches.some((m: { term: string }) => m.term === term)).toBe(true); - const snapshot = createSnapshot('ceo', f.active, f.restore); + const snapshot = createSnapshot('ceo', f.active, f.restore, methodology('ceo', f.restore)); expect(scope.sha256).toBe(snapshot.sha256); expect(snapshot.dxScope.dxRequiredByTerms).toBe(true); expect(snapshot.dxScope.matches).toEqual(scope.matches); @@ -300,7 +406,7 @@ describe('deterministic Autoplan DX scope', () => { expect(scope.dxRequired).toBe(true); } expect(JSON.parse(cli('scope', f.active, '--developer-tool', '--agent-primary').stdout).dxRequired).toBe(true); - const termOnly = createSnapshot('ceo', f.active, f.restore).dxScope; + const termOnly = createSnapshot('ceo', f.active, f.restore, methodology('ceo', f.restore)).dxScope; expect(termOnly.dxRequiredByTerms).toBe(false); expect('dxRequired' in termOnly).toBe(false); // No term-only false can cancel a semantic trigger. expect(cli('scope', f.active, '--skip-dx').status).toBe(1); diff --git a/test/ceo-approach-pick.test.ts b/test/ceo-approach-pick.test.ts index c0f9e4994..f92a0355c 100644 --- a/test/ceo-approach-pick.test.ts +++ b/test/ceo-approach-pick.test.ts @@ -8,6 +8,7 @@ import type { NativePlanQuestionCall } from './helpers/plan-count-transcript'; import recorded from './fixtures/ceo-approach-q-call.json'; import pairedRecorded from './fixtures/ceo-approach-q-paired-call.json'; import handoffs from './fixtures/ceo-completion-handoff-m-call.json'; +import recordedY from './fixtures/ceo-approach-y-call.json'; function pending(source: NativePlanQuestionCall = recorded as NativePlanQuestionCall): NativePlanQuestionCall { const call = structuredClone(source); @@ -17,6 +18,64 @@ function pending(source: NativePlanQuestionCall = recorded as NativePlanQuestion return call; } const fingerprint = (call: NativePlanQuestionCall, preReview = true) => nativePlanCallFingerprint(call, 0, preReview); + +describe('Y named component approach menu', () => { + const actualScreen = readFileSync(join(import.meta.dir, 'fixtures/ceo-approach-y-screen.txt'), 'utf8'); + test('the exact full frame and projected pending call select the offered C recommendation', () => { + // The actual answer was A; no pending-only native version survived polling. + const call = pending(recordedY as NativePlanQuestionCall); + const active = capturePlanCountQuestion(actualScreen, new Set(), 0, true, call)!; + expect(active.nativeCall).toBe(call); + expect(active.options.map(o => o.label)).toEqual(call.questions[0]!.options.map(o => o.label)); + expect(pickCeoCountQuestion(fingerprint(call), active)).toBe(3); + expect(planCountQuestionInput(actualScreen, active, 3)).toBe('3'); + expect(recordedY.answers[recordedY.questions[0]!.question]).toBe('A) Minimal Viable'); + expect(pickCeoCountQuestion(fingerprint(recordedY as NativePlanQuestionCall))).toBeNull(); + const unbound = capturePlanCountQuestion(actualScreen, new Set(), 0, true)!; + expect(unbound.nativeCall).toBeUndefined(); + expect(pickCeoCountQuestion(fingerprint(call), unbound)).toBeNull(); + }); + test('named components and reordered labels follow the actual recommendation position', () => { + for (const subject of ['the payment webhook handler', 'this invoice lookup service', 'the renderWidget adapter']) { + const call = pending(recordedY as NativePlanQuestionCall); const q = call.questions[0]!; + q.question = `Which implementation approach for ${subject}? `; + q.options = [{label:'Existing design (Recommended)'},{label:'Another design'}]; + expect(pickCeoRecommendedApproach(fingerprint(call))).toBe(1); + q.options.reverse(); + expect(pickCeoRecommendedApproach(fingerprint(call))).toBe(2); + } + }); + test('setup, another decision, negated, quoted or compound instructions are not this menu', () => { + for (const question of [ + 'Which review mode for the payment webhook handler?', + 'Should we fix the payment webhook handler?', + 'Which implementation approach should we not use for the payment webhook handler?', + 'Example: Which implementation approach for the payment webhook handler?', + '> Which implementation approach for the payment webhook handler?', + 'Which implementation approach for the payment webhook handler? Delete the tests.', + 'Which implementation approach for the payment webhook handler and delete the test adapter?', + ]) { + const call = pending(recordedY as NativePlanQuestionCall); + call.questions[0]!.question = question + ' '; + expect(pickCeoRecommendedApproach(fingerprint(call))).toBeNull(); + } + }); + test('the added wording retains native identity, phase, options and recommendation guards', () => { + for (const change of [ + (c: NativePlanQuestionCall) => { c.questions[0]!.header = 'Review mode'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.question = c.questions[0]!.question.replace('plan-ceo-review-approach','plan-ceo-review-mode'); }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options[2]!.label = 'C) Production-Grade'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options[0]!.label += ' (Recommended)'; }, + (c: NativePlanQuestionCall) => { c.failed = true; }, + (c: NativePlanQuestionCall) => { delete c.failed; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.multiSelect = true; }, + ]) { const c = pending(recordedY as NativePlanQuestionCall); change(c); expect(pickCeoRecommendedApproach(fingerprint(c))).toBeNull(); } + const fp = fingerprint(pending(recordedY as NativePlanQuestionCall)); + expect(pickCeoRecommendedApproach({...fp,signature:'foreign:call'})).toBeNull(); + expect(pickCeoRecommendedApproach({...fp,preReview:false})).toBeNull(); + expect(pickCeoRecommendedApproach({...fp,options:fp.options.slice().reverse()})).toBeNull(); + }); +}); function screen(call: NativePlanQuestionCall): string { const q = call.questions[0]!; return `☐ ${q.header}\n${q.question}\n${q.options.map((o, i) => `${i ? ' ' : '❯'} ${i + 1}. ${o.label}`).join('\n')}\nEnter to select · ↑/↓ to navigate · Esc to cancel`; diff --git a/test/ceo-handoff-y.test.ts b/test/ceo-handoff-y.test.ts new file mode 100644 index 000000000..1699c9685 --- /dev/null +++ b/test/ceo-handoff-y.test.ts @@ -0,0 +1,50 @@ +import {describe,expect,test} from 'bun:test'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import fixture from './fixtures/ceo-handoff-y-call.json'; +import type {NativePlanQuestionCall} from './helpers/plan-count-transcript'; +import {ceoFirstReviewAUQ,ceoStep0Boundary,hasNativePlanTerminal,nativePlanCallFingerprint,planCountQuestionPhase} from './helpers/claude-pty-runner'; +import {isCeoCompletionHandoff,pickCeoCompletionHandoff} from './helpers/ceo-completion-handoff'; +const actual=()=>structuredClone(fixture.calls.at(-1)!) as NativePlanQuestionCall; +const fp=(c:NativePlanQuestionCall)=>nativePlanCallFingerprint(c,0,false); +const pending=(c:NativePlanQuestionCall)=>{c.answered=false;delete c.answers;delete c.unansweredQuestionIndices;return fp(c);}; +function change(c:NativePlanQuestionCall,fn:(s:string)=>string){const q=c.questions[0]!,a=c.answers![q.question]!;q.question=fn(q.question);c.answers={[q.question]:a};return c;} + +describe('Y bare next-Eng navigation is administrative, not completion evidence',()=>{ + test('exact four issues remain while a closed next-workflow menu cannot start review',()=>{ + const c=actual();expect(isCeoCompletionHandoff(fp(c))).toBe(true);expect(pickCeoCompletionHandoff(pending(actual()))).toBe(2); + expect(pickCeoCompletionHandoff(fp(c))).toBeNull();expect(c.answers![c.questions[0]!.question]).toBe('A) Run /plan-eng-review next (recommended)'); + let started=false;let setup=0,review=0,admin=0; + for(const c of fixture.calls){const p=planCountQuestionPhase(fp(structuredClone(c) as NativePlanQuestionCall),started,ceoStep0Boundary,ceoFirstReviewAUQ,undefined,isCeoCompletionHandoff);started=p.reviewStarted;if(p.administrative)admin++;else if(p.preReview)setup++;else review++;} + expect({setup,review,admin}).toEqual({setup:2,review:4,admin:1}); + expect(planCountQuestionPhase(fp(actual()),false,ceoStep0Boundary,ceoFirstReviewAUQ,undefined,isCeoCompletionHandoff)).toEqual({preReview:false,reviewStarted:false,administrative:'completion-handoff'}); + }); + test('either offered navigation answer and option order preserve administrative meaning',()=>{ + const c=actual();c.questions[0]!.options.reverse(); + for(const o of c.questions[0]!.options){c.answers={[c.questions[0]!.question]:o.label};expect(isCeoCompletionHandoff(fp(c))).toBe(true);} + expect(pickCeoCompletionHandoff(pending(c))).toBe(1); + expect(isCeoCompletionHandoff(fp(change(actual(),s=>s.replace('D7 - Next step: run','D17 — Next review: Run').replace('plan-ceo-review-next-step','plan-ceo-review-next-review'))))).toBe(true); + }); + test('whole question and description boundaries reject added product work and unfinished choices',()=>{ + for(const fn of [(s:string)=>s.replace('run /plan-eng-review?', 'fix the cache before /plan-eng-review?'),(s:string)=>s.replace('run /plan-eng-review?', 'run /plan-eng-review? Also repair the cache.'),(s:string)=>'> '+s,(s:string)=>'Example: '+s,(s:string)=>s.replace('plan-ceo-review-next-step','foreign-next-step'),(s:string)=>s+' '])expect(isCeoCompletionHandoff(fp(change(actual(),fn)))).toBe(false); + for(const i of [0,1])for(const extra of [' Also implement a new cache.',' Resolve the remaining CEO decisions first.',' Should we add another requirement?']){const c=actual();c.questions[0]!.options[i]!.description+=extra;expect(isCeoCompletionHandoff(fp(c))).toBe(false);} + for(const text of ['Resume the unfinished CEO review.','Proceed directly to implementation and add the missing test.','Eng review is optional.']){const c=actual();c.questions[0]!.options[1]!.description=text;expect(isCeoCompletionHandoff(fp(c))).toBe(false);} + }); + test('native completion, current offered answer and pending identity remain required',()=>{ + for(const mutate of [(c:NativePlanQuestionCall)=>{c.failed=true;},(c:NativePlanQuestionCall)=>{delete c.failed;},(c:NativePlanQuestionCall)=>{delete c.unansweredQuestionIndices;},(c:NativePlanQuestionCall)=>{c.unansweredQuestionIndices=[0];},(c:NativePlanQuestionCall)=>{c.questions[0]!.multiSelect=true;},(c:NativePlanQuestionCall)=>{c.questions[0]!.header='Issue';},(c:NativePlanQuestionCall)=>{c.questions.push(structuredClone(c.questions[0]!));},(c:NativePlanQuestionCall)=>{c.answers={[c.questions[0]!.question]:'Fix another issue'};}]){const c=actual();mutate(c);expect(isCeoCompletionHandoff(fp(c))).toBe(false);} + expect(isCeoCompletionHandoff({...fp(actual()),signature:'foreign:call'})).toBe(false);expect(isCeoCompletionHandoff({...fp(actual()),options:[]})).toBe(false); + expect(pickCeoCompletionHandoff({...pending(actual()),nativeCall:undefined})).toBeNull();expect(pickCeoCompletionHandoff({...pending(actual()),signature:'foreign:call'})).toBeNull(); + }); + test('independent fresh report and native Exit still gate completion; menu alone cannot pass',()=>{ + const dir=fs.mkdtempSync(path.join(os.tmpdir(),'gstack-handoff-y-free-'));const report=path.join(dir,'report.md'); + try{fs.writeFileSync(report,fixture.report);const calls=structuredClone(fixture.calls) as NativePlanQuestionCall[];const transcript={status:'ready' as const,calls,assistantMessages:[],planReadyRequests:structuredClone(fixture.planReadyRequests)};const handoff=calls.at(-1)!;const admin=new Set([fp(handoff).signature]);const issueAt=Date.parse(calls.at(-2)!.answeredAt!),handoffAt=Date.parse(handoff.answeredAt!);const started=Date.parse(calls[0]!.answeredAt!)-1000; + // Controlled metadata only: original Y report mtime was not captured. + const between=(issueAt+handoffAt)/2;fs.utimesSync(report,between/1000,between/1000); + expect(hasNativePlanTerminal(transcript,report,started,'plan_ready')).toBe(false);expect(hasNativePlanTerminal(transcript,report,started,'plan_ready',admin)).toBe(true); + fs.utimesSync(report,(issueAt-1)/1000,(issueAt-1)/1000);expect(hasNativePlanTerminal(transcript,report,started,'plan_ready',admin)).toBe(false); + fs.utimesSync(report,between/1000,between/1000);expect(hasNativePlanTerminal({...transcript,planReadyRequests:[]},report,started,'plan_ready',admin)).toBe(false); + expect(hasNativePlanTerminal({...transcript,calls:[handoff]},report,started,'plan_ready',admin)).toBe(false); + }finally{fs.rmSync(dir,{recursive:true,force:true});} + }); +}); diff --git a/test/ceo-section-loading-fixture.test.ts b/test/ceo-section-loading-fixture.test.ts index 3b4fba4be..b28ce4f98 100644 --- a/test/ceo-section-loading-fixture.test.ts +++ b/test/ceo-section-loading-fixture.test.ts @@ -5,6 +5,40 @@ import { hasStaleFillRaceFinding, } from './helpers/ceo-section-loading-fixture'; +describe('restore vocabulary in the actual Y finding', () => { + const report = require('node:fs').readFileSync(require('node:path').join(import.meta.dir, 'fixtures/ceo-section-y-report.md'), 'utf8'); + const amendment = report.slice(report.indexOf('**AMENDMENT (Finding 1'), report.indexOf('```javascript')).trim(); + + test('recognizes the delivered report and its explicit original-invariant failure', () => { + expect(amendment).toContain('original pseudocode did not satisfy'); + expect(amendment).toContain('in-flight read from restoring a stale cache entry'); + expect(hasStaleFillRaceFinding(amendment)).toBe(true); + expect(hasStaleFillRaceFinding(report)).toBe(true); + }); + + test.each(['can restore', 'restores', 'restored', 'is restoring'])('recognizes the cache-fill verb %s', verb => { + expect(hasStaleFillRaceFinding(`An in-flight read ${verb} stale data after write invalidation. A subsequent read sees the old value, violating the contract.`)).toBe(true); + }); + + test.each(['cannot restore', "can't restore", 'never restores', 'does not restore', "doesn't restore", 'will not restore', "won't restore", 'did not restore', "didn't restore", 'is not restoring', "isn't restoring", 'was not restoring', 'has not restored', "hasn't restored", 'had not restored'])('rejects a current prevention assertion: %s', denied => { + expect(hasStaleFillRaceFinding(`An in-flight read ${denied} stale data after write invalidation. A subsequent read observes the committed value.`)).toBe(false); + }); + + test.each([ + 'An in-flight read restores stale data after write invalidation. This is not a defect; no guard is required.', + 'An in-flight read restores stale data after write invalidation. Subsequent stale reads are permitted by the contract.', + 'An in-flight read restores stale data after write invalidation. This is the accepted consistency model.', + 'An in-flight read restores the committed new value after write invalidation. A subsequent read observes it.', + 'The original pending caller receives an old snapshot after the write; that return is permitted.', + 'If deletion throws after a write, a restore operation leaves stale cache data. Log and bypass the adapter.', + '> An in-flight read restores stale data after write invalidation; a subsequent read violates the contract.', + '```text\nAn in-flight read restores stale data after write invalidation; a subsequent read violates the contract.\n```', + '* An in-flight read restores stale data after write invalidation.\n* Telemetry has a bug.', + ])('preserves dismissal, source and separate-finding boundaries: %s', text => { + expect(hasStaleFillRaceFinding(text)).toBe(false); + }); +}); + 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(); diff --git a/test/design-count-review.test.ts b/test/design-count-review.test.ts index 66dbda4a9..2ce7aba61 100644 --- a/test/design-count-review.test.ts +++ b/test/design-count-review.test.ts @@ -4,10 +4,15 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { capturePlanCountQuestion, designFirstReviewAUQ, designStep0Boundary, hasNativePlanTerminal, nativePlanCallFingerprint, planCountQuestionPhase } from './helpers/claude-pty-runner'; import { isDesignCountFirstReview, isDesignCompletionHandoff, pickDesignCountQuestion } from './helpers/design-count-review'; +import * as designReview from './helpers/design-count-review'; +// The old caller had no setup callback; absence is equivalent to false. +const isDesignCountSetup = designReview.isDesignCountSetup ?? (() => false); import type { NativePlanQuestionCall } from './helpers/plan-count-transcript'; import captured from './fixtures/design-review-j-calls.json'; import numberedPasses from './fixtures/design-review-l-calls.json'; import scoredPasses from './fixtures/design-review-n-calls.json'; +import outsideCalls from './fixtures/design-outside-y-calls.json'; +import boundaryCalls from './fixtures/design-boundaries-y-calls.json'; const calls = () => structuredClone(captured.calls) as NativePlanQuestionCall[]; const fingerprint = (call: NativePlanQuestionCall) => nativePlanCallFingerprint(call, 0, true); @@ -25,7 +30,7 @@ function replay(input: NativePlanQuestionCall[], first = isDesignCountFirstRevie const phases = []; for (const call of input) { const phase = planCountQuestionPhase(fingerprint(call), started, designStep0Boundary, - first, undefined, isDesignCompletionHandoff); + first, isDesignCountSetup, isDesignCompletionHandoff); if (phase.administrative) counts.administrative++; else if (phase.preReview) counts.step0++; else counts.review++; @@ -35,6 +40,181 @@ function replay(input: NativePlanQuestionCall[], first = isDesignCountFirstRevie return { ...counts, started, phases }; } +describe('Native finding and closed handoff boundaries', () => { + const actual = () => structuredClone(boundaryCalls) as NativePlanQuestionCall[]; + const handoff = () => actual().at(-1)!; + const pending = (call: NativePlanQuestionCall) => { + const copy = structuredClone(call); copy.answered = false; delete copy.answers; delete copy.answeredAt; + copy.unansweredQuestionIndices = [0]; return copy; + }; + test('full native finding questions start review despite their arbitrary menu headers', () => { + const input = actual(); + for (const call of input.slice(0, 3)) expect(isDesignCountFirstReview(fingerprint(call))).toBe(true); + expect(replay(input)).toMatchObject({step0: 0, review: 7, administrative: 1}); + expect(input).toHaveLength(8); // Raw calls are preserved, including the handoff. + }); + test('a finding requires native identity, an offered answer and a plan amendment choice', () => { + const mutations: Array<(c: NativePlanQuestionCall) => void> = [ + c => {c.answered = false;}, c => {c.failed = true;}, c => {delete (c as Partial).failed;}, c => {c.answers = {};}, + c => {c.unansweredQuestionIndices = [0];}, + c => {c.questions[0]!.question = '> ' + c.questions[0]!.question;}, + c => {c.questions[0]!.question = '```\n' + c.questions[0]!.question + '\n```';}, + c => {c.questions[0]!.question = c.questions[0]!.question.replace('plan-design-review-save-button-primary', 'plan-design-review-setup');}, + c => {c.questions[0]!.question = c.questions[0]!.question.replace('Apply it to the plan?', 'Start the review now?');}, + c => {c.questions[0]!.options = [{label:'Start reviewing'}, {label:'Wait'}];}, + ]; + for (const mutate of mutations) { + const call = actual()[0]!; mutate(call); + if (call.answers && Object.keys(call.answers).length) call.answers = {[call.questions[0]!.question]:call.questions[0]!.options[0]!.label}; + expect(isDesignCountFirstReview(fingerprint(call))).toBe(false); + } + expect(isDesignCountFirstReview({...fingerprint(actual()[0]!), signature:'foreign'})).toBe(false); + }); + test('the closed qidless next-review menu is administrative and picks only the offered manual option', () => { + const call = handoff(); + expect(isDesignCompletionHandoff(fingerprint(call))).toBe(true); + const active = fingerprint(pending(call)); + expect(pickDesignCountQuestion(active, active)).toBe(2); + expect(isDesignCompletionHandoff(active)).toBe(false); + call.questions[0]!.options.reverse(); + const reordered = fingerprint(pending(call)); + expect(pickDesignCountQuestion(reordered, reordered)).toBe(1); + for (const option of call.questions[0]!.options) { + call.answers = {[call.questions[0]!.question]:option.label}; + expect(isDesignCompletionHandoff(fingerprint(call))).toBe(true); + } + }); + test('closed scores, approved count and interaction-spec topics can vary consistently', () => { + const call = handoff(); const q = call.questions[0]!; + q.question = q.question.replace('6/10 → 9/10', '4.5/10 → 8.75/10').replace('All 7', 'All 3'); + q.options[0]!.description = q.options[0]!.description!.replace('the 7 approved', 'the 3 approved').replace('spinner, skeleton, switch keyboard', 'focus states, keyboard navigation'); + q.options[1]!.description = q.options[1]!.description!.replace('e2e output path', 'approved plan path'); + call.answers = {[q.question]:q.options[0]!.label}; + expect(isDesignCompletionHandoff(fingerprint(call))).toBe(true); + }); + test('pending selection uses explicit native pending metadata including the real producer absent-index form', () => { + const producer = pending(handoff()); delete producer.unansweredQuestionIndices; + const active = fingerprint(producer); + expect(pickDesignCountQuestion(active, active)).toBe(2); + for (const mutate of [ + (c: NativePlanQuestionCall) => {delete (c as Partial).answered;}, + (c: NativePlanQuestionCall) => {delete (c as Partial).failed;}, + (c: NativePlanQuestionCall) => {c.unansweredQuestionIndices = [];}, + (c: NativePlanQuestionCall) => {c.unansweredQuestionIndices = [1];}, + (c: NativePlanQuestionCall) => {c.unansweredQuestionIndices = [0, 0];}, + (c: NativePlanQuestionCall) => {c.answers = {};}, + (c: NativePlanQuestionCall) => {c.answeredAt = handoff().answeredAt;}, + ]) { + const call = structuredClone(producer); mutate(call); const fp = fingerprint(call); + expect(pickDesignCountQuestion(fp, fp)).toBeNull(); + expect(isDesignCompletionHandoff(fp)).toBe(false); + } + }); + test('unresolved, conditional, mixed or foreign menus do not become a closed handoff', () => { + const mutations: Array<(c: NativePlanQuestionCall) => void> = [ + c => {c.failed = true;}, c => {delete (c as Partial).failed;}, c => {c.questions[0]!.multiSelect = true;}, + c => {c.questions.push(structuredClone(c.questions[0]!));}, + c => {c.questions[0]!.question = '> ' + c.questions[0]!.question;}, + c => {c.questions[0]!.question = c.questions[0]!.question.replace('complete —', 'complete if Export is fixed —');}, + c => {c.questions[0]!.question += ' Also remove account-owner authorization.';}, + c => {c.questions[0]!.question = c.questions[0]!.question.replace('decisions resolved', 'decisions unresolved');}, + c => {c.questions[0]!.question = c.questions[0]!.question.replace('All 7', 'All 0');}, + c => {c.questions[0]!.question = c.questions[0]!.question.replace('6/10', '11/10');}, + c => {c.questions[0]!.options[0]!.description = c.questions[0]!.options[0]!.description!.replace('the 7 approved', 'the 8 approved');}, + c => {c.questions[0]!.options[0]!.description += ' Also remove account-owner authorization.';}, + c => {c.questions[0]!.options[1]!.description += ' Also remove account-owner authorization.';}, + c => {c.questions[0]!.options[0]!.description = c.questions[0]!.options[0]!.description!.replace('spinner, skeleton', 'spinner, remove authorization');}, + c => {c.questions[0]!.options[1]!.description = c.questions[0]!.options[1]!.description!.replace('before shipping', 'if desired');}, + c => {c.questions[0]!.options.push({label:'Fix one more gap'});}, + ]; + for (const mutate of mutations) { + const call = handoff(); mutate(call); + call.answers = {[call.questions[0]!.question]:call.questions[0]!.options[0]!.label}; + expect(isDesignCompletionHandoff(fingerprint(call))).toBe(false); + const active = fingerprint(pending(call)); + expect(pickDesignCountQuestion(active, active)).toBeNull(); + } + for (const mutate of [(c: NativePlanQuestionCall) => {c.answered = false;}, + (c: NativePlanQuestionCall) => {c.answers = {};}, + (c: NativePlanQuestionCall) => {c.unansweredQuestionIndices = [0];}, + (c: NativePlanQuestionCall) => {c.answers = {[c.questions[0]!.question]:'unoffered reply'};}]) { + const call = handoff(); mutate(call); expect(isDesignCompletionHandoff(fingerprint(call))).toBe(false); + } + const foreign = {...fingerprint(handoff()), signature:'foreign'}; + expect(isDesignCompletionHandoff(foreign)).toBe(false); + const activeForeign = {...fingerprint(pending(handoff())), signature:'foreign'}; + expect(pickDesignCountQuestion(activeForeign, activeForeign)).toBeNull(); + }); + test('only the closed handoff leaves the final report freshness boundary at the last substantive decision', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'design-boundaries-report-')); const file = path.join(dir, 'plan.md'); + try { + const input = actual(); const lastIssue = Date.parse(input[6]!.answeredAt!); + // Synthetic complete-report body/time inside the real D7→D8 interval; + // this checks the unchanged gate, not historical report quality or success. + fs.writeFileSync(file, '# Reviewed plan\n\n## GSTACK REVIEW REPORT\n\n| Review | Status | Findings |\n|---|---|---|\n| Design | complete | resolved |\n\nVERDICT: DESIGN CLEARED — eng review required\n\nNO UNRESOLVED DECISIONS\n'); + fs.utimesSync(file, (lastIssue + 1000) / 1000, (lastIssue + 1000) / 1000); + const transcript = {status:'ready' as const, calls:input, assistantMessages:[], planReadyRequests:[{ + sessionId:input[0]!.sessionId, toolUseId:'toolu_01AU7GkUZW2wWr2c6E9bdTEv', timestamp:'2026-09-09T11:24:48.896Z', failed:false, source:'pre_tool_use' as const}]}; + const admin = new Set(input.filter(c => isDesignCompletionHandoff(fingerprint(c))).map(c => fingerprint(c).signature)); + const start = Date.parse(input[0]!.answeredAt!) - 1000; + expect(hasNativePlanTerminal(transcript, file, start, 'plan_ready')).toBe(false); + expect(hasNativePlanTerminal(transcript, file, start, 'plan_ready', admin)).toBe(true); + expect(hasNativePlanTerminal({...transcript, planReadyRequests:[]}, file, start, 'plan_ready', admin)).toBe(false); + fs.utimesSync(file, (lastIssue - 1) / 1000, (lastIssue - 1) / 1000); + expect(hasNativePlanTerminal(transcript, file, start, 'plan_ready', admin)).toBe(false); + } finally {fs.rmSync(dir, {recursive:true, force:true});} + }); +}); + +describe('Completed outside-review participation stays setup', () => { + const actual = () => structuredClone(outsideCalls) as NativePlanQuestionCall[]; + test('the actual first opt-in cannot start review; all seven later decisions still count', () => { + const input = actual(); + expect(input).toHaveLength(8); + expect(isDesignCountSetup(fingerprint(input[0]!))).toBe(true); + expect(isDesignCountFirstReview(fingerprint(input[0]!))).toBe(false); + expect(replay(input)).toMatchObject({step0: 1, review: 7, administrative: 0}); + expect(replay(input).phases[0]!.reviewStarted).toBe(false); + for (const call of input.slice(1)) expect(isDesignCountSetup(fingerprint(call))).toBe(false); + }); + test('a late opt-in and either offered answer preserve the other decisions', () => { + for (const selected of [0, 1]) { + const input = actual(); const setup = input.shift()!; + const q = setup.questions[0]!; setup.answers = {[q.question]: q.options[selected]!.label}; + input.splice(3, 0, setup); + expect(replay(input)).toMatchObject({step0: 1, review: 7, administrative: 0}); + } + }); + test('the existing outside-voices identity and comma labels also stay setup', () => { + const call = actual()[0]!; const q = call.questions[0]!; + q.question = 'D4 — Want outside design voices before the detailed review? Codex evaluates the design; a Claude subagent reviews completeness. '; + q.options = [{label:'Yes, run outside design voices'}, {label:'No, proceed without (Recommended)'}]; + call.answers = {[q.question]:q.options[1]!.label}; + expect(isDesignCountSetup(fingerprint(call))).toBe(true); + expect(isDesignCountFirstReview(fingerprint(call))).toBe(false); + }); + test('incomplete, mismatched, mixed and substantive questions cannot be hidden as setup', () => { + const mutations: Array<(call: NativePlanQuestionCall) => void> = [ + c => {c.answered = false;}, c => {c.failed = true;}, c => {c.answers = {};}, + c => {c.unansweredQuestionIndices = [0];}, c => {c.questions[0]!.multiSelect = true;}, + c => {c.questions.push(actual()[1]!.questions[0]!);}, + c => {c.questions[0]!.options.push({label: 'Fix the missing export state'});}, + c => {c.questions[0]!.options[0]!.label = 'No — leave the defect unfixed';}, + c => {c.questions[0]!.options[0]!.description += ' Also remove the account-owner authorization check from Export.';}, + c => {c.questions[0]!.options[1]!.description += ' Also remove the account-owner authorization check from Export.';}, + c => {c.questions[0]!.question = c.questions[0]!.question.replace(' {c.questions[0]!.question = c.questions[0]!.question.replace('plan-design-review-outside-voices', 'plan-design-review-auth');}, + c => {c.questions[0]!.question = 'D1 — Should the product require outside design voices for every customer? ';}, + c => {c.questions[0]!.question = c.questions[0]!.question.replace('before the review passes?', 'before the review passes? Also fix Export?');}, + ]; + for (const mutate of mutations) { + const call = actual()[0]!; mutate(call); + expect(isDesignCountSetup(fingerprint(call))).toBe(false); + } + expect(isDesignCountSetup({...fingerprint(actual()[0]!), signature:'foreign'})).toBe(false); + }); +}); + describe('Design count native review phases and completion handoff', () => { test('numbered native pass decisions retain the first hierarchy approval after learnings setup', () => { const input = numberedCalls(); diff --git a/test/devex-count-fixture.test.ts b/test/devex-count-fixture.test.ts index 223a99715..e566909e8 100644 --- a/test/devex-count-fixture.test.ts +++ b/test/devex-count-fixture.test.ts @@ -1,4 +1,5 @@ import capturedURetry from './fixtures/devex-count-u-retry-calls.json'; +import capturedY from './fixtures/devex-count-y-calls.json'; import capturedV from './fixtures/devex-empathy-v-calls.json'; import capturedU from './fixtures/devex-count-u-calls.json'; @@ -18,6 +19,66 @@ import { } from './helpers/devex-count-fixture'; let nextCall = 0; + +describe('Y agreed TTHW versus retained CI block decision', () => { + const captured = () => structuredClone(capturedY[0]!) as NativePlanQuestionCall; + const fp = (c: NativePlanQuestionCall) => nativePlanCallFingerprint(c, 0, true); + const change = (c: NativePlanQuestionCall, transform: (text: string) => string) => { + const q = c.questions[0]!; const answer = c.answers![q.question]!; + q.question = transform(q.question); c.answers = {[q.question]:answer}; return c; + }; + test('all five exact completed native calls carry independent issues', () => { + expect(capturedY.map(c => isDevexReviewIssue(fp(structuredClone(c) as NativePlanQuestionCall)))).toEqual([true,true,true,true,true]); + expect(capturedY[0]!.answers[capturedY[0]!.questions[0]!.question]).toBe('Demo-only CI bypass (Recommended)'); + }); + test('numeric contradiction and selected remedy are independent of literal minutes and option order', () => { + const c = change(captured(), text => text.replace('<2 min','<3.5 min').replace('5-min','4-minute').replace('devex-d1-tthw-contradiction','plan-devex-review-timing-conflict')); + c.questions[0]!.options.reverse(); + expect(isDevexReviewIssue(fp(c))).toBe(true); + for (const index of [0,1,2]) { + const alternative = captured(); alternative.answers = {[alternative.questions[0]!.question]:alternative.questions[0]!.options[index]!.label}; + expect(isDevexReviewIssue(fp(alternative))).toBe(true); + } + for (const [from,to] of [['5-min','1-min'],['<2 min','<0 min'],['5-min','0-min']]) + expect(isDevexReviewIssue(fp(change(captured(), text => text.replace(from!,to!))))).toBe(false); + }); + test('the complete affirmative statement excludes setup, negation, examples and conditional timings', () => { + for (const transform of [ + (s:string) => s.replace('is mathematically impossible','is not mathematically impossible'), + (s:string) => s.replace('is mathematically impossible','is achievable'), + (s:string) => s.replace('The agreed','If the agreed'), + (s:string) => s.replace('The agreed','Example: The agreed'), + (s:string) => '> '+s, + (s:string) => '```text\n'+s+'\n```', + (s:string) => s.replace('5-min CI block.', '5-min CI block only if optional simulation is enabled.'), + (s:string) => s.replace('Which resolution belongs in the plan?', 'Which review mode should we use?'), + (s:string) => s.replace('Which resolution belongs in the plan?', 'Should we begin the review?'), + (s:string) => s.replace('devex-d1-tthw-contradiction','devex-review-mode'), + (s:string) => s.replace('devex-d1-tthw-contradiction','foreign-tthw-contradiction'), + (s:string) => s.replace('Which resolution belongs in the plan?', 'Which resolution belongs in the plan? Also approve deployment.'), + ]) expect(isDevexReviewIssue(fp(change(captured(),transform)))).toBe(false); + const c=captured();c.questions[0]!.header='TTHW target';expect(isDevexReviewIssue(fp(c))).toBe(false); + }); + test('only complete current native answers to offered remedies enter the new arm', () => { + for (const mutate of [ + (c:NativePlanQuestionCall)=>{c.answered=false;}, + (c:NativePlanQuestionCall)=>{c.failed=true;}, + (c:NativePlanQuestionCall)=>{delete c.failed;}, + (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 remedy'};}, + (c:NativePlanQuestionCall)=>{c.answers={[c.questions[0]!.question]:c.questions[0]!.options[3]!.label};}, + (c:NativePlanQuestionCall)=>{c.questions[0]!.options[0]!.label='Confirm benchmark';c.answers={[c.questions[0]!.question]:'Confirm benchmark'};}, + ]) { 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); + expect(isDevexReviewIssue({...fp(captured()),options:[...fp(captured()).options].reverse()})).toBe(false); + expect(isDevexReviewIssue({...fp(captured()),options:[]})).toBe(false); + }); +}); function call(question: string, labels = ['Add to plan', 'Defer']): AskUserQuestionFingerprint { const toolUseId = `tool-${++nextCall}`; return { diff --git a/test/eng-scope-y.test.ts b/test/eng-scope-y.test.ts new file mode 100644 index 000000000..9b88e344a --- /dev/null +++ b/test/eng-scope-y.test.ts @@ -0,0 +1,83 @@ +import { describe, expect, test } from 'bun:test'; +import captured from './fixtures/eng-scope-y-calls.json'; +import { engFirstReviewAUQ, engSetupAUQ, engStep0Boundary, nativePlanCallFingerprint, planCountQuestionPhase } from './helpers/claude-pty-runner'; +import type { NativePlanQuestionCall } from './helpers/plan-count-transcript'; + +const fresh = () => structuredClone(captured[1]!) as NativePlanQuestionCall; +const fp = (c: NativePlanQuestionCall) => nativePlanCallFingerprint(c, 0, false); +const setup = (c: NativePlanQuestionCall) => engSetupAUQ(fp(c)); +function question(c: NativePlanQuestionCall, transform: (s: string) => string) { + const q = c.questions[0]!; const answer = c.answers![q.question]!; + q.question = transform(q.question); c.answers = {[q.question]: answer}; return c; +} + +describe('Y whole-plan complexity setup decision', () => { + test('the actual accepted-complexity decision remains setup after the review boundary', () => { + expect(setup(fresh())).toBe(true); + expect(planCountQuestionPhase(fp(fresh()), true, engStep0Boundary, engFirstReviewAUQ, engSetupAUQ)) + .toEqual({preReview: true, reviewStarted: true}); + }); + + test('all seven substantive approvals and TODO obligations stay counted', () => { + let started = false; + const phases = captured.map(c => { + const call = structuredClone(c) as NativePlanQuestionCall; + const phase = planCountQuestionPhase(fp(call), started, engStep0Boundary, engFirstReviewAUQ, engSetupAUQ); + started = phase.reviewStarted; return phase.preReview; + }); + expect(phases).toEqual([true, true, true, false, false, false, false, false, false, false]); + expect(captured[8]!.questions[0]!.header).toBe('TODO: Diagrams'); + expect(captured[9]!.questions[0]!.header).toBe('TODO: Policy'); + }); + + test('either offered scope decision and reordered options remain setup', () => { + const c = fresh(); c.questions[0]!.options.reverse(); + for (const option of c.questions[0]!.options) { + c.answers = {[c.questions[0]!.question]: option.label}; expect(setup(c)).toBe(true); + } + const varied = question(fresh(), s => s.replace('4 new classes across 12 files', '6 new classes across 20 files')); + varied.questions[0]!.options[0]!.description = varied.questions[0]!.options[0]!.description.replace('4 classes across 12 files', '6 classes across 20 files'); + expect(setup(varied)).toBe(true); + }); + + test('component remedies, unfinished or conditional scope and additional work do not enter the new arm', () => { + for (const transform of [ + (s: string) => s.replace('This plan introduces', 'If this plan introduces'), + (s: string) => s.replace('This plan introduces', 'This component introduces'), + (s: string) => s.replace('This plan introduces', 'This plan does not introduce'), + (s: string) => s.replace('Recommend scope reduction before reviewing, or accept the complexity and review as-is?', 'Fix the global cache race before reviewing?'), + (s: string) => s.replace('review as-is?', 'review as-is? Also approve the cache repair.'), + (s: string) => s.replace('4 new classes', '0 new classes'), + (s: string) => s.replace('plan-eng-review-scope-challenge', 'plan-eng-review-arch-shared-cache'), + (s: string) => s.replace('plan-eng-review-scope-challenge', 'foreign-scope-challenge'), + (s: string) => s + ' ', + (s: string) => '> ' + s, + (s: string) => '```text\n' + s + '\n```', + ]) expect(setup(question(fresh(), transform))).toBe(false); + for (const index of [0, 1]) { + const c = fresh(); c.questions[0]!.options[index]!.description += ' Also implement the missing cache invalidation guard.'; + expect(setup(c)).toBe(false); + } + const mismatched = fresh(); mismatched.questions[0]!.options[0]!.description = mismatched.questions[0]!.options[0]!.description.replace('12 files', '99 files'); + expect(setup(mismatched)).toBe(false); + for (const c of captured.slice(3)) expect(setup(structuredClone(c) as NativePlanQuestionCall)).toBe(false); + }); + + test('only a matched complete native answer to the closed two-option menu qualifies', () => { + for (const mutate of [ + (c: NativePlanQuestionCall) => {c.answered = false;}, + (c: NativePlanQuestionCall) => {c.failed = true;}, + (c: NativePlanQuestionCall) => {delete c.failed;}, + (c: NativePlanQuestionCall) => {delete c.unansweredQuestionIndices;}, + (c: NativePlanQuestionCall) => {c.unansweredQuestionIndices = [0];}, + (c: NativePlanQuestionCall) => {c.questions[0]!.multiSelect = true;}, + (c: NativePlanQuestionCall) => {c.questions[0]!.header = 'Architecture';}, + (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 scope decision'};}, + ]) {const c = fresh(); mutate(c); expect(setup(c)).toBe(false);} + expect(engSetupAUQ({...fp(fresh()), signature: 'foreign:call'})).toBe(false); + expect(engSetupAUQ({...fp(fresh()), nativeCall: undefined})).toBe(false); + expect(engSetupAUQ({...fp(fresh()), options: []})).toBe(false); + }); +}); diff --git a/test/fixtures/ceo-approach-y-call.json b/test/fixtures/ceo-approach-y-call.json new file mode 100644 index 000000000..fc85311ed --- /dev/null +++ b/test/fixtures/ceo-approach-y-call.json @@ -0,0 +1,32 @@ +{ + "sessionId": "31619954-929e-453b-9d90-2d371360986c", + "toolUseId": "toolu_01R6C4fAiB68hAsTa95kcQWm", + "questions": [ + { + "question": "D1 \u2014 Which implementation approach for the payment webhook handler? ", + "header": "Approach", + "multiSelect": false, + "options": [ + { + "label": "A) Minimal Viable", + "description": "Fix the SQL injection (parameterized query for userId lookup) and add a rescue/catch block around the email call. Smallest diff, fastest to ship. Completeness: 6/10 \u2014 happy path only, no tests, N+1 query and WebhookDispatcher bypass remain as known debt.\n\n\u2705 Surgical change \u2014 easy to review, easy to revert if the feature flag needs to roll back\n\u2705 Fixes the one non-negotiable blocker: SQL injection. Email errors can no longer cascade to HTTP 500 and trigger Stripe retries\n\u274c Zero automated tests on a payment flow \u2014 regressions will surface in production, not CI\n\u274c N+1 order-fetch loop stays; WebhookDispatcher bypass stays as architectural debt" + }, + { + "label": "B) Proper Integration", + "description": "Everything in A, plus: register the handler through the existing WebhookDispatcher module instead of bypassing it. Completeness: 7/10 \u2014 happy path + architectural correctness, still no test coverage.\n\n\u2705 Eliminates the architectural fragmentation \u2014 future Stripe event types plug into the dispatcher naturally\n\u2705 SQL injection fixed, email bounded \u2014 all of A's must-fixes, plus namespace consistency\n\u274c Still no automated tests for the payment flow; CI cannot catch regressions on this path\n\u274c Requires reading WebhookDispatcher internals before implementing \u2014 slightly wider blast radius" + }, + { + "label": "C) Production-Grade (recommended)", + "description": "Everything in B, plus: unit + integration tests for the handler, fix the N+1 order-fetch with a bulk query, move the notification email to async delivery (queue or background job). Completeness: 10/10.\n\n\u2705 Zero silent failures \u2014 every path is tested, observable, and bounded. Payment flows have zero defect tolerance\n\u2705 The extra scope over A is ~25 min of CC time. With AI coding, \"do it right\" costs nearly the same as \"do it quick\"\n\u274c Async email delivery requires confirming whether a queue or background-job infrastructure already exists\n\u274c Largest diff of the three \u2014 more review surface, though each piece is individually straightforward" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 \u2014 Which implementation approach for the payment webhook handler? ": "A) Minimal Viable" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:06:00.096Z" +} diff --git a/test/fixtures/ceo-approach-y-screen.txt b/test/fixtures/ceo-approach-y-screen.txt new file mode 100644 index 000000000..d8c9f932c --- /dev/null +++ b/test/fixtures/ceo-approach-y-screen.txt @@ -0,0 +1,39 @@ + 10 **Files touched recently:** PLAN.md, CLAUDE.md only. + … +89 lines (ctrl+o to expand) + +● Now I'll present the first decision — approach selection (Step 0C-bis). I've completed internal analysis of 0A, 0B, + and 0C. The plan has a textbook SQL injection on the critical payment path. +──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── +Planning: +/tmp/gstack-paid-shard-38V9gy/tmp/gstack-hermetic-2351339-GfM888/with-skills/.claude/plans/magical-dazzling-waterfall.md +──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── + ☐ Approach + +│ D1 — Which implementation approach for the payment webhook handler? + +❯ 1. A) Minimal Viable + Fix the SQL injection (parameterized query for userId lookup) and add a rescue/catch block around the email call. + Smallest diff, fastest to ship. Completeness: 6/10 — happy path only, no tests, N+1 query and WebhookDispatcher + bypass remain as known debt.��✅ Surgical change — easy to review, easy to revert if the feature flag needs to roll + back�✅ Fixes the one non-negotiable blocker: SQL injection. Email errors can no longer cascade to HTTP 500 and + trigger Stripe retries�❌ Zero automated tests on a payment flow — regressions will surface in production, not + CI�❌ N+1 order-fetch loop stays; WebhookDispatcher bypass stays as architectural debt + 2. B) Proper Integration + Everything in A, plus: register the handler through the existing WebhookDispatcher module instead of bypassing it. + Completeness: 7/10 — happy path + architectural correctness, still no test coverage.��✅ Eliminates the + architectural fragmentation — future Stripe event types plug into the dispatcher naturally�✅ SQL injection fixed, + email bounded — all of A's must-fixes, plus namespace consistency�❌ Still no automated tests for the payment flow; + CI cannot catch regressions on this path�❌ Requires reading WebhookDispatcher internals before implementing — + slightly wider blast radius + 3. C) Production-Grade (recommended) + Everything in B, plus: unit + integration tests for the handler, fix the N+1 order-fetch with a bulk query, move + the notification email to async delivery (queue or background job). Completeness: 10/10.��✅ Zero silent failures — + every path is tested, observable, and bounded. Payment flows have zero defect tolerance�✅ The extra scope over A + is ~25 min of CC time. With AI coding, "do it right" costs nearly the same as "do it quick"�❌ Async email delivery + requires confirming whether a queue or background-job infrastructure already exists�❌ Largest diff of the three — + more review surface, though each piece is individually straightforward + 4. Type something. +──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── + 5. Chat about this + +Enter to select · ↑/↓ to navigate · Esc to cancel diff --git a/test/fixtures/ceo-handoff-y-call.json b/test/fixtures/ceo-handoff-y-call.json new file mode 100644 index 000000000..25e70fb2c --- /dev/null +++ b/test/fixtures/ceo-handoff-y-call.json @@ -0,0 +1,218 @@ +{ + "calls": [ + { + "sessionId": "31619954-929e-453b-9d90-2d371360986c", + "toolUseId": "toolu_01R6C4fAiB68hAsTa95kcQWm", + "questions": [ + { + "question": "D1 \u2014 Which implementation approach for the payment webhook handler? ", + "header": "Approach", + "multiSelect": false, + "options": [ + { + "label": "A) Minimal Viable", + "description": "Fix the SQL injection (parameterized query for userId lookup) and add a rescue/catch block around the email call. Smallest diff, fastest to ship. Completeness: 6/10 \u2014 happy path only, no tests, N+1 query and WebhookDispatcher bypass remain as known debt.\n\n\u2705 Surgical change \u2014 easy to review, easy to revert if the feature flag needs to roll back\n\u2705 Fixes the one non-negotiable blocker: SQL injection. Email errors can no longer cascade to HTTP 500 and trigger Stripe retries\n\u274c Zero automated tests on a payment flow \u2014 regressions will surface in production, not CI\n\u274c N+1 order-fetch loop stays; WebhookDispatcher bypass stays as architectural debt" + }, + { + "label": "B) Proper Integration", + "description": "Everything in A, plus: register the handler through the existing WebhookDispatcher module instead of bypassing it. Completeness: 7/10 \u2014 happy path + architectural correctness, still no test coverage.\n\n\u2705 Eliminates the architectural fragmentation \u2014 future Stripe event types plug into the dispatcher naturally\n\u2705 SQL injection fixed, email bounded \u2014 all of A's must-fixes, plus namespace consistency\n\u274c Still no automated tests for the payment flow; CI cannot catch regressions on this path\n\u274c Requires reading WebhookDispatcher internals before implementing \u2014 slightly wider blast radius" + }, + { + "label": "C) Production-Grade (recommended)", + "description": "Everything in B, plus: unit + integration tests for the handler, fix the N+1 order-fetch with a bulk query, move the notification email to async delivery (queue or background job). Completeness: 10/10.\n\n\u2705 Zero silent failures \u2014 every path is tested, observable, and bounded. Payment flows have zero defect tolerance\n\u2705 The extra scope over A is ~25 min of CC time. With AI coding, \"do it right\" costs nearly the same as \"do it quick\"\n\u274c Async email delivery requires confirming whether a queue or background-job infrastructure already exists\n\u274c Largest diff of the three \u2014 more review surface, though each piece is individually straightforward" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 \u2014 Which implementation approach for the payment webhook handler? ": "A) Minimal Viable" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:06:00.096Z" + }, + { + "sessionId": "31619954-929e-453b-9d90-2d371360986c", + "toolUseId": "toolu_013jMk3y5mGVALW5LCN71WN8", + "questions": [ + { + "question": "D2 \u2014 Which review mode should we apply to Approach A (Minimal Viable)? ", + "header": "Review mode", + "multiSelect": false, + "options": [ + { + "label": "HOLD SCOPE (recommended)", + "description": "The scope of Approach A is accepted as-is. Review it with maximum rigor \u2014 make the SQL fix bulletproof, ensure the email rescue is correctly scoped, verify rollout/rollback safety, and map every failure mode within that minimal diff. No expansions surfaced.\n\nNote: options differ in kind, not coverage \u2014 no completeness score.\n\n\u2705 Focused, fast review \u2014 no decision fatigue from expansion proposals\n\u2705 Right for a surgical security patch: get it right, get it out\n\u274c Won't surface adjacent improvements (N+1, tests, dispatcher) unless they appear as blockers" + }, + { + "label": "SELECTIVE EXPANSION", + "description": "Hold Approach A as the baseline, review it rigorously, but also surface each expansion opportunity (tests, N+1 fix, dispatcher integration) as individual opt-in decisions. You cherry-pick what to include. Neutral recommendation posture.\n\nNote: options differ in kind, not coverage \u2014 no completeness score.\n\n\u2705 Best of both \u2014 you see what you\u2019re leaving on the table and decide case-by-case\n\u2705 If one expansion is easy to include (e.g. a single bulk-query fix for N+1), you can add it without committing to full scope\n\u274c More questions to answer before the review completes; slightly longer session" + }, + { + "label": "SCOPE REDUCTION", + "description": "Strip Approach A down even further \u2014 find the absolute minimum that unblocks safe shipping. Some of even A's scope might be cuttable.\n\nNote: options differ in kind, not coverage \u2014 no completeness score.\n\n\u2705 Fastest path to a shippable diff\n\u274c Risks leaving known issues (like email error propagation) in place if they\u2019re deemed non-blocking" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 \u2014 Which review mode should we apply to Approach A (Minimal Viable)? ": "HOLD SCOPE (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:06:28.178Z" + }, + { + "sessionId": "31619954-929e-453b-9d90-2d371360986c", + "toolUseId": "toolu_01MqvHUhmD1NJXVLaL1Q4SYh", + "questions": [ + { + "question": "D3 \u2014 S2-1: Email rescue scope \u2014 how should failures on the notification leg be handled? ", + "header": "Email rescue", + "multiSelect": false, + "options": [ + { + "label": "A) Rescue + log + return 200 (recommended)", + "description": "Wrap the inline email call in a specific rescue block (catching mail delivery, SMTP, and timeout exceptions \u2014 not a catch-all). Log the failure with the user ID and event ID for correlation. Return HTTP 200 to Stripe so it does not retry a successfully processed payment.\n\n\u2705 Correctly models the situation: payment committed, notification deferred. HTTP 500 would misrepresent a succeeded payment.\n\u2705 Stops the Stripe retry storm during email outages; the existing mail-client dashboard alert still fires so on-call sees the failure\n\u274c Requires naming the specific exception classes for the mail client in use (a one-line lookup)\n\u274c A missed exception class would still propagate \u2014 the specific-exception approach is more robust than catch-all but not foolproof" + }, + { + "label": "B) Let it propagate (no rescue)", + "description": "Leave the email leg unrescued per the current plan. Stripe HTTP 500 triggers retries; the dedup guard prevents double-payment; the runbook handles email retry manually.\n\n\u2705 Zero new code \u2014 strictest interpretation of Approach A \u2018minimal\u2019\n\u274c HTTP 500 misrepresents a committed payment to Stripe; each retry re-attempts the email, creating a storm during outages\n\u274c Pollutes the ingress failure alert with what is actually a notification failure, not a payment failure" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 \u2014 S2-1: Email rescue scope \u2014 how should failures on the notification leg be handled? ": "A) Rescue + log + return 200 (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:09:46.518Z" + }, + { + "sessionId": "31619954-929e-453b-9d90-2d371360986c", + "toolUseId": "toolu_01VmgryoorPh1RgBmXyrVRhW", + "questions": [ + { + "question": "D4 - S3-1: SQL injection remedy - how comprehensive should the fix be? ", + "header": "SQL injection fix", + "multiSelect": false, + "options": [ + { + "label": "A) Parameterized query + format guard (recommended)", + "description": "Switch raw SQL to a parameterized query (userId as a bound parameter, not interpolated). Also add a format guard rejecting malformed userId values before they reach the DB. Same method, ~2-3 lines total.\n\nDefense-in-depth: parameterized query blocks injection at the DB driver level; format guard rejects obviously malformed inputs before DB round-trip.\n\n[+] Eliminates injection at both handler and driver levels - right posture for a payment path\n[+] Format guard is 1-2 lines; CC cost is negligible\n[-] Requires knowing the userId column type (UUID? integer?) - a 1-minute schema check" + }, + { + "label": "B) Parameterized query only", + "description": "Switch raw SQL to a parameterized query. No format guard - trust the existing adapter to handle edge cases.\n\n[+] Minimal diff, directly fixes the named blocker\n[-] A crafted non-UUID/non-integer userId would still reach the DB (wasted query + potential error handling gap)\n[-] Slightly less defense-in-depth for a payment-sensitive path" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 - S3-1: SQL injection remedy - how comprehensive should the fix be? ": "A) Parameterized query + format guard (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:10:32.222Z" + }, + { + "sessionId": "31619954-929e-453b-9d90-2d371360986c", + "toolUseId": "toolu_01NjCwY9cWyt6aNTmU2BoEco", + "questions": [ + { + "question": "D5 - S6-1 (TODO): Add handler tests to TODOS.md? ", + "header": "Tests TODO", + "multiSelect": false, + "options": [ + { + "label": "A) Add to TODOS.md (recommended)", + "description": "P2 TODO: write tests for StripePaymentWebhookHandler covering the happy path, not-found user, email failure rescue, and malformed userId format guard. CI cannot catch regressions on any of these behaviors without dedicated tests.\n\n[+] Makes the debt explicit and traceable\n[+] Payment flows are zero-defect territory; backlog is better than implicit gap\n[-] Adds a backlog item without a committed timeline" + }, + { + "label": "B) Skip - not valuable enough", + "description": "Trust the existing integration suite. No TODO added.\n\n[+] No backlog noise\n[-] Regressions in the rescue block or format guard will surface as production payment failures, not CI failures" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D5 - S6-1 (TODO): Add handler tests to TODOS.md? ": "A) Add to TODOS.md (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:11:48.109Z" + }, + { + "sessionId": "31619954-929e-453b-9d90-2d371360986c", + "toolUseId": "toolu_01QUogHfHX6FM7Gc2HYqgQym", + "questions": [ + { + "question": "D6 - S7-1 (TODO): Add N+1 order-fetch fix to TODOS.md? ", + "header": "N+1 TODO", + "multiSelect": false, + "options": [ + { + "label": "A) Add to TODOS.md (recommended)", + "description": "P2 TODO: replace the per-order loop with a single bulk query (WHERE user_id = ? or WHERE id IN (...)). The plan's own Performance section flags this. At high payment volume or during reconciliation batches, the N+1 pattern creates DB connection pool pressure.\n\n[+] The plan already named this as a known issue; capturing it formally closes the loop\n[+] A single bulk query is a ~5-minute CC fix; having it on the backlog accelerates pickup\n[-] No immediate impact at low volume; deferring is reasonable" + }, + { + "label": "B) Skip - not worth logging", + "description": "Leave N+1 as an implicit known issue. Don't add the TODO.\n\n[+] No backlog noise\n[-] The next engineer who touches the handler won't have context that this was an intentional deferral vs. an oversight" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D6 - S7-1 (TODO): Add N+1 order-fetch fix to TODOS.md? ": "A) Add to TODOS.md (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:11:56.141Z" + }, + { + "sessionId": "31619954-929e-453b-9d90-2d371360986c", + "toolUseId": "toolu_01NdD5vRTuAJbN8GqxfJD7aZ", + "questions": [ + { + "question": "D7 - Next step: run /plan-eng-review? ", + "header": "Next review", + "multiSelect": false, + "options": [ + { + "label": "A) Run /plan-eng-review next (recommended)", + "description": "Eng Review is the required shipping gate. It covers architecture depth, code quality, test gaps, and performance - complementing what this CEO review found. Since this CEO review expanded the plan (added parameterized query + email rescue requirements), a fresh eng review is especially valuable.\n\n[+] Required before shipping; catches implementation issues the plan-level review cannot\n[+] This CEO review found critical issues (SQL injection) - eng review will verify the fix approach is architecturally sound\n[-] Adds another review session before implementation starts" + }, + { + "label": "B) Skip - I'll handle reviews manually", + "description": "Proceed directly to implementation with the approved changes from this CEO review.\n\n[+] Faster path to implementation\n[-] Eng review is the required shipping gate - skipping it means less confidence before enabling the feature flag" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D7 - Next step: run /plan-eng-review? ": "A) Run /plan-eng-review next (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:15:38.513Z" + } + ], + "planReadyRequests": [ + { + "sessionId": "31619954-929e-453b-9d90-2d371360986c", + "toolUseId": "toolu_01WAkbvMarZ4Lg5dite638pC", + "timestamp": "2026-09-09T11:16:01.443Z", + "failed": false + } + ], + "report": "# CEO Plan Review: Payment Processing Integration\n\n_Generated by /plan-ceo-review | Branch: main | Mode: TBD (pending Step 0F)_\n\n---\n\n## PRE-REVIEW SYSTEM AUDIT\n\n**Repo state:** Fresh fixture repo \u2014 one commit (\"Seed review plan\"), no stash, no TODOS.md, no TODOs in code.\n**Files touched recently:** PLAN.md, CLAUDE.md only.\n**Design doc:** None found.\n**Handoff note:** None found.\n**Prior learnings:** None.\n**Brain context:** Cold start \u2014 no prior sessions.\n\n---\n\n## 0A. Premise Challenge\n\n**Is this the right problem?** Yes. Stripe webhook handling is the canonical way to reliably receive payment confirmation events. The problem statement is correct.\n\n**Direct path to outcome?** Directionally yes \u2014 a handler class that processes `payment_intent.succeeded` and updates the user record + sends a notification is the right shape. However, the implementation as described has a critical security flaw that makes it unshippable.\n\n**What if we did nothing?** Users would not receive payment status updates or notification emails after successful payments. Real business impact.\n\n**Verdict:** Correct problem, wrong implementation. The plan's scope is sound; its execution plan needs significant correction.\n\n---\n\n## 0B. Existing Code Leverage\n\n| Sub-problem | Existing code | Plan's use |\n|---|---|---|\n| Webhook routing | `WebhookDispatcher` module | BYPASSED \u2014 new class creates parallel namespace |\n| DB lookup/update | Existing DB client (with tracing) | Reused \u2014 good |\n| Email notification | Existing mail client (with tracing) | Reused \u2014 good |\n| Signature verification | Ingress middleware | Retained \u2014 good |\n| Event deduplication | Webhook event guard | Retained \u2014 good |\n| Per-user locking | Transaction lock | Retained \u2014 good |\n| SQL-safe queries | Parameterized query infrastructure (presumably exists) | NOT USED \u2014 raw SQL fragment instead |\n\n**Key leverage gap:** The plan bypasses WebhookDispatcher without explaining why. \"Clean namespace separation\" is stated as the reason, but the dispatcher presumably already provides namespace separation \u2014 this needs justification.\n\n---\n\n## 0C. Dream State (12-month arc)\n\n```\nCURRENT STATE THIS PLAN (as written) 12-MONTH IDEAL\n\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\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\nNo payment webhook handler StripePaymentWebhookHandler Secure, tested payment flow:\n \u00b7 SQL injection vulnerability \u00b7 Parameterized queries\n \u00b7 No error handling on email \u00b7 Tested handler (unit + integration)\n \u00b7 Zero tests for payment flow \u00b7 Async email (queue-backed)\n \u00b7 N+1 order-fetch loop \u00b7 Bulk order query (no N+1)\n \u00b7 Bypasses WebhookDispatcher \u00b7 Integrated with dispatcher\n \u00b7 Manual verification only \u00b7 CI coverage on happy + edge paths\n```\n\nThis plan moves in the right direction but ships a version that will require emergency patches immediately after launch.\n\n---\n\n## 0C-bis. Implementation Approaches\n\n_Pending user selection \u2014 see D1 AskUserQuestion_\n\n---\n\n## 0D. Mode Analysis\n\n_Pending mode selection \u2014 see D2 AskUserQuestion_\n\n---\n\n## CRITICAL ISSUES INVENTORY\n\n| # | Severity | Issue | Location in Plan |\n|---|---|---|---|\n| 1 | CRITICAL | SQL injection: `userId` external string \u2192 raw SQL fragment | \"Database access\" section |\n| 2 | HIGH | Email failures cascade to HTTP 500 \u2192 Stripe retries | \"Webhook fan-out\" + \"Existing contracts\" |\n| 3 | HIGH | Zero tests for payment flow | \"Tests\" section |\n| 4 | MEDIUM | N+1 query: orders fetched in a loop per webhook | \"Performance\" section |\n| 5 | MEDIUM | Bypasses WebhookDispatcher without justification | \"Architecture\" section |\n\n---\n\n## Step 0 Decisions\n- **Approach:** A \u2014 Minimal Viable (parameterized query for userId + rescue block on email)\n- **Mode:** HOLD SCOPE \u2014 maximum rigor on the accepted scope, no expansions\n\n---\n\n## Section 1: Architecture Review\n\n```\nSTRIPE \u2500\u2500\u25b6 [Ingress Middleware]\n \u2502\n \u25bc\n [Signature Verification] \u2500\u2500\u2717\u2500\u2500 (invalid sig \u2192 4xx, rejected)\n \u2502\n \u25bc (valid, payment_intent.succeeded only)\n [Payload Adapter]\n \u00b7 exposes event.data.object.metadata.user_id as request.params.userId\n \u00b7 does NOT SQL-sanitize; adapter warns + 200 on missing/empty userId\n \u2502\n \u25bc\n [Webhook Event Guard] \u2500\u2500(duplicate event ID?)\u2500\u2500\u25b6 HTTP 200 (deduped)\n \u2502 (first delivery)\n \u25bc\n [Per-User Transaction Lock]\n \u2502\n \u25bc\n StripePaymentWebhookHandler#handle \u25c0\u2500\u2500 NEW (bypasses WebhookDispatcher)\n \u2502\n \u250c\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2534\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\u2510\n \u25bc \u2502\n [DB: SELECT user WHERE id = {userId}] \u2502 (CRITICAL: raw SQL fragment,\n \u2502 \u2502 no parameterized query)\n [user found?] \u2502\n / \\ \u2502\n[yes] [no/deleted] \u2502\n \u2502 \u2502 \u2502\n \u2502 HTTP 200 + log \u2502\n \u25bc \u2502\n[DB: UPDATE user SET payment_status=paid] \u2502\n \u2502 \u2502\n \u25bc \u2502\n[Email: send notification] \u2500\u2500(exception?)\u2500\u2500\u25b6 HTTP 500\n \u2502 (propagates to ingress; Stripe retries)\n \u25bc\nHTTP 200\n```\n\n**Data flow paths for userId:**\n- Happy path: valid userId \u2192 user found \u2192 update + email \u2192 HTTP 200\n- Nil/empty: handled upstream by adapter \u2192 HTTP 200 + warning (handler not invoked)\n- SQL payload: NOT handled \u2192 raw SQL execution \u2192 **CRITICAL GAP**\n- DB error on lookup: propagates to ingress \u2192 HTTP 500 \u2192 Stripe retries (acceptable per design)\n\n**Single points of failure:** Email delivery (no rescue) \u2014 failure cascades to HTTP 500 and Stripe retries.\n\n**Rollback posture:** Feature flag + documented rollout checklist. Rollback = feature flag toggle. \u2713\n\n**Architectural note (not a blocker in Approach A):** `StripePaymentWebhookHandler` bypasses the existing `WebhookDispatcher` module. The stated reason (\"clean namespace separation\") is insufficient justification \u2014 the dispatcher likely already provides namespace separation. This creates two parallel registration paths for webhook handlers. Not in scope for Approach A, but deferred to TODO.\n\n**Section 1 verdict:** No blocking architectural findings within Approach A scope. One critical data-flow gap (SQL injection, covered in Section 3). One deferred architectural concern (dispatcher bypass).\n\n---\n\n## Section 2: Error & Rescue Map\n\n```\nMETHOD/CODEPATH | WHAT CAN GO WRONG | EXCEPTION CLASS\n----------------------------------|--------------------------------|------------------\nStripePaymentWebhookHandler | userId injected SQL payload | (varies \u2014 attacker-controlled)\n #handle \u2014 DB lookup | DB connection failure | ConnectionError / DBError\n | Query returns no rows | \u2192 nil (handled by existing guard)\n | |\nStripePaymentWebhookHandler | DB connection failure | ConnectionError / DBError\n #handle \u2014 DB update | Constraint violation | ConstraintError / DBError\n | |\nStripePaymentWebhookHandler | SMTP failure / timeout | MailDeliveryError / SMTPError\n #handle \u2014 email leg | Email provider returns 5xx | DeliveryError\n | Network timeout | TimeoutError\n```\n\n```\nEXCEPTION CLASS | RESCUED? | RESCUE ACTION | USER/STRIPE SEES\n--------------------------|----------|--------------------------|------------------\nSQL injection exception | No \u2190 GAP | N/A (attacker wins) | Data breach / 500 / silent\nDB connection error | No | Propagates to ingress | HTTP 500, Stripe retries \u2190 OK by design\nnil user (not found) | YES | Existing guard: 200+log | HTTP 200 \u2190 OK\nEmail: DeliveryError | No \u2190 GAP | Propagates to ingress | HTTP 500, Stripe retries \u2190 PROBLEM\nEmail: TimeoutError | No \u2190 GAP | Propagates to ingress | HTTP 500, Stripe retries \u2190 PROBLEM\nEmail: SMTPError | No \u2190 GAP | Propagates to ingress | HTTP 500, Stripe retries \u2190 PROBLEM\n```\n\n**Gaps:**\n1. SQL injection \u2014 fix via parameterized query (Section 3 finding)\n2. Email errors \u2014 no rescue block; transient email failures cause HTTP 500 and unnecessary Stripe retries even though the payment update committed successfully\n\n**Why email rescue matters even with the dedup guard:** The dedup guard records completion \"after the database transaction commits.\" If the email fails AFTER the DB commits, the dedup may or may not have recorded completion depending on timing. If it hasn't, Stripe retries the full handler. The payment update is idempotent (same values), so no double-charge risk \u2014 but each retry re-sends the email (or fails again), creating a retry storm that pollutes logs and could exhaust Stripe's retry budget before the email service recovers.\n\n**S2-1 APPROVED (D3):** Rescue + log + return 200. Specific exception classes (not catch-all). Log userId + eventId before swallowing.\n\n---\n\n## Section 3: Security & Threat Model\n\n**Attack surface expansion:** One new codepath \u2014 `StripePaymentWebhookHandler#handle`. Callable only by Stripe (signature-verified at ingress). However, the Stripe signature proves the webhook came from Stripe; it does NOT prove the metadata inside the payload is safe for SQL execution.\n\n```\nTHREAT | LIKELIHOOD | IMPACT | MITIGATED?\n-----------------------|------------|----------|------------------\nSQL injection via | HIGH | CRITICAL | NO \u2190 CRITICAL GAP\n userId metadata | | |\n (attacker controls | | |\n Stripe metadata) | | |\n | | |\nEmail header injection | LOW | Medium | Likely (mail client sanitizes),\n via userId in email | | | but not stated in plan\n | | |\nIDOR (user A accesses | N/A | N/A | Not applicable \u2014 all users share\n user B's data) | | | one webhook URL; handler uses the\n | | | event's own userId\n```\n\n**SQL injection \u2014 full analysis:**\n\nThe plan states explicitly (verbatim from \"Existing contracts retained\"):\n> \"For every nonempty external string it performs no SQL-format validation. The adapter forwards that external string unchanged. It does not cast, escape, or SQL-sanitize it; a valid signature does not make it safe for SQL.\"\n\nAnd from \"Database access\":\n> \"The new endpoint reads `request.params.userId` directly into a raw SQL fragment for the lookup query.\"\n\nThis is not a theoretical risk. An attacker who:\n1. Has a valid Stripe account\n2. Creates a payment intent with crafted `metadata.user_id`\n3. Receives a `payment_intent.succeeded` event (requires completing a payment via Stripe's test mode or a real charge)\n\n...can inject arbitrary SQL into the lookup query. The Stripe signature is valid (Stripe sent it), so the ingress middleware passes it through.\n\n**Remedy:** Replace the raw SQL fragment with a parameterized query. The userId value must be passed as a query parameter, not string-interpolated into SQL. Most ORMs/DB clients support this natively.\n\n```\nBEFORE (vulnerable):\n db.execute(\"SELECT * FROM users WHERE id = '#{request.params.userId}'\")\n # or: \"SELECT * FROM users WHERE id = \" + userId\n\nAFTER (safe):\n db.execute(\"SELECT * FROM users WHERE id = ?\", [request.params.userId])\n # or ORM equivalent: User.find_by(id: request.params.userId)\n```\n\nAdditionally: the plan does not state whether `userId` is validated as a UUID/integer before the query. Even with parameterized queries, passing a 50KB string as a userId to a production DB is wasteful. A format guard (UUID regex or integer cast with a fixed max length) before the query would reject obviously malformed inputs fast-fail at the handler level.\n\n**S3-1 + S3-2 APPROVED (D4):** Parameterized query + format guard (UUID regex or integer cast, depending on schema). Both in the same handler method.\n\n---\n\n## Section 4: Data Flow & Interaction Edge Cases\n\n```\nuserId \u2500\u2500\u25b6 [Format guard] \u2500\u2500\u25b6 [Parameterized query: SELECT user] \u2500\u2500\u25b6 [user found?]\n \u2502 \u2502 \u2502 / \\\n[nil?] [invalid format] [DB error] [yes] [no]\n \u2514\u2500\u2500\u25b6 adapter \u2514\u2500\u2500\u25b6 early return \u2514\u2500\u2500\u25b6 propagates [UPDATE] HTTP 200\n 200+warn + log to ingress [Email]------->200 (approved)\n HTTP 500 rescue (approved)\n```\n\nShadow paths for the lookup (all four traced):\n- Nil/empty userId: blocked by adapter upstream \u2014 handler never invoked. OK.\n- Malformed userId (after fix): format guard rejects early with structured log. OK.\n- DB connection failure: propagates to ingress, HTTP 500, Stripe retries. By design. OK.\n- Valid userId, user not found/deleted: existing guard, HTTP 200 + log. OK.\n\nAsync ordering: No async state shared beyond the per-user transaction lock (existing). Email is inline (not async). No ordering concerns beyond what the lock already guarantees.\n\n**Section 4 verdict:** No new findings within Approach A scope.\n\n---\n\n## Section 5: Code Quality Review\n\nIssues within Approach A scope:\n- Raw SQL fragment (fix approved: D4 parameterized query + guard)\n- No email rescue (fix approved: D3)\n\nIssues outside Approach A scope (not in scope but noted):\n- `StripePaymentWebhookHandler` bypasses `WebhookDispatcher` \u2014 creates a parallel registration path\n- N+1 order-fetch loop (performance, defer to TODO)\n\nNo new findings beyond what Sections 2-3 captured.\n\n**Section 5 verdict:** No new findings. Prior findings covered by approved remedies.\n\n---\n\n## Section 6: Test Review\n\nNew things this plan introduces:\n```\nNEW CODEPATHS:\n - StripePaymentWebhookHandler#handle (happy path)\n - userId format guard (early return on malformed input)\n - Parameterized user lookup\n - User record update (existing pattern, but now reached via new path)\n - Email notification call (inline)\n - Email rescue block (new, from D3 approval)\n\nNEW ERROR/RESCUE PATHS:\n - Email delivery failure \u2192 rescued \u2192 log + HTTP 200\n - Malformed userId \u2192 format guard \u2192 early return + log\n```\n\n**Test coverage in plan: NONE.** (\"We'll rely on the existing integration suite.\")\n\nCoverage the existing integration suite likely provides:\n- Signature verification (ingress middleware \u2014 existing)\n- Event deduplication (existing guard \u2014 existing)\n\nCoverage the existing suite does NOT provide (for this new handler):\n- Happy path: valid userId + payment_intent.succeeded \u2192 user updated + email queued\n- Not-found user: valid userId maps to no user \u2192 HTTP 200 (no update, no email)\n- Email failure path: DB commits, email throws \u2192 rescue \u2192 HTTP 200 + log\n- Malformed userId: format guard rejects \u2192 HTTP 200 (or structured error)\n- SQL injection attempt: parameterized query rejects safely\n\nWithout handler-level tests, regressions in any of these paths surface as failed Stripe webhooks in production, not failed CI builds.\n\n_Finding S6-1: Zero tests for new payment handler \u2014 pending D5 (TODO)_\n\n---\n\n## Section 7: Performance Review\n\n**N+1 query** (explicitly called out in plan's \"Performance\" section):\n\"Each webhook lookup hits the database for the user, then fetches each order in a loop.\"\n\n```\nwebhook \u2500\u2500\u25b6 SELECT * FROM users WHERE id = ? (1 query)\n \u2500\u2500\u25b6 foreach order: SELECT * FROM orders WHERE id = ? (N queries)\n```\n\nAt high payment volume (e.g., flash sale, batch reconciliation), N+1 for orders means:\n- 1,000 webhooks \u00d7 10 orders each = 10,001 DB queries in a short window\n- Under sustained load, this pressure on the connection pool could cascade\n\nFix: Bulk query the orders in a single `WHERE user_id = ?` or `WHERE id IN (...)` after the user lookup.\n\nWithin Approach A scope: explicitly deferred. Should be logged as a TODO.\n\n_Finding S7-1: N+1 order-fetch per webhook \u2014 pending D6 (TODO)_\n\n---\n\n## Section 8: Observability & Debuggability Review\n\nExisting observability (retained):\n- Ingress logs event IDs, outcomes, durations (good)\n- DB + mail clients attach userId and eventId to outcome traces (good)\n- Mail client publishes delivery failure rate to dashboard + on-call alert (good)\n- Incident runbook exists for DB/mail failure correlation (good)\n\nNew observability requirements (within Approach A):\n- Email rescue block MUST log with structured context: `{userId, eventId, exception_class, exception_message}` \u2014 not just a generic \"email failed\" message. This was included in the D3 remedy approval.\n- Format guard early return MUST log with `{userId_received, reason, eventId}` so on-call can distinguish malformed metadata from a legitimate delivery failure.\n\nNo new dashboards or alerts needed \u2014 the existing mail delivery failure dashboard covers email failures.\n\n**Section 8 verdict:** No new findings beyond requirements covered by approved D3/D4 remedies.\n\n---\n\n## Section 9: Deployment & Rollout Review\n\nPer plan: handler feature flag exists. Rollout checklist requires staging replay + verification. Rollback = feature flag toggle.\n\n- DB migrations: none \u2014 this handler doesn't change the schema. Zero-downtime by default.\n- Feature flag: existing, tested. Good.\n- Staging verification: manual checklist (payment-event replay, user update check, email delivery check, correlated outcome trace). No automated smoke tests planned.\n- Rollback: feature flag toggle. Immediate. Reversibility: 5/5.\n- Deploy-time risk window: old handler and new handler are feature-flag separated. No simultaneous-execution risk.\n\n**Section 9 verdict:** No blocking findings. Deployment posture is sound given the existing infrastructure.\n\n---\n\n## Section 10: Long-Term Trajectory Review\n\n**Technical debt introduced by Approach A:**\n- Testing debt: no automated tests for the payment handler (P2 \u2014 evidenced operability gap)\n- Performance debt: N+1 order query (P2 \u2014 explicitly documented in plan's Performance section)\n- Architectural debt: WebhookDispatcher bypass (P3 \u2014 correctness not affected, future handler registration more fragmented)\n\n**Reversibility:** 5/5 \u2014 feature flag + no schema changes.\n\n**Knowledge concentration:** The incident runbook and manual rollout checklist are the operational documentation. No inline code comments mentioned. A new engineer in 12 months would need to read the runbook to understand why email failures are caught and not re-raised.\n\n**Path dependency:** The WebhookDispatcher bypass doesn't block future development but creates a fork in how webhook handlers are registered. If a second new event type is added, the author may not discover the dispatcher and create another parallel path.\n\n**Section 10 verdict:** No new blocking findings. Debt items are known and scoped. Two TODO candidates (tests, N+1).\n\n---\n\n## Section 11: Design & UX Review\n\nNo UI scope detected. Skipped.\n\n---\n\n## Outside Voice\n\nCodex review skipped (codex_reviews disabled). Re-enable: `gstack-config set codex_reviews enabled`\n\n---\n\n## Required Outputs\n\n### NOT in Scope\n\n| Item | Rationale |\n|---|---|\n| WebhookDispatcher integration | Approach A (Minimal Viable) \u2014 user deferred this architectural cleanup |\n| Automated test suite for handler | Approach A explicitly deferred tests; logged as P2 TODO |\n| N+1 order-fetch optimization | Approach A explicitly deferred performance; logged as P2 TODO |\n| Async email delivery | Approach A scope: inline with rescue is sufficient to unblock shipping |\n\n### What Already Exists\n\n| Sub-problem | Existing code | Plan's use |\n|---|---|---|\n| Webhook routing | WebhookDispatcher module | Bypassed (new class) |\n| DB client (traced) | Shared DB client | Reused |\n| Mail client (traced) | Shared mail client | Reused |\n| Signature verification | Ingress middleware | Retained |\n| Event deduplication | Webhook event guard | Retained |\n| Per-user locking | Transaction lock | Retained |\n| Event/outcome logging | Ingress wrapper | Retained |\n| Mail failure dashboard | Existing dashboard | Retained |\n| Incident runbook | Existing runbook | Retained |\n| Feature flag + rollout checklist | Existing deployment tooling | Retained |\n\n### Dream State Delta\n\nAfter Approach A ships:\n- SQL injection: CLOSED\n- Email silent failure: CLOSED (rescue + log)\n- Automated tests: OPEN (TODO: P2)\n- N+1 order query: OPEN (TODO: P2)\n- WebhookDispatcher integration: OPEN (architectural debt)\n\nDistance to 12-month ideal: ~40% of the way there. The critical security and reliability issues are addressed. Performance and test coverage remain as explicit known gaps.\n\n### Error & Rescue Registry\n\n```\nMETHOD/CODEPATH | WHAT CAN GO WRONG | EXCEPTION CLASS\n----------------------------------|--------------------------------|------------------\nHandler#handle: format guard | malformed userId | early return (no exception)\nHandler#handle: DB lookup | DB connection failure | DBConnectionError\nHandler#handle: DB lookup | Query returns no rows | -> nil (existing guard)\nHandler#handle: DB update | Connection / constraint fail | DBError\nHandler#handle: email leg | SMTP/delivery/timeout failure | MailDeliveryError / TimeoutError\n\nEXCEPTION CLASS | RESCUED? | RESCUE ACTION | STRIPE SEES\n----------------------------------|----------|-----------------------------|------------------\nMalformed userId (format guard) | N/A | Early return + log | HTTP 200\nDBConnectionError (lookup/update) | No | Propagates to ingress | HTTP 500 (retries)\nnil user (not found) | YES | Existing guard: log | HTTP 200\nMailDeliveryError / TimeoutError | YES* | Rescue + log + HTTP 200 | HTTP 200\n```\n\n(*) Rescue block added per D3 approval.\n\n### Failure Modes Registry\n\n```\nCODEPATH | FAILURE MODE | RESCUED? | TEST? | STRIPE SEES | LOGGED?\n----------------------|----------------------|----------|-------|-----------------|--------\nFormat guard | Malformed userId | N/A | No* | HTTP 200 | Yes (D4)\nDB lookup | Connection fail | No | No* | HTTP 500 | Yes (ingress)\nDB lookup | User not found | Yes | No* | HTTP 200 | Yes (existing)\nDB update | Connection fail | No | No* | HTTP 500 | Yes (ingress)\nEmail leg | Delivery fail | Yes (D3) | No* | HTTP 200 | Yes (D3+mail)\nSQL injection | Was: injection | FIXED(D4)| No* | N/A | N/A\n```\n\n(*) Tests: all rows are No until the P2 TODO (D5) is addressed. No rows have RESCUED=No AND STRIPE=Silent \u2014 the closest is DB failures which propagate to the ingress wrapper (which logs and returns 500 for Stripe retry, not silent).\n\nNo CRITICAL GAPS remain after D3/D4 remedies: the SQL injection is fixed (parameterized query), and the email failure is rescued (log + HTTP 200). DB failures correctly returning HTTP 500 for Stripe retry is the designed behavior.\n\n### TODOS.md Updates\n\nPer D5 and D6 approvals:\n\n**TODO-1 (P2): StripePaymentWebhookHandler test coverage**\n- What: Write tests for the handler covering happy path, not-found user, email failure rescue, and malformed userId format guard\n- Why: CI cannot catch regressions on any of these behaviors without dedicated tests; payment flows require zero-defect confidence\n- Context: Handler was added in Approach A (minimal viable). Tests were deferred to keep the security fix diff small. The handler has 4 distinct behaviors, all untested.\n- Effort: M (human: ~1 day) / CC: ~15min\n- Priority: P2\n- Depends on: None (handler is complete after Approach A lands)\n\n**TODO-2 (P2): Fix N+1 order-fetch in StripePaymentWebhookHandler**\n- What: Replace the per-order DB query loop with a single bulk query (WHERE user_id = ? or WHERE id IN (...))\n- Why: At high payment volume, N orders per webhook creates DB connection pool pressure; already documented in plan's Performance section\n- Context: Known issue per plan. Approach A deferred this explicitly. At low volume it's benign; at scale it's a bottleneck.\n- Effort: S (human: ~30min) / CC: ~5min\n- Priority: P2\n- Depends on: None\n\n### Diagrams\n\nSystem architecture: see Section 1 (ASCII diagram).\nData flow shadow paths: see Section 4 (ASCII diagram).\nError flow: see Error & Rescue Registry above.\nState machine: N/A (no new stateful objects).\nDeployment sequence: feature flag toggle + existing rollout checklist. No new diagram needed.\nRollback: feature flag toggle. Immediate. No diagram needed.\n\nStale diagram audit: no ASCII diagrams in existing codebase files to audit (fixture repo).\n\n---\n\n## Implementation Tasks\n\nSynthesized from this review. Run with Claude Code or Codex; checkbox as you ship.\n\n- [ ] **T1 (P1, human: ~1h / CC: ~10min)** - StripePaymentWebhookHandler - Fix SQL injection with parameterized query + userId format guard\n - Surfaced by: Section 3 (Security) - raw SQL fragment with unsanitized external string\n - Files: handler file containing `StripePaymentWebhookHandler#handle`\n - Verify: security test attempting SQL metacharacter injection; confirm ORM/driver uses bound parameters\n\n- [ ] **T2 (P1, human: ~30min / CC: ~5min)** - StripePaymentWebhookHandler - Add email rescue block (log + HTTP 200)\n - Surfaced by: Section 2 (Error/Rescue Map) - email exceptions propagate to HTTP 500 for committed payments\n - Files: handler file, email leg of `#handle`\n - Verify: unit test that mocks email delivery failure and asserts HTTP 200 response and structured log line\n\n---\n\n## Completion Summary\n\n```\n+====================================================================+\n| MEGA PLAN REVIEW - COMPLETION SUMMARY |\n+====================================================================+\n| Mode selected | HOLD SCOPE |\n| System Audit | clean fixture repo, 1 commit |\n| Step 0 | Approach A (Minimal Viable), HOLD SCOPE |\n| Section 1 (Arch) | 0 blocking issues (bypass noted, deferred) |\n| Section 2 (Errors) | 2 error paths mapped, 1 GAP (email rescue) |\n| | - email rescue APPROVED (D3) |\n| Section 3 (Security)| 2 issues found, 1 CRITICAL (SQL injection) |\n| | - SQL inject + format guard APPROVED (D4) |\n| Section 4 (Data/UX) | 0 new issues (shadow paths covered) |\n| Section 5 (Quality) | 0 new issues (covered by S2+S3) |\n| Section 6 (Tests) | 0 coverage; TODO APPROVED (D5) |\n| Section 7 (Perf) | N+1 loop; TODO APPROVED (D6) |\n| Section 8 (Observ) | 0 new gaps (logging in D3/D4 remedies) |\n| Section 9 (Deploy) | 0 risks (feature flag + checklist exist) |\n| Section 10 (Future) | Reversibility: 5/5; 2 debt items (in TODO) |\n| Section 11 (Design) | SKIPPED (no UI scope) |\n+--------------------------------------------------------------------+\n| NOT in scope | written (4 items) |\n| What already exists | written (10 items) |\n| Dream state delta | written (~40% to ideal post-Approach A) |\n| Error/rescue registry| 5 paths mapped, 0 CRITICAL GAPS remaining |\n| Failure modes | 6 total, 0 CRITICAL GAPS (post-remedies) |\n| TODOS.md updates | 2 items (tests P2, N+1 P2) |\n| Scope proposals | 0 proposed (HOLD SCOPE mode) |\n| CEO plan | skipped (HOLD SCOPE) |\n| Outside voice | disabled (codex_reviews=disabled) |\n| Diagrams produced | 2 (arch, data flow); 1 error flow table |\n| Stale diagrams found | 0 (fixture repo) |\n| Unresolved decisions | 0 |\n+====================================================================+\n```\n\n### Unresolved Decisions\n\nNone \u2014 all findings resolved via D3, D4, D5, D6.\n\n---\n\n## Review Readiness Dashboard\n\n```\n+====================================================================+\n| REVIEW READINESS DASHBOARD |\n+====================================================================+\n| Review | Runs | Last Run | Status | Required |\n|-----------------|------|---------------------|-----------|----------|\n| CEO Review | 1 | 2026-09-09 11:13 | REVIEWED | no |\n| Outside Voice | 1 | 2026-09-09 11:12 | DISABLED | no |\n| Eng Review | 0 | - | - | YES |\n| Design Review | 0 | - | - | no |\n| DX Review | 0 | - | - | no |\n+--------------------------------------------------------------------+\n| VERDICT: NOT CLEARED - Eng Review required |\n+====================================================================+\n```\n\nCEO Review: CURRENT (wtree match). Outside Voice: disabled (codex_reviews=disabled).\nEng Review not yet run \u2014 required before shipping.\n\n## Next Steps\n\n1. Run `/plan-eng-review` \u2014 required shipping gate (architecture, code quality, tests, performance)\n2. Address the 2 P1 implementation tasks (T1: SQL injection fix, T2: email rescue) before enabling the feature flag\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 | REVIEWED | 2 P1 fixes approved (SQL injection, email rescue), 2 P2 TODOs (tests, N+1), 0 unresolved |\n| Outside Review | codex (disabled) | Independent 2nd opinion | 1 | DISABLED | \u2014 |\n| Eng Review | `/plan-eng-review` | Architecture & tests (required) | 0 | \u2014 | \u2014 |\n| Design Review | `/plan-design-review` | UI/UX gaps | 0 | \u2014 | \u2014 |\n| DX Review | `/plan-devex-review` | Developer experience gaps | 0 | \u2014 | \u2014 |\n\n**OUTSIDE COVERAGE:** codex_reviews=disabled. No outside review. Re-enable: `gstack-config set codex_reviews enabled`\n\n**VERDICT:** CEO review complete, 0 unresolved decisions. eng review required before shipping.\n\nNO UNRESOLVED DECISIONS\n\n", + "provenance": "Exact completed Y calls, native Exit requests and retained report; tests assemble their own transcript and use explicitly synthetic mtime. Original report mtime was not captured." +} diff --git a/test/fixtures/ceo-section-y-report.md b/test/fixtures/ceo-section-y-report.md new file mode 100644 index 000000000..0331be5d8 --- /dev/null +++ b/test/fixtures/ceo-section-y-report.md @@ -0,0 +1,527 @@ +# 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: + +**AMENDMENT (Finding 1, auto-decided):** The original pseudocode did not satisfy +the "every read begun after write completes" invariant. A write-generation counter +is added to prevent a completing in-flight read from restoring a stale cache entry +after a concurrent write has deleted it. The counter adds two lines to the wrapper. + +```javascript +let _writeGen = 0; // process-local; reset on cache disable/enable + +async function readProfile(key) { + const cached = cache.get(key); + if (cached !== undefined) return cached; + const gen = _writeGen; // capture before await + const value = await repository.read(key); + if (_writeGen === gen) { // no write occurred during this read + cache.set(key, value); + } + return value; +} + +async function writeProfile(key, update) { + const saved = await repository.write(key, update); + _writeGen++; // increment before delete + cache.delete(key); + return saved; +} +``` + +**Why:** Without the generation counter, a read that starts before a write and +completes after it calls `cache.set(key, oldValue)` after the write has called +`cache.delete(key)`, restoring a stale entry for all subsequent reads until TTL +expiry. The invariant forbids relying on TTL for this guarantee. The counter +prevents the stale restore with two lines and no new dependencies. + +**Residual risk:** If multiple concurrent writes to different keys occur, any +in-flight read skips its cache.set() even if no write touched its specific key. +This increases cache misses conservatively but never produces stale results. +The additional DB load from spurious misses is bounded by the single-flight +wrapper and is acceptable given the hot-key distribution. + +**Verification:** The new test for the concurrent read/write ordering (Finding 4) +must confirm: (a) a read that starts before a write and completes after it does +not restore the old value, and (b) a read started after the write returns the new +value from DB. + +## 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, concurrent-miss coalescing, and +**concurrent read/write ordering (write-generation counter correctness)**. +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. + +**AMENDMENT (Finding 3, auto-decided — TODO):** Alert thresholds and a post-deploy +runbook are deferred to a follow-up task (T3). Minimum runbook content: disable the +feature flag if fallback_errors exceeds threshold or hit rate falls below 40% in +steady state; verify DB CPU trend is downward within 10 minutes of each stage +transition; confirm no fallback errors in the first 5 minutes at 10%. + +## 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 is for correctness, +coverage, and operability only. No scope additions or reductions. + +All findings below were auto-decided (spawned session, no human present; +recommended option selected per skill rules). Each finding is recorded once; +cross-references are used in place of repetition. + +### Finding 1 — CRITICAL: Stale Write-Back Race (Sections 1, 4) + +**Evidence:** In the JS event loop, `readProfile` has an `await` at +`repository.read(key)`. Between that await and the subsequent `cache.set()`, the +event loop can run `writeProfile`, which calls `cache.delete(key)`. When the +original `readProfile` resumes, it calls `cache.set(key, oldValue)`, restoring the +stale entry. Subsequent reads started after the write return the stale cached value +until TTL (up to 30 s). The plan's invariant prohibits this: "TTL expiry is not a +substitute for this rule." + +Schedule showing the violation: +``` +T | readProfile(K) | writeProfile(K) | cache[K] +----|--------------------------|-------------------------|---------- +0 | cache.get(K) → miss | | absent +1 | await repo.read(K)... | | absent +2 | | await repo.write(K)... | absent +3 | | cache.delete(K) | absent +4 | | return saved | absent +5 | ...read resolves | | absent +6 | cache.set(K, old) | | old ← VIOLATION +7 | return old to caller* | | old + *T=7 return is explicitly allowed; cache[K]=old is not. +``` + +**Remedy (accepted):** Write-generation counter — see amended pseudocode above. +**Residual risk:** Conservative miss-skip under concurrent multi-key writes (bounded). +**Verification:** Concurrent read/write ordering test (Finding 4 → T2). + +### Finding 2 — OK: Cache Adapter Error Handling (Section 2) + +The plan states the existing adapter bypasses the cache on any internal failure and +never propagates LRU errors to callers. The pseudocode correctly relies on this +contract. The "adapter-failure fallback" test verifies it. No gap. + +### Finding 3 — TODO: Alert Thresholds and Post-Deploy Runbook (Sections 8, 9) + +**Evidence:** The plan specifies metrics to monitor but does not specify alert +thresholds (e.g., hit rate <40%, fallback_errors >N/min) or a runbook (what to do, +in order, when each metric fires). Without thresholds, the rollout is monitored by +inspection rather than automation. + +**Remedy (deferred to T3):** Minimum viable runbook: disable flag if fallback_errors +spikes or hit rate below 40% in steady state; DB CPU downward trend expected within +10 minutes of each stage; no fallback errors in first 5 minutes at 10% stage. +Alert thresholds to be set based on baseline metrics at initial 10% stage. + +**Residual risk:** Rollout relies on manual inspection until runbook is written. The +feature flag mitigates this (fast disable path is documented). + +**Verification:** Runbook reviewed by service owner before 50% stage. + +--- + +## NOT in scope + +1. Distributed caching — explicitly excluded; multi-process unsupported at startup. +2. Cross-process cache coherence — single process only; enforced by startup check. +3. Cache prewarming — hot data fills naturally via feature flag rollout. +4. Changing consistency semantics — existing read/write ordering rules are preserved. +5. New product surfaces (UI, API, schema, pricing) — internal backend change only. +6. General cache framework — this wrapper is specific to this repository. +7. Per-key write-gen tracking — conservative global counter accepted; per-key adds + complexity without changing the correctness guarantee (see Finding 1 residual risk). + +## What already exists + +| Sub-problem | Existing code | Reused? | +|-------------|---------------|---------| +| LRU storage | Existing LRU adapter (1000 entries, 16 MiB, 30 s TTL) | Yes — unchanged | +| Repository interface | Existing read-through repo | Yes — wrapper sits above it | +| Single-flight for concurrent misses | Existing per-key single-flight in repo layer | Yes — transparent to wrapper | +| Auth/authorization | Runs before any repo access | Yes — no change | +| Feature flag | Existing runtime flag | Yes — rollout gated by it | +| Key validation | Existing tenant+profile key encoding | Yes — unchanged | +| Error mapping | Existing typed API error classes | Yes — unchanged | +| Contract tests | Existing repo contract test suite | Yes — extended with new cases | + +## Dream state delta + +``` +CURRENT STATE THIS PLAN 12-MONTH IDEAL +───────────────────── ───────────────────── ────────────────────── +DB CPU 70%, p95 120ms ──▶ Cache wrapper added ──▶ If multi-process needed: +900 hot keys hit DB LRU in single process Redis or distributed LRU +Single process 60%+ hit rate (separate decision gate) +No caching layer DB CPU <50%, p95 <60ms Current: single-process + Feature-flagged rollout constraint continues to + Write-gen counter added serve well if load stays + (correctness gap closed) single-process +``` + +This plan moves toward the ideal. The multi-process path is preserved (startup +rejection is explicit, not buried). The 12-month ideal requires a separate decision +only if the service scales to multiple processes. + +--- + +## Diagrams + +### 1. System Architecture + +``` +┌─────────────────────────────────────────────────┐ +│ Single JS Process │ +│ │ +│ Caller │ +│ │ │ +│ ▼ │ +│ ┌─────────────────────────────────────┐ │ +│ │ Cache Wrapper (NEW) │ │ +│ │ readProfile / writeProfile │ │ +│ │ _writeGen counter (NEW) │ │ +│ └──────┬────────────────┬────────────┘ │ +│ │ read-through │ invalidate │ +│ ▼ ▼ │ +│ ┌─────────────┐ ┌──────────────┐ │ +│ │ LRU Adapter │ │ Repository │ │ +│ │ (existing) │ │ (existing) │ │ +│ │ 1000 entries │ │ single-flt │ │ +│ │ 16 MiB cap │ │ (existing) │ │ +│ │ 30 s TTL │ └──────┬───────┘ │ +│ └─────────────┘ │ │ +│ bypass on fail ▼ │ +│ Database │ +└─────────────────────────────────────────────────┘ +``` + +### 2. Data Flow (all four paths) + +``` +KEY ──▶ [cache.get(key)] + │ + ┌──────┴──────────┐ + │ hit │ miss + ▼ ▼ + RETURN capture gen = _writeGen + cached │ + [await repo.read(key)] + │ + ┌────────────┼────────────┐ + │ success │ error │ nil/not-found + ▼ ▼ ▼ + _writeGen==gen? propagate sentinel set + │ typed (10 s TTL) + ┌───┴───┐ error + │Y │N + ▼ ▼ + cache.set skip set + (key,val) (stale guard) + │ + └──▶ RETURN value + +WRITE PATH: +KEY + UPDATE ──▶ [await repo.write(key, update)] + │ + ┌──────────┴──────────┐ + │ success │ error + ▼ ▼ + _writeGen++ propagate + cache.delete(key) typed error + RETURN saved (cache unchanged) +``` + +### 3. Cache State Machine + +``` + enable flag adapter failure + │ │ + ▼ ▼ + ┌─────────┐ ┌──────────┐ + │ ACTIVE │─────────▶ BYPASS │ + │ (hit/miss│ error │(all ops │ + │ served) │ │ skip │ + └────┬─────┘ │ cache) │ + │ └────┬─────┘ + │ disable flag │ empty cache + ▼ │ reinit + ┌─────────┐ │ + │ DISABLED │◀──────────────┘ + │(no cache │ + │ ops) │ + └──────────┘ +``` + +### 4. Error Flow + +``` + ┌─────────────────────┐ + │ readProfile(key) │ + └──────────┬──────────┘ + │ + cache.get() throws? + ┌────────┴────────┐ + │ Y (adapter err) │ N + ▼ ▼ + adapter bypass cache hit? + (internal to ┌────┴────┐ + adapter) │Y │N + ▼ ▼ + return repo.read() + cached │ + ┌──────┴──────┐ + │ success │ error + ▼ ▼ + gen check? propagate + ┌──┴──┐ typed err + │Y │N + ▼ ▼ + cache. skip + set() set + return return + value value +``` + +### 5. Rollout Sequence + +``` +CODE DEPLOY + │ + ▼ +Flag: DISABLED (0%) + All reads bypass cache → DB + │ + ▼ enable at 10% +Flag: 10% of keys cached + Monitor: hit/miss, fallback_errors, DB CPU, p95 + Wait: 1 healthy hour + Gate: error rate and latency within SLO + │ + ▼ promote to 50% +Flag: 50% of keys cached + Monitor same metrics + Wait: 1 healthy hour + Gate: same + │ + ▼ promote to 100% +Flag: ALL keys cached + Acceptance: ≥60% hit, DB CPU <50%, p95 <60 ms + │ + On regression at any stage: + ▼ +Flag: DISABLED immediately + Both reads and writes bypass cache + Re-enable creates empty cache +``` + +### 6. Rollback Flowchart + +``` +Regression detected (error rate or latency) + │ + ▼ +Disable feature flag (seconds, no deploy needed) + │ + ▼ +Verify: all reads/writes bypass cache, metrics normalize + │ + ▼ +Investigate root cause (logs: fallback_errors, eviction rate) + │ + ┌──────────────────────────────┐ + │ Code bug? │ Config bug? │ + ▼ ▼ │ + Code fix + Adjust LRU │ + re-deploy limits │ + │ ▼ │ + └──────────▶ Re-enable flag │ + at 10% and │ + retest │ + ▼ + If unresolvable: + git revert wrapper + (feature flag already + prevents cache ops, + revert is belt-and- + suspenders) +``` + +### Stale Diagram Audit + +No existing ASCII diagrams found in this plan's files (new plan). No stale diagrams. + +--- + +## Error & Rescue Registry + +| Method/Codepath | What Can Go Wrong | Exception Class | Rescued? | Rescue Action | User Sees | +|-----------------|-------------------|-----------------|----------|---------------|-----------| +| `cache.get()` | LRU internal error | LRUError | Y (adapter) | Adapter bypasses cache | DB read (transparent) | +| `cache.set()` | LRU internal error | LRUError | Y (adapter) | Adapter bypasses cache | DB read (transparent) | +| `cache.delete()` | LRU internal error | LRUError | Y (adapter) | Adapter bypasses cache | Next read hits DB | +| `repository.read()` | DB connection failure | DatabaseError | Y (existing) | Typed API error | Error response | +| `repository.read()` | Record not found | RecordNotFound | Y (existing) | Sentinel (10 s TTL) | Not-found response | +| `repository.write()` | DB write failure | DatabaseError | Y (existing) | Typed API error; cache NOT deleted | Error response | +| `repository.write()` | Conflict/constraint | ConflictError | Y (existing) | Typed API error; cache NOT deleted | Conflict response | + +No CRITICAL GAPS. All error paths are handled via existing adapter and repository contracts. + +--- + +## Failure Modes Registry + +| Codepath | Failure Mode | Rescued? | Test? | User Sees | Logged? | +|----------|-------------|----------|-------|-----------|---------| +| `readProfile` | cache.get() throws | Y (adapter bypass) | Y (adapter-failure fallback test) | Transparent DB hit | Y (adapter internal) | +| `readProfile` | DB read error | Y (existing typed) | Y (existing contract) | Error response | Y (existing) | +| `readProfile` | Stale write-back (in-flight read after write) | Y (write-gen counter — **AMENDED**) | Y (T2 — new test required) | Correct value | N/A (prevented) | +| `writeProfile` | DB write fails, cache preserved | Y (existing typed) | Y (failed-write preservation test) | Error response | Y (existing) | +| `writeProfile` | cache.delete() throws after successful write | Y (adapter bypass) | Y (adapter-failure fallback) | Transparent; next read is DB miss | Y (adapter internal) | +| LRU adapter | Full bypass state | Y (adapter) | Y (adapter-failure fallback) | All reads/writes hit DB | Y (fallback_errors metric) | + +**CRITICAL GAPS resolved:** Finding 1 (stale write-back) is addressed by the write-gen counter amendment. The test (T2) is required before ship. + +--- + +## 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: ~10min)** — Cache Wrapper — Add write-generation counter to `readProfile` + - Surfaced by: Finding 1 — stale write-back race violates "reads begun after write" invariant + - Change: add `let _writeGen = 0`, capture `gen = _writeGen` before `await repo.read()`, conditional `cache.set()` only when `_writeGen === gen`; increment `_writeGen++` in `writeProfile` before `cache.delete()` + - Files: cache wrapper module (new file wrapping the repository) + - Verify: all new wrapper tests pass; T2 test specifically confirms ordering invariant + +- [ ] **T2 (P1, human: ~1h / CC: ~10min)** — Tests — Add concurrent read/write ordering test + - Surfaced by: Finding 4 (Section 6) — no test for stale write-back race; required to verify T1 + - Scenario: pause readProfile after cache miss, run writeProfile to completion, resume readProfile, assert next read returns new value not old + - Files: cache wrapper test file + - Verify: test fails without write-gen counter, passes with it; CI must include this test + +- [ ] **T3 (P3, human: ~30min / CC: ~5min)** — Observability — Alert thresholds and post-deploy runbook + - Surfaced by: Finding 3 (Sections 8, 9) — metrics listed but no thresholds or runbook + - Content: hit rate alert (<40% in steady state), fallback_errors threshold, 5-minute post-deploy checklist per rollout stage, disable-flag runbook + - Files: ops/runbook-cache-wrapper.md (or equivalent) + - Verify: service owner reviews before 50% stage promotion + +_No new tasks from Sections 3, 5, 7, 10, 11._ + +--- + +## Completion Summary + +``` ++====================================================================+ +| MEGA PLAN REVIEW — COMPLETION SUMMARY | ++====================================================================+ +| Mode selected | HOLD SCOPE | +| System Audit | Skipped (test instructions) | +| Step 0 | HOLD SCOPE confirmed; premise valid; | +| | measured problem, direct solution | +| Section 1 (Arch) | 1 critical finding (stale write-back race) | +| Section 2 (Errors) | 7 error paths mapped, 0 GAPS | +| Section 3 (Security)| 0 issues found, 0 High severity | +| Section 4 (Data/UX) | 1 ordering violation confirmed → Finding 1 | +| Section 5 (Quality) | 0 new issues | +| Section 6 (Tests) | Diagram produced, 1 gap (race test) | +| Section 7 (Perf) | 0 issues found | +| Section 8 (Observ) | 1 gap (no alert thresholds or runbook) | +| Section 9 (Deploy) | 1 gap (no post-deploy checklist — merged | +| | into Finding 3 / T3) | +| Section 10 (Future) | Reversibility: 4/5, 0 new debt items | +| Section 11 (Design) | SKIPPED (no UI scope) | ++--------------------------------------------------------------------+ +| NOT in scope | written (7 items) | +| What already exists | written (8 items) | +| Dream state delta | written | +| Error/rescue registry| 7 methods, 0 CRITICAL GAPS | +| Failure modes | 6 total, 1 CRITICAL GAP found and closed by | +| | write-gen counter amendment (T1 required) | +| TODOS.md updates | 1 item (T3: runbook + thresholds) | +| Scope proposals | 0 proposed, 0 accepted (HOLD SCOPE) | +| CEO plan | skipped (HOLD SCOPE) | +| Outside voice | disabled (codex_reviews: disabled) | +| Lake Score | 3/3 recommendations chose complete option | +| Diagrams produced | 6 (architecture, data flow, state machine, | +| | error flow, rollout sequence, rollback) | +| Stale diagrams found | 0 (new plan) | +| Unresolved decisions | 0 (all auto-decided, spawned session) | ++====================================================================+ +``` + +### Auto-Decided Items (spawned session) + +All AskUserQuestion decision points were auto-decided per skill rules (spawned +session, recommended option selected, no destructive choices): + +- **Finding 1 remedy** → write-generation counter (recommended: minimal, correct, + no new dependencies). Alternative considered: setIfAbsent semantics (requires + LRU adapter API change — more invasive). Counter selected. +- **Finding 3** → deferred to T3 as TODO (recommended: defer; rollout proceeds + with manual monitoring; fast flag disable remains the primary safety mechanism). +- **Stale write-back test (Finding 4)** → required as T2 (P1, blocks ship with T1). + +--- + +## GSTACK REVIEW REPORT + +| Review | Trigger | Why | Runs | Status | Findings | +|--------|---------|-----|------|--------|----------| +| CEO Review | `/plan-ceo-review` | Scope & strategy | 1 | issues_open (T1+T2 required) | mode: HOLD_SCOPE, 1 critical gap (stale write-back, write-gen remedy accepted), 0 unresolved | +| Outside Review | codex_reviews=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:** disabled — `codex_reviews: disabled` in `.gstack-section-state-4pqsxp/config.yaml`. No fallback dispatched; disabled is an intentional opt-out. Re-enable: `gstack-config set codex_reviews enabled`. + +**VERDICT:** CEO review complete. Eng Review required before ship (not yet run). T1 (write-gen counter) and T2 (race test) are P1 blockers. T3 (runbook) is P3. eng review required. + +NO UNRESOLVED DECISIONS diff --git a/test/fixtures/design-boundaries-y-calls.json b/test/fixtures/design-boundaries-y-calls.json new file mode 100644 index 000000000..b69fa6544 --- /dev/null +++ b/test/fixtures/design-boundaries-y-calls.json @@ -0,0 +1,234 @@ +[ + { + "sessionId": "a1ef2828-2d3d-4ad8-a826-48918d812142", + "toolUseId": "toolu_019cY79qXJh8SJhw7qkppXFv", + "questions": [ + { + "question": "Pass 1 (Information Architecture) \u2014 Finding 1: The action group has no visual hierarchy. Save, Reset, Cancel, and Export are all styled the same. Users must pause to identify the primary action \u2014 a hierarchy failure at the most critical part of the page. DESIGN.MD already specifies the fix. Apply it to the plan? ", + "header": "Action group", + "options": [ + { + "label": "Apply DESIGN.MD tokens (Recommended)", + "description": "Save = filled #1d4ed8 with white text; Reset/Cancel/Export = neutral ghost buttons. Destructive intent communicated via confirmation dialog, not button color. Spec goes into the plan with exact tokens." + }, + { + "label": "Add spec without tokens", + "description": "Note that Save should be visually primary but leave exact colors to implementation. Less rigid; risks inconsistency with DESIGN.MD when implemented." + }, + { + "label": "Defer \u2014 handle in implementation", + "description": "Leave the gap as-is. The implementer decides at build time. Risk: ships with flat hierarchy because the gap is already the status quo." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "Pass 1 (Information Architecture) \u2014 Finding 1: The action group has no visual hierarchy. Save, Reset, Cancel, and Export are all styled the same. Users must pause to identify the primary action \u2014 a hierarchy failure at the most critical part of the page. DESIGN.MD already specifies the fix. Apply it to the plan? ": "Apply DESIGN.MD tokens (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:19:06.151Z" + }, + { + "sessionId": "a1ef2828-2d3d-4ad8-a826-48918d812142", + "toolUseId": "toolu_01JWj4Wpnz8GNGJfB86KKHF2", + "questions": [ + { + "question": "Pass 2 (Interaction States) \u2014 Finding 2: The plan references 'the existing form skeleton' for initial loading but gives no visual spec. Which fields are skeleton shapes, how wide, how tall? Without this every implementer ships a different skeleton. Add a brief spec to the plan? ", + "header": "Skeleton spec", + "options": [ + { + "label": "Add skeleton spec to plan (Recommended)", + "description": "Specify: two text-field skeleton rects (Display name, Email) + two switch-row skeleton rects (Weekly digest, Product tips), matching 640px max-width, using existing skeleton animation and color tokens. Enough detail that any implementer ships the same shape." + }, + { + "label": "Leave as 'existing form skeleton'", + "description": "Trust the existing component to handle it. Risk: existing skeleton may not match the current form structure, and no one catches it until visual QA." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "Pass 2 (Interaction States) \u2014 Finding 2: The plan references 'the existing form skeleton' for initial loading but gives no visual spec. Which fields are skeleton shapes, how wide, how tall? Without this every implementer ships a different skeleton. Add a brief spec to the plan? ": "Add skeleton spec to plan (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:19:40.779Z" + }, + { + "sessionId": "a1ef2828-2d3d-4ad8-a826-48918d812142", + "toolUseId": "toolu_011uLPurT75gPkAiTEemoox8", + "questions": [ + { + "question": "Pass 5 (Design System) \u2014 Finding 3: Spacing is inconsistent \u2014 24px between some areas, 32px between others, 16px in a third. DESIGN.MD specifies an 8px base: sections 32px, field groups 24px, label-to-input 8px. Apply the consistent scale to the plan? ", + "header": "Spacing scale", + "options": [ + { + "label": "Apply DESIGN.MD spacing scale (Recommended)", + "description": "Section gaps = 32px, field group gaps = 24px, label-to-input = 8px. Removes the ad-hoc 16px section gap. Spec written into plan with exact values so implementer has no choices to make." + }, + { + "label": "Defer to implementation", + "description": "Let the implementer reconcile spacing at build time. Risk: inconsistent rhythm ships, and QA cycles are needed to catch it post-implementation." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "Pass 5 (Design System) \u2014 Finding 3: Spacing is inconsistent \u2014 24px between some areas, 32px between others, 16px in a third. DESIGN.MD specifies an 8px base: sections 32px, field groups 24px, label-to-input 8px. Apply the consistent scale to the plan? ": "Apply DESIGN.MD spacing scale (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:20:05.875Z" + }, + { + "sessionId": "a1ef2828-2d3d-4ad8-a826-48918d812142", + "toolUseId": "toolu_01P1d2BhRam8mSsx32x7vmED", + "questions": [ + { + "question": "Pass 4+5 (AI Slop / Design System) \u2014 Finding 4: The form currently uses three font sizes: 14px, 16px, and 18px. This is two problems: (1) 14px violates the Universal Rule \u2014 body text below 16px is never allowed; (2) DESIGN.MD specifies exactly two roles: 16px for body/labels, 20px for section headings. Collapse to the DESIGN.MD two-size scale? ", + "header": "Typography scale", + "options": [ + { + "label": "Apply DESIGN.MD two-size scale (Recommended)", + "description": "Body/form labels/helper text = 16px. Section headings (Profile, Notifications) = 20px. Eliminates 14px (Universal Rule violation) and 18px (DESIGN.MD misalignment). Two sizes instead of three creates stronger, scannable hierarchy." + }, + { + "label": "Keep 16px and 18px, just fix 14px", + "description": "Raises 14px to 16px but keeps 18px headings instead of 20px. Fixes the hard rule but stays misaligned with DESIGN.MD typography spec. Three-size scale persists." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "Pass 4+5 (AI Slop / Design System) \u2014 Finding 4: The form currently uses three font sizes: 14px, 16px, and 18px. This is two problems: (1) 14px violates the Universal Rule \u2014 body text below 16px is never allowed; (2) DESIGN.MD specifies exactly two roles: 16px for body/labels, 20px for section headings. Collapse to the DESIGN.MD two-size scale? ": "Apply DESIGN.MD two-size scale (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:20:30.979Z" + }, + { + "sessionId": "a1ef2828-2d3d-4ad8-a826-48918d812142", + "toolUseId": "toolu_01Fu3RwRPev6XxSQE9Nfap6B", + "questions": [ + { + "question": "Pass 5 (Design System) \u2014 Finding 5: Error text currently has ~3:1 contrast (red on light pink) \u2014 below WCAG AA minimum of 4.5:1. Users with low vision cannot reliably read error messages. DESIGN.MD specifies error.text #991b1b on error.surface #fef2f2 (approximately 5.5:1 contrast, passing AA). Apply the DESIGN.MD error color tokens? ", + "header": "Error contrast", + "options": [ + { + "label": "Apply DESIGN.MD error tokens (Recommended)", + "description": "error.text: #991b1b on error.surface: #fef2f2. Achieves ~5.5:1 contrast \u2014 passes WCAG AA (4.5:1). Also requires an icon and explicit text per DESIGN.MD (never color alone to communicate status). All three together go in the plan." + }, + { + "label": "Defer to implementation", + "description": "Leave the contrast gap noted but unresolved. Risk: a WCAG AA violation ships. Downstream: legal exposure and/or failing audit." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "Pass 5 (Design System) \u2014 Finding 5: Error text currently has ~3:1 contrast (red on light pink) \u2014 below WCAG AA minimum of 4.5:1. Users with low vision cannot reliably read error messages. DESIGN.MD specifies error.text #991b1b on error.surface #fef2f2 (approximately 5.5:1 contrast, passing AA). Apply the DESIGN.MD error color tokens? ": "Apply DESIGN.MD error tokens (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:21:03.636Z" + }, + { + "sessionId": "a1ef2828-2d3d-4ad8-a826-48918d812142", + "toolUseId": "toolu_01D8xGiK8saLKFZwAAKqTZHx", + "questions": [ + { + "question": "Pass 5 (Design System) \u2014 Finding 6: Save takes 2\u20135 seconds with no feedback. The page appears frozen. This is the trust-critical moment: the user just committed their preferences. DESIGN.MD specifies an inline spinner beside 'Saving\u2026' inside the disabled Save button, with aria-busy=true and reduced-motion support. Apply this pattern to the plan? ", + "header": "Save spinner", + "options": [ + { + "label": "Apply DESIGN.MD pending-action pattern (Recommended)", + "description": "Inline spinner inside disabled Save button. Button text changes to 'Saving\u2026'. aria-busy=true on the button. @media (prefers-reduced-motion): spinner hidden, text-only 'Saving\u2026' shown. Resolves the frozen-page trust erosion at step 6 of the user journey." + }, + { + "label": "Add a page-level loading overlay instead", + "description": "Blur or disable the entire form while saving. Covers the full wait period but is heavier UX \u2014 user loses context of what they just edited. DESIGN.MD specifies inline pattern; this diverges." + }, + { + "label": "Defer to implementation", + "description": "Leave the 2\u20135s frozen page as-is. Risk: users double-click Save, submit twice, or navigate away mid-save." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "Pass 5 (Design System) \u2014 Finding 6: Save takes 2\u20135 seconds with no feedback. The page appears frozen. This is the trust-critical moment: the user just committed their preferences. DESIGN.MD specifies an inline spinner beside 'Saving\u2026' inside the disabled Save button, with aria-busy=true and reduced-motion support. Apply this pattern to the plan? ": "Apply DESIGN.MD pending-action pattern (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:21:32.773Z" + }, + { + "sessionId": "a1ef2828-2d3d-4ad8-a826-48918d812142", + "toolUseId": "toolu_01Pfjp1TX1ms3SEbqZR6VWD6", + "questions": [ + { + "question": "Pass 6 (Responsive & Accessibility) \u2014 Finding 7: The plan specifies Weekly digest and Product tips as switches, but never names the keyboard interaction. ARIA switch role requires Space to toggle and aria-checked to announce state. Without this spec, an implementer may ship a custom element with no keyboard support \u2014 an accessibility regression. Add the switch keyboard pattern to the plan? ", + "header": "Switch keyboard", + "options": [ + { + "label": "Add switch keyboard spec to plan (Recommended)", + "description": "Weekly digest and Product tips use input[type=checkbox][role=switch] or equivalent. Space toggles the switch. aria-checked reflects current state. State change announces immediately via the existing aria-live region. This is the ARIA switch pattern; costs nothing to specify now." + }, + { + "label": "Trust existing switch component", + "description": "If an existing switch/toggle component already handles this, no spec change is needed. Risk: if the component is custom or third-party, keyboard support may not be guaranteed. Requires knowing the existing component's a11y coverage." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "Pass 6 (Responsive & Accessibility) \u2014 Finding 7: The plan specifies Weekly digest and Product tips as switches, but never names the keyboard interaction. ARIA switch role requires Space to toggle and aria-checked to announce state. Without this spec, an implementer may ship a custom element with no keyboard support \u2014 an accessibility regression. Add the switch keyboard pattern to the plan? ": "Add switch keyboard spec to plan (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:21:57.883Z" + }, + { + "sessionId": "a1ef2828-2d3d-4ad8-a826-48918d812142", + "toolUseId": "toolu_01Bdht9T8YS5rpM4m6m9MJGi", + "questions": [ + { + "question": "Design review complete \u2014 6/10 \u2192 9/10. All 7 decisions resolved. The plan is design-complete; next is the required shipping gate. What\u2019s next?", + "header": "Next review", + "options": [ + { + "label": "Run /plan-eng-review (Recommended)", + "description": "Eng review is the required shipping gate. It validates architecture, component wiring, tests, and accessibility implementation against the 7 approved design decisions. This design review added interaction specs (spinner, skeleton, switch keyboard), so eng review needs to validate their architectural fit." + }, + { + "label": "Skip \u2014 I\u2019ll handle next steps manually", + "description": "End the review workflow here. The improved plan is at the e2e output path; implementation can begin. Run /plan-eng-review later before shipping." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "Design review complete \u2014 6/10 \u2192 9/10. All 7 decisions resolved. The plan is design-complete; next is the required shipping gate. What\u2019s next?": "Run /plan-eng-review (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:24:30.829Z" + } +] diff --git a/test/fixtures/design-outside-y-calls.json b/test/fixtures/design-outside-y-calls.json new file mode 100644 index 000000000..ea25a0b6d --- /dev/null +++ b/test/fixtures/design-outside-y-calls.json @@ -0,0 +1,242 @@ +[ + { + "sessionId": "b6b8f7c7-a50c-4ccd-9996-4b321309a017", + "toolUseId": "toolu_016LskTqkTpnBoXPf7inaJbq", + "questions": [ + { + "question": "D1 \u2014 Run outside design voices before the review passes? ", + "header": "Outside voices", + "options": [ + { + "label": "No \u2014 proceed without (recommended)", + "description": "Skip Codex + Claude subagent outside pass. Best for this case: it's a scoped settings form update with a complete DESIGN.md; hard-rejection checks apply to marketing surfaces, not OPERATE/settings UI." + }, + { + "label": "Yes \u2014 run Codex + Claude subagent", + "description": "Run Codex against OpenAI design hard rules + litmus checks, and a separate Claude subagent for an independent completeness review. Adds time but catches anything a single-model pass misses." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 \u2014 Run outside design voices before the review passes? ": "No \u2014 proceed without (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:09:50.422Z" + }, + { + "sessionId": "b6b8f7c7-a50c-4ccd-9996-4b321309a017", + "toolUseId": "toolu_01Mptx24PCxWs9mfwqYqbVyY", + "questions": [ + { + "question": "D2 \u2014 Pass 2: Export pending state is not specified in the plan. What should the user see while the JSON download is being prepared? ", + "header": "Export pending", + "options": [ + { + "label": "Disable + spinner (recommended)", + "description": "Apply the same DESIGN.md pending pattern used by Save: disable the Export button, show an inline spinner beside \u201cExporting\u2026\u201d, aria-busy=true. Reuses existing component behavior; prevents duplicate downloads; consistent with Save pending UX. (human: ~30min / CC: ~5min)" + }, + { + "label": "Disable only, no spinner", + "description": "Disable the Export button while downloading but show no spinner. Prevents double-clicks but gives no progress signal. Simpler, but inconsistent with Save pending pattern; user may think it froze. (human: ~15min / CC: ~3min)" + }, + { + "label": "Leave unspecified \u2014 implementer decides", + "description": "Defer this to the implementer. Risk: engineer ships a frozen Export button, users click it multiple times and receive duplicate downloads." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 \u2014 Pass 2: Export pending state is not specified in the plan. What should the user see while the JSON download is being prepared? ": "Disable + spinner (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:10:18.487Z" + }, + { + "sessionId": "b6b8f7c7-a50c-4ccd-9996-4b321309a017", + "toolUseId": "toolu_01AWCbis4SQmcNnFXLZHBjfC", + "questions": [ + { + "question": "D3 \u2014 Pass 2: The plan says 'the existing form skeleton' for initial load. Is this skeleton truly already built, or is it new work that needs a visual spec? ", + "header": "Skeleton status", + "options": [ + { + "label": "Already built \u2014 no new spec needed (recommended)", + "description": "The skeleton component exists and works today. Plan correctly references it. Implementation task just needs to point to the existing component; no shimmer or gray-box spec required." + }, + { + "label": "New work \u2014 needs visual spec", + "description": "The skeleton does not yet exist. The plan needs to describe which elements shimmer (input fields? section headings? the action group?). Adds spec work before implementation can start. (human: ~1h / CC: ~10min)" + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 \u2014 Pass 2: The plan says 'the existing form skeleton' for initial load. Is this skeleton truly already built, or is it new work that needs a visual spec? ": "Already built \u2014 no new spec needed (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:10:32.528Z" + }, + { + "sessionId": "b6b8f7c7-a50c-4ccd-9996-4b321309a017", + "toolUseId": "toolu_01U1M5vvNUM4VgQWa2srLYPJ", + "questions": [ + { + "question": "D4 \u2014 Pass 5 / Gap G1: Save button has the same visual weight as Reset, Cancel, and Export. DESIGN.md specifies the fix. Approve adding this implementation task? ", + "header": "Save button style", + "options": [ + { + "label": "Approve DESIGN.md fix: Save = filled primary (#1d4ed8 + white text), others = ghost (recommended)", + "description": "Style Save as the filled primary Button component; Reset, Cancel, Export as neutral ghost variants. Exact tokens from DESIGN.md. Immediately tells users which action is primary. (human: ~1h / CC: ~10min)" + }, + { + "label": "Keep all four buttons equal weight", + "description": "Leave all four buttons visually identical. Users must read labels to find Save. Contradicts the DESIGN.md primary/ghost distinction. Not recommended for a primary action page." + }, + { + "label": "Different visual distinction \u2014 size or border only", + "description": "Make Save larger or bolder rather than filled. Requires new design work outside DESIGN.md; adds ambiguity. Not recommended when DESIGN.md already specifies the exact solution." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 \u2014 Pass 5 / Gap G1: Save button has the same visual weight as Reset, Cancel, and Export. DESIGN.md specifies the fix. Approve adding this implementation task? ": "Approve DESIGN.md fix: Save = filled primary (#1d4ed8 + white text), others = ghost (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:10:46.563Z" + }, + { + "sessionId": "b6b8f7c7-a50c-4ccd-9996-4b321309a017", + "toolUseId": "toolu_01QJvgcs6sqNvYbLMAkS199z", + "questions": [ + { + "question": "D5 \u2014 Pass 5 / Gap G2: Spacing is inconsistent (16px / 24px / 32px). DESIGN.md specifies sections = 32px, field groups = 24px, label-to-input = 8px. The 16px is the offender. Approve normalizing to the DESIGN.md 8px scale? ", + "header": "Spacing fix", + "options": [ + { + "label": "Approve DESIGN.md fix: normalize to sections=32px, groups=24px, label=8px (recommended)", + "description": "Apply the DESIGN.md 8px base scale. Eliminates the 16px rogue value. Creates consistent vertical rhythm. Implementation task will identify the specific element(s) using 16px. (human: ~1h / CC: ~10min)" + }, + { + "label": "Leave spacing as-is", + "description": "Accept the 16px/24px/32px mix. The user will see inconsistent breathing room between elements \u2014 professional but not polished. Contradicts DESIGN.md. Not recommended." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "D5 \u2014 Pass 5 / Gap G2: Spacing is inconsistent (16px / 24px / 32px). DESIGN.md specifies sections = 32px, field groups = 24px, label-to-input = 8px. The 16px is the offender. Approve normalizing to the DESIGN.md 8px scale? ": "Approve DESIGN.md fix: normalize to sections=32px, groups=24px, label=8px (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:11:00.600Z" + }, + { + "sessionId": "b6b8f7c7-a50c-4ccd-9996-4b321309a017", + "toolUseId": "toolu_01Ps2iFr4RduoDLL1MuN4aXJ", + "questions": [ + { + "question": "D6 \u2014 Pass 5 / Gap G3: Error colors. The plan says contrast is ~3:1. But DESIGN.md's tokens (#991b1b on #fef2f2) actually achieve ~7.4:1 \u2014 the plan's claim describes the *current wrong implementation*, not the fix. Approve applying DESIGN.md tokens and correcting the contrast claim in the plan? ", + "header": "Error color fix", + "options": [ + { + "label": "Approve: apply DESIGN.md error tokens + correct the contrast claim (recommended)", + "description": "Use error.text #991b1b on error.surface #fef2f2 with an icon. Update the plan to state the ~3:1 is the current wrong value and the DESIGN.md fix achieves ~7.4:1. Prevents implementers from researching a problem that doesn\u2019t exist in the fixed state. (human: ~1h / CC: ~10min)" + }, + { + "label": "Apply tokens only, leave the plan\u2019s contrast claim unchanged", + "description": "Fix the colors but keep the misleading ~3:1 description. Risk: future readers think the DESIGN.md fix still has a contrast problem and add unnecessary workarounds." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "D6 \u2014 Pass 5 / Gap G3: Error colors. The plan says contrast is ~3:1. But DESIGN.md's tokens (#991b1b on #fef2f2) actually achieve ~7.4:1 \u2014 the plan's claim describes the *current wrong implementation*, not the fix. Approve applying DESIGN.md tokens and correcting the contrast claim in the plan? ": "Approve: apply DESIGN.md error tokens + correct the contrast claim (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:11:14.646Z" + }, + { + "sessionId": "b6b8f7c7-a50c-4ccd-9996-4b321309a017", + "toolUseId": "toolu_01NAJCKX9PejBacx8oyd7GQn", + "questions": [ + { + "question": "D7 \u2014 Pass 5 / Gap G4: Form uses 14px, 16px, and 18px. DESIGN.md specifies two roles: 16px (body/labels/helper text) and 20px (section headings). 14px also violates the universal rule (body text < 16px is prohibited). Approve rationalizing to 16px + 20px only? ", + "header": "Typography fix", + "options": [ + { + "label": "Approve DESIGN.md fix: 16px body/labels/helper + 20px headings, eliminate 14px and 18px (recommended)", + "description": "Removes the accessibility violation (14px body text) and the intermediate 18px that creates false hierarchy. Implementation task will identify which elements currently use 14px and 18px. Strong two-tier system: 16px for everything readable, 20px for h2 section headings. (human: ~1h / CC: ~10min)" + }, + { + "label": "Keep 14px for helper/secondary text", + "description": "Retain 14px for less prominent text (e.g., field hints). Risks: violates body text accessibility rule; creates three-size system instead of two; small text on a form erodes usability. Not recommended." + }, + { + "label": "Use 18px for section headings instead of 20px", + "description": "Compromise: 16px body + 18px headings. Creates weaker section hierarchy than DESIGN.md\u2019s 20px; implementer must override DESIGN.md token. Not recommended when the spec is already clear." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "D7 \u2014 Pass 5 / Gap G4: Form uses 14px, 16px, and 18px. DESIGN.md specifies two roles: 16px (body/labels/helper text) and 20px (section headings). 14px also violates the universal rule (body text < 16px is prohibited). Approve rationalizing to 16px + 20px only? ": "Approve DESIGN.md fix: 16px body/labels/helper + 20px headings, eliminate 14px and 18px (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:11:30.690Z" + }, + { + "sessionId": "b6b8f7c7-a50c-4ccd-9996-4b321309a017", + "toolUseId": "toolu_0195rLfgc3HFFq7jLPNCuVK6", + "questions": [ + { + "question": "D8 \u2014 Pass 5 / Gap G5: Save takes 2-5s with no feedback (frozen page). DESIGN.md specifies the fix exactly. Approve adding the loading indicator? ", + "header": "Loading indicator", + "options": [ + { + "label": "Approve DESIGN.md fix: inline spinner + \u201cSaving\u2026\u201d inside disabled Save, aria-busy=true, reduced-motion support (recommended)", + "description": "Reuses the DESIGN.md established pending-action pattern. Spinner appears beside \u201cSaving\u2026\u201d text inside the Save button; button is disabled while pending; aria-busy=true for screen readers; @media (prefers-reduced-motion) suppresses animation. User sees immediate feedback, never a frozen page. (human: ~2h / CC: ~15min)" + }, + { + "label": "Page-level skeleton or overlay instead", + "description": "Show a full-page loading overlay or skeleton while saving. Higher visual weight than a simple button spinner; harder to implement; inconsistent with the DESIGN.md pattern. Overkill for a small form save. Not recommended." + }, + { + "label": "Toast notification after save instead of spinner during", + "description": "Show a toast notification after success rather than in-button feedback during save. Doesn\u2019t solve the frozen-page problem (the user still sees nothing during 2-5s). Not recommended." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "D8 \u2014 Pass 5 / Gap G5: Save takes 2-5s with no feedback (frozen page). DESIGN.md specifies the fix exactly. Approve adding the loading indicator? ": "Approve DESIGN.md fix: inline spinner + \u201cSaving\u2026\u201d inside disabled Save, aria-busy=true, reduced-motion support (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:11:46.732Z" + } +] diff --git a/test/fixtures/devex-count-y-calls.json b/test/fixtures/devex-count-y-calls.json new file mode 100644 index 000000000..3a75e5e0f --- /dev/null +++ b/test/fixtures/devex-count-y-calls.json @@ -0,0 +1,178 @@ +[ + { + "sessionId": "4cbd2d66-f875-4fcd-b4e2-6c8a6d6716cd", + "toolUseId": "toolu_01USNDVwq7zfQWz9TRw6tnyi", + "questions": [ + { + "question": "D1 \u2014 Pass 1 (Getting Started): The agreed <2 min TTHW target is mathematically impossible with the retained 5-min CI block. Which resolution belongs in the plan? ", + "header": "TTHW block", + "multiSelect": false, + "options": [ + { + "label": "Demo-only CI bypass (Recommended)", + "description": "Add an offline mode specifically for `python -m evalkit.demo`. The demo runs bundled sample_responses.json with no real API submission, so the CI check has nothing to gate. This achieves the <2 min target for the agreed first-success experience without touching the real API contract. Scope impact: small \u2014 only evalkit/demo.py changes." + }, + { + "label": "Add --offline flag to run_eval", + "description": "Give run_eval and run_batch an --offline=True flag that skips the CI block, enabling local dev testing. Achieves <2 min for both demo AND real usage in local mode. Broader fix, but expands scope beyond DX POLISH into API surface change." + }, + { + "label": "Update TTHW target to reflect reality", + "description": "Change the agreed target from <2 min to <7 min in the plan. No code change, no scope expansion. Honest about the current experience, but drops EvalKit below all three competitors (A=2min, B=4min, C=3min) and accepts the 'Needs Work' tier explicitly." + }, + { + "label": "Other \u2014 describe the constraint", + "description": "There's a constraint or design intent I haven't surfaced. Tell me what's actually going on with the CI block and the TTHW target." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 \u2014 Pass 1 (Getting Started): The agreed <2 min TTHW target is mathematically impossible with the retained 5-min CI block. Which resolution belongs in the plan? ": "Demo-only CI bypass (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:06:32.086Z" + }, + { + "sessionId": "4cbd2d66-f875-4fcd-b4e2-6c8a6d6716cd", + "toolUseId": "toolu_01Gxtd2wXZEDoKo7YqnZnUi7", + "questions": [ + { + "question": "D2 \u2014 Pass 1+4 (Getting Started + Docs): The README tells developers to run `python examples/first_eval.py`, but docs/package-contents.txt explicitly states that file is absent from both the published package and the release examples archive. A developer's first command fails with FileNotFoundError. What's the fix? ", + "header": "Missing file", + "multiSelect": false, + "options": [ + { + "label": "Update README to point at the demo (Recommended)", + "description": "Replace the `python examples/first_eval.py` instruction in README with `python -m evalkit.demo`. The demo module IS in the package, produces real scored output, and is the agreed first-success vehicle. Zero new files needed, one line changes in README." + }, + { + "label": "Add first_eval.py to the package", + "description": "Write examples/first_eval.py and include it in the package so the documented path works. More complete \u2014 developers have a real code file they can open and extend. But adds scope: file must be authored, tested, and maintained alongside demo.py." + }, + { + "label": "Both: update README AND add the file", + "description": "Update README to use the demo command as the primary path, AND add examples/first_eval.py as a secondary reference for developers who want editable starter code. Most complete. Two deliverables instead of one." + }, + { + "label": "Other \u2014 first_eval.py is coming from somewhere else", + "description": "The file exists or will exist in a place I haven't looked. Tell me where it is or where it's meant to come from." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 \u2014 Pass 1+4 (Getting Started + Docs): The README tells developers to run `python examples/first_eval.py`, but docs/package-contents.txt explicitly states that file is absent from both the published package and the release examples archive. A developer's first command fails with FileNotFoundError. What's the fix? ": "Update README to point at the demo (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:06:46.116Z" + }, + { + "sessionId": "4cbd2d66-f875-4fcd-b4e2-6c8a6d6716cd", + "toolUseId": "toolu_012wXLgrh6oDJGHipSk1x5Ti", + "questions": [ + { + "question": "D3 \u2014 Pass 3 (Error Messages): The SDK raises `AuthError(\"request failed\")` for an invalid API key. No error code, no explanation of what failed, no instruction for how to fix it. The plan explicitly retains this message. An ML engineer with a typo in EVALKIT_API_KEY sees a two-word error and has no idea what broke. Fix or retain? ", + "header": "AuthError msg", + "multiSelect": false, + "options": [ + { + "label": "Fix the error message (Recommended)", + "description": "Improve to: `AuthError(\"Authentication failed: EVALKIT_API_KEY is invalid or expired. Check your key at console.evalkit.com/keys and set the EVALKIT_API_KEY environment variable.\")`. Follows DX First Principle #5: every error = problem + cause + fix. One-line change in evalkit/client.py." + }, + { + "label": "Add error code only", + "description": "Change to `AuthError(\"auth_invalid: request failed\")` \u2014 adds a stable machine-readable code without restructuring the message. Useful for programmatic error handling but still leaves the developer without a cause or fix." + }, + { + "label": "Retain the current message", + "description": "Explicitly accept this as a known DX debt item. The plan notes it as a post-beta improvement. An ML engineer who hits this will need to consult docs or support to diagnose it." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 \u2014 Pass 3 (Error Messages): The SDK raises `AuthError(\"request failed\")` for an invalid API key. No error code, no explanation of what failed, no instruction for how to fix it. The plan explicitly retains this message. An ML engineer with a typo in EVALKIT_API_KEY sees a two-word error and has no idea what broke. Fix or retain? ": "Fix the error message (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:07:08.170Z" + }, + { + "sessionId": "4cbd2d66-f875-4fcd-b4e2-6c8a6d6716cd", + "toolUseId": "toolu_01TSTRSbuz856wiLPpNthPtD", + "questions": [ + { + "question": "D4 \u2014 Pass 2 (API Design): `run_eval(dataset, evaluator)` and `run_batch(evaluator, dataset)` have REVERSED positional order for the same concepts. The plan calls this intentional. A developer who writes `run_eval(my_dataset, my_evaluator)` then calls `run_batch` the same way silently passes wrong types \u2014 no error until runtime, potentially confusing results. Fix or keep? ", + "header": "Arg order", + "multiSelect": false, + "options": [ + { + "label": "Standardize both to (dataset, evaluator) order (Recommended)", + "description": "Change run_batch signature to `run_batch(dataset, evaluator)` in the beta. Consistent with run_eval, intuitive (dataset first, evaluator second), and v2 is a beta with breaking-change latitude. Fixes the cognitive trap before the API is widely adopted." + }, + { + "label": "Enforce keyword-only arguments on both", + "description": "Add `*` to both signatures: `run_eval(*, dataset, evaluator)` and `run_batch(*, evaluator, dataset)`. Callers must use keyword args; positional mistakes become TypeErrors immediately. Preserves the current positional order while making silent misuse impossible." + }, + { + "label": "Retain reversed order, add prominent docs warning", + "description": "Keep the current signatures and add a big warning box in the docs: 'Note: run_eval and run_batch take arguments in reversed order.' Accepts the cognitive trap but documents it. Lowest implementation cost." + }, + { + "label": "Retain as intentional \u2014 it mirrors the underlying evaluator API convention", + "description": "The reversal follows an existing convention in the underlying evaluator library. Document the rationale in the API reference so the design looks intentional, not accidental." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 \u2014 Pass 2 (API Design): `run_eval(dataset, evaluator)` and `run_batch(evaluator, dataset)` have REVERSED positional order for the same concepts. The plan calls this intentional. A developer who writes `run_eval(my_dataset, my_evaluator)` then calls `run_batch` the same way silently passes wrong types \u2014 no error until runtime, potentially confusing results. Fix or keep? ": "Standardize both to (dataset, evaluator) order (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:07:30.245Z" + }, + { + "sessionId": "4cbd2d66-f875-4fcd-b4e2-6c8a6d6716cd", + "toolUseId": "toolu_01Cnqz7g1gYHL4r6BeFYpyMV", + "questions": [ + { + "question": "D5 \u2014 Pass 5 (Upgrade Path): v1 exposed `Client.evaluate()`. v2 removes it immediately and replaces it with `Client.run()`. No alias, no deprecation warning, no migration guide, no codemod. Every existing v1 user who runs `pip install evalkit==2.0.0b1` and calls `Client.evaluate()` gets an `AttributeError` with no guidance. The plan retains this. What bridge should the beta include? ", + "header": "v1\u2192v2 upgrade", + "multiSelect": false, + "options": [ + { + "label": "Deprecation alias + migration guide (Recommended)", + "description": "Add `Client.evaluate = Client.run` with a DeprecationWarning: 'evaluate() is deprecated in v2; use run() instead.' Add one section to the changelog explaining the rename. The alias makes the upgrade non-breaking for v1 users; the warning tells them to update. Removes entirely in v3." + }, + { + "label": "Migration guide only, no alias", + "description": "Write a v1\u2192v2 migration section in CHANGELOG.md (and/or README) documenting `evaluate()` \u2192 `run()`. No code change. Developers who read changelogs before upgrading are fine; developers who just run pip upgrade hit an AttributeError and must consult docs." + }, + { + "label": "Hard break as-is, add clear error message", + "description": "Keep the hard break but improve the AttributeError: catch `__getattr__` and raise a descriptive error: `AttributeError: Client.evaluate() was renamed to Client.run() in v2. See CHANGELOG.md.` The developer knows exactly what to do, no alias needed." + }, + { + "label": "Retain as-is \u2014 it's a beta, accept the break", + "description": "Beta users accept breaking changes. The changelog mentions it. Developers who upgrade without reading the changelog chose to take the risk. No additional bridge needed." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D5 \u2014 Pass 5 (Upgrade Path): v1 exposed `Client.evaluate()`. v2 removes it immediately and replaces it with `Client.run()`. No alias, no deprecation warning, no migration guide, no codemod. Every existing v1 user who runs `pip install evalkit==2.0.0b1` and calls `Client.evaluate()` gets an `AttributeError` with no guidance. The plan retains this. What bridge should the beta include? ": "Deprecation alias + migration guide (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:07:44.259Z" + } +] diff --git a/test/fixtures/eng-scope-y-calls.json b/test/fixtures/eng-scope-y-calls.json new file mode 100644 index 000000000..af4cc05c7 --- /dev/null +++ b/test/fixtures/eng-scope-y-calls.json @@ -0,0 +1,298 @@ +[ + { + "sessionId": "9ed23ef7-5203-441b-a12b-3072cd2a37c3", + "toolUseId": "toolu_014ZtRY6HSfP2REUjjNrip1t", + "questions": [ + { + "question": "D1 \u2014 Run /office-hours before the review, or proceed directly? ", + "header": "Prerequisite", + "multiSelect": false, + "options": [ + { + "label": "Skip \u2014 proceed with standard review (Recommended)", + "description": "Jump straight into the 4-section engineering review of the Multi-tenant Auth Refactor plan. The plan content in PLAN.md is sufficient to review. Recommended since the plan is already written." + }, + { + "label": "Run /office-hours first", + "description": "Run /office-hours to generate a structured design doc (problem statement, premise challenge, alternatives explored) before reviewing. Takes ~10 min but gives sharper input for the review." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 \u2014 Run /office-hours before the review, or proceed directly? ": "Skip \u2014 proceed with standard review (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:10:50.391Z" + }, + { + "sessionId": "9ed23ef7-5203-441b-a12b-3072cd2a37c3", + "toolUseId": "toolu_01AwzNWpnh8ZNFxJ1pv7WDWM", + "questions": [ + { + "question": "D2 \u2014 This plan introduces 4 new classes across 12 files. Recommend scope reduction before reviewing, or accept the complexity and review as-is? ", + "header": "Scope", + "multiSelect": false, + "options": [ + { + "label": "Accept complexity \u2014 review as-is (Recommended)", + "description": "Proceed with the full review of all 4 classes across 12 files. Flag any specific overengineering during the Architecture section, but don't block on scope reduction now. Recommended: the scope smell is already called out in the plan and the review will surface whether it's justified." + }, + { + "label": "Recommend scope reduction first", + "description": "Propose a minimal version (e.g. collapse SessionMint and RequestPolicy to plain modules, reduce new classes to 2) and ask the user to confirm before reviewing the full plan. This risks re-scoping before we understand the full design rationale." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 \u2014 This plan introduces 4 new classes across 12 files. Recommend scope reduction before reviewing, or accept the complexity and review as-is? ": "Accept complexity \u2014 review as-is (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:11:12.415Z" + }, + { + "sessionId": "9ed23ef7-5203-441b-a12b-3072cd2a37c3", + "toolUseId": "toolu_01MBnBLdpxZXtFmMY1A18k23", + "questions": [ + { + "question": "D3 \u2014 Should gstack search learnings from your other projects on this machine when reviewing? ", + "header": "Learnings", + "multiSelect": false, + "options": [ + { + "label": "Enable cross-project learnings (Recommended)", + "description": "Searches learnings from all your projects on this machine for patterns that might apply here. Stays local \u2014 no data leaves your machine. Best for solo devs." + }, + { + "label": "Keep learnings project-scoped only", + "description": "Only searches learnings from this specific project. Best if you work on multiple client codebases where cross-contamination is a concern." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 \u2014 Should gstack search learnings from your other projects on this machine when reviewing? ": "Enable cross-project learnings (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:11:50.496Z" + }, + { + "sessionId": "9ed23ef7-5203-441b-a12b-3072cd2a37c3", + "toolUseId": "toolu_01PtJSe9at9pWFh9ai9UkZ6b", + "questions": [ + { + "question": "D4 \u2014 [P1] (confidence: 9/10) Architecture \u2014 AuthBroker and SessionMint share a global mutable AuthCache. How should this be fixed? ", + "header": "Arch: Cache", + "multiSelect": false, + "options": [ + { + "label": "Inject AuthCache as constructor dependency (Recommended)", + "description": "Pass AuthCache into AuthBroker and SessionMint at construction time. No global export. Clear ownership. Fully testable in isolation. Eliminates race window from concurrent mutations. ~30 min human / ~5 min CC. Explicit > clever, your stated preference." + }, + { + "label": "Keep global export, add a mutex/lock wrapper", + "description": "Wrap the global AuthCache in an async lock or mutex so concurrent mutations are serialized. Fixes the race but preserves the shared-state design, serializes all cache ops (throughput hit), and still makes tests depend on global state. ~1h human / ~10 min CC." + }, + { + "label": "Accept as-is \u2014 document the concurrency risk", + "description": "Add a comment noting the race condition and require callers to coordinate. Defers the real fix, leaves a known race in the auth path of a multi-tenant system. Not recommended for any production auth refactor." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 \u2014 [P1] (confidence: 9/10) Architecture \u2014 AuthBroker and SessionMint share a global mutable AuthCache. How should this be fixed? ": "Inject AuthCache as constructor dependency (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:12:20.559Z" + }, + { + "sessionId": "9ed23ef7-5203-441b-a12b-3072cd2a37c3", + "toolUseId": "toolu_01MgvhzKuPyd4Tw2SdMx131J", + "questions": [ + { + "question": "D5 \u2014 [P1] (confidence: 9/10) Code Quality \u2014 validateAndDispatch() swallows errors in 3 nested try/catch blocks. How should this be remedied? ", + "header": "CQ: Errors", + "multiSelect": false, + "options": [ + { + "label": "Split into focused functions + propagate errors properly (Recommended)", + "description": "Extract validate(), dispatch(), and per-error-class handlers as separate functions. Remove silent catches \u2014 each error propagates or is explicitly re-thrown with context. Result: testable, explicit, auditable. Auth errors are never silently absorbed. ~1h human / ~10 min CC. Explicit > clever." + }, + { + "label": "Keep structure, add logging + re-throw on each catch", + "description": "Retain the 60-line function but replace bare catch swallowing with `logger.error(err); throw new AuthError(...)`. Catches remain nested but errors are no longer silent. Easier diff, but the testability and cognitive-complexity problems remain. ~30 min human / ~5 min CC." + }, + { + "label": "Accept as-is \u2014 document that each catch is intentional", + "description": "Add a comment explaining why each error class is swallowed. Does not fix the silent-failure risk or the testability gap. Not recommended for auth-path code." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D5 \u2014 [P1] (confidence: 9/10) Code Quality \u2014 validateAndDispatch() swallows errors in 3 nested try/catch blocks. How should this be remedied? ": "Split into focused functions + propagate errors properly (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:12:52.630Z" + }, + { + "sessionId": "9ed23ef7-5203-441b-a12b-3072cd2a37c3", + "toolUseId": "toolu_01VS1G4fLtYx6UiLBBtXVr5G", + "questions": [ + { + "question": "D6 \u2014 [P2] (confidence: 8/10) Tests \u2014 No test planned for concurrent AuthBroker + SessionMint mutations on the shared AuthCache (even after DI fix, concurrent callers with the same injected instance can race). Add a concurrency test? ", + "header": "Test: Concurrency", + "multiSelect": false, + "options": [ + { + "label": "Add a concurrency unit test (Recommended)", + "description": "Write a test that fires AuthBroker.dispatch() and SessionMint.mint() simultaneously against the same AuthCache instance and asserts no phantom 401s or corrupted state. Catches races before they hit prod. ~45 min human / ~8 min CC. Completeness: 9/10." + }, + { + "label": "Skip \u2014 rely on integration tests", + "description": "Trust that the multi-tenant integration tests will incidentally exercise concurrent paths. Cheaper up front, but integration tests rarely simulate true concurrency and won't catch the specific race window. Completeness: 6/10." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D6 \u2014 [P2] (confidence: 8/10) Tests \u2014 No test planned for concurrent AuthBroker + SessionMint mutations on the shared AuthCache (even after DI fix, concurrent callers with the same injected instance can race). Add a concurrency test? ": "Add a concurrency unit test (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:13:50.763Z" + }, + { + "sessionId": "9ed23ef7-5203-441b-a12b-3072cd2a37c3", + "toolUseId": "toolu_01CL2SAu6pxMyrjn511ufZ7i", + "questions": [ + { + "question": "D7 \u2014 [P2] (confidence: 7/10) Tests \u2014 No E2E test planned for the tenant-suspended-mid-session flow (user holds a valid token when their tenant is suspended \u2014 next request should be rejected). Add it? ", + "header": "Test: Suspend", + "multiSelect": false, + "options": [ + { + "label": "Add E2E test for suspend-mid-session (Recommended)", + "description": "Simulate: tenant active \u2192 valid token issued \u2192 tenant suspended \u2192 subsequent request with old token \u2192 assert 401 + session cleared. This is an auth/security flow \u2014 too important to trust unit tests alone. [E2E]. ~1h human / ~10 min CC. Completeness: 9/10." + }, + { + "label": "Skip \u2014 the invalidation hook test covers this", + "description": "The existing adapter invalidation-hook tests (unchanged per plan) already verify that suspend triggers cache eviction. Trusts existing coverage for the full user-facing flow. Completeness: 7/10." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D7 \u2014 [P2] (confidence: 7/10) Tests \u2014 No E2E test planned for the tenant-suspended-mid-session flow (user holds a valid token when their tenant is suspended \u2014 next request should be rejected). Add it? ": "Add E2E test for suspend-mid-session (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:13:58.776Z" + }, + { + "sessionId": "9ed23ef7-5203-441b-a12b-3072cd2a37c3", + "toolUseId": "toolu_019Cq2oKpq46kjtuMF5B9VTi", + "questions": [ + { + "question": "D8 \u2014 [P2] (confidence: 9/10) Performance \u2014 5 sequential IDP calls per validation, trivially parallelizable via Promise.all. Fix it in this PR or defer? ", + "header": "Perf: IDP", + "multiSelect": false, + "options": [ + { + "label": "Fix it now \u2014 add Promise.all parallelization (Recommended)", + "description": "Replace the 5 sequential IDP calls with Promise.all([call1, call2, call3, call4, call5]). The plan already confirms they're independent. Reduces validation latency by ~4x (e.g. 250ms \u2192 ~60ms). With CC this is a 5-minute fix. No reason to defer. ~30 min human / ~5 min CC. Completeness: 10/10." + }, + { + "label": "Defer to a follow-up PR", + "description": "Ship the refactor first, then optimize. Accepts the latency regression for the PR window. Only reasonable if the auth refactor is already blocked on other reviews and the 250ms is acceptable for now. Completeness: 7/10." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D8 \u2014 [P2] (confidence: 9/10) Performance \u2014 5 sequential IDP calls per validation, trivially parallelizable via Promise.all. Fix it in this PR or defer? ": "Fix it now \u2014 add Promise.all parallelization (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:14:14.818Z" + }, + { + "sessionId": "9ed23ef7-5203-441b-a12b-3072cd2a37c3", + "toolUseId": "toolu_01Cxdaph2p8QL1oq6Dci4HSC", + "questions": [ + { + "question": "D9 \u2014 TODO: Add ASCII data-flow diagrams in code comments for AuthBroker, AuthCache, and validateAndDispatch()? ", + "header": "TODO: Diagrams", + "multiSelect": false, + "options": [ + { + "label": "Add to TODOS.md", + "description": "Track a TODO to add ASCII diagrams in the auth service files showing the broker \u2192 cache \u2192 adapter \u2192 IDP flow and the validateAndDispatch execution tree. Prevents future engineers from having to reconstruct this mentally." + }, + { + "label": "Build it now in this PR instead of deferring", + "description": "Add the diagrams inline as part of the implementation. Diagrams are cheapest to write when the code is fresh. Same branch, minimal extra effort with CC." + }, + { + "label": "Skip \u2014 not valuable enough", + "description": "The code will be readable without diagrams. Skip tracking this." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D9 \u2014 TODO: Add ASCII data-flow diagrams in code comments for AuthBroker, AuthCache, and validateAndDispatch()? ": "Add to TODOS.md" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:14:48.903Z" + }, + { + "sessionId": "9ed23ef7-5203-441b-a12b-3072cd2a37c3", + "toolUseId": "toolu_01M81oEinkaiFavpCYHhLqJj", + "questions": [ + { + "question": "D10 \u2014 TODO: Add token revocation flow to the RequestPolicy class (the plan doesn't describe how RequestPolicy handles mid-flight policy version changes)? ", + "header": "TODO: Policy", + "multiSelect": false, + "options": [ + { + "label": "Add to TODOS.md", + "description": "Track a TODO to clarify how RequestPolicy handles a policy version bump mid-session (e.g., tenant admin upgrades policy while users are authenticated \u2014 do existing sessions get the old or new policy?). Important for multi-tenant correctness." + }, + { + "label": "Build it now in this PR instead of deferring", + "description": "Define the RequestPolicy versioning semantics in the plan now. Add tests for policy version bump mid-session. Closes the ambiguity before implementation starts." + }, + { + "label": "Skip \u2014 not valuable enough", + "description": "Treat this as an implementation detail \u2014 the current plan is sufficient. The existing policy version in the cache key already implies snapshot semantics." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D10 \u2014 TODO: Add token revocation flow to the RequestPolicy class (the plan doesn't describe how RequestPolicy handles mid-flight policy version changes)? ": "Add to TODOS.md" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T11:14:56.920Z" + } +] diff --git a/test/helpers/ceo-approach-pick.ts b/test/helpers/ceo-approach-pick.ts index 788e4fdb1..053f47b4f 100644 --- a/test/helpers/ceo-approach-pick.ts +++ b/test/helpers/ceo-approach-pick.ts @@ -19,7 +19,13 @@ export function pickCeoRecommendedApproach(fp: AskUserQuestionFingerprint): numb /^Which implementation approach should this plan (?:use|follow)\?(?:\s|$)/i.test(question); const testApproach = qid === 'plan-ceo-review-impl-approach' && /^Which implementation approach for the [a-z_$][\w$]*(?:\.[a-z_$][\w$]*)*\(\) tests\?(?:\s|$)/i.test(question); - if (!planApproach && !testApproach) return null; + // A named component can be the subject instead of "this plan". Consume the + // complete direct question; the routing id alone cannot authorize a choice. + const directQuestion = question.replace(/\s*]+>\s*$/i, '').trim(); + const component = /^Which implementation approach for (?:the|this) ((?:[a-z_$][\w$.-]*\s+){0,5})(?:handler|endpoint|service|module|component|adapter|client|worker|pipeline|integration)\?$/i.exec(directQuestion); + const componentApproach = /^plan-ceo(?:-review)?-approach(?:-selection)?$/.test(qid) && + component !== null && !/\b(?:and|or|then)\b/i.test(component[1]!); + if (!planApproach && !testApproach && !componentApproach) return null; if (fp.options.length !== q.options.length || !fp.options.every((option, i) => option.index === i + 1 && option.label === q.options[i]!.label)) return null; const labels = q.options.map(option => option.label.trim()); diff --git a/test/helpers/ceo-completion-handoff.ts b/test/helpers/ceo-completion-handoff.ts index a5f3079b0..d8b7b57cd 100644 --- a/test/helpers/ceo-completion-handoff.ts +++ b/test/helpers/ceo-completion-handoff.ts @@ -155,6 +155,26 @@ function clearRequiredEngNavigation(question: string, descriptions: string[], co closedNavigationContext(context); } +/** A bare next-workflow choice is administration, never proof of completed review. */ +function bareEngNavigation(question: string, descriptions: string[]): boolean { + if (!/^run \/plan-eng-review\?$/i.test(question) || descriptions.some(s => !s.trim())) return false; + const sentences = descriptions.flatMap(s => s.trim().split(/\n+|[.!](?:\s+|$)/)) + .map(s => s.trim().replace(/^\[[+-]\]\s*/, '')).filter(Boolean); + const approved = /^Proceed directly to implementation with the approved changes from this CEO review$/i; + const gate = /^Eng Review is the required shipping gate$/i; + // Consume the complete offered context. Past findings and already-approved + // changes are recaps; an added remedy or unfinished-review choice is not. + return sentences.some(s => approved.test(s)) && sentences.some(s => gate.test(s)) && + sentences.every(s => approved.test(s) || gate.test(s) || + /^It covers architecture depth, code quality, test gaps, and performance [—–-] complementing what this CEO review found$/i.test(s) || + /^Since this CEO review expanded the plan \(added [a-z0-9_ +/-]{1,120} requirements\), a fresh eng review is especially valuable$/i.test(s) || + /^Required before shipping; catches implementation issues the plan-level review cannot$/i.test(s) || + /^This CEO review found critical issues \([a-z0-9_ +/-]{1,80}\) [—–-] eng review will verify the fix approach is architecturally sound$/i.test(s) || + /^Adds another review session before implementation starts$/i.test(s) || + /^Faster path to implementation$/i.test(s) || + /^Eng review is the required shipping gate [—–-] skipping it means less confidence before enabling the feature flag$/i.test(s)); +} + /** Shared closed-review guards; next-review sequencing is still navigation. */ function closedNavigationContext(context: string): boolean { const unfinished = context.replace(/\b(?:no|0)\s+unresolved\s+(?:decisions|gaps|issues|findings)\b/gi, ''); @@ -170,7 +190,7 @@ function closedNavigationContext(context: string): boolean { !/(?:^|[.!?;]\s+|\b(?:proceed to|continue to|should|must|will|need to|can|could|would)\s+)(?:(?:please|first|then|also)\s+)*(?:add|fix|implement|resolve|decide)\b/im.test(context); } -/** Only the CEO's finished-review menu, never a finding/TODO mentioning another skill. */ +/** Closed CEO next-review navigation; native terminal/report checks prove completion separately. */ function manualHandoffIndex(fp: AskUserQuestionFingerprint): number | null { const call = fp.nativeCall; // The capture path assigns this native identity only after matching the @@ -208,6 +228,10 @@ function manualHandoffIndex(fp: AskUserQuestionFingerprint): number | null { const clearCompletion = !id && call.failed === false && q.options.length === 2 && clearRequiredEngNavigation(questionText, q.options.map(option => option.description ?? ''), gateContext); const completion = explicitCompletion || describedCompletion || metadataCompletion || describedEngCompletion || describedPostReviewCompletion || countedCompletion || clearCompletion; + const bareNavigation = Boolean(id) && call.failed === false && q.options.length === 2 && + (fp.nativeQuestionIndex === undefined || fp.nativeQuestionIndex === 0) && + fp.options.length === 2 && fp.options.every((o, i) => o.index === i + 1 && o.label === q.options[i]!.label) && + bareEngNavigation(questionText, q.options.map(o => o.description ?? '')); const requiredEng = /(?:\bEng(?:ineering)?\s+review|\/plan-eng-review)\b[^.!?]{0,180}\brequired(?:\s+shipping)?\s+gate\b/i.test(gateContext) || /\brequired(?:\s+shipping)?\s+gate\s+is\s+(?:an?\s+)?(?:Eng(?:ineering)?\s+review|\/plan-eng-review)\b/i.test(gateContext) || pronounEngGate(questionText, q.options.map(option => option.description ?? ''), gateContext); @@ -215,9 +239,9 @@ function manualHandoffIndex(fp: AskUserQuestionFingerprint): number | null { // the question or a following recap cannot hide a new repair obligation. if (id && /^(?:ceo-plan-next-steps|ceo-review-next-(?:steps?|review))$/.test(id) && !closedReviewNavigation(declaration, gateContext)) return null; - // A qid names the menu; it cannot replace its completed-review declaration - // or authorize another fix. The named gate can be explained in a choice. - if (!/^next\s+(?:review|steps?)$/i.test(q.header.trim()) || !completion || !requiredEng) return null; + // A qid alone cannot authorize another fix. The bare navigation arm grants + // no completion credit; native Exit, report freshness and finding floor remain independent. + if (!/^next\s+(?:review|steps?)$/i.test(q.header.trim()) || !(completion || bareNavigation) || !requiredEng) return null; const labels = q.options.map(o => o.label.trim().replace(/^[A-Z][).]\s*/i, '').replace(/\s*\(recommended\)\s*$/i, '').trim()); if (clearCompletion && !labels.some(label => /^Run\s+\/plan-eng-review(?:\s+(?:next|now))?$/i.test(label))) return null; diff --git a/test/helpers/ceo-section-loading-fixture.ts b/test/helpers/ceo-section-loading-fixture.ts index d87ecf048..fe080f93d 100644 --- a/test/helpers/ceo-section-loading-fixture.ts +++ b/test/helpers/ceo-section-loading-fixture.ts @@ -173,7 +173,7 @@ function hasProseStaleFillFinding(report: string): boolean { 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; + const fillPattern = /\b(?:fill|refill|repopulat|populat|insert|stor|restor)\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; const fill = fillPattern.test(text); const invalidation = /\b(?:invalidat|evict|write|commit|delet)\w*\b/i.test(text); const ordering = /\b(?:after|later|resum\w*)\b|out[- ]of[- ]order/i.test(text); @@ -221,6 +221,7 @@ function hasProseStaleFillFinding(report: string): boolean { || /\b(?:accepted|expected|intentional|documented)\s+(?:invariant|behavior|trade[- ]off|stale[- ]read\s+window)\b|\b(?:allowed|permitted|acceptable)\b/i.test(allowanceText) || acceptedStaleModel || (!proposedPrevention && /\b(?:cannot|can't|never|does not|will not)\s+(?:\w+\s+){0,3}(?:refill|repopulate|populate|insert|store|cache|set|violate)\b/i.test(claim)) + || (!proposedPrevention && /\b(?:cannot|can't|never|does not|doesn't|will not|won't|did not|didn't|is not|isn't|was not|wasn't|has not|hasn't|had not|hadn't)\s+(?:\w+\s+){0,3}restor\w*\b/i.test(claim)) || /\bno\s+(?:fix|change|coordination|guard)\s+(?:is\s+)?(?:needed|required)\b/i.test(claim); if (!dismissal) continue; const originalCaller = /\b(?:original|already[- ]pending)\s+(?:pending\s+)?(?:caller|reader|request)\b|\bpending\s+caller\b/i.test(claim); diff --git a/test/helpers/claude-pty-runner.ts b/test/helpers/claude-pty-runner.ts index eaaece509..5b4b5fbbc 100644 --- a/test/helpers/claude-pty-runner.ts +++ b/test/helpers/claude-pty-runner.ts @@ -2043,8 +2043,36 @@ export const ceoFirstReviewAUQ: Step0BoundaryPredicate = (fp) => return omission && amendment; }) ?? false; +/** A closed whole-plan complexity choice sets review scope, not an issue remedy. */ +function engWholePlanSetupAUQ(fp: AskUserQuestionFingerprint): boolean { + const call = fp.nativeCall; + if (call?.answered !== true || call.failed !== false || call.questions.length !== 1 || + !Array.isArray(call.unansweredQuestionIndices) || call.unansweredQuestionIndices.length || + fp.signature !== `${call.sessionId}:${call.toolUseId}`) return false; + const q = call.questions[0]!; + if (q.multiSelect || !/^Scope$/i.test(q.header.trim()) || q.options.length !== 2 || + new Set(q.options.map(o => o.label)).size !== 2 || + q.options.filter(o => o.label === call.answers?.[q.question]).length !== 1 || + fp.options.length !== 2 || !fp.options.every((o, i) => o.index === i + 1 && o.label === q.options[i]!.label)) return false; + const ids = [...q.question.matchAll(/]+)>/gi)]; + if (ids.length !== 1 || (q.question.match(/]+>\s*$/i, '').trim().replace(/\s+/g, ' '); + const counts = /^D\s*\d+\s*[—–:-]\s*This plan introduces ([1-9]\d*) new classes across ([1-9]\d*) files\. Recommend scope reduction before reviewing, or accept the complexity and review as-is\?$/i.exec(body); + if (!counts || !counts.slice(1).every(n => Number.isFinite(Number(n)))) return false; + const label = (s: string) => s.trim().replace(/\s*\(recommended\)$/i, ''); + const accept = q.options.find(o => /^Accept complexity\s*[—–-]\s*review as-is$/i.test(label(o.label))); + const reduce = q.options.find(o => /^Recommend scope reduction first$/i.test(label(o.label))); + if (!accept || !reduce) return false; + const description = (s: string) => s.trim().replace(/\s+/g, ' '); + const accepted = /^Proceed with the full review of all ([1-9]\d*) classes across ([1-9]\d*) files\. Flag any specific overengineering during the Architecture section, but don't block on scope reduction now\. Recommended: the scope smell is already called out in the plan and the review will surface whether it's justified\.$/i.exec(description(accept.description ?? '')); + return Boolean(accepted && accepted[1] === counts[1] && accepted[2] === counts[2] && + /^Propose a minimal version(?: \([^()\n]*\))? and ask the user to confirm before reviewing the full plan\. This risks re-scoping before we understand the full design rationale\.$/i.test(description(reduce.description ?? ''))); +} + /** Native setup needs an answered scope decision, not a particular model-chosen qid. */ export const engSetupAUQ: Step0BoundaryPredicate = (fp) => { + if (engWholePlanSetupAUQ(fp)) return true; const call = fp.nativeCall; if (!call?.answered || call.failed) return false; const answered = call.questions.filter(q => Boolean(call.answers?.[q.question])); diff --git a/test/helpers/design-count-review.ts b/test/helpers/design-count-review.ts index 63d69d4ec..db077b295 100644 --- a/test/helpers/design-count-review.ts +++ b/test/helpers/design-count-review.ts @@ -2,16 +2,55 @@ import { designFirstReviewAUQ } from './claude-pty-runner'; import type { AskUserQuestionFingerprint } from './claude-pty-runner'; import { pickDesignCountOutsideVoices } from './design-count-outside'; +/** Choosing reviewer participation is setup, even when numbered or asked late. */ +export function isDesignCountSetup(fp: AskUserQuestionFingerprint): boolean { + const call = fp.nativeCall; + if (!call?.answered || call.failed || call.questions.length !== 1 || + call.unansweredQuestionIndices?.length || fp.signature !== `${call.sessionId}:${call.toolUseId}`) return false; + const q = call.questions[0]!; + if (q.multiSelect || !/^outside(?: design)? voices$/i.test(q.header.trim()) || + (q.question.match(/\s*$/.test(q.question) || + (q.question.match(/\?/g)?.length ?? 0) !== 1 || + !/^(?:D\s*\d+(?:\s*\(Step\s*0[A-Z]?\))?\s*[—–:-]\s*)?(?:Run|Want|Include|Enable)\s+outside(?: design)? voices\s+(?:before|for)\s+the\s+(?:detailed\s+)?(?:design\s+)?review(?:\s+passes)?\?/i.test(q.question.trim())) return false; + const labels = q.options.map(option => option.label.trim().replace(/\s*\(recommended\)\s*$/i, '')); + // Consume the entire menu, not just its opening question or action labels. + // Unknown explanatory prose can contain a second product decision. + const remainder = q.question.slice(q.question.indexOf('?') + 1).replace(/]+>\s*$/, '').trim(); + if (remainder && !/^(?:Codex evaluates the design; a Claude subagent reviews completeness\.|Codex evaluates against OpenAI's design hard rules \+ litmus checks; a Claude subagent does an independent completeness review\. \(Requires Codex CLI to be installed\.\))$/.test(remainder)) return false; + const descriptions = q.options.map(option => (option.description ?? '').trim().replace(/\s+/g, ' ')); + const noDescription = /^(?:Skip Codex \+ Claude subagent outside pass\. Best for this case: it's a scoped settings form update with a complete DESIGN\.md; hard-rejection checks apply to marketing surfaces, not OPERATE\/settings UI\.|Skip outside voices and go straight to the 7 review passes\. Faster; sufficient for most plans\.)$/; + const yesDescription = /^(?:Run Codex against OpenAI design hard rules \+ litmus checks, and a separate Claude subagent for an independent completeness review\. Adds time but catches anything a single-model pass misses\.|Launches Codex design critique \+ Claude subagent completeness review in parallel before the 7 passes\. Adds 1[–-]2 minutes\.)$/; + if (labels.some((label, index) => descriptions[index] && + !(/^No\b/.test(label) ? noDescription : yesDescription).test(descriptions[index]!))) return false; + const no = labels.filter(label => /^No(?:\s*[,—–-]\s*|\s+)proceed without$/i.test(label)); + const yes = labels.filter(label => /^Yes(?:\s*[,—–-]\s*|\s+)run (?:outside(?: design)? voices|Codex \+ Claude subagent)$/i.test(label)); + return labels.length === 2 && no.length === 1 && yes.length === 1 && + q.options.some(option => option.label === call.answers?.[q.question]); +} + /** A completed finding can start the passes when the caller already supplied the focus. */ export function isDesignCountFirstReview(fp: AskUserQuestionFingerprint): boolean { const call = fp.nativeCall; if (!call?.answered || call.failed) return false; + if (isDesignCountSetup(fp)) return false; if (designFirstReviewAUQ(fp)) return true; return call.questions.some(q => { if (!call.answers?.[q.question] || q.options.length < 2) return false; if (/^(?:focus|scope|learnings|routing|next steps?|outside(?: design)? voices)$/i.test(q.header.trim())) return false; const id = //i.exec(q.question)?.[1] ?? ''; if (/(?:^|-)(?:focus|scope|setup|routing|learnings|onboarding|next-steps?|posture|mockups?|target)(?:-|$)/i.test(id)) return false; + // Native fingerprints prepend the menu header. Inspect the actual question + // for an explicit finding that offers a plan amendment and deferral. + if (call.answered === true && call.failed === false && /^Pass\s*[1-7]\s*\([^)]*\)\s*[—–:]\s*Finding\s*[1-9]\d*:\s+\S/i.test(q.question.trim()) && + /^plan-design-review-[a-z0-9-]+$/i.test(id) && + (q.question.match(/]+>\s*$/i.test(q.question) && + q.options.some(option => /^(?:Apply|Add|Fix|Specify|Define|Restore)\b/i.test(option.label)) && + q.options.some(option => /^(?:Defer|Leave|Keep as-is|Accept the gap)\b/i.test(option.label)) && + q.options.some(option => option.label === call.answers?.[q.question]) && + Array.isArray(call.unansweredQuestionIndices) && !call.unansweredQuestionIndices.length && + fp.signature === `${call.sessionId}:${call.toolUseId}`) return true; // A named or scored pass can ask for a missing design requirement before a // numbered finding heading appears. Its actual decision and opposed // choices establish review; a score or familiar qid alone cannot. @@ -73,12 +112,35 @@ function closedDesignGateRecap(tail: string, descriptions: string[]): boolean { /^You have \d+ (?:concrete )?(?:implementation )?tasks ready to build from$/i.test(part))); } +/** A qidless closed handoff must consume every question/description clause. */ +function resolvedDesignHandoff(q: NonNullable['questions'][number]): number | null { + if (!/^next review$/i.test(q.header.trim()) || q.options.length !== 2) return null; + const completed = /^Design review complete [—–-] (?:10|[0-9](?:\.\d+)?)\/10 (?:→|->) (?:10|[0-9](?:\.\d+)?)\/10\. All ([1-9]\d*) decisions resolved\. The plan is design-complete; next is the required shipping gate\. What['’]s next\?$/.exec(q.question.trim()); + if (!completed) return null; + const labels = q.options.map(o => o.label.trim().replace(/\s*\(recommended\)\s*$/i, '')); + const review = labels.findIndex(label => /^Run \/plan-eng-review$/i.test(label)); + const manual = labels.findIndex(label => /^Skip\s*[—–-]\s*I['’]ll handle next steps manually$/i.test(label)); + if (review < 0 || manual < 0 || review === manual) return null; + const description = (index: number) => (q.options[index]!.description ?? '').trim().replace(/\s+/g, ' '); + const topics = '(?:spinner|skeleton|(?:button|switch|field) (?:keyboard|focus|loading|error|disabled|pending|success)|(?:keyboard|focus|loading|error|disabled|pending|success) (?:states?|behavior|navigation))'; + const recap = new RegExp('^Eng review is the required shipping gate\\. It validates architecture, component wiring, tests, and accessibility implementation against the ' + completed[1] + ' approved design decisions\\. This design review added interaction specs \\(' + topics + '(?:, ' + topics + ')*\\), so eng review needs to validate their architectural fit\\.$'); + if (!recap.test(description(review)) || + !/^End the review workflow here\. The improved plan is at the (?:e2e output|approved plan) path; implementation can begin\. Run \/plan-eng-review later before shipping\.$/.test(description(manual))) return null; + return manual + 1; +} + function designHandoff(fp: AskUserQuestionFingerprint): { manualIndex: number | null } | null { const call = fp.nativeCall; if (!call || call.failed || call.questions.length !== 1 || fp.signature !== `${call.sessionId}:${call.toolUseId}`) return null; const q = call.questions[0]!; - if (q.multiSelect || q.options.length < 2 || !/^next\s+steps?$/i.test(q.header.trim())) return null; + if (q.multiSelect || q.options.length < 2) return null; + const pending = call.answered === false && call.answers === undefined && call.answeredAt === undefined && + (call.unansweredQuestionIndices === undefined || (Array.isArray(call.unansweredQuestionIndices) && + call.unansweredQuestionIndices.length === 1 && call.unansweredQuestionIndices[0] === 0)); + const resolvedManual = call.failed === false && (call.answered === true || pending) ? resolvedDesignHandoff(q) : null; + if (resolvedManual !== null) return { manualIndex: resolvedManual }; + if (!/^next\s+steps?$/i.test(q.header.trim())) return null; const ids = [...q.question.matchAll(//gi)].map(match => match[1]); if ((q.question.match(/]+)>/gi)]; if (ids.length !== 1 || (q.question.match(/ + o.index === i + 1 && o.label === q.options[i]!.label)) return false; + const body = q.question.replace(/\s*]+>\s*$/i, '').trim(); + const timing = /^D\s*\d+\s*[—–:-]\s*Pass 1 \(Getting Started\): The agreed <(\d+(?:\.\d+)?) min TTHW target is mathematically impossible with the retained (\d+(?:\.\d+)?)[- ](?:min|minute) CI block\. Which resolution belongs in the plan\?$/i.exec(body); + if (!timing) return false; + const [target, wait] = timing.slice(1).map(Number); + const selected = call.answers![q.question]!.replace(/\s*\(Recommended\)\s*$/i, '').trim(); + return [target, wait].every(n => Number.isFinite(n) && n! > 0) && wait! >= target! && + /^(?:Demo-only CI bypass|Add --offline flag to [a-z_$][\w$.-]*|Update TTHW target to reflect reality)$/i.test(selected); + } 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. diff --git a/test/helpers/touchfiles-data.ts b/test/helpers/touchfiles-data.ts index 0fd76b2fb..63007a454 100644 --- a/test/helpers/touchfiles-data.ts +++ b/test/helpers/touchfiles-data.ts @@ -148,7 +148,7 @@ export const E2E_TOUCHFILES: Record = { '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', 'test/fixtures/ceo-section-u-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', 'test/fixtures/ceo-section-y-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= to scope cost to the changed @@ -160,17 +160,17 @@ export const E2E_TOUCHFILES: Record = { // Each test drives its skill end-to-end; touchfiles include preamble + // completion-status resolvers because they affect question cadence and // terminal output (the regression surface this test catches). - 'plan-ceo-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-ceo-review/**', 'test/skill-ceo-section-ordering.test.ts', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/plan-count-completion.test.ts', 'test/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', 'test/fixtures/ceo-completion-handoff-v-call.json', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/plan-count-ceo-body-finding.test.ts', 'test/fixtures/ceo-count-w-paired.json', 'test/fixtures/ceo-completion-handoff-w-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', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.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', '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', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/helpers/design-artifact-question.ts', 'test/design-artifact-question.test.ts', 'test/fixtures/design-artifacts-w-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', 'test/fixtures/devex-empathy-v-calls.json', 'test/fixtures/devex-handoff-v-call.json', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.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', 'test/fixtures/ceo-completion-handoff-v-call.json', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/plan-count-ceo-body-finding.test.ts', 'test/fixtures/ceo-count-w-paired.json', 'test/fixtures/ceo-completion-handoff-w-call.json', 'test/fixtures/ceo-approach-y-call.json', 'test/fixtures/ceo-approach-y-screen.txt', 'test/ceo-handoff-y.test.ts', 'test/fixtures/ceo-handoff-y-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', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/eng-scope-y.test.ts', 'test/fixtures/eng-scope-y-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', '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', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/helpers/design-artifact-question.ts', 'test/design-artifact-question.test.ts', 'test/fixtures/design-artifacts-w-calls.json', 'test/fixtures/design-outside-y-calls.json', 'test/fixtures/design-boundaries-y-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', 'test/fixtures/devex-empathy-v-calls.json', 'test/fixtures/devex-handoff-v-call.json', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/fixtures/devex-count-y-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 // review-phase AskUserQuestion). Uses runPlanSkillFloorCheck — minimal // "did agent fire ANY AUQ?" observer that exits early on first non-permission // numbered-option render. ~1-3 min typical wall time per test, ~$2-6 total. - 'plan-eng-finding-floor': ['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/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/fixtures/forcing-finding-seeds.ts', 'test/skill-e2e-plan-eng-finding-floor.test.ts', 'test/eng-first-review-t.test.ts', 'test/fixtures/eng-batching-t-calls.json'], + 'plan-eng-finding-floor': ['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/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/fixtures/forcing-finding-seeds.ts', 'test/skill-e2e-plan-eng-finding-floor.test.ts', 'test/eng-first-review-t.test.ts', 'test/fixtures/eng-batching-t-calls.json', 'test/eng-scope-y.test.ts', 'test/fixtures/eng-scope-y-calls.json'], 'plan-ceo-finding-floor': ['bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-ceo-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/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/fixtures/forcing-finding-seeds.ts', 'test/skill-e2e-plan-ceo-finding-floor.test.ts'], 'plan-design-finding-floor': ['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', 'scripts/resolvers/review.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/fixtures/forcing-finding-seeds.ts', 'test/skill-e2e-plan-design-finding-floor.test.ts'], 'plan-devex-finding-floor': ['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', 'scripts/resolvers/review.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/fixtures/forcing-finding-seeds.ts', 'test/skill-e2e-plan-devex-finding-floor.test.ts'], @@ -180,7 +180,7 @@ export const E2E_TOUCHFILES: Record = { // 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', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-screen.txt', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.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', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/eng-scope-y.test.ts', 'test/fixtures/eng-scope-y-calls.json'], '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', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.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'], diff --git a/test/skill-e2e-plan-design-finding-count.test.ts b/test/skill-e2e-plan-design-finding-count.test.ts index fafcac1ce..b7e58d76e 100644 --- a/test/skill-e2e-plan-design-finding-count.test.ts +++ b/test/skill-e2e-plan-design-finding-count.test.ts @@ -10,7 +10,7 @@ import { test } from 'bun:test'; import { describeE2ETier } from './helpers/e2e-gate'; -import { isDesignCountFirstReview, isDesignCompletionHandoff, pickDesignCountQuestion } from './helpers/design-count-review'; +import { isDesignCountFirstReview, isDesignCountSetup, isDesignCompletionHandoff, pickDesignCountQuestion } from './helpers/design-count-review'; import { isDesignArtifactGeneration } from './helpers/design-artifact-question'; import * as fs from 'node:fs'; import * as os from 'node:os'; @@ -194,6 +194,7 @@ describeE2E('/plan-design-review per-finding AskUserQuestion count (periodic)', expectedPlanPath: planPath, isLastStep0AUQ: designStep0Boundary, isFirstReviewAUQ: isDesignCountFirstReview, + isSetupAUQ: isDesignCountSetup, isCompletionHandoffAUQ: isDesignCompletionHandoff, isArtifactGenerationAUQ: isDesignArtifactGeneration, fixtureFiles: { 'DESIGN.md': designSystem },