The Formatting Only Pull Request That Changed Behaviour

Share
The Formatting Only Pull Request That Changed Behaviour. Abstract code review illustration in orange and dark grey on debugly.dev

The pull request was titled "format: apply prettier across services". It touched four thousand lines. The reviewer, an experienced engineer, approved it in eleven minutes. Two days later a payment retry fired twice and we had an incident.

The semantic change was one operator, moved by the formatter, in the middle of line 2300. The review process had done nothing wrong, and that is exactly what worries me.

This is not a story about a careless reviewer. It is a story about what human attention does to a large uniform diff, and how to design reviews so that uniformity cannot hide a change.

What the formatter did

The original code:

if (retryCount < maxRetries && !idempotent
    || forceRetry) {
  await charge(order);
}

The formatter, which is configured to normalise indentation and line breaks, produced:

if (
  (retryCount < maxRetries && !idempotent) ||
  forceRetry
) {
  await charge(order);
}

Now, a formatter should not change meaning, and this one did not change it either. The meaning was changed by the author, in a separate commit in the same branch, who "fixed" the operator precedence while the formatter was running and trusted the diff to be formatting only.

The original condition, by JavaScript precedence, is (A && !B) || C. The author believed it was A && (!B || C) and reformatted it to make that explicit, which is a real behaviour change. The diff, buried under four thousand lines of reindentation, showed one line that looked like the formatter's work.

Why the review failed, structurally

Reviewers do not read diffs line by line. They read them in bands of attention, and a diff that is 99.9 percent formatting teaches the reviewer, within the first screen, that the changes are mechanical. Attention then collapses to skimming. This is not a character flaw. It is how humans process large uniform stimuli.

The defect is that the diff mixed two kinds of change: a mechanical, provably safe transformation, and a semantic one. The safe transformation acted as camouflage for the unsafe one.

The fix is therefore not "review more carefully". It is to make the two kinds of change impossible to mix.

Separate mechanical from semantic, always

The rule I now enforce: a pull request is either formatting or behaviour, never both. If you want to run the formatter across the codebase, that is its own pull request, and it is reviewed by a machine, not a human.

For the mechanical side, the review is a verification that nothing semantic changed. That is checkable:

git checkout main && npm run build && cp -r dist /tmp/before
git checkout feature && npm run build && diff -r /tmp/before dist

If the compiled output is identical, the formatting change is, by definition, semantic free. Some teams use a stronger form with the language's own AST, comparing parse trees before and after, which is exactly right.

For the semantic side, the precedence fix is a one line change in its own pull request, with a title that says "fix retry precedence", reviewed with full attention, with a test that encodes the truth table of the condition.

How to review a diff that is already mixed

You will inherit mixed diffs, so you need a technique for them too. Three things work.

Hide whitespace. Every review tool has it. git diff -w ignores whitespace changes and collapses the four thousand line diff down to the handful of lines that actually differ. If you review nothing else, review with -w first. The semantic change becomes the only thing on screen.

git diff main...feature -w

Sort by suspicion, not by file. Look at the lines the formatter would not have produced. A formatter reindents and rewraps. It does not move an operator from one side of a condition to the other, rename a variable, or delete a negation. Train your eye to skip reindentation and stop on anything structural.

Ask the author to separate it. If you receive a mixed diff in review, do not review it. Send it back with a request to split into a formatting only commit and a behaviour commit. This is not pedantry. It is the difference between an eleven minute skim and a real review, and the author can produce the split in minutes with their own tooling.

The wider lesson about review attention

The formatting diff is a special case of a general problem: review attention is a budget, and large diffs spend it on noise. This is why reviewing large AI generated pull requests is hard for the same reason. The volume itself is the camouflage.

The structural answer is the same everywhere. Reduce the diff to the semantic delta before a human looks at it, and spend human attention only on things a machine cannot certify.

For formatting, the machine can certify. For the precedence fix, it cannot, and that line deserved a reviewer's full attention, a truth table and a failing test written first.

The checklist

For any diff over a few hundred lines, before reviewing: run git diff -w to see the semantic core. Ask whether the mechanical part can be certified by identical build output or AST comparison. If the diff mixes the two, split it. And if the title says "format" or "chore" or "cleanup", treat that as a claim to verify, not a category that exempts it from review.

None of this is an argument against formatters, which are unambiguously good. It is an argument against mixing their output with hand edits in a single reviewable unit, and against treating a diff's title as evidence of its contents.

The eleven minute approval was rational behaviour given the information the reviewer had. The failure was that the information was arranged to hide the one line that mattered. Arrange it differently, and the next reviewer gets to be both fast and correct.