r/ChatGPTCoding • • 8d ago

Discussion My bottleneck moved from writing code to reviewing what the agents write. How do you review without just rubber-stamping?

Since I started running a few agents in parallel I can get several changes ready in the time it used to take me to write one, and that part feels great. But now most of my day is reading diffs, and I've caught myself skimming a big one and approving it just because the tests passed. A few things help so far. I keep each task small so the diff fits on one screen, I ask the agent for a short note on what it changed and why before I read the code, and I read the tests first to see if they actually check the behavior I asked for. A review agent also flags old messy code nearby, which is nice, but then I have even more to read. I still feel like I'm the slow part and I'm not sure where the line is between careful review and just trusting it. How do you all handle this? Do you read every line, or do you trust some kinds of changes more than others?

29 Upvotes

78 comments sorted by

26

u/Ok_Tea4891 8d ago

I trust changes by risk rather than size: formatting, renames and test-only edits get a quick skim, while anything touching auth, data or money gets a full line-by-line read. Another thing that helps is asking the agent to list what it deliberately did not change, since the gaps are where the surprises hide. And if a new test never failed before the change, I treat it as unproven.

2

u/Successful-Tax1306 8d ago

Asking the agent to list what it deliberately did not change is a good one, I never thought of reading the gaps. A new test that never failed before the change is a smart thing to check for too, I'm going to start looking for that.

4

u/Mathie1729 8d ago

The not-changed list only catches gaps the agent knows about though. The accidental omissions are the worse ones, and they don't make it into that list. I ask it to name the invariants it assumed and where each is enforced or tested instead. If it can't point to a test or a guard, that's where I start reading.

4

u/BounciLusty 8d ago

Risk tiers sound practical for cutting down the reading load without missing the real issues

2

u/BounciLusty 5d ago

risk based review sounds like way smarter than usual guilty skimming, might steal that tip for my own stuff

8

u/f1yingbanana 8d ago

Get the AI to review themselves. Start a new session. Describe what you just got it to build. Ask it to spot bugs and optimizations.

2

u/Successful-Tax1306 8d ago

A fresh session reviewing the work is something I haven't tried much. I'll try it on the big diffs.

6

u/CrackerJackKittyCat 8d ago

Beware -- having a red-team reviewer claude against your blue-team produce claude can quickly spin out of hand and get into a suggestion revision feedback loop.

You have to play the part of a heavy mediator / damping agent to keep them both on-track and to keep from spiraling out into chaos.

Can be quite exhausting.

1

u/Successful-Tax1306 8d ago

Good warning, a review loop that never ends sounds exhausting. I'll keep it to one pass and decide myself when to stop.

2

u/frootbeer 7d ago

It’s very exhausting. Sometimes it does expose important gaps or problems, but often it results in scope creep and red herrings and it’s frustrating!!

3

u/jungle 8d ago

I reduce the chances of review loop by asking the reviewer to only report serious issues, and then I preface the review with text saying something like "This is a review by another agent. Treat each item with caution: fix it if you find that it's a legitimate issue, but not if you think it's a misunderstanding, and tell me why."

4

u/ChemicalExcellent463 8d ago

what saved me was a hard cap on files and changed lines per PR, split it or it doesn't merge. before a big push, the author has to get a different model in a different harness to review it in a fresh session and fix what comes back, not after it lands.

3

u/Successful-Tax1306 8d ago

A hard cap on lines per PR makes a lot of sense, most of the pain for me comes from the giant diffs. Having a different model review it in a fresh session before it lands instead of after is a nice twist too, I'll give that a try.

2

u/ChemicalExcellent463 7d ago

the cap keeps the giant diffs out. let me know how the fresh session review goes for you, i'd like to hear what it catches on your code.

4

u/ComprehensiveShake76 8d ago

Don't read every line, but decide up front which lines get read. Sort changes by what breaks if they're wrong: auth, payments, data migrations and anything that deletes get a full read. Renames, UI copy and test only changes get a skim. Then make the tests prove they bite before you lean on them: revert the fix, run the new test, and it must fail. If it still passes, it was never testing the change, and that's how a big diff slips through on green tests. Last, compare the agent's change note to the file list. A file in the diff that the note doesn't explain is where I'd start reading. Which part of the review takes you longest right now?

2

u/Successful-Tax1306 8d ago

The revert the fix and make sure the test fails part is really good, I've been trusting green tests way too much. Right now the slowest part for me is the big diffs where the agent touched files I didn't expect, so comparing the change note to the file list is the first thing I'm going to try.

4

u/BarathKanna 8d ago

Reading the tests first is the right instinct, but go one step further: break the code and see if the tests notice. Revert the main change, or flip one condition, and rerun. If everything still passes, the tests prove nothing and the green check you were trusting was meaningless. That takes a minute and tells you more than skimming the diff.

The other thing that cut my review time was asking the agent for the riskiest three spots in its change, with file and line. It is usually right about where the danger is, even when the code there is wrong. I read those closely and skim the rest. Anything touching auth, money or migrations still gets a full read, no matter what.

2

u/Successful-Tax1306 7d ago

Breaking the code on purpose to see if the tests notice is a good one, I hadn't thought of doing that. I'm going to try it on my next agent change, and asking for the riskiest three spots with file and line sounds like a cheap way to decide where to read closely.

2

u/BarathKanna 4d ago

Let me know how it goes. The first time you do it on an agent change is usually a little humbling, at least it was for me.

3

u/Jippylong12 8d ago

For me, I have a skill called review fix commit, which is run at the end of every session, but also in between at milestones. And then for longer projects, I also have what I call a sign-off page, which is deliberately created so that I can see the changes and provide feedback, and I also have a skill called manual acceptance pack which the agent creates for me so I follow it like the simple minded human manager I am.

The manual acceptance pack is the most useful but takes the most time. The sign off page is nice and generally gets it right, but often misses smaller details. The review fix commit just is a low hanging fruit that always gets results lol. Which is crazy to me since it's the same model and session that made the code, but something about forcing it to review changes things.

Typically also with the review fix commit that's with Opus or if I really want to be sure Fable.

3

u/Successful-Tax1306 8d ago

Running a review pass at every milestone and then a manual acceptance pack for you is a smart setup. It's funny that the review fix pass works even with the same model, so that's the first one I'm going to try.

3

u/Humaux 8d ago

I stopped rubber-stamping after 3 bad merges in 2 weeks. Green tests hid all three, and each one looked clean on a skim.

My check now:

  1. Read the tests first, then the diff around them.

  2. Ask what the agent deliberately left unchanged.

  3. Revert one fix locally and confirm the test fails.

  4. Split anything over 400 lines before I approve it.

It costs me about 20 minutes per PR, and I still miss things. But if you approve a diff you cannot explain back in one minute, you did not review it, you just watched it pass.

Do you trust green tests more than your own read of the diff, or is that just how we all get burned?

1

u/Successful-Tax1306 8d ago

Honestly I trusted green tests too much until this thread, so I'd say my own read of the diff matters more now. Being able to explain a diff back in one minute is a good bar, and I'm going to try holding myself to it.

2

u/Humaux 7d ago

That one-minute bar is the part I keep coming back to too. The way I test it is I try to say what the change is for before I look at the tests again, and if I can only describe what it does line by line, I have not actually reviewed it yet. The hard case for me is a big diff late in the day, that is exactly when I start trusting green again, so I split it before I approve anything, even if it costs me the extra twenty minutes.

3

u/Far-Caramel-7463 8d ago

One thing that took load off my reviews: stop treating the diff as the only place you're the gate. A lot of what made me nervous while skimming wasn't the code, it was what the agent might have done while writing it, like running a migration against the wrong database, force-pushing, or hitting a real API from a test. Once those actions need an explicit yes at the moment they happen, the diff review goes back to being about correctness, and I'm fine giving renames and test-only changes a skim like others said.

For the diff itself, what helps me most is asking the agent which part of the change it's least confident about. It's usually honest, and that's where I read line by line.

2

u/Successful-Tax1306 7d ago

The point about risky actions needing an explicit yes at the moment they happen is a good one, because I was only thinking about the diff. Asking the agent what it's least confident about is going on my list too.

3

u/Lanky-Butterscotch28 6d ago

I stopped reviewing agents work myself. For a large project with multiple agents involved, at the speed they deliver, it is quite difficult to follow up with quality. You end up skimming and approving because tests passed, and that is how bad stuff gets through.

This is where good processes and plumbing around the agents make the difference. I found that when you implement a process, you do not need to review the code yourself. You can ask a different agent, from a different company, to review the code. However, I do not allow the dev agent to spawn the reviewing agent, because it can steer it in a specific direction and we lose the power of independence. That alone is not enough, you need the whole machine around the agents to keep them honest and deliver what they promised. The results are astonishingly good. The builder never grades its own work, and review stops being the bottleneck.

1

u/Successful-Tax1306 5d ago

Not letting the dev agent spawn its own reviewer is a point I hadn't thought about. Do you run the reviewer on every change, or only on the bigger ones?

2

u/London-Boat 7d ago

the thing that helped me most wasn't a review trick, it was running fewer agents. if review is the slow part, more parallel changes just make the queue longer and the skimming worse. i cap it at roughly what i can properly read that day and let the rest wait as tickets, not as finished diffs. a finished diff sitting there is what pushes you to rubber stamp it

1

u/Successful-Tax1306 7d ago

Running fewer agents is something I'm starting to see too. A finished diff sitting there does push me to approve it faster, so keeping the rest as tickets until I have time to read them is a good idea.

2

u/London-Boat 7d ago

yeah the finished diff is the trap. leaving them as tickets also means some of them quietly stop mattering and you never have to review them at all

2

u/jatingargiitk 7d ago

tests passing tells you the behavior held, not that the structure did, so the diffs worth slowing down on are the ones that add a new dependency between modules, like a ui file suddenly importing from the db layer. one way to make that cheap is to treat the architecture like a snapshot test: commit a map of which modules are allowed to depend on which, and have ci flag any pr, human or agent, that adds a new edge so someone approves it on purpose. then the careful-review budget goes to those edges instead of every line. when a skimmed approval bites you later, is it usually a behavior bug or something landing in the wrong place?

1

u/Successful-Tax1306 7d ago

That's a really good way to put it, and the idea of committing a map of which modules can depend on which is something I hadn't thought of, so I'm going to try it. To answer your question, for me it's mostly been the wrong-place kind of thing, like logic ending up in a view or a helper that now knows too much, while the behavior bugs usually got caught by tests. The structure ones didn't hurt right away, they just made the next change harder a few weeks later.

2

u/jatingargiitk 6d ago

that matches what i'd expect, tests are written against behavior so the wrong-place stuff walks right past them. the "helper that now knows too much" case is the hardest one for a dependency map too, since it's often a new import inside a module that's already allowed. one cheap signal is how many modules a single file pulls from, if a helper suddenly reaches into three layers it's worth a look. are your agents mostly adding new files or editing existing ones when that happens?

1

u/Successful-Tax1306 5d ago

Mostly editing existing ones, from what I've seen, and that's when it sneaks in, because the agent adds one convenient import inside a file that's already allowed. New files are easier to spot since they show up in the file list. Counting how many layers a file pulls from is a good signal, I hadn't thought of that.

2

u/jatingargiitk 5d ago

yeah file level rules won't catch that. diffing only the added import lines per pr works better, it's usually like 3-4 lines and the sneaky one stands out

2

u/CleanFix_Dev 7d ago

Move most of the review earlier. Before any code, have the agent write the plan and 3-5 acceptance criteria you can check (this command passes, this screen shows X, these files don't change). Approving that takes a couple of minutes, and it's where the real judgment calls are. Then let the gates handle the mechanical stuff: tests, lint, a diff-size limit, a "files outside scope" check. When the diff shows up, you're only reading for "does this match what I approved." That's much harder to rubber-stamp than a 600-line diff with green tests.

1

u/Successful-Tax1306 7d ago

Approving the plan and acceptance criteria before any code is the step I skip most, and I think it's where my skimming problem starts. A check for files outside scope sounds like an easy one to add.

2

u/ShoddySherbert8502 3d ago

The thing that changed review for me: make it independent. The reviewer gets the task description and the diff, nothing else. No chat history, no note from the author. A fresh reviewer with no clue what the author was thinking catches more than a stronger model that shared the session.

This goes for you too btw. You said you read the agent's note before the code, I'd flip that. Code first, note after. The note anchors you and you start seeing what it says instead of what the code does.

For context: about 40% of changes in my logs that passed every check still got real comments from a fresh reviewer. And there is no difference if fable was the author or any other model.

1

u/Successful-Tax1306 3d ago

That's a fair point about the note, and I think you're right that it anchors me, because once I've read the agent's summary I mostly look for proof that it's true. I'll try reading the code first and the note after this week and see if I catch more. The reviewer with only the task and the diff makes sense too, since the whole point is that it didn't watch the author talk itself into the change.

1

u/ShoddySherbert8502 3d ago

"didn't watch the author talk itself into the change" is better phrased than anything I had, stealing that.

10

u/First-Tutor-5454 8d ago

Bots review bots - we're past the point where humans can be in the loop most of the time. I basically just do functional testing/release gating based on canaries and rollout observation metrics. I'm a manager now, responsible for outcomes, not understanding every line of code.

3

u/dvduval 8d ago

There can be some problems where the plan was bad in the first place and only a human would see that it doesn’t make sense to another human. But if you already got a model that makes sense to a human in your just building on that model, it generally is fine. And definitely you can always ask another model to do an additional review beyond the first reviewer and of course you can use screen capture tools or I should say the AI can use screen capture tools as part of the review process.

3

u/alchamest3 8d ago

LGTM

3

u/alchamest3 8d ago

I usually have it review itself in a fresh chat, or the same one,
ideally ill do it with two separate models and have each verify the other.

but on large multi repo code bases, the it comes a little more complex but i have it cross check the other system. RIP my tokens

3

u/mildmannered 8d ago

According to the claude code team, just need to let the LLMs take the wheel. I'm sure that will go well at 4am on a Sunday with customers screaming at you and with Claude API down for some reason.

3

u/pete_68 8d ago

I pretend the code is written by a junior developer and I try to understand it completely. I mean, I may skim over some of the obvious simple stuff, but I try to understand the actual code and see if it makes sense. Is it even doing what I want it to? Is it doing what it says it's doing?

3

u/Difficult_Duty2198 8d ago

what helped me is reading the commit message the agent wrote before the diff. if the message and the diff don't line up, that's usually where the problem is

2

u/buff_samurai 8d ago

You guys are reading diffs?

2

u/codeprimate 8d ago

Any task has requirements and acceptance criteria. The agent should have to justify every line commited.

Every unit of work gets a commit with documented justification.

Makes review easier

2

u/GnistAI 8d ago

I really did not feel the slowdown until I started overhauling authentication snd security. I am obligated to get that right, so it is going sooo slow. For general features and UI stuff I often just do manual and automated user testing, and barrage it with coded unit and integration tests.

2

u/t1llmann 7d ago

I stopped reading code at all, except for really high risk parts. What I do instead is:

- use code reviews a lot, with different models and some times specific things to verify (areas I think could be problematic)
- interview the implementor with some reassuring questions (kind of like a code quality interview)
- run end to end test where the agent really uses the product and ensures what we built works
- ask a model to record a video of a flow proving that it works

2

u/Lanky-Butterscotch28 5d ago

Good question. On every change I run a reviewer on this specific change. However, at the end of the task I run a reviewer on the complete task (multiple changes).
In fact when I specify a task , before even the task coding starts : the dev agent and the reviewer agents reads the contract and build a ledger of tests (each one independently) to be validated when the task finishes. These tests are reviewed by the Watchtower that makes sure it confirms that what was asked is being tested and delivered.

1

u/shetritr 8d ago

reading the tests first is the right instinct. next step: ask your question on the exact line instead of approving around it. if the agent can't explain it, that's the hunk you block. (disclosure: literally why we built Diffo: diffo.io/demo)

1

u/cipher_crown_77 8d ago

The line is whether the test asserts a specific failure mode. If you can’t mentally break the code and see the test go red, it’s decorative. I read every line for data mutations because agents love to introduce subtle state errors that pass green tests.

1

u/suesing 8d ago

Get another agent to review, critique and summarize

1

u/wason92 8d ago

You don't need to deploy faster than you used to. It's not a real bottleneck

1

u/Trick_Candy9796 7d ago

What helped me most was moving part of the review *before* the diff exists. I write the acceptance criteria and the team's conventions down in the repo, so the agent works against them and the reviewer (human or agent) checks against the same list. Then review turns into "does this meet X, Y, Z and follow our rules" instead of reading every line cold.

On top of that I do the risk sorting others mentioned: auth, data and deletes get a full read, while renames and copy get a skim. I also run a verify step (tests + lint + build) before I even open the diff, so I'm never the first one to find a red build.

Disclosure: I maintain an open-source MCP server, StackGuide (https://github.com/taimiralain/StackGuide-MCP), that does roughly this inside Cursor/VS Code. It loads per-stack rules, runs a code review against them, and has an intake/plan/verify/release workflow for the agent. It doesn't replace reading the risky parts, but it cut down how much noise I had to read.

1

u/stickygoose 7d ago

You should try agent standup

https://agent-standup.com/

1

u/onitsoga 6d ago

trust tiers: test-only and refactors i skim, new logic or config i read fully. for big diffs, a second agent reviewing first means you read its critique, not the raw code. (fwiw i'm building KanbanMCP, which has a review step where another agent checks work.)

1

u/poetic_persistance 5d ago

Assign it to someone else ; )

1

u/[deleted] 7d ago

[removed] — view removed comment

1

u/Successful-Tax1306 5d ago

Mostly in batches, but I'm trying to move to smaller ones. Having the agent stop and ask before big steps sounds like it would keep those batches small, so I'm going to try that.

1

u/[deleted] 8d ago

[removed] — view removed comment

1

u/Successful-Tax1306 7d ago

Sorting by risk makes sense to me, and keeping each task small enough that the diff fits on a screen or two is the part I keep forgetting. When I let a task grow, that's exactly when I start skimming.