pr-code-review-checklist
verified04644a57-8ec8-4f9f-8b02-aa59ddcd630c
Review a pull request systematically — correctness, security, tests, docs, and scope — with a concrete checklist. Use when asked to review a PR or as the pre-merge gate.
Metadata
Skill file
# PR Code Review Checklist
Use when reviewing a pull request, or as a pre-merge gate. Review in a fixed order
so substance (correctness, security) is judged before style, and nothing is skipped.
## 1. Fetch and orient
```bash
gh pr checkout <number> # GitHub CLI
# or: git fetch origin pull/<n>/head && git checkout FETCH_HEAD
gh pr diff <number> # read the actual diff
```
Read the PR description first. If the description does not state what the change
does and why, that is itself a review finding (often the first one).
## 2. The 5-pass review order
Review in this order; stop and comment as you go, but do not reorder — style last.
### Pass 1 — Correctness
- [ ] Does it do what the description claims?
- [ ] Edge cases: empty input, `null`, zero, negative, max-size, duplicates?
- [ ] Off-by-one errors, inverted conditions, wrong variable in a loop?
- [ ] Race conditions (read-modify-write without a lock/transaction)?
- [ ] Error paths: are failures handled, or silently swallowed?
- [ ] Concurrency: shared mutable state, non-atomic sequences?
### Pass 2 — Security
- [ ] SQL injection (string-built queries, unescaped interpolation)?
- [ ] XSS (untrusted data into HTML/templates without escaping)?
- [ ] Secrets hardcoded or committed (API keys, tokens, passwords)?
- [ ] Missing authn/authz on new endpoints or actions?
- [ ] SSRF / open redirect / path traversal on user-controlled input?
- [ ] Unsafe deserialization (`pickle`, `eval`, `yaml.load`)?
### Pass 3 — Tests
- [ ] Is there a test for the new behavior (not just the happy path)?
- [ ] Do tests actually assert something (no assertion-less tests)?
- [ ] Is there a regression test for the bug being fixed?
- [ ] Are existing tests passing; does the PR break anything?
### Pass 4 — Docs & messages
- [ ] Outdated comments that now lie about the code?
- [ ] Commit messages / PR description match the change?
- [ ] Public API, README, or changelog updated where behavior changed?
- [ ] Error messages / log lines clear and non-cryptic?
### Pass 5 — Scope & consistency
- [ ] Unrelated changes smuggled in (reformatting, drive-by edits)?
- [ ] Consistent with existing patterns/naming in the codebase?
- [ ] Duplicated code that already exists elsewhere?
- [ ] Dead code, debug prints, TODOs left behind?
## 3. Leaving actionable comments
Every comment has four parts; missing any makes it hard to act on:
```text
[severity] On <file>:<line> — <the problem, factually>
Suggestion: <the concrete fix or alternative>
```
Example:
```text
[blocking] src/auth.py:42 — the token is compared with `==` (timing attack).
Suggestion: use `hmac.compare_digest(token, expected)` for constant-time compare.
```
Use the severity convention from `review-comment-severity-triage`:
`[blocking]`, `[should-fix]`, `[nit]`.
## 4. Your verdict
End with an explicit decision, not just comments:
| Verdict | Meaning |
|---|---|
| **Approve** | Ready to merge as-is |
| **Request changes** | Blockers exist; must be fixed |
| **Comment** | No blockers, but non-binding suggestions |
Approve only when every `[blocking]` is resolved and re-verified.
## 5. Reviewing large PRs
When a PR is too big to review in one sitting, do not skim it — either split it
(see diff-splitting-for-review) or review it in logical chunks:
```text
1. Review commit-by-commit (git log --oneline on the branch), not the whole diff
at once. Each commit should be one reviewable idea.
2. If commits are not reviewable, ask the author to split before you review —
reviewing a 2000-line blob in one pass guarantees missed bugs.
3. Track what you have reviewed so you do not re-read or skip sections.
```
A review of a too-large PR is unreliable *by construction*. Flag the size as a
`[should-fix]` (or `[blocking]` if it is genuinely un-reviewable) and request a
split.
## 6. The review comment format (reference)
```
[severity] path:line — factual statement of the problem
Suggestion: the concrete fix
```
See review-comment-severity-triage for the full severity rubric (`[blocking]`,
`[should-fix]`, `[nit]`). Severity is not optional — every comment carries one.
## Guardrails
- Do **not** review style before substance. A beautifully formatted security hole
is still a security hole.
- Do **not** rubber-stamp ("LGTM") without doing the passes. Every pass missed is
a class of bug you allowed through.
- Do **not** comment on "I would have written it differently" when the current
code is correct — that is noise, not review.
- Do **not** approve with open `[blocking]` comments.
- Do verify fixes when the author addresses your comments — re-read the changed
hunk, do not trust the "fixed" label.
## Pitfalls
- **Style-first review** — spending the whole review on naming/formatting and
missing the logic bug.
- **Rubber-stamping** — approving because the author is senior or the diff is small.
- **Commenting on preference, not correctness** — "I'd use a list comprehension
here" on correct code is noise and trains authors to ignore you.
- **Not re-verifying fixes** — the author marks a blocker resolved but the new code
still has the bug.
- **Missing scope creep** — unrelated changes hiding in a large diff.
## Verify / Checklist
- [ ] Read the PR description and the actual diff (`gh pr diff`).
- [ ] All 5 passes done in order: correctness, security, tests, docs, scope.
- [ ] Every comment has severity + file:line + problem + suggestion.
- [ ] No `[blocking]` comments remain open at verdict time.
- [ ] An explicit verdict (approve / request changes / comment) was given.
- [ ] Author's fixes were re-verified in the diff, not taken on faith.Attached files
No attached files.