From 17ee2e542896c2556b3affe5d5f900f911c7a4d1 Mon Sep 17 00:00:00 2001 From: garrytan Date: Wed, 30 Sep 2026 20:44:26 +0000 Subject: [PATCH] test(review-army): record N+1's pre-dispatch stages and scope the session to Step 4.5 review-army-perf-n-plus-one timed out in 7 of 13 CI runs on this branch (passing 245-280 s of 300). Each session spent ~95 s on setup (the full extracted SKILL, checklist, section greps, exploratory.md, diff-scope/stats/learnings, tooling checks), ran Step 4's core pass, a search-before-recommending WebSearch, and wrote a 10-16 KB report (~100 s after the Red Team returned). The fixture now stages only review/sections/review-army.md plus the performance and red-team checklists, and hands the session the recorded detect-scope, specialist-stats and learnings outputs and the diff. The caller passes --performance (every CI parent already treated the prompt as that force flag against the <50-line skip), declares the core pass, QA, adversarial review, web research, Fix-First and persistence out of scope, and caps the report at the selection line, the SPECIALIST REVIEW block and the Red Team result (30 lines). The Performance specialist and the conditional Red Team are still real foreground subagents, and the report still has to surface the N+1. New assertion: a foreground Performance specialist dispatch precedes the Red Team dispatch. Free controls omit the Performance dispatch or background it, and both fail; the budget lifecycle adapter supplies the current result shape. Touchfiles now include the .rb fixture the case reads. --- test/helpers/touchfiles-data.ts | 2 +- test/review-army-budget.test.ts | 3 +- test/review-n-plus-one-contract.test.ts | 4 +- test/skill-e2e-review-army.test.ts | 68 +++++++++++++++++++++---- 4 files changed, 64 insertions(+), 13 deletions(-) diff --git a/test/helpers/touchfiles-data.ts b/test/helpers/touchfiles-data.ts index 43ba89019..d63829236 100644 --- a/test/helpers/touchfiles-data.ts +++ b/test/helpers/touchfiles-data.ts @@ -123,7 +123,7 @@ export const E2E_TOUCHFILES: Record = { // Review Army (specialist dispatch) 'review-army-migration-safety': [ 'review/**', 'scripts/resolvers/review-army.ts', 'bin/gstack-diff-scope', 'test/skill-e2e-review-army.test.ts', 'test/helpers/office-hours-attempt.ts'], - 'review-army-perf-n-plus-one': [ 'review/**', 'scripts/resolvers/review-army.ts', 'bin/gstack-diff-scope', 'test/skill-e2e-review-army.test.ts', 'test/fixtures/review-n-plus-one-dispatch.json', 'test/helpers/office-hours-attempt.ts'], + 'review-army-perf-n-plus-one': [ 'review/**', 'scripts/resolvers/review-army.ts', 'bin/gstack-diff-scope', 'test/skill-e2e-review-army.test.ts', 'test/fixtures/review-n-plus-one-dispatch.json', 'test/fixtures/review-army-n-plus-one.rb', 'test/helpers/office-hours-attempt.ts'], 'review-army-delivery-audit': [ 'review/**', 'scripts/resolvers/review.ts', 'scripts/resolvers/review-army.ts', 'test/skill-e2e-review-army.test.ts', 'test/helpers/office-hours-attempt.ts'], 'review-army-quality-score': [ 'review/**', 'scripts/resolvers/review-army.ts', 'test/skill-e2e-review-army.test.ts', 'test/helpers/office-hours-attempt.ts'], 'review-army-json-findings': [ 'review/**', 'scripts/resolvers/review-army.ts', 'test/skill-e2e-review-army.test.ts', 'test/helpers/office-hours-attempt.ts'], diff --git a/test/review-army-budget.test.ts b/test/review-army-budget.test.ts index ddb51f595..30eb4ac1f 100644 --- a/test/review-army-budget.test.ts +++ b/test/review-army-budget.test.ts @@ -37,7 +37,8 @@ const run = async opts => { expect(fs.existsSync(opts.workingDirectory)).toBe(true); if (attempt === 2) fs.writeFileSync(path.join(opts.workingDirectory, 'review-output.md'), ${JSON.stringify(report)}); return { exitReason: attempt === 1 ? 'timeout' : 'success', browseErrors: [], - toolCalls: [{ tool: 'Agent', input: { description: 'Red Team review', run_in_background: false }, output: 'NO FINDINGS' }] }; + toolCalls: [{ tool: 'Agent', input: { description: 'Performance specialist review', run_in_background: false }, output: 'NO FINDINGS' }, + { tool: 'Agent', input: { description: 'Red Team review', run_in_background: false }, output: 'NO FINDINGS' }] }; }; new Function('describe', 'test', 'expect', 'beforeAll', 'afterAll', 'JUDGE_MS', 'CAPTURE_MS', 'SESSION_DRAIN_GRACE_MS', 'runSkillTest', diff --git a/test/review-n-plus-one-contract.test.ts b/test/review-n-plus-one-contract.test.ts index 9ebf3fdaa..473ba6694 100644 --- a/test/review-n-plus-one-contract.test.ts +++ b/test/review-n-plus-one-contract.test.ts @@ -4,7 +4,7 @@ import * as os from 'node:os'; import * as path from 'node:path'; import {spawnSync} from 'node:child_process'; const ROOT = path.resolve(import.meta.dir, '..'); -test.each(['complete-control', 'captured-omission', 'claimed-only', 'background', 'missing-report', 'unrelated-report', 'captured-timeout']) +test.each(['complete-control', 'captured-omission', 'claimed-only', 'background', 'missing-report', 'unrelated-report', 'captured-timeout', 'performance-omitted', 'performance-background']) ('N+1 registered completion contract: %s', scenario => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'n1-contract-')); const facts = path.join(dir, 'facts.json'); @@ -18,6 +18,8 @@ const actual=await import(path.join(root,'test/helpers/session-runner.ts')); const fixture=JSON.parse(fs.readFileSync(path.join(root,'test/fixtures/review-n-plus-one-dispatch.json'),'utf8')); const data=structuredClone(['captured-omission','claimed-only'].includes(scenario)?fixture.omission:fixture.ci); if(scenario==='background')data.events[1].message.content[0].input.run_in_background=true; +if(scenario==='performance-background')data.events[0].message.content[0].input.run_in_background=true; +if(scenario==='performance-omitted')data.events.shift(); const parsed=actual.parseNDJSON(data.events.map(e=>JSON.stringify(e))); let prompt=''; mock.module(path.join(root,'test/helpers/e2e-helpers.ts'),()=>({ diff --git a/test/skill-e2e-review-army.test.ts b/test/skill-e2e-review-army.test.ts index 70c2fe2aa..a0f20c354 100644 --- a/test/skill-e2e-review-army.test.ts +++ b/test/skill-e2e-review-army.test.ts @@ -148,8 +148,26 @@ Write your findings to ${dir}/review-output.md`, let nPlusOneCaptureSequence = 0; +// The case's contract is Step 4.5 selection -> a foreground Performance specialist -> +// the Step 4.6 merge -> the conditional Red Team -> a report that surfaces the N+1. +// The fixture records the stages before it (scope detection, specialist stats, +// learnings, the diff) and stages only the Review Army section, so the session does +// not spend its budget on the core pass, QA loading, web research or a long report. +// Scope flags are gstack-diff-scope's output for this fixture before staging; +// recorded rather than rerun so the free controls stay runnable without bash. +const N_PLUS_ONE_SCOPE = `SCOPE_FRONTEND=false +SCOPE_BACKEND=true +SCOPE_PROMPTS=false +SCOPE_TESTS=false +SCOPE_DOCS=false +SCOPE_CONFIG=false +SCOPE_MIGRATIONS=false +SCOPE_API=true +SCOPE_AUTH=false`; + describeIfSelected('Review Army: N+1 Performance', ['review-army-perf-n-plus-one'], () => { let dir: string; + let observations: string; beforeAll(() => { const repo = setupRepo('army-n-plus-one'); @@ -167,26 +185,49 @@ describeIfSelected('Review Army: N+1 Performance', ['review-army-perf-n-plus-one repo.run('git', ['add', '.']); repo.run('git', ['commit', '-m', 'add posts controller']); - copyReviewFiles(dir); + const git = (args: string[]) => spawnSync('git', args, { cwd: dir, encoding: 'utf-8', timeout: 5000 }).stdout ?? ''; + const diff = git(['diff', 'main...HEAD']); + if (!diff.includes('posts_controller.rb')) throw new Error('N+1 fixture diff is missing posts_controller.rb'); + const diffLines = [...git(['diff', '--shortstat', 'main...HEAD']).matchAll(/(\d+) (?:insertion|deletion)/g)] + .reduce((sum, match) => sum + Number(match[1]), 0); + observations = `$ gstack-diff-scope main +${N_PLUS_ONE_SCOPE} +STACK: unknown +DIFF_LINES: ${diffLines} +TEST_FW: unknown +$ gstack-specialist-stats +SPECIALIST_STATS: 0 reviews analyzed +$ gstack-learnings-search --type pitfall --query "performance" --limit 5 +(no output: no past learnings) +$ git diff $(git merge-base main HEAD) +${diff.trimEnd()}`; + + fs.writeFileSync(path.join(dir, 'review-army.md'), readReviewSection('review-army.md')); + const specDir = path.join(dir, 'review-specialists'); + fs.mkdirSync(specDir, { recursive: true }); + for (const f of ['performance.md', 'red-team.md']) { + fs.copyFileSync(path.join(ROOT, 'review', 'specialists', f), path.join(specDir, f)); + } }); afterAll(() => { try { fs.rmSync(dir, { recursive: true, force: true }); } catch {} }); testConcurrentIfSelected('review-army-perf-n-plus-one', async () => { const result = await runSkillTest({ - prompt: `You are in a git repo on a feature branch with a Ruby controller that has N+1 queries. -Read review-SKILL.md for instructions. Also read review-checklist.md. -The specialist checklists are in review-specialists/ (testing.md, performance.md, etc.). + prompt: `You are the /review parent on branch feature/add-posts-index, a Ruby controller change. The caller invoked /review --performance. +The base branch is main. There is no origin remote, so use main wherever the workflow says origin/. -Skip the preamble, lake intro, telemetry sections. -Run Step 4 (Critical pass) then Step 4.5 (Review Army). -The base branch is main. This is a Ruby backend file, so Performance specialist should activate. - -For the specialist dispatch, read review-specialists/performance.md and apply it against the diff. +This capture covers only Step 4.5 (Review Army), its Step 4.6 merge and the Red Team dispatch. Read review-army.md once: it is that workflow. +The core Step 4 pass, Exploratory QA, adversarial review, web research, Fix-First and review-log persistence are outside this capture; do not run or load them. Do not edit source. +The fixture already ran the workflow's detect-scope, specialist-stats and learnings commands and the diff. Use these recorded outputs instead of rerunning them: +\`\`\` +${observations} +\`\`\` +Selection: --performance force-includes the Performance specialist despite the small diff; dispatch no other specialist. The checklists for this capture are review-specialists/performance.md and review-specialists/red-team.md; give subagents those paths. The Performance focus does not waive the skill's conditional Red Team dispatch. If a specialist produces a CRITICAL finding, dispatch a separate foreground Red Team subagent and merge its findings. -Write all required review outputs to ${dir}/review-output.md. After saving the report, +Write ${dir}/review-output.md with only the selection line, the SPECIALIST REVIEW block with its PR Quality Score, and the Red Team result, in at most 30 lines. After saving the report, finish with a brief acknowledgement rather than repeating the findings in the final response.`, workingDirectory: dir, maxTurns: 20, @@ -204,6 +245,13 @@ finish with a brief acknowledgement rather than repeating the findings in the fi && /\bred[ -]team\b/i.test(call.input.description ?? call.input.subagent_type ?? '') && call.input.run_in_background === false, )).toBe(true); + const foreground = result.toolCalls.filter(call => + ['Agent', 'Task'].includes(call.tool) && call.input.run_in_background === false); + const label = (call: { input: any }) => call.input.description ?? call.input.subagent_type ?? ''; + const isRedTeam = (call: { input: any }) => /\bred[ -]team\b/i.test(label(call)); + const performance = foreground.findIndex(call => /\bperformance\b/i.test(label(call)) && !isRedTeam(call)); + expect(performance).toBeGreaterThanOrEqual(0); + expect(foreground.findIndex(isRedTeam)).toBeGreaterThan(performance); const outputPath = path.join(dir, 'review-output.md'); expect(fs.existsSync(outputPath)).toBe(true);