8 ms·

You review it like it wasn't AI generated. That is: ask author to split it in reviewable blocks. Or if you don't have an obligation to review it, you leave it t

You review it like it wasn't AI generated. That is: ask author to split it in reviewable blocks. Or if you don't have an obligation to review it, you leave it there.

gpm 1y ago
Eh, ask the author to split it in reviewable blocks if you think there's a chance you actually want a version of the code. More likely if it's introducing tons of complexity to a conceptually simple service you just outright reject it on that basis. 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.
resonious 1y ago
This is it. The fact that the PR was vibe coded isn't the problem, and doesn't need to influence the way you handle it.
- gdulli 1y ago
  It would be willfully ignorant to pretend that there's not an explosion of a novel and specific kind of stupidity, and to not handle it with due specificity.
  - deleted 1y ago
    [deleted]
  - WalterSear 1y ago
    I contend that, by far and away the biggest difference between entirely human-generated slop and AI-assisted stupidity is the irrational reaction that some people have to AI-assisted stuff.
    - hatefulmoron 1y ago
      Calling things "slop" is just begging the question. The real differentiating factor is that, in the past, "human-generated slop" at least took effort to produce. Perhaps, in the process of producing it, the human notices what's happening and reconsiders (or even better, improves it such that it's no longer "slop".) Claude has no such inhibitions. So, when you look at a big bunch of code that you haven't read yet, are you more or less confident when you find out an LLM wrote it?
      - WalterSear 1y ago
        I have pretty much the same amount of confidence when I receive AI generated or non-AI generated code to review: my confidence is based on the person guiding the LLM, and their ability to that. Much more so than before, I'll comfortably reject a PR that is hard to follow, for any reason, including size. IMHO, the biggest change that LLMs have brought to the table is that clean code and refactoring are no longer expensive, and should no longer be bargained for, neglected or given the lip service that they have received throughout most of my career. Test suites and documentation, too. (Given the nature of working with LLMs, I also suspect that clean, idiomatic code is more important than ever, since LLMs have presumably been trained on that, but this is just a personal superstition, that is probably increasingly false, but also feels harmless) The only time I think it is appropriate to land a large amount of code at once is if it is a single act of entirely brain dead refactoring, doing nothing new, such as renaming a single variable across an entire codebase, or moving/breaking/consolidating a single module or file. And there better be tests. Otherwise, get an LLM to break things up and make things easier for me to understand, for crying out loud: there are precious few reasons left not to make reviewing PRs as easy as possible. So, I posit that the emotional reaction from certain audiences is still the largest, most exhausting difference.
        grey-area 1y ago
        clean code and refactoring are no longer expensive Are you contending that LLMs produce clean code?
  - rablackburn 1y ago
    > It would be willfully ignorant to pretend that there's not an explosion of a novel and specific kind of stupidity I 100% know what you mean, and largely agree, but you should check out the guidelines, specifically: > Don't be curmudgeonly. Thoughtful criticism is fine, but please don't be rigidly or generically negative. And like, the problem _is_ *bad*. A fun, on-going issue at work is trying to coordinate with a QA team who believe chatgpt can write css selectors for HTML elements that are not yet written. That same QA team deeply care about the spirit of their work, and are motivated by, the _very_ relatable sentiment of, you DONT FUCKING BREAK USER SPACE. Yeah, in the unbridled, chaotic, raging plasma that is our zeitgeist at the moment, I'm lucky enough to have people dedicating a significant portion of their life to trying to do quality assurance in the idiomatic, industry best-standard way. Blame the FUD, not my team. I would put to you that the observation that they do not (yet) grok what, for lack of a more specific universally understood term we are calling, "AI" (or LLMs if you are Fancy. But of course none of these labels are quite right). People need time to observe, and learn. And people are busy with /* gestures around vaguely at everything /*. So yes, we should acknowledge that long-winded trash PRs from AI are a new emergent problem, and yes, if we study the specific problem more closely we will almost certainly find ever more optimal approaches. Writing off the issue as "stupidity" is mean. In both senses.
    - watwut 1y ago
      I do not think that is being curmudgeonly. Instead, OP is absolutely right. We collectively used the strategy of "we pretend we are naively stupid and dont talk directly about issues" in multiple areas ... and it failed every single time in all of them. It never solves the problem, it just invites to bad/lazy/whatever actors to play semantic manipulative games.
- cespare 1y ago
  It is 1995. You get an unsolicited email with a dubious business offer. Upon reflection, you decide it's not worth consideration and delete it. No need to wonder how it was sent to you; that doesn't need to influence the way you handle it. 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.)
  - baq 1y ago
    Didn’t even hit the barn, sorry. Codegen tools were obvious, review assistance tools are very lagging, but will come.
    - eru 1y ago
      We already have some of them. And if you have a wide enough definition, we had them for a while.
  - resonious 1y ago
    The original ask was about one PR.
- lm28469 1y ago
  It 100% is. Why would I bother reviewing code you didn't write and most likely didn't read ?
- f311a 1y ago
  It's the problem. I often have to guide LLMs 2-4 times to properly write 150-300 LOC changes because I see how the code can be simplified or improved. 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.
- ericmcer 1y ago
  It is a huge problem. PR reviews are a big deal, not just for code reasons, but they are one of the best teaching tools for new hires. Good ones take time and mental energy. 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.
danenania 1y ago
I’m curious how people would suggest dealing with large self-contained features that can’t be merged to main until they are production-ready, and therefore might become quite large prior to a PR. 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?
- foxglacier 1y ago
  The partial implementation could be turned off with a feature flag until it's complete.
- arachnid92 1y ago
  The way we do it where I work (large company in the cloud/cybersecurity/cdn space): - 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.
- JonChesterfield 1y ago
  They come from people who have established that their work is worth the time to review and that they'll have put it together competently. If it's a newcomer to the project, a large self contained review is more likely to contain malware than benefits. View with suspicion.
- exe34 1y ago
  you line up 10-20 PRs and merge them in a temporary integration branch that gets tested/demoed. The PRs still have to be reviewed/accepted and merged into main separately. You can say 'the purpose of this pr is to do x for blah, see top level ticket'. often there will be more than one ticket based on how self-contained the PRs are.
userbinator 1y ago
If you try to inspect and question such code, you will usually quickly run into that realisation that the "author" has basically no idea what the code even does. "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.
- ahtihn 1y ago
  If it's objectively bad code, it should be easy enough to point out specifics. 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.
- charlieyu1 1y ago
  You are optimistic like the author even cared about the code. Most of the time you get another LLM response on why the code “works”
ashdksnndck 1y ago
If you ask them to break it into blocks, are they not going to submit 10 more AI-generated PRs (each having its own paragraphs of description and comment spam), which you then have to wade through. Why sink even more time into it?
- Buttons840 1y ago
  Being AI-generated is not the problem. Being AI-generated and not understandable is the problem. If they find a way to make the AI-generated code understandable, mission accomplished.
  - ashdksnndck 1y ago
    How much of their time should open source maintainers sink into this didactic exercise? Maybe someone should vibe-code a bot to manage the process automatically.
- mrweasel 1y ago
  There's probably also a decent chance that the author can't actually do it. 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.
  - 1y ago
ivanjermakov 1y ago
My record is 45 comments on a single review. Merge conditions were configured so that every comment must be resolved. If PR author can satisfy it - I'm fine with it.
- cryptonym 1y ago
  They will let AI somewhat satisfying it and ask you for further review
  - ivanjermakov 1y ago
    Reminds me of curl problems with vulnerability report: https://news.ycombinator.com/item?id=43907376 https://news.ycombinator.com/item?id=43907376 At that point it is just malicious.
    - cryptonym 1y ago
      Some people genuinely believe agentic coding works great and they mastered it. Someone who PR a simple feature with its own DSL probably is on that team and won't see the issue with their way. They may think you are too old and resist AI. They probably would tell you if that's too much for your old fashioned coding skills, then just use an agent for the PR. If you think that way, who cares about the code and additional DSL? If there is an issue or evolution required, we'll let AI work on it. If it works, just let it merge. Much cheaper than human reviewing everything. I hate it, maybe I'm too old.
latexr 1y ago
> Or if you don't have an obligation to review it, you leave it there. 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.

You review it like it wasn't AI generated. That is: ask author to split it in reviewable blocks. Or if you don't have an obligation to review it, you leave it there.

gpm 1y ago

Eh, ask the author to split it in reviewable blocks if you think there's a chance you actually want a version of the code. More likely if it's introducing tons of complexity to a conceptually simple service you just outright reject it on that basis. 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.

resonious 1y ago

This is it. The fact that the PR was vibe coded isn't the problem, and doesn't need to influence the way you handle it.

gdulli 1y ago
It would be willfully ignorant to pretend that there's not an explosion of a novel and specific kind of stupidity, and to not handle it with due specificity.
- deleted 1y ago
  [deleted]
- WalterSear 1y ago
  I contend that, by far and away the biggest difference between entirely human-generated slop and AI-assisted stupidity is the irrational reaction that some people have to AI-assisted stuff.
  - hatefulmoron 1y ago
    Calling things "slop" is just begging the question. The real differentiating factor is that, in the past, "human-generated slop" at least took effort to produce. Perhaps, in the process of producing it, the human notices what's happening and reconsiders (or even better, improves it such that it's no longer "slop".) Claude has no such inhibitions. So, when you look at a big bunch of code that you haven't read yet, are you more or less confident when you find out an LLM wrote it?
    - WalterSear 1y ago
      I have pretty much the same amount of confidence when I receive AI generated or non-AI generated code to review: my confidence is based on the person guiding the LLM, and their ability to that. Much more so than before, I'll comfortably reject a PR that is hard to follow, for any reason, including size. IMHO, the biggest change that LLMs have brought to the table is that clean code and refactoring are no longer expensive, and should no longer be bargained for, neglected or given the lip service that they have received throughout most of my career. Test suites and documentation, too. (Given the nature of working with LLMs, I also suspect that clean, idiomatic code is more important than ever, since LLMs have presumably been trained on that, but this is just a personal superstition, that is probably increasingly false, but also feels harmless) The only time I think it is appropriate to land a large amount of code at once is if it is a single act of entirely brain dead refactoring, doing nothing new, such as renaming a single variable across an entire codebase, or moving/breaking/consolidating a single module or file. And there better be tests. Otherwise, get an LLM to break things up and make things easier for me to understand, for crying out loud: there are precious few reasons left not to make reviewing PRs as easy as possible. So, I posit that the emotional reaction from certain audiences is still the largest, most exhausting difference.
      - grey-area 1y ago
        clean code and refactoring are no longer expensive Are you contending that LLMs produce clean code?
- rablackburn 1y ago
  > It would be willfully ignorant to pretend that there's not an explosion of a novel and specific kind of stupidity I 100% know what you mean, and largely agree, but you should check out the guidelines, specifically: > Don't be curmudgeonly. Thoughtful criticism is fine, but please don't be rigidly or generically negative. And like, the problem _is_ *bad*. A fun, on-going issue at work is trying to coordinate with a QA team who believe chatgpt can write css selectors for HTML elements that are not yet written. That same QA team deeply care about the spirit of their work, and are motivated by, the _very_ relatable sentiment of, you DONT FUCKING BREAK USER SPACE. Yeah, in the unbridled, chaotic, raging plasma that is our zeitgeist at the moment, I'm lucky enough to have people dedicating a significant portion of their life to trying to do quality assurance in the idiomatic, industry best-standard way. Blame the FUD, not my team. I would put to you that the observation that they do not (yet) grok what, for lack of a more specific universally understood term we are calling, "AI" (or LLMs if you are Fancy. But of course none of these labels are quite right). People need time to observe, and learn. And people are busy with /* gestures around vaguely at everything /*. So yes, we should acknowledge that long-winded trash PRs from AI are a new emergent problem, and yes, if we study the specific problem more closely we will almost certainly find ever more optimal approaches. Writing off the issue as "stupidity" is mean. In both senses.
  - watwut 1y ago
    I do not think that is being curmudgeonly. Instead, OP is absolutely right. We collectively used the strategy of "we pretend we are naively stupid and dont talk directly about issues" in multiple areas ... and it failed every single time in all of them. It never solves the problem, it just invites to bad/lazy/whatever actors to play semantic manipulative games.
cespare 1y ago
It is 1995. You get an unsolicited email with a dubious business offer. Upon reflection, you decide it's not worth consideration and delete it. No need to wonder how it was sent to you; that doesn't need to influence the way you handle it. 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.)
- baq 1y ago
  Didn’t even hit the barn, sorry. Codegen tools were obvious, review assistance tools are very lagging, but will come.
  - eru 1y ago
    We already have some of them. And if you have a wide enough definition, we had them for a while.
- resonious 1y ago
  The original ask was about one PR.
lm28469 1y ago
It 100% is. Why would I bother reviewing code you didn't write and most likely didn't read ?
f311a 1y ago
It's the problem. I often have to guide LLMs 2-4 times to properly write 150-300 LOC changes because I see how the code can be simplified or improved. 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.
ericmcer 1y ago
It is a huge problem. PR reviews are a big deal, not just for code reasons, but they are one of the best teaching tools for new hires. Good ones take time and mental energy. 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.

danenania 1y ago

I’m curious how people would suggest dealing with large self-contained features that can’t be merged to main until they are production-ready, and therefore might become quite large prior to a PR. 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?

foxglacier 1y ago
The partial implementation could be turned off with a feature flag until it's complete.
arachnid92 1y ago
The way we do it where I work (large company in the cloud/cybersecurity/cdn space): - 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.
JonChesterfield 1y ago
They come from people who have established that their work is worth the time to review and that they'll have put it together competently. If it's a newcomer to the project, a large self contained review is more likely to contain malware than benefits. View with suspicion.
exe34 1y ago
you line up 10-20 PRs and merge them in a temporary integration branch that gets tested/demoed. The PRs still have to be reviewed/accepted and merged into main separately. You can say 'the purpose of this pr is to do x for blah, see top level ticket'. often there will be more than one ticket based on how self-contained the PRs are.

userbinator 1y ago

If you try to inspect and question such code, you will usually quickly run into that realisation that the "author" has basically no idea what the code even does. "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.

ahtihn 1y ago
If it's objectively bad code, it should be easy enough to point out specifics. 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.
charlieyu1 1y ago
You are optimistic like the author even cared about the code. Most of the time you get another LLM response on why the code “works”

ashdksnndck 1y ago

If you ask them to break it into blocks, are they not going to submit 10 more AI-generated PRs (each having its own paragraphs of description and comment spam), which you then have to wade through. Why sink even more time into it?

Buttons840 1y ago
Being AI-generated is not the problem. Being AI-generated and not understandable is the problem. If they find a way to make the AI-generated code understandable, mission accomplished.
- ashdksnndck 1y ago
  How much of their time should open source maintainers sink into this didactic exercise? Maybe someone should vibe-code a bot to manage the process automatically.
mrweasel 1y ago
There's probably also a decent chance that the author can't actually do it. 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.
- 1y ago

ivanjermakov 1y ago

My record is 45 comments on a single review. Merge conditions were configured so that every comment must be resolved. If PR author can satisfy it - I'm fine with it.

cryptonym 1y ago
They will let AI somewhat satisfying it and ask you for further review
- ivanjermakov 1y ago
  Reminds me of curl problems with vulnerability report: https://news.ycombinator.com/item?id=43907376 https://news.ycombinator.com/item?id=43907376 At that point it is just malicious.
  - cryptonym 1y ago
    Some people genuinely believe agentic coding works great and they mastered it. Someone who PR a simple feature with its own DSL probably is on that team and won't see the issue with their way. They may think you are too old and resist AI. They probably would tell you if that's too much for your old fashioned coding skills, then just use an agent for the PR. If you think that way, who cares about the code and additional DSL? If there is an issue or evolution required, we'll let AI work on it. If it works, just let it merge. Much cheaper than human reviewing everything. I hate it, maybe I'm too old.

latexr 1y ago

> Or if you don't have an obligation to review it, you leave it there. 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.