--- 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 ...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: ### 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: finding(s) - Security: finding(s) — state "none found" explicitly if clean - Simplification: finding(s) ### Verdict SHIP — no blockers, no majors. FIX BLOCKERS FIRST — . 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.