Code Review
Review a change for correctness first, quality second. A review is not a formatting pass — it's a
search for the ways this diff is wrong or will become wrong. Approve when you'd be comfortable
being paged for it at 3am.
Scope the review before reading line-by-line
Don't review the diff in isolation. A diff shows what changed, not whether the change is correct.
- Read the description / linked issue. What is this change claiming to do? You're verifying
the claim, not just the syntax.
- Read the surrounding code, not just the red/green lines. A removed null check looks fine in
the diff; whether it's a bug depends on the caller three functions up. Open the files.
- Reproduce the claim mentally (or actually). Trace one real input through the new path. If the
PR says "fixes the retry loop," follow a failing request through the loop and confirm it now
terminates. If you can run it, run it.
- Check the tests changed with the code. New behavior with no new test is a finding. A test
that passes against both the old and new code tests nothing.
What to actually look for
Go in this order — correctness bugs are worth more than style nits.
| Category |
Concretely |
| Logic bugs |
Off-by-one, inverted conditions, wrong operator (&& vs ||), = vs ==, swapped args, fallthrough. |
| Edge cases |
Empty list/string, zero, negative, null/None/undefined, single element, max size, duplicate keys, unicode. |
| Error handling |
Swallowed exceptions, ignored return/error values, partial failure leaving inconsistent state, no cleanup on the error path, error message that loses the cause. |
| Concurrency |
Shared mutable state without a lock, check-then-act (TOCTOU), non-atomic read-modify-write, missing await, deadlock-prone lock ordering, assuming callback order. |
| Resource safety |
Leaked file/socket/connection, missing close/defer/finally/context manager, unbounded growth (caches, queues). |
| API misuse |
Calling a function with wrong assumptions about its contract, ignoring documented preconditions, depending on undefined behavior or implementation details. |
| Boundaries / input |
Untrusted input used unvalidated (defer real security to a dedicated security review, but flag the obvious). |
| Tests |
New branch with no test, happy-path-only coverage, assertions that can't fail, flaky timing-dependent tests. |
| Performance |
N+1 queries, work inside a hot loop that could hoist out, accidental O(n²), a query with no index, loading a whole collection to count it. Flag only when it matters at expected scale. |
| Readability / reuse |
Duplicated logic that already exists elsewhere, a 6-deep nesting that an early return flattens, a name that lies about what the thing does. |
Severity model
Tag every finding so the author knows what blocks merge and what doesn't.
| Severity |
Meaning |
Blocks merge? |
| Blocker |
Correctness bug, data loss, security hole, breaks the build/tests. |
Yes |
| Major |
Likely-wrong under realistic conditions, missing error handling on a real failure path, missing test for new behavior. |
Usually |
| Minor |
Works, but harder to maintain, slightly off-pattern, small inefficiency. |
No |
| Nit |
Pure style/naming preference. Prefix with nit: so it's clearly optional. |
No |
If you can't articulate the failure mode, it's at most a Minor — don't inflate taste into a Blocker.
How to phrase feedback
A useful comment points at a line, names the failure, and proposes a fix. Three parts:
payment.go:142 (Blocker) — If charge() throws after markPaid() runs, the order is marked
paid but no charge exists; the next reconciliation will refund a payment that never happened.
Move markPaid() after the charge succeeds, or wrap both in a transaction.
- Point at
file:line so it's locatable.
- Explain the failure mode ("if X then Y goes wrong"), not just "this is wrong."
- Suggest the fix — even a rough direction. A finding with no path forward stalls the PR.
- Ask, don't assert, when unsure. "Is
items guaranteed non-empty here? If a caller can pass
[], this indexes out of bounds." — invites the author to confirm rather than starting a fight.
- Praise sparingly and specifically when a tricky thing is done well; it calibrates the rest.
Avoid: vague drive-bys ("this seems off"), restating the diff, bikeshedding naming on a Blocker-laden
PR, and demanding rewrites that don't change behavior.
Approve vs request changes
- Approve — no Blockers or Majors; remaining items are Minors/Nits you're happy to see deferred.
Approving with a couple of
nit:s the author can take or leave is normal and keeps things moving.
- Request changes — any Blocker, or a Major that materially risks correctness. Be explicit about
which findings gate the merge so the author isn't guessing.
- Comment (no verdict) — you have questions that change your assessment depending on the answer,
or you only skimmed and want to flag that.
Don't block a PR on preferences. Don't approve a PR you didn't actually understand — "LGTM" on a
diff you skimmed is how bugs ship.
Review checklist
On reviewing your own diff
Self-review before you send is worth doing and this checklist applies to it. It lowers what a
reviewer finds.
It does not make you the reviewer of record. The mistakes you cannot catch in your own diff are
the ones that come from your own model of the code — you will re-derive the same wrong assumption
reading it back, because it is the assumption that produced the line. That is precisely what a
second reader is for. Self-review reduces the defect rate; it does not discharge the independent
check.
See also
verify — the complementary skill. Use code-review when you have a diff and the
question is whether the change is any good; use verify when you have a claim and the question is
whether it is true, including claims with no diff to read (a backfill, a migration, a deploy).
1---
2name: code-review
3description: Review a diff or pull request for correctness and quality — logic bugs, edge cases, error handling, race conditions, missing tests, API misuse, performance, and readability. Use when reviewing a PR, auditing a change before merge, giving feedback on someone's code, or self-reviewing your own diff. Keywords code review, PR review, pull request, diff review, review my changes, approve, request changes.
4version: 1.0.0
5license: MIT
6---
7
8# Code Review
9
10Review a change for correctness first, quality second. A review is not a formatting pass — it's a
11search for the ways this diff is wrong or will become wrong. Approve when you'd be comfortable
12being paged for it at 3am.
13
14## Scope the review before reading line-by-line
15
16Don't review the diff in isolation. A diff shows what changed, not whether the change is correct.
17
181. **Read the description / linked issue.** What is this change *claiming* to do? You're verifying
19 the claim, not just the syntax.
202. **Read the surrounding code, not just the red/green lines.** A removed null check looks fine in
21 the diff; whether it's a bug depends on the caller three functions up. Open the files.
223. **Reproduce the claim mentally (or actually).** Trace one real input through the new path. If the
23 PR says "fixes the retry loop," follow a failing request through the loop and confirm it now
24 terminates. If you can run it, run it.
254. **Check the tests changed with the code.** New behavior with no new test is a finding. A test
26 that passes against *both* the old and new code tests nothing.
27
28## What to actually look for
29
30Go in this order — correctness bugs are worth more than style nits.
31
32| Category | Concretely |
33| --- | --- |
34| **Logic bugs** | Off-by-one, inverted conditions, wrong operator (`&&` vs `\|\|`), `=` vs `==`, swapped args, fallthrough. |
35| **Edge cases** | Empty list/string, zero, negative, `null`/`None`/`undefined`, single element, max size, duplicate keys, unicode. |
36| **Error handling** | Swallowed exceptions, ignored return/error values, partial failure leaving inconsistent state, no cleanup on the error path, error message that loses the cause. |
37| **Concurrency** | Shared mutable state without a lock, check-then-act (TOCTOU), non-atomic read-modify-write, missing `await`, deadlock-prone lock ordering, assuming callback order. |
38| **Resource safety** | Leaked file/socket/connection, missing `close`/`defer`/`finally`/context manager, unbounded growth (caches, queues). |
39| **API misuse** | Calling a function with wrong assumptions about its contract, ignoring documented preconditions, depending on undefined behavior or implementation details. |
40| **Boundaries / input** | Untrusted input used unvalidated (defer real security to a dedicated security review, but flag the obvious). |
41| **Tests** | New branch with no test, happy-path-only coverage, assertions that can't fail, flaky timing-dependent tests. |
42| **Performance** | N+1 queries, work inside a hot loop that could hoist out, accidental O(n²), a query with no index, loading a whole collection to count it. Flag only when it matters at expected scale. |
43| **Readability / reuse** | Duplicated logic that already exists elsewhere, a 6-deep nesting that an early return flattens, a name that lies about what the thing does. |
44
45## Severity model
46
47Tag every finding so the author knows what blocks merge and what doesn't.
48
49| Severity | Meaning | Blocks merge? |
50| --- | --- | --- |
51| **Blocker** | Correctness bug, data loss, security hole, breaks the build/tests. | Yes |
52| **Major** | Likely-wrong under realistic conditions, missing error handling on a real failure path, missing test for new behavior. | Usually |
53| **Minor** | Works, but harder to maintain, slightly off-pattern, small inefficiency. | No |
54| **Nit** | Pure style/naming preference. Prefix with `nit:` so it's clearly optional. | No |
55
56If you can't articulate the failure mode, it's at most a Minor — don't inflate taste into a Blocker.
57
58## How to phrase feedback
59
60A useful comment points at a line, names the failure, and proposes a fix. Three parts:
61
62> **`payment.go:142` (Blocker)** — If `charge()` throws after `markPaid()` runs, the order is marked
63> paid but no charge exists; the next reconciliation will refund a payment that never happened.
64> Move `markPaid()` after the charge succeeds, or wrap both in a transaction.
65
66- **Point at `file:line`** so it's locatable.
67- **Explain the failure mode** ("if X then Y goes wrong"), not just "this is wrong."
68- **Suggest the fix** — even a rough direction. A finding with no path forward stalls the PR.
69- **Ask, don't assert, when unsure.** "Is `items` guaranteed non-empty here? If a caller can pass
70 `[]`, this indexes out of bounds." — invites the author to confirm rather than starting a fight.
71- **Praise sparingly and specifically** when a tricky thing is done well; it calibrates the rest.
72
73Avoid: vague drive-bys ("this seems off"), restating the diff, bikeshedding naming on a Blocker-laden
74PR, and demanding rewrites that don't change behavior.
75
76## Approve vs request changes
77
78- **Approve** — no Blockers or Majors; remaining items are Minors/Nits you're happy to see deferred.
79 Approving with a couple of `nit:`s the author can take or leave is normal and keeps things moving.
80- **Request changes** — any Blocker, or a Major that materially risks correctness. Be explicit about
81 *which* findings gate the merge so the author isn't guessing.
82- **Comment (no verdict)** — you have questions that change your assessment depending on the answer,
83 or you only skimmed and want to flag that.
84
85Don't block a PR on preferences. Don't approve a PR you didn't actually understand — "LGTM" on a
86diff you skimmed is how bugs ship.
87
88## Review checklist
89
90- [ ] I read the description and know what the change claims to do
91- [ ] I read the surrounding code, not just the diff lines
92- [ ] I traced at least one real input through the new path
93- [ ] Edge cases covered: empty / null / zero / negative / max / duplicate
94- [ ] Every error/failure path leaves state consistent and is observable
95- [ ] No shared mutable state touched without synchronization
96- [ ] Resources are released on every path (success and error)
97- [ ] New/changed behavior has a test that fails against the old code
98- [ ] No obvious N+1, accidental quadratic, or unindexed hot query
99- [ ] No duplicated logic that already exists in the codebase
100- [ ] Every finding is tagged with a severity and names a failure mode
101- [ ] My verdict (approve / request changes) matches the findings
102
103## On reviewing your own diff
104
105Self-review before you send is worth doing and this checklist applies to it. It lowers what a
106reviewer finds.
107
108**It does not make you the reviewer of record.** The mistakes you cannot catch in your own diff are
109the ones that come from your own model of the code — you will re-derive the same wrong assumption
110reading it back, because it is the assumption that produced the line. That is precisely what a
111second reader is for. Self-review reduces the defect rate; it does not discharge the independent
112check.
113
114## See also
115
116[`verify`](../verify) — the complementary skill. Use `code-review` when you have a diff and the
117question is whether the change is any good; use `verify` when you have a claim and the question is
118whether it is true, including claims with no diff to read (a backfill, a migration, a deploy).
5dive-ai