# Pr Review

> Review the current diff for regressions, correctness bugs, tests, simplifications, and docs issues, scaling depth to a low/medium/high/xhigh/max effort level. Use when the user asks to review changes, review a diff/branch/PR, or runs /pr-review. Pass --comment to post a top-level PR comment, --comment inline for inline PR comments, or --fix to apply findings.

- Skill: `mui/pr-review` (Agent Skill, multi-file: 2 files)
- Install (CLI): `npx skillmds@latest add mui/pr-review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/mui/pr-review/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- Author: mui (https://skillmd.com/u/mui)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/mui/pr-review

---


# PR Review

Review current diff. Report **regressions and correctness bugs** plus
**cleanup** (reuse / simplification / efficiency). Effort default `medium`; use
[Effort levels](#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](#scope) or the invoking prompt, every subagent prompt you
construct — finder, specialist, verifier, sweep — must begin verbatim with it:

```text
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](#effort-levels) 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](#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:

````md
# 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.