← All skills

Reviewing Pull Requests

Reviews a pull request or diff for correctness bugs, missing tests and unclear naming, ranked by consequence rather than line order. Use when the user asks for a code review, shares a PR link or a diff, or asks whether a change is safe to merge.

Skill name

reviewing-pull-requests

Category

coding

Price

Free

Install

~/.claude/skills/reviewing-pull-requests/SKILL.md

Tags

code review, pull request, quality, git

Reviewing Pull Requests

A review is worth reading in proportion to how much of it the author would have caught anyway. Style, formatting and naming preferences are the cheap half. Lead with what breaks.

Read in this order

1. The description and linked issue — what was this meant to do?
2. The tests — what does the author believe the change guarantees?
3. The diff, largest file first
4. What the diff does not touch: callers, migrations, docs, feature flags

Reviewing in diff order is how reviewers end up commenting on a rename in file 1 and missing a swallowed exception in file 9.

What to look for, in order of consequence

Correctness. Off-by-one and boundary conditions. Null and empty cases. Error paths that swallow the error. Concurrency: two callers, interleaved. Data that crosses a trust boundary without validation. Anything that changes behaviour for existing rows or existing callers.

Blast radius. Is this reversible? What happens to work already in flight when it deploys? Does a migration lock a large table? Is there a flag?

Tests. Not coverage percentage — whether the test would fail if the change were wrong. A test that asserts the function was called is not a test of behaviour.

Interfaces. Names that will be wrong in a month. Parameters that only make sense if you know how the function is implemented. New public surface that did not need to be public.

Then style, structure and naming, briefly.

Writing the comment

State the failure, not the preference. Every finding needs the input that triggers it and the outcome:

blocking — parseRange returns null when start === end, and applyRange calls
.length on it directly. A single-character selection crashes the editor.
Suggest: return an empty range instead of null, and keep applyRange total.

That is actionable. "This could be cleaner" is not — it makes the author guess what you would accept, and they usually guess wrong.

Mark severity plainly so the author can triage:

• blocking — will break in production, or loses data
• should fix — real problem, does not have to be in this PR
• note — worth knowing, no action needed

If a finding is a matter of taste, say so and approve anyway.

Verify before you claim

Before writing a finding, trace it once more with a concrete input. A confident review comment about a bug that does not exist costs the author an hour and costs you the next review's credibility. If a claim cannot be traced to specific lines, either drop it or phrase it as a question.

Finishing

End with a verdict and a reason, in one line: approve, approve with comments, or request changes. A review with no verdict leaves the author waiting.