A year of measuring what a model-backed assistant does produced one clear finding: the volume of code needing review went up and the heuristics reviewers use went down in reliability. This is about the second half — what changes when the code being reviewed was not written by a person who was thinking about the problem.
The symptom
// the change, as it arrived in review
public function applyDiscount(Order $order, Discount $discount): Money
{
$total = $order->total();
if ($discount->isPercentage()) {
return $total->minus($total->multipliedBy($discount->value() / 100));
}
return $total->minus($discount->amount());
}
// idiomatic, readable, tested, and wrong for a discount
// larger than the total — which returns a negative Money
// that the rest of the system assumes cannot exist.The code reads well, the tests pass, and it is the kind of change that gets approved in ninety seconds. The bug is in a case the tests do not cover and the reviewer does not think to check, because the code looks like it was written by somebody who had considered the cases.
Why it happens
Review has always relied on a signal nobody states: code that is idiomatic and confident was probably written by somebody who understood the problem. Generated code is idiomatic and confident regardless, which removes the correlation without removing the reviewer’s reliance on it.
The fix
The heuristics that stop working
"it looks like our code" it matches the local
style because the surrounding file is in context.
that is not knowledge of our conventions.
"the variable names are good" naming is what these
tools are best at, and it is uncorrelated with
correctness.
"there are tests" generated tests test what the
code does rather than what it should do.
"it handles the error case" a branch exists.
whether it is the right one is a question nobody
asks once they see a try block.The tests one is the most dangerous, because a change arriving with tests reads as more careful and is exactly the case where the tests carry no independent information. A test written from the same understanding as the implementation cannot detect a wrong understanding.
What to read first
the parts a model has no way of knowing:
1 our invariants — money is minor units, an order
cannot have a negative total
2 the state around this code: what the caller has
validated, what the database constrains
3 which of several correct approaches this codebase
has chosen
4 anything decided in a conversation
and the parts it is reliably good at, which can be
skimmed: syntax, naming, structure, the happy path.Boundary conditions and error paths
// "what happens at and beyond the boundary?"
$discount->value() === 0 // no discount. fine.
$discount->value() === 100 // free. fine.
$discount->value() === 120 // negative total.
$order->total()->isZero() // fine, accidentally.Reading for boundaries rather than for correctness is a different activity and is faster, because there are usually three or four of them and they are findable from the signature. The question “what is the largest and smallest value each parameter can take” catches most of what generated code gets wrong.
The tests that assert nothing
// generated, and useless
public function testApplyDiscountReturnsMoney(): void
{
$result = $this->service->applyDiscount($order, $discount);
self::assertInstanceOf(Money::class, $result);
}
// generated, and useful
public function testAPercentageDiscountReducesTheTotal(): void
{
$result = $this->service->applyDiscount(
$this->orderTotalling(10000),
Discount::percentage(10),
);
self::assertEquals(Money::pence(9000), $result);
}The ratio in our experience is about one useful test in three, and the two that are not useful raise the coverage number without raising the verification. A coverage-based quality gate rewards exactly this, which is an argument for the mutation score being the metric anybody looks at.
Making the origin visible
a pull request template with one checkbox:
[ ] This change contains substantially
model-generated code.
why: not to gate it, and not to shame anybody. so the
reviewer knows which heuristics to distrust.
what happened: it is ticked on about 30% of pull
requests, and the review time on those is measurably
longer — 14 minutes against 8.
which is the intended outcome and is a cost.The checkbox is self-reported and unenforceable and it works, because nobody has an incentive to lie about it. The longer review time on flagged changes is the mechanism working rather than a problem, and it is the number that would be quoted if anybody wanted to argue the tool is not saving time.
The three findings, and what they share
// 1. concatenation, in a codebase where every query is
// bound
$sql = "SELECT * FROM orders WHERE reference = '$ref'";
// 2. a signature compared with ===
if ($expected === $provided) { }
// 3. an error response echoing the exception message
return response()->json(['error' => $e->getMessage()], 500);All three are patterns that appear constantly in public code, all three look ordinary, and all three would pass a review that is reading for style and structure. The second is the one that worries me most — hash_equals is not visually distinctive and its absence is not something anybody scans for.
The checklist, which is four lines
For flagged changes, before approving:
1. What are the boundary values of each parameter?
2. Which of our invariants does this touch, and does
it preserve them?
3. Do the tests assert behaviour, or existence?
4. Is anything here in the not-used-for list?Four questions rather than a process, because a longer checklist is one nobody follows. The fourth is the cheapest and has caught two changes — both of them a query built in a place the list says the tool is not used, which suggests the list needs enforcement rather than agreement.
Verifying it worked
$ ./bin/review-findings --since=6m --by-category
flagged unflagged
correctness 18 9
convention 41 8
security 3 1
style 26 44
$ ./bin/review-duration --since=6m
flagged median 14m, unflagged 8m
$ ./bin/escaped-defects --since=6m --by-origin
flagged 2 unflagged 3
# no signal, on a sample this smallConvention findings five times higher on flagged changes is the clearest number and matches what the controlled comparison predicted a year ago. The escaped defect counts are too small to mean anything, which is the honest state of the evidence after eighteen months.
What this costs
Reviewing more code than before with the same attention available. Fourteen minutes against eight on thirty per cent of pull requests is roughly an extra hour a week per reviewer, and the assumption underneath all of this is that the reviewer is having a good day — which nothing in the arrangement protects.
The checklist is also a norm rather than a mechanism, self-reported, unenforced, and dependent on people being honest about something with no penalty either way. It works now, with three people who agreed to it. Nothing about it survives a team that has not had the conversation, and the conversation is the part that does not scale.