4 ms·

One of my co-workers just asked me to review his pull request that was all AI generated. 600 files were touched, over 40k lines of code added. I'm sure he thou

by malfist 5mo ago

One of my co-workers just asked me to review his pull request that was all AI generated. 600 files were touched, over 40k lines of code added. I'm sure he thought that was a crowning achievement, proof that AI can enable 10X developers, after all, what engineer could write 40k lines of code in a week? I declined to review it, stating that I couldn't possibly vet 40k lines of code, and wouldn't put my reputation on the line to stamp the work as good. The PR nagged me for 2 weeks from my todo list and then disappeared. I don't know if he found another dev to get an approval from, or if the PR was abandoned. But I know for sure that him and I are on two totally separate islands around the value of LLMs.

CamperBob2 5mo ago
I declined to review it, stating that I couldn't possibly vet 40k lines of code Gee, that sounds like a job for Claude if there ever was one.
- malfist 5mo ago
  You're absolutely right!
- danparsonson 5mo ago
  And how would you verify that the review was accurate?
  - sebasv_ 5mo ago
    Same way that I would trust your review to be accurate. Because the reviewer has built a reputation for correctness. Its not Claude doing the review. Its a human doing the review, but using Claude to do the reading. Its still on the human to ask the right questions to Claude.
    - kalaksi 5mo ago
      For large changes that are not straightforward and include architectural decisions, I wouldn't trust Claude enough to not read most of the code myself. I'll have to read it to be able to understand it and ask about the decisions in detail anyway. And when I start to understand it, it's not uncommon to find out that the solution can be improved and simplified in many places, and after iterating, 25-30% of code disappears. And trying to just hand-wave it to Claude, to somehow "improve it" or "simplify it", without detailed questions hasn't been very successful. It can work for some things, though.
    - danparsonson 5mo ago
      So you'd produce the code using Claude, and then use Claude to verify it? Would you accept my review of my own code?
      - fragmede 5mo ago
        Depends. Do you take pills that let you forget that you wrote the code so you can review that same code with fresh eyes that haven't seen that code before? Though you could just use ChatGPT to review the code that Claude wrote if that's really the issue.
  - altmanaltman 5mo ago
    Claude to the rescue
    - Towaway69 5mo ago
      Claude Van Damme ? I'd prefer Chuck to the rescue but I guess it's a cultural preference. /s
  - aetch 5mo ago
    You can use Claude for that
  - TeMPOraL 5mo ago
    Ask it to prove it. My approach for AI-first code review, or really any kind of AI technical opinion, is that if the claim AI made is both important and not obviously true at a glance, it has to prove it to me, and keep trying until I'm convinced or can spot an obvious mistake in the proof. With reviews, this is usually the case where AI is making a claim that something in the PR will fail because of some assumptions or behaviors in code outside of the PR - e.g. "this change will fail in scenario X, because foo is null in this case, because the SQL query doesn't populate it when bar == quux, and it gets propagated as null through the JSON deserialization (optional field)...", where all the SQL and JSON parsing was not part of the code under review, and "bar == quux" is some weird domain special case. Stuff like this is both critical, and there's no way for me to judge it without an expensive context switch. So I learn to ask for a more detailed walk-through once, and if that doesn't make me "see" it, I just ask it to reproduce it with tests, and confirm it's a real problem. Reviewing the reproduction is usually enough for me to either "see it" or accept they're probably right and ask the author to recheck it. (Why not jump straight to "reproduce it" for every finding? Because it still takes time to have AI do the repro. It's cheaper than a deep context switch, but not free.)
- not_that_d 5mo ago
  At work we had copilot. It said "the diff is too big to review"
squidsoup 5mo ago
That's a process problem at your company - no developer should be proposing branches over 1k loc (or whatever your agreed tolerance threshold is) without a very good reason, vibe coded or not.
- rstuart4133 5mo ago
  > no developer should be proposing branches over 1k loc I've seen that reaction many times. It seems to work well enough when someone is maintaining existing code. However, greenfield projects can often require literally orders of magnitude more code to deliver something that can be integration tested. The first step is to break it up into a stack of commits. Each one must compile and pass its unit tests, of course. Keeping it under 1k loc of released executable code is usually easy, but often becomes difficult to impossible if you want well commented code with excellent unit test coverage. Assuming you have kept all your commits under 1k loc, there is still the problem of whether you present them in one PR, or as a stack of PRs. The issue with a stack is why an API is designed a certain way often isn't evident until you see how it's used. Responses to PR comments are explanations that point to later PRs in the stack, which is irritating for both the reviewer and the author. I haven't found a good solution. I'm not sure there is one.
  - torben-friis 5mo ago
    I mean, you're not creating the api from thin air as you write code right? Usually you'd have some larger doc about the project with the design you can point to.
- esailija 5mo ago
  It isn't about small or big, it's about cohesion of the changes. I prefer a big feature to be one big PR rather than a lot of small ones. We had a dev do a big feature with a ton of small PRs, each one was individually impossible to review because each concern was out of scope for the small PR and "would be fixed in later PRs". Once it all came together as as whole, the big picture was a total horror show and I had to rewrite basically the whole thing. In order to review those small PRs properly, each time I would have to read and understand all the current code so far from the beginning. Without that, each small PR individually looks OK because you won't remember the other PRs from weeks back that already duplicated what the current small PR does for example.
fg137 5mo ago
Same here. A co-worker touched a few hundred files in a PR and asked us to review. They merged it directly to main when nobody approved it. (The repo was not set up to enforce PR approval.) I don't personally use that feature, and I couldn't care less at this point. If our customers are frustrated by the bugs, at least my name is not on it.
- triyambakam 5mo ago
  The challenge is that you may not have customers, and thus a job, if that continues
  - LtWorf 5mo ago
    His job will be to fix all the mess that other people created.
- matltc 5mo ago
  Crazy they merged into main holy moly
afro88 5mo ago
This is a branching point. One dev would find someone else and convince them to approve it. Another would redo the task (code is cheap now, right?) in a PR stack that can actually be reviewed, cleaned up etc. I hope they were the latter.
SpaceNoodled 5mo ago
You could surely check on the status of that PR.
LtWorf 5mo ago
A former coworker sent me an AI generated PR to review and I just said NAK after the first two issues I found and I said to not send me AI slop to review. They went to HR who said I am more senior and I should act as a mentor (they had my same work title and were probably making 4x more due to being in USA) and I just no longer reviewed anything from them until I changed jobs.
- fg137 5mo ago
  People actually go to HR for such trivial things?
  - LtWorf 5mo ago
    All the time. And HR are generally bored and enjoy some drama.
otikik 5mo ago
My review would have been along the lines of: 'Please split this PR into smaller ones'. I would even sketch which groups/phases would make sense, perhaps with the help of AI.

by malfist 5mo ago

CamperBob2 5mo ago

I declined to review it, stating that I couldn't possibly vet 40k lines of code Gee, that sounds like a job for Claude if there ever was one.

malfist 5mo ago
You're absolutely right!
danparsonson 5mo ago
And how would you verify that the review was accurate?
- sebasv_ 5mo ago
  Same way that I would trust your review to be accurate. Because the reviewer has built a reputation for correctness. Its not Claude doing the review. Its a human doing the review, but using Claude to do the reading. Its still on the human to ask the right questions to Claude.
  - kalaksi 5mo ago
    For large changes that are not straightforward and include architectural decisions, I wouldn't trust Claude enough to not read most of the code myself. I'll have to read it to be able to understand it and ask about the decisions in detail anyway. And when I start to understand it, it's not uncommon to find out that the solution can be improved and simplified in many places, and after iterating, 25-30% of code disappears. And trying to just hand-wave it to Claude, to somehow "improve it" or "simplify it", without detailed questions hasn't been very successful. It can work for some things, though.
  - danparsonson 5mo ago
    So you'd produce the code using Claude, and then use Claude to verify it? Would you accept my review of my own code?
    - fragmede 5mo ago
      Depends. Do you take pills that let you forget that you wrote the code so you can review that same code with fresh eyes that haven't seen that code before? Though you could just use ChatGPT to review the code that Claude wrote if that's really the issue.
- altmanaltman 5mo ago
  Claude to the rescue
  - Towaway69 5mo ago
    Claude Van Damme ? I'd prefer Chuck to the rescue but I guess it's a cultural preference. /s
- aetch 5mo ago
  You can use Claude for that
- TeMPOraL 5mo ago
  Ask it to prove it. My approach for AI-first code review, or really any kind of AI technical opinion, is that if the claim AI made is both important and not obviously true at a glance, it has to prove it to me, and keep trying until I'm convinced or can spot an obvious mistake in the proof. With reviews, this is usually the case where AI is making a claim that something in the PR will fail because of some assumptions or behaviors in code outside of the PR - e.g. "this change will fail in scenario X, because foo is null in this case, because the SQL query doesn't populate it when bar == quux, and it gets propagated as null through the JSON deserialization (optional field)...", where all the SQL and JSON parsing was not part of the code under review, and "bar == quux" is some weird domain special case. Stuff like this is both critical, and there's no way for me to judge it without an expensive context switch. So I learn to ask for a more detailed walk-through once, and if that doesn't make me "see" it, I just ask it to reproduce it with tests, and confirm it's a real problem. Reviewing the reproduction is usually enough for me to either "see it" or accept they're probably right and ask the author to recheck it. (Why not jump straight to "reproduce it" for every finding? Because it still takes time to have AI do the repro. It's cheaper than a deep context switch, but not free.)
not_that_d 5mo ago
At work we had copilot. It said "the diff is too big to review"

squidsoup 5mo ago

That's a process problem at your company - no developer should be proposing branches over 1k loc (or whatever your agreed tolerance threshold is) without a very good reason, vibe coded or not.

rstuart4133 5mo ago
> no developer should be proposing branches over 1k loc I've seen that reaction many times. It seems to work well enough when someone is maintaining existing code. However, greenfield projects can often require literally orders of magnitude more code to deliver something that can be integration tested. The first step is to break it up into a stack of commits. Each one must compile and pass its unit tests, of course. Keeping it under 1k loc of released executable code is usually easy, but often becomes difficult to impossible if you want well commented code with excellent unit test coverage. Assuming you have kept all your commits under 1k loc, there is still the problem of whether you present them in one PR, or as a stack of PRs. The issue with a stack is why an API is designed a certain way often isn't evident until you see how it's used. Responses to PR comments are explanations that point to later PRs in the stack, which is irritating for both the reviewer and the author. I haven't found a good solution. I'm not sure there is one.
- torben-friis 5mo ago
  I mean, you're not creating the api from thin air as you write code right? Usually you'd have some larger doc about the project with the design you can point to.
esailija 5mo ago
It isn't about small or big, it's about cohesion of the changes. I prefer a big feature to be one big PR rather than a lot of small ones. We had a dev do a big feature with a ton of small PRs, each one was individually impossible to review because each concern was out of scope for the small PR and "would be fixed in later PRs". Once it all came together as as whole, the big picture was a total horror show and I had to rewrite basically the whole thing. In order to review those small PRs properly, each time I would have to read and understand all the current code so far from the beginning. Without that, each small PR individually looks OK because you won't remember the other PRs from weeks back that already duplicated what the current small PR does for example.

fg137 5mo ago

Same here. A co-worker touched a few hundred files in a PR and asked us to review. They merged it directly to main when nobody approved it. (The repo was not set up to enforce PR approval.) I don't personally use that feature, and I couldn't care less at this point. If our customers are frustrated by the bugs, at least my name is not on it.

triyambakam 5mo ago
The challenge is that you may not have customers, and thus a job, if that continues
- LtWorf 5mo ago
  His job will be to fix all the mess that other people created.
matltc 5mo ago
Crazy they merged into main holy moly

afro88 5mo ago

This is a branching point. One dev would find someone else and convince them to approve it. Another would redo the task (code is cheap now, right?) in a PR stack that can actually be reviewed, cleaned up etc. I hope they were the latter.

SpaceNoodled 5mo ago

You could surely check on the status of that PR.

LtWorf 5mo ago

A former coworker sent me an AI generated PR to review and I just said NAK after the first two issues I found and I said to not send me AI slop to review. They went to HR who said I am more senior and I should act as a mentor (they had my same work title and were probably making 4x more due to being in USA) and I just no longer reviewed anything from them until I changed jobs.

fg137 5mo ago
People actually go to HR for such trivial things?
- LtWorf 5mo ago
  All the time. And HR are generally bored and enjoy some drama.

otikik 5mo ago

My review would have been along the lines of: 'Please split this PR into smaller ones'. I would even sketch which groups/phases would make sense, perhaps with the help of AI.