review-code

name	review-code
description	Use when a code change / diff / PR is ready for review or code-review findings have landed.
version	1.0.0
spec_backlink	archive/specs/2026-05/2026-05-06-review-code-super-skill.md

coordinator:review-code

Trigger: EM has a code change ready for review (outgoing): mid-session diff before commit, completed task before next dispatch, branch ready for /merge-to-main, PR landing inline. OR a code-review's findings have landed and need processing (incoming).

When NOT to use: Plan / design doc / RFC review → coordinator:review. Frozen weekly diff at /workweek-complete Step 7 → coordinator:parallel-code-review. Mid-implementation (no clean review surface yet) → keep coding. Stuck pattern → see docs/wiki/stuck-detection.md. Pure mechanical citation check with no Opus → run docs-checker directly. Pure mechanical test-output classification → dispatch test-evidence-parser directly.

---

Branch A — Outgoing

Condition: a code change exists (committed or staged); no reviewer has been invoked on this iteration.

A.1 — Pre-flight workers (run BEFORE the Opus reviewer)

Three independent checks. Each fires or skips on its own condition; multiple can fire on the same diff.

Check 1 — TDD compliance (self-check, no agent)

• Code introduces new behavior (feature, fix, refactor that changes externally observable behavior)? → Verify against the TDD checklist before dispatching reviewer. If any test was written after the production code, surface to PM with the gap — the review will surface it anyway and a pre-review acknowledgment is cheaper than a the Staff Engineer finding. See docs/wiki/test-driven-development.md § Verification Checklist.
• Code is config-only / doc-only / generated code? → Skip TDD self-check.

Check 2 — Cited external APIs (docs-checker)

• Code cites C++ or Unreal Engine APIs? → Scaffold the docs-check sidecar first: coordinator-doc-new --type docs-check --plan <stem> (derive <stem> from the plan or artifact under review — use the plan-file stem for plan-tied diffs, or the branch slug otherwise). Pass the scaffolded path in the docs-checker dispatch brief: "fill the pre-scaffolded sidecar at <path>". → Mandatory dispatch of docs-checker. Other external APIs are EM judgment; in-repo-only skips. See docs/wiki/docs-checker-pre-review.md for the full row table.

Check 3 — Test evidence (test-evidence-parser)

• Diff includes unintended failing tests, runtime artifacts, or stack traces in the work-in-progress notes? (i.e., evidence of a problem the EM has not yet diagnosed — NOT verdict-bearing smoke output where a BLOCK or WARN is the expected outcome) → Dispatch test-evidence-parser with the failing test command. Read the structured output. Real failures block the dispatch; flakes/env failures get logged and the review proceeds. See docs/wiki/reviewer-routed-workers.md § test-evidence-parser.
• Diff has clean test runs (or no test surface yet — pre-TDD scaffold)? → Skip test-evidence-parser at pre-flight; reviewer may still call it via Worker Dispatch Recommendations.

(EM-initiated pre-flight; the normal case is reviewer-routed dispatch via the Worker Dispatch Recommendations block at Branch B — see docs/wiki/reviewer-routed-workers.md § Gotchas. Pre-flight loses the routing-intelligence framing, so use only when the diff already exhibits failure evidence the reviewer would otherwise stumble into cold.)

A.2 — Reviewer selection and dispatch

Routing table assembly: Read the base routing table from coordinator/routing.md, scan all enabled plugins for root-level routing.md fragments, merge into a composite routing table. Match the diff's signals against the composite table to identify Reviewer 1 (domain specialist) and Reviewer 2 (generalist, if needed).

Composite routing table (reference — assembled at dispatch time from fragment discovery):

Signal	Reviewer 1 (Domain)	Reviewer 2 (Generalist)	Effort
Sonnet-tier code review (workstream-complete, handoff, mise-en-place, mid-session quick review)	code-reviewer (Sonnet, locked)	(none)	Low
Game dev / Unreal / example-sim-repo	the Game Dev Reviewer (Opus only)	the Staff Engineer (Opus only)	Medium → Medium
Architectural change, new subsystem	the Staff Engineer (Opus only)	(backstop: the Director of Engineering, Opus only)	High
Cross-team / cross-repo seam (consumer ↔ producer, plugin ↔ host)	the Director of Engineering (standalone — DoE altitude, Opus only)	(none)	High
Generic-substrate / consumer-leak risk on producer-side surface	the Director of Engineering (standalone — DoE altitude, Opus only)	(none)	High
Front-end, CSS, UI components	the Front-End Reviewer (Opus only)	(backstop: the UX Reviewer, Opus only)	Medium
Front-end + architecture	the Front-End Reviewer (Opus only)	the Staff Engineer (Opus only)	Medium → High
ML/AI pipeline, model serving, RAG	the Data Science Reviewer (Opus only)	the Staff Engineer (Opus only)	High → High
UX flow, user-facing feature	the UX Reviewer (Opus only)	(backstop: the Staff Engineer, Opus only)	Low → Medium
Cross-cutting (many files, new pattern)	the Staff Engineer (Opus only)	(backstop: the Director of Engineering, Opus only)	High
Major example-sim-repo feature / new game mode	the Game Dev Reviewer (Opus only)	the Staff Engineer (Opus only)	High → High
Other / unmatched	code-reviewer (Sonnet) for shape-only diffs (no domain signal, no cross-system boundary); the Staff Engineer (Opus only) when the diff carries architectural shape (new abstraction, cross-system seam, new pattern, schema/security boundary)	(none)	Low → Medium

Personas are Opus-only. the Staff Engineer, the Game Dev Reviewer, the Data Science Reviewer, the Front-End Reviewer, the UX Reviewer, the Director of Engineering carry model: opus in their agent frontmatter; dispatching them at Sonnet altitude (via model: "sonnet" override on the Agent call) is a doctrine violation. Sonnet-tier code review uses code-reviewer (agents/code-reviewer.md). Sonnet-tier mechanical analysis uses the relevant worker (test-evidence-parser, security-audit-worker, dep-cve-auditor, doc-link-checker). The persona's value is structured judgment under Opus context; running it at Sonnet costs the prompt complexity without the judgment payoff — empirically the result is a degraded "Sonnet-flavored the Staff Engineer" that produces persona affect without the architectural lens. → agents/code-reviewer.md for the Sonnet-tier surface.

the Director of Engineering standalone vs. The Director of Engineering backstop. When the signal matches a cross-team or consumer-leak row above, dispatch the Director of Engineering directly as the primary reviewer — describe the standalone / DoE-altitude posture in the brief (do NOT pass a mode argument; that is the harness tool param and will error). Do NOT run the Staff Engineer first. Standalone the Director of Engineering is a peer of the Staff Engineer in technical rigor with the additional cross-team authority the Staff Engineer's EM altitude would hedge on. The "(backstop: the Director of Engineering)" entries above are the chained-after-the Staff Engineer usage for High-effort architectural reviews; that posture is still in play but does not exhaust the Director of Engineering's role.

If --reviewers "name1,name2" was provided, skip auto-detection. Use the explicit list — first name is Reviewer 1, second (if any) is Reviewer 2. Report: "PM-directed review: [name1] then [name2]."

Match tier to complexity, not importance. Routing every "important" diff to a staff session burns budget without finding more bugs. The heuristic: would a second reviewer likely contradict the first, or just add diminishing-return notes? If contradiction is unlikely, one reviewer is enough.

Situation	Correct tier
Single-subsystem code change (one feature, one bug fix, one refactor)	One reviewer. Sonnet-tier (post-implementation, mid-session, no architectural call): code-reviewer. Opus-tier (domain-flagged, architectural, named-persona match): dispatch the matching persona at Opus. For known-target single-reviewer cases, direct Agent(subagent_type=...) dispatch is an acceptable shortcut; routing table is preferred for routing intelligence. Never dispatch a persona with model: "sonnet" — that is the violation code-reviewer exists to replace.
Cross-subsystem code change (e.g., UE + Python pipeline; front-end + auth backend)	Two sequential reviewers: --reviewers "<domain>,the Staff Engineer"
Contested architectural code change with ≥2 valid implementations AND PM authorized	/staff-session review-mode
"This is important, I want it done right"	One reviewer (auto-detects domain)
Code touches auth, security, billing — high stakes but clear approach	One reviewer (auto-detects domain)

• Code change is genuinely trivial? (typo, comment-only, single-line config bump with no behavior change) → No review needed; commit and proceed.
• PM has explicitly waived review on a non-trivial change? ("ship it", "skip review", "straight to merge") → Exit; this skill does not run. Log the waiver in commit message or PR description: review: skipped per PM direction YYYY-MM-DD (greppable).

See CLAUDE.md § Challenging the PM — /staff-session is PM-gated; ask first.

Pipeline phases (docs-checker, prior-art-checker, external-pattern-checker, integrator, backstop, report) live in docs/wiki/reviewer-pipeline.md. Walk those phases inline — they are not optional. Walk Phase 2.5 → 2.7 → 2.7b → 2.7c → 2.8, then dispatch the reviewer, then Phase 3.5 → 3.7 → 4 → 5.

Persist the reviewer findings — zero EM ceremony. Every reviewer self-persists its findings to state/review-trail/findings/ and returns only a pointer line. There is no EM pre-scaffold, no variant selection, and no cs_write_review_claim. The integrator hard-stops on inline-relayed findings (agents/review-integrator.md § Intake precondition).

Pattern A — persona / Opus reviewer (the Staff Engineer, the Game Dev Reviewer, the Data Science Reviewer, the Front-End Reviewer, the UX Reviewer, the Director of Engineering): Append the contents of snippets/findings-self-persist-sentinel.md verbatim to the reviewer's dispatch brief. The persona scaffolds its own sidecar in state/review-trail/findings/ and returns DONE: <sidecar-path> | verdict: <OK|WARN|BLOCKED> | findings: <N>. EM reads the returned path and passes it to the integrator. No EM pre-scaffold, no injected docs/plans/*.review.md path. Multi-reviewer chain: each reviewer self-persists to its own sidecar; the integrator is dispatched once per reviewer pointing at the returned path.

Pattern B — unnamed Sonnet code-reviewer: Dispatch coordinator:code-reviewer (background, UNNAMED — no name: param). The reviewer self-persists its findings to state/review-trail/findings/ and returns DONE: <sidecar-path> | verdict: <OK|WARN|BLOCKED> | findings: <N>. EM reads the returned path and passes it to the integrator. There is exactly one coordinator:code-reviewer — no -selfpersist variant, no claim marker. Spec backlink: cross-repo/inbox/2026-07-01-reviewer-selfpersist-confinement-redirect.md.

A.3 — Sequencing

• In-session code reviews → sequential (HARD RULE). Integrate Reviewer 1's findings via coordinator:review-integrator BEFORE dispatching Reviewer 2. See CLAUDE.md § Review Sequencing.
• Merge-gate parallel carve-out applies ONLY at /workweek-complete Step 7 on a frozen weekly diff with orthogonal lenses + no-rewrite synthesizer. Does NOT apply to mid-session, /merge-to-main, or /workday-complete reviews. See CLAUDE.md § Review Sequencing ¶ exception (merge-gate code review on frozen diff).
• Frozen weekly diff at /workweek-complete Step 7? → Exit this skill; use coordinator:parallel-code-review.

---

Branch B — Incoming

Condition: a reviewer has returned output on a code change. EM is deciding what to do with each finding.

Forbidden triage outcomes (never valid): defer-to-later, capture-for-backlog, time-estimate-as-rationale. If any of these would be the disposition, surface to PM — the PM decides whether to defer, not the EM. See CLAUDE.md § Plan-First Workflow ¶ implement-and-iterate (project-level) and § Operating Assumptions (global ~/.claude/CLAUDE.md).

Walk each finding against the triage table — it lands in exactly one row:

• Tradeoff-free correctness fix? (wrong API name, broken citation, missing import, wrong precedence, factual error, internally inconsistent identifier, off-by-one in clear path) → Dispatch coordinator:review-integrator with mode: "auto" pointing at the on-disk sidecar path (the DONE: <path> the reviewer returned — see Persist the reviewer findings in A.2 above). EM spot-checks the diff. Never hand-author the fix yourself, however small it looks. The integrator is not a cheaper typist — it is a fresh agent that independently re-checks each finding against current disk before applying it. That second check catches findings that were wrong, stale, or mis-scoped (a concurrent executor moved the schema; the rename collides with a constraint that landed after the review) — exactly what self-authoring discards by applying the reviewer's claim at face value. "One line / obvious" is the rationalization this rule defeats, not an exception to it. See CLAUDE.md § Reviewer findings — apply, don't ratify; review-integration-doctrine.md § Review Sequencing.

  • (i) Math / algebra / precedence / symbolic-reasoning finding from a single agent? → Verify against source before applying. See CLAUDE.md ¶ Exception — math, algebra, precedence.

• Code-shape tradeoff? (architectural direction, scope question, file-organization call, abstraction boundary) → Surface to PM with finding + reasoning. Wait for direction. See CLAUDE.md § Reviewer findings — apply, don't ratify (escalation triggers).

  • (i) YAGNI / scope-trim argument? → Always escalation, never auto-trim. YAGNI is a product decision. See CLAUDE.md § Challenging the PM.
  • (ii) Refactor-over-patch signal? → Refactor is the default when AI is the implementer. Surface to PM with refactor proposal, not a piecemeal fix. See CLAUDE.md § Core Principles ('Do the right thing, not the easy thing').
  • (iii) Build-vs-defer call? → Always PM. See CLAUDE.md § Challenging the PM ¶ Ask the PM when.

• Multiple findings collectively suggest a structural refactor (not just patches)? → Do NOT integrate piecemeal. Surface to PM with refactor proposal — the aggregate signal is the finding. See CLAUDE.md § Core Principles ('Refactor over patch') and § Convergence as Confidence.

• Premise / hypothesis question? (reviewer challenges what the code is supposed to do, or claims a bug exists where the code is correct) → Read the cited code at the cited line — not the reviewer's paraphrase. Confirm or revise. See docs/wiki/reviewer-premise-challenge.md and CLAUDE.md § P0/P1 Verification Gate.

• Multiple reviewers converged on the same issue from different entry points? → High-confidence; apply via integrator without per-finding verification. See CLAUDE.md § Convergence as Confidence.

• Worker Dispatch Recommendations block present in reviewer output? → Dispatch each named worker. For code reviews, all four are in scope: test-evidence-parser, security-audit-worker, dep-cve-auditor, doc-link-checker. Reviewers name only the workers that fire on this diff (e.g., dep-cve-auditor only if the diff touches dependency manifests; security-audit-worker only if the diff plausibly touches an attack surface). Feed worker output back into EM context. Reviewers do not dispatch — EM does. See docs/wiki/reviewer-routed-workers.md and CLAUDE.md § Reviewer-Routed Workers.

• Performative-agreement urge? ("you're absolutely right!", "great catch!", "thanks for catching that") → STOP. Delete the urge. State the fix factually. See docs/wiki/receiving-code-review.md § Forbidden Responses.

• Default / unmatched? → Apply via integrator. Default is to integrate, not to ratify. See docs/wiki/receiving-code-review.md (triage tables, push-back patterns, performative-agreement guard) and CLAUDE.md § Reviewer findings — apply, don't ratify.

Executor brief out-of-scope reminder. When building an executor brief from these findings, include this constraint: Removing/weakening production safeguards to satisfy pre-existing test mocks is OUT OF SCOPE. Tests follow production; surface the conflict instead.

Probe-wiring brief authority surface. When building an executor brief that wires a new coordinator-doctor probe (adding or editing a health/drift probe), name docs/wiki/coordinator-doctor.md (plugin-root-relative: plugins/coordinator/docs/wiki/coordinator-doctor.md) as the authority surface in the brief. The coordinator doctor is wiki-only by design — there is no commands/doctor.md. A brief that tells a delegate to "find the doctor command" sends it searching for a file that does not exist; name the wiki explicitly so the delegate reads the authority surface directly. Confirm the wiki path exists before quoting it (spec backlinks outlive their cited spec).

---

Cross-reference exit

After Branch B completes for a multi-reviewer review and Reviewer 1 is integrated, return to A.2 to dispatch Reviewer 2. This skill is re-entrant — each pass walks one direction.