163 points•utiiiD•4 days ago•115 comments•

115 comments

dimbletimbers3 days ago
A defense of human code review I wish I saw more often, especially in light of the concerns people have about cognitive/comprehension debt: comprehension redundancy. At the end, if taken seriously, at least two people understand how the feature works (even if that number is, on average, trending closer to between one and zero). Ideally at least one of the two also comes away with a better understanding of the wider system and how the feature fits into or stands out from that landscape.
atomicnumber33 days ago
If only one person knows the code, then PR time isn't going to save you.

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.

20k3 days ago
This is wild to read, I always review PRs and frequently find bugs or significant problems in them that get them bounced back
Shacklz3 days ago
PRs are the final chance to avoid mishaps; be it some junior overdoing DRY, be it someone in the team misunderstanding (or insufficiently understanding) a requirement, something being forgotten, edge-case missed - practically anything! Before/During PR, a single person owns the code, after merge it's everyone in the project.

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...

setr3 days ago
I consider PRs to be primarily a defense of the architecture, and to a lesser degree a general sanity check. It’s also a useful opportunity to enforce automations are being run
regularfry2 days ago
Just going to nitpick on one thing:

> "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.

infinitebit3 days ago
THANK YOU. I’ve always felt this way about PR review. I feel like people should write a natural language description of the change, and every section of it should link to part of the diff, and every part of the diff should be linked to by part of the description. That or just leave a comment on every chunk of the diff.
n4r93 days ago
There's been a lot of talk about the purpose of code review recently. It makes sense in the face of AI. Heres a link that was submitted a little while ago: https://mathstodon.xyz/@mjd/115096720350507897

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.

aeonik3 days ago
Missing my biggest issues as you ask the agents to do larger tasks with less up front planning.

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.

bunderbunder3 days ago
“Net new” is one it seems to be particularly bad at.

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.

anarazel3 days ago
- Do we want this? Cost/Benefit etc

- Is the change architecturally right?

Particularly the latter LLMs seem still pretty useless at.

birdatlaw3 days ago
The former feels more like a product leadership problem.

Although I do think that LLMs have made it much easier to justify writing low-value code which can make this more common now.

Boxxed3 days ago
Code review also transfers knowledge to the reviewers!
nnevatie3 days ago
I think we're kind of missing a layer of testing, that should sit above unit and integration tests.

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.

crabbone3 days ago
> I think LLMs are okay at most of these, and worst at the first.

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.

clintonb3 days ago
I am contemplating code review within my own organization, and the question I return to is:

> 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.

bunderbunder3 days ago
Well, I can tell you that at my current job we had something of a crisis over the summer when we realized that nobody could make changes without fear of breaking things anymore because we lost the ability to tell which existing behaviors were and were not safe to change.

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.)

beecasthurlbow3 days ago
What plans did you put in place? What did you do to ensure knowledge transfer?
metalspot3 days ago
The true reason why code review is universal is that it provides a liability shield for negligence. Negligence is interesting. It has nothing to do with whether or not you ship something broken. As long as you follow a process that attempts to not ship something broken, then you are not negligent.

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.

geraneum3 days ago
> 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?

You’re going a bit hand wavy for an answer by redefining the term into something that fits what you’re promoting.

Anamon1 day ago
> If companies want to compete in the world of AI generated code, human code review has to go.

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.

luisgvv3 days ago
I guess out in the wild vibe coders will tell you code review is replaced by "prompt review"
jillesvangurp3 days ago
People forget that pull requests and doing code reviews in the context of those is still a fairly recent thing. People did some code reviews before that of course but nowhere near as strictly. Same with testing practices, static code analysis, etc. Most of that wasn't all that common until beginning of this century. I remember using findbugs with Java around 2004. It actually found bugs in my code the first time I used it. No review had caught those. And we got lucky not finding them in production. But they were definitely bugs. Our system didn't have unit tests; it was all manual. Junit was a fairly recent system that hadn't been around for that long yet. Our build was done with Ant. There was no test phase. Our tech lead would of course check my work and correct & educate me (I learned a lot). But a lot of bugs slipped through as well.

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.

cyh5553 days ago
"does the test coverage adequately demonstrate the functionality of the code?"

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

metalspot3 days ago
> and is this a solved problem?

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.

ares6233 days ago
It's "largely solved". I'm not clarifying, thank you.
bob10293 days ago
One of my clients has an automatic "best practices" AI robot that runs each time you create a PR. It is pure downside. Even the developer responsible for creating it admits as much.

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.

crazybonkersai3 days ago
This does not reflect my experience. AI code review has improved tremendously in the last years to the point that nobody in my team does manual code review any more. It used to be ridiculously bad two years ago, but not any more.
griffiths3 days ago
Do you use some "framework" specifically or (custom) skills, or is it just plain prompt to review the code?
crabbone3 days ago
As recently as yesterday, CodeRabbit was absolutely ridiculously bad.

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?

alansaber3 days ago
Outdated/bad CICD is surely extremely common in every org

Read the full thread on Hacker News →

Related stories