There is more to code review than (automatable) detection
adaptivecapacitylabs.com
adaptivecapacitylabs.com
I'm tech lead and I basically don't review PRs, and I tell people this, with a caveat - if you can tell me what you specifically want me to review, for what specific purpose, I'm happy to!
So "can you review this bit for race conditions" is great, love it. This forces people to actually think about what in their code they should be suspicious of, if anything.
"Can you review this" [link to PR] is getting a rubber stamp because humans have never been good enough at "just spotting bugs" to make this worth it and now any LLM is better than a human.
And for the purpose of understanding - PR time is too late. I have not reviewed a PR ("for real") in a long time and yet I could tell you how every system my people have built works down to a very fine level of detail. And it's because _we talk to each other!_ We don't just chill in the same slack channel and code independently, we all value each others brains and want each others inputs because we know it will improve our product and we value what perspectives others will bring.
Trying to learn via PR is a sad substitute for real collaboration and teamwork.
If PRs are a notable part of my architecture defense, I'm going to work on investing in the team instead of reviewing PRs.
Nice way to avoid the responsibility for any team fuckup: it's not me, I only teach them, they decide themselves.
Welcome to the tech industry. Its all about shifting responsibility in case something goes wrong. Thats the only reason companies use third party software in the first place, to have a scapegoat...
But also, I must defend against the hordes of unwashed masses and maintain the sanctity of my domain. End of the day, a codebase I own is a codebase I own, and others cannot be allowed to poison the well, intentionally or not. That’s how you get cholera
I don't quite understand how someone in charge of a software (techlead or similar title) can't take this serious; I know a few such people and I generally prefer to avoid working with them...
Sneding a PR should not be for understanding or just for rubber stamping. It’s about getting someone to look at your approach and helping you find flaws or proposing ideas that could make it better.
When I review PR, the primary question is: For the stated problem, is the diff a good solution? Sometimes I don’t know enough about the problem, so I just try to see if the code has glaring mistakes (mispellings, styles,…) but those are just comments, not suggestions.
Which, incidentally, is a really enjoyable way to work.
And arguably if the description needs linking to parts of the diffs then the commit is too large?
It stores issues as an immutable event log in your repo, so you can go back and inspect the context behind a change without having to litter the code with comments. Helps with traceability of intent.
As for finding bugs, what happens if you miss them? Do they go out to production and potentially lose user data? Finding bugs in PR is a big problem. For a start it shows your automated tests aren't good enough, and secondly it shows the devs aren't checking their code works well enough.
If you do them at all, PRs should be a gate for checking whether the code meets the team's quality bar, not if it even works. The team should be able to deliver working code without them.
Not all domains are like that. The majority of bugs I see are business/domain logic bugs. It's akin to having misunderstood or missed some aspect of the question, not causing data loss.
I lead a group of teams that build frontend software, so not really. :)
The majority of bugs I see are business/domain logic bugs. It's akin to having misunderstood or missed some aspect of the question, not causing data loss.
Financial loss, reputational harm, degraded UX, etc. They're all significant problems. They're more recoverable than a data loss, but equally bad from an accepted low quality standpoint.
I'm also going to guess that you don't have BAs, PMs, or people responsible for the logic reviewing the code in a PR. Consequently you can't spot those problems in at the PR gate unless the issue is that the dev didn't understand the requirements and wrote code that didn't do what it's supposed to. In which case we're back to the quality and testing problem. By raising questions in standup ("Can I clarify that I understand the AC right?"), pair programming ("Let's check the code against the AC") and communicating properly ("Can you demo the feature to the BA so we can be sure it's correct") you move the problem to the people who can answer, and stop the devs needing to review that someone wrote working code.
I just don't believe PRs are the right point to be finding out that the requirements were wrong or that the dev didn't understand what to build. That needs to happen as early as possible. PR is as late as possible.
At a much smaller company, you might find that a single person does part of the job of a BA, PM, and engineer, that they can produce a PR much more quickly as a result, and that it's more common for a PR to prompt the first detailed discussion about how something will work. A small team has quicker turnaround time on PRs and design, and can thus position in-depth reviews later in the process because less work will be thrown away in the case of a rejection.
An example of this might be adding load shedding. You could spend hours talking through it, or you could say "I'm going to add a load shedder to the blah service as a proof of concept" in standup then take an hour to implement it, and have the team critique it from there.
I agree that whether they're a useful gate is debateble, but they can be a useful means of expressing an idea to be approved or rejected.
This is a strong signal that you're looking at different things than a lot of people doing code reviews are.
- Why are you using this api for this instead of this other one? We use the other one because it avoids a specific issue.
- Your code isn't following the same patterns that we use in these places. It should be using the same patterns so that it's more obvious to anyone else that works on it
- The name you gave this function/class/whatever doesn't accurately represent what it is for
These are the kinds of things that are generally noticeable during a code review, and can make a big difference later on. And they're generally not going to be identified during standups and/or design.
Pairing is an option, since it is (effectively) code review _while_ writing the code (with a certain amount of blinders on, so not quite as effective). That being said, I hate pairing, so it's certainly not on my recommendation list.
Wait, does that mean that E2E tests that catch bugs are a strong signal that the team isn't doing well? How about component tests that catch bugs? Wouldn't they be better caught at the unit-test level? Do you see how your argument is flawed? - nobody is "waiting until PR to catch all bugs" but that doesn't mean that PR review can't/ shouldn't catch bugs! Sometimes even significant ones, yes.
You don't eliminate E2E tests because "you have good unit tests". You shouldn't just eliminate PR reviews because "we communicate inside the team".
Yes. E2E tests are there to give you confidence that future changes haven't broken things. They're not there to catch bugs before the feature goes to production. Unit and integration tests should do that though.
You shouldn't just eliminate PR reviews because "we communicate inside the team".
You should eliminate them as soon as they're not giving you any real value, but if you don't eliminate them before that team's will stop trying to get that value in a better way because they believe the PR process catches bugs. It doesn't though, so all it really achieves is stopping the team trying to improve.
> They're not there to catch bugs
What's the difference between those 2 things? I genuinely can't tell. Are you saying "E2E tests are only useful if they are *always* green"? No - of course they'd ideally be always green - but it they would be guaranteed to be green, only then they would become completely useless.
It's like saying "ideally you shouldn't write code with bugs in the first place". Sure. Ideally we shouldn't. Now back in the real world...
How would you restructure the approach to open source? Honest question.
Bugs usually include business logic edge cases, and significant problems include someone realising while reading the PR that we can actually create a much better solution to the underlying problem. Ideally that realisation would happen prior to the PR being offered, but that's also not really how humans work
Example: During a PR review for a graphical feature for a game, someone reading it realises that we can actually have a significantly better solution to the underlying problem. You can't catch this in testing, because it doesn't even make sense conceptually to test it. It also sucks that it happened after someone put in a lot of work, but with graphics development you expect a lot of what you write to get canned and replaced with a better solution, because the technology evolves over time. The work is iterative towards the final goal anyway
Its also very common to miss subtle edge cases with graphics hardware, eg someone misunderstood the intricacies of GPU hardware, or a team member has relevant experience that someone else does not have. Or they missed a problematic memory access pattern on some hardware for example. Or something simple like they've technically forgotten a barrier, that the validation layer doesn't report for some reason
Eg: The function "tanh" is broken on some AMD GPU hardware, and should never be used under any circumstances. The actual GPU implementation of it is just screwed. Ideally everyone would know this, but its a very common function to crop up during specific graphics algorithms (as it smoothly remaps the range [-inf, +inf] -> [-1, 1]). So occasionally I've spotted that, and had to explain that we need to use an approximation instead, and then now everyone knows . It rarely gets caught during testing setups, because people don't know they need to include that hardware in their tests in the first place
Someone who just rubberstamps PRs works either in a completely different setting than anything I can imagine or it's just someone who doesn't take ownership & responsibility as serious as I'd require people I want to work with; I can't quite see much room for gray area there...
> "Can you review this" [link to PR] is getting a rubber stamp because humans have never been good enough at "just spotting bugs" to make this worth it and now any LLM is better than a human.
Spotting bugs is the one thing we do have evidence that code inspection is good for. But there's a massive difference between the type of code review there's good evidence for and a github-style PR review, so it's mixed but not entirely without foundation.
And in response I wrote a non-exhaustive checklist of things that a code review can look for:
- Does it functionally achieve what it sets out to (as per tacker issue or PR description)?
- Does it have extraneous code? Leftover debug prints, private API keys etc...
- Does it have any obvious defects? Memory leaks, un-handled edge cases, security flaws, obsolete API calls, etc...
- Could it be more understandable? Add/remove abstractions, better variable/method names, more/less functional etc...
- Is the style consistent with the codebase and/or style guidelines?
- Are there obvious performance improvements? Hashset instead of list, lazy evaluations, etc...
- Is it sufficiently well tested?
I think LLMs are okay at most of these, and worst at the first.
- Is the change architecturally right?
Particularly the latter LLMs seem still pretty useless at.
Although I do think that LLMs have made it much easier to justify writing low-value code which can make this more common now.
But AI has engendered a collapse in developers’ ability to actually do that. Those of us who are stuck on the vibecoding bandwagon have lost the comprehensive understanding of the systems under our care that we need to understand and explain the quality and maintenance implications of a change.
Worse, if you happen to lose your mind and suggest the initial development cost is anything more than ~zero, your friendly neighborhood Claude keener will publicly shame you for not having sufficient faith in the Glorious Agentic Future. Product leadership will then have no choice but to side with them, not necessarily because they agree, but because they, too, are aware that we’re still in the phase of the hype cycle where openly questioning said hype is a career-limiting move.
Is there already a pattern or code on in in the existing codebase that handles this functionality,
Do we really need net new code to achieve this functionality?
Can existing code be extended or abstracted to more cleanly implement this feature or functionality.
I don’t think I have ever even once seen an LLM solve a problem related to overengineering by simply removing the overengineering. They always choose to add more epicycles and further compound the complexity.
I'll lay out a vague plan and let the AI fill in the blanks, mostly it'll get them right and where it doesn't I'll just tell it to do it differently and how. Works great for me.
Something akin to "meta-tests", which are not about testing the code itself, but the approaches taken by the implementation - i.e. architecture, understandability, terseness, etc.
These tests would operate on the source code level, even when testing code for a compiled language.
LLMs are worst at not realizing problems that I'd call "meta" problems. Here's one example to illustrate it:
I was allowed by my employer to work on a small project within the large collection of the projects which all constitute the product the company sells. Like a few dozens of other projects, it's written in Python. The company doesn't have any explicit policies about how Python projects have to be organized, it requires testing, linting, a CI code to package it etc, but the guidelines are very permissive. It just so happens that, beside the guidelines, there's a tradition: every other Python project in my company uses the typical Python bloatware, like masonry with a lot of insanity and mental flips going on in pyproject.toml, which is, in general, very typical for Python community at large.
My project used none of that. Instead, I wrote a ~100 lines setup.py file (no dependency on setuptools/distutils) that assembles the wheel and runs project maintenance tasks in the same way (interface-wise) things used to work decade or two ago (eg. "./setup.py test" if you want to run unit tests).
The AI reviewer didn't bat an eyelash. Found some typos in the comments, a problem with Base64 formatting, and generally OK'd the whole thing.
I knew I was on my way out. And I generally enjoy seeing people having a fit of rage when they know they are wrong (especially, together with many more like them), and scrambling for arguments that they know to be lies. I felt a little bit vindicated for the years of suffering I had to endure working with what might have been the dumbest and the most entitled manager I had in my life. :D
Anyways. My point is: the AI caught none of it. It was very happy with my approach to Python project management.
* * *
While my story is... more of an odd case, where this does have much wider implications is the AI-generated code. AI-generated code often fails to match these meta-requirements. I've seen AI reviewer OK'ing a PR containing AI-generated 10K loc Python file. I human would probably break after reading the first 1K lines. But AI doesn't get "tired", it just kept picking on typos in comments, criticizing short variables names etc. And there are other aspects in which AI-generated code is weird to humans in the ways that humans simply won't accept it, but AI reviewer would completely ignore as non-issue.
> Does this organization prioritize human learning?
That has been my primary motivator for code reviews. I want to teach and learn from others, especially given the decreasing levels of collaboration due to increased AI usage.
The sad truth is that all of my feedback just goes straight to agents. Maybe 10% is reacted to by a human, so I’m left wondering if there’s any value to a real review aside from poorly training robots to do my job, and further atrophying the abilities of my team members.
Previously the knowledge needed to discern that sort of thing would be disseminated through both design and code review sessions. But plan mode and AI code review largely put an end to that.
So we put our heads together and came up with some new policies about project management and how we use AI, and things have steadily getting better since then.
(Though, in fairness, the one guy who seems to actually enjoy getting paged after hours seems to be having less fun.)
This is actually in line with a well-established Six Sigma practice. When you've got a poorly performing value chain, the first thing to do is find out which step is struggling and then force the step immediately before it to slow down. If the reason why isn't obvious, search for a clip of the I Love Lucy chocolate factory scene on YouTube for a perfect explanation.
Engineers played along with this farce because code review served valuable team collaboration, coordination and management functions, about which the author of the article is correct.
Understanding a system by reading code is harder than understanding a system by writing code.
If AI can generate code at 100X, 1000X, or 10000X human capacity (no ceiling here), and you are gated on code review as your mechanism for system understanding, then a team's productive output will barely increase.
If companies want to compete in the world of AI generated code, human code review has to go. The only question is, what replaces it?
Continuing to apply human code review to AI generated code is negligent, if you are shipping at AI generation speed, with that as your only gate, and no other systems and processes to validate correctness and limit risk.
On the engineering side we can adapt easily.
Code review was never about finding bugs. When we do code review the first thing we check is: "do the tests pass?" Then we look at the change and the test coverage added for it and ask: "does the test coverage adequately demonstrate the functionality of the code?" The we ask: "What is the scope and potential impact of this change?" "What is the deployment and rollback plan and how will we monitor and detect defects after deployment?"
Code review was never about the code. It made the lawyers happy and provided a vehicle for doing the things that actually make systems work.
You’re going a bit hand wavy for an answer by redefining the term into something that fits what you’re promoting.
and is this a solved problem? If not, then the bottleneck is right here, if it is solved, then yeah we shouldn't need anymore software engineers other than the elites
yes. it was solved before but when writing code by hand the cost of building exhaustive test suites was far to high to do it in practice, except in very narrow cases where high assurance was required. now that AI can implement all of the testing frameworks for you it can be done for everything.
> we shouldn't need anymore software engineers
no. the job changes, but the skills that software engineers have are more valuable than ever because they now gate a much higher level of productive output.
corporations aren't really ruthless profit optimizers. micro incentives don't actually favor efficiency. hiring decisions don't actually have much to do with output and productivity. for example: it has been known forever that adding more people to a project usually decreases velocity, but that has never stopped anyone.
technology changes but people don't. AI makes higher quality software faster and at greater scale, and velocity is what is really valuable, so companies that master AI development will be making more money, and they will hire more people, because that is what they do.
Git did not exist either, I migrated out cvs to a beta release of Subversion. We only used branches for releases. We'd cut a branch just before a release. Test it (manually) and then ship. That was a process I helped put in place actually. After release, master would diverge quickly so back porting fixes was not really a thing. We'd support releases for as long as our customers used them. Often that involved just upgrading them to the recent version. We shipped when things were good enough.
I think the notion of people reviewing any meaningful amount of generated code is simply delusional. As you say, we do need alternative means to replace those checks. And a lot of that is going to be AI driven as well. AI driven testing, code reviews, and all the rest. Essentially all the stuff we used to do manually (poorly).
And we do have an important new tool as well: clean room code replacement. That used to be prohibitively expensive but now it's not. If you have something that is well specified through documentation, APIs, specifications, tests, etc. replacing it is fairly straightforward now. There are some early examples of people using LLMs to generate functioning replacements for things like Postgresql, browsers, compilers and similarly large and complex systems. While not perfect, these things seem to work, pass their tests, and generally not be completely horrible. It's only going to get better from here.
The notion that people are going to ever manually review code that was generated for such systems in mere hours/days is beyond imagination. How? When? Who? Why? It simply does not scale. It's only going to be more and more code. The amount of code no person will have ever looked at will soon dwarf the amount of code that is still manually inspected/created pretty rapidly.
This is a HUGE non-sequitur. It only follows if by "compete" you mean producing more LoC. How often does that translate to market fit or economic success?
This is the mindset that makes me want to leave this industry immediately. Somehow, an industry that already annoyed me with how much "mediocre is good enough" was an acceptable stance, with the emergence of LLM coding tools suddenly decided that "absolute dogshit is good enough" was just as acceptable, as long as everybody else is also fine with eliminating the few quality standards they might have had.
However, for some weird reason it's still in place. This is the part that actually concerns me. Ignoring the bullshit comment is trivial. The quiet and relentless accumulation of entropy is happening everywhere. This is why GitHub crashes at noon every business day.
I mean, depends on your reference point of course. Sometime around 2015 I participated in ICFP contest, where the task was in the code synthesis domain. At the time writing code that can generate basic arithmetical, well, forget it, even logical operations to implement some high-level description of a program was far out of hand. So, compared to that, CodeRabbit is light years ahead and is awesome beyond belief. But, compared to a trained human it still sucks.
Hearing conflicting reports on performance of AI aids, my attempt at explanation is that some problem domains have much better coverage. Essentially, the further away you are from "fullstack" the worse the performance is. So, maybe it does well on your end, it's because the project you work on is a well-researched problem that has many similar projects that help AI to distinguish the patterns it can then readily find and implement?
If you will tell me precisely what it is that my machine cannot do, then I can always prompt my machine to do just that.
- John von Altman
You may think you can pretend your way out of it, by, eg. copying human concerns onto AI's behavior, but it won't work because if the AI is "smart" enough, it will discover those to be a lie, and if it's dumb enough to get confused by it... then you will have an artificial stupidity instead of intelligence...
More so, you actually don't want AI that has value judgement, because, if, hypothetically, you have created one, then it might as well start fighting for resources against humans...
So, AI is bound to be limited in what it can do the more the decision belongs in the strategic or meta domain. And you very much want it to not grow a mind of its own, so that it doesn't write or approve code that optimizes AI's happiness, instead of yours.
Although AI is still way better in code reviews than in writing code. It make sense to use it as another automatic check in pipeline and it looks it will eventually be better.
Most of these people were deeply concerned that if we lean into using GenAI for “everything” that our collective knowledge will dissipate.
I was the vocal contrarian. There are many historical examples of humans obfuscating knowledge to simplify progress.
Does anyone solder their own microchips at scale anymore? No. We have highly sophisticated robots and machinery to do that work with extraordinary outcomes.
In software engineering, if you remove “coding” as a discipline you’re left with all the other aspects of designing software which I contend can be retargeted in college CS curriculum.
The leap isn’t about code reviews. It’s about design reviews and that’s where better outcomes are served regardless of whether GenAI is involved or not.
I have a roughly year old codebase at https://github.com/ChicagoDave/sharpee/ that is designed by me, but generated by Claude Code with my own skills and agents as guardrails. I’m fairly certain the code I extract from Claude doesn’t require human review, but the design of the system and its changes are continually reviewed by me.
My contention is that we “collectively” are still trying to discern where the AI/human line is and most are still “holding” that line to human interactions.
Let it go. Define what part you do need human decisions on and focus on those things.
That comparison doesn't work because up until now, it was only the execution we automated or optimised. Now, we're trying to outsource the understanding itself. No one hand-crafts a microchip, but we know how they're put together. We can debug and improve the process. The suggestion now is to just trust the magical statistics machine and not care that the artifacts become unfixable and unreproducible if and when the magical machine fails.
You can guard the the variance, test for quality, and still maintain design requirements.
I honestly didn't see as much improvement in code quality as people assume. Friends approve their friends PRs. Most coding cultures really push back on refusing a PR.
Automated testing in CI was the much bigger improvement, at least the code compiles now.
We have all the linters, tests, and AI writing code for us. I don’t need the left hand to tell the right hand it did a good job. I’m very certain my code runs when I push the PR.
What I need now is architectural, long-horizon and business perspective.
> What I need now is architectural, long-horizon and business perspective.
That's exactly what these tools are now good at. They have a huge gap when fixing these issues properly but they can spot these issues no problem
They also have shockingly weak ability to identify business acceptance criteria that are completely missing in the implementation or test coverage.
Lately I've seen some pretty glaringly obvious issues caught during the preliminary automated code review, and the issues seem to be coming from individuals who don't actually understand what the code is doing.
For the rest of us who are actually using the whole stack effectively, the automated code review is essentially a CI gate to protect the repo from the devs who don't know what they're doing.
But how does automated AI code review help, here? Doesn’t it just reinforce that they don’t need to look at it (or change their habits), because the AI review will catch the issues?
The author hints at the bidirectional aspect of code review, but they miss that each MR is teaching you how your contributors are getting confused.
I'm not interested in being a middle man between Claude and my colleague, nor am I interested in using a colleague as a middle man between myself and Claude.
I do agree with you
They talk about astractions as if "requirements -> code" is the same as "Go -> ASM -> bytecode". Which is just utterly stupid. But downvotes tell me a different story haha.
I'm also management btw, and the two startups I am a CTO at all have clear buy-in from other C-levels that we can't just spam AI slop w/o reading it and hope for the best. But I do see that most probably don't see it.
Wonder how it will age.
I'm a bit shocked they don't care much about the strangle contract this introduces by being so dependant on external models.
„The indent is wrong here“
„Comments should end with a period“
Because this kind of feedback is and was always easy.
Still not solved? Guess it was really about the commas and not the value delivered anyway, so do whatever you feel like.
What you actually need to do is have a manager lead assert that it’s happening in a top down way and just run the formatter with the defaults. Let people argue case-by-case on what to change after that.
Gpt-zero scores "human", and I've always found it to be a better judge
Something is missing in the new ai bot review paradigm we’ve all sleepwalked into.
I’ve been building Archme.io for this reason. PR reviews for the age of AI