Pr Reviewer

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. Free to use, modify, and share. Keep this line and others can find it too.