5 ms·
As someone who pushed ~4x the median PRs on my team before LLMs were a thing, I kind of think the problem here is PRs as a concept. Code review doesn't scale to
by swiftcoder 4mo ago
As someone who pushed ~4x the median PRs on my team before LLMs were a thing, I kind of think the problem here is PRs as a concept. Code review doesn't scale to prolific humans, it definitely can't scale to agents.
And the exact same things you would need to safely give up on PRs for human developers (auto-formatters, linters, comprehensive end-to-end tests, continuous deployment pipelines, etc), are also things that place meaningful guardrails on LLMs, and help them maintain a reasonable quality bar.
- jvanderbot 4mo agoGently, as long as you work with humans, you should consider yourself working _for_ those humans. Everyone needs shared state to work from, and that's just the cost of doing business. That said, sometimes low-trust environments are the issue, not PRs. In a higher trust environment, PR review is a helpful thing you usually desire, not dread.
- swiftcoder 4mo ago> In a higher trust environment, PR review is a helpful thing you usually desire, not dread Respectfully, in a high-trust environment, feedback should be delivered well before the PR stage. If you've let someone write a whole bunch of code without having a shared understanding of how the solution should work, you may have earlier process issues that PRs are papering over
- jvanderbot 4mo agoAgree. All the subtleties of how a high trust environment work are hard to enumerate
- jonahx 4mo agoDepends on how PRs function within teams. For some, the PR is a lightweight thing that is the preferred method of communication. It sounds like you are imagining a case where face to face communication, or communication over chat, is preferred for early stages, with the PR being a nearly final artifact. But it doesn't have to work like that.
- swiftcoder 4mo agoI think that's a valuable point. Especially as LLMs bring the cost of prototyping down (and reduce emotional investment in code written), it may be more viable to use PRs as proposals/sketches of a solution. With human reviewers, I find that by the time someone has churned out enough of a solution to post a PR, they are already quite invested in specifics of the solution, and it makes it emotionally costly (to both author and reviewer) when someone says "hey, I'm not a fan of this whole approach, lets start over and do it this other way"
- necovek 4mo agoI have seen many a PR where it is obvious it is an exploratory work: eg. figuring out how to use an external dependency that is imperfectly or incorrectly documented, etc. (You can claim this should be done ahead of time, but experience tells me you need to code it to learn it) The emotional toll there is real, but this is exactly the moment when you expose the knowledge of that external dependency to the unbiased party that is the reviewer. I like combining approvals to satisfy the urge for completion and closure, with a request for fast-follow refactor to better match the newly discovered model of interaction. (The worst code review experience I have seen is when a reviewer accepts it as-is and does a fast follow refactor themselves, depriving the author of the opportunity to learn and remain an expert in that area)
- teiferer 4mo agoAgreed. But those things are not mutually exclusive.
- rplnt 4mo agoYou cannot deliver feedback on something that doesn't exist. If you mean a review in the style of "all of this is wrong and needs to be rewritten differently" then yes, that's something to be discussed beforehand. But I don't imagine this is what people think of when discussing a review.
- IshKebab 4mo agoDepends on the change. Certainly most PRs don't need feedback before the PR is ready - the task is too obvious, and there's little to feed back on before there's any code. For bigger changes, of course you need feedback on designs. But that could easily be in the form of draft PRs. I definitely would push back on anything that required feedback before PRs. That's way too much process. Just going to slow you down for no benefit.
- necovek 4mo agoA discussion ahead of the implementation can also bias the two parties to that discussion and have them overlook the same implementation issue: many things you only understand once you start implementing. If you have these parties review each other's code, I agree that rarely brings much value. I think the best way to understand our experience with reviews is to stop and say: in a few sentences, what do you expect out of a quality code review? (sounds like nothing in your case, but I am curious)
- swiftcoder 4mo ago> in a few sentences, what do you expect out of a quality code review? (sounds like nothing in your case, but I am curious) From my perspective, there are three sorts of PRs: - One is very close to the final form of a particular change, and any feedback you get at that late stage is indicative of holes in your process. - Another is one where someone throws something up and says "hey, this is an experiment, can I get feedback on the approach". This is great, the parameters are clear, not much to say about these. - The 3rd sort is someone making a trivial 5-line patch to a makefile/cargo.toml/github workflow/etc. These add basically no value to anyone. Of those only the 2nd type really brings much value, and those are the ones that folks would keep posting even if you didn't require PRs (since they have an actual question, or a cool thing to show off). I'll also note that this only really negatively impacts small remote teams, because on a sufficiently large, co-located team, you just ask your buddy one desk over to rubber stamp all the trivial commits...
- necovek 4mo agoOn the first category, what is a process you use which has no "holes" in it? Does everybody produce completely readable, tested code every time? Perhaps that's just "style" to you when it is "maintainability" to me?
- swiftcoder 4mo ago> Does everybody produce completely readable, tested code every time? Do your coworkers not reliably produce readable, tested code? That's kind of the minimum bar for a software engineer in my book
- deleted 4mo ago[deleted]
- meta_gunslinger 4mo agoComprehensive end-to-end tests and CI can only attest to correctness, most engineers worth their salt won't review code only in regards to that aspect though.
- swiftcoder 4mo agoIn the bad old days before auto-formatters and linters, PRs were heavily used to enforce style guidelines. If we can enforce both style and correctness in our CI pipeline, what is actually left?
- skydhash 4mo agoCode architecture and technical design. You can have a solution that works fine, but are too complex or will impede future changes. Maybe you have code that has already been solved or your variables’ name are too generic. Maybe your modules are messy and your data structures are not modeled well.
- KptMarchewa 4mo agovibe check
- rplnt 4mo agoIf the correctness check was vibecoded there's a good chance it was cheated. So maybe that, on top of the, you know, code review (see the sibling comment). While PRs may have been used to correct style, that shouldn't have been their only or even main purpose. That's on whoever was using it that way, not on the concept of reviews.
- meta_gunslinger 4mo agoThe functionally correct code could be rejected in PR for many reasons other than style: 1. Solution under-engineered/over-engineered. 2. Code is hard to read or comprehend. 3. Design/Archtecture lacking. 4. Principles decided upon by team not adhered to. These are just some of the reasons I've rejected functionally correct code before. To summarize, in any software engineering course you learn that there are other metrics used to evaluate code other than correctness (maintainability, readability, scalability, portability, efficiency etc.)
- throwaw12 4mo ago> Code review doesn't scale to prolific humans, it definitely can't scale to agents. Then don't review the code. Ask Agents to review and merge it, also shift the responsibilities to the AI agents as well. If you think human is a bottleneck, then either optimize for humans, or remove humans. What's the problem?
- swiftcoder 4mo ago> If you think human is a bottleneck, then either optimize for humans, or remove humans. What's the problem? Sadly, in my case, it is the auditor. Our SOC2 documents have this lovely "every change has been reviewed by at least one other human", and it's going to be a fun battle to get that reworded
- throwaw12 4mo ago> Sadly, in my case, it is the auditor. Change your auditor and compliance, SOC2 is created for a trust between organizations employing humans, if you think agents can own the things, lead the way, introduce a new compliance, if companies sign up for it, then you will be the first who is removing the human bottleneck.
- wccrawford 4mo agoI think the "and merge it" is the problem in the above comment. If a coworker is creating a ton of AI-made PRs, I think the first step should always be to run an AI against them with the "assume this is low quality code and find all problems, big and small" text that was suggested in a comment here, and let that be the first line of defense. To keep the dev on their toes, each dev should come up with their own prompt for AI PR review and they can switch off who reviews it each time, until there are no problems remaining. Then a human can start to review it. It will quickly show the low quality code being produced and the massive waste of time it is for everyone, not to mention all the money spent on tokens for the whole process. Or it'll work, and everyone will have their way, and only have to review code that's pretty decent.
- throwaw12 4mo ago
- dust-jacket 4mo ago> Code review doesn't scale to prolific humans I've worked with people who consider themselves 'prolific humans'. Someone always has to tidy upp later, and its never them
- swiftcoder 4mo ago> I've worked with people who consider themselves 'prolific humans'. Someone always has to tidy upp later, and its never them I run both infrastructure and security - that means a lot of relatively self-contained PRs to infrastructure-as-code and dependency management systems. I'm also the team lead, which makes me responsible for a lot of throwaway prototyping, as well as cleaning up anyone else's mess... Yes, the prolific-but-damaging engineers are all too common in corporate. But particularly in startup land, you tend to find your high-performers wearing a lot of hats at once.
- my-next-account 4mo agoThere's also those that burn themselves out, and John Carmack!
- jagged-chisel 4mo ago> … and its never them IME, it’s because they lack the experience to have the Taste one develops as a senior engineer. “This works, and is somewhat understandable” is as far as they get. Little to no understanding of how this solution could fit better in the codebase.
- whstl 4mo agoThat's such bullshit. I've managed some incredibly prolific developers and some very slow ones, and the prolific ones are pretty much always the ones more available, more willing to fix things, more willing to take feedback. And also: they make less mistakes because their skills are sharp. This anecdote comes to mind: https://austinkleon.com/2020/12/10/quantity-leads-to-quality-the-origin-of-a-parable/ https://austinkleon.com/2020/12/10/quantity-leads-to-quality... If you have to constantly rationalize performance differences by demeaning others, this says more about you than the prolific people.
- PaulKeeble 4mo agoI have always considered Kent Beck understood this the best, the scaling for code reviews as you go to reduced release timeframes is to pair program, that brings the number of people reviewing it down but also increases the understanding for the reviewer. Comprehensive end to end tests are more a replacement for manual quality assurance for regressions. I am not sure there is a good analogue for reviews in the AI world. The human operating the AI should obviously review everything produced but that is clearly not as good as a second pair of human MK1 eye balls from pair programming.
- skydhash 4mo agoNo need to pair program, you can always send a message to your colleague about the design of the upcoming code, especially if it’s going to impact them or if it’s an area that they’re more familiar with. Waiting till a PR for feedback is wrong IMO. Code review is not for feedback, it’s for ensuring quality (many eyes on the output) and have a shared involvement in the evolution of the code. The time for feedback is earlier, once you have an idea of the solution.
- loglog 4mo agoWriting and reading design documentation can be slower than pair programming. On the other hand, info about code design also belongs into inline documentation or commit messages (in this order of preference), so the effort might not be wasted.
- skydhash 4mo agoI don't think so. There can be a lot of shared context within the team which can make prose shorter than writing code. And written words last longer than verbal exchange.
- samiv 4mo agoEither you were a head above the rest of the team and had the intellect to produce high quality value adding work, or then you were the "move fast break things" type of guy producing a lot of extra liability and work for others.
- meindnoch 4mo agoWell, it's either: 1. Your skills are >2 standard deviations above everyone else's. 2. You're fast at producing a lot of half-baked garbage, and your coworkers are too shy to confront you, so they just try to ignore it. (one of these scenarios is much more likely)
- swiftcoder 4mo agoAre PRs honestly helping with either case? Either you severely rate-limit your high-performers, or you drown everyone else in review, and both outcomes are bad for the overall team
- Tade0 4mo agoThe latter has an easy fix: the perpetrator is not allowed to take new work while there are pending review comments left unaddressed.
- neogodless 4mo agoBy perpetrator you mean the person postponing performing a code review? Right? Right?! Otherwise you place all burden on high performers to not only push PRs but babysit the rest of the team. It's not an easy fix, especially with AI letting people cosplay as high performers.
- rightbyte 4mo ago> you place all burden on high performers If their PRs don't get merged they don't perform. It is trivial to overload your coworkers with secondary tasks due to your "high performance".
- swiftcoder 4mo ago> If their PRs don't get merged they don't perform. It is trivial to overload your coworkers with secondary tasks due to your "high performance". We're all aware that a huge portion of the busywork that makes a team successful is not actually reflected in their upwards-facing deliverables (increasing test coverage, improving infra, adopting new tools/methodologies, preemptive security patching, etc). Your actual high performers, if you have any, are doing all that stuff in addition to their regularly-scheduled duties. If management weren't at least tacitly on board with this arrangement, your high performers would go work somewhere else. So my experience is that good managers don't tend to see this your way.
- teiferer 4mo ago> Code review doesn't scale to prolific humans If that's genuinely your attitude then your org has a problem. Code review is slow and less fun, for the average sw eng. But for high quality work it's indispensable. So treat code reviews as a scarce resource. Optimize for code reviewer time and attention. Have your PRs the right size? Are they well described? Do you give context? Do they fit in the bigger story? Do you mix in unrelated drive-by fixes? How easy is it to deal with you once you have received comments? Do you address them promptly? Do you give your reviewers credit (if not praise) for their help? Do you give back by doing code reviews yourself with high quality feedback? There are lot of things you can do to streamline things and give code reviews the place in a teams workflow that it deserves.
- bartread 4mo ago> Have your PRs the right size? I’ve noticed that large PRs aren’t just a problem for human reviewers: they’re a problem for AI reviewers too. If I submit a 100 line PR I’m likely to get some useful comments back from both humans and LLMs. In fact the LLM is likely to come back with so much feedback it gets down to the nitpicky/annoying level. If I submit 1000+ lines in my PR, the humans either don’t have time and/or get scrolling blindness, and the AI reviewer is likely to give me a response that amounts to, “<<slaps roof>> Looks good to me bro: ship it!” I guess they have a limited token budget for reviews so you can bamboozle them simply by blowing most or all of that budget.
- swiftcoder 4mo agoThe flip side of this tends to be that if 1,000 lines of code need to happen, filling the review queue up with 10x PRs each of 100 lines isn't exactly great either. The author spends a bunch of extra effort producing a raft of atomic PRs, and the reviewers get to context-switch a whole bunch (and may not end up with a clear picture of the feature end-to-end). I think the ultimate answer to this is a stacked PR workflow (which we had at Meta), where I can cheaply maintain/review a 1,000 line PR as a stack of 10 incremental PRs. But unfortunately GitHub et al are still not quite there on this one.
- fg137 4mo ago
- coldtea 4mo ago>As someone who pushed ~4x the median PRs on my team before LLMs were a thing, I kind of think the problem here is PRs as a concept. Code review doesn't scale to prolific humans Prolific humans should scale to the review/test/QA/staging backpressure - not just push to have whatever they produce accepted. Prolific is not a badge of honor, and "lines of code" is not a quality metric.
- ozim 4mo agoQuestions arise like, maybe instead of doing 4x PRs he could to 2x more code reviews and 2x more PRs still or even doing 3x more reviews. Why parent poster didn’t write anything about his involvement in reviewing the code - could he be just asshole team member ?
- epage 4mo agoAs a prolific PR author, I've found how I communicate has a major factor on how well and quickly people respond to PRs. I've recorded my lessons at https://epage.github.io/dev/pr-style/ https://epage.github.io/dev/pr-style/.
- anthuswilliams 4mo agoI have been championing this mindset since well before LLMs. It is an admittedly controversial opinion, but one I hold strongly. Code reviews are a productivity tax. No truly effective team would rely on them. The fact that so many software teams view them as indispensable just shows how few effective software teams there are in our industry. They are akin to a quality check step in manufacturing. Part of what Deming did in revolutionizing manufacturing was eliminating the step in favor of a holistic quality metric owned by all participants and enforced with rigorous statistical process controls. As you say, we in the software industry have all the pieces (autoformatters, tests, benchmarks, etc) to operate this way, but it seems our organizational and management dynamics combat this shift at every turn. Relevant: When this conversation comes up at work, I like to share Avery Pennarun's post about the review tax: https://apenwarr.ca/log/20260316 https://apenwarr.ca/log/20260316
- bobsomers 4mo ago> owned by all participants How does this work in practice? In my experience, any metrics owned by a group inevitably languish and are largely ignored. Anything you want to improve needs a DRI.
- anthuswilliams 4mo agoYou still have a DRI. In factories this would be a foreman; in software teams this could be a team lead or product owner or whatever. Their job is to apply the statistical process controls and the gemba walk, to help the team see the problems and develop the causal mental model for why the problems happen. They hold the team responsible, together, for combatting the issues so uncovered. They know who is not pulling their weight. Of course, to do that, a business, and in turn the DRI, would have to empower the team to act in its business's best interest and stop micromanaging them.
- WalterBright 4mo agoI had one contributor who would submit hundreds of lines of disconnected changes. One of his PRs was isolated as being the source of the bug. After some hours of work, I discovered that his actual semantic change was one line of code, and was the source of the bug. The rest was just reshuffling code around with no apparent purpose. At a recent meeting, the agenda was generated by LLM. About 20% of the action items were hallucinations.
- iovrthoughtthis 4mo agoCode review should be a separate function