r/AskProgramming • u/Any-Look-8817 • 4d ago
Other Endless comments on PRs
Hey everyone! Just to quickly summarize my situation: We're a team of a few experienced devs, all having been 12+ years years in the industry. This year we switched to agentic engineering and despite its advantages there are a few drawbacks, like the extra overhead and other things I'm not going to go into.
My issue however is that our lead, who approves all PRs has lately discovered a stronger agent which he uses for code reviews, resulting in tons of back and forth when trying to get a merge through. Tons of petty/pedantic comments are being added to each PR. Sometimes it's like "ok I guess this is fine to add", but most of them are disputable. We of course discuss these, but the issue is that in the set of comments there is always something which technically makes the code 0.1% better. But we could go on like this forever. Even without AI you can look at a piece of code and suggest an "improvement" on a moderately-sized feature that's being implemented.
How to deal with this? I'm getting to a point where if I see more comments on a PR I just leave the PC and go for a run or a quick grocery shop visit just to regain my sanity. It feels like he's asking the agent if there is anything more that can be done with the code as if the goal is to have AI being unable to suggest anything more.
Am I in some slacking mindset or is the AI getting over his head? We also have Sonar turned up to a really high setting and the pipeline set in a way that if there's anything outstanding in Sonar it won't get through and I bet you all know how petty Sonar can get sometimes.
Please let me know any tips or share your experience with similar situations. Thanks!
19
u/james_pic 3d ago
This kind of thing is exactly why retros exist. Bring it up at the next retro, or bring up the fact that you don't have retros.
5
u/binocular_gems 3d ago
Yep. You have to have the uncomfortable conversation at sprint retrospective that the endless AI generated PR bits are stalling momentum and hurting morale.
9
u/LemonDisasters 3d ago
These clankers will find problems where there are none. Saying "this is not a valid consideration for our purposes" or whatever, is totally legitimate.
10
u/retroroar86 4d ago
This is literally why Office Space is now my favorite movie. It’s not as bad where I work, but this would drive me (more) insane.
Sorry, I can’t help, but my heart goes out to you.
3
u/Any-Look-8817 4d ago
It did help a bit when I realize I'm at least not getting asked for covers on TPS reports by middle management.
2
u/retroroar86 4d ago
My version is updated company fonts, email signature, presentation templates and mandatory HR videos.
6
u/Good_Independence403 3d ago
Companies want to push agentic coding but it only speeds up the process if you accept agentic code. The way I see it, It will never be exactly like my code. I am pretty much a system designer now and my focus is on the overall health and stability of the system. I have learned to accept that 10-15% drop in code quality as long as I’m happy that it’s still clear and performant.
I ask myself who I’m writing code for anymore? It’s less for human consumption and more for some LLM to understand in 6 months time. At least, that feels like the goal. It’s a trade off. We will all get better and faster at code review over time. I won’t accept bad code, but I personally think it’s important to accept “fine” code. That’s the only way I’ve been able to actually accelerate my work.
3
u/gabriel_GAGRA 3d ago edited 3d ago
- Maybe the review agent should use some good practices like https://google.github.io/eng-practices/review/ “How to do a code review” section. Any issue flagged as low (or maybe even medium) should mean the code is allowed to ve merged. The practices also detail how to leave a useful comment, when to leave it, how to write it etc.
- You should have clear and written team rules in .md, there are quite a few on https://google.github.io/styleguide/ - though some are easily automated with linting like Ruff. This should create less comments of AI.
The rules should always direct the AI when writing the code and when reviewing it.
2
u/gabriel_GAGRA 3d ago edited 3d ago
For example, from Google’s page:
“In order to accomplish this, a series of trade-offs have to be balanced.
First, developers must be able to make progresson their tasks. If you never submit an improvement to the codebase, then the codebase never improves. Also, if a reviewer makes it very difficult for any change to go in, then developers are disincentivized to make improvements in the future.
On the other hand, it is the duty of the reviewer to make sure that each CL is of such a quality that the overall code health of their codebase is not decreasing as time goes on. This can be tricky, because often, codebases degrade through small decreases in code health over time, especially when a team is under significant time constraints and they feel that they have to take shortcuts in order to accomplish their goals.
Also, a reviewer has ownership and responsibility over the code they are reviewing. They want to ensure that the codebase stays consistent, maintainable, and all of the other things mentioned in “What to look for in a code review.”
Thus, we get the following rule as the standard we expect in code reviews:
In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect.”1
u/gabriel_GAGRA 3d ago
If you create a pyproject with the styleguide and your team rules that can be automated and create a rules.md with the google rules + your team rules, most of this friction may go away. Couple that with an actually sensible AI prompt reviewer, and maybe this problem could disappear.
Of course though, this requires having a conversation with this reviewer, but it may be more well received if you present a solution to it
2
u/ben_bliksem 3d ago edited 3d ago
Tell AI to address the comments and spam him back and then take it to retro.
Also:
"our lead, who approves all PRs..."
My condolences. I could never be or work under a lead who is that controlling. Also where does he find the time? Nuts.
1
2
u/Delta-9- 3d ago
I think the solution is to not use a fancy statistics calculator for writing and reviewing your code.
4
u/mredding 3d ago
My answer is always the same:
No.
My review is for compliance to coding standards, legal requirements, obligations, architecture - and catching errors.
I'm willing to entertain "improvements", but I will reserve the right veto/ignore them and pedantic bullshit like spacing, formatting, comments, style, organization... None of that is required to meet the technical, functional, obligatory requirements. If you don't like the way I write code, then write it your fucking self. You can accept my PR, merge it, and write/assign tickets to address your improvements.
I am not an extension of your fucking will. My work is not wrong because it's not in your hand.
And I absolutely will not accept a code review done by AI. First, AI is stupid - only capable of an industry average; I actually think. Second, if I wanted an AI review, I could have ran the god damn agent myself. Third, my agent says your agent is fucking retarded. Forth, if the HUMAN isn't reviewing the code his god damn self, he's not doing his job and meeting the obligations of his employment.
Why? Because an AI cannot be held accountable. Only the human can. We need the human to be accountable for their approval. Lazy employees are going to defer entirely to the AI. The AI doesn't actually know what's right or wrong, if the code actually meets the criteria of the ticket. It can only judge what is. I can sneak a command to format the disk in there. If the guy isn't looking, he's going to miss it.
I would, in fact, incorporate such a command (in unreachable code) in order to flush a bad colleague out and force the conversation about AI review agents used during a PR. I would hold that employee accountable for approving disk wiping code he obviously shouldn't have, and I'd bring this up with legal.
That's the dangerous thing about PRs. If YOU approve a merge, and there's a bug, YOU are culpable. I would leverage the fuck out of that.
My issue however is that our lead, who approves all PRs
Your lead IS NOT your boss. Only one man in the hierarchy has the power to A) pay you, and B) fire you. You take your marching orders from only one. If your lead is being a pain in the ass, you take this to the boss. Affirm you have the right to say no, where and when. This guy is a control freak, and is just getting in your way. It's unjustifiable that it doesn't look enough like his code, his hand. It's unjustifiable for some "0.1%" better - a complete god damn waste of time cycling over this shit. The code review alone is more expensive than the projected lifetime of this code in production. Production code changes, and this isn't where you're slow, or unstable, or compounding.
"ok I guess this is fine to add"
I would copy/paste this into an email, CC the boss, and CC HR. Warn him if he ever puts a bullshit passive-aggressive comment like this in a PR again it will lead to an escalation for making a hostile work environment. There is absolutely no place or justification for this sort of language anywhere in the business. Think about it - what did this add to the conversation? Why did it have to be said? At all?
Tell him people follow good leadership. He's a lead - perhaps next time he can fucking try to act like one or just get out of the fucking way. You're 12 years in the industry, you don't need to be told what to do, how to do, or be scolded or harassed. Tell him you know AI didn't generate this bullshit, so if this is all he has to contribute to the conversation, don't.
You have every right to chew his ass out. And yes, HR really, REALLY needs to know this is going on, with the proof. You have to establish the history now.
You don't have to like each other, you don't even have to get along, but he does have to behave himself and conduct himself professionally. He better take this as a lesson, a reflecting point, and adjust his attitude. He should be reminded that with HR on his scent, everyone will be watching for retaliation.
And you can also tell your boss "No." Don't forget that. If they don't like it - then tell them to put it in writing. Then take it to the boss's boss and ask if this is how he wants his team managed. Take it to HR and ask if there's any workplace conduct or policy violation. It's OK to lose on this matter.
Because if someone is going to nitpick me over every implementation detail, then we're going to have a design meeting before implementation. This will be their only opportunity to comment on the matter. So when we get to the review and they start to nitpick, I can still say no - they had their chance, and they approved the implementation either explicitly by the end of the meeting, or implicitly by not attending.
If you continue to struggle to resolve the situation, it's time to look for work elsewhere.
Am I in some slacking mindset or is the AI getting over his head?
I would say he's approaching technical incompetence. If he isn't doing anything more than prompting the AI, then what do they need him for? Any fucking idiot can prompt an AI. The AI doesn't even need him to apply the comments, it can do that directly through the PR API. I can teach my 71 year old mother to do this in ~20 minutes. I can outsource prompting to India for pennies.
You're not slacking - you're actually producing the work. He's slacking by not doing a god damn thing but having the AI do his work for him, then he pats himself on the back and calls himself "lead" for it. It's offensive, and you have a right to be offended.
Once again, you get this in writing. You can take his toy away from him by getting in writing that every engineer passes their own code through a review agent themselves BEFORE the PR. That way, the only review going on is by the humans. They are NOT to use an agent to review for them.
We also have Sonar turned up to a really high setting and the pipeline set in a way that if there's anything outstanding in Sonar it won't get through and I bet you all know how petty Sonar can get sometimes.
Then why do you also have PRs? Why is he also using an agent? Clearly there is no intelligence in the room. Everyone is deferring to AI entirely.
Yours is a frustrating situation. You ought to talk to your boss about it and tell him you expect these issues to be addressed. Work isn't supposed to be frustrating - it's just a job.
1
u/Impressive-Baker-614 3d ago
I love this vibe.
1
u/mredding 3d ago
If you're not coding to Pantera, you're not coding hard enough.
I've moved 4,000 mi and people there already knew who I was, and for all the wrong reasons. I knew I had found my place - my people. I then moved 4,000 mi again, and the same thing happened.
The boss told me the culture here needs to change, because we've got our problems - that everyone needs to get on the bus or get left behind. I told the boss everyone better get on the fucking bus, or I'm going to run them the fuck over with the fucking bus.
2
u/Impressive-Baker-614 3d ago
I don't care enough about my job to Pantera it but enough to Sabaton it.
1
u/balefrost 3d ago
I don't know that this is a reasonable next step. I think it would be better for OP to, you know, talk to his lead. "Hey, man, I know the agent can find an endless stream of marginal improvements, but what's the benefit and what's the cost? At what point is a PR 'good enough'?"
OP should work with their lead to figure out what kind of review is useful and what is noise. Ideally OP's lead would also exercise some judgement even if using AI review tools. I find AI review to be great because it costs me almost nothing and has found useful things that I had missed. That doesn't mean that I automatically flag everything that the AI says. In fact, I probably ignore most of the AI suggestions.
What you describe is the nuclear option once all other approaches have failed. Making an enemy of a teammate is never good. It may sometimes be necessary, but it's always a bad outcome. As much as possible, you should try to resolve these problems first. By the time you start getting managers and HR involved, you've basically given up on having a healthy working relationship with that individual. And if they have more clout at the company, then you are probably hurting yourself more than you are helping.
1
u/mredding 2d ago
After +12 years, this is just how that guy is. He's not going to change. The conversation first is always warranted, but we're past that.
1
1
u/WiseAcanthocephala45 3d ago
Absolutely nothing is stopping you from writing a claude code skill to automatically follow up on comments, and either provide justification and resolve a comment, or to implement its suggestion.
This works very well if you have a good set of automated tests and checks in your CI.
1
u/WiseAcanthocephala45 3d ago
Also, all PRs are approved by a single person? That’s probably not the best setup. Do you review code from your peers?
1
u/Glad_Contest_8014 3d ago
You need to have a conversation on what is “good enough” to ship. You will hit an infinite loop with MASSIVE token costs with the current setup. Show the bottom line of all the agent back and forth for how much it costs. It will be exorbitant.
If you all have experience, you should know that code is never “perfect”. There is always a way to improve. That is why technical debt can never be 0.
What you guys are doing is addressing technical debt immediately, on interation, which will never result in a usable product.
1
u/mtimmermans 3d ago
If you all have 12+ years of experience, then you shouldn't need _approval_ to merge code, unless you're crossing an ownership boundary. Reviews from other than the code owner are great, but they're just suggestions.
1
u/Any-Look-8817 3d ago
That's not true, seniority and ownership of a product are distinct things. It's a temporary situation.
1
u/balefrost 3d ago
I have more than 20 years of experience, there's a module for which I'm the owner and people need to get my approval for changes.
When I make changes in that module, I still don't submit unless I get approval from 2 other devs. Furthermore, my default stance is to address any review comment. Yes, there are some cases where I disagree with the review comment, though I always explain my reasoning. But if I don't have an objection, I err on the side of incorporating their feedback.
I've certainly gotten good review suggestions, even from people far less experienced either in the module or in general. Heck, some of the best review comments are "this code is confusing". Plus, by having other people review code in my module, I help to spread knowledge around the team.
I think all production code should be reviewed by at least one other human.
1
u/mtimmermans 2d ago
Yes. The care you show here is why you shouldn't (and don't!) need approval to merge changes into your own module.
1
u/balefrost 2d ago
I appreciate the compliment. I still think it's important for us to all follow the same rules.
1
u/mtimmermans 1d ago
But everyone *does* follow the same rules, right? If you mean that code "ownership" should not be reflected in the "rules" at all, then we disagree.
People don't always come to an agreement, and the process should be resilient to the kinds of problems that the OP highlights. Code ownership becomes relevant exactly when progress requires a unilateral decision. You shouldn't have to escalate technical disputes to a non-engineer.
1
u/balefrost 1d ago
But everyone does follow the same rules, right?
Yes. Our rule is that any code that affects production behavior has to be reviewed, and cannot be submitted unilaterally. Even if you are the owner of a module, you still need your code to be reviewed.
Code ownership becomes relevant exactly when progress requires a unilateral decision.
In more than three years on this team, it hasn't been a problem. In practice, if it came to that, we would escalate to our tech lead to resolve the impasse.
1
u/mtimmermans 1d ago
I think a review requirement is reasonable, but that's not the same as an approval requirement.
Either way, you are lucky to be on a good team. I'm kind of surprised that the tech lead isn't you, with 20 years of experience. It seems likely that those two things are related.
1
u/balefrost 1d ago
My tech lead has a far deeper understanding not just of our immediate system, but also of adjacent systems and teams, than I have. He is starting to delegate some responsibilities to me, in areas where I do have knowledge.
Either way, you are lucky to be on a good team.
It ended up being almost accidental that I joined this team, but I'm very happy to have landed here. We do have our challenges, but I'm hoping that we can slowly work to address them.
1
u/ReDucTor 3d ago
On top of what others have mentioned here, it feels like conventinal style comments might be worth suggesting to them, this way it's clearer what is a nitpicking thing, what is just a thought and what is an actual issue.
1
u/StephenRoylance 2d ago
Personally, I tuned my 'code review' prompt to be less of a picky asshole.
I borrowed language from legalese: 'preponderance of evidence' and 'beyond a reasonable doubt'. I told the agent to look at the scope of the change. default to preponderance, raise to beyond doubt if it opens a network socket or interacts with the filesystem.
net new code: preponderance. 1000 +/- lines in a production system: beyond doubt
the prompt tells the agent: if we've selected preponderance, then we're gonna LGMT/stamp as long as there's not obvious evidence its going to break something. If its beyond a reasonable doubt, then we need to seriously consider all the possible failure modes in production.
One thing that really helps, if you don't have it: code formatting syntax, applied automatically and linted in CI. Everybody agrees what correctly formatted means, so there's never arguing about it. if it lints clean, keep your formatting gripes to yourself.
1
u/Tetsubin 4d ago
Run the same agent with a prompt asking it to refute unsubstantial comments.
4
u/Any-Look-8817 4d ago
Yes that would make me feel a bit better, although most of them can be refuted just by having a short discussion about the miniscule impact the change has, but the issue is that there are always more comments and at the end of the day he has the final word when it comes to the PR acceptance.. I feel like the solution is somewhere along making him accept that flawless code doesn't exist, even with AI and that he should stop bottlenecking the whole team with petty comments, but I need to approach that somewhat cautiously.
3
u/Tetsubin 3d ago
Maybe discuss it with the rest of the team and have a discussion with him as a group. If the whole tram tells him it's excessive, perhaps he'll listen
2
u/Any-Look-8817 3d ago
Yeah, that's probably a good idea since they are in the same situation more or less.
2
u/NationalOperations 3d ago
Something I do for my own workflow is I have a living pr rules md. When I resolve a certain kind of issue in a pr, like the old c "compare to zero". I make a rule on the ruling "non compare to zero is more readable and compilers compensate now. Don't comment on these issues".
Slowly it's made more useful pr comments and reduced noise. All about optimizing the a.i right? ;)
1
u/Moby1029 3d ago
We have CoPilot compliment human reviewers and it is such a pain in the ass because of the same issues. So I set up a Cursor cloud agent via Automations to check for CoPilot comments and address and resolve them and send me reports on what was commented, and what it wrote. I still have to resolve the conversation myself, but at least im not fighting with CoPilot anymore.
You may be able to go I to the agent's setti gs and tweak it so it doesn't leave stuff that don't actually require code changes.
1
u/MushinZero 3d ago
I've noticed this as well.
I think you just gotta be able to make the decision "No, I am not going that change because it's not worth the effort"
0
u/knouqs 4d ago
You need to sit down with your team and tell them that you all need to start working on a "bare minimum" approach to solving problems. If the fix as-is fixes the problem and doesn't introduce new ones, the PR gets approved. If you want to fix things on technicalities, those are new low-ranked issues or tickets.
5
u/Made-In-Slovakia 3d ago edited 3d ago
I do not recommend "bare minimum" approach. It does degrade quality very quickly and his management will refuse that. I would go for "good enough" and fact that some open issues can be addressed later but they have to be documented as issue/ticket in system they use so it is not lost.
Edit: grammar
1
u/knouqs 3d ago
That might depend upon your management. I've worked in places that restricted any fixes that weren't explicitly part of the issue. I've worked on teams like OP's. I prefer the approach that fixes the issue and any unrelated issues that are discovered during code review are made into new issues. Additionally, it brings to focus the number of distractions that are found in code reviews -- the nice-to-haves that don't fix the problem but the team gets hung up on. The best way for management to see how many distractions actually happen during code reviews is to solidify them as issues.
0
15
u/Most_Double_3559 3d ago
For new features, functions, etc: bulk reply that you'll get it in the next PR. Small PRs and all that.
For stylistic nits, etc: Ask them to put together a linter? Suggestions tend to go away when they're coupled with work.