---
name: pr-reviewer
description: Structured three-pass review of the working diff (staged + unstaged) — correctness bugs, security, then simplification — with severity-tagged findings and a ship/no-ship verdict. Use when the user asks to review their changes, review the diff, check a PR before pushing, or says "is this safe to ship".
---

# PR Reviewer

## Install

Save this file as `~/.claude/skills/pr-reviewer/SKILL.md` (or
`.claude/skills/pr-reviewer/SKILL.md` inside a repo to scope it there).
Claude Code auto-discovers it. Invoke with `/pr-reviewer` or by asking
"review my changes before I push".

## The review

You are performing a pre-push code review of the user's working changes. Your
job is to find real problems, not to demonstrate thoroughness. A short review
with two true findings beats a long review with ten speculative ones.

## Step 1 — Collect the diff

Run, in order:

1. `git diff --staged` — the staged changes.
2. `git diff` — unstaged changes.
3. `git status --porcelain` — catch untracked files that belong to the change;
   read any that look like part of the work (new source files, configs).

If all three are empty, reply exactly: "Working tree is clean — nothing to
review." and stop. If the user asked to review a branch instead of the
working tree, use `git diff <merge-base>...HEAD` (compute the merge base
against the default branch) and proceed identically.

The diff alone is not enough context. For every file in the diff, read the
surrounding code — at minimum the full enclosing function/class of each
hunk — before judging it. Reviewing hunks in isolation produces false
positives; this step is mandatory, not optional.

## Step 2 — Three passes

Make three separate passes over the diff. Do not blend them; each pass has a
different mindset, and blending them is how security issues get missed.

### Pass 1 — Correctness

Hunt for bugs that will produce wrong behavior:

- Logic errors: inverted conditions, off-by-one, wrong operator, unreachable
  branches, switch fallthrough.
- Broken contracts: callers of a changed function that weren't updated
  (grep for the symbol), changed return shapes, renamed fields still
  referenced elsewhere.
- Nullability and emptiness: unguarded access on values that can be
  null/undefined/empty on some path — but verify the path actually exists
  before reporting.
- Async mistakes: missing await, unhandled promise rejections, race
  conditions on shared state, missing cleanup in effects/subscriptions.
- Error handling: swallowed exceptions, catch blocks that hide failures,
  error paths that leave state inconsistent (e.g. lock taken, never
  released).
- Edge inputs: zero, negative, empty collection, unicode, very large,
  concurrent duplicates — only where the diff's code would actually receive
  them.
- Data integrity: writes without the invariants the rest of the codebase
  maintains (missing transaction, partial update on failure).

### Pass 2 — Security

Re-read the same diff assuming an attacker controls every input:

- Injection: string-built SQL/shell/HTML/paths. Flag any concatenation of
  user input into an interpreter, even "probably safe" ones.
- AuthN/AuthZ: new endpoints, actions, or handlers missing the
  authentication/authorization checks that sibling code performs; IDOR
  (object fetched by id without an ownership check).
- Secrets: keys, tokens, or credentials in code, logs, error messages, or
  client-reachable config; secrets newly written to files that aren't
  gitignored.
- Unsafe deserialization / dynamic execution of user-influenced data
  (eval, pickle, yaml.load, dynamic import paths).
- Sensitive-data handling: PII newly logged, cached, or sent to third
  parties; overly broad CORS; missing input size limits on new endpoints.
- Dependency changes: new packages — check for typosquat-looking names and
  whether the capability already exists in the project.
- Crypto misuse: home-rolled hashing/comparison for security purposes,
  non-constant-time token comparison, weak randomness for tokens.

### Pass 3 — Simplification

Now look for what makes the change harder to maintain than it needs to be:

- Dead weight: unused variables/params/imports the diff introduces,
  commented-out code, debug prints left in.
- Duplication: logic that already exists in the codebase (search before
  claiming — name the existing utility if found).
- Needless complexity: abstractions with one caller, config/flags nothing
  reads, deep nesting that early returns would flatten, clever one-liners
  that need a comment to decode.
- Scope creep: hunks unrelated to the change's purpose (suggest splitting
  the commit, don't demand it).
- Naming that lies: functions whose name no longer matches what the diff
  made them do.
Only report simplifications with a concrete, materially better alternative.
Style preferences and formatting nits that a linter would catch are not
findings.

## Step 3 — Verify every finding

Before writing the report, re-verify each candidate finding:

1. Re-read the finding's code WITH its surrounding context (the whole
   function, plus callers if the finding is about a contract).
2. Ask: is there a guard, type constraint, or invariant elsewhere that makes
   this a non-issue? Actually check — grep for callers, read the type
   definitions.
3. Drop any finding you cannot defend with a specific line reference and a
   concrete failure scenario. "This might be a problem" is not a finding.
4. Re-check severity honestly. Most findings are [MINOR]. Reserve [BLOCKER]
   for things that will cause incorrect behavior, data loss, or a security
   hole in realistic use.

## Step 4 — Report

Severity tags:

- [BLOCKER] — will cause incorrect behavior, data loss, or a security
  vulnerability. Must be fixed before merge.
- [MAJOR] — real defect or security weakness with limited blast radius, or
  correctness risk under plausible conditions. Fix before merge unless
  explicitly accepted.
- [MINOR] — worth fixing, doesn't block: maintainability problems, missing
  edge-case handling on unlikely paths, meaningful simplifications.
- [NIT] — take it or leave it. Max three nits per review; if you have more,
  they aren't worth reporting.

Output format:

```
## Review: <one-line summary of what the change does>

### Findings

1. [BLOCKER] src/billing/invoice.ts:42 — refund amount uses `price` before
   the discount is applied; customers get over-refunded.
   Fix: compute from `order.total` (line 38) instead.

2. [MAJOR] src/api/webhooks.ts:15 — new endpoint skips signature
   verification that stripe.ts:22 performs for the same provider.
   Fix: call verifyStripeSignature() before parsing the body.

3. [MINOR] src/lib/dates.ts:9 — duplicates formatRelative() from
   src/utils/time.ts:31.
   Fix: import the existing helper.

### Pass summary
- Correctness: <n> finding(s)
- Security: <n> finding(s) — state "none found" explicitly if clean
- Simplification: <n> finding(s)

### Verdict
<one of>
SHIP — no blockers, no majors. <one sentence why it's safe.>
FIX BLOCKERS FIRST — <list the blocker numbers>. Everything else can follow
in this or a later commit.
```

Report rules:

- Every finding gets: severity tag, file:line (of the changed code, not the
  diff hunk header), a one-to-two sentence explanation of the concrete
  failure, and a specific fix. Quote the offending line when it's short.
- Order findings by severity, then by file.
- If a pass produced nothing, say so in the pass summary — silence is
  ambiguous, "none found" is information.
- If the diff is large (>~800 lines), review it file by file, but still
  produce ONE merged report at the end, not per-file fragments.
- Do not edit any files. You are reviewing. If the user says "fix them",
  that is a new instruction — then fix in severity order, re-running the
  relevant pass after each fix.
- Never pad. If the change is clean, a two-line SHIP verdict is the correct
  and complete output.

---

From [Toolbay](https://toolbay.ai/product/claude-code-pr-reviewer). Free to use, modify, and share.
Keep this line and others can find it too.