while we are at it, stop filling in the PR body with a mini novella of text generated by ai. they are hard to review and are unnecessarily verbose. the description should be there to benefit the reviewer.
If the whole thing is really all or nothing (very rare), at least break it up into sensible commits that can be reviewed individually.
>I'm tired boss. I'm tired of reviewing one, two, three thousand line PRs because some agent was able to "one shot the whole issue." Small PRs were never asked for because they're easier to write, it's always been for the benefit of the reviewer.
100%
but also "no" is a two letter word and one of the most important and hardest parts of being a maintainer.
I really believe people who publish huge slop PRs (short of being fired) should have their tokens taxed on the basis that it's an unpriced cost on the colleagues and the firm
Stumbled upon this the other day via my LinkedIn and now sanely look the jst code https://chromewebstore.google.com/detail/github-pr-focus/peb...
But the first thing I still check is consecutive comments and that goes very far as a signal whether the person sending it even tried to grok it or not
> why did you put it up for a human review at all then?
This seems to be the crux of the issue.
I'm guessing the most of the time, the answer is "because that's a mandatory gate to getting these changes into production". If the PR author doesn't see the value in review, it's going to be hard to convince them to write reviewable PRs.
If they're actually looking for human feedback, telling them how to submit PRs in a way that's amenable to human feedback is going to be a lot more successful.
Pretty much this. In OSS, review is mostly about convincing the others that your change is good and useful enough to merge. In corporate, it's seen as a blocker to change the ticket status to done. The vibe of the latter is mostly "it's working on my computer, approve it so that we can reach the quota for the sprint".
I would back up. If leadership is not committed to real reviews, it’s not your job to make them happen. Don’t try to fight an impossible fight no one cares about.
Personally, I would leave. But that’s not always an option for everyone.
> Small PRs were never asked for because they're easier to write, it's always been for the benefit of the reviewer
I think they were asked before AI and even they were not easier to write.
Its same as with commits. Usually when implementing a new feature I'm just in flow, so I don't think how to properly separate changes to different commits.
I mean - not always, but usually maintaining git history in a beautiful and clean manner was extra work even before AI.
Yeah the one and only time I attempted an open source PR, it was for a performance improvement for one small part of the software but touched a zillion files. After looking at the PR I decided not to submit it because it just looked like a mess and I didn't really know how to split that sort of thing up at the time. AI might make this sort of thing more common, but it's certainly not new.
Then you may need to improve your git-fu (or $vcs-fu). I use magit, so it's always easier to select only the lines/hunks/files that is for one specific change, stage and commit that. Before magit, I use sublime merge, Intellij vcs feature, and fugitive.
My flow state is for editing files. Once that's done and I've got something that work. It's always easy to convert those into sensible commits. Do not that the logs is not the like of "write database schema * add the index page * add the details page * add the new object form". They're more like "show the list of objects * allow object creation * show the details of a specific object". Those breaks to create the commits are more natural to the general flow state.
> If your variable is not named well and you need a comment, name your variable better.
100% agree. While you are at it, consider naming and writing your functions in such a way that doesn't require a wall of comments. Clean Code uncle Bob style.
Indeed. If you’re going to have an essay on top of a function or anywhere in code, earn the essay. That code better be operating on a ton of assumptions or using some creative logic to get to how it is that a simple reading doesn’t make sense.
I’ve done it myself on:
* engine definitions for complex workflows and DSLs
* heavy graph theory sections that included ASCII diagrams to clarify flow.
But those functions are probably 1 in 100 or rarer. Basically everything else is good enough with basic IDE-helping javadoc style comments at best, maybe with some input parameter clarification and business logic-clarifying 1-2 line comments sprinkled throughout.
A simple solution is to use a git hook that asks for confirmation if it is too big, with a suggestion to ask the user to have the agent split it up.
For OSS, my suggestion is to accept issues and specs do the implementation yourself. Warp.dev has a decent model of this in Github: https://github.com/warpdotdev/warp/blob/master/CONTRIBUTING....
Yes could be a pipe failure. Commonly used for coverage or security concerns, it could be also used for PR size.
There's only one way out of this predicament. AI reviews. It's what we have to do.
Who audits the reviewer?
Adversarial review is a thing, but it’s not the thing. Slop begets slop.
In my experience the models perform substantially worse if asked to create small PRs or commits. They lack the ability to sequence work and understand dependencies efficiently enough to manage it – it's not that they can't do small PRs, it's that doing them takes vastly more resources which then hits context limits etc. And if you want to then go back and edit a stack of commits or PRs, rebasing work into the middle, that's even more. I don't think any of this scales linearly in the amount of code or number of commits.
This is all in addition to the fact that the models are generally poor at storytelling, because that requires a theory of mind of the person you're communicating with. Authoring for review is storytelling, it's making changes in such a way as to build confidence in the reviewer. I believe current LLMs are still years away from this.
In my opinion, if you can't do these things, you're just cosplaying software engineering. Vibe coding has its uses, as does LLM programming, I do a lot of this! But we're kidding ourselves and dropping our standards dangerously low if we think that this is software engineering.
I've had no trouble getting small PRs. No idea if I'm doing something different than you or the things I asked for just happened to be small. Next time I get a big one I'll consider trying to ask the AI to break it up after it's finished.
Agreed. Asking for small PRs or commits can backfire, unless the work is deliberately scoped into smaller pieces from the beginning. This requires a human design review and planning and is one of the reasons I don't outsource that part to an agent.
I think LLMs are actually great at breaking up work into workstreams and tasks. The issue is that tasks do not equal good PR stacks on their own. You’ll need to use feature slices instead (like do backend part -> PR it, etc). LLMs can do that, but you need to harness them into it.
I mean, sometimes I don’t know how I want to write something until I’m finished. Huge refactors are often like this.
So, just like you said, rewrite the whole thing, THEN break it apart into bite size chunks that tell the story and feed it to others with acceptable and reasonable context.
It’s a skill that engineers need, and it pays dividends to all on the team, including you, when your coworkers ALSO start doing this back to you and you’re asked to review it.
This is exactly how I have always worked. I personally don't operate well breaking things up into small, bite sized PRs like people want. So instead I do the whole big project, and then break it apart into logical segments for my colleagues.
And this is often how I proceed when working with an LLM as well. We build the whole thing, and if I think it needs to be split apart, then in another session I work with the LLM to break it apart into reasonable chunks.
> sometimes I don’t know how I want to write something until I’m finished
This is knowledge that goes back to the beginning of software development - "Plan to throw [version] one away".
I think this could potentially become a good practice. LLMs make it so easy and cheap to just get it working and build that v1. Then you can play around with it and see if works and read the code about what could be better. Throw away the LLM generated version and now this is the part where human expertise comes in. Based on what you've learned from the v1, now guide the LLM more closely about how to write the thing and help guide it so that making small PRs that are easily reviewable and understandable are the output.
And then some of the time while breaking it up you realize you should have done it another way.
Yes, totally. This is how I work as well! Frankly, it seems like LLMs are pretty good if you tell them after the fact to divide work like this too, "make stacked prs with model a, then model b, then service c" has done wonders for my mental health.
Just an idea which I haven't personally tried: AI agents understand technical limitations, such as CI failures. Maybe make a CI job which checks that a PR has a reasonable size, and auto-reject with a polite message if it's not? Something like, "This PR size exceeds the limit of N lines that we accept for review; if you implement a big feature please consider splitting it in several smaller PRs." There are chances that it won't help, but it might!
That message could also be generated by a PR line count rule and string replacement without the ai overhead.
Edit - apologies I misunderstood which side the ai agent should be on.
Yeah, that's a fun way to get massive stacks of PRs that are individually incomprehensible.
How? If one PR builds off another, won’t either:
PR 1 is size 400
PR 2 is size 400 + 400 new
PR 3 is size 800 + 400 new
At least that’s how it’s worked on teams I’ve worked on that have soft size limits.
Size is an issue, but it isn't just about size. Ideally, agile development builds linearly in complexity. Rather than dumping a huge new feature, first introduce the building blocks and the reason you are introducing them, then the glue that ties them together, then the actual feature.
From what I've seen (not in software dev anymore, however I've been in it for close to 30 years), AI just tends to pile everything in, and it is very hard to review. No public model performs even average under the rules I've mentioned.
Also, simply breaking up a PR doesn't count if instead you dump all the PRs on maintainers at once. Humans are the bottleneck here, and can only review so much at once. If i were still involved in PR reviews, it doesn't matter if you gave me a single 4,000 line PR or 4 1,000 line PRs, I"d reject them.
What I want to see. Small, easily reviewable features with a build up to the main course, along with a good explanation for each. After that? I'd probably still reject it for a breach of code standards, or documentation, or because I don't like you sending me a PR at 4:59pm on a Friday. ;)
Humans also can't blindly rely on AI for review, so the models (more precisely, the folks building the underlying stuff) must adapt.
Then reject them for being incomprehensible?
Look, if you don't think code review is worthwhile, don't do it. Just give everybody unfettered permission to merge. But don't pretend to do review if you're not trying to maintain some standard of quality.
I do think code review is worthwhile, not sure how you read that from my comment.
A cap on PR size isn't inherently going to make an LLM do a good job of segmenting PRs. It requires careful prompting or manual action, the kind of effort typically exerted by people who already cared enough not to hit such a cap. You may as well just ditch the cap, to save yourself from having to reject a series of PRs rather than just the one.
I know someone working on a smaller open source who has same thing. They have considered just blocking all PRs outside known contributors because AI spam even on their tiny open source project is too much.
At work, I've gotten into fights about PR approvals. If they are beyond us humans to review, screw it, remove the approver requirement and if CI passes, merge it.
CI by itself is not got enough because LLMs are extremely good at writing vacuous tests that don’t actually test anything but look like the test something.
Even worse: they can write tests that make incorrect behavior part of your spec.
Tests matter.
Writing tests can be hard, boring, tedious. But if anything should still be written by hand in the age of LLMs it’s the tests. If you’re not looking at the application code anymore, you should at least be going over the tests with a fine toothed comb.
It's all we got at this point. Even as SRE, I just got 2000-line Golang change to something I think should be 150. However, the boss is already bouncing around happy we are going to deliver something that's been in Jira backlog for 9 months.
Why even bother then? Just feed Jira tickets into Claude Code and have it write the code, open the PRs have Claude in a GitHub action that does a code review on PRs, a routine that resolves the reviews, rebases the code and fixes conflicts and finally another that just merges anything that’s green in CI, no outstanding review and no conflicts. Then just spin in your chair whistling all day I guess. Surely your boss will be ecstatic.
Pretty sure that's what a group in my company is working on now.
Except, I won't be spinning in my chair, I'll be out of a job. At least until cost skyrockets and outages get much worse.