You open a pull request with eleven comments on it. Four are about import order. Three suggest a different name for a variable. Two ask whether a class belongs in that package. One says the tests do not actually check the thing the ticket asked for.
Guess which one gets addressed last, if at all. The author works down the list, the easy ones go first, and by the time they reach the eleventh they have been in the diff for forty minutes and want it merged.
The comments that should not exist
Every one of those first ten was decidable. Import order is a formatter. Naming is a lint rule or it is a preference nobody should be spending review on. Whether a class belongs in a package is an architecture rule, and if you cannot write that rule, the boundary was never agreed in the first place.
If a machine can decide it and the machine has not been asked, the fix is to ask the machine, not to ask a person every time forever.
| the concern | who should decide it |
|---|---|
| formatting, import order | a formatter, applied automatically |
| naming conventions | a lint rule, or nobody |
| package boundaries | an architecture test |
| test coverage | a threshold in the build |
| whether tests assert anything | a mutation score |
| whether the change does what was asked | a person |
Only the last row needs judgement. Everything above it arrives at the review already answered, and answering it again is not thoroughness.
Say it out loud, in the instructions
This is easy to agree with and hard to hold, because a reviewer who spots something feels obliged to mention it. So it is worth writing down as an instruction rather than a value.
The reviewer in my own harness is told, in as many words, what it may not do:
## What you must not review
Formatting, import order, naming style, architecture boundaries, coverage
percentages, mutation score. All of those are already decided by gates, and a
gate has already said yes or the diff would not be in front of you.
Re-reviewing them is reviewer fatigue, it buries the one comment that mattered,
and it teaches people that review is noise.
If you believe a gate is wrong, say so as a separate note. Do not enforce it
again by hand.
That last paragraph is the release valve. Disagreeing with a gate is legitimate and common. Doing it silently, one comment at a time, on somebody else’s change, is how a rule ends up meaning two things.
The failure it exists to catch
With everything mechanical settled, one question is left, and it has two links that both have to hold: is there a scenario covering each requirement, and is there a test that asserts it?
The second link is the one that goes wrong quietly. A test that runs the code and
checks isNotNull on a method whose whole job is a boundary condition is
coverage without verification. It passes every gate in the build. A mutation
score catches some of it; a reader who knows what the ticket asked catches the
rest.
That is the review worth a person’s attention, and it is exactly the thing that gets crowded out by a comment about a blank line.
Two more that stay with the human
Scope. Anything in the diff no requirement asked for is a defect, even when it is an improvement. Especially then: an unrequested improvement is a change nobody planned, tested against, or can attribute later.
The seam. If a diff changes a request or a response shape, the contract should have changed first. A wire change that starts anywhere else is a defect whether or not it happens to work, for the same reason a spec and a contract test answer different questions.
What to do about it on Monday
Take your last ten reviews and sort the comments into the two columns above. Most teams find eight in one and two in the other, and the two are the ones that prevented a defect.
Then pick the largest bucket on the left and make it a gate. You will get the review time back immediately, and the next person to read a review comment will find one that was worth writing.
If your reviews are long and your defects still ship, that’s the work I do.