A Review Checklist Worth Reading
Six questions, in the order attention runs out: behaviour against requirement, invariants moved, failure modes added, simpler alternative, meaningful tests, and whether you could debug it at three in the morning.
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.
What should a reviewer actually ask, and in what order, so the expensive questions get asked before attention runs out?
After an incident caused by a change that was reviewed and approved, a team wants "a proper review checklist". Someone has already produced a twenty-two item list from a blog post.
Write down everything a good review looks at — twenty-two items covering naming, error handling, logging, tests, performance, security, documentation, accessibility and backwards compatibility — and require reviewers to work through it.
A twenty-two item list is used for two weeks and then ticked. This is not laziness; it is what any human does with a list longer than the attention available, and the ticked list is worse than no list because it produces evidence that review happened.
- A twenty-two item list is used for two weeks and then ticked. This is not laziness; it is what any human does with a list longer than the attention available, and the ticked list is worse than no list because it produces evidence that review happened.
- Order is doing more work than content. The first three questions get real thought and the rest get progressively less, so a list that opens with naming and documentation has spent the attention before it reaches invariants.
- Uniform lists misfire on non-uniform diffs. Applying "are the tests meaningful" to a copy change is noise, and noise on the easy changes is what trains reviewers to stop reading the list on the hard ones.
- Most of these lists ask about the code and not about the change. "Is error handling present" is answerable by looking; "what new failure modes does this introduce" requires thinking about the system, and only the second one would have caught the incident.
- Checklists are also a way of avoiding the argument. Converting a design disagreement into a list item makes it non-negotiable without anyone having to defend it (Changeability Is the Goal).
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.
- A reviewer has roughly fifteen to thirty minutes of genuine thought per change, and no more. Any list longer than that will be ticked rather than used.
- The reviewer does not have the author's context and cannot acquire it inside the review.
- The team does not want a process artefact; they want the questions that would have caught the incident.
- Different diffs need different questions — a config change, a schema migration and a new endpoint have almost nothing in common.
- Every question on the list must be answerable from the diff plus the ticket. A question requiring information the reviewer cannot get is a question that will be answered by guessing.
- A checklist must be able to fail. If every item can be ticked for every change, it is measuring nothing.
Who owns what, and where the seams fall
Responsibilities decide boundaries; boundaries decide what an interface has to say.
- The list owns being short enough to survive contact with a busy reviewer — six questions is already at the limit.
- The reviewer owns asking them in order and stopping honestly when attention runs out, rather than skimming the remainder.
- The author owns pre-answering the ones they can: a PR description that states the requirement, the invariant touched and the failure modes considered removes three questions from the reviewer's budget.
- The team owns keeping the list falsifiable — an item nobody has ever answered "no" to should be deleted.
- The boundary between checklist and judgement: a checklist can tell you which questions to ask. It cannot tell you whether the answer is good, and pretending otherwise is how it becomes a ritual.
- The boundary between this list and a security review or a migration review: high-blast-radius changes get an additional, specific list, because the general one is deliberately general (Designing the Migration).
- The questions are ordered by cost-if-missed, not by where they appear in the diff. That ordering is the design decision in this lesson.
The six, in the order attention runs out
The order is the design. Attention decays sharply through a review, so whichever question is asked first gets real thought and whichever is asked sixth gets a glance — which means the ordering decides what review is actually for far more than the content of the list does.
Each step below carries the way it fails, because every one of these questions has a degenerate form that looks like the real thing.
- 11. Behaviour vs requirement
Read the ticket, then the diff. Ask whether the code does what was asked, including the cases the ticket did not mention.
fails by Reading the diff first. You then reconstruct the requirement from the code, and the mismatch becomes literally invisible — this is the single most common way a reviewed change ships the wrong behaviour.
- 22. Which invariant moved
Name the rule that must stay true, and identify who enforces it after this change. New paths to the same state are the thing to look for.
fails by Accepting "the service validates it" without checking whether the new code path goes through the service (Invariant Leaks).
- 33. What can now fail
Enumerate what is new: a network call, a transaction boundary, a retry, a partial write, a background job. Each is a new state the system can occupy.
fails by Checking that errors are caught rather than asking what the system looks like when they happen. A swallowed error passes this question and fails production (Swallowed Errors).
- 44. Simpler alternative
Ask what the version with fewer moving parts would have looked like, and what the extra machinery buys.
fails by Turning into a design debate on a finished PR, which is the most expensive place to have it and the least likely to change anything (Review as Design Feedback — and Why It Arrives Too Late).
- 55. Would the tests fail?
Read the assertions. Ask whether they would go red if the behaviour were wrong — not whether tests exist.
fails by Counting tests. Coverage and test count are both satisfied by tests that cannot fail (Mocking).
- 66. Debuggable on a Sunday
Ask whether a failure here would be diagnosable from logs and metrics alone, by someone who has never seen this code.
fails by Being asked in a system with no observability, where the answer is always no and everyone learns to skip it (Debuggability by Design).
If attention runs out at question three, say so in the review rather than skimming four to six. "I read this for behaviour and invariants; I did not review the tests" is a useful thing for the next reader to know, and a false full approval is not.
Question five, which everybody skips
Reviewers skip the tests because the tests are long, boring and green. But a test file is the part of the diff where the author wrote down what they believe the code does, which makes it the highest-information part of the change and the place where a misunderstanding is most visible.
The specific thing to look for is an assertion whose expected value came out of the same test's own mocks. Those tests assert that the code calls the collaborator it obviously calls, and they survive any change to the actual rule.
1it('applies the discount', async () => {2 const discount = { apply: jest.fn().mockReturnValue(90) }3 const pricing = new Pricing(repo, tax, discount)4 5 const total = await pricing.total(order)6 7 expect(discount.apply).toHaveBeenCalledWith(order)8 expect(total).toBe(90)9})10 11// Change the discount rule to anything at all.12// Change it to charge double. This test stays green.Every number in the assertion originated in the mock three lines above it. The test asserts the wiring — that Pricing calls the collaborator it was constructed with — and asserts nothing whatever about the discount rule, which is the behaviour the ticket was about. The review comment is not "add more tests"; it is "what would this test catch?"
The state the review itself is in
A checklist describes what happens inside one reading. The review as a whole is a lifecycle, and most of the process failures teams actually hit are transitions that should not exist rather than questions that were not asked.
The forbidden transitions matter more than the permitted ones. Each corresponds to a real thing teams do under deadline pressure, and each converts the approval into a statement about a diff nobody read.
| From | On | To | Guard | Effect |
|---|---|---|---|---|
| draft | author marks ready | in-review | pipeline green — mechanical work already done (What to Automate Out of Review) | a reviewer is assigned by name, not by group |
| in-review | reviewer objects | changes-requested | — | the objection names a consequence, not a preference (Tone, Disagreement and Receiving Review) |
| in-review | reviewer is satisfied | approved | — | — |
| changes-requested | author pushes a fix | in-review | — | the reviewer re-reads the delta, not the whole diff again |
| approved | author pushes new commits | in-review | any change beyond a rebase | the previous approval is voided, because it referred to a different diff |
| approved | merge | merged | diff unchanged since approval and checks still green | — |
| in-review | the change is not wanted | abandoned | — | — |
- draft → merged — Bypassing review entirely. It is nearly always justified as "it is only config" or "it is only a version bump" — which describes a large share of production incidents, because config changes have the blast radius of code with none of the tests (Validate at Startup, Fail Clearly in DevOps).
- changes-requested → merged — The objection is unresolved and now invisible: it lives in a closed thread nobody will read. If the objection was wrong, it should be answered in the thread; if it was right and is being deferred, that is a debt entry, not a merge (The Debt Register).
- in-review → merged — Merging while a review is open makes the review meaningless retroactively and teaches the reviewer that their attention is optional. Once a team has done this twice under deadline, reviews start being approved unread, because the reviewers have correctly learned what the process is for.
- approved → merged — Forbidden specifically when the branch has been force-pushed or extended since approval. The approval is a statement about bytes that no longer exist, and this is the transition that turns "reviewed" into a claim nobody has actually made (Protected Branches in DevOps).
Three of the four forbidden transitions are enforceable in branch protection settings. That is the point: these are not culture problems, they are configuration, and leaving them to discipline means they fail exactly when the pressure is highest.
How to build it
Most important first.
- Does the behaviour match the requirement? Read the ticket first, then the diff, in that order. Reading the diff first anchors you to what the code does and makes the mismatch invisible. This question catches more real defects than every other item combined.
- What invariant does this change, and who protects it now? A change usually moves responsibility for some rule. If the rule used to be enforced in one place and this change adds a second path around it, that is the finding (Invariant Leaks).
- What failure modes does this add? New network call, new transaction boundary, new partial-failure window, new retry. Each one is a state the system can now be in that it could not be in before (Failure-Aware Feature Design).
- Is this simpler than the alternatives that were available? Not "is it simple" — simplicity in the abstract is unanswerable. Was there a version with fewer moving parts, and does the extra machinery buy a change we actually expect (The Cost of Change)?
- Would these tests fail if the behaviour were wrong? The most-skipped question, because a green suite looks like an answer. A test whose assertions are all derived from its own mocks passes regardless of the behaviour (Mocking).
- Could you debug this from production, on a Sunday, without the author? Is there a stable id in the log line, does the error say which of the four inputs was rejected, is the failure distinguishable from a timeout (Debuggability by Design)?
- Stop there. If a seventh question is genuinely needed for a class of change, it belongs on a specific list for that class, not on the general one.
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.
- The list costs about ten minutes of reviewer time per change, every change, forever. That is the real price and it should be stated: for a team merging forty PRs a week it is most of a day.
- What it buys is a shift in *which* defects escape. Behaviour-versus-requirement mismatches and invariant leaks get caught earlier, where fixing them costs an edit rather than an incident and a migration.
- The next change to the checklist itself is cheap only if the team has agreed items can be removed. A list with an implicit append-only policy costs more at every future incident, because the only available response is to make it longer.
- What does not get cheaper: none of these six questions makes a badly-placed boundary cheap to move. They make it visible, which is worth something, and visible is not the same as cheap (Review as Design Feedback — and Why It Arrives Too Late).
- Six questions asked properly is slower than eleven nits, and the slowdown is immediate while the benefit is a defect that did not happen — which nobody experiences.
- Ordering by cost-if-missed means naming and readability come last and often get no attention at all. That is a deliberate sacrifice, and on a long-lived codebase it is a real loss (Naming).
- A short list is easier to defend and easier to game. "I asked all six" is available to a reviewer who thought about none of them, and no process fixes that.
What can go wrong
- The list becomes a form. Six checkboxes, all ticked, no comments — and now there is an audit trail asserting that a review took place.
- The list grows. Every incident adds an item, nobody ever removes one, and within a year it is the twenty-two item list again by a different route.
- Question four is used as a licence to relitigate the design at review time, which is where it is most expensive and least likely to work (Review as Design Feedback — and Why It Arrives Too Late).
- The mitigation fails: shortening the list to six causes people to add a "general quality" catch-all item, which restores the original problem in one line.
- Reviewers answer question one from the diff instead of the ticket, because the diff is right there and the ticket is a click away. This single ordering failure defeats the most valuable question on the list.
- Questions one and four depend on the requirement being written down; without it they degrade into the reviewer's opinion of what the feature should be.
- Question five depends on the reviewer being willing to read the tests, which most reviewers skip — the diff-viewing tools generally collapse test files by default, which is a small piece of tooling with a large effect.
- Question six depends on the team having any production observability at all. Where they do not, the honest answer to it is always no, and asking it every time is theatre (Logging at Boundaries).
- "So checklists work." Checklists work for procedures with known steps and a failure mode of omission. Review is a judgement task, and the list only decides *which* judgements get made — it does not make them.
- "Question four means demanding the simplest possible design." It means asking whether the extra machinery bought a change you expect. Sometimes it did, and the answer is a sentence rather than a rewrite (The Cost of Change).
- "If the tests are green, question five is answered." Green means the assertions held. Question five asks whether the assertions would have failed under wrong behaviour, which is a different property and usually untested (Test Doubles, Precisely).
- "This replaces design review." It cannot. Five of the six questions are about a design that already exists; only question four gestures at alternatives, and by then the alternative costs a week (Design Review).
Testing it, and how it ages
- Test the list against history: take the last five defects that escaped review and ask which question would have caught each one. Items that catch nothing in a year of real defects should be deleted.
- For question five specifically, the practical test is mutation — change the behaviour deliberately and see whether the suite goes red. Doing this by hand for one critical module is a fifteen-minute exercise with a reliably uncomfortable result (Testing as Design Feedback).
- Check that the answer "no" is ever recorded. A checklist on which every item has always been ticked is not being used.
- The list should shrink as tooling grows. Anything that becomes decidable by a linter or an architecture test leaves the human list permanently (What to Automate Out of Review).
- It should specialise as the system grows. Schema migrations, authorization changes and anything touching money acquire their own short lists, and the general list stays general (Destructive Migrations in DevOps).
- It stops working when the team's dominant risk changes. A list built around behaviour correctness is the wrong list for a team whose incidents are all capacity and configuration, and nothing about the list will announce that.
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 six questions are about the relationship between a change and the system around it, so they hold across languages and paradigms; only the mechanisms they point at differ — an invariant protected by a type in one stack is protected by a guard clause and a test in another.
- DOMAIN-SPECIFICIn a domain with genuinely simple rules — a content site, an internal dashboard — questions two and four rarely have interesting answers, and the honest version of the list there is three questions long. In payments, healthcare or anything with a regulator, question two dominates and deserves its own specialised list.
- CONTESTEDThe strongest opposing view is that any checklist harms review by converting an expert judgement task into a procedure, and the evidence from other fields is mostly about procedural work where the failure mode is forgetting a step, not about design judgement. Reviewers who are already good get worse when handed a list, because they start working through it instead of thinking. That is a real effect; the counter is that most reviewers are not already good and the list mainly reallocates attention away from formatting.
Where the depth lives
This domain teaches the codebase-level structure and hands the rest off.
- — Testing & Reliability Engineering — question five is really a mutation-testing question asked by hand, and the discipline that formalises it lives there.
- — System Design — question three, "what can now fail", is the entry point to failure analysis at the system level; this list only asks the reviewer to notice that the surface grew.