Today I came across a PR for a (in theory) relatively simple service.
It span across 9000 LOC, 63 new files, including a DSL parser and much more.
How would you go about reviewing a PR like this?
Readit NewsReadit NewsIt span across 9000 LOC, 63 new files, including a DSL parser and much more.
How would you go about reviewing a PR like this?
AI doesn't directly make this stuff worse, it accelerates a team's journey towards embracing engineering practices around the code being written by humans or LLMs.
To start things off the meeting, I would say something like, "To me this is a surprising PR. I would expect it to be maybe 500(e.g.) lines including unit tests. Why does it need to be this complicated?"
If your manager just folds over, I would just accept it, because he's probably so beat down by the organization that he's not going to stick up for his employees anyway. At that point, it's time to look for another job.
But if the managers listen to their employees, and you have a better alternative, and your arguments are reasonable, it should be fine.
Usually within a few questions the answer "the AI wrote it that way" will come out.
Which feels bananas to me, like you don't understand how the code you PR is doing what you want? That would feel like showing up to work with no pants on for me.
You can use the LLM to generate as much documentation on the changes as you want. Just give it your PR. If someone tries to reject your vibe coded AI slop, just generate more slop documentation to drown them in it. It works every time.
If they push back, report them to their manager for not being "AI first" and a team player.
This is not new stuff, Goldratt warned us about this twenty+ years ago.
https://en.wikipedia.org/wiki/Theory_of_constraints
AI slop code is becoming the go-to way for a lot of written code. At some point it’ll make more sense to find a solution to the problem (“how to be confident in slop code”), rather than going against the financial motives of the business owners (cut expenses, maximize profit somehow through AI). I’m not sure if it’s right or wrong, but it is what it is.
What's so non-coop in this? This huge PR is a non-coop work and it requires to be fixed. Just imagine if someone locks the work of the whole branch just to review 9000 LOC, which they even didn't wrote. It's just no-go option
And even if there would be an AI which explains what this code is doing, people wouldn't be able to check it manually in reasonable time. It just enormous piece of work. So until there is a solution to this, such PRs should be declined and rewritten
Also it seems like the worker who brought this code doesn't know how evaluate the complexity of the task relatively to the solution, so there is a question about their qualification and level
I don't think I've ever worked in a company that would fire someone for something like that. Maybe you'd get a scheduled conversation to talk about it, to try to resolve whatever is going on (in this case verbose AI slop PRs), since obviously something is going wrong when people start engaging in malicious compliance.
But then I also never worked in a country where people can be fired for whatever reason, it's always been a legal requirement (in the countries I've lived at least) that the firing needs to be properly justified and explained, and firing someone like that wouldn't be justified.
Depends on the context. Is this from:
1. A colleague in your workplace. You go "Hey ____, That's kind of a big PR, I am not sure I can review this in a reasonable time frame can you split it up to more manageable pieces? PS: Do we really need a DSL for this?"
2. A new contributor to your open source project. You go "Hey ____, Thanks for your interest in helping us develop X. Unfortunately we don't have the resources to go over such a large PR. If you are still interested in helping please consider taking a swing at one of our existing issues that can be found here."
3. A contributor you already know. You go "Hey I can't review this ___, its just too long. Can we break it up to smaller parts?"
Regardless of the situation be honest, and point out you just can't review that long a PR.
The problem being that once someone has put together a PR, it’s often too late to go back to the serious thinking step and you end up having to massage the solution into something workable.
In life in general having the wherewithal to say no is a superpower. While I appreciate the concern about alienating newcomers, you don't start contributing to an existing project by adding 9k lines of the features you care about. I have not run any open source projects that accept external contributions, but my understanding in general is that you need to demonstrate that you will stick around before being trusted with just adding large features. All code is technical debt, you can't just take on every drive by pull request in hopes they will come back to fix it when it brakes a year down the line.
Refusing such a PR (which, again, is most of them) is easy. But it is also time consuming if you don’t want to be rude. Everything you point out as inadequate is a chance for them to rebut or “fix” in a way which is again unsatisfactory, which only leads to more frustration and wasted time. The solution is to be specific about the project’s goals but vague about the code. Explain why you feel the change doesn‘t align with what you want for the project, but don’t critique specific lines.
There are, of course, exceptions. Even when I refuse a PR, if it’s clear it was from a novice with good intentions and making an effort to learn, I’ll still explain the issues at length so they can improve. If it’s someone who obviously used an LLM, didn’t understand anything about what they did and called it a day, I’ll still be polite in my rejection but I’ll also block them.
Ginger Bill (creator of Odin) talked about PRs on a podcast a while back and I found myself agreeing in full.
https://www.youtube.com/watch?v=0mbrLxAT_QI&t=3359s
If you want to be really nice, you can even give them help in breaking up their PR.
For work? Close it and remind them that their AI velocity doesn't save the company time if it takes me many hours (or even days depending on the complexity of the 9k lines) to review something intended to be merged into an important service. Ask them to resubmit a smaller one and justify the complexity of things like a DSL if they wanted it included. If my boss forces me to review it, then I do so and start quietly applying for new jobs where my job isn't to spend 10x (or 100x) more time reviewing code than my coworkers did "writing" it.
Another equally correct approach (given the circumstances of the organisation) is to get a different AISlopBot to do the review for you, so that you spend as much time reviewing as the person who submitted the PR did coding.
In this job market? And where pretty much every company seems to be following the top-down push for AI-driven "velocity"?
Otherwise you need to be the person at the company who cuts through the bullshit and saves it from when the VibeCodeTechDebt is popping the industry.
Edit: left out that the user got flamed by non contributors for their apparently AI generated PR and description (rude), in defense of which they did say they were using several AI tools to drive the work. :
We have a performance working group which is the venue for discussing perf based work. Some of your ideas have come up in that venue, please go make issues there to discuss your ideas
my 2 cents on AI output: these tools are very useful, please wield them in such a way that it respects the time of the human who will be reading your output. This is the longest PR description I have ever read and it does not sound like a human wrote it, nor does it sound like a PR description. The PR also does multiple unrelated things in a single 1k line changeset, which is a nonstarter without prior discussion.
I don't doubt your intention is pure, ty for wanting to contribute.
There are norms in open source which are hard to learn from the outside, idk how to fix that, but your efforts here deviate far enough from them in what I assume is naivety that it looks like spam.
“AI Slop attacks on the curl project” https://youtu.be/6n2eDcRjSsk
There is no way that 9000 lines of code are decent. It's also very hard to review them and find bad spots. Why spent your time in the first place? It probably took one hour for a person to generate it, but it will take ten to review and point out hundreds (probably) problems.
Without AI, no one would submit 9000 lines, because that's tens of hours of work which you usually split into logical parts.
No. We need spam filters for this stuff. If it isn't obvious to you yet, it will be soon. (Or else you're one of the spammers.)
Why would I bother reviewing code you didn't write and most likely didn't read ?
Asking me to review a shitty PR that you don't understand is just disrespectful. Not only is it a huge waste of everyones time, you're forcing me to do your work for you (understanding and validating the AI solution) and you aren't learning anything because it isn't your work.
Possibly you reject it with "this seems more suitable for a fork than a contribution to the existing project". After all there's probably at least some reason they want all that complexity and you don't.
"review it like it wasn't AI generated" only applies if you can't tell, which wouldn't be relevant to the original question that assumes it was instantly recognisable as AI slop.
If you use AI and I can't tell you did, then you're using it effectively.
After pointing out 2-3 things, you can just say that the quality seems too low and to come back once it meets standards. Which can include PR size for good measure.
If the author can't explain what the code does, make an explicit standard that PR authors must be able to explain their code.
While it would be nice to ship this kind of thing in smaller iterative units, that doesn’t always make sense from a product perspective. Sometimes version 0 has bunch of requirements that are non-negotiable and simply need a lot of code to implement. Do you just ask for periodic reviews of the branch along the way?
- Chains of manageable, self-contained PRs each implementing a limited scope of functionality. “Manageable” in this context means at most a handful of commits, and probably no more than a few hundred lines of code (probably less than a hundred tbh).
- The main branch holds the latest version of the code, but that doesn’t mean it’s deployed to production as-is. Releases are regularly cut from stable points of this branch.
- The full “product” or feature is disabled by a false-by-default flag until it’s ready for production.
- Enablement in production is performed in small batches, rolling back to disabled if anything breaks.
Are you hiding them from CIA or Al-Qaeda?
Feature toggles, or just plain Boolean flag are not rocket science.
If it's a newcomer to the project, a large self contained review is more likely to contain malware than benefits. View with suspicion.
It is about ownership to me. I own my PRs. If I throw garbage out and expect you to fix it I am making you own my PRs. No one wants to be forced to own other peoples work.
Sometimes I get really into a problem and just build. It results in very large PRs.
Marking the PR as a draft epic then breaking it down into a sequence smaller PRs makes it much easier to review. But you can solicit big picture critique there.
I’m also a huge fan of documentation, so each PR needs to be clear, describe the bigger picture, and link back to your epic.
Let's say it's the 9000 lines of code. I'm also not reviewing 900 lines, so it would need to be more than 10 PRs. The code needs to be broken down into useful components, that requires the author to think about design. In this case you'd probably have the DSL parser as a few PRs. If you do it like that it's easier for the reviewer to ask "Why are you doing a DSL?" I feel like in this case the author would struggle to justify the choice and be forced to reconsider their design.
It's not just chopping the existing 9000 lines into X number of bits. It's submitting PRs that makes sense as standalone patches. Submitting 9000 lines in one go tells me that you're a very junior developer and that you need guidance in terms of design and processes.
For open source I think it's fine to simply close the PR without any review and say: Break this down, if you want me to look at it. Then if a smaller PR comes in, it's easier to assess if you even want the code. But if you're the kind of person that don't think twice about submitting 9000 lines of code, I don't think you're capable of breaking down you patch into sensible sub-components.
Don’t just leave it there, that reflects badly on you and your project and pushes away good contributors. If the PR is inadequate, close it.
If PR author can satisfy it - I'm fine with it.
That alone should be the reason to block it. But LLM generated code is not protected by law, and by extension you can damage your code base.
My company does not allow LLM generated code into anything that is their IP. Generic stuff outside of IP is fine, but every piece has to flagged that it is created by an LLM.
In short, these are just the next evolution of low quality PRs.
Accepting code into the project when only one person (the author) knows what it does is a very bad idea. That's why reviews exist. Accepting code that zero persons know what it does is sheer screaming insanity.
But for any open source or enterprise project? Hell no.
that's the point though, if they can't do it, then you close the ticket and tell them to fork off.
Having spent some time vibe coding over the weekend to try it out, I disagree. I understand every line of code the super-specific Android app I generated does, even if I don't have the Android dev experience to come up with the code from the top of my head. Laziness is as good a reason to vibe code as inexperience or incompetence.
I wouldn't throw LLM code at a project like this, though, especially not in a PR of this size.
“This PR is really long and I’m having a hard time finding the energy to review it all. My brains gets full before I get to the end. Does it need to be this long?”
Force them to make a case for it. Then see how they respond. I’d say good answers could include:
- “I really trieeld to make it smaller, but I couldn’t think of a way, here’s why…”
- “Now that I think about it, 95% of this code could be pushed into a separate library.”
- “To be honest, I vibe coded this and I don’t understand all of it. When I try to make it smaller, I can’t find a way. Can we go through it together?”