A Boring Code Review Checklist
- code review
- process
When I review a pull request, I have learned to delay having opinions about the interesting parts. Architecture, naming, library choices — all of that is worth discussing, but only after ten boring questions have been answered, because the boring questions catch the failures that actually page people.
The ten questions
- What does this diff do when the network is down? Any new outbound call is a future incident report. Timeouts, retries with jitter, and a sane behavior on total failure must be visible in the diff itself, not in the author's intentions.
- What happens if it runs twice? Idempotency is not just for cron. Queues redeliver, users double-click, deploys replay. If running the code twice is different from running it once, I want a comment saying why that is safe.
- What does it log? No secrets, no personal data, no request identifiers that correlate a user across services without a reason documented next to them. I have spent more review hours on this question than on any performance question in my career, and it has paid better.
- How does it fail under load? Not “does it stay fast” — “what degrades first, and does it degrade politely.” Unbounded queues, unbounded goroutines, and caches without limits are the three usual suspects.
- Is there a path back? Can the change be reverted without ceremony? Migrations that cannot be walked back are sometimes necessary; they are never casual. The diff should show which kind this is.
- Who gets paged? If this breaks at 3 a.m., does the alert route to someone who can fix it, and does the runbook sentence exist yet? The answer “we will add the runbook after launch” is how runbooks never happen.
- What does this make harder in the future? Every change forecloses some other change. Not a reason to refuse the diff — a reason to say out loud what is being foreclosed, so the next person inherits a decision instead of a mystery.
- Is the new flag defaulting safely? New behavior behind a default-off flag gets tested default-on before merge, not the week of rollout. The flag that has never been on is not a mitigation, it is a second untested code path.
- Does the test read like documentation? Test names that describe behavior, fixtures a stranger can follow, and at least one test for the failure mode from question one. Tests for the happy path alone are demos.
- Would I understand this diff in a year? Read the diff as if you had just been paged for it and it was written by someone who has since left. Anything you have to reconstruct from memory of the discussion is a future archaeology project. Comment now, in the code.
What this is not
This is not a style guide, and none of it is about correctness of the algorithm. Compilers are good at algorithms; they are useless at “this function will be called with the network partitioned during a deploy.” The boring questions exist precisely because they are the ones the machine cannot ask for you.
They are also deliberately cheap to answer. A diff that cannot answer all ten quickly is not a bad diff — it is a diff whose author and reviewer should have had a five-minute conversation before the review, and still can.