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
1---2name: code-review-23description: 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.4license: MIT5---67# Code Review89Review a change for correctness first, quality second. A review is not a formatting pass — it's a10search for the ways this diff is wrong or will become wrong. Approve when you'd be comfortable11being paged for it at 3am.1213## Scope the review before reading line-by-line1415Don't review the diff in isolation. A diff shows what changed, not whether the change is correct.16171. **Read the description / linked issue.** What is this change *claiming* to do? You're verifying18 the claim, not just the syntax.192. **Read the surrounding code, not just the red/green lines.** A removed null check looks fine in20 the diff; whether it's a bug depends on the caller three functions up. Open the files.213. **Reproduce the claim mentally (or actually).** Trace one real input through the new path. If the22 PR says "fixes the retry loop," follow a failing request through the loop and confirm it now23 terminates. If you can run it, run it.244. **Check the tests changed with the code.** New behavior with no new test is a finding. A test25 that passes against *both* the old and new code tests nothing.2627## What to actually look for2829Go in this order — correctness bugs are worth more than style nits.3031| Category | Concretely |32| --- | --- |33| **Logic bugs** | Off-by-one, inverted conditions, wrong operator (`&&` vs `\|\|`), `=` vs `==`, swapped args, fallthrough. |34| **Edge cases** | Empty list/string, zero, negative, `null`/`None`/`undefined`, single element, max size, duplicate keys, unicode. |35| **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. |36| **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. |37| **Resource safety** | Leaked file/socket/connection, missing `close`/`defer`/`finally`/context manager, unbounded growth (caches, queues). |38| **API misuse** | Calling a function with wrong assumptions about its contract, ignoring documented preconditions, depending on undefined behavior or implementation details. |39| **Boundaries / input** | Untrusted input used unvalidated (defer real security to a dedicated security review, but flag the obvious). |40| **Tests** | New branch with no test, happy-path-only coverage, assertions that can't fail, flaky timing-dependent tests. |41| **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. |42| **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. |4344## Severity model4546Tag every finding so the author knows what blocks merge and what doesn't.4748| Severity | Meaning | Blocks merge? |49| --- | --- | --- |50| **Blocker** | Correctness bug, data loss, security hole, breaks the build/tests. | Yes |51| **Major** | Likely-wrong under realistic conditions, missing error handling on a real failure path, missing test for new behavior. | Usually |52| **Minor** | Works, but harder to maintain, slightly off-pattern, small inefficiency. | No |53| **Nit** | Pure style/naming preference. Prefix with `nit:` so it's clearly optional. | No |5455If you can't articulate the failure mode, it's at most a Minor — don't inflate taste into a Blocker.5657## How to phrase feedback5859A useful comment points at a line, names the failure, and proposes a fix. Three parts:6061> **`payment.go:142` (Blocker)** — If `charge()` throws after `markPaid()` runs, the order is marked62> paid but no charge exists; the next reconciliation will refund a payment that never happened.63> Move `markPaid()` after the charge succeeds, or wrap both in a transaction.6465- **Point at `file:line`** so it's locatable.66- **Explain the failure mode** ("if X then Y goes wrong"), not just "this is wrong."67- **Suggest the fix** — even a rough direction. A finding with no path forward stalls the PR.68- **Ask, don't assert, when unsure.** "Is `items` guaranteed non-empty here? If a caller can pass69 `[]`, this indexes out of bounds." — invites the author to confirm rather than starting a fight.70- **Praise sparingly and specifically** when a tricky thing is done well; it calibrates the rest.7172Avoid: vague drive-bys ("this seems off"), restating the diff, bikeshedding naming on a Blocker-laden73PR, and demanding rewrites that don't change behavior.7475## Approve vs request changes7677- **Approve** — no Blockers or Majors; remaining items are Minors/Nits you're happy to see deferred.78 Approving with a couple of `nit:`s the author can take or leave is normal and keeps things moving.79- **Request changes** — any Blocker, or a Major that materially risks correctness. Be explicit about80 *which* findings gate the merge so the author isn't guessing.81- **Comment (no verdict)** — you have questions that change your assessment depending on the answer,82 or you only skimmed and want to flag that.8384Don't block a PR on preferences. Don't approve a PR you didn't actually understand — "LGTM" on a85diff you skimmed is how bugs ship.8687## Review checklist8889- [ ] I read the description and know what the change claims to do90- [ ] I read the surrounding code, not just the diff lines91- [ ] I traced at least one real input through the new path92- [ ] Edge cases covered: empty / null / zero / negative / max / duplicate93- [ ] Every error/failure path leaves state consistent and is observable94- [ ] No shared mutable state touched without synchronization95- [ ] Resources are released on every path (success and error)96- [ ] New/changed behavior has a test that fails against the old code97- [ ] No obvious N+1, accidental quadratic, or unindexed hot query98- [ ] No duplicated logic that already exists in the codebase99- [ ] Every finding is tagged with a severity and names a failure mode100- [ ] My verdict (approve / request changes) matches the findings
5dive-ai