Code Review Battery

Mechanical routing: don't decide from memory or from the "Wrong skill?" prose below -- run tools/review.sh route <path> [<path> ...] first (paths of the files you're about to review). It wraps tools/which-gate.sh and prints the correct skill + sentinel + runner for each artifact. If the router says a different skill, follow the router, not this banner. If the router errors or is unavailable, stop and report -- do not fall back to the prose. The banner is an inner backstop, not a substitute for the mechanical check.

Wrong skill? File-protocol review handoff → code-review. PR inline → providing-code-review. Pre-commit gate → progressive-code-review-gate. Full-repo security audit → repo-security-scan or /sp-devsec-audit. Skill.md files or skill-adjacent tooling infrastructure → llm-skill-review. Slash commands: /sp-cr-battery [min-score] (primary; optional 1.0–10.0 quality threshold, default 7.0), /sp-deepreview (legacy).

Run ONE combined reviewer by default. Dispatches up to 7 specialized reviewer agents in parallel, but only those whose signal the diff carries. A triage coordinator selects which to activate based on the diff, then aggregates findings into a unified report.

Why this exists: A single reviewer tries to evaluate everything simultaneously, leading to shallow coverage, inconsistent focus, and ~40% false positive rates. Specialized reviewers with focused prompts produce deeper analysis with near-zero false positives.

Strategy before execution

Before every automated review or test/check pass, resolve a snapshot-bound REUSE, WAIT, RUN_MINIMAL, REVIEW_REQUIRED or STOP decision at its supported execution/dispatch boundary. Reuse valid coverage or join its active producer. Missing/stale decisions refuse; unknown coverage stays conservative. No model calls or recursive checks inside strategy. Status/HEAD changes, clean reviews and score targets alone never justify another expensive pass. For REVIEW_REQUIRED, include SPP_REVIEW_CLASS=code SPP_REVIEW_SNAPSHOT=<strategy.snapshot> from the current native preflight JSON in the reviewer prompt.

When to Use

• When requesting-code-review or progressive-code-review-gate triggers a review
• When you want a thorough review of staged changes, a commit range, or a PR diff
• When reviewing someone else's code
• When verification-before-completion detects the implementation→presentation transition and no valid sentinel exists for HEAD

Gate chain position 3 of 4:

One-per-unit rule: Battery fires at most once per coherent unit of work. Reuse a native, provenance-bound review for unchanged content/context; HEAD equality alone does not prove that binding.

Procedure

Phase 0: Sentinel Check (canonical skip gate — run before dispatching anything)

This is the canonical skip gate for the one-per-unit rule. Run tools/review-preflight.py first (--base <integration-branch>, default origin/dev, or --staged for an uncommitted change; pass --mode=bug-fix|feature when given) and use its JSON -- sentinel states, BugPath mode, fired signal rows with file:line hits, rows left to judgment, and the inline-exemption answer -- instead of applying the Phase 0-1 tables by hand; the tables stay the spec. Callers (requesting-code-review, finishing-a-development-branch, progressive-code-review-gate) should run this before dispatching. If a caller does not implement Phase 0 explicitly, the agent should run the preflight (or apply the tables) before invoking battery.

Use the native parser's per-class result and reviewed content/context, rather than a raw SHA comparison or a clean-status shortcut:

Progress messages and history-only commits do not invalidate reviewed content. Never delete a sentinel to force another initial review.

Phase 0.5: BugPath Mode Detection

Load gate: This phase requires reference.md to be loaded alongside this skill. If absent AND the branch matches a BugPath pattern (hotfix/* or fix/[A-Z]+-[0-9]+, per the detection script in reference.md): hard halt — emit only: "Cannot enter BugPath Mode: reference.md is absent. Load it before proceeding." Output nothing further — no phase summaries, no partial results, no clarifying questions. Return control to the user. If absent AND the branch does NOT match a BugPath pattern: treat BugPath Mode as INACTIVE and note in triage line.

Run immediately after the sentinel check. Detect whether this is a targeted bug fix, then set the mode before triage.

(Detection script: reference.md § BugPath Detection Script.)

When BugPath Mode is active: BugPath Verifier mandatory (not skippable), threshold 9.2, path-coverage floor applies (see Phase 3). SCOPE-SKIP on a confirmed bug-fix branch = Important finding, score -1.5. State mode in executive summary triage line (reference.md § Executive Summary Template). Also supports --mode=bug-fix and --mode=feature. If the diff also matches the Sibling Path Trace signal (Phase 1), BugPath Verifier's dispatch payload (Phase 2 step 4) MUST additionally include the "Sibling Path Trace" excerpt from reviewers/defect-finder.md, since BugPath Verifier's Sibling Bug Scan dimension runs that method rather than re-deriving it.

Phase 1: Triage

Analyze the diff and select reviewers:

Decision rules: Docs-only → Standards Enforcer only. Config-only → Guardian (+ Standards Enforcer / Defect Finder only if a metric/alarm or other dispatch signal below is present). Any code → ONE combined reviewer carrying the Defect Finder, Guardian and Standards Enforcer lenses in a single agent, plus a mandatory placement dimension (one sentence + file:line: "is this the right place, or is there a simpler root cause?"; missing = incomplete review). Add separate agents only from the signal table below and the Mandatory activation rules. Measured 2026-09-20: 5 fresh agents on a 212-line diff cost 393k tokens, mostly re-reading the same files and re-running the same tests; the root-cause finding came from the placement question, which no diff signal triggers — hence mandatory.

Signal-driven dispatch (additive — a signal activates its reviewer(s); it never deactivates one already selected above). Scan the diff for these signals and route accordingly:

When no signal and no default activates a reviewer, skip it and say why in the triage line.

Mandatory activation (not subject to triage exclusion):

• Guardian is ALWAYS activated when changes touch: retry logic, circuit breakers, rollback behavior, deployment config, feature flags, authentication/authorization, or state machine transitions.
• Design Critic is ALWAYS activated in Standard Mode when changes touch: interfaces, public APIs, contracts, message schemas, shared state types, or cross-module boundaries.

Design Critic in Bug Fix Mode (SUPPRESSED by default): Only re-activated if diff contains API-change signals (new exports, public method signatures). State in triage line; see reference.md § Design Critic BugPath Logic for the detection command (if reference.md absent: default to SKIPPED unless --all flag set). Reduced dispatch for a cited port claim (evidence required, not self-attested): A "verbatim/near-verbatim port of already-reviewed code" claim does NOT by itself reduce the activated reviewer set. It only reduces dispatch when the triage line pastes both the cited source (repo + commit SHA) and the literal git diff <cited-source-sha> -- <changed files> command plus its output — the command itself, not just the output, so the claim is independently re-runnable rather than plausible-looking. The combined reviewer is never dropped by this exemption -- it carries Guardian's lens, and a byte-identical port can still be dangerous at its new call site (different callers, different exposure). What the exemption can suppress is the signal-triggered specialist agents, for the content the citation covers. In Bug Fix Mode BugPath Verifier stays mandatory. Carve-out: if the cited-port diff touches concurrency-relevant constructs (locks, threads/goroutines, async/await, shared mutable state, retry/backoff timing), the combined reviewer must still run Defect Finder's interaction-path pass on it -- the exemption covers correctness/edge-case redundancy only, not concurrency. Content beyond the cited source (new tests, new logic, fixes not upstream) gets full review; the exemption never extends past what the citation covers. No evidence in the triage line means unverified — dispatch normally. (A genuine verbatim port produces a trivial/empty comparison here — if it doesn't, the "port" framing was wrong, not the review depth.) A second, distinct exemption — reduced dispatch for a small diff the reviewing agent already has current, verified context on (parallel dispatch traded for a single inline self-review pass, not a lower evidence bar) — is defined in reference.md § Diff-Size-Scaled Dispatch Exemption; same audit-ability requirement (state which trigger conditions were confirmed in the triage line), same rule that it reduces dispatch overhead, never dispatch substance for a risk class that's actually present.

Overrides: --all (force all), --only=<name>, --skip=<name>, --round1-only (skip escalation), --security (force AttackerPersona on), --no-security (force AttackerPersona off), --mode=bug-fix (force Bug Fix Mode), --mode=feature (force Standard Mode). --skip=BugPath Verifier is silently ignored in Bug Fix Mode (the reviewer is mandatory).

State your triage decision before dispatching:

**Triage**: Activated: [list] | Skipped: [list] | Reason: [1-2 sentences]

Phase 2: Diff + Source Context + Dispatch

Sub-agents have NO conversation context. Pass diff + source context inline.

1. Capture diff: git diff --cached, git diff HEAD~1, or git diff main..HEAD. Use git diff main..HEAD (or --cached for a single in-progress commit) whenever a full-branch-diff-dependent check applies (e.g. Ticket IDs in Comments, reviewers/standards-enforcer.md §3b) — git diff HEAD~1 only shows the tip commit and silently misses anything introduced earlier in a multi-commit branch. Sub-agents have no conversation context or tool access to re-run this themselves (see above); get the scope right before dispatching.

2. Source context for ripple analysis (#1 missed-finding cause = reviewing diff in isolation): Fields SET/RESET/NULLED → grep READERS. Symbols DEFINED (metrics, events, enums, error codes) → grep PRODUCERS; zero producers = dead definition (mandatory finding for metric/alarm signals). Threshold comparisons → grep PRODUCERS of the compared value. Stateful code → full state type + transitions. State machine flag context (mandatory): when the diff touches a boolean state flag (any is*, has*, in*, or similar guard variable) that is SET in 2+ distinct methods in the same class, pass the FULL source of ALL methods that set or read that flag PLUS every code-path comment explaining the PURPOSE of each path. Without intent comments, sub-agents cannot detect semantic contradictions (e.g., a path whose purpose is "wait for customer speech" setting a flag that drops speech). Changed signatures → all callers. Cross-module calls → full callee body (or signature + state-mutating/throwing/early-return branches if budget-constrained). On-disk format changed (ad-hoc-parsed, no shared parser) → grep ALL consumers repo-wide incl. tests; prefer "extract a shared parser" over "update each consumer" (see reference.md Failure Modes). Symbol whose only call site the diff removes or reroutes → grep the FULL repo for remaining references; zero = dead code introduced by this diff; also pass package.json (exports/publishConfig/main) into context, since the severity ladder downgrades an exported symbol from Important to Possible when the repo is a published library (see Caller Removal Trace). New/changed field that supplements or partially supersedes an existing field on shared state → grep ALL existing readers of the field it supplements (not just the new field's own readers, which may not exist yet) → verify every such reader was updated, or confirm why it doesn't need to be (see Sibling Path Trace).

3. Inbound reference scan (mandatory when diff renames, moves, or deletes files):

git diff --diff-filter=RD --name-only main..HEAD   # old paths
grep -rn "old-filename" . --include="*.md" --include="*.ts" --include="*.sh"  # scan ENTIRE repo

MUST scan outside the changed directory. The #1 failure mode: scoping grep to the refactored directory, missing sibling modules that reference old paths. Hits outside the diff are mandatory CRITICAL findings — broken consumers the author didn't update. Include grep results in every reviewer's context.

4. Dispatch ALL activated reviewers simultaneously via sub-agent-code-reviewer (Augment) or Task() (Claude). Each gets: reviewer prompt + full diff + source context + inbound reference scan results. In BugPath Mode with the Sibling Path Trace signal also matched (Phase 1), this is the step where BugPath Verifier's payload must actually get the "Sibling Path Trace" excerpt promised in Phase 0.5 -- not just noted as a requirement earlier.

Phase 3: Aggregate

After all reviewers return:

1. Sort findings: Critical → Important → Minor → Possible, then by file path
2. Prefix each with [Reviewer Name]
3. BugPath Verifier SCOPE-SKIP (BugPath Mode only): if the BugPath Verifier reports SCOPE-SKIP, add an Important finding "BugPath Verifier SCOPE-SKIP on confirmed bug-fix branch — manual path-coverage review required" and deduct 1.5 from the score.
4. Convergence: same location from 2+ reviewers — keep both; True convergence (different reasoning paths) → promote to ≥ Important (never demote a Critical); Echo convergence (same evidence/phrasing) → retain original severity.
5. Clean dimensions need same evidence block as findings; missing evidence on any clean dimension causes the verifier to cap the overall run score at 7.0.
6. Severity: Critical=broken now; Important=breaks under conditions; Minor=standards gap. Elevate to Important when operator-visible signal is wrong/missing. Reclassify process gaps downward. See reference.md § Severity Definitions.
7. Triple-filter each Important/Critical on CX impact, complexity, testability: Implement (all 3 pass) — propose exact code change + enumerate collateral_test_changes (required, may be empty-list: grep it(/test(/describe( in diff's test files for tests whose description references the behavior being removed; only include if assertion CLEARLY validates that specific behavior). Defer — document for future work. Reject — fix adds more complexity than it removes. Preserve Regressions Risked + Durable Check per Implement finding.

Tightening: >10 findings → suppress Minors from body (count in summary; state "Tightening applied: [N] Minor findings suppressed"). Score: 10.0 − 2.5×C − 1.5×I − 0.25×M − (durable<50%?0.5:0), floor 0.0. Extract threshold from invocation (e.g. /sp-cr-battery 8.5 → 8.5; default 7.0, or 9.2 in BugPath Mode). Verdict is anything other than PASS or PASS_WITH_NITS (below-threshold score, OR a Reject-classified Critical at or above threshold) → skip the sentinel write step in Phase 6 (still write the JSON envelope to .cr-battery-runs/). BugPath path-coverage floor: INSUFFICIENT→cap 6.5, PARTIAL→8.0, FULL→none. Metrics: durable ≥50%, convergent count, unresolved Critical=0.

Report format: Executive Summary (see reference.md § Executive Summary Template) → Header → Critical → Important → Minor → Possible → Clean Dimensions → Action Classification → Durable Checks → Summary (Findings: [N]C/[N]I/[N]M ([N] suppressed)/[N]P | durable=[N]%, convergent=[N], unresolved-critical=[N]).

Phase 4: Escalation (Round 2)

Reopen only for changed scope, actionable findings, invalidated evidence or a newly justified independent safety lens. An already-covered static trigger cannot require another clean review:

Collect every trigger that fires and dispatch all of them together as ONE round (same "simultaneous" dispatch discipline as Phase 2 step 4) — e.g. if both the interaction-path and inbound-reference triggers fire, that is still Round 2, one dispatch, not two sequential round-trips. Firing triggers one-at-a-time adds pure serial latency with no rigor gain: dispatching N triggered re-checks together costs the same wall-clock as dispatching one. Continue the Round 1 reviewer where possible, sending the diff slice + trigger signal and stating its earlier context is superseded for changed files; a fresh agent needs the complete Round 1 findings, not a summary. Append under ### Round 2 Findings. Skip if --round1-only, all clean, or diff <20 lines.

Phase 5: Convergence

STOP after one initial review when clean and above its floor. CONTINUE only for a material change, actionable finding, invalidated evidence or a justified new safety lens. ESCALATE TO HUMAN after 3 passes; never chase a score.

Correlated-Failure Detection

After synthesis: (1) evidence overlap: ≥3 reviewers cite same file+line → flag and expand scope; (2) phrasing similarity: near-identical rationale across findings → flag and re-examine from different entry; (3) clean sweep: all-zero findings → verify reviewers examined different slices. Flags trigger scope expansion, not verdict changes. See reference.md § Correlated-Failure Detection.

Phase 6: Finalize Verdict + Write Sentinel

Prerequisite: Correlated-Failure Detection has completed and no re-examination was triggered.

Preserve the run (before sentinel write):

1. Determine the verdict from this MR's score vs threshold: PASS if score >= threshold (no unresolved nits); PASS_WITH_NITS if at or above threshold but Minor nits remain; PASS_WITH_FIXES if below threshold, every Critical finding is Implement-classified (fixable path exists — vacuously true if there are no Critical findings), and every Important finding is Implement- or Defer-classified with a documented rationale in the report (llm-skill-review dogfood review, 2026-07-17: a Critical is NEVER Defer-classified, with or without rationale — that always forces REJECT; a follow-up pass the same day found the first fix still left exactly this Critical+Defer combination unmapped); REJECT if any Critical is Reject- or Defer-classified, any Important Defer has no documented rationale, or there are unresolvable blockers.
2. Write a JSON envelope to .cr-battery-runs/<HEAD-sha>.json with tools/review-envelope.py (init --kind battery, add-clean/add-finding, resolve, set, check): it runs each evidence command as the claim is written and refuses one whose expectation fails. Schema: reference.md § Run Envelope Schema. Every finding AND clean-dimension verdict must carry an evidence block; verifiable: false caps at 7.0. Expectation types: reference.md § Verifier Details. tools/run-battery.sh refuses sentinel write if JSON missing in Bug Fix Mode; graceful degrade in Standard Mode. Note: collateral_test_changes is not yet captured in the run envelope schema — it appears only in the human-readable report. Tracked in candidates/candidate-009.yaml for future schema update.

If final verdict is PASS or PASS_WITH_NITS (all nits resolved):

# tools/run-battery.sh is the ONLY permitted way to write .code-review-cleared -- never echo it directly.
tools/run-battery.sh --verdict PASS --min-score "<threshold>"         # no unresolved nits
tools/run-battery.sh --verdict PASS_WITH_NITS --min-score "<threshold>" # Minor nits remain unresolved

Timing: Native content-scoped clearance survives unchanged commits/merges. REJECT or PASS_WITH_FIXES: preserve findings, fix material defects, then strategize affected verification/delta review; never write a false sentinel.

Gap Analysis + Error Handling

Monolith found something no specialist found → candidate pattern → candidates/. Reviewer fails → note, don't retry. Diff >3000 lines → warn, suggest chunks, keeping structurally-related files (Caller Removal Trace or Sibling Path Trace candidates) in the same chunk or cross-passing their excerpts so neither chunk's reviewer works blind to the other file -- this bounds what one dispatch reads, not what Phase 2's full-repo grep obligation searches; a chunked sub-battery still greps the whole repo for out-of-diff references. Empty diff → skip.

Failure Modes & Anti-Patterns

See reference.md for the 5 standard failure modes (no-findings, FPs-from-isolation, convergence-stuck, monolith-vs-specialist, duplicated-format-parsing) and the 5 anti-patterns (all-agree, duplicates, fatigue, missing-context, over-scoping), with a companion table each (detection/correction for anti-patterns, a fix column for failure modes). Moved to the companion file to keep this skill under the per-skill line budget.

Companion Skills

• progressive-code-review-gate: Primary consumer (dispatches this battery pre-commit) · providing-code-review: Engineering rigor checklist (informs reviewer focus)
• inter-agent-review-protocol: File-protocol review (alternative dispatch method) · micro-harsh-review: Per-batch review