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);