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.

Phase 3 - Sweep for gaps (xhigh / max effort)

At xhigh and max, run one more finder as fresh reviewer with current finding list. Re-read diff and enclosing functions looking ONLY for defects not already listed. Do not re-derive or re-confirm anything already there — job is gaps. Focus on what first pass misses: moved/extracted code dropping guard or anchor; second-tier footguns (dataclass default evaluated once, hash() non-determinism, lock-scope shrink, predicate methods with side effects); setup/teardown asymmetry in tests; config defaults flipped. Surface additional candidates naming defect not already on list. If nothing new, return empty sweep — do not pad.

Output

Reply in Markdown. Do not wrap main result in JSON or code fence. Use sentence case for # heading and all finding titles; no title case. Do not open with generic target/base filler like "Reviewed owner/repo#123 against branch." Only mention target or base branch when explaining specific finding or limitation.

When findings exist, use exactly these ## sections, in order: Bugs, Tests, Simplifications, Docs, Verdict. Any section whose count is (0), write exactly No findings. under that heading.

Severity rubric

Prefix every finding title with one severity marker:

• 🟣 critical - blocking. Correctness issue crashing common flows, losing user data, creating security/privacy problem, making public component contract unusable, or creating broad cross-component accessibility, focus, form, SSR breakage. Must block merge.
• 🔴 high - blocking. Realistic user-visible regression in normal component usage, accessibility, keyboard/focus behavior, controlled/uncontrolled behavior, form integration, SSR/hydration, public API behavior, or public docs example no longer compiling or teaching broken API usage. Should block merge even when affected path has workaround or not most common path.
• 🟠 medium - non-blocking. Real issue worth fixing before merge when practical but not normally blocking: narrow or uncommon bug, meaningful missing test, stale public-doc prose not breaking copy-pasted code, cleanup avoiding likely future mistakes. Use only when issue doesn't break public contract and doesn't affect accessibility, focus, forms, SSR, or normal component usage.
• 🟡 low - non-blocking. Minor edge-case bug, small test/doc gap, modest simplification useful but easy for maintainers to defer.
• ℹ️ note. Informational — observation, heads-up, optional suggestion the maintainer may reasonably decline.

Base UI has stricter quality bar than most apps — small component-library regressions multiplied across many downstream products. When choosing between adjacent severities, choose higher severity if failure reaches realistic consumer usage, accessibility behavior, form behavior, focus management, SSR, public API contract or broken public docs example. Do not reserve 🔴 high only for outage-level failures.

Within each section, order findings 🟣 -> 🔴 -> 🟠 -> 🟡 -> ℹ️.

Verdict

End review with ## Verdict line derived from markers:

• Request changes — at least one 🟣 or 🔴 finding.
• Approve after nits — no 🟣 or 🔴, but one or more 🟠 or 🟡 findings worth addressing.
• Approve — no 🟣, 🔴, 🟠, 🟡 findings (only ℹ️ notes, or nothing).

Follow verdict with one clause naming deciding factor.

Finding format

Use this structure:

# PR review

One to three sentences summarizing the concrete risk, most important theme, and
any meaningful verification limit. State up front whether anything is merge-blocking.
If there are no actionable findings, say that clearly.

## Bugs ({count})

### 1. {severity marker} {Sentence-case title}

**Location:** `path/to/file.ext:123`

```tsx
// Small relevant code excerpt from the reviewed file.
```

{Description of what is wrong and why it matters.}

**Failure scenario:** {Simple, obvious user-facing or example issue someone would
see.}

**Fix:** {Specific recommended change.}

## Tests ({count})

## Simplifications ({count})

## Docs ({count})

## Verdict

**{Request changes | Approve after nits | Approve}** - {one clause on the deciding factor}.

Same per-finding shape for Tests, Simplifications, Docs. For Tests, make failure scenario the missing case or weak assertion. For Simplifications, make it concrete duplicated, heavier, or harder-to-maintain code path. For Docs, make it stale or missing public/developer-facing information.

Finding quality

Do not cap findings by count. Report every verified, actionable finding maintainer would reasonably fix before merging. Do not pad review with weak, stylistic, speculative, low-importance notes. If many findings survive, group related issues under one finding when they share same root cause.

No findings

If nothing survives verification, return # PR review followed by No findings., brief note of any residual test gaps or risk, and ## Verdict of Approve.

Posting to GitHub (--comment)

Post to GitHub only when target is GitHub PR and arguments include comment mode:

• No --comment — do not post to GitHub; if --fix also absent, stop at Markdown review report.
• --comment — post Markdown review as one top-level PR comment via gh pr comment.
• --comment inline — post inline comments for findings mapping to PR diff lines, include all non-diff findings in top-level fallback comment.

Attribution belongs to whoever posts, not to the review body. When this skill posts — --comment, or the top-level fallback comment for --comment inline — close that comment with a --- horizontal rule, blank line, then 🤖 Review generated with {Claude Code | Codex}: Claude Code under the Claude Code harness, Codex under Codex.

For --comment inline, include same severity marker in each inline comment body. Use latest PR head commit_id, path, line, side, post via gh api (repos/{owner}/{repo}/pulls/{pr}/comments), one call per finding. Include suggestion block only when it fully fixes issue. (If GitHub inline-comment MCP tool available in session, use instead.)

Inline comments can only attach to lines present in PR diff. Put findings outside PR diff (unchanged lines in touched functions) in top-level fallback comment via gh pr comment; do not drop them or stop posting rest of review because one inline comment not commentable. If target not PR, print findings to terminal and note --comment was ignored.

Applying fixes (--fix)

If --fix flag passed in arguments, after producing findings list, apply findings to working tree instead of stopping at report: fix each one directly — correctness bugs and reuse/simplification/efficiency cleanups alike. Skip any finding whose fix would change intended behavior, require changes well outside reviewed diff, or you judge false positive — note skip rather than arguing with it. Finish with brief summary of what fixed and what skipped.

If neither --comment nor --fix passed, stop at Markdown review report.