Every pull request review ends in one of three verdicts, and most reviewers pick between them by mood. Knowing when to request changes in a code review, versus approving with your comments attached, is one of the highest-leverage judgment calls a reviewer makes, because the verdict, not the comments, decides whether the author’s day continues or stalls. The comments carry the information. The verdict carries the cost.
The short answer: Request changes only when merging would cause harm you can name: a correctness bug, a security hole, data loss, or a design decision that becomes expensive to reverse once code depends on it. Everything opinion-shaped goes into comments on an approval, trusting the author to handle them. Wrongly blocking costs trust and a day of velocity; wrongly approving usually costs a follow-up PR. Price the verdicts accordingly.
Three verdicts, three signals
- Approve: nothing further needed from me. Merge when ready.
- Approve with comments: I found things worth saying, and I trust you to handle them without another round trip.
- Request changes: merging now would cause concrete harm, and I need to look again before this ships.
These are messages about trust and process, not just quality scores. Google’s review guidance treats review latency as a first-order cost: respond within one business day at the outside, because slow reviews drain velocity and morale together. A request-changes verdict schedules at least one more full latency cycle. That is sometimes exactly right. It should never be an accident.
When to request changes in a code review
The rule that survives contact with real teams: block on correctness, security, or irreversibility, and nothing else. If you cannot finish the sentence “if this merges, X breaks,” “X leaks,” or “X becomes very hard to undo,” you do not have a blocking concern. You have a comment. Irreversibility deserves the emphasis: a badly named local variable is a thirty-second rename next week, while a badly designed public API or database schema accretes dependents the moment it lands. Block on the second kind, comment on the first, and be honest with yourself about which one you are looking at. A useful self-check: if the author shipped it exactly as written and you were on call, would you actually be paged, or merely annoyed? Paged is a block. Annoyed is a comment, however strongly you feel it.
Approve with comments is the default
Everything opinion-shaped lands here: naming, structure you would have chosen differently, style beyond what the linter enforces, the refactor that could ride along but does not have to. Marking severity explicitly makes this verdict work: Conventional Comments prefixes like nit: and suggestion: tell the author what is optional, so an approval with six notes reads as help rather than as a passive-aggressive block. The verdict is also a deal: you extend trust that the author will read and act in good faith, and authors who repeatedly merge without reading the notes are the reason the deal has to stay explicit.
The asymmetric cost of the wrong verdict
Wrongly approving usually costs a bug, a revert, or a follow-up PR: real but bounded, and mostly paid by the code. Wrongly blocking is paid by the person. It costs a latency cycle, teaches the author to route future PRs around you, and over time trains a team to fear review instead of using it. Both failure modes are real, and the asymmetry is not an argument for rubber-stamping: it is an argument for spending your blocks where they are unambiguous. A reviewer who blocks rarely, and only with a nameable harm attached, finds that nobody argues when they do.
Re-review the delta, not the PR
Etiquette after you request changes: when the author pushes fixes, review what changed since your last pass, not the whole PR from scratch. Confirm the blocking issues are resolved, resist discovering brand-new nitpicks on lines you already read, and answer with the same day-one latency you would want yourself. Nothing corrodes a review culture faster than a re-review that arrives three days late and raises objections that were visible in round one.
Say what you reviewed
Every verdict gets sharper with a scope statement: “reviewed the logic and the migration, skimmed the tests, did not look at the Terraform.” It makes an approval honest, tells the author what still needs eyes, and matters most on large PRs where nobody actually reviews everything. The verdict says whether the change can merge. The scope statement says what that verdict is worth. Reviewers who provide both are the ones whose approvals mean something, and whose rare blocks get taken seriously without a fight.
Frequently asked questions
When should I request changes on a pull request?
Block only when merging would cause harm you can name: a correctness bug, a security hole, data loss, or a design decision that will be expensive to reverse once other code depends on it. Everything else, naming, style, structure you would have done differently, belongs in comments on an approval. If you cannot name the concrete harm, do not block.
What does approve with comments actually mean?
It means: I trust you to address these notes without me re-checking. The author fixes what they agree with, replies where they differ, and merges without another round trip. It is the right default for opinion-shaped feedback because it delivers the signal without spending a day of latency on it. Reserve plain approval for changes with genuinely nothing to say.
How should re-review work after I requested changes?
Review the delta, not the whole PR again. Look at what changed since your last pass, confirm the blocking issues are resolved, and resist raising brand-new nitpicks on lines you already read once. Re-review should be fast by design: if the author addressed the blockers, dragging out extra rounds punishes them for responding to feedback.
