How we made our AI code review bot stop leaving nitpicky comments
greptile.com
greptile.com
Straight-forwardly true and yet I'd never thought about it like this before. i.e. that there's a perverse incentive for LLM vendors to tune for verbose outputs. We rightly raise eyebrows at the idea of developers being paid per volume of code, but it's the default for LLMs.
Something that has an impact on the long term maintainability of code is definitely not nitpikcky, and in the majority of cases define a type fits this category as it makes refactors and extensions MUCH easier.
On top of that, I think the approach they went with is a huge mistake. The same comment can be a nitpick on one CR but crucial on another, clustering them is destined to result in false-positives and false-negatives.
I'm not sure I'd want to use a product to review my code for which 1) I cannot customize the rules, 2) it seems like the rules chosen by the creators are poor.
To be honest I wouldn't want to use any AI-based code reviewer at all. We have one at work (FAANG, so something with a large dedicated team) and it has not once produced a useful comment and instead has been factually wrong many times.
1) This is an ego problem. Whoever is doing the development cannot handle being called out on certain software architecture / coding mistakes, so it becomes "nitpicking".
2) The software shop has a "ship out faster, cut corners" culture, which at that point might as well turn off the AI review bot.
I’ve found it’s nice to talk to an LLM about personal issues because I know it’s not a real person judging me. Maybe if the comments were kept private with the dev, it’d be more just a coaching tool that didn’t feel like a criticism?
This goes both ways. I worked for a company where the majority of PR comments were of the "Well, _I_ wouldn't do it this way." form. In some cases, the "way" they were complaining about were direct versions of examples in the language library docs.
One specific case was a PR was held up from merging because I used the plural of "regex" as "regexen" and not "regexes". IN A COMMENT. <eye roll>
That's why it is called Generative AI.
Classical ML people who recommended we try training a classifier, possibly on the embeddings.
Fine tuning platforms that recommended we try their platform.
The challenge there would be gathering enough data per customer to meaningfully capture their definition and standard for nit-pickiness.
It's like saying "don't comment on elephants" when you mean don't comment on the obvious stuff.
I would start with something like: 'Avoid writing comments which only concern stylistic issues or which only contain criticisms considered pedantic or trivial.'
Furthermore most of the code reviews I perform, rarely do I ever really leave commentary. There are so many frameworks and libraries today that solve whatever problem, unless someone adds complex code or puts a file in a goofy spot, it’s an instant approval. So an AI bot doesn’t help something which is a minimal non-problem task.
When it does manage to identify the areas of a PR with the highest importance to review, other problematic parts of the PR will go effectively unreviewed, because the juniors are trusting that the AI tool was 100% correct in identifying the problematic spots and nothing else is concerning. This is partially a training issue, but these tools are being marketed as if you can trust them 100% and so new devs just... do.
In the worst cases I see, which happen a good 30% of the time at this point based on some rough napkin math, the AI directs junior engineers into time-wasting rabbit holes around things that are actually non-issues while actual issues with a PR go completely unnoticed. They spend ages writing defensive code and polyfills for things that the framework and its included polyfills already 100% cover. But if course, that code was usually AI generated too, and it's incomplete for the case they're trying to defend against anyway.
So IDK, I still think there's value in there somewhere, but extracting it has been an absolute nightmare for me.
This, improper handling of exceptions, missed testing cases or no tests at all, incomplete types, a misunderstanding of a nuanced business case, etc. An automatic approval would leave the codebase in such a dire state.
I generally use MRs as an opportunity to give feedback like how you'd get feedback on a set of math problem statements. I inferred from "rarely do I ever really leave commentary" that you're not using MRs as a training tool. How else do you train junior engineers?
For context, I work in the financial industry where mistakes are costly and users are hostile so the "accept & merge" workflow may not be for me.
At some level if I didn't trust you to not write shitty code you wouldn't be on our team. I don't think I want to go all the way and say code review is a smell but not really needing it is what code ownership, good integration tests, and good qa buys you.
Code review isn't about preventing "shitty code" because you don't trust your coworkers.
Of course, if 99% of the time you don't care about the code review then it's not going to be an effective process, but that's a self-fulfilling prophecy.
* The work that is done is decided beforehand, the code in every PR corresponds to a card that's already been discussed.
* There's no incentive whatsoever to "sneak something in" and if you do so maliciously you'll be fired and maybe prosecuted depending on the damage.
* Your code goes through integration testing and QA so you'll never (immediately) take down prod.
* I have backups coming out my ears that assume the code running on our app servers is actively malicious so you couldn't cause data loss if you tried.
* Norms are all enforced in code, which makes discussions about style pointless. If it passes CI it's good enough.
How would you discover the snuck in code in a timely fashion?
Usually the issue is that the models have a bias for action, so you need to give it an accetpable action when there isn't a good comment. Some other output/determination.
I've seen this in many other similar applications.
Usually it comes down to one of the following:
- ambiguity and semantics (I once had a significant behavior difference between "suggest" and "recommend", i.e. a model can suggest without recommending.)
- conflicting instructions
- data/instruction bleeding (delimiters help, but if the span is too long it can loose track of what is data and what is instructions.)
- action bias (If the task is to find code comments for example, even if you tell it not to, it will have a bias to do it as you defined the task that way.)
- exceeding attention capacity (having to pay attention to too much or having too many instructions. This is where structures output or chain of thought type approaches help. They help focus attention on each step of the process and the related rules.)
I feel like these are the ones you encounter the most.
I recently signed up for Korbit AI, but it's too soon to provide feedback. Honestly, I’m getting a bit fed up with experimenting with different PR bots.
Question for the author: In what ways is your solution better than Coderabbit and Korbit AI?
E.g., I've never hesitated to add a linting rule requiring `except Exception:` in lieu of `except:` in Python, since the latter is very rarely required (so the necessary incantations to silence the linter are cheap on average) and since the former is almost always a bug prone to making shutdown/iteration/etc harder than they should be. When I add that rule at new companies, ~90% of the violations are latent bugs.
AI has the potential to (though I haven't seen it work well yet in this capacity) lint most such problematic patterns. In an ideal world, it'd even have the local context to know that, e.g., since you're handling a DB transaction and immediately re-throwing the raw `except:` is appropriate and not even flag it in the "review" (the AI linting), reducing the false positive rate. You'd still want a human review, but you could avoid bugging a human till the code is worth reviewing, or you could let the human focus on things that matter instead of harping on your use of low-level atomic fences yet again. AI has potential to improve the transition from "code complete" to "shipped and working."
Linters are useful. But you're arguing that you have such complex rules that they cannot be performed by a linter and thus must be offloaded to an LLM. I think that's wrong, and it's a classical middle management mistake.
We can all agree that some rules are good. But that does not mean more rules are good, nor does it mean more complex rules are good. Not only are you integrating a whole extra system to support these linting rules, you are doing so for the sake of adding even more complex linting rules that cannot be automated in a way that prevents developer friction.
How does AI fit into that picture then? The main benefits IMO are the abilities to (1) use contextual clues, (2) process "intricate" linting rules (implicitly, since it's all just text for the LLM -- this also means you can process many more linting rules, since things too complicated to be described nicely by the person writing a linter without too high of a false positive rate are unlikely to ever be introduced into the linter), and (3) giving better feedback when rules are broken. Some examples to compare and contrast:
For that `except` vs `except Exception:` thing I mentioned, all a linter can do is check whether the offending pattern exists, making the ~10% of proper use cases just a little harder to develop. A smarter linter (not that I've seen one with this particular rule yet) could allow a bare `except:` if the exception is always re-raised (that being both the normal use-case in DB transaction handling and whatnot where you might legitimately want to catch everything, and also a coding pattern where the practice of catching everything is unlikely to cause the bugs it normally does). An AI linter can handle those edge cases automatically, not giving you spurious warnings for properly written DB transaction handling. Moreover, it can suggest a contextually relevant proper fix (`except BaseException:` to indicate to future readers that you considered the problems and definitely want this behavior, `except Exception:` to indicate that you do want to catch "everything" but without weird shutdown bugs, `except SomeSpecificException:` because the developer was just being lazy and would have accidentally written a new bug if they caught `Exception` instead, or perhaps just suggesting a different API if exceptions weren't a reasonable way to control the flow of execution at that point).
As another example, you might have a linting rule banning low-level atomics (fences, seq_cst loads, that sort of thing). Sometimes they're useful though, and an AI linter could handle the majority of cases with advice along the lines of "the thing you're trying to do can be easily handled with a mutex; please remove the low-level atomics". Incorporating the context like that is impossible for normal linters.
My point wasn't that you're replacing a linter with an AI-powered linter; it's that the tool generates the same sort of local, mechanical feedback a linter does -- all the stuff that might bog down a human reviewer and keep them from handling the big-picture items. If the tool is tuned to have a low false-positive rate then almost any advice it gives is, by definition, an important improvement to your codebase. Human reviewers will still be important, both in catching anything that slips through, and with the big-picture code review tasks.
Many of these tools can be integrated into a local workflow so they will never ping you on a PR but many developers prefer to just write some code and let the review process suss things out.
That's the crucial bit. If it ends up hallucinating issues as LLMs tend to do[1], then it just adds more work for human developers to confirm its report.
Human devs can create false reports as well, but we're more likely to miss an issue than misunderstand and be confident about it.
[1]: https://www.theregister.com/2024/12/10/ai_slop_bug_reports/
It does however serve as a good first pass, so by the time the human reviewer gets it, the little things have been addressed and they can focus on broader technical decisions.
Would you want your coworkers to run their PRs through an AI reviewer, resolve those comments, then send it to you?
Once we have an AI starting to do something, there’s an art to gradually adopting it in a way that makes sense.
We don’t lose the ability to have humans review code. We don’t have to all use this on every PR. We don’t have to blindly accept every comment it makes.
The things I've seen juniors try to do because "the AI said so" is staggering. Furthermore, I have little faith that the average junior gets competent enough teaching/mentoring that this attitude won't be a pervasive problem in 5ish years.
Admittedly, maybe I'm getting old and this is just another "darn kids these days" rant
1. We are better at full codebase context, because of how we index the codebase like a graph and use graph search and an LLM to determine what other parts of the codebase should be taken into consideration while reviewing a diff.
2. We offer an API you can use to build custom workflows, for example every time a test fails in your pipeline you can pass the output to Greptile and it will diagnose with full codebase context.
3. Customers of ours that switched from CodeRabbit usually say Greptile has far fewer and less verbose comments. This is a subjective, of course.
That said, CodeRabbit is cheaper.
Both products have free trials for though, so I would recommend trying both and seeing which one your team prefers, which is ultimately what matters.
I’m happy to answer more questions or a demo for your team too -> daksh@greptile.com
This metric would go up if you leave almost no comments. Would it not be better to find a metric that rewards you for generating many comments which are addressed, not just having a high relevance?
You even mention this challenge yourselves: "Sadly, even with all kinds of prompting tricks, we simply could not get the LLM to produce fewer nits without also producing fewer critical comments."
If that was happening, that doesn't sound like it would be reflected in your performance metric.
I have seen this pattern a few times actually, where you want the AI to mimic some heuristic humans use. You never want to ask it for the heuristic directly, just create the constitute data so you can do some simple regression or whatever on top of it and control the cutoff yourself.
We’ve found that having the LLM provide a “severity” level (simply low, medium, high), we’re able to filter out all the nitpicky feedback.
It’s important to note that this severity level should be specified at the end of the LLM’s response, not the beginning or middle.
There’s still an issue of context, where the LLM will provide a false positive due to unseen aspects of the larger system (e.g. make sure to sanitize X input).
We haven’t found the bot to be overbearing, but mostly because we auto-delete past comments when changes are pushed.
[0] https://magicloops.dev/loop/3f3781f3-f987-4672-8500-bacbeefc...
We had it output a json with fields {comment: string, severity: string} in that order.
Effectively you are turning a somewhat arbitrary numeric “rating” task , into a multi label classification problem with well defined labels.
The natural evolution is to then train a BERT based classifier or similar on the set of labels and comments, which will get you a model judge that is super fast and can achieve good accuracy.
It should not substitute a human, and probably wasted more effort than it solves by a wide margin.
I would encourage you to try one (nearly all including ours have a free trial).
When done right, they serve as a solid first pass and surface things that warrant a second look. Repeated code where there should be an abstraction, inconsistent patterns from other similar code elsewhere in the codebase, etc. Things that linters can’t do.
Would you not want your coworkers to have AI look at their PRs, have them address the relevant comments, and then pass it to you for review?
God no. Review is where I teach more junior people about the code base. And where I learn from the more senior people. Either of us spending time making the AI happy just to be told we’re solving the wrong problem is a ridiculous waste of time.
If the comment could be omitted without affecting the codes functionality but is stylistic or otherwise can be ignored then preface the comment with
NITPICK
I'm guessing you've tried something like the above and then filtering for the preface, as you mentioned the llm being bad at understanding what is and isn't important.
What about PossiblyWrong, then?
So if the boss's nitpick is "you must pass your code thru an indicator that doesn't affect the output", I'm going to ignore it. I don't understand what point you're trying to make.
- Hilarious that a cutting edge solution (document embedding and search) from 5-6 years ago was their last resort.
- Doubly hilarious that "throw more AI at it" surprised them when it didn't work.
Wouldnt this be achievable with a classifier model? Maybe even a combo of getting the embedding and then putting it through a classifier? Kind of like how Gans work.
Edit: I read the article before the comment section, silly me lol
It's hard to avoid thinking of a pink elephant, but easy enough to consciously recognize it's not relevant to the task at hand.
I found this article surprisingly enjoyable and interesting and if like to find more like it.
Anything you do today might become irrelevant tomorrow.
As I see it, the solution assumes the embeddings only capture the form: say, if developers previously downvoted suggestions to wrap code in unnecessary try..catch blocks, then similar suggestions will be successfully blocked in the future, regardless of the module/class etc. (i.e. a kind of generalization)
But what if enough suggestions regarding class X (or module X) get downvoted, and then the mechanism starts assuming class X/module X doesn't need review at all? I mean the case when a lot of such embeddings end up clustering around the class itself (or a function), not around the general form of the comment.
How do you prevent this? Or it's unlikely to happen? The only metric I've found in the article is the percentage of addressed suggestions that made it to the end user.
For the typical team size that uses us (at least 20+ engineers) the number of downvotes gets high enough to show results within a workday or two, and achieves something of a stable state within a week.
The solution of filtering after the comment is generated doesn’t seem to address the “paid by the token” piece.
I took it as a comment on that generally models will be biased towards being nitty and that's something that needs to be dealt with as the incentives are not there to fix things at the origin.
You can run that locally.
wow. this is really expensive... especially given core of this technology is open source and target customers can set it up themselves self-hosted
For reference, IntelliJ Ultimate - a full IDE with leading language support - costs that much.
UPD: oh, you mean management will fire SWEs and replace them with this? well, yeah, then it makes sense to them. but the quality has to be good. and even then many mid to large size orgs I know are cutting all subscriptions (particularly per developer or per box) they possibly can (e.g. Microsoft, Datadog etc.) so even for them cost is of importance
> Giving few-shot examples to the generator didn't work.
> Using an LLM-judge (with no training) didn't work.
> Using an embedding + KNN-classifier (lots of training data) worked.
I don't know why they didn't try fine-tuning the LLM-judge, or at least give it some few-shot examples.
But it shows that embeddings can make very simple classifiers work well.
Nothing but advantages.