Serguey Asael Shinder: A code review that only reads the diff cannot see what the change breaks
Every review tool in use shows you the same thing: the lines that changed, with a little context above and below. That framing is so normal that it stops looking like a choice. It is one, and it decides what reviews can catch.
A diff answers what did the author write. Almost every expensive defect I have shipped or reviewed answered a different question: what did this change mean somewhere that is not in the diff.
Three shapes, all invisible in a patch
A method loosens a precondition. A validation moves from the top of a method into the one caller that needed it relaxed. The diff is two lines and reads fine. The other eleven callers now pass unvalidated input into code written when validation was guaranteed. Nothing in the patch mentions them.
A field gains a meaning. status had three values, now it has four. The switch in the diff handles all four. The four other switches on status across the codebase do not, and three of them have a default branch that quietly does the wrong thing instead of throwing.
A timeout changes. A client's read timeout goes from 2s to 30s because one slow endpoint needed it. Correct in the diff, correct in the ticket. Upstream, a thread pool that used to recycle a worker every two seconds now holds it for thirty, and the pool sizing that was derived from the old number is nowhere near the change.
In all three the patch is defensible line by line. The failure is a relationship between the patch and code the reviewer was never shown.
What I ask for instead
I have stopped trying to read patches harder. The questions that actually find these are mechanical, and they work on anyone's code:
- "Show me the callers." Not all of them — the count, and the two that look least like the one being changed. If the author has not looked, the review has not happened yet.
- "What was true before that is not true now?" Phrased as an invariant, in one sentence. If nobody can state it, the change is bigger than the diff.
- "Which number here came from another number?" Timeouts, pool sizes, batch sizes and retry counts are almost never independent. Changing one in isolation is how a system gets tuned into a corner.
None of that requires a tool. It requires treating the diff as the evidence rather than the subject.
The part that is uncomfortable
This makes reviews slower, and slower reviews are unpopular for good reasons — a patch sitting for two days has its own costs, and I am not arguing for ceremony.
But there is an honest trade here and it should be named: a fast review that only reads the diff is a check that the author wrote what they meant to write. That is worth something. It is not a check that the system still works, and the two get reported with the same green tick.
Approving a diff is not the same act as accepting a change. Most of the arguments I have watched between reviewers and authors are really disagreements about which of those two they thought they were doing.