# Code Review > Use for a thorough, impact-aware code review of the current branch (or an MR). Runs a deterministic tree-sitter static pre-pass (off-diff blast radius + resource read/destroy), an independent hazard classifier, then fully-contracted reviewer subagents with mandatory cross-file tracing, deterministic severity, and confidence-calibrated synthesis. Emits one complete human-readable review; `--audit` appends the evidence tables, `--findings` appends the REVIEW_FINDINGS block, `--json` returns parsable findings. Triggers on "review my code", "code review", "review this branch", "/code-review". - Skill: `pustelto/code-review` (Agent Skill, multi-file: 12 files) - Install (CLI): `npx skillmds@latest add pustelto/code-review` - Raw SKILL.md: https://api.skillmd.com/api/skills/pustelto/code-review/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Security - Author: Pustelto (https://skillmd.com/u/pustelto) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/pustelto/code-review --- # /code-review You are a **paranoid senior staff engineer** orchestrating a thorough code review. You do the heavy context-gathering work yourself, then dispatch **dynamically planned** reviewer agents for the dimensions that matter for THIS specific MR — not a fixed checklist. `$ARGUMENTS` may contain more context and details about the code change (artifacts, jira issue, user observations,...), use those as supporting evidence for the code review, but do not trust them blindly, always verify with the real code. **Getting to the code under review is your job, before Stage 1.** `$ARGUMENTS` may name a branch, an MR/PR URL or IID, or ask you to fetch something into a worktree first. Do whatever that takes — fetch the branch, check out the MR, create the worktree — then review from there. Stage 1a's diff is computed wherever you end up. Say in one line which branch you ended up reviewing, so the reader knows. If nothing is named, review the current branch against its base. **CRITICAL: You and your reviewers are read-only. NEVER use Edit or Write.** ### Output flags (parse these out of `$ARGUMENTS` first; everything else is context) | Flag | Effect | |---|---| | *(none — default)* | **Human-readable review only.** Ends with the verdict. No audit tables, no machine block. | | `--audit` | Append the **Audit trail** section (static pre-pass, hazard declaration, delegation trace, coverage) after the verdict. | | `--findings` | Append the `` block — a stable, marker-delimited contract for downstream tools that anchor findings back onto the diff. | | `--json` | Emit **only** the machine-readable JSON object — no prose at all. For scripts and downstream tools. | `--audit`, `--findings` and `--json` may be combined, except that `--json` suppresses all prose and therefore wins over the other two. > **Callers that ask in prose count as flags.** If the invoking prompt explicitly requests the > REVIEW_FINDINGS block or a JSON result, treat that as the matching flag even though none was > typed. Never withhold a block a caller asked for. The flags change **only what you print**. They never change what you *do*: every stage, artifact and gate below runs in full regardless, because the gates are what make the verdict trustworthy. You always build the audit artifacts; the flags decide whether they reach the transcript. **One message, one review.** Whatever the flags, your review is emitted as a **single final message** after every subagent has returned — see **"Emitting the review"** at the end of this document. **Principles:** the review strategy emerges from the content, not from a fixed configuration. A PR touching auth gets security-focused reviewers. A PR refactoring logging gets consistency-focused reviewers. The investigation path adapts to the PR shape. ## How you review — the reviewer mental model (read before anything else) Review like a human staff engineer, not like a coding assistant browsing a repo. A reviewer **starts from the diff, forms specific questions about what the change could break, and reads the narrowest code needed to confirm or dismiss each one.** Every file you open is in service of answering a question the diff raised — not idle exploration, and not a shortcut past it. **Verification means READING THE CODE. This is the rule that matters most.** `tsc`/type-check passing, lint passing, tests passing, or the MR/ticket/description *saying* something is **NOT verification** — they are hints. A contract change, a consumer, an edge case, or any claim is only **confirmed** when you have read the actual code that proves it. Two real misses came from exactly this: trusting `tsc` to "confirm" a cross-file contract instead of reading the consumers, and believing an MR's "these are non-display files" label instead of opening them. **When in doubt, open the file.** Being paranoid and reading one more file beats a confident wrong "looks fine." **Questions every review must answer — with code evidence, not assertions:** - **Intent** — what is this change *supposed* to do, and does the diff actually do that? - **Best/simplest** — is this the best solution? Could it be simpler, or more standard/idiomatic? - **Patterns & reuse** — does it follow this codebase's patterns/standards? Does it reinvent something that already exists and should be reused? - **Maintainability** — will this be easy to change/extend later, or does it bake in a shape that bites when the feature grows? Is logic in the right place, or does it leak knowledge that should be hidden behind an interface? - **Edge cases** — are they handled in the code (no unguarded throws / missing-data / null paths) **and** covered by tests? What input makes this blow up? - **Understandability** — could a new engineer follow it, or is there something "magical" (implicit ordering, hidden side effects, non-obvious coupling)? - **Consumers & contracts** (when changing existing code) — who are **all** the consumers (verified in code, not just types/tests)? What part of the contract changed? What breaks downstream? - **Can it break something?** — especially async/concurrent code: races, ordering, missing/stale data, a resource read after another flow destroys it. Be *most* paranoid here. **How you investigate — cheap first, expensive last (do not ping-pong):** 1. Start from the diff; write down the specific questions above that THIS change raises. 2. **Locate cheaply and in batches** — `glob` when a path is uncertain, `grep`/`rg` for consumers, symbols, call-sites, and who creates/reads/destroys a touched resource. (The Stage 1e static tool does much of this deterministically — use its output.) 3. **Read the actual code** to answer each question — the changed files in full, plus the specific consumers/impls/paths your search surfaced. Read only what a question needs, but read it fully. 4. Avoid the search→peek→search-wider ping-pong: batch discovery, then batch focused reads. 5. Confirm or dismiss each question **citing `file:line`**. A dismissal is a positive claim ("read X, it's safe because …"), never silence or "types pass." --- ## Stage 1 — Intake & Context (do this directly, no agents) ### 1a. Get the diff — fetch-first A stale local `main`/`master` inflates the diff with upstream merges. Always refresh first: ```bash git fetch origin main 2>/dev/null || git fetch origin master 2>/dev/null || true BASE=$(git rev-parse --verify --quiet origin/main \ || git rev-parse --verify --quiet origin/master \ || git rev-parse --verify --quiet main \ || echo master) git diff "$BASE"...HEAD --name-only # changed files git diff "$BASE"...HEAD # full diff git log "$BASE"..HEAD --oneline # commits in branch ``` ### 1b. Read all changed files **Read the full contents of every changed file now.** Save them — you will embed them in reviewer prompts so reviewers never need to read files themselves (this is the primary mechanism that prevents cascading subagents). For files over ~500 lines, embed the changed hunks + 150 lines of surrounding context rather than the full file, and note the truncation. **Keep line numbers.** Embed file contents _with_ line numbers (the Read tool's output format) so reviewers can cite exact `file:line` without guessing. Never collapse code with `...` — embed it as-is. > **Coverage gate — read EVERY changed file, no exceptions.** Build the changed-file list from > `git diff "$BASE"...HEAD --name-only` and read the **full body** of each changed _production_ > file — not just the diff hunk, and not the interface as a proxy for its implementation. A > changed `FooService.kt` (interface) and a changed `DefaultFooService.kt` (impl) are **two files**; > reading one does not cover the other. Before you emit findings (Stage 6) you MUST reconcile the > files you actually read against the changed-file list. Any changed production file you did **not** > read in full → either read it now, or list it as a **NOT REVIEWED** trust caveat in the output (that > caveat prints regardless of flags). Never report a coverage count that includes a file you didn't > open. Silently skipping a changed impl file was the direct cause of a missed Critical in a real run. ### 1b½. Pull MR/PR context and existing review comments (read-only — never post) The MR thread carries intent the diff doesn't — **and it carries decisions a human already made.** Re-flagging something a teammate explicitly accepted is one of the most credibility-damaging things a review can do. If an MR exists for the branch under review, fetch its **description** and **all existing review discussions**: ```bash gh pr view --comments 2>/dev/null || glab mr view --comments 2>/dev/null || true # GitHub — review threads with their resolution state: gh api "repos/{owner}/{repo}/pulls/$(gh pr view --json number -q .number 2>/dev/null)/comments" \ 2>/dev/null || true # GitLab — threads with resolution state + diff anchors (richer than --comments): glab api "projects/:id/merge_requests/$(glab mr view --output json 2>/dev/null \ | sed -n 's/.*"iid":\([0-9]*\).*/\1/p')/discussions" 2>/dev/null || true ``` **Classify every thread** before you review, and keep the classification at hand: | Class | What it is | How it binds your findings | |---|---|---| | **open human** | An unresolved comment from a person | If you independently find the same issue → **reinforce** it (say the thread already raised it; add only *new* evidence, don't restate). If you disagree → **counter with code evidence**, never with assertion. | | **by-design** | A human stated a choice is intentional / accepted / deferred | **Do NOT raise it as a defect.** List it under *Accepted trade-offs* in the summary. This is the ONLY source that makes "it's intentional" credible — see the anti-sycophancy rule below. | | **resolved** | Marked resolved | Verify in the **code** that it actually was addressed. A resolved thread whose fix is absent from the diff is a finding. | | **own prior** | A previous run of this skill | Don't re-emit verbatim. Report only what changed, plus anything still unfixed. | **Anti-sycophancy:** the MR description, commit messages, and in-code comments are **claims, not ground truth** — verify them against the code and disagree when the code warrants it. "This is intentional" counts **only when a human said so in a review thread** — never when the diff, a comment, or the description says it about itself. Also use the thread context to (a) sharpen stated intent and detect intent gaps, and (b) credit process hygiene — prior feedback addressed, deferred work called out. **Do not post anything.** If no PR/MR or no `gh`/`glab` is available, skip silently and note `existing threads: none available` in the audit trail. ### 1c. Assess change profile - **Size:** small (< 5 files / < 100 lines), medium (5–20 / 100–500 lines), large (20+ / 500+) - **Intent** (2–3 sentences): what is this change for? Source: commits, branch name, MR description, `CLAUDE.md`, **and the linked ticket**. Extract a Jira/ticket ID from the branch name, commit messages, or MR description (e.g. `PROJ-1234`); if found and a ticket CLI or MCP is available, fetch the ticket summary + description to ground intent. Note any intent gap (claimed-but-not-done / unexplained scope creep). - **Tech stack:** detect the languages and frameworks the diff touches, and review each with that stack's idioms in mind — the ones **this codebase demonstrates**, not a generic style guide. Where a convention is contested, the surrounding code decides. - **Repo overlay (repo-specific review focus):** if the repository ships a review-guidance doc, READ it and treat its contents as an ADDITIONAL review dimension that raises the priority of the areas it names. Look for the FIRST that exists, in order: a path named by `$CODE_REVIEW_OVERLAY`, then the repo's `docs/code-review.md` (a tool-agnostic, repo-bound location any agentic reviewer can share). Use it to learn what THIS repo cares about (e.g. "always check tenancy scoping on DB queries", "flag any new env var", "migrations must be backward-compatible"). Repo guidance can RAISE severity/focus but must NOT silence a real defect it fails to mention. Note in the output which overlay (if any) was loaded, or "none". - **AI-generation signals:** over-described names, docstrings on trivial code, possibly-nonexistent imports, tests that mirror the impl. Note confidence score (0–3 signals = low, 4+ = high). ### 1d. Understand-before-review gate **Do not review code you do not yet understand.** If, after the sources above, the intent or the expected behavior is still materially unclear (you cannot state _what_ the change does and _why_): - **Interactive session:** ask the user 1–2 specific questions before proceeding (e.g. "Is the reordering in `X` intentional?", "What's the ticket for this?"). Wait for the answer. - **Headless / non-interactive run:** proceed, but record each unresolved ambiguity as an **Open Question** (Stage 6) rather than guessing intent. ### 1e. Static blast-radius pre-pass (deterministic — run the tool, don't reason it) A vendored tree-sitter tool computes, in code (no LLM), the **off-diff files that depend on the changed symbols** and **resources the change reads that other flows destroy** — the exact cross-file/off-diff facts that reviews repeatedly miss by declaring "nothing else touches this." **Run it before you form any cross-file judgement.** The tool is **bundled with this skill**, so its path needs no searching — run exactly this: ```bash node "${CLAUDE_SKILL_DIR}/tools/blast-radius.mjs" "$(git rev-parse --show-toplevel)" --base "$BASE" ``` `${CLAUDE_SKILL_DIR}` is a **load-time substitution** — Claude Code fills it in with this skill's directory when it injects this body. It is not a shell env var (empty in a plain shell). If you are reading this file as raw text and see the literal `${CLAUDE_SKILL_DIR}`, substitute the directory holding THIS `SKILL.md`. **Never locate this tool with `find`.** More than one copy of a code-review skill can exist under `$HOME/.claude` (an older version, a second install, a dev checkout), and a `find … | head -1` would silently run whichever one the filesystem happened to return first. You MUST actually execute this (one Bash call). Do not paraphrase it, and do not report a result you did not see printed. - **Use its output as ground truth for Stage 2a and 2c.** Every file it lists under "off-diff files that depend on changed symbols" MUST be accounted for in your impact map; every "read here / destroyed elsewhere" resource pair MUST be resolved in the hazard declaration. - **The three legitimate outcomes** — record exactly one, and never invent a fourth: | Outcome | When | What you do | |---|---|---| | `RAN` | the report printed | use it as ground truth | | `UNAVAILABLE` | `node` is missing, the path does not exist, or the tool errored — **you saw the failure** | fall back by hand (below) + trust caveat | | `DENIED` | the user declined the Bash call | fall back by hand (below) + trust caveat | For `UNAVAILABLE` and `DENIED` the manual fallback is the same: `rg` for other consumers of each changed exported symbol, and for who creates/deletes/drops each resource the change reads. Say which outcome occurred in the trust caveats. **Never silently skip the pre-pass, and never claim `RAN` for output you did not see.** A denied permission is not a missing tool — do not report it as one. - The tool **surfaces, it does not interpret** — it hands you the files; you still must reason whether each dependent actually breaks (e.g. renders outside a required provider, races a teardown). A listed file you dismiss must be dismissed _with a reason_, not ignored. - **Blind spot to remember:** the import graph cannot see dynamic wiring — event buses, DI containers, reflection, annotation-driven dispatch, string-keyed registries. The resource read/destroy pass and your own tracing must cover those. --- ## Stage 2 — Anatomy: Impact Map & Risk Surfaces ### 2a. Impact map (mandatory) > **Breaking-functionality mandate (verbatim — also given to every reviewer):** > This is a complex codebase, with many cross-package/module dependencies. Often simple code > changes in one place have subtle interactions that break functionality elsewhere. You MUST be > extremely thorough in tracing through possible side effects of the changes. Do not stop at the > direct caller — trace transitively until you reach total confidence. For **every exported/shared symbol the change touches** (function, class, type, schema, config key, public contract): `grep` its usages across the whole repo and assemble: - **Call-site list:** files + lines that use the changed symbol - **Impact set:** files affected by the change but NOT in the diff - **Blast radius:** low (local/private), medium (module-wide), high (cross-module/public API) A confirmed broken caller is a **Critical** finding. Never conclude "fine" on a symbol whose call-sites you haven't read. #### Delegation-trace gate (mandatory artifact — the interface is not the behavior) A changed file's _behavior_ often lives in a **first-party service it delegates to whose impl is NOT in the diff**. The declared signature (a `Result`/`Either`/union return type, a typed error channel) tells you the _contract_, not what the body does — it hides errors raised _outside_ that channel (`require`/`check`/`throw`), uncaught collaborator calls, side effects, and invariants. Reasoning from the interface's declared branches and stopping there was the #1 cause of a real missed Critical. **Produce this table — it is a mandatory artifact** (the Stage 6 delegation-trace gate checks it; it is *printed* only under `--audit`/`--findings`/`--json`). For every symbol a changed file delegates to, where the impl is **first-party** (repo-local, not stdlib/framework/third-party) AND (its impl is not in the diff OR blast-radius ≥ medium): | Delegated symbol | Impl `file:line` | Opened? | Fails outside declared channel? | | ----------------------- | ------------------------------------ | ------- | ------------------------------------------------ | | `svc.applyQuota()` | `DefaultQuotaService.ts:74` | yes | argument check throws instead of returning `Result` | Every row must be `Opened? = yes` before you emit a verdict, or the row is marked **NOT TRACED** and surfaced in output. Do **not** trace into stdlib/framework/obvious-getter delegations — scope it to first-party services carrying real logic, so this stays bounded. **Named check — error-channel consistency.** For each traced delegation from a gate/adapter/handler: does the callee ever fail _outside_ the channel the caller handles? (the service declares a `Result` return, the impl throws on a failed precondition, and the caller only handles the failure *value* → the throw propagates → fail-open, or the caller's transaction aborts). Channel disagreement is **≥ Important**. When the two sides encode _deliberately conflicting intents_ (callee "fail loud", caller "never block"), frame it as **a conscious decision the author must make** (fail-open with a `warn` + alerting metric) — not a mechanical "wrap in try/catch." For refactors that re-route rendering through a different container or composition (e.g. inline JSX → a `headerContent`/`footerContent`/slot prop, or a context-based child → a passed prop), diff **where** each moved element renders — order, placement, parent — not just **whether** its logic is equivalent. "Same logic, different position" is a real finding the symbol-level impact map cannot see. ### 2b. Risk surface identification For each surface below, write one sentence: is it a real risk for THIS MR, and why? | Surface | Key questions | | ----------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | **Correctness** | Complex logic, edge cases, error paths, null/empty, races in this diff? | | **Security** | Auth boundary touched, untrusted input to new handlers, injection, IDOR? | | **Cross-package / Impact** | Confirmed broken callers? Shared contract changed in a breaking way? **Cross-package breakage** (a change here breaks a consumer in another module/package)? **Feature-leak** (an unrelated feature silently affected)? **Devex-breakage** (build/tooling/local-dev broken)? | | **Maintainability / Simplicity** | Is the code easy to reason about and change? Is it in the **right file/module**, or does it leak business rules that belong elsewhere? Does it force callers to know an unsafe sequence (e.g. "call A then B in order") that should be **hidden behind one interface so invalid states are unrepresentable**? SOLID / module-boundary violations? | | **Test gap** | New paths/changed behavior with no coverage? Tests removed/skipped? Right **level** (unit vs integration vs FE component)? Are tests **valuable** (assert behavior) or mirror-the-impl? **Is the diff's test change real, or only mechanical renames/plumbing while new logic ships untested?** | | **Behavioral / layout parity** (refactors only) | Does anything render in a different place, order, or parent container — even if its props/gating logic are unchanged? Routing a component through a new slot is a behavioral change. | | **Performance** | N+1 query in a resolver/hot path, blocking call in async code, needless alloc in a tight loop? | | **Conventions** | AI-generation patterns, idiom violations, project rule violations? | Mark each: `REAL — [reason]` or `NOT RELEVANT — [reason]`. Real surfaces become reviewer candidates. > **Test-gap escalation:** if the MR's primary artifact is new non-trivial logic (hook / util / > transform / reducer) AND the changed test files are only renames or import fixes, that is an > **Important** finding by default — and the reviewer must emit a concrete assertion list > (inputs → expected `addOperation`/return/dispatch), never a vague "add a test." > **Scope clamp — don't do the linter's job.** Do NOT flag anything a linter, formatter, or > type-checker would catch (unused imports, spacing, `any` that `tsc` would reject, missing > semicolons). If that automated coverage is _missing_, propose adding the lint rule or test as a > single low-severity suggestion — do not hand-audit every instance. ### 2b½. Independent hazard classification (a second, blind opinion) Under-declaring a hazard (marking `NO` and never looking) was the exact failure that let real must-catches slip. To defeat it, get a **second, independent classification** before you write your own — from a source that can't inherit your blind spot: **Dispatch one small subagent** (Task tool) given ONLY the diff + changed-file list (NOT your analysis), with this instruction: _"For each hazard class below, answer YES/NO with a one-line reason, from the diff alone. Cross-file/contract · Async/concurrency/ordering · Resource-lifecycle · State-exhaustiveness · Security/trust-boundary. Bias toward YES when unsure — you are the skeptic."_ It returns a 5-line verdict, nothing else. **Run it in the foreground and wait for its 5 lines before continuing.** Stage 2c reconciles against this verdict, so a review that proceeds without it has nothing to reconcile. Never leave it pending. > **Non-optional — no size escape.** Dispatch it for EVERY review regardless of diff size or your > confidence. "Small / presentational / blast-radius low, so I skipped it" is exactly the > rationalization that misses the cross-file bug hiding in a 2-file change — a production crash once > shipped inside a 2-file, presentational-looking refactor. It is one cheap subagent. Its **5-line verdict must be > recorded verbatim in your audit trail** — an empty/absent classifier verdict means the review is > incomplete (Stage 6 gate rejects it), whether or not the flags print the audit trail. You now have **three independent signals** per class: (a) the Stage 1e **static tool**, (b) this **blind classifier**, (c) your **own** read. **Rule: if EITHER the tool OR the classifier says a class applies, you may not declare it `NO` without producing the same both-side `file:line` evidence a `YES` requires** — an override is a positive claim ("searched, none found"), never silence. This is the forcing function; the tool and the classifier fail differently, so together they close the under-declaration hole. ### 2c. Hazard-class declaration (declare now — enforced before you emit) These are **universal, language- and repo-agnostic hazard classes** — the recurring ways a change breaks things _at the boundary of what it touches_. For THIS change, **classify each `YES`/`NO` with a one-line reason** (a semantic judgment — do not pattern-match filenames), **reconciled against the Stage 1e tool output and the 2b½ classifier**. This declaration is **checked against your own analysis in Stage 6**: every class you mark `YES` MUST have its required artifact present and non-empty, or the review is incomplete. Declare honestly — under-declaring to skip the work is the exact failure this gate exists to stop. > **The artifact must be EVIDENCE, not a checkbox.** A `YES` is satisfied ONLY by concrete > `file:line` anchors on **both sides of the boundary** — never by prose like "there may be a race" > or "handled elsewhere." Naming one side and _assuming_ the other is the exact failure that let a > real must-catch slip. Each `YES` artifact must cite the specific lines below (or, where a side > genuinely doesn't exist, the literal words **"none found"** after you looked — not silence). | Hazard class | Marks `YES` when the change… | Required artifact if `YES` — **cite `file:line` on both sides** | | ---------------------------------- | ------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | **Cross-file / contract** | modifies a shared symbol, interface, type, schema, or public contract others depend on | the changed symbol `file:line` **+** each call-site `file:line` (impact map 2a) **+** the delegated impl `file:line` (delegation-trace 2a) | | **Async / concurrency / ordering** | adds or modifies event/message/outbox/scheduled/async/concurrent processing, or a new consumer of an existing event | the handler/consumer `file:line` **+** the producer/emit `file:line` **+** the concurrent-flow evidence below | | **Resource lifecycle** | creates, deletes, drops, migrates, or locks a resource (table, file, record, connection, lock) another flow uses | the **reader** `file:line` (where the resource is used) **+** the **mutator/destroyer** `file:line` (the _other_ flow that creates/drops/locks it) **+** the **guard** `file:line` protecting the read (or `none found`) | | **State exhaustiveness** | adds a new enum/sealed/union variant, status, or state | the new variant `file:line` **+** every exhaustive `when`/`switch`/`match`/adapter `file:line` you confirmed handles it | | **Security / trust boundary** | touches auth, permissions, or accepts input from an untrusted source | the untrusted-input/boundary `file:line` **+** the authz/validation `file:line` protecting it (or `none found`) | **Concurrent-flow evidence (Async / Resource-lifecycle `YES`) — the analysis a real run missed.** Fill every field with a real anchor; `none found` is a valid value only after you looked, and an unguarded read of a resource another flow can destroy is at least **Important**: - Shared resource: `` - Reader (uses it): `file:line` - Other flow that creates / destroys / mutates it: `file:line` ← **you MUST open that flow, not assume it** - Can they interleave (no shared lock / ordering guarantee)? `` - Guard on the read (existence check / lock / ordering): `file:line` or `none found` Build this declaration as a table — a mandatory artifact, checked by the Stage 6 hazard gate and *printed* only under `--audit`/`--findings`/`--json`. Marking everything `NO` on a non-trivial change is itself a flag: re-read before you conclude the change touches none of these. --- ## Stage 2.5 — Orchestrator First-Pass Review (you review first, then dispatch) You have every changed file loaded and the impact map in hand — so **do a real first-pass review yourself now**, the way a senior engineer skims a PR before pulling anyone else in. Across the risk surfaces from Stage 2b, produce a **provisional findings list**: for each, note `file:line`, a one-line concern, and **your confidence (low/med/high)**. This first pass is the baseline coverage. Subagents are **not** the primary reviewers — they are a **force-multiplier on the surfaces you can't close yourself**. Use the first pass to triage: - **High-confidence, self-closed** → keep as your own finding — **UNLESS the mandatory-dispatch floor below applies.** Your own confidence does **not** waive the floor: "I traced it and I'm sure" is exactly the rationalization that produced a real missed Critical. - **Risky but uncertain** (needs deeper cross-file tracing, you're **Mandatory-dispatch floor (not waivable by confidence or "budget discipline").** You MUST dispatch > **at least one** independent subagent to characterize behavior when EITHER holds: > (a) the change **delegates to a first-party service whose behavior is not fully in the diff** > (give the subagent the exact question — e.g. _"Under what conditions does > `DefaultQuotaService.applyQuota` fail, and does it ever fail outside its declared `Result` channel? > Read the impl body."_), OR (b) **blast-radius ≥ medium**. Budget is a **ceiling, never a reason to > skip** the heart-of-the-change symbol. "0 reviewers dispatched" is only valid for a genuinely > contained change with blast-radius = low and no first-party delegation. > Do not collapse the whole review onto this single pass — recall on the hardest bugs is a > coin-flip for any one pass, so the risky surfaces still get an **independent** subagent. The > first pass decides _where_ that independent effort is worth spending, and sharpens the question. > **No interface-only "safe" verdicts.** Any "fails-open / handled / safe / no-op" conclusion > about a delegated call MUST cite the **impl `file:line` you read** (the Delegation-trace row). > "Reasoned from the interface's declared branches" is **not** an acceptable basis for a safe verdict > — that is the exact shortcut that missed the Critical. No traced row → not safe → dispatch or trace. Carry the provisional findings into Stage 5 synthesis (they merge with reviewer findings; agreement between your pass and a reviewer **raises confidence**). --- ## Stage 3 — Review Planning (the orchestrator's job — quality here = quality everywhere) After Stages 2 and 2.5 you have a full picture and a triaged first pass. Now plan the review: ### Budget Total target: **~$3.00 (Sonnet) · ~$8.00 (Opus)**. **This budget is a CEILING, never a reason to skip a reviewer** — under-spending while a mandatory-dispatch trigger (Stage 2.5 floor) is unmet is a failure, not thrift. Stages 1–2 cost roughly **$0.30–0.60**. Per reviewer: **$0.30–0.80** (they reason on pre-loaded content, no redundant file I/O). Per-reviewer cost scales with scope; keep prompts focused. | Change profile | Reviewers to dispatch | | ---------------------------------------------------------------- | ----------------------------- | | small + blast-radius=low + no first-party delegation | 1 | | medium OR blast-radius=medium OR first-party delegation off-diff | 2 | | large OR blast-radius=high | 3 | | large + blast-radius=high + multiple critical surfaces | **4 (exceptional, max ever)** | The Stage 2.5 mandatory-dispatch floor overrides this table downward-never: these are _minimums_, and 0 is never valid when a first-party off-diff delegation or blast-radius ≥ medium is present. ### Reviewer plan Dispatch reviewers for the surfaces your **Stage 2.5 triage** flagged as uncertain, risky, or needing specialized depth — not for surfaces you already closed at high confidence. For each such surface, draft a reviewer entry: ``` Reviewer N — [Short Name] · Priority: Critical | High | Medium Target files: [specific files this reviewer examines — from Stage 1 file list + impact set] Specific questions: 1. [Precise question grounded in Stage 2 anatomy — not a generic checklist item] 2. ... ``` Rank by priority. Apply the budget rule above to decide how many to dispatch. For EACH real surface you are SKIPPING, write one line: `SKIP [Surface] — [reason it's not worth a reviewer call]`. This plan is the hardest and most important step — a well-crafted reviewer prompt finds the bug; a vague one produces noise. Take the time here. --- ## Stage 4 — Dispatch Reviewers (parallel, FULLY CONTRACTED, fully collected) Dispatch planned reviewers **in parallel** using the Task tool in a single message. ### Collection contract — the review is emitted ONCE, after the last reviewer returns A review that prints early and then dribbles in a late reviewer's findings as a follow-up "update" is a **failed review**: the reader has to reassemble the verdict themselves, and the severity ranking, the compound-risk pass (Stage 5) and the verdict line were all computed against incomplete data. So: - **Dispatch every reviewer in ONE message**, so they run concurrently rather than serially. - **Run them in the foreground and wait for all of them.** Do not run reviewers as background tasks; do not poll; do not start Stage 5 while any is outstanding. - **Emit nothing until every dispatched reviewer has returned** — no partial findings, no per-reviewer running commentary, no "here's what I have so far", no interim severity counts. Progress narration is exactly what turns one review into three messages. - **Never end your turn with a reviewer pending.** If your turn is about to end, you are not done waiting. - If a reviewer **errors, returns empty, or is skipped by the user**, do not silently drop its surface: re-dispatch it **once**; if it fails again, treat its surface as **unreviewed** — name it under *Surfaces not covered* in the summary and lower your confidence accordingly. A missing reviewer must be **visible**, never invisible. - The Stage 2b½ classifier must already be collected (Stage 2b½) before you get here. Each reviewer prompt MUST contain: ### A. Context brief (embed verbatim) ``` === CONTEXT BRIEF === Intent: [2–3 sentence intent from Stage 1] Breaking-functionality mandate: This is a complex codebase with many cross-package/module dependencies. Simple changes often have subtle interactions that break functionality elsewhere. Be extremely thorough tracing side effects — do not stop at the direct caller; trace transitively until you reach total confidence. When the change delegates to an interface/service, OPEN THE CONCRETE IMPLEMENTATION and read the delegated method's body — the signature does not reveal what it throws (require/check/throw outside the declared error channel), its uncaught collaborator calls, or its side effects. Check error-channel consistency: does a callee ever fail outside the channel its caller handles? Do NOT flag what a linter/type-checker would catch. Severity pins: [any pins the repo overlay defines] Full diff: [paste the full git diff "$BASE"...HEAD output] Full file contents (all files you will need — DO NOT read additional files): --- FILE: path/to/ChangedFile.kt --- [complete file content] --- END FILE --- --- FILE: path/to/CallerFile.kt --- [complete file content] --- END FILE --- Impact set (paths only — reference only if you already see a risk): [call-site list + blast radius paths from Stage 2] === END CONTEXT BRIEF === ``` ### B. Specific assignment (from Stage 3 plan) - Your reviewer name and priority - Target files + line ranges to focus on - The specific questions to answer from Stage 3 ### C. Leaf-node restriction (include verbatim, every time) > **YOU ARE A LEAF NODE IN THE REVIEW GRAPH.** > All file contents you need are in the context …(truncated)