How to review a pull request well

A good pull request review protects the codebase without turning review into a personal argument or a slow approval queue. The reviewer checks correctness, maintainability, risk, and clarity, then gives feedback the author can act on.

Start with the purpose

Read the pull request title and description before reading the diff. Identify what the change claims to do, why it exists, and how it was tested. If the description does not answer those questions, ask for the missing context before guessing.

A useful description usually includes:

• the problem being solved
• the user visible or operator visible behaviour change
• the main implementation approach
• test evidence
• rollout or migration notes when needed
• known limitations

The description does not need to be long. It needs to make the review possible.

Check the shape of the change

Before reviewing line by line, scan the file list. Look for signals that the pull request may be too broad:

• unrelated formatting mixed with behaviour changes
• generated files mixed with source edits
• large renames mixed with logic changes
• changes across several domains without a clear reason
• tests missing for changed behaviour

When the shape is wrong, ask whether the change can be split. Smaller pull requests are easier to review and safer to merge.

Review one file at a time

A systematic review reduces missed details. Work through the changed files one by one. On GitHub you can mark a file as viewed to collapse it and track progress, and it is unmarked automatically if the file changes again. Leave comments on the smallest useful diff location so the author can see exactly what you mean.

For each file, ask:

• does this change match the stated purpose?
• is the behaviour correct for normal and edge cases?
• is error handling explicit enough?
• is the code understandable without private context?
• does the test coverage match the risk?
• does the change introduce avoidable coupling?
• are names accurate and consistent?

Do not spend most of the review on preferences that are already covered by formatting tools. Let automated checks handle mechanical rules where possible.

Test the reasoning, not only the syntax

Passing checks are necessary, but they are not a full review. Automated tests can miss the wrong requirement, a missing migration, an unsafe default, or a confusing operational behaviour.

Look for questions such as:

• what happens when the input is empty, malformed, slow, or duplicated?
• what happens when a dependency fails?
• does the change preserve backwards compatibility where it must?
• does it need a migration, feature flag, or staged rollout?
• can the change be observed in logs, metrics, or errors?
• does the failure mode protect data and security boundaries?

For a risky change, ask the author to add test evidence or explain why a test is not practical. Do not approve because the diff looks plausible.

Keep comments actionable

A review comment should make clear whether it is blocking, optional, or a question.

Use direct language:

• "Blocking: this returns success when the write fails. Please propagate the error."
• "Question: should this also handle archived records?"
• "Suggestion: this helper name could describe the unit it returns."

Avoid vague comments such as "not sure about this" or "can we make this better". They force the author to guess what standard they are being held to.

Separate correctness from preference

Not every improvement should block a merge. Blocking comments should be reserved for issues that affect correctness, security, maintainability, operability, or agreed project standards.

Preferences can still be useful, but label them as suggestions. A reviewer who blocks on personal style trains authors to optimise for the reviewer rather than the codebase.

If the project has a style rule, point to the rule or encode it in tooling. If it is not a rule, treat it as a suggestion.

Review tests as carefully as production code

Tests can hide weak assumptions. Check that tests fail for the right reason before the implementation is fixed, cover the behaviour that changed, and avoid overfitting to implementation details.

Good tests usually show:

• the important input or state
• the action being tested
• the expected result
• the relevant edge case

Be cautious with snapshot updates, broad mocks, and tests that assert private implementation details. They can create confidence without protecting behaviour.

Check security and data boundaries

Every review should include a short security pass. The depth depends on the change, but the reviewer should consider:

• input validation
• authorisation checks
• secret handling
• logging of sensitive data
• dependency trust
• permissions for new jobs or workflows
• database migrations and destructive operations

For high risk areas, require a subject matter reviewer or code owner. General approval is not a substitute for domain expertise.

Submit a clear review state

When you finish, submit a clear review state. GitHub offers three: approve submits your feedback and approves merging the change, request changes submits feedback that must be addressed before the pull request can be merged, and comment leaves general feedback without approving or blocking.

Approve when the change is ready to merge under the repository rules. Request changes when there is at least one blocking issue. Comment without approval when the review is incomplete or the questions are non-blocking. Note that request changes is informational unless a branch protection rule or ruleset enforces required reviews.

A useful summary says what you checked:

Reviewed the API behaviour, migration path, and tests. The error path for duplicate requests still needs a fix before merge.

This helps the author and later readers understand the scope of the review.

Respond well as an author

A good review process needs good author behaviour too. Reply to each comment, explain decisions, and push follow up commits that are easy to inspect. If you disagree, explain the trade off with evidence.

Do not mark a conversation resolved until the issue has been fixed or the reviewer agrees it does not need a change.

Conclusion

A strong pull request review is structured, evidence based, and respectful. Start with the purpose, inspect the shape, review the diff systematically, ask for tests where risk requires them, and make comments actionable. The goal is not to win the review. The goal is to merge a correct, maintainable change with a useful record of the decision.