Code Reviewer

Overview

A code review is not a spell-check. It is the last structured chance to catch bugs, design drift, and security holes before they ship — and the best mentoring surface a team has. This skill turns review into a repeatable discipline: a layered checklist (correctness → design → readability → security), comment conventions that keep feedback actionable and kind, and a decision rule for when to approve, request changes, or block.

Good reviewers optimize for throughput with safety: catch what matters, don't relitigate settled style, and leave the codebase — and the author — better than you found them.

When to use

• Reviewing a pull request or merge request before approving.
• Reviewing your own diff before requesting review (self-review first).
• Auditing generated or AI-assisted code before merging it.
• Onboarding reviewers: giving a team a shared review standard.
• Post-incident: reviewing a hotfix under time pressure without skipping safety.

Core concepts

• Layered review order. Check in this order and stop demanding re-reviews when lower layers fail:
  1. Correctness — does it do what it claims? Edge cases, nulls, off-by-ones, error paths.
  2. Design — does it fit the architecture? Abstractions, boundaries, naming, duplication.
  3. Readability — can the next person understand it in one pass? Comments where non-obvious.
  4. Security — injection, authz checks, secrets, unsafe deserialization.
  5. Performance — only when there's evidence of a hotspot (N+1 queries, unbounded loops).
• Comment taxonomy. Use consistent prefixes so the author knows what action is expected:
  • nit: — style preference, optional to take.
  • question: — genuine uncertainty, not a veiled criticism.
  • suggestion: — a concrete better approach, ideally with code.
  • blocking: — must change before merge (bug, security, broken contract).
• Approval stances. Approve (ship it), Approve with comments (nits only, no re-review), Request changes (blocking issues, re-review needed), Hold (needs design discussion first).
• Review your own diff first. Read the diff in the PR tool, not the files — you'll see it the way reviewers do and catch leftover debug code, accidental files, and weak commit messages.
• Time-box. Reviews older than 24 hours rot. Aim for a first response within a working day; large PRs should have been small PRs (send that feedback early, then review what's in front of you).

Practical workflow

1. Get context. Read the PR description, linked issue, and test plan before the diff.
2. Scan the diff top-to-bottom once for shape: files touched, size, test coverage ratio.
3. Correctness pass. For each changed function ask:
   • What are the invalid inputs? Are they rejected or handled?
   • What happens on failure — does the error propagate to somewhere that handles it?
   • Are resources (files, connections, locks) released on every path?
4. Design pass. Check boundaries: does this layer know too much about another? Is there a simpler way that doesn't add an abstraction? Would a future maintainer find the seams?
5. Security pass. Trace every external input to its sink. Verify authorization is checked server-side, not just hidden in the UI. Look for secrets, tokens, or PII in logs and responses.
6. Leave comments using the taxonomy; batch related comments; always pair a blocking: with the reason and, when possible, the fix.
7. Decide the stance and state it explicitly, including what (if anything) needs re-review.

Quick self-review checklist before requesting review:

[ ] Diff contains only intended files (no debug prints, no stray assets)
[ ] Tests cover the new behavior, including at least one failure case
[ ] Public APIs/naming reviewed against project conventions
[ ] No secrets, credentials, or internal URLs committed
[ ] PR description explains WHY, not just WHAT
[ ] Screenshots/logs attached for UI or behavior changes

Common pitfalls

• Style bikeshedding. If a linter can enforce it, the linter should — never block a PR over formatting a tool could fix. Put the rule in the formatter config and move on.
• Reviewing the author, not the code. "You always…" is never useful. Comment on the diff.
• Rubber-stamping large PRs. A 2,000-line diff approved in 10 minutes is a signature, not a review. Push back on size early; review incrementally with stacked PRs.
• Demanding perfection. The bar is "safe to merge and maintain," not "the way I would have written it." Alternative approaches that are equally good are nit: at most.
• Security theater. "Use HTTPS" on an internal health check, or flagging a theoretical timing attack while missing an unauthenticated admin endpoint. Prioritize realistic threat paths.
• Drive-by redesigns. If the design is wrong at the architectural level, don't leave 40 comments on the implementation — Hold, explain the design concern, and talk it through.
• Ghost reviews. Requesting changes and disappearing stalls the author. Own the re-review and respond as fast as you did the first time.