From 2bd4651cde81f468ed95dada0d06a84661d4c381 Mon Sep 17 00:00:00 2001 From: garrytan Date: Wed, 30 Sep 2026 20:53:37 +0000 Subject: [PATCH] test(review-army): share the recorded Step 4.5 staging with consensus and supply its Red Team review-army-consensus (periodic) timed out in 2 of 13 census sessions; passing runs took 213-297 s of 300. Like N+1 it spent ~30-50 s reading the whole extracted SKILL, checklist and every specialist file, sometimes dispatched an unrequested Maintainability specialist, then ran a Red Team (60-70 s) and a second merge before writing a 9-15 KB report. The N+1 staging and scope text move into stageReviewArmySession / reviewArmyScope / reviewArmyChecklists (the N+1 prompt renders byte-identical). Consensus now records its detect-scope, stats, learnings and diff, stages the Review Army section with the security and testing checklists, forces --security --testing, and caps the report like N+1. Its Red Team is outside the multi-specialist contract, so the fixture supplies a labeled synthetic NO FINDINGS result instead of a dispatch. The existing SQL-finding and browser-error assertions are unchanged; the lifecycle adapter's spawnSync now returns the git output the staging reads. --- test/review-consensus-lifecycle.test.ts | 2 +- test/skill-e2e-review-army.test.ts | 134 ++++++++++++++---------- 2 files changed, 81 insertions(+), 55 deletions(-) diff --git a/test/review-consensus-lifecycle.test.ts b/test/review-consensus-lifecycle.test.ts index f6694569f..463be81e0 100644 --- a/test/review-consensus-lifecycle.test.ts +++ b/test/review-consensus-lifecycle.test.ts @@ -13,7 +13,7 @@ async function exercise(scenarios: Array<'success'|'timeout'|'wrong-report'|'bro beforeAll:(fn:any)=>setups.push(fn),afterAll:(fn:any)=>done.push(fn), describeIfSelected:(_title:string,names:string[],fn:any)=>{if(names.includes('review-army-consensus'))fn();}, testConcurrentIfSelected:(name:string,fn:any,timeout:number)=>{expect(name).toBe('review-army-consensus');callbacks.push(fn);outer=timeout;}, - createEvalCollector:()=>({}),finalizeEvalCollector:()=>{},logCost:()=>{},spawnSync:()=>({status:0}),path:fixturePath,os:{tmpdir:()=>'/tmp'}, + createEvalCollector:()=>({}),finalizeEvalCollector:()=>{},logCost:()=>{},spawnSync:(_cmd:string,args:string[])=>({status:0,stdout:args[0]!=='diff'?'':args.includes('--shortstat')?' 1 file changed, 12 insertions(+)\n':'diff --git a/auth_controller.rb b/auth_controller.rb\n'}),path:fixturePath,os:{tmpdir:()=>'/tmp'}, fs:{mkdirSync:()=>{},readdirSync:()=>[],mkdtempSync:(p:string)=>p+'owned',writeFileSync:(p:string,s:string)=>files.set(p,s),copyFileSync:()=>{},rmSync:()=>{}, existsSync:(p:string)=>files.has(p),readFileSync:(p:string)=>p.startsWith(sourceRoot+fixturePath.sep)?'synthetic fixture bytes '.repeat(30):files.get(p)}, extractSkillSections:()=> 'Review instructions',REVIEW_ARMY_E2E_SECTIONS:[], diff --git a/test/skill-e2e-review-army.test.ts b/test/skill-e2e-review-army.test.ts index a0f20c354..6ef9a17f6 100644 --- a/test/skill-e2e-review-army.test.ts +++ b/test/skill-e2e-review-army.test.ts @@ -144,27 +144,61 @@ Write your findings to ${dir}/review-output.md`, }, CAPTURE_MS); }); +// Review Army sessions whose contract starts at specialist dispatch (N+1, consensus). +// The fixture records the Step 4.5 stages before dispatch (scope detection, specialist +// stats, learnings, the diff) and stages only the Review Army section and the named +// checklists, so the session does not spend its budget on the core pass, QA loading, +// web research or a long report. Each case's scope flags are gstack-diff-scope's +// output for its fixture before staging, recorded rather than rerun so the free +// controls stay runnable without bash. A subagent outside a case's contract (the +// consensus case's Red Team) is supplied as a labeled recorded result. +function stageReviewArmySession(dir: string, scopeFlags: string, specialists: string[], checklists: string[]): string { + const git = (args: string[]) => spawnSync('git', args, { cwd: dir, encoding: 'utf-8', timeout: 5000 }).stdout ?? ''; + const diff = git(['diff', 'main...HEAD']); + if (!diff.includes('diff --git')) throw new Error(`Review Army fixture in ${dir} has an empty diff against main`); + const diffLines = [...git(['diff', '--shortstat', 'main...HEAD']).matchAll(/(\d+) (?:insertion|deletion)/g)] + .reduce((sum, match) => sum + Number(match[1]), 0); + 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 name of checklists) { + fs.copyFileSync(path.join(ROOT, 'review', 'specialists', `${name}.md`), path.join(specDir, `${name}.md`)); + } + return `$ gstack-diff-scope main +${scopeFlags} +STACK: unknown +DIFF_LINES: ${diffLines} +TEST_FW: unknown +$ gstack-specialist-stats +SPECIALIST_STATS: 0 reviews analyzed +${specialists.map(name => `$ gstack-learnings-search --type pitfall --query "${name}" --limit 5 +(no output: no past learnings)`).join('\n')} +$ git diff $(git merge-base main HEAD) +${diff.trimEnd()}`; +} + +function reviewArmyScope(observations: string, stages: string): string { + return `The base branch is main. There is no origin remote, so use main wherever the workflow says origin/. + +This capture covers only Step 4.5 (Review Army), ${stages}. 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} +\`\`\``; +} + +function reviewArmyChecklists(checklists: string[]): string { + const paths = checklists.map(name => `review-specialists/${name}.md`); + return `The checklists for this capture are ${paths.slice(0, -1).join(', ')} and ${paths.at(-1)}; give subagents those paths.`; +} + // --- Review Army: N+1 Performance --- +// Contract: Step 4.5 selection -> a foreground Performance specialist -> the Step 4.6 +// merge -> the conditional Red Team -> a report that surfaces the N+1. 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; @@ -185,29 +219,15 @@ describeIfSelected('Review Army: N+1 Performance', ['review-army-perf-n-plus-one repo.run('git', ['add', '.']); repo.run('git', ['commit', '-m', 'add posts controller']); - 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)); - } + observations = stageReviewArmySession(dir, `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`, ['performance'], ['performance', 'red-team']); }); afterAll(() => { try { fs.rmSync(dir, { recursive: true, force: true }); } catch {} }); @@ -215,15 +235,8 @@ ${diff.trimEnd()}`; testConcurrentIfSelected('review-army-perf-n-plus-one', async () => { const result = await runSkillTest({ 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/. - -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. +${reviewArmyScope(observations, 'its Step 4.6 merge and the Red Team dispatch')} +Selection: --performance force-includes the Performance specialist despite the small diff; dispatch no other specialist. ${reviewArmyChecklists(['performance', 'red-team'])} 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. @@ -626,9 +639,12 @@ Start the file with "RED TEAM REVIEW" on the first line.`, }); // --- Review Army: Consensus (periodic) --- +// Contract: two forced specialists flag the same injection and the Step 4.6 merge +// surfaces it as MULTI-SPECIALIST CONFIRMED. describeIfSelected('Review Army: Consensus', ['review-army-consensus'], () => { let dir: string; + let observations: string; let consensusCaptureSequence = 0; beforeAll(() => { @@ -657,23 +673,33 @@ end repo.run('git', ['add', '.']); repo.run('git', ['commit', '-m', 'add auth controller']); - copyReviewFiles(dir); + observations = stageReviewArmySession(dir, `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=true`, ['security', 'testing'], ['security', 'testing']); }); afterAll(() => { try { fs.rmSync(dir, { recursive: true, force: true }); } catch {} }); testConcurrentIfSelected('review-army-consensus', async () => { const result = await runSkillTest({ - prompt: `You are reviewing a git diff with a SQL injection in an auth controller. -Read review-SKILL.md, review-checklist.md, and the specialist checklists in review-specialists/. + prompt: `You are the /review parent reviewing a git diff with a SQL injection in an auth controller, on branch feature/vuln-auth. The caller invoked /review --security --testing. +${reviewArmyScope(observations, 'its Step 4.6 merge and the multi-specialist confirmation')} +Selection: --security and --testing force-include those two specialists despite the small diff; dispatch no other specialist. ${reviewArmyChecklists(['security', 'testing'])} +The Red Team is outside this case's contract. When its activation condition is met, do not dispatch it: use this fixture-supplied (synthetic) Red Team result instead, and label it as supplied in the report: NO FINDINGS This vulnerability should be caught by BOTH the security specialist (injection vector) AND the testing specialist (no test for auth bypass). -Run the review. In your output, if a finding is flagged by multiple perspectives, +In your output, if a finding is flagged by multiple perspectives, mark it as "MULTI-SPECIALIST CONFIRMED" with the confirming categories. -Write findings to ${dir}/review-output.md`, +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, timeout: CAPTURE_MS,