Nobody reviews their own code well, not even an agent

The idea gets passed around a lot: if an AI wrote the code, have another AI review it, because the one that wrote it is biased. The Claude Academy subagents course puts it this way: whoever wrote something doesn't review it well, because they read it with the memory of having written it. I repeated that in my previous post, and I have cases where it worked.

Open image full size

×Close image

It's true, but incomplete. A Microsoft study on code review found that review turns up fewer defects than people expect, and "even more rarely detects deep, subtle, or 'macro' level issues." I kept wondering why.

I went through the repo at work looking for bugs that had already been fixed and had this shape: the code looks correct line by line, the tests are green, and the bug lives at the boundary between two layers. I found three. All three went through review, and none of them was caught by reading the diff.

They're anonymized: generic names, nothing from the domain.

One: the test that documented the bug#

An ingestion process stores external codes with the leading zero, "012345". Another process, the one that looks things up by that same code, strips the zero before searching:

for line in lines:
    code = line.code
    if len(code) == 6 and code.startswith("0"):
        code = code[1:]
    item = Item.objects.filter(ext_code=code).first()
    if not item:
        log.warning("not found: %s", code)
        continue

No code with a leading zero ever matched anything. Every line got skipped by that continue, so the records were created, but without any of their lines. The only trace was a warning in the logs.

What makes this case special is the test fixture:

Item(ext_code="12345")  # leading zero removed

It's not that a test was missing. The test was green, documenting the bug as if it were the expected behavior, with a comment explaining it. Michael Feathers has a name for tests like this: characterization tests, which "document your system's actual behavior, not check for the behavior you wish your system had." Ours was one by accident. And the other fixtures used non-numeric codes, so the leading zero case never got tested.

It started with a refactor that moved the lookup from an old field, where stripping the zero was correct, to a new one where it wasn't. Neither that change nor its review caught it.

QA found it. The clue was in the logs: the external service returned 012345 and the log said it couldn't find 12345.

What the reviewer needed to know: how the other layer stores the data. Reading this diff, there's nothing odd to see. The author, the test and anyone reviewing it shared the same assumption.

Two: the fallback that never ran#

A function fetches remote data inside a try/except and, if that fails, sends the line to a fallback list:

if line.has_detail:
    try:
        d = client.get_detail(line.id)
        if d:
            attach(line, d)
    except Exception as e:
        log(e)
        pending.append(line)
elif line.code:
    pending.append(line)

It looks defensive and correct. The problem is that the HTTP layer underneath never raises: when it gets a 4xx, it logs it and returns None. So the line goes into the if, nothing raises, nothing gets attached, and the elif is never evaluated. The line disappears without ever reaching the fallback list.

The fix is to move the decision into a flag:

attached = False
if line.has_detail:
    try:
        d = client.get_detail(line.id)
        if d:
            attach(line, d)
            attached = True
    except Exception as e:
        log(e)
if not attached and line.code:
    pending.append(line)

It hit exactly the records furthest along in their lifecycle, the ones that had detail available: they were saved empty, with no visible error. It showed up in production, when monitoring kept firing after an earlier fix was already deployed. It came out of investigating that incident with help from an agent, already knowing where to look.

And here's the uncomfortable part: there was no test for the case where the detail lookup fails. But if someone had written one, it probably would have passed anyway, because when you mock, you follow the contract you imagine, "if it fails, it raises", and not the real one, "if it fails, it returns None". Fowler makes the point in his note on contract tests: "testing against a double always raises the question of whether the double is indeed an accurate representation of the external service."

What the reviewer needed to know: the real behavior of the HTTP layer, which lives in another file.

Three: the signal that skips the guard#

Before writing to the search engine, the reindex function makes sure the index exists with its explicit schema:

def reindex_all():
    ensure_index_with_explicit_schema()
    bulk_write(docs)

The guard is there, it's tested, and reading the repo it looks complete. What you don't see is that the library also writes on its own on every save(), through a signal, and that path doesn't go through the guard. If the index doesn't exist, the engine creates it by itself and infers the schema from the data.

All it took was someone editing a record from the admin panel at the wrong moment, and the index ended up with the wrong types. After that, filtering and sorting fail on every search. That same failure mode had already broken search in production once.

The fix was to put the same guard in the signal layer:

class SafeIndexer(LibraryIndexer):
    def on_save(self, instance):
        ensure_index_with_explicit_schema()
        super().on_save(instance)

It never showed up in tests because there the index always exists. A person found it, reasoning it through after the incident and checking it by hand against the development environment. And that fix later brought a second-order bug, because the guard ran even when automatic writes were turned off.

What the reviewer needed to know: the guts of a third-party library.

What fresh eyes do find#

To be fair, the same repo has a bug where a new reviewer would have shone. A default argument evaluated at import time:

def fetch(to_date=datetime.today() - timedelta(days=730)):
    ...

It freezes when the process starts, no caller passed it, and on top of that to_date pointed two years back, so every request went out with the range backwards. That one needs no context at all. Anyone who knows Python spots it in two seconds, human or agent. It's right there as a warning in the official Python tutorial: "The default value is evaluated only once." And that's the difference.

The distinction#

A reviewer who comes in fresh finds what looks odd on its own: the known antipattern, the default that's evaluated only once, the bare except, the obvious N+1. For that, fresh eyes are enough, and it actually helps, because whoever wrote the code carries around the reason they wrote it that way, and that reason blocks the view.

But the three bugs above don't look odd. They look normal. They look normal because we all shared the wrong assumption: the author, the test and the reviewer. There, fresh eyes only catch what their own experience taught them to look for, and an assumption everyone shares is exactly the one nobody looks for. Addy Osmani gets at the same thing from another angle in his post on agentic code review: "a model reviews the code that exists and rarely flags the requirement that nobody thought to write down."

What finds them is knowing something the author didn't. How the other layer stores the data. That this function returns None instead of raising. That the library writes through a parallel path.

When an agent does the reviewing, that knowledge has to be written down somewhere. It's not clean-context magic: it's a file that says "review against the behavior that already exists" or "confirm what the layer below returns before assuming it raises." Without that, a new agent reviewing the diff reaches exactly the same wrong conclusion we did.

What I don't know#

None of these three was found by an agent. People found two, and one came out of investigating an incident with help from an agent, already knowing where to look. So this isn't "agents find what humans don't". It's a taxonomy of what kind of knowledge you need, and it applies just as much to a human reviewer who's new to the team.

What's left is the real test: writing those rules into a reviewer agent and reporting back here on whether it catches the next boundary bug before production does. If you've already tried it, write to me.

For now, this is where I land: having someone else review is necessary, but it's not enough. They have to know something the author didn't. It's a cousin of what happened to me with the jobfit threshold I never measured: an assumption nobody questions is an assumption nobody reviews.