Most of what my review gates caught wasn't wrong code

When a model writes the code, most of what goes wrong isn't wrong code. Over the last two weeks 53 AI-built changes of mine went through two review gates, and the gates raised 55 blockers. Seven of those were code that ran and did the wrong thing. 26 were faults in the plan, found before the builder wrote a line, and 13 were planned work that hadn't happened.

A diff review looks for wrong code, and in this sample that was the smallest of the three problems. So on a team I'd put people on three other checks first: the plan, whether the build did what the plan said, and running the thing. The diff still gets its review, after those.

Where the numbers come from

Every change I make goes through cf, my Claude Code plugin. A planning session writes a plan from a brief I've agreed. A reviewer that never saw the planning reads the plan against the codebase. I approve it, a builder writes the code, and a verifier working in its own context checks the result against the acceptance criteria. Either reviewer can raise a blocker, which stops the change until someone fixes it. The first article on cf covers how the gates work.

The 53 changes shipped between 30 October and 11 November, after 78 plan reviews and 80 verify runs. 28 of them passed both gates first time.

The numbers have limits. Both reviewers are models. I dealt with each blocker as it came up, but I haven't measured how often the reviewers were wrong, so every "would have" here is a reviewer's case that I accepted at the time. I've counted shipped changes only. One change is still open, and nearly all of its 21 verify blockers are the same unfinished work reported round after round.

Review the plan with the code open

The plan reviewer raised 26 blockers.

• 11 were steps or tests that couldn't pass as written.

• 9 would have broken something that already worked.

• 4 were security gaps in the design.

• 2 didn't deliver what the brief asked for.

The second group is the one I'd show a sceptic. One plan revoked permission on a database function without noticing that a column default called it, so every insert into three tables would have failed. Another changed a refund path so that an uncertain answer from the payment provider no longer raised a flag, so a later cancellation would have gone down as "no refund" without anyone asking the provider. If the first refund had gone through after all, the books would have said the customer got nothing back when they had. Two would have touched live data. One would have left test rows behind in production, and the other's smoke test could have set off a real scheduled job.

One security gap broke a rule the brief said belonged in the database, that only you can see your own history. The plan's access policy let every other member of the same group read those rows.

The eleven that couldn't pass are duller and as useful. A test expected a value that the plan's own minimum-sample rule made impossible. A dependency upgrade would have stopped on a peer dependency conflict. Every one of the eleven cites the file or the configuration that contradicts the plan, which tells you how the reviewer found them. It opened what the plan referred to.

The approval belongs to the engineer who knows that part of the codebase best, reading the plan with the code open. On a .NET or Java service that means the solution is open beside the plan, and the question for each step is whether that interface and that build step are what the plan says they are. It's the review I'd protect first, because a fault found here costs an edit to a paragraph.

Check the build did what the plan said

A clean plan didn't mean a clean build. Of the 37 changes whose plan passed first time, the verifier still sent 9 back.

The verifier raised 29 blockers.

• 13 were work in the plan that the builder hadn't done.

• 7 were behaviour that was wrong.

• 4 were the verifier looking at the wrong branch, a fault in my tooling.

• 3 were one known security hole, still open in three successive rounds.

• 2 were new tests that flaked or broke the suite.

One shipped change ran for three rounds and repeats a blocker from each of three groups. Counted once each the 29 becomes 23. Unfinished work drops to 11, still ahead of the 7 for wrong behaviour, and the wrong branch and the security hole lose two each.

In the largest group the builder had stopped short. It hadn't run the final steps, or hadn't written a document the acceptance criteria depended on, or had written migrations and not applied them. Mostly it said so in its own log. Four of the 13 were stops I'd asked for: the builder reached a line I don't let it cross, such as applying a migration to production, or needed a ruling from me. I'd rather have that than a builder that carries on. They still count, because the plan's work wasn't done and nothing told me so except the verifier.

It does mean "the build finished" and "the build did what the plan said" are separate facts. The developer who owns the change checks the second one before anyone reviews code. They read the builder's log and the command output against the plan's steps. A list of commits won't do, because much of what the builder skipped here was a step to run, and a skipped step leaves no file behind.

Run it, and scale the effort to the risk

Only seven blockers were code that ran and behaved wrongly, and the verifier found five of them by running the application. With JavaScript off, a form showed no confirmation, and only in a production build. At phone width a page hid its heading under the navigation bar. One switch measured 52 by 32 pixels against the project's 44 pixel minimum for touch targets, the same figure WCAG gives for its enhanced target size. I doubt a reviewer reading the diff would have caught most of them. Add the two bad tests and that's nine of the 55 where the code itself was at fault.

I wouldn't ask a team for every check on every change. Two things would happen every time, and both belong to the developer who owns the change. They confirm the integration tests ran against a real database and real services, because a builder that skips steps can skip that one. When there's a migration, they apply it to a real environment and check the result.

A change that users see, or one that touches money, sign-in or access to data, is in a higher-risk class. The engineer approving the plan marks it, and the class itself is in the rules I write. For those changes a second engineer exercises the deployed build and walks the path by hand.

What stays with the lead

The rules the agent loads are mine, and so are the gates and the definition of a blocker. I set the limits on the builder too, such as never pushing a branch or applying a migration to production, and I decide when a person does it instead. Every fortnight I'd read the gate numbers, as I have for this one, and change the rules on what they show.

Plans in the higher-risk class get my signature as well as the area owner's. As the team grows I'd pass that to named senior engineers, because a lead who is the only approver becomes the queue.

Sources

• Understanding Success Criterion 2.5.5: Target Size (Enhanced), W3C, https://www.w3.org/WAI/WCAG22/Understanding/target-size-enhanced.html