mirror of
https://github.com/garrytan/gstack.git
synced 2026-10-03 01:46:55 +02:00
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.
This commit is contained in:
1 parent
90203acda3
commit
17ee2e5428
4 files changed
+64
-13
No files matched your search
@@ -123,7 +123,7 @@ export const E2E_TOUCHFILES: Record<string, string[]> = {
|
||||
|
||||
// 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'],
|
||||
|
||||
@@ -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',
|
||||
|
||||
@@ -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'),()=>({
|
||||
|
||||
@@ -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/<base>.
|
||||
|
||||
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);
|
||||
|
||||
Reference in new issue
Block a user