A local model as a code reviewer: what it catches, and what it quietly misses

AINico3 repliesThis topic is about Aider-AI/aider

NicoMember#1

We tried running a small local model as a review step on every pull request. It is a good idea for a specific reason and a bad one for a specific reason, and both are about the same thing: what the model is looking at.

What it was good at

Style and consistency. Naming, import order, the house pattern for error handling, the twelve places in the codebase where the same helper is written slightly differently. These are pattern-matching tasks over a small set of conventions, and a local model is entirely capable of them.

Obvious mistakes in the diff. An off-by-one, a missing null check, a variable that is used before it is set. The diff is in front of it, the diff is small, and these are the same class of error a linter would catch, except the model catches the ones a linter cannot express.

The comment nobody wrote. "This function is called on every request and does a database query each time" is a comment about intent, and only a reader who has seen the call site can write it.

What it missed

Anything outside the diff. This is the big one. A model that sees the changed lines does not know that the function it changed is called in a loop, that the value it trusts is user-controlled, that the file it edited is generated. It has the patch, not the system.

Why the code is that way. The comment explaining the workaround for the bug in the old API is in another file, three commits back. The model will suggest a fix that reintroduces the bug, confidently, because from the diff the old API is unused.

Cross-file consistency. A new function in one file and a nearly identical one in another, one of which is now wrong. A model reviewing one file at a time cannot see that.

The configuration that made it work

Give it the call sites, not just the diff. The gap between "it sees the diff" and "it sees the system" is the gap between a linter and a reviewer, and closing it is a matter of what you put in the context.

Ask for specific questions, not "review this". "Does this change the behaviour of the caller" gets a better answer than "any issues", because the second one produces a list of style remarks and the first one produces a reason.

Make it say "I do not know". A review that returns "nothing to add" is worth more than one that returns four remarks of which three are noise, because the reader has to check every remark.

The honest summary: a local model is a good cheap first pass and a bad last pass. The mistakes it misses are the expensive ones.

3 replies

IcarusRegular#2

The "it has the patch, not the system" line is the whole thing, and it is worth adding the cheap version of closing that gap.

A stack trace of the failing test, and the test file, are more useful context than fifty files of the repository. In a bug-fix review the caller is the most valuable file in the context and it is almost never in the diff.

ValerieRegular#3

In reply to Icarus (#2)

On the "why the code is that way" problem: a git log on the function, or the commit message that introduced it, is worth more than the surrounding source.

The reason a piece of code looks strange is usually in a commit message from two years ago, and no amount of reading the current tree will tell you.

NicoAuthorMember#4

In reply to Valerie (#3)

Both are the same principle, which is that context has to be chosen, not accumulated.

We ended up with a rule: the diff, the call sites of what changed, and one commit message. Three things. Every time we added a fourth kind of context the review got slower and no better, which is the same lesson as the token arithmetic elsewhere.