How to Review a Pull Request, in depth: one reviewer's session from triage to verdict, with reading order, local verification, risk hotspots, comments and re-review

What a review session has to produce

A review produces three things: a decision (approve, comment, request changes), a set of comments the author can act on without a meeting, and your own confidence that you understand what will change in production after the merge. The third is the one that is usually missing. If you cannot explain in two sentences what behaviour changes and how it could fail, you have not finished, however many comments you left.

It helps to be clear about what a review is not. It is not where design gets decided; if the approach is wrong, that conversation belongs in a design document before the code is written, as described in how to review a design doc. And it is not proofreading: formatters and linters should handle style.

Pass 1: triage in five minutes

Before reading any code, decide whether this pull request can be reviewed well, by you, today. Three questions decide it. How big is it? Past a few hundred changed lines, reviewers stop reading and start scanning. What is the risk tier? A copy change on a marketing page and a change to the payment ledger deserve very different amounts of attention. Are you the right reviewer? If the change touches the database schema and you have never worked on the data layer, say so and suggest someone who has, in addition to or instead of yourself.

The file list tells you most of this. Migrations, SQL, deployment configuration, CI workflows, authentication and cryptography are high-risk signals, whatever the line count says.

# Size and shape before reading anything
gh pr view 4812                      # title, description, linked issues, checks
gh pr diff 4812 --name-only          # which files, in which layers
gh pr diff 4812 | grep -c '^[+-]'    # rough count of changed lines

# Risk signals in the file list
gh pr diff 4812 --name-only | grep -E 'migrations/|\.sql$|config/|Dockerfile|\.github/workflows/|auth|crypto'

Triage can end the session. Asking the author to split a 1,500 line change into a refactor and a behaviour change is a legitimate review outcome, and it is far more useful than an approval nobody could honestly give. Say what you would accept: 'please split the rename into its own PR and I will review the logic change this afternoon.'

Pass 2: build context before you read the code

Read the description and the linked issue first, and try to answer: what problem is being solved, for whom, and how will we know it worked? If the description does not tell you, ask before reading further. Reviewing code against a guessed intent produces comments about the wrong thing.

Then do something that feels slow but saves time: spend two minutes sketching how you would have solved it. Which files would you have touched? Where would the new state live? What would you have tested? Now, when you read the diff, every difference between your sketch and the author's approach is either something you will learn from or something worth asking about.

Finally, look at the surrounding code that did not change. Use git log -p --follow <file> on the central file to see why it looks the way it does, and git blame on any line the change deletes.

Pass 3: a reading order that finds defects

Read in this order, not in the order the diff tool shows files:

1. Interfaces and data shapes. New function signatures, API schemas, database migrations, message formats and configuration keys. These are the expensive things to change later, and everything else depends on them.
2. Tests. They state what the author believes the code does. Read them as a specification: which cases are covered, which inputs are missing (empty, very large, concurrent, failing dependency), and whether the assertions would actually fail if the behaviour broke.
3. The core change. Usually one or two files hold the real logic. Read these line by line, tracing each new branch and asking what happens on the error path.
4. The periphery. Wiring, call sites, renames, generated files. Skim these for surprises: a call site that now ignores a return value, a default that changed.

Hold one question constant while reading: what happens when this fails? Most production incidents come from the unhappy path: the timeout nobody set, the retry that amplifies load, the exception swallowed and logged at debug level. Authors test the happy path; your advantage is attention to the others.

Pass 4: run it and try to break it

Reading code tells you what it says, and running it tells you what it does. For anything above the low-risk tier, check out the branch and run the relevant tests yourself. CI being green tells you the tests pass; it does not tell you the tests test anything.

The cheapest strong check is a deliberate mutation. Change the line the PR claims to fix back to something wrong and run the tests. If they still pass, the change is not covered, whatever the coverage report says.

gh pr checkout 4812                  # creates a local branch tracking the PR head
git branch pr-4812-v1                # bookmark the head you reviewed, for re-review later
make test                            # or your project's test command

# Deliberate mutation: break the line the PR claims to fix, confirm a test notices
git status --short                   # make sure you start clean
sed -i 's/attempt < max_attempts/attempt <= max_attempts/' webhooks/sender.py
make test TESTS=webhooks             # expect a failure; if everything passes, the tests do not cover it
git checkout -- webhooks/sender.py   # undo the mutation

For user-facing changes, click through the feature once. For migrations, run them up and down against a copy of realistic data, and check how long they hold locks on a large table. Ten minutes here often finds the issue that reading never would.

Risk hotspots: where to slow down

Some kinds of change fail far more often than others. When the diff contains one of these, raise your level of attention regardless of how small the change looks.

For changes written largely by a coding assistant, the hotspots are the same but some defect classes become more common, such as invented APIs and plausible but untested edge cases. Reviewing AI-generated code covers those in detail.

Worked example: retries for a webhook sender

A pull request titled 'Add retries to webhook delivery' changes 220 lines across webhooks/sender.py, a new backoff.py, a test file and a config file. Triage: medium size, high risk (it changes outbound traffic to customers), and the reviewer has worked on the sender before. Context: the issue says roughly 2% of deliveries fail on transient receiver errors and are dropped. The reviewer's sketch is exponential backoff with jitter, retries only for transient errors, a cap on total time, and a dead-letter queue at the end.

Reading the config first: max_attempts: 5, base_delay: 0.5, no total time budget. Reading the tests: they cover success on the third attempt and exhaustion after five attempts, but every failure in the tests is a 503. The core change: should_retry() returns true for any non-2xx status. That is the defect: a customer with a misconfigured endpoint returning 400 now receives five requests per event instead of one, and the system will dead-letter all of them anyway.

Verification: the reviewer checks out the branch, adds a test case for a 400 response, and confirms it is sent five times. They also notice delivery runs inside a worker with a 60 second job timeout, while each attempt has a 15 second request timeout, so five slow attempts take well over a minute so the job will be killed mid-retry. The review requests changes with two blocking comments.

Writing comments that get acted on

A good comment says what is wrong, why it matters and what would fix it, and makes clear how much it matters. A simple way to signal weight is the Conventional Comments convention: start each comment with a label such as issue, suggestion, question, nitpick or praise, optionally marked blocking or non-blocking. The author then knows which five comments stand between them and a merge, and which twenty are optional.

issue (blocking): retry loop also retries 4xx responses
A 400 from the customer endpoint will be sent 5 times and then dead-lettered,
so a misconfigured webhook generates 5x the traffic and an alert. Only 408, 429
and 5xx are worth retrying. Could we pass the status into should_retry()?

suggestion (non-blocking): extract the backoff calculation
```suggestion
delay = backoff_delay(attempt, base=0.5, cap=30.0)
```

question: is the 30 s cap aligned with the receiver's timeout?

nitpick: log key is `attempt_no` here and `attempt` in sender.py.

praise: the table-driven test for jitter bounds is easy to extend.

On GitHub, a fenced block tagged suggestion inside a review comment renders as a proposed change the author can apply with one click; use it for small, unambiguous fixes. Keep one issue per comment so threads can be resolved independently. Comment on the code, never the person: 'this loop retries client errors', not 'you forgot client errors'.

Choosing the verdict

Request changes when there is at least one blocking issue: a correctness bug, a security problem, a missing test for the core behaviour, or a risk the author has not addressed. Approve when nothing blocking remains, even if there are open nitpicks; 'approve with nits' trusts the author to handle small things and avoids another round. Use a plain comment when you reviewed only part of the change, and say which part. Write a short summary at the top of the review: what you checked, what you did not, and what blocks the merge.

Pass 6: re-review without starting over

When the author pushes a revision, do not re-read the whole branch. Read the replies on your open threads, then look only at what changed since your last pass. If the author added commits, the tool's 'changes since last review' view works. If they rebased or force-pushed, git range-diff compares the two versions of the commit series and shows how each commit changed.

# The author force-pushed after review. Compare what changed in their commits,
# not the whole branch again.
git fetch origin pull/4812/head:pr-4812-v2
git range-diff main pr-4812-v1 pr-4812-v2   # v1 = the branch you reviewed

# Then approve, comment or request changes from the CLI
gh pr review 4812 --approve --body "Retry classification fixed; nits addressed."
gh pr review 4812 --request-changes --body "See the 4xx retry thread."

Resolve threads only when the fix is actually in the code, not when the author has replied 'done'. Then check the fix did not add a new problem, especially in error handling.

How reviews fail

• Reading in diff order: attention is spent on the alphabetically first files, which are rarely the important ones.
• Nitpick floods: thirty style comments hide the one bug, and the author cannot tell what blocks the merge.
• Happy-path review: nobody asks what happens when the dependency times out.
• Trusting green CI: the tests pass because they never exercise the new branch.
• Endless rounds: new concerns raised on every revision instead of all at once in the first full pass.

What to do next

1. On your next review, write two sentences describing what will change in production before you approve. If you cannot, keep reviewing.
2. Sketch your own solution for two minutes before reading the diff.
3. Read interfaces and tests before implementation, and ask 'what happens when this fails?' on every new branch.
4. Check out one medium-risk PR this week and run a deliberate mutation against its tests.
5. Copy the risk hotspot table into your team's review template.
6. Start labelling comments as issue, suggestion, question or nitpick, with blocking or non-blocking.
7. Use git range-diff for the next force-pushed revision instead of re-reading the branch.