# Code Review > 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. - Skill: `5dive-ai/code-review` (Agent Skill) - Install (CLI): `npx skillmds@latest add 5dive-ai/code-review` - Raw SKILL.md: https://api.skillmd.com/api/skills/5dive-ai/code-review/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Integrations & APIs - License: MIT - Author: 5dive-ai (https://skillmd.com/u/5dive-ai) - Updated: 2026-09-09 - Page: https://skillmd.com/skills/5dive-ai/code-review --- # 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. 1. **Read the description / linked issue.** What is this change *claiming* to do? You're verifying the claim, not just the syntax. 2. **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. 3. **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. 4. **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 - [ ] I read the description and know what the change claims to do - [ ] I read the surrounding code, not just the diff lines - [ ] I traced at least one real input through the new path - [ ] Edge cases covered: empty / null / zero / negative / max / duplicate - [ ] Every error/failure path leaves state consistent and is observable - [ ] No shared mutable state touched without synchronization - [ ] Resources are released on every path (success and error) - [ ] New/changed behavior has a test that fails against the old code - [ ] No obvious N+1, accidental quadratic, or unindexed hot query - [ ] No duplicated logic that already exists in the codebase - [ ] Every finding is tagged with a severity and names a failure mode - [ ] My verdict (approve / request changes) matches the findings ## 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`](../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).