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.
This commit is contained in:
garrytan committed 2026-09-30 20:54:35 +00:00
1 parent 17ee2e5428
commit 2bd4651cde
2 files changed
+81 -55

No files matched your search

+1 -1
View File
@@ -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), 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();}, 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;}, 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:()=>{}, 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)}, 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:[], extractSkillSections:()=> 'Review instructions',REVIEW_ARMY_E2E_SECTIONS:[],
+80 -54
View File
@@ -144,27 +144,61 @@ Write your findings to ${dir}/review-output.md`,
}, CAPTURE_MS); }, 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/<base>.
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 --- // --- 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; 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'], () => { describeIfSelected('Review Army: N+1 Performance', ['review-army-perf-n-plus-one'], () => {
let dir: string; let dir: string;
let observations: 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', ['add', '.']);
repo.run('git', ['commit', '-m', 'add posts controller']); repo.run('git', ['commit', '-m', 'add posts controller']);
const git = (args: string[]) => spawnSync('git', args, { cwd: dir, encoding: 'utf-8', timeout: 5000 }).stdout ?? ''; observations = stageReviewArmySession(dir, `SCOPE_FRONTEND=false
const diff = git(['diff', 'main...HEAD']); SCOPE_BACKEND=true
if (!diff.includes('posts_controller.rb')) throw new Error('N+1 fixture diff is missing posts_controller.rb'); SCOPE_PROMPTS=false
const diffLines = [...git(['diff', '--shortstat', 'main...HEAD']).matchAll(/(\d+) (?:insertion|deletion)/g)] SCOPE_TESTS=false
.reduce((sum, match) => sum + Number(match[1]), 0); SCOPE_DOCS=false
observations = `$ gstack-diff-scope main SCOPE_CONFIG=false
${N_PLUS_ONE_SCOPE} SCOPE_MIGRATIONS=false
STACK: unknown SCOPE_API=true
DIFF_LINES: ${diffLines} SCOPE_AUTH=false`, ['performance'], ['performance', 'red-team']);
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 {} }); afterAll(() => { try { fs.rmSync(dir, { recursive: true, force: true }); } catch {} });
@@ -215,15 +235,8 @@ ${diff.trimEnd()}`;
testConcurrentIfSelected('review-army-perf-n-plus-one', async () => { testConcurrentIfSelected('review-army-perf-n-plus-one', async () => {
const result = await runSkillTest({ 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. 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/<base>. ${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'])}
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 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. 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) --- // --- 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'], () => { describeIfSelected('Review Army: Consensus', ['review-army-consensus'], () => {
let dir: string; let dir: string;
let observations: string;
let consensusCaptureSequence = 0; let consensusCaptureSequence = 0;
beforeAll(() => { beforeAll(() => {
@@ -657,23 +673,33 @@ end
repo.run('git', ['add', '.']); repo.run('git', ['add', '.']);
repo.run('git', ['commit', '-m', 'add auth controller']); 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 {} }); afterAll(() => { try { fs.rmSync(dir, { recursive: true, force: true }); } catch {} });
testConcurrentIfSelected('review-army-consensus', async () => { testConcurrentIfSelected('review-army-consensus', async () => {
const result = await runSkillTest({ const result = await runSkillTest({
prompt: `You are reviewing a git diff with a SQL injection in an auth controller. 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.
Read review-SKILL.md, review-checklist.md, and the specialist checklists in review-specialists/. ${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) This vulnerability should be caught by BOTH the security specialist (injection vector)
AND the testing specialist (no test for auth bypass). 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. 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, workingDirectory: dir,
maxTurns: 20, maxTurns: 20,
timeout: CAPTURE_MS, timeout: CAPTURE_MS,