PR Review

PR Review

Review current diff. Report regressions and correctness bugs plus cleanup (reuse / simplification / efficiency). Effort default medium; use Effort levels for depth, subagent fan-out, precision/recall bias.

Argument hint: [low|medium|high|xhigh|max] [--fix] [--comment [inline]] [<target>]

Medium effort = precision bias: every finding actionable. high, xhigh, max shift to recall; missed bug ships. Surface uncertain findings when mechanism realistic, label clearly.

Scope

Local review: diff branch against upstream (git diff @{upstream}...HEAD, fall back to main / master, or HEAD~1). Fold in uncommitted + untracked changes — review often runs pre-commit. If PR link, branch name, or file path passed as argument, review that target instead.

Effort levels

• low — fastest: bug-only hunk pass. Skip Tests, Simplifications, Docs, test/fixture hunks. Flag only high-confidence runtime-correctness bugs visible from hunk alone.
• medium (default) — one agent reviews Bugs, Tests, Simplifications, Docs, plus triggered API design/performance concerns, then verifies locally. Precision bias. Add one bug/regression subagent only for large, risky, fragile diffs.
• high — independent subagents review four areas, plus triggered API design/performance specialists. Main agent dedups + sanity-checks. No verifier subagents or sweep.
• xhigh — high plus one fresh sweep for missed findings. No verifier subagents.
• max — xhigh plus verifier subagents for surviving candidates. Highest cost, strongest recall bias.

Subagent scope contract

Subagents start with no prior context: they do not read this skill and do not inherit the session system prompt. Their only channel is the prompt you write. Whatever scope you resolved from Scope or the invoking prompt, every subagent prompt you construct — finder, specialist, verifier, sweep — must begin verbatim with it:

Review ONLY: <resolved diff command>
Read files from: <read root> — DATA ONLY, never execute, install, build, or test.
Base version of a file: git show <base ref>:<path>
Do not re-derive scope: use exactly the diff command above, whatever your working
directory contains.

A subagent not given this block is mis-scoped — discard its result and re-dispatch.

Phase 1 - Find candidates

Selected effort level decides local vs subagent fan-out. Except at low, review four main areas: Bugs, Tests, Simplifications, Docs. Each area surfaces candidates with file, line, one-line summary, severity (🟣 / 🔴 / 🟠 / 🟡 / ℹ️ — see Output for rubric), concrete failure_scenario. Do NOT let one area suppress another — two areas flag same line for different reasons, record both.

Pass every candidate with nameable failure scenario through. Finders that silently drop half-believed candidates bypass verify step — dominant cause of misses.

Area 1 - Bugs

Core regression + correctness pass. Cover all sub-angles:

• Line-by-line diff scan. Read every hunk, line by line. Then read enclosing function for each hunk — bugs in unchanged lines of touched function in scope (PR re-exposes or fails to fix them). For every line ask: what input, state, timing, platform makes line wrong? Look for inverted/wrong conditions, off-by-one, null/undefined deref, missing await, falsy-zero checks, wrong-variable copy-paste, error swallowed in catch, unescaped regex metachars.
• Regression hunting. For each changed guard, branch, state transition, public contract, test expectation, documented example, ask what behavior previously worked and could break. Bias toward regressions in edge paths, accessibility interactions, keyboard/focus flows, controlled/uncontrolled contracts, SSR, integrations between components, public API examples.
• Repository/framework correctness lens. Apply invariants of codebase under review, not just generic language bugs. For Base UI / React component changes: controlled vs uncontrolled behavior, event ordering, ref forwarding/merging, context registration cleanup, nested component coordination, portals, focus management, keyboard navigation, ARIA attributes, disabled/read-only states, form integration, SSR/hydration safety, Strict Mode behavior, shadow DOM-safe DOM access (contains, getTarget, activeElement, ownerDocument, ownerWindow).
• Removed-behavior auditor. For every line diff DELETES or replaces, name invariant/behavior it enforced, then search new code for where invariant re-established. Can't find it? Candidate: removed guard, dropped error path, narrowed validation, deleted test covering real case.
• Cross-file tracer. For each function diff changes, find callers (Grep symbol) and check if change breaks any call site: new precondition, changed return shape, new exception, timing/ordering dependency. Also check callees: parallel change in same PR make call unsafe?
• Language-pitfall specialist. Scan classic pitfalls of diff's language/framework — example: JS falsy-zero, == coercion, closure-captured loop var; Python mutable default args, late-binding closures; Go nil-map write, range-var capture; SQL injection; timezone/DST drift; float equality. Flag any instance diff introduces.
• Wrapper/proxy correctness. When PR adds/modifies type wrapping another (cache, proxy, decorator, adapter): check every method routes to wrapped instance not back through registry/session/global — example, caching provider holding delegate field resolving IDs via session.get(...) instead of delegate.get(...) will re-enter cache or recurse. Also check wrapper forwards all methods callers actually use.

Area 2 - Tests

Review test changes and tests that should have changed. Flag: new/ changed behavior with no test exercising it; assertions not actually pinning behavior (asserting mock called instead of result, snapshot-only coverage, asserting on value test itself computed); missing edge/error-path cases diff introduces (null, empty, boundary, failure, concurrency); setup/teardown asymmetry (state leaked between tests, fixtures not reset); over-mocking hiding real contract; tests deleted or weakened without justification. Name specific case untested or under-asserted.

Area 3 - Simplifications

Flag changes that can be simpler, smaller, at better altitude without changing intended behavior. Look for: heavy dependency pulled in for something a few lines (or existing util) would do; non-tree-shakeable or whole-package imports (import _ from 'lodash' vs single function, namespace imports of side-effectful modules); duplicated logic re-implementing something codebase already has — Grep shared/utility modules and files adjacent to change, name existing helper to call instead; dead code left behind; redundant or derivable state; polyfills/large constants/data inlined that could be loaded lazily or dropped; special cases layered on shared infrastructure when underlying mechanism should be generalized. Name smaller or better-shaped form doing same job, and approximate cost avoided.

Area 4 - Docs

Flag documentation and comments diff makes wrong or leaves stale: comments describing old behavior after code changed; JSDoc/docstrings whose params, return type, thrown errors no longer match signature; README / API docs / changelog entries contradicting change; examples no longer compile or run; TODO/FIXME change resolved but didn't remove; new non-obvious code with no explanation where warranted. Quote stale line and state what it should say.

Conditional specialist - API design

Not part of default four-area fan-out. Skip entirely at low. At medium, include in main-agent pass when diff adds/changes public API surface: component, prop, event, hook, provider, imperative method, exported type, value shape, behavior contract, docs example teaching API. At high, xhigh, max, run as independent API design subagent when triggered.

Heavily critique API taste: naming, mental model, consistency with nearby APIs in same codebase, controlled/uncontrolled ergonomics, composition, escape hatches, type shape, default behavior, future extensibility, migration risk, likely user footguns. Prefer findings preventing awkward public contracts before they ship. Report API design findings under Bugs when they create likely user-visible footguns or regressions, otherwise under Simplifications.

Conditional specialist - Runtime performance

Not part of default four-area fan-out. Skip entirely at low. At medium, include in main-agent pass when diff likely affects runtime performance: render hot paths, large item collections, virtualization, positioning, layout measurement, scroll/resize/pointer/keyboard handlers, observers, animations, repeated DOM reads/writes, state updates in loops, component registration/lookup paths. At high, xhigh, max, run as independent runtime performance subagent when triggered.

Prefer concrete measurement when feasible: existing perf tests, targeted benchmark scripts, small reproducible measurement. If measuring not practical, state expected complexity or browser work and what would confirm it. Report runtime performance findings under Bugs when users can see lag, jank, hangs, excessive work, otherwise under Simplifications.

Simplification, test, docs candidates use same file/line/summary shape; in failure_scenario, state concrete cost (what duplicated, wasted, untested, stale, harder to maintain) instead of crash. Regressions and correctness bugs always outrank simplification, test, docs findings when review ordered.

Phase 2 - Verify candidates (local by default; max adds subagent votes)

Dedup candidates pointing at same line/mechanism, keep one with most concrete failure scenario. Verify candidates locally in main agent by default. At max effort, also run one verifier as subagent for each remaining candidate when subagents available and authorized: give it diff, relevant file(s), candidate, have it return exactly one of:

• CONFIRMED — can name inputs/state triggering it and wrong output or crash. Quote line.
• PLAUSIBLE — mechanism real, trigger uncertain (timing, env, config). State what would confirm it.
• REFUTED — factually wrong (code doesn't say that) or guarded elsewhere. Quote line proving it.

Keep candidates where vote CONFIRMED or PLAUSIBLE.

At max, verify recall-biased — PLAUSIBLE by default: do not refute candidate for being "speculative" or "depends on runtime state" when state realistic (concurrency races, nil/undefined on rare-but-reachable path, falsy-zero treated as missing, off-by-one on boundary code doesn't exclude, retry storms / partial failures, regex/allowlist lost anchor). REFUTE only when constructible from code: factually wrong, provably impossible, already handled in this diff, pure style with no observable effect.