review-army.ts
353 lines · scripts/resolvers/review-army.ts at 3f79d81
1/**
2 * Review Army resolver — parallel specialist reviewers for /review
3 *
4 * Generates template prose that instructs Claude to:
5 * 1. Detect stack and scope (via gstack-diff-scope)
6 * 2. Select and dispatch specialist subagents in parallel
7 * 3. Collect, parse, merge, and deduplicate JSON findings
8 * 4. Feed merged findings into the existing Fix-First pipeline
9 *
10 * Shipped as Release 2 of the self-learning roadmap (SELF_LEARNING_V0.md).
11 */
12import type { TemplateContext } from './types';
13import { CC_BACKGROUND_DEFAULT_SINCE, FOREGROUND_IF_AVAILABLE, BACKGROUND_RECOVERY } from './constants';
14import { learningsCapture, LEARNINGS_VERDICT } from './learnings';
15import { DEFENSIVE_REVIEW_FRAMING, FIXTURE_SUMMARY_MODE } from './defensive-review';
17function generateSpecialistSelection(ctx: TemplateContext): string {
18 const isShip = ctx.skillName === 'ship';
19 const stepSel = isShip ? '9.1' : '4.5';
20 const stepMerge = isShip ? '9.2' : '4.6';
21 const nextStep = isShip ? 'Step 9.3 (cross-review dedup)' : 'Step 4.8 (adversarial review), then Step 5';
22 return `## Step ${stepSel}: Review Army — Specialist Dispatch
24### Detect stack and scope
26\`\`\`bash
27_SM=$(${ctx.paths.binDir}/gstack-state-matrix detect --base <base>) || _SM=SCOPE_STATE_MACHINE=unknown; source <(echo "$_SM")
28source <(${ctx.paths.binDir}/gstack-diff-scope <base> 2>/dev/null) || true
29set | grep '^SCOPE_'
30STACK=""
31[ -f Gemfile ] && STACK="\${STACK}ruby "
32[ -f package.json ] && STACK="\${STACK}node "
33[ -f requirements.txt ] || [ -f pyproject.toml ] && STACK="\${STACK}python "
34[ -f go.mod ] && STACK="\${STACK}go "
35[ -f Cargo.toml ] && STACK="\${STACK}rust "
36[ -f pubspec.yaml ] && STACK="\${STACK}dart "
37echo "STACK: \${STACK:-unknown}"
38DIFF_BASE=$(git merge-base origin/<base> HEAD)
39DIFF_LINES=$(git diff "$DIFF_BASE" --numstat | awk '{n+=$1+$2} END {print n+0}')
40echo "DIFF_LINES: $DIFF_LINES"
41# Detect test framework
42TEST_FW=""
43{ [ -f jest.config.ts ] || [ -f jest.config.js ]; } && TEST_FW="jest"
44[ -f vitest.config.ts ] && TEST_FW="vitest"
45{ [ -f spec/spec_helper.rb ] || [ -f .rspec ]; } && TEST_FW="rspec"
46{ [ -f pytest.ini ] || [ -f conftest.py ]; } && TEST_FW="pytest"
47[ -f go.mod ] && TEST_FW="go-test"
48[ -f pubspec.yaml ] && TEST_FW="dart-test" && grep -qsE '^ +sdk: *.?flutter' pubspec.yaml && TEST_FW="flutter-test"
49echo "TEST_FW: \${TEST_FW:-unknown}"
50\`\`\`
52### Read specialist hit rates (adaptive gating)
54\`\`\`bash
55${ctx.paths.binDir}/gstack-specialist-stats 2>/dev/null || true
56\`\`\`
58### Select specialists
60Select specialists from the scope signals above.
62**Always-on (every review with 50+ changed lines):**
631. **Testing** — read \`${ctx.paths.skillRoot}/review/specialists/testing.md\`
642. **Maintainability** — read \`${ctx.paths.skillRoot}/review/specialists/maintainability.md\`
66**If DIFF_LINES < 50:** Skip all specialists except State Machines (item 9 decides it). Print: "Small diff ($DIFF_LINES lines) — specialists skipped." Continue to Step ${stepMerge} with the ${isShip ? 'core/design-lite' : 'core'} findings and an empty or State Machines-only specialist list, then the parent's Exploratory QA step and ${nextStep}. Small diffs skip fan-out, never the parent-owned smoke probes or core shared-code checks.
68**Conditional (dispatch when its scope signal is true):**
693. **Security** — if SCOPE_AUTH=true, OR if SCOPE_BACKEND=true AND DIFF_LINES > 100. Read \`${ctx.paths.skillRoot}/review/specialists/security.md\`
704. **Performance** — if SCOPE_BACKEND=true OR SCOPE_FRONTEND=true. Read \`${ctx.paths.skillRoot}/review/specialists/performance.md\`
715. **Data Migration** — if SCOPE_MIGRATIONS=true. Read \`${ctx.paths.skillRoot}/review/specialists/data-migration.md\`
726. **API Contract** — if SCOPE_API=true. Read \`${ctx.paths.skillRoot}/review/specialists/api-contract.md\`
737. **Design** — if SCOPE_FRONTEND=true. Use the existing design review checklist at \`${ctx.paths.skillRoot}/review/design-checklist.md\` and run the mechanical pass at the top of that checklist (the user-installed design detector, when present) before the LLM items
748. **Simplification** — if DIFF_LINES > 100. Read \`${ctx.paths.skillRoot}/review/specialists/simplification.md\`. Advisory-only lens: hunts unrequested structure (hand-rolled stdlib, one-implementation abstractions, dependencies duplicating platform features), never coverage.
759. **State Machines** — if SCOPE_STATE_MACHINE is true or unknown: read \`${ctx.paths.skillRoot}/review/specialists/state-machines.md\`, run \`${ctx.paths.binDir}/gstack-state-matrix outfile\`, Read the file it prints, write the full output there, then run \`${ctx.paths.binDir}/gstack-state-matrix check <file>\` on it.
77### Adaptive gating
79After scope-based selection, apply adaptive gating based on specialist hit rates:
81For each conditional specialist that passed scope gating, read the \`gstack-specialist-stats\` output:
82- If tagged \`[GATE_CANDIDATE]\` (0 findings in 10+ dispatches): skip it. Print: "[specialist] auto-gated (0 findings in N reviews)."
83- If tagged \`[NEVER_GATE]\`: always dispatch regardless of hit rate.
85**Force flags:** \`--<specialist>\` (e.g. \`--simplification\`) or \`--all-specialists\` in the prompt force-includes it.
87Print the selection:
88"Dispatching N specialists: [names]. Skipped: [names] (scope not detected). Gated: [names] (0 findings in N+ reviews)."`;
89}
91/**
92 * The diff command every dispatched reviewer runs. In /ship the parent writes the
93 * base sha into the prompt (the merge base on pass 1, RANGE_BASE from pass 2 on):
94 * a reviewer is its own process and never sees the parent's shell variables, so a
95 * `$RANGE_BASE` reference there would silently fall back to the whole branch.
96 */
97function diffBase(ctx: TemplateContext): string {
98 return ctx.skillName === 'ship' ? 'DIFF_BASE=<diff-base>' : 'DIFF_BASE=$(git merge-base origin/<base> HEAD)';
99}
101function generateSpecialistDispatch(ctx: TemplateContext): string {
102 return `### Dispatch specialists in parallel
104For each selected specialist, launch an independent subagent via the Agent tool.
105**Launch ALL selected specialists in a single message** (multiple Agent tool calls)
106so they run in parallel. Each subagent has fresh context — no prior review bias.
107${ctx.skillName === 'ship' ? `
108**Fan-out (\`--review-fanout auto|serial|parallel\` in the prompt; default \`auto\`).**
109Under \`auto\`, when the host reports fewer free subagent slots than selected
110specialists (Capy caps a thread at 8 running subagents, and waiting or idle ones hold
111a slot), dispatch in batches of the free-slot count, at least one per batch, each
112batch in one message, and start the next batch only after every reader of the
113previous one has finished or is confirmed stopped; print \`Fan-out: N specialists in
114M batches of K\`. A host that reports no slot count runs one parallel batch.
115\`serial\` is batches of one; \`parallel\` is one batch regardless of slots (a refused
116launch is a failed reader, never silently dropped coverage). Batching changes the
117order, never the set: every selected specialist still runs before Step 9.2 merges.
118` : ''}
119**Each specialist subagent prompt:**
121Each specialist prompt includes:
1231. The specialist's checklist path from the selection above (the subagent reads it; never paste its content)
1242. Stack context: "This is a {STACK} project."
1253. Past learnings for this domain (if any exist):
127\`\`\`bash
128${learningsCapture(`${ctx.paths.binDir}/gstack-learnings-search --type pitfall --query "{specialist domain}" --limit 5`)}
129${LEARNINGS_VERDICT}
130\`\`\`
132If learnings are found, include them: "Past learnings for this domain: {learnings}"
1344. **Defensive framing (Security specialist and the Red Team below only).** Begin the prompt with:
135"${DEFENSIVE_REVIEW_FRAMING}"
136and replace its full-diff command with: "${FIXTURE_SUMMARY_MODE}"
137Every other specialist, including Testing, reads the full diff with fixtures.
1395. Instructions:
141"You are a specialist code reviewer. Read the checklist at {checklist path}, then run
142\`${diffBase(ctx)} && git diff "$DIFF_BASE"${ctx.skillName === 'ship' ? ' <exclude>' : ''}\` to get the ${ctx.skillName === 'ship' ? 'diff (`<diff-base>`: write the sha in before dispatch, the merge base on pass 1 and `RANGE_BASE` from pass 2 on; never a shell variable; `<exclude>`: everything after `<diff-base>` on this reviewer\'s `DIFF_CMD:` line, verbatim; empty on `DIFF_EXCLUDE: none`)' : 'full diff'}. Apply the checklist against the diff.
144\`NO FINDINGS\` alone is the expected, complete result for a clean diff; never invent an item. Otherwise output one JSON object per finding per line, 400 words at most:
145{\\"severity\\":\\"CRITICAL|INFORMATIONAL\\",\\"confidence\\":N,\\"path\\":\\"file\\",\\"line\\":N,\\"category\\":\\"category\\",\\"summary\\":\\"description\\",\\"fix\\":\\"recommended fix\\",\\"fingerprint\\":\\"path:line:category\\",\\"specialist\\":\\"name\\"}
147Required fields: severity, confidence, path, category, summary, specialist.
148Optional: line, fix, fingerprint, evidence, test_stub, advisory, evidence_paths, helper_target, cause.
149CRITICAL means a test that would fail, a production break, or a security or data-loss defect; everything else is INFORMATIONAL.
150A defect the diff causes but that shows elsewhere (a caller broken by a changed signature, a consumer of a removed export) keeps the victim as \`path\` and names the changed line in \`cause\`: {\\"cause\\":{\\"path\\":\\"file\\",\\"line\\":N}}. Without it the finding reads as pre-existing code.
152Leave extraction advice to the core shared-code check; report real defects in duplicated code independently, keep advisory metadata on structural advice, and never call a demonstrated defect advisory because a shared helper could fix it.
154If you can write a test that would catch this issue, include it in the \`test_stub\` field.
155Use the detected test framework ({TEST_FW}). Write a minimal skeleton — describe/it/test
156blocks with clear intent. Skip test_stub for architectural or design-only findings.
158Output nothing else: no preamble, no summary or commentary.
160Stack context: {STACK}
161Past learnings: {learnings or 'none'}"
163**Subagent configuration:**
164- Use \`subagent_type: "general-purpose"\`
165- Pass ${FOREGROUND_IF_AVAILABLE} on every specialist Agent call — background is the default since ${CC_BACKGROUND_DEFAULT_SINCE}; omitting an available flag is not foreground. ${BACKGROUND_RECOVERY}
167**Wait for readers before editing:**
168- Confirm that each task has finished or is stopped. A timeout alone does not prove termination. If a reader or writer is still active, wait; if its state is unknown, inspect its task/process status. If you cannot confirm it stopped, use the parent's Fix-First stop path without edits.
169- A failed task may be stopped without having completed its review. Record the failure and retain usable partial findings.
170- Continue independent evidence collection after a terminal failure. Missing dispatched coverage remains incomplete, never completed or clean; successful peers cannot replace it.`;
171}
173function generateFindingsMerge(ctx: TemplateContext): string {
174 const isShip = ctx.skillName === 'ship';
175 const stepMerge = isShip ? '9.2' : '4.6';
176 const fixFirstRef = isShip ? 'Step 9.3 dedup, then Step 9.4 Fix-First' : 'Step 5 Fix-First';
177 const critPassRef = isShip ? 'the checklist pass (Step 9)' : 'the CRITICAL pass findings from Step 4';
178 const persistRef = isShip ? 'the review-log persist' : 'the review-log entry in Step 5.8';
179 return `### Step ${stepMerge}: Collect and merge findings
181Follow these stages in order. Validate core and specialist findings alike, but keep
182their source labels: specialist scoring is not the final review's defect count.
184#### 1. Parse outputs
186After specialist attempts settle, collect their outputs, tagged by actual source.
187\`NO FINDINGS\` is a completed, clean result, never a failed reviewer. Otherwise parse
188each JSON line and skip invalid lines. Missing or unusable output is incomplete
189coverage, not an empty success. Keep each specialist's returned findings for activity stats.${isShip ? `
190An output that carries a state-machine matrix record (State Machines, or a batched
191reader that read its checklist) is parsed from \`${ctx.paths.binDir}/gstack-state-matrix findings <file>\`,
192after \`check\` ran on the raw file: an untested cell that is the only finding on its
193cell comes back INFORMATIONAL (\`"state_matrix":"untested-cell"\`), the test-gap ruling
194in \`docs/review-policy.md\`; a wrong row or collapsed kinds stays CRITICAL.` : ''}
196#### 2. Validate severity
198For core and specialist findings with \`"severity":"CRITICAL"\` and \`"advisory":true\`,
199remove \`advisory\` and retain its \`CRITICAL\` severity. Treat these as defects before
200identity, merging, counting, scoring or Fix-First. Never downgrade severity to make
201advisory metadata consistent. Valid INFORMATIONAL advisories remain advisory in
202every category, including simplification.
204#### 3. Identify and merge
206Partition defects and advisories BEFORE grouping by fingerprint. Never merge a
207defect with advice, even on a supplied-hash collision. Neither higher-confidence
208advice nor a prior skipped extraction may replace, downgrade or suppress a defect.
210Compute identities for both core and specialist findings:
211- Shared-code advice (category \`shared-libs\` or fingerprint prefix \`shared-libs:\`):
212 call installed \`sharedLibsFingerprint\` from \`${ctx.paths.skillRoot}/lib/review-evidence.ts\`
213 with \`evidence_paths\` and \`helper_target\` as literal JSON on stdin, as in the core pass;
214 never trust a supplied hash or generate one yourself. Missing/malformed metadata
215 cannot deduplicate or reuse a saved decision.
216- Other findings: use supplied \`fingerprint\`, else \`{path}:{line}:{category}\`
217 or \`{path}:{category}\` when no line exists.
219Within the specialist list, merge matching identities in the same partition: keep
220the highest confidence and all source names. Confirmation by distinct specialists adds +1 (cap at 10) and \`MULTI-SPECIALIST CONFIRMED ({specialist1} + {specialist2})\`.
221Core findings never earn a specialist confidence boost. Preserve \`advisory\`,
222\`evidence_paths\` and \`helper_target\` through every merge.
224#### 4. Lead judgment
226Reviewers fill the space they are given. Before a merged finding counts, the
227parent reads it and drops or downgrades it on these grounds, saying so:
228- **Nit gravity.** A reviewer whose whole return is style, naming and preference
229 found nothing; say so and count no defect.
230- **Hypothetical, not actual.** A defect names the call site or input that reaches
231 it; one that does not is a note until someone traces it.
232- **Structure for a case that does not exist.** Advice to abstract, generalize or
233 guard a path nothing takes is advisory.
234- **A different design is not a defect.** "I would have done it differently" with
235 no failing case is dropped.
236- **Missing context.** A finding that assumes a rule the repository documents
237 otherwise is dropped with the pointer.
238- **On record.** A class \`docs/review-policy.md\` rules below the floor is a note,
239 named by its ruling.
240Independent agreement between reviewers is the strongest signal (the
241\`MULTI-SPECIALIST CONFIRMED\` boost records it). A reviewer that
242returned \`NO FINDINGS\` was right until a traced defect says otherwise.
244#### 5. Apply specialist confidence gates
246- Confidence 7+: show normally in the findings output
247- Confidence 5-6: show with caveat "Medium confidence — verify this is actually an issue"
248- Confidence 3-4: move to appendix (suppress from main findings)
249- Confidence 1-2: suppress entirely
251Core findings keep the core Confidence Calibration gates.
253#### 6. Score and present specialists
255Only specialist findings enter this header and \`quality_score\`; core findings do not.
256Use the merged NON-advisory specialist findings for both counts and score;
257the header's N is X + Y, so advisory findings never add to it:
258\`quality_score = max(0, 10 - (critical_count * 2 + informational_count * 0.5))\`
259Cap at 10 and retain for ${persistRef}. These are not final unresolved-defect totals.
260Print only this block: the stage 7 activity object and \`test_stub\` bodies are log and Fix-First data.
261Validated \`"advisory": true\` findings from any source are excluded from score,
262header, unresolved-defect totals and clean-status blockers. Show them separately;
263they remain ASK-only, never auto-applied. Real defects follow normal Fix-First.
265\`\`\`
266SPECIALIST REVIEW: N findings (X critical, Y informational) from Z specialists
268[For each finding, in order: CRITICAL first, then INFORMATIONAL, sorted by confidence descending;
269 advisory findings last, each rendered with an [ADVISORY] label in place of the severity]
270[SEVERITY] (confidence: N/10, specialist: name) path:line — summary
271 Fix: recommended fix
272 [If MULTI-SPECIALIST CONFIRMED: show confirmation note]
274PR Quality Score: X/10
275\`\`\`
277**Simplification footer (after the score line):**
278- If the simplification specialist was dispatched and returned findings, sum
279 their \`lines_removable\` values and print: \`net: -N lines possible\` (omit
280 findings without the field from the sum).
281- If it was dispatched and returned NO FINDINGS, print:
282 \`Simplification: lean already — nothing to cut.\`
283- If it was not dispatched, print neither line.
285Core shared-code savings never enter this footer; explain any overlap once in the core proposal.
287#### 7. Save specialist activity
289Compile a \`specialists\` object for ${persistRef}.
290${isShip ? 'For each specialist' : 'For DIFF_LINES < 50, keep `specialists: {}` (or `state-machines` alone when it dispatched); do not manufacture per-specialist scope records. Otherwise record each considered specialist'} (testing, maintainability, security, performance, data-migration, api-contract, design, simplification, state-machines, red-team):
291- If dispatched: \`{"dispatched": true, "findings": N, "critical": N, "informational": N}\`
292- If skipped by scope: \`{"dispatched": false, "reason": "scope"}\`
293- If skipped by gating: \`{"dispatched": false, "reason": "gated"}\`
294- If not applicable (e.g., red-team not activated): omit from the object
296Count only findings that specialist actually returned, before deduplication.
297Advisory findings COUNT in the stats \`findings\` field, not its defect counts.
298Include Design despite its different checklist. Preserve dispatch/failure status:
299zero returned findings from a failed attempt is not a clean review.
301#### 8. Hand off to Fix-First
303Send these findings to ${fixFirstRef} alongside ${critPassRef}.
304Consolidate equivalent shared-code advice under the core proposal, retaining all
305sources and counting overlapping savings once. Keep actual specialist stats;
306core-only advice must not create a specialist dispatch or finding.
307Normal AUTO-FIX/ASK rules apply, with advice ASK-only. Missing coverage still blocks
308completion. Advice never permits edits while readers are active or replaces a required review.`;
309}
311function generateRedTeam(ctx: TemplateContext): string {
312 const isShip = ctx.skillName === 'ship';
313 const stepMerge = isShip ? '9.2' : '4.6';
314 const fixFirstRef = isShip ? 'Step 9.3 dedup, then Step 9.4 Fix-First' : 'Step 5 Fix-First';
315 return `### Red Team dispatch (conditional)
317**Activation:** Only if DIFF_LINES > 200 OR any specialist produced a CRITICAL finding.
319If activated, dispatch one more subagent via the Agent tool (pass ${FOREGROUND_IF_AVAILABLE} — foreground; subagents default to background since ${CC_BACKGROUND_DEFAULT_SINCE}; ${BACKGROUND_RECOVERY})
321The Red Team subagent receives:
3221. The red-team checklist path \`${ctx.paths.skillRoot}/review/specialists/red-team.md\` (it reads the file)
3232. The merged specialist findings from Step ${stepMerge}, one line each (so it knows what was already caught)
3243. The git diff command
326Prompt, after the defensive framing and fixture handling from item 4 of the specialist dispatch: "You are a red team reviewer. The code has already been reviewed by N specialists
327who found the following issues: {merged findings summary}. Your job is to find what they
328MISSED. Read the checklist at {red-team checklist path}, list changed files with \`${diffBase(ctx)} && git diff --name-status "$DIFF_BASE"\`, read them as that fixture handling says, and look for gaps.
329Output findings as JSON objects (same schema as the specialists), or \`NO FINDINGS\` alone, the expected result when they missed nothing. Focus on cross-cutting
330concerns, integration boundary issues, and failure modes that specialist checklists
331don't cover."
333Tag Red Team findings \`"specialist":"red-team"\`.
334Add them to the original specialist outputs and rerun stages 1–8 of Step ${stepMerge}
335before ${fixFirstRef}; do not boost or count the earlier findings twice.
337If the Red Team returns NO FINDINGS, note: "Red Team review: no additional issues found."
338If the Red Team fails or times out, confirm it stopped and record its review as incomplete, just as for other specialists. ${isShip ? "Return to the parent's Exploratory QA step, then dedup and persistence; Step 9.4 cannot certify missing dispatched coverage as completed or clean." : 'Continue independent Step 4.7 QA and Step 4.8 adversarial review; Step 5.8 cannot certify missing dispatched coverage as completed or clean.'}`;
339}
341export function generateReviewArmy(ctx: TemplateContext): string {
342 // Codex host: strip entirely — Codex should not run Review Army
343 if (ctx.host === 'codex') return '';
345 const sections = [
346 generateSpecialistSelection(ctx),
347 generateSpecialistDispatch(ctx),
348 generateFindingsMerge(ctx),
349 generateRedTeam(ctx),
350 ];
352 return sections.join('\n\n---\n\n');
353}