The reviews I have done badly were all the same review: I opened a diff without having decided what I was reading it for, then wrote whatever occurred to me, line by line. Six engineers merging into a shared branch turns that habit into a tax the whole team pays. This is what I decided a review is for, and — more usefully — what it is not for.
Three things I am checking
Correctness, which is not "is this good code" but "does this do what it claims under conditions the author was not sitting in when they wrote it". The author has run the happy path; the reviewer has the other one.
Boundaries — whether the change alters what the rest of the system is allowed to assume. The approval engine I own is entered through exactly one public gateway class; other modules call that and nothing else. A comment that says call the gateway, not the service behind it looks like a style note and is not one: it is the difference between one entry point and four. I wrote it a few times; then the boundary became a lint rule and nobody had to write it again.
The reader six months from now, who is usually the author and usually on-call. I am the first outside reader this code will ever have. If I have to reconstruct why a branch exists from the diff alone, so will the person holding the pager, at 3am, with a partner waiting.
What it is not for
It is not for rewriting the author's code in my head and then grading the distance. The pull is strong, especially with the title: I would have structured this differently, so here are eleven comments walking the author toward my structure. What comes out is code shaped like a compromise between two mental models and matching neither — and the person debugging it at 3am is usually the one whose model lost. Code I would not have written is allowed to exist if it is correct, inside its boundaries, and readable.
It is also not the place to relitigate the design. If the shape is wrong, no number of line comments fixes it, and forty notes steering a finished branch back toward a different shape is the most expensive way to hold a ten-minute conversation. So the strongest move a review can make is sometimes to end early: this is a design question, not a diff question, take it off the diff. When I find myself writing the same blocking comment on two branches, that is not a review problem — it is a design conversation I owed the team before either branch existed.
Every comment says whether it blocks
Each comment starts with a word and a colon. Blocking: something is wrong and I will not approve until it changes. Preference: I would have done it another way, take it or leave it, I will approve either way. Question: I do not understand this and the answer may be that I am wrong.
That prefix does more work than anything after it. Before I used it, authors treated everything I wrote as mandatory — I was the lead, and an unlabelled comment from a lead is an instruction — so people made changes they disagreed with, and some were worse than what they replaced. Three blocking comments and eleven preferences is a completely different message from fourteen comments, and the author cannot tell which they got unless I say so.
The rule I hold myself to: if I cannot name what breaks, it is not blocking. "This feels fragile" is a preference until I can write down the input that makes it fail.
Review against the repo, not against my habits
Every repository has opinions, and on a platform with roughly 150 supplier codes across about 90 integrations, those opinions are load-bearing. A supplier module is written against a fixed contract and registered by its code in one registry; there are exactly two documented offer patterns and no third. So the first review question on a new integration is which pattern is this, and does the code say so out loud — not whether I like the variable names. A convention I mildly dislike, applied consistently, beats my preferred style in one file: consistency is what lets someone open a module they have never read and be right about it.
When I do want the convention changed, it goes in its own merge request, with the argument written down. Smuggling house style into a feature review is how a repo ends up with three styles and no record of who chose them.
The four questions
Asked of every change that touches money, persisted state, or a supplier:
What happens on retry? Not "is it idempotent" as a checkbox — what concretely happens if this runs twice. The per-level SLA clocks in the approval engine are two delayed jobs used as pure clocks: a job that fires re-reads the database and decides from what it finds, so a stale fire is a no-op and nothing ever needs cancelling. That property exists because the question was asked before the code was written.
What happens when this returns empty? An empty result raises nothing, alerts nothing and looks exactly like an answer — a whole search region once returned zero hotels while every service in the chain reported 200.
Who else reads this state? A write here is a read somewhere I am not looking. Insurance rows have to agree with the flight legs and the order total across several services, and a change that is locally correct in one of them is the standard way that agreement breaks.
What does the failure look like from outside? Some suppliers answer HTTP 200 with an error inside the body — a permanent reminder that the transport's opinion of a call is worthless. If the failure path terminates in a documented code the caller can act on, the review is nearly finished.
What I can count
I have no review-time figure and no defect-rate figure. Nobody instrumented review before or after any of this, so there is no before-and-after to publish.
What is countable is what the reviews left behind. The approval engine carries 11 documented error codes, 7 flow diagrams, and 18 user stories written back from the code that already existed. Its module boundaries are enforced by lint rather than by me. Every one of those started as a comment I had to write twice — which is the only reliable signal I have found that a review comment should stop being a comment and become a rule.