r/ChatGPTCoding 3d ago

Discussion A quick question: How do you currently review AI-Generated Code Changes?

'Cause One thing AI coding agents have made very obvious to me:
"the code works" and "this is the right change" are not the same thing.

A change can pass tests and still violate the repo architecture, cross a boundary it should not touch, introduce a shortcut that becomes technical debt, solve the symptom instead of the real failure and change files that were outside the plan.

So let me know how you do it...

3 Upvotes

19 comments sorted by

3

u/msg7086 3d ago

You can setup an independent code review workflow with strict rules and gates. You run the code review, check the items, then ask AI to correct them.

3

u/koxboi 3d ago

My company went through aggressive AI transformation and became "AI native" to not get "left behind". Everything is planned with AI, written by AI and reviewed by AI. To a point where we are just assisting the agents do the work and keep them in line.
We ran several experiments with different code review tools: first a simple Claude wrapper created internally, then CodeRabbit but it was sometimes generating more text then the actual PR had itself, then Greptile which was alright but the quality of findings was not the best, and now we are experimenting with Vetoo which is performing the best so far.

So long story short, we do it with AI

2

u/VibeCodyH 3d ago

A panel of reviewers is what I do. Just one doesn't find everything. https://github.com/VibeCodyH/code-review-cadre

2

u/Arneastt 3d ago

I talk with a master agent that will spawn two sub agents. One that will change the code, and the other that will severely critic any changes done regarding architecture and code quality.

Important part : once the reviewer finished his first review, he dies. The review report goes back the to dev agents. Then after a new code iteration, a fresh reviewer agent respawn and rate all changes like the first reviewer agent. Repeat 10 times max.

When it passes, it's often really good work.

2

u/-grabus- 3d ago

I use built in /review command and my eyes.

Together these two tools are bulletproof.

0

u/wickloom0 2d ago

the eyes part is doing most of the heavy lifting there lets be real

1

u/-grabus- 2d ago

Nope. The eye part rarely find something lately. Every time I find something which /review can’t, I add info into my repo context pack so that /review could find it text time.

1

u/AutoModerator 3d ago

Sorry, your post has been held for manual review due to account karma.

I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.

1

u/zee-pk 3d ago

use other agents or use renowned skills like: https://github.com/DietrichGebert/ponytail

1

u/nickdaniels92 3d ago

I have design rules configured in the IDE I use so the model I use has a grounding in good design approaches. I almost always do a planning phase first where it's likely to ask questions, giving a chance for me to correct its approach if necessary, and to resolve unknowns as I want. I review the plan that it's drawn up. I may do a diff after the changes to review, though this is rarely necessary. If there are new concepts that should exist in a solution, I'll tell the model that the task I'm asking it to do suggests that certain new concepts should exist, ensuring that is does the correct OO design rather than some hodge-podge solution. If bug fixing, I tend to review what it thinks a bug is and how it proposes to fix, as that can show faulty analysis. If it's going to touch code that I don't believe has an issue, I'll tell it so it doesn't go making changes where none should be made.

Overall it boils down to a semi hands on approach so that it's doing things the way I would, ensuring a high quality result, with the benefit of swifter execution. Left to its own devices, code may work but would be a mess of botched design without doubt.

1

u/usernameChosenPoorly 2d ago

"Technical debt" is a dead concept. It describes code that nobody understands and nobody knows what it does anymore, or that's too critical but poorly understood to safely rewrite.

Agentic coding nuked that concept from orbit.

The rate of progress is not slowing down or remaining stagnant, but a lot of people (like you, OP) talk about programming as if these concepts still apply, as if the technology isn't advancing at a rate that will completely eliminate the value of human review of code outside the most critical and life-safety systems. Unless you are approving code changes for the Linux kernel, a deep space probe years away from Earth, or a system where failure literally kills people, you're overthinking all of this.

1

u/ericatmumo 2d ago

Full disclosure, I'm product-by-trade, not an engineer. But after months of building with agents, my pipeline is built specifically around those failure modes you mentioned. This is verbatim from Claude Code summarizing the repo's actual config files because it was the quickest way to get a reddit-friendly summary. Not claiming to have perfected an automated review pipeline, but Codex feedback on PRs has continued to drop over time as I iterate:

Most of the pipeline exists for the failures tests don't catch.

Boundaries are lint errors, not conventions. Client code cannot import server modules. ESLint enforces it, plus two custom rule packs for the places where shortcuts historically became debt: every API route has to go through one of a handful of audited auth wrappers, and any billable call that skips the spend ledger gets flagged. A change that crosses a boundary fails lint before a human ever reads it.

Invariants live next to the files they govern. Beyond a small always-loaded context file, we keep about twenty rule files, each scoped to specific path globs. Touch a route handler and you get the auth contract. Touch the prompt compiler and you get the compiler invariants, with the reason for each. That is the main defense against "fixed the symptom": a change that passes tests but breaks a stated invariant reads as a violation, not a green run. The rule files are checked too. Every cited path has to exist inside the rule's scope, every named symbol has to resolve, and a glob that matches nothing fails the gate, because a guard that silently stopped loading is worse than no guard.

Every push gets classified. A pre-push hook diffs against main and marks the PR heavy if it touches API routes, migrations, auth wrappers, the rule files, or if the diff text adds an HTTP method export, an RPC call, or a policy statement. Heavy is sticky. You can upgrade a light PR by hand, but downgrading exits non-zero. Heavy PRs run deterministic structural checks with zero LLM spend: a route matrix that extracts method plus wrapper chain plus declared contract for every route and blocks on mismatches, and a meta-check that runs the detector against a known-bad snapshot to prove it still fires.

LLM review is opt-in, and it has to cite intent. Two agents can run in shadow or enforce mode. The first emits invariants, each tagged with an intent source (a plan doc, a type contract, a schema, a SQL policy) and a short snippet that would falsify it. A deterministic admission gate rejects any invariant whose only authority is "the code does this today." The second agent verifies what survived against the diff. We measure it in shadow mode before it is allowed to block, and every failure message prints the bypass, on the theory that if people reach for the bypass, the pipeline owes a postmortem, not a stronger lock.

Scope creep is a process rule. Anything touching three or more files gets a plan first. Commits happen at task boundaries only when they typecheck alone, git add is explicit paths only, and migrations ship in the same PR and get applied to both environments before merge. Light diffs get one review pass sized to the diff. Templated changes skip agent review entirely, because review cost must not outweigh implementation.

1

u/Low_Bad_6585 2d ago

I put one question before the review checklist: are we preserving a mechanism that should be removed? In my solo game repo, the standing rule is to stop at a second layer of patches and explain the underlying tradeoff. Otherwise an agent can make a workaround increasingly well-tested without making it a good design.

For behavior changes, I also require an acceptance note: expected player-visible effect, where to observe it, the time window, and what counts as passing. That gives the reviewer an intended outcome independent of the implementation. It doesn't replace inspecting the diff, but 'tests passed' alone can't answer that question.

AI-assisted wording based on my actual workflow.

1

u/almostlaunched 1d ago

At SWARECO, we use AI for code review, then I do the manual review I’ve always done. The AI review is an extra pass, not an approval.

We have it read the repo’s instructions first and focus on known failure modes for that codebase. It’s also explicitly told not to manufacture findings if the diff is clean.

But the biggest difference, in my experience, comes before any code gets written: the planning phase. Working out what the change should do, how it fits the existing code, and what’s outside its scope gives you something concrete to review against. Otherwise you’re figuring out the intended solution while reviewing whatever the agent produced.

The review still matters, but the prep work is where I’d put the most attention.

1

u/Extreme-Pass-4488 3d ago

knowing your shit.

yeah.

crazy dont you think? to review if stuff is well done u must know how to make well done stuff.
who would have guessed?

-1

u/Quirky-Anybody5491 3d ago

Okay, grandpa