Nine rounds with a machine reviewer

What nine adversarial review rounds on a single pull request taught me about where AI code review actually stops — including three defects in fixes the reviewer itself asked for.

I run a small fleet of coding agents at home, and it has one rule that matters more than the others: coder ≠ reviewer ≠ merger. One model writes the code. A different, more capable model reviews it. A human merges. The reviewer never edits; findings go back to the coder.

Worth stating plainly before anything else, because it changes how you should read what follows: in this story I am the merger. I did not write the code under review, and I did not write the reviews. I set the loop running, paid for it, read the arguments, and decided when to stop. Everything below is me watching two models argue about a change neither of them could merge.

Last week that rule got a real test. A single pull request went through nine review rounds before I ran out of budget — not out of findings. This is what the transcript of that argument actually shows, because I think the shape of it is more interesting than the usual "AI caught a bug" story.

What the change was

The PR added a watcher with an unusually sharp security property. In this fleet, merging a plan file is the ignition: when a markdown plan lands on main with status: ready, the watcher starts an autonomous agent to build it. There is no separate "arm" step. Reachability on main is the authorisation.

That makes one function load-bearing for the entire design. Before dispatching, the watcher asks: can anything other than a human's reviewed merge write to this repo's main branch? If yes, it refuses — because if ignition is forgeable, an agent can start itself.

So the reviewer had a crisp job: find the ways that check is wrong.

The arc, round by round

It found something real every single round. But what it found changed shape as it went, and that arc is the actual story.

Round six found the check vouching on partial evidence. It refused three ways of writing to main and was silent on three others visible in the same API response it was already parsing. The sharpest: branch protection has an unprotected_file_patterns field, and a thoroughly ordinary "don't gate docs" rule of **/*.md exempts plans/*.md — which is the ignition file. An agent could push a ready plan straight to main and self-ignite while the function vouched for the repo.

Round seven found three more, and one of them was in the previous round's own fix. Round six's remedy added a brace expander for the glob patterns, with a confident docstring explaining that over-matching was the safe direction. The reviewer executed it against crafted input and showed that it expanded exactly one brace group, so {plans,docs}/{0001,0002}*.md kept a literal brace that never matches anything — fail-open, precisely the opposite of what its docstring claimed.

Round eight found that the next fix checked a weaker property than its own documentation claimed. It probed one representative filename; **/README.md misses that probe while still exempting a real ignition file.

Round nine found an eighth lane: apply_to_admins, which defaults to off, and while off a repository admin bypasses the whole rule — every check above it, at once.

The finding I didn't expect

Three of the defects across those rounds were in fixes the reviewer itself had asked for. It requested a change, the coder made the change, and the change was wrong in a new way.

That sounds like an indictment and isn't. It's the mechanism working. But it kills a comfortable assumption: that a reviewer asking for a fix has, in some sense, already validated the fix. It hasn't. A requested change is new code, and new code has never been reviewed. The loop does not converge just because the reviewer is confident.

The second thing I didn't expect is that it never hit zero. The findings narrowed steadily — round six's failures came from ordinary configuration anyone might write, while round nine's needed an agent account holding repository admin. But narrower is not none. Each round found one more field in the same API response.

Which raises a question I can't answer cleanly: is "review until the reviewer approves" the same loop as "review until the reviewer exhausts the surface"? For a function that reads a response body with dozens of fields, those may be indistinguishable from the inside.

How it actually ended

Not with an approval. The reviewer model hit its monthly spend limit, I moved it to prepaid API credits, and those ran out too. Nine rounds at roughly a dollar each.

I want to be plain about that, because it is the honest ending and it generalises: termination was a budget decision, not a quality signal. If I'd had another twenty dollars I'd have another few rounds, and I'd probably have another few findings — each one narrower than the last, each one real.

That's an uncomfortable thing to design around. It means "reviewed until clean" is not a state you reach; it's a line you draw. Better to draw it deliberately — at a severity threshold, or a round count, or a budget — than to pretend the loop terminates on its own.

The mistake in the plumbing

Here is the part that actually cost me time, and it had nothing to do with the model.

When the spend limit hit, the dispatcher reported FATAL: claude exited 1: (no stderr). The hunt went after a configuration problem, found a real one, fixed it — and nothing changed. The refusal was on stdout; the error handler read stderr. Later the same day a second refusal was masked the same way, because the handler preferred stderr and stderr had a harmless warning in it.

Two lessons, and the second is the one I'd tattoo somewhere. A CLI refusal is not a shell error, so report both streams and let the reader judge. And a fix that changes no behaviour has not been validated — a genuine bug had been found and assumed to be the bug, which bought two hours of debugging the wrong thing.

What I'd change about the order

That PR carried 293 passing tests and zero commit statuses. The tests passed because a session ran them. There was no CI, so a merge would have rested on somebody's recollection that they were green.

The obvious fix is to run the cheap deterministic gate before the expensive nondeterministic one: tests take seconds and cost nothing, a review round takes ten minutes and costs a dollar. Don't spend the second on code that fails the first. That has since been wired in — the dispatcher now refuses to spend a round when CI is red or still running.

But I want to be careful about the claim, because in this case it would have saved nothing. The suite was run before every push and was green every time, and not one of the nine rounds' findings was in territory the tests covered. They were in protection lanes, error paths, page storms, config parsing. Tests passing predicted none of it.

So the gate is about cost discipline and honest evidence, not about correctness. Those are different arguments and it's worth not confusing them.

Would I do it again

Yes, with a stopping rule.

The nine rounds produced a genuinely better function — it now refuses eight distinct ways of writing to a protected branch, each one traceable to a specific finding, each one with a test. Reading the diff myself I would have caught maybe two of them, and none of the ones hiding inside the fixes.

What I'd give up is the fantasy that the process ends by itself. An adversarial reviewer with tool access is a search over a surface, not an oracle that converges. It will keep finding things for as long as you keep paying, and the value of each round declines while the cost of each round stays flat.

Decide in advance where you stop. Then let the machine argue with you until you get there.