More reviews don’t always help

Nico BelloHead of Engineering · Qurable

With agents, a code review takes minutes, and the temptation is to always ask for one more. On a concurrency fix, the team ran seven reviews across three rounds, and each round found bugs introduced by the previous one. What finally measured the quality of the change was breaking the code on purpose.

A concurrency fix in a sync process went through seven clean-context reviews, spread across three rounds. Each review was done by an agent that read the diff without having seen the conversation in which the change was written. All three rounds found real problems, and in all three most of those problems had originated in the fixes from the previous round.

The change was narrow. A process that dumps the results of a query into another store decided whether a job was already in progress by looking at a counter that only advanced when the consumer acknowledged a batch. While the producer was preparing the data, the counter stayed at zero, and every scheduled run concluded that nobody was handling the job and started it again from scratch. In a single night, the same work was repeated more than twenty times. The fix added an explicit claim: a producer takes the job, renews it as it progresses and releases it when it finishes. That changed a system invariant, the order in which certain things have to be recorded before others happen.

This post is about what happened while that change was being reviewed, more than about the bug.

The first round was the most productive. It found a claim that was taken in one place and released in another, so that five error paths could leave it held until it expired. It also found a state transition that could mark a just-completed job as failed, triggering exactly the full rebuild the change was meant to prevent. These were bugs that would have reached production.

The second round was requested with an explicit focus: look for defects introduced by the first round's fixes. It found several. One was a claim renewal that had been moved “to every row” and didn't cover skipped rows, which was precisely one of the cases the new comment said it covered. The reviews kept finding things, but what they found was starting to be of a different kind.

From the second round on, a good share of the findings weren't in the code but in the prose describing it: comments, documentation, the migration header. One section talked about “three columns” for a table with four rows. A configuration comment named the wrong class.

The explanation is simple. A concurrency fix changes what is true about the system, and a repository has many places that make claims about the system. Each round of fixes edits some of them and leaves the rest as they were.

Code has tests that cover it, better or worse; prose has none.

The most serious case showed up in the third round. The repository has a feature catalog that the review guide designates as the source of truth, the file you're supposed to read before touching a feature. Nobody had modified it on the entire branch, and it still stated that a certain value had to be recorded before dispatching the batches, which was exactly what the PR had reversed.

That document, in that state, invited reopening the bug. Someone arriving months later, reading the catalog and seeing that the code “didn't comply” with what was documented, would have every reason to “fix” it, and in doing so would return the system to the state in which it repeated the work.

A practice came out of that. After each round of fixes, reviewing the touched files isn't enough: you have to search the whole repository for the trace of the claim that was corrected (the phrase, the field name, the order of the steps) and look at every place where it still appears. The useful question is which other places still say the opposite of what is now true.

With human reviewers, cost sets the limit. Nobody asks a teammate for a fourth review of the same PR without a good reason. With agents, that brake disappears: a clean-context review costs a few minutes and a few cents, and it always returns something.

Each additional read found less, and what it found looked more and more like noise generated by the previous rounds. At the same time, each read triggered a new batch of fixes, with their own bugs.

Past a certain point, reviewing again didn't bring the change closer to done; it kept it in motion.

There was also a side effect. Some reviewers read a version from before the latest fixes and reported already-resolved issues as pending. In one of the passes, eleven of twenty-five findings didn't hold up under later verification. That's why, for every finding, the first step is to confirm the problem exists in the current version. Fixing the same thing twice is one of the most direct ways to introduce a new bug.

The reviewers weren't the problem. A read-through, however careful, answers whether the code looks correct, and after two rounds the code looked correct. What we still needed to know was whether the tests would notice if it weren't.

That question is what mutation testing is for. You introduce a deliberate defect in the code (invert a condition, remove a guard, change a throw into a log, reorder two writes) and run the suite. If any test fails, the mutation “dies”: that behavior is protected. If the suite passes with the defect in place, the mutation “survives,” and that behavior has no protection, whatever the coverage report says.

The first campaign applied twelve mutations and seven survived, on a change that had already gone through several reviews.

The tests existed, ran those lines and added coverage. What they didn't do was fail when the code was wrong.

The team closed those gaps and ran a second, broader campaign of thirty-eight mutations. Six new ones survived, and two of them reproduced the failure mode the PR was meant to fix: a job could be marked as done with a result size that had never been durably saved, which would have published a truncated result. None of the seven reads had flagged it, and that made sense: the code did the right thing. What was missing was a test that checked it kept doing so.

The surviving mutations repeated patterns that show up in any suite.

The most common is checking that a function was called without looking at the arguments. A test that checks the status update was invoked passes just the same whether it's invoked with the right condition or with none, and in a concurrency change the condition is what matters.

// Passes with the bug in place:
expect(repo.updateStatus).toHaveBeenCalled();

// Fails if the write stops being conditional:
expect(repo.updateStatus).toHaveBeenCalledWith(
  jobId, FAILED, { expectedStatus: RUNNING },
);

A variant is using toHaveBeenCalled() when the function is invoked from two places and only one matters for the case. The test is satisfied by the wrong call.

The third pattern is harder to spot. A shared mock always returned a valid row and no test overrode that value. Every guard of the “if the write returned nothing, abort” kind was left uncovered: you could delete it or turn it into a warning and the suite stayed green.

A mock that always returns something reasonable decides in advance which branches won't be exercised.

And one practice worth not skipping: when you write a test to kill a mutation, you have to check that it kills it. Reapply the mutation, run the suite, confirm it fails and only then restore it. A test written “for this case” can pass with the bug in place for any of the reasons above, and without that check it isn't worth much more than a comment.

Reviews are still necessary. The first two rounds found serious bugs no test would have flagged, and reading in a clean context is still the best way to catch a wrong design or an unhandled case.

Each instrument measures something different, and repeating a measurement doesn't make it more precise.

A review measures whether the change is understandable and looks correct. A mutation campaign measures whether the suite will hold that change when another person, or another agent, touches it without knowing the history. From the second round on, the second question weighs more than the first.

With AI, this matters more than before. When reviewing costs almost nothing, it no longer makes sense to ask whether there's time for another review; what matters is whether another review is what the change needs at that moment. Asking for one more is easy and feels like progress. Noticing that the instrument has stopped paying off and switching to another is a judgment call, and that judgment still sits with the engineer.

The way of working we kept is this: one or two clean-context review rounds; after each round of fixes, a search for the corrected claim across all the documentation; and from there, real mutations instead of more reads, with every new test verified against the mutation it claims to cover. It takes more work than asking for another review, and it's what shows whether the tests protect the change or just pass through its lines.