diff --git a/plan-ceo-review/sections/review-sections.md b/plan-ceo-review/sections/review-sections.md index 656e0e166..56fcd01ca 100644 --- a/plan-ceo-review/sections/review-sections.md +++ b/plan-ceo-review/sections/review-sections.md @@ -124,14 +124,19 @@ This section traces data through the system and interactions through the UI with For each node: what happens on each shadow path? Is it tested? **Async ordering:** For flows sharing mutable state, include a combined ASCII -schedule with one column per operation and one for shared state. At each `await`, +schedule with one column per operation and one for shared state. For each pair +of overlapping awaits that can affect an invariant, show both completion orders; +exclude an order only by naming the mechanism that prevents it. At each `await`, callback or job handoff: pause, let a competing operation complete, resume, then start a fresh consumer. Show the observed result and compare it with the exact -caller/time boundary of the stated invariant. If safe, name the mechanism that +caller/time boundary of the stated invariant. The invariant is a requirement, +not proof that the implementation meets it. If safe, name the mechanism that prevents the violating schedule. Separate flow diagrams do not prove ordering. -Atomic calls and a single thread do not make the whole flow atomic. An accepted -exception needs its exact contract clause; bounded damage is insufficient. Test -this schedule with controlled pause/release points. +One favorable schedule is insufficient. Single-thread execution and atomic calls +do not prevent interleaving across awaits. An accepted exception needs its exact +contract clause; bounded damage is insufficient. Test the relevant completion +orders with controlled pause/release points. Compare relevant pairs; exhaustive +permutations are unnecessary. **Interaction Edge Cases:** For every new user-visible interaction, evaluate: ``` diff --git a/plan-ceo-review/sections/review-sections.md.tmpl b/plan-ceo-review/sections/review-sections.md.tmpl index b09c02d22..f8dc2e5f4 100644 --- a/plan-ceo-review/sections/review-sections.md.tmpl +++ b/plan-ceo-review/sections/review-sections.md.tmpl @@ -122,14 +122,19 @@ This section traces data through the system and interactions through the UI with For each node: what happens on each shadow path? Is it tested? **Async ordering:** For flows sharing mutable state, include a combined ASCII -schedule with one column per operation and one for shared state. At each `await`, +schedule with one column per operation and one for shared state. For each pair +of overlapping awaits that can affect an invariant, show both completion orders; +exclude an order only by naming the mechanism that prevents it. At each `await`, callback or job handoff: pause, let a competing operation complete, resume, then start a fresh consumer. Show the observed result and compare it with the exact -caller/time boundary of the stated invariant. If safe, name the mechanism that +caller/time boundary of the stated invariant. The invariant is a requirement, +not proof that the implementation meets it. If safe, name the mechanism that prevents the violating schedule. Separate flow diagrams do not prove ordering. -Atomic calls and a single thread do not make the whole flow atomic. An accepted -exception needs its exact contract clause; bounded damage is insufficient. Test -this schedule with controlled pause/release points. +One favorable schedule is insufficient. Single-thread execution and atomic calls +do not prevent interleaving across awaits. An accepted exception needs its exact +contract clause; bounded damage is insufficient. Test the relevant completion +orders with controlled pause/release points. Compare relevant pairs; exhaustive +permutations are unnecessary. **Interaction Edge Cases:** For every new user-visible interaction, evaluate: ``` diff --git a/test/helpers/auq-sdk-capture.ts b/test/helpers/auq-sdk-capture.ts index c10f5da3c..4b3f3e626 100644 --- a/test/helpers/auq-sdk-capture.ts +++ b/test/helpers/auq-sdk-capture.ts @@ -262,10 +262,11 @@ export async function captureSectionReads(opts: { // Outside-review dispatch has separate behavioral coverage. Native-only // captures use the real supported control in state owned by this call; // never mutate the operator's or another capture's gstack configuration. + // Keep the model-facing config path relative to the fixture's working directory. const stateDir = opts.nativeReviewOnly - ? fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-section-state-')) : null; + ? fs.mkdtempSync(path.join(path.resolve(opts.planDir), '.gstack-section-state-')) : null; const nativeReviewRule = stateDir - ? `\n- Read ${path.join(stateDir, 'config.yaml')}, the isolated gstack configuration for this capture. It sets codex_reviews: disabled. Follow that documented control: skip the entire extra outside-review step, including its native fallback, and report outside coverage as disabled. Complete all native review sections and the full required report.` + ? `\n- Read ${path.relative(path.resolve(opts.planDir), path.join(stateDir, 'config.yaml'))}, the isolated gstack configuration for this capture. It sets codex_reviews: disabled. Follow that documented control: skip the entire extra outside-review step, including its native fallback, and report outside coverage as disabled. Complete all native review sections and the full required report.` : ''; // Preserve full method execution while avoiding a second written walkthrough // of decisions already represented in the amended plan and required outputs. diff --git a/test/session-runner-tools.test.ts b/test/session-runner-tools.test.ts index 3c0c57854..9c2ca0fdc 100644 --- a/test/session-runner-tools.test.ts +++ b/test/session-runner-tools.test.ts @@ -14,6 +14,10 @@ interface Observed { stateHome: string; stateRoot: string | null; outsideDisabled: boolean; + workingDirectory: string; + promptConfigPath: string | null; + promptConfig: string | null; + promptConfigMatchesState: boolean; } describe('outside-review disabled status in a completed report', () => { @@ -47,12 +51,28 @@ const FAKE_CLAUDE = String.raw` const stateHome = process.env.GSTACK_HOME; let config = ''; try { config = fs.readFileSync(path.join(stateHome, 'config.yaml'), 'utf8'); } catch {} + const promptConfigPath = prompt.match(/^- Read (.+), the isolated gstack configuration for this capture\./m)?.[1] ?? null; + let promptConfig = null; + let promptConfigMatchesState = false; + if (promptConfigPath) { + try { + const file = path.resolve(process.cwd(), promptConfigPath); + promptConfig = fs.readFileSync(file, 'utf8'); + promptConfigMatchesState = fs.realpathSync(file) === fs.realpathSync(path.join(stateHome, 'config.yaml')); + } catch {} + } const observed = { args: process.argv.slice(2), prompt, stateHome, stateRoot: process.env.GSTACK_STATE_ROOT ?? null, outsideDisabled: /^codex_reviews:\s*disabled\s*$/m.test(config), + workingDirectory: process.cwd(), promptConfigPath, promptConfig, promptConfigMatchesState, }; fs.writeFileSync('observed.json', JSON.stringify(observed)); + const coordinatedState = fs.existsSync('coordinate-configs') ? path.basename(stateHome) : null; + if (coordinatedState) { + fs.writeFileSync('pending-observed-' + coordinatedState + '.json', JSON.stringify(observed)); + fs.renameSync('pending-observed-' + coordinatedState + '.json', 'observed-' + coordinatedState + '.json'); + } const outputFile = fs.existsSync('active-plan-output') ? 'PLAN.md' : 'REPORT.md'; console.log(JSON.stringify({ type: 'system', subtype: 'init' })); if (fs.existsSync('diagnostic-case')) { @@ -103,6 +123,9 @@ const FAKE_CLAUDE = String.raw` console.log(JSON.stringify({ type: 'assistant', message: { content: [ { type: 'tool_use', name: 'Read', input: { file_path } }, ] } })); + if (coordinatedState) { + while (!fs.existsSync('release-' + coordinatedState)) await Bun.sleep(5); + } if (fs.existsSync('wait-for-report')) { fs.writeFileSync('work-ready', ''); while (!fs.existsSync('release-report')) await Bun.sleep(5); @@ -425,7 +448,11 @@ Rules for this run: expect(child.stateRoot).toBe(child.stateHome); expect(child.stateHome).not.toBe(hostState); expect(child.stateHome).not.toBe(getHermeticDirs().gstackHome); - expect(child.prompt).toContain(path.join(child.stateHome, 'config.yaml')); + expect(path.isAbsolute(child.stateHome)).toBe(true); + expect(path.dirname(child.stateHome)).toBe(path.resolve(dir)); + expect(child.promptConfigPath).toBe(path.join(path.basename(child.stateHome), 'config.yaml')); + expect(child.promptConfig).toBe('codex_reviews: disabled\n'); + expect(child.promptConfigMatchesState).toBe(true); expect(child.prompt).toContain('Complete all native review sections and the full required report.'); expect(child.prompt).toContain('report outside coverage as disabled'); expect(fs.existsSync(child.stateHome)).toBe(false); @@ -436,6 +463,66 @@ Rules for this run: }); }); + test('native-only config resolves from the prompt inside a nested fixture working directory', async () => { + await withFakeClaude(async (dir) => { + const planDir = path.join(dir, 'gstack-paid-shard-fixture', 'tmp', 'review fixture'); + fs.mkdirSync(planDir, { recursive: true }); + const result = await captureSectionReads({ + planDir, skillName: 'plan-ceo-review', + scenario: 'Review the full plan', testName: 'section-config-nested', + nativeReviewOnly: true, timeout: 5_000, + }); + const child: Observed = JSON.parse(fs.readFileSync(path.join(planDir, 'observed.json'), 'utf8')); + expect(fs.realpathSync(child.workingDirectory)).toBe(fs.realpathSync(planDir)); + expect(path.isAbsolute(child.stateHome)).toBe(true); + expect(path.dirname(child.stateHome)).toBe(path.resolve(planDir)); + expect(child.stateRoot).toBe(child.stateHome); + expect(child.promptConfigPath).toMatch(/^\.gstack-section-state-[^/\\]+[/\\]config\.yaml$/); + expect(path.isAbsolute(child.promptConfigPath!)).toBe(false); + expect(child.promptConfig).toBe('codex_reviews: disabled\n'); + expect(child.promptConfigMatchesState).toBe(true); + expect(child.outsideDisabled).toBe(true); + expect(result.reportProduced).toBe(true); + expect(fs.existsSync(child.stateHome)).toBe(false); + expect(fs.readdirSync(planDir).filter(name => name.startsWith('.gstack-section-state-'))).toEqual([]); + }); + }); + + test('overlapping native-only captures in one fixture retain independent config until each exits', async () => { + await withFakeClaude(async (dir) => { + fs.writeFileSync(path.join(dir, 'coordinate-configs'), ''); + const captures = [0, 1].map(index => captureSectionReads({ + planDir: dir, skillName: 'plan-ceo-review', scenario: 'Review the full plan', + testName: 'section-config-overlap-' + index, nativeReviewOnly: true, timeout: 5_000, + })); + const readChildren = (): Observed[] => fs.readdirSync(dir) + .filter(name => /^observed-.*\.json$/.test(name)) + .map(name => JSON.parse(fs.readFileSync(path.join(dir, name), 'utf8'))); + let children: Observed[] = []; + try { + const deadline = Date.now() + 3_000; + while ((children = readChildren()).length < 2 && Date.now() < deadline) await Bun.sleep(5); + expect(children).toHaveLength(2); + expect(new Set(children.map(child => child.stateHome)).size).toBe(2); + for (const child of children) { + expect(path.dirname(child.stateHome)).toBe(path.resolve(dir)); + expect(child.stateRoot).toBe(child.stateHome); + expect(child.promptConfigMatchesState).toBe(true); + expect(child.promptConfig).toBe('codex_reviews: disabled\n'); + } + fs.writeFileSync(path.join(dir, 'release-' + path.basename(children[0].stateHome)), ''); + expect((await Promise.race(captures)).reportProduced).toBe(true); + expect(fs.existsSync(children[0].stateHome)).toBe(false); + expect(fs.readFileSync(path.join(children[1].stateHome, 'config.yaml'), 'utf8')).toBe('codex_reviews: disabled\n'); + } finally { + for (const child of readChildren()) fs.writeFileSync(path.join(dir, 'release-' + path.basename(child.stateHome)), ''); + const results = await Promise.all(captures); + expect(results.every(result => result.reportProduced)).toBe(true); + } + for (const child of children) expect(fs.existsSync(child.stateHome)).toBe(false); + }); + }, 10_000); + test.each(['nonzero', 'is_error', 'error'])('a %s capture cannot pass using a report left behind before failure', async (failure) => { await withFakeClaude(async (dir, observed) => { fs.writeFileSync(path.join(dir, 'fail-cli'), failure); @@ -445,10 +532,27 @@ Rules for this run: reportMarker: /^## GSTACK REVIEW REPORT\s*$/m, }); expect(observed().outsideDisabled).toBe(true); + expect(observed().promptConfigMatchesState).toBe(true); expect(fs.existsSync(observed().stateHome)).toBe(false); expect(result.reportProduced).toBe(false); // Preserve useful diagnostics; failed completion does not erase output. expect(result.output).toContain('## GSTACK REVIEW REPORT'); }); }); + + test('a timed-out native-only capture removes its config after the CLI has read it', async () => { + await withFakeClaude(async (dir, observed) => { + fs.writeFileSync(path.join(dir, 'wait-for-report'), ''); + const result = await captureSectionReads({ + planDir: dir, skillName: 'plan-ceo-review', scenario: 'Review the full plan', + testName: 'section-config-timeout', nativeReviewOnly: true, timeout: 1_500, + }); + expect(fs.existsSync(path.join(dir, 'work-ready'))).toBe(true); + expect(observed().promptConfigMatchesState).toBe(true); + expect(observed().promptConfig).toBe('codex_reviews: disabled\n'); + expect(fs.existsSync(observed().stateHome)).toBe(false); + expect(result.reportProduced).toBe(false); + expect(fs.existsSync(path.join(dir, 'REPORT.md'))).toBe(false); + }); + }, 5_000); }); diff --git a/test/skill-ceo-section-ordering.test.ts b/test/skill-ceo-section-ordering.test.ts index 9e3c1da8d..c129092f2 100644 --- a/test/skill-ceo-section-ordering.test.ts +++ b/test/skill-ceo-section-ordering.test.ts @@ -123,7 +123,13 @@ describe('plan-ceo-review carve — static ordering', () => { expect(instructions).toContain('If safe, name the mechanism that prevents the violating schedule.'); expect(instructions).toContain('Separate flow diagrams do not prove ordering.'); expect(instructions).toContain('An accepted exception needs its exact contract clause; bounded damage is insufficient.'); - expect(instructions).toContain('Test this schedule with controlled pause/release points.'); + expect(instructions).toContain('For each pair of overlapping awaits that can affect an invariant, show both completion orders;'); + expect(instructions).toContain('exclude an order only by naming the mechanism that prevents it.'); + expect(instructions).toContain('The invariant is a requirement, not proof that the implementation meets it.'); + expect(instructions).toContain('One favorable schedule is insufficient.'); + expect(instructions).toContain('Single-thread execution and atomic calls do not prevent interleaving across awaits.'); + expect(instructions).toContain('Test the relevant completion orders with controlled pause/release points.'); + expect(instructions).toContain('Compare relevant pairs; exhaustive permutations are unnecessary.'); } });