DEV Community

GinoCorp Industries
GinoCorp Industries

Posted on

Your code review checklist assumes the author understood the code

Every code review checklist you have ever used was written for a diff whose author could explain it.

Nobody wrote that assumption down, because until about two years ago it was free. Somebody typed the code, so somebody had a reason for every line of it, and the review was a conversation with that reason. Now a meaningful share of diffs arrive with no reason behind them at all, and the checklists have not changed one word.

What the assumption was doing

Reviewing has always been sampling. You do not verify a 300-line diff line by line; you read it for signals, and where the signals are good you extend trust across the rest. There has never been a version of the job with time to do otherwise.

The signals were all proxies for a mind. Does this person know the codebase. Did they think about the failure case. Do the names suggest they understand the domain. A model produces the surface features of every one of those without the thing underneath.

Three specific consequences:

Style consistency stops being evidence. A human whose code matches your conventions has usually read your code. A model matching your conventions has pattern-matched from whatever was in its context window, which might be two files.

Confidence stops being evidence. People hedge where they are unsure. They leave a TODO, a slightly defensive comment, a question in the PR description. Generated code is uniformly assured across the parts it got right and the parts it invented, so the tell you have been reading for years is gone.

Plausibility went up and correctness stayed put. The mistakes that used to reach review were visibly mistakes. Those are gone now. What survives is the API that does not exist, the config flag whose name means the opposite of what it does, the edge case handled in a way that compiles.

Take a retry loop. Suppose the diff wraps an HTTP call in exponential backoff with jitter, three attempts, a sane cap. Textbook. The question the checklist never had to ask is whether the operation being retried is idempotent, because a human who wrote that loop had at least glanced at the question, and could answer it in a sentence. The diff looks identical either way.

The twelve questions

Twelve questions for a diff you did not write. All of them here, nothing withheld. Most were already on somebody's checklist. What changed is that they used to be spot checks and now they are the review.

Before reading the code

  1. What was this supposed to do, in one sentence, from the person who asked for it? If that sentence does not exist, you are reviewing an answer with the question missing.
  2. Which parts of this arrived whole and which parts did a human write? The point is knowing where to spend your attention, and generated sections deserve more of it than hand-written ones.
  3. Can the submitter explain a line you point at? One line, chosen by you rather than by them. This takes fifteen seconds and it is the highest-yield question on the list.

Reading it

  1. Does every function, flag and constant in here actually exist? Your compiler catches the typed ones. It does not catch a config key, a feature flag name, a CLI argument, or a field on a JSON payload from another team's service.
  2. Is the error handling doing something, or is it catching and continuing? Generated try/except blocks are frequently shaped correctly and empty of intent.
  3. What happens on the second call? Retries, concurrent execution, the user double-clicking submit.
  4. If this fails halfway, what state is the system in? Partial writes, a queue message consumed but not acted on, a file moved but not registered.
  5. Where does untrusted input enter, and what touches it before it is validated?
  6. What is the blast radius outside the files in the diff? Schema, shared config, a public API shape, a utility three other services import.

Before approving

  1. What does this cost at production volume? Models write correct code at test-fixture scale. A query inside a loop over ten items is fine and over ten thousand is an incident.
  2. Do we already have this? Generated code re-implements things sitting in your utils package, because the model could not see your utils package.
  3. If this breaks at 3am, can somebody who has never read it work out why from the code and the logs alone?

If you only ever adopt one of these, adopt number three.

The tests are not a second opinion

The same process wrote the implementation and the tests, so you do not have a check. You have one opinion, written twice, in two files.

A test derived from an implementation asserts that the implementation does what it does. It will pass. It will keep passing. It tells you nothing about whether the behaviour was ever right.

Three things that cost about a minute each:

Break it on purpose. Invert a comparison. Return early. Delete a branch. Run the suite. If it still passes, that test is decoration and you have just learned that for free. This is mutation testing done by hand, and on generated tests it fails far more often than people expect.

Read the assertions before the implementation. If the assertions restate the function's steps in order, they are a mirror. Assertions worth having are about the observable contract: given this input, this comes back, this row exists, this call was made once.

Write the cases nobody asked about. The model tests the spec it was given. The spec did not mention the empty list, the duplicate submit, the string with a newline in it, or the clock going backwards. Those are yours, and they are where the bugs are.

Shipping: four gates you can actually enforce

Most teams' AI policy is a sentence in a wiki that says something like "use AI tools responsibly." That is not a gate. A gate is a thing that is either true or false at merge time, and someone notices when it is false.

Four that pass that bar:

  • Attribution on the pull request. A checkbox or a label saying how much of this was generated. Six months later when something breaks, knowing the answer changes where you look first.
  • A named accountable human. One person on the record who is responsible for that code exactly as if they had typed it. This is the entire policy compressed into one line, and it is the one most teams have quietly stopped having.
  • No fast lane. Generated code does not skip your slowest check. The speed gain is in writing, not in verifying, and the trade teams make without deciding to is spending the writing gain on a shorter review.
  • A size cap. A 900-line diff produced in forty seconds does not get reviewed. It gets skimmed and approved, by everyone, always. Cap the reviewable diff and split the rest.

What none of this fixes

Two things, and it is worth being straight about them.

Review throughput. Generation got faster and reading did not. If a team is producing three times the diff on an unchanged review budget, no checklist rescues that; the queue just grows and the standard drops to meet it. That is a scheduling problem wearing a quality problem's clothes, and smaller pull requests are a much better answer to it than any amount of discipline about how you read them.

And the slow one: reviewing is how most engineers actually learn a codebase. If the writing gets delegated and the reading gets skimmed, the number of people who could explain the system goes down every quarter, and nobody notices for about a year.

The old checklist worked because authorship carried information. It does not any more. The fix is not more suspicion, it is moving the burden of proof: a diff now has to demonstrate what it used to be allowed to imply.

What is on your list that is not here? The interesting ones are always the questions somebody added after a diff got through.


Disclosure, since it should be in the open: I sell small toolkits for engineers, and one of them is a $5 pack of 15 system prompts for engineering work, including a strict PR-review pass and a test-matrix builder. Flagging it because it is mine and it would be odd not to. The twelve questions above are the complete list; nothing was held back for the paid thing.

Top comments (0)