---
title: Reviewing a Pull Request
description: What a rigorous reviewer checks in a pull request, from first read to approval.
category: software
tags: [software]
version: "1.0"
updated: 2026-07-07
sources:
  - name: Google Engineering Practices — Code Review Guide
    url: https://google.github.io/eng-practices/review/reviewer/
  - name: SmartBear — Best Practices for Peer Code Review
    url: https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/
---

For reviewing someone else's pull request on a shared codebase. The
goal is not a perfect PR — it's a PR that makes the codebase better
than it was, reviewed fast enough that the author is still thinking
about it.

## Before you start

- [ ] Start the review within one business day
  Review latency, not review depth, is what makes teams route around
  the process. A stalled review costs more than most bugs.
- [ ] Read the PR description and linked issue before any code
  You are judging the diff against its intent. If you can't tell what
  the change is for, ask for a description — that's the whole review.
- [ ] Ask for a split if the diff is over ~400 lines
  Defect detection falls off sharply past 400 lines per review
  session. Two small PRs get two real reviews; one big PR gets a skim.
- [ ] Check out the branch and run it `{when: user-facing-change}`
  Screenshots show the happy path the author chose. Clicking around
  yourself finds the state they didn't.

## Correctness

- [ ] Trace the main path through the diff line by line `{essential}`
  Read the code, don't pattern-match it. Most approved-then-reverted
  PRs were skimmed, not read.
- [ ] Hunt the edge cases: empty, null, zero, negative, huge, duplicate, concurrent `{essential}`
  Ask of every input: what's the worst value this could hold?
- [ ] Follow every error path to its end
  What happens when the network call fails, the file is missing, the
  parse throws? Swallowed exceptions are bugs with a delay timer.
- [ ] Verify the tests would fail if the change were wrong
  For a bug fix, the new test must fail without the fix. A test that
  can't fail is worse than no test — it's false confidence.
- [ ] Check that tests cover the new behavior, not just the new lines
  Coverage tools count lines executed, not outcomes asserted.
- [ ] Check that input crossing a trust boundary is validated `{essential}`
  Anything from a user, another service, or a file: injection, path
  traversal, unbounded size, unexpected type.
- [ ] Check authorization, not just authentication `{essential}`
  Verifying *who* the user is but not *whether they may touch this
  resource* is the most common access-control bug in web code.
- [ ] Scan the diff for secrets and credentials `{essential}`
  Keys, tokens, passwords, internal URLs. Once merged, they're in
  history forever and must be rotated, not just deleted.

## Design

- [ ] Flag anything unrelated to the PR's stated purpose
  Drive-by refactors hide bugs in the noise and complicate reverts.
  Ask for them as a separate PR — most authors happily agree.
- [ ] Check that names say what things actually are
  A function named `getUser` that also creates one is a future bug.
  Names are the API most readers will ever see.
- [ ] Question every new dependency
  Each one is a maintenance commitment and supply-chain surface. Could
  thirty lines of code do the same job?
- [ ] Verify the change follows the codebase's existing patterns
  A second way to do the same thing costs every future reader a
  decision. If the new pattern is better, that's a proposal, not a PR.
- [ ] Verify API and schema changes are backwards compatible `{when: public-api}` `{essential}`
  Old clients keep working; removals get a deprecation cycle. Renaming
  a JSON field is a breaking change no compiler will catch.
- [ ] Check the change can be debugged in production
  Enough logging and metrics that whoever is on call at 3 a.m. can
  tell what it's doing without adding print statements first.

## Before you approve

- [ ] Reread your comments and label the non-blocking ones
  Prefix preferences with "nit:" so the author knows what actually
  gates approval. Unlabeled comments all read as blocking.
- [ ] Confirm blocking comments are resolved, not just replied to
  "Will fix in a follow-up" is a resolution only if the follow-up
  ticket exists.
- [ ] Check the PR title and commit message describe the change
  They become the changelog and the answer `git blame` gives in three
  years. "Fix bug" answers nothing.
- [ ] Check docs and changelog are updated `{when: user-facing-change}`
- [ ] Approve only if the change improves overall code health
  Google's standard: not perfect, better. Don't hold approval hostage
  to your personal style — and don't approve code you wouldn't be
  willing to maintain.