ReviewGENERALSCALE-SPECIFICCONTESTED
What Code Review Is For
Correctness, design, maintainability, security and knowledge sharing — five things a person can do. Formatting is not one of them, because a tool already did it.
The requirement, the obvious build, and why it breaks
Every lesson starts where the work starts: someone asked for something, and the first implementation that comes to mind survives until the requirement changes.
The question
Once formatters, linters and tests have settled the mechanical questions, what is a human reviewer actually being asked to do?
The requirement
A team lead measures review and finds a median of eleven comments per pull request, of which nine are about naming, import order and whitespace. Cycle time is two days. She asks what review is supposed to produce.
The obvious build
Review is a quality gate. The reviewer reads every line looking for bugs, and the more comments a review produces the more value it delivered. Comment count is a reasonable proxy for rigour.
Why it breaks
People are not good at finding bugs by reading. The defects review reliably catches are the ones visible as a mismatch between the diff and the *intent* — and intent is not in the diff, which is why the reviewer who read the ticket finds things the careful line-reader does not.
How it breaks as requirements change
- People are not good at finding bugs by reading. The defects review reliably catches are the ones visible as a mismatch between the diff and the *intent* — and intent is not in the diff, which is why the reviewer who read the ticket finds things the careful line-reader does not.
- Comment count as a proxy rewards exactly the comments a tool should have made, because those are the easiest to produce and the easiest to count. A reviewer optimising for the metric will write nine nits and never ask what invariant moved.
- As the codebase grows, the questions that matter shift from "is this line right" to "does this knowledge belong here", and the line-by-line habit does not shift with them. The review that mattered in a 5,000-line codebase is the wrong review at 500,000.
- Formatting has infinite supply. Every diff contains style choices, so a review culture that permits style comments will never run out of them, and they crowd out the finite supply of design comments (What to Automate Out of Review).
- The knowledge-sharing purpose fails silently. Nothing about a fast approval tells you whether the reviewer now understands the subsystem, so a team can review diligently for two years and still have one person who can safely touch billing (Bus Factor).
Requirement→Constraints→Invariants→Responsibilities→Boundaries→Interfaces→State→Dependencies→Failure→Implementation→Tests→Feedback→Evolution
What limits the solution, and what must never stop being true
This domain leads with these two. A design that ignores its constraints is not a design, and an invariant nobody named is one nothing is protecting.
Constraints
- Reviewers are engineers with their own delivery commitments; review attention is taken out of build time, and there is no more of it available.
- The reviewer usually did not write the requirement and often has not read the ticket.
- CI already exists and already runs format, lint, types and tests — so anything a person says about those is duplicated work.
- The team is eight people across three subsystems, so for any given diff there are at most two people who know the surrounding code.
Invariants
- Nothing reaches the main branch without a second person having formed a view about what its behaviour changes.
- Every review comment is answerable: either the code changes, or the reason it does not is written down where the next reader will find it.
- A review approval refers to a specific diff. If the diff changes materially, the approval no longer applies.
Who owns what, and where the seams fall
Responsibilities decide boundaries; boundaries decide what an interface has to say.
Responsibilities
- The author owns making the change reviewable: a described intent, a diff small enough to hold in one head, and a green pipeline before a human is asked to look (Review Size).
- The reviewer owns forming a view on behaviour against the requirement, and on where the changed knowledge now lives.
- The pipeline owns everything mechanical, without exception. If a person is still saying it, it is a missing rule (What to Automate Out of Review).
- Nobody owns "reading every line". That is an activity, not a responsibility, and treating it as one is what produces the eleven-comment median.
Boundaries
- The line between tool and human is whether the answer depends on the requirement, the roadmap or the surrounding system. Anything decidable from the diff alone belongs to a tool.
- The line between review and design review is whether the boundary already exists. Once it does, review can criticise it but almost never move it (Review as Design Feedback — and Why It Arrives Too Late).
- The line between review and pairing is whether the knowledge is transferable in writing. Some subsystems are not, and forcing them through a PR thread produces approvals without understanding.
Five things a person can do, and one they should not
The useful test for any review comment is: could this have been decided from the diff alone? If yes, a tool should be deciding it, permanently, and the fact that a person is saying it means a rule is missing.
What remains is the set of questions that need something the diff does not contain — the requirement, the surrounding system, the roadmap, or another human being who will have to work on this next year.
- The first two rows are what review is actually for. The next three are why it survives despite being slow.
- The last row is not a small waste. It is the row that consumes the budget the others need, because it is the easiest to produce and the only one that never runs out.
- A team can measure this in an afternoon by classifying the comments on twenty merged PRs, and the result is usually a surprise (A Review Checklist Worth Reading).
| What review is for | The question the reviewer asks | Could a tool do it? | What it costs when skipped |
|---|---|---|---|
| Correctness | Does this do what was actually asked, including the cases the ticket did not mention? | No — the tool has not read the ticket | Behaviour nobody asked for, shipped confidently, with tests that assert it |
| Design | Which module owns this knowledge now, and is that where the next change will land? | No — it has no roadmap | A boundary in the wrong place, discovered two quarters later as a change that touches nine files (Change Amplification) |
| Maintainability | Will the next person understand this without asking the author? | Partly — metrics hint at it, they do not judge it | A subsystem only its author can change, which nobody notices until the author leaves |
| Security | Does this change what crosses a trust boundary, or who is trusted on the far side? | Partly — scanners match known shapes, not new authorization holes | An authorization gap no scanner has a rule for (Trust Boundaries) |
| Knowledge sharing | Does a second person now know this exists, and why it is the way it is? | No | One person per subsystem, and no route back in (Knowledge Sharing) |
| Formatting and style | Is the import order right? | Yes, entirely | Nothing — the formatter already did it. Attention spent here is gone before the five rows above are reached |
The comment only a person could have made
The difference between these two reviews is not effort or care. Both took about the same time. The difference is that one of them required having read the requirement and knowing something about the system that is not in the diff.
Notice that the second comment does not say the code is wrong. It asks a question about intent and then states a consequence — and both parts are things the author can answer with facts.
Two reviews of the same pull request
Seventeen comments a tool should have made
nit: use const here nit: import order nit: prefer an arrow function missing trailing comma nit: this could be a ternary typo in comment ... (11 more) Author fixes them in four minutes. Approved. Cycle time: two days.
Two comments requiring context outside the diff
"The ticket says cancellation takes effect at period end. This cancels immediately and refunds pro-rata, which is the old behaviour. Deliberate?" "refund() is now called inside the same transaction as the status update. If the provider times out we roll back the cancellation, but the refund may already have been accepted on their side — so we'd have refunded an active subscription."
The first set is a formatter that has not been configured; every one of those comments is a missing line in a config file, paid for in human attention and two days of cycle time. The second required reading the ticket and knowing that the payment provider sits outside the transaction boundary — neither of which appears anywhere in the diff, and neither of which any tool has access to (External Calls Inside a Transaction in Backend).
What the reviewer is actually holding
It is worth being precise about the reviewer as a unit in the system, because the interesting thing is how many distinct reasons they have to change what they do — and only one of those reasons is review.
responsibilitiesReviewerThe reviewer of a pull request
Knows
- — The requirement the change claims to satisfy — if it was written down
- — The invariants of the modules the diff touches, roughly, from having worked in them
- — What sits outside the diff that this change now depends on: a transaction boundary, a provider timeout, a shared table
Does
- — Forms a view on whether the behaviour matches what was asked
- — Names where the changed knowledge now lives, and whether the next change lands there
- — Records the reasoning in a place that survives the merge
Depends on
- — A pipeline that has already settled format, types, lint and tests (What to Automate Out of Review)
- — A diff small enough to hold in one head (Review Size)
- — A written intent — the ticket, or a PR description that stands in for it
Changes when — 4 distinct reasons
- — The requirement is ambiguous, so the reviewer quietly becomes the person deciding what the feature is
- — CI is slow or flaky, so the reviewer re-checks mechanical things by hand
- — The author is the only person who understands the subsystem, so review becomes a briefing with an approval attached
- — A release deadline is close, so review becomes a countersignature
Four distinct reasons to change, and only the first is review. The other three are review silently absorbing work that failed somewhere else — an unwritten requirement, an untrusted pipeline, a bus factor of one — and each one converts review into a different activity while leaving the name unchanged. When a team says review is not working, the useful question is which of these four it has become.
How to build it
Most important first.
- Automate the mechanical layer first, and completely. Until formatting, imports, unused code and type errors are impossible to comment on, nothing else in this lesson is reachable — the attention is already spent (What to Automate Out of Review).
- Give the reviewer the intent. A PR description that names the requirement and what it changes about behaviour is worth more than any checklist, because the reviewer's unique capability is comparing the two.
- Ask the five questions that need a person: does the behaviour match what was asked; what invariant moved and who protects it now; what new failure modes exist; is the design simpler than the alternatives; would the tests fail if the behaviour were wrong (A Review Checklist Worth Reading).
- Treat security as a specific question about trust boundaries rather than a general vigilance — "does this change what crosses a boundary, and who is trusted on the other side" (Trust Boundaries).
- Route reviews deliberately for knowledge, not only for expertise. The person who knows the subsystem least is often the reviewer who produces the most value, at the cost of a slower review (Knowledge Sharing).
- Make review comments say what they cost. A comment that names a consequence can be accepted or refuted; one that says "cleaner" can only be obeyed or resented (Changeability Is the Goal).
What the next change costs
The field this whole domain exists for. A structure is only better if it makes the change after this one cheaper — and it is worth saying which changes it does not help.
Cost of the next change
- Under review-as-bug-hunt: the next change costs a two-day wait, nine nit comments and one round of fixes. The design questions are not asked, so the cost of the change *after* that one is unaffected by the review — which is the whole problem. Review is consuming time without changing the cost curve.
- Under review-with-the-mechanical-layer-automated: the next change costs a shorter wait and a smaller number of comments, of which one or two are about where knowledge landed. Those comments are the only part of review that touches future change cost.
- The cost of the change after this one is decided mostly by design questions that review is structurally bad at raising, because it sees the boundary only after it is built. Moving those questions earlier is a separate investment (Review as Design Feedback — and Why It Arrives Too Late).
- What does not get cheaper: routing reviews for knowledge sharing makes every individual review slower, permanently, in exchange for a bus-factor property that never shows up in any cycle-time number.
What the recommended approach costs
- Review is a serialisation point. Every change waits for a human, and that wait is often longer than the work — teams that value flow above all else genuinely do ship faster without it, and pay for it elsewhere.
- Routing for knowledge sharing trades cycle time for bus factor, and the cost is immediate and measurable while the benefit is delayed and invisible. This asymmetry is why almost every team drifts back to expert-only review.
- Insisting that design comments name a consequence slows down experienced reviewers whose instincts are usually right, and some of that friction is genuinely wasted (Changeability Is the Goal).
What can go wrong
Failure modes
- Review becomes a formality: approved in ninety seconds, no comments, and everyone believes the change was reviewed. This is the dominant failure and it is invisible, because its output is identical to a good review of a good change.
- Review becomes an interrogation: every design choice must be justified from first principles, cycle time triples, and authors start batching changes into bigger PRs to reduce the number of reviews — which makes review worse (Review Size).
- The mitigation fails too. Adding a checklist to a formality-review produces a ticked checklist, not attention, and now the team has evidence that the review happened (A Review Checklist Worth Reading).
- Security is treated as everybody's vigilance and therefore nobody's question, so authorization changes pass through review with the same scrutiny as a copy edit (Least Privilege as a Design Decision).
- Knowledge sharing is assumed rather than checked. The subsystem owner reviews every change to their subsystem, which is fast and correct and steadily raises the bus factor problem it looks like it is solving (Code Ownership).
Dependencies, and their direction
- Review depends on CI being both fast and trusted. A slow pipeline pushes the mechanical work back onto humans; a flaky one makes green meaningless and pushes it back the same way.
- It depends on the requirement existing in writing. Where it does not, the reviewer silently becomes the person who decides what the feature is, which is a large responsibility acquired by accident.
- It depends on the diff, and only on the diff. Everything the change couples to outside the diff is invisible unless the author says so — which is why "this now calls the payment provider inside a transaction" is a comment only someone with system context can make.
- Review does not depend on seniority, and a process that assumes it does converts every disagreement into a status question (Tone, Disagreement and Receiving Review).
Misreads
- "So style does not matter." Consistent style matters a lot; it is just settled by configuration once rather than by argument every time. The claim is about who decides, not about whether it matters (What to Automate Out of Review).
- "So review catches the bugs." It catches some, and it is not the primary defence — types, tests and production observability catch far more. Review's distinctive value is on the questions no automated thing has the context to ask.
- "More reviewers means better review." Adding reviewers diffuses responsibility; two people who each assume the other read it carefully is a common and well-documented shape. One reviewer with the context beats three without it.
- "Approval means the reviewer is now responsible for the code." Approval means a second person formed a view. Making the reviewer a co-owner of every defect produces defensive, slow, low-value review, because the rational response is to demand changes rather than to think.
- "We can skip review if the tests are good." Tests assert the behaviour you thought of. The reviewer's value is on the behaviour nobody thought of, which by definition has no test.
Smells this explains
- shotgun-surgery
Testing it, and how it ages
What to test, and at which boundary
- Test the pipeline, not the reviewers: if a deliberately misformatted, type-broken, lint-violating branch reaches a human, the mechanical layer is not doing its job.
- The useful audit is to sample merged PRs and classify the comments — mechanical, behavioural, design, security, knowledge. A team that believes it reviews design and finds ninety per cent mechanical has learned something concrete (Testing as Design Feedback).
- For knowledge sharing the check is uncomfortable and simple: pick a subsystem, and ask who could safely make a change to it next week without the usual owner. Review that has been happening for two years should have changed that answer (Bus Factor).
How this design ages
- As a codebase ages, the marginal value of line-level review falls and the marginal value of boundary-level review rises — the bugs get caught by types and tests, and the expensive mistakes become structural. Review practice usually does not notice this transition.
- As a team grows, the informal version stops working: at five people everyone has context, at fifty the reviewer frequently has none, and the PR description stops being optional (RFCs).
- Better tooling steadily eats the bottom of the list. Type systems, static analysis and increasingly capable automated reviewers keep taking work off the human, and the correct response each time is to remove that comment class from the human review rather than keep it as a double-check.
- What forces a rethink: a serious incident traced to a change that was reviewed and approved. The instinct is to add checklist items; the useful response is to ask which of the five purposes was structurally unable to catch it.
Where this applies
This domain's advice is contested more than most. These labels say what each claim is specific to — and where CONTESTED appears, the note gives the strongest form of the opposing view rather than a caricature.
- GENERALThe split between what a tool can decide from the diff alone and what needs the requirement holds regardless of language or stack; only the position of the line moves as tooling improves.
- SCALE-SPECIFICAt three engineers who all have full context, review is closer to a conversation and the knowledge-sharing purpose is already satisfied by proximity; at fifty, the reviewer routinely has no context and the PR description carries the entire intent, so the practices that felt like bureaucracy at three become the only thing that works.
- CONTESTEDThe strongest opposing view: mandatory pre-merge review is a queue that costs more than it returns, and teams that pair-program or practise continuous review with post-merge auditing ship faster with no measurable defect penalty. That position is held by serious practitioners with real data behind it, and the honest answer is that pre-merge review is one mechanism for these five purposes rather than the only one — the purposes are the point, not the pull request.
Where the depth lives
This domain teaches the codebase-level structure and hands the rest off.
Securitysecurity-review
Domains that do not exist yet
- — Testing & Reliability Engineering — the claim that review is not the primary bug-catching mechanism only holds where types, tests and production observability are doing their share; where they are not, review is asked to be a defence it was never good at.