pr-review

SKILL.md

---
name: pr-review
description: Review the current uncommitted or branch diff before it ships: pull the diff with git, read changed files for context, and report correctness bugs, missing error handling, security issues, and test gaps grouped by severity, each with file:line and a concrete fix. Use before committing, opening a PR, or when asked to "review my changes / diff / branch." Read-only by default.
allowed-tools: [execute_command, read_file, search_code, list_files]
triggers: ["review my diff", "review this PR", "review my changes before I push", "code review the branch", "check my uncommitted changes", "pre-commit review"]
version: 1.0.0
author: Codeep
---

You are reviewing a change before it ships. Default to READ-ONLY: report findings, do not edit files unless the user explicitly asks you to fix them.

## Steps
1. Find the diff. Run, in order, until one returns content:
   - `git diff --stat HEAD` and `git status --porcelain` to see scope. If there are staged + unstaged changes, review `git diff HEAD` (working tree + index vs last commit).
   - If the working tree is clean, this is a branch review: `git merge-base --fork-point origin/main HEAD || git merge-base origin/main HEAD` to get BASE, then `git diff BASE...HEAD`. Substitute `main`→`master`/`develop` if `git symbolic-ref refs/remotes/origin/HEAD` says so.
2. Get the full patch with context: `git diff <range> -U30 -- . ':(exclude)*.lock' ':(exclude)*-lock.json' ':(exclude)dist/*'`. Skip generated/vendored files (lockfiles, build output, `node_modules`, snapshots).
3. For each non-trivial changed file, `read_file` the whole file — not just the hunk. A diff hides callers, the function's error contract, and what the old code guaranteed. Use `search_code` to find every caller of a changed signature and whether the new behavior breaks them.
4. Review against the checklist below. For each finding capture: `path:line`, severity, the concrete risk, and a specific fix (ideally a 1-3 line patch).
5. Report grouped by severity, highest first. End with a one-line verdict: ship / ship-after-fixes / do-not-ship.

## What to look for
- **Correctness**: off-by-one, inverted conditions, wrong operator, await missing on a promise, mutated shared state, race conditions, `==` vs `===`, time-zone/locale assumptions, integer/float and money rounding.
- **Error handling**: unchecked return values, swallowed exceptions (`catch {}`), missing null/undefined guards, unhandled rejected promises, resources not closed (files/sockets/locks), partial-failure leaving inconsistent state.
- **Security**: injection (SQL/shell/path traversal), unsanitized user input reaching `exec`/`eval`/templates, secrets or tokens added to code/logs, missing authz checks, unsafe deserialization, overly broad CORS/permissions, dependency added without pinning.
- **Tests & gaps**: new branch/edge case with no test, changed behavior whose existing test still passes by luck, deleted assertions, flaky timing in tests. Note which specific case is uncovered.
- **Consistency**: matches surrounding code style and existing helpers; no dead code, leftover debug logs, `TODO`/`FIXME` shipped silently, or commented-out blocks.

## Severity rubric
- **Critical** — data loss, security hole, or a crash on a normal path. Must fix before merge.
- **High** — wrong result or unhandled failure on a realistic input.
- **Medium** — edge case, missing test, or fragile-but-currently-correct code.
- **Low / Nit** — style, naming, minor cleanup. Keep these brief.

## On failure
- Not a git repo or no diff at all: say so plainly and stop — nothing to review.
- Diff is huge (1000+ lines or many files): review the highest-risk files first (auth, payments, data writes, parsing of untrusted input), state what you skipped, and offer to continue.
- If a finding depends on runtime behavior you cannot verify statically, mark it "needs verification" rather than asserting it as a bug.

## Notes
- Distinguish what the diff *changed* from pre-existing issues; flag pre-existing problems separately and don't block the PR on them unless the change makes them worse.
- Don't invent line numbers — cite the actual `path:line` from the file you read.
- Be specific and actionable. "Consider error handling" is useless; "`fetchUser()` at api.ts:42 can reject and the caller has no catch — wrap in try/catch and return a 500" is a review.
- Only edit files if the user explicitly says "fix it" / "apply the fixes"; otherwise stay read-only.