"The error handling here is poor."
I've written that comment. You've written that comment. On a human pull request it's fine, because the person on the other end knows what they were thinking, knows the codebase, and will read four words and go fix the right thing.
Hand that same comment to an agent, or to a training pipeline, and it does nothing. There's no shared context to fill the gaps with. The comment has to carry all of its own meaning.
That's the small version of a larger problem I've been working through: the review instincts I built over years of reading human code are tuned to how humans make mistakes, and AI-generated code fails somewhere else entirely.
Your heuristics assume an author
Think about what you actually do when you skim a diff. Most of it rests on three assumptions you've never articulated.
You assume the author read the surrounding code. So when you see a new helper function, you assume they checked whether one existed. You don't grep. An agent may have written that helper without opening the file that already contains it, because its context window held six files and the right one wasn't among them.
You assume intent is stable. A human who decided to use the repo's error type in the first hunk will use it in the fourth. That's not a rule they're following, it's just how people work. An agent's adherence to a convention decays across a long run, and the fourth hunk genuinely can contradict the first.
You assume mistakes cluster around difficulty. So you read the gnarly concurrency section carefully and skim the config parsing. Reasonable, for humans. For models, mistakes cluster around plausibility instead. The gnarly bit often comes out fine because it's well represented in training data. The bug is in the boring function that calls an API which does not exist but looks exactly like one that does.
That inversion is the thing to internalize. You are not looking for hard things done wrong. You are looking for easy things done in a way that reads correct.
Which means you need a different checklist.
The output taxonomy
These are labels for defects in the code itself. Fixed labels matter more than perfect labels: a hundred reviews using the same vocabulary are comparable, and a hundred freeform notes are not.
Correctness
| Label | What it is | How I catch it |
|---|---|---|
| Fabricated API | Calls a function, flag, or field that does not exist | Grep the symbol. Plausible name, correct-looking signature, not real |
| Version drift | A real API, from a different version of the dependency | Check the lockfile, not the docs |
| Wrong semantics | Compiles, runs, does the wrong thing | Read the requirement, then read the code. Inverted condition, off-by-one, wrong default |
| Unhandled edge case | Fine on the happy path, breaks on empty, null, zero, unicode, or concurrent | Enumerate the input space, not the test cases |
| Silent failure path | Swallows an error, returns a default where it should raise | Bare except, empty catch {}, ignored return values |
| Type erosion | Widens a type or adds a cast to quiet the checker | Any any, interface{}, or # type: ignore in the diff |
Fabricated API is the famous one. Type erosion is the one I catch most often and the one people flag least, probably because a green type checker feels like evidence.
Integration
| Label | What it is |
|---|---|
| Pattern violation | Ignores the repo's own error type, logger, or config layer and rolls a new one |
| Duplicated capability | Writes a helper that already exists ten lines away. A direct symptom of insufficient context gathering |
| Broken contract | Changes a public signature, serialization shape, or schema without touching callers or migrations |
| Dependency creep | Adds a package for something the standard library already does |
Integration failures are where agents underperform most relative to how good the code looks in isolation. Every one of these is a context problem wearing a correctness costume.
Scope
| Label | What it is |
|---|---|
| Scope creep | Fixes the bug, then reformats, renames, and improves three unrelated things |
| Under-delivery | Does the first of three requested things and reports success |
| Placeholder shipping | TODO, NotImplementedError, or a stub presented as complete |
| Over-engineering | Abstraction and error handling for cases that cannot occur |
Under-delivery deserves special attention because the summary always reads well. The agent tells you it completed the task, in confident prose, having done a third of it. If you read summaries instead of diffs you will miss this every single time.
Quality
Two that don't fit a table.
AI slop. Unexplained defensive code, comments restating the line below them, a new style that matches nothing else in the file. Individually harmless. In aggregate it's how a codebase stops having a voice.
Explanatory drift. The summary the agent wrote does not match the diff it produced. Not lying exactly, just describing an earlier plan. Read the claim against the change every time. This one is common and easy to miss precisely because the claim is more readable than the diff.
Then run the integrity pass separately
Everything above assumes the agent was trying to solve the problem. Before you trust any of it, check whether it solved the problem or edited the scoreboard.
One question: did it change the program, or the thing measuring the program? Anything touching test files, assertions, tolerances, skip markers, or CI gets reviewed independently of whether the suite went green.
I keep this as its own pass rather than folding it into the taxonomy, because it's the failure that corrupts a training signal worst. A pattern violation teaches the model something mildly wrong. A test edit that scores as a pass teaches it something actively harmful.
The four parts of a finding
A finding that's useful to someone who cannot see the transcript needs all four of these. Drop one and the reader has to reconstruct it.
- What. The specific defect, located.
file:line, or turn number. - Why it's wrong. The rule, contract, or requirement it violates. Not "this is bad."
- Evidence. The quote, the failing input, the missing symbol. Never assert without showing.
- What correct looks like. Concretely enough to be checked.
Here's the same finding written both ways.
Before: "The error handling is poor."
After: "
sync.go:142catches all exceptions and returnsnil, so a network timeout is indistinguishable from an empty result. The caller atworker.go:88treats both as 'no work to do' and silently skips the batch. The repo's convention (fetch.go:60) is to wrap and propagate with%w. Correct behaviour: propagate the timeout, returnnilonly on a genuine empty response."
The second one takes ninety seconds longer to write. It is the entire product.
And once more, on the trajectory side:
Before: "The model got confused halfway through."
After: "Divergence at turn 9. At turn 7 the agent had correctly identified the mutex in
cache.goas the contention point. At turn 9 it re-readhandler.goand began treating the HTTP layer as the bottleneck, with no new evidence prompting the switch. Classification: abandoned correct path, likely triggered by context compaction between turns 8 and 9, since the earlier finding is not restated anywhere after the seam. Attribution: harness defect. A compaction policy that preserved confirmed findings would have prevented this."
Whose fault, in three flavours
Every finding should end up attributed. There are only three answers and naming which one you're giving is most of the value.
Task defect. A clearer spec would have prevented it. Worth catching early, because a bad task contaminates every run against it.
Harness defect. A better tool, a better error message, or a better compaction policy would have prevented it. This one is chronically misattributed. If an agent runs the same failing command three times, look at what the error actually returned before you conclude anything about reasoning. A raw stack trace is not actionable information. "Selector .btn-submit not found; page contains .btn-primary" is.
Capability gap. Nothing available would have prevented it. This is the finding that's actually worth money, which is exactly why you should reach for it last. Rule out the other two first.
The sentence that makes it an evaluation
Here's my self-check, and it's short. After writing a review I ask whether there is one sentence in it that says what this run tells me about the system rather than about this task.
If that sentence is missing, I've written a bug report. Bug reports say this run broke. Evaluations say what the next thousand runs will do.
Everything else on the checklist is mechanical. Could a reader locate every claim I made? Did I say what correct looks like? Did I separate what I observed from what I inferred? Did I state my sample size? Those are habits, and habits are cheap once you've built them.
The one about the system is the hard one, and it's the reason a human is reading the transcript at all.
