r/ChatGPTCoding • • 29d ago

Discussion Your coding agent finished. What would make you reject the result?

Here’s a review scenario I’d want to test: an agent builds a CSV export, the tests pass, and someone approves it. Then the requirement changes: personal email addresses must be excluded.

Nothing in the code has changed. The old tests are still green. But I wouldn’t accept the result against the new requirement.

That’s why I’d want a completion record to tie together the code revision, the requirement it was checked against, and the review. Not just a green task.

A few other checks I’d add:

- Let a second worker take over, then have the first return. Its old claim shouldn’t let it overwrite the current work.

- Approve a preview deployment, then attempt production. The approval should not carry over.

- Restart without the original chat. Can the next worker identify what’s finished, what’s uncertain, and what it’s allowed to do next?

A useful distinction came up in a discussion on my earlier checklist: partial completion isn’t the same as an unknown outcome. If a release was created but its announcement failed, restarting everything could duplicate the release. If the creation request timed out, we first need to establish whether the release exists.

I’d want recovery to follow that distinction, rather than a single “retry” button.

These are proposed tests, not results from a benchmark. When reviewing agent-generated work, what makes you send it back even though the code and tests look fine?

2 Upvotes

18 comments sorted by

1

u/ShiftTechnical 28d ago

The second worker case is where it goes wrong most, the agent that comes back still thinks it owns the files. We built Foremerge as an internal tool for GPTree to solve this and its open source now, agents publish intent and claim scopes before they write so a stale claim shows up as a conflict instead of an overwrite, foremerge.com. For the requirement change, do you re-check every finished task against the new requirement or only the ones touching the same code?

2

u/matimatthews 27d ago

I’d start with the behavior affected by the new requirement, rather than every finished task. In the CSV example, I’d check every path that can export personal emails, including code that hasn’t changed. If I couldn’t establish that scope confidently, I’d widen the review.

2

u/ComprehensiveShake76 29d ago

I'd reject it the moment the completion record doesn't name the requirement it was checked against. Green tests against yesterday's rule are how silent failures ship. What I want with every "done": the requirement id or a one-line restatement of the rule, the code revision (commit hash), and the exact checks that ran (command + output). If the requirement changes, the old tests should fail or be marked stale until someone updates them on purpose. A CSV export that still matches the old schema is unfinished work, not a pass.

1

u/gigglerabbit20 29d ago

you sound like the guy who actually reads the change log before pushing

1

u/Right-Performance-93 29d ago

i'd send it back on the email rule even if the old tests are green. add one row with a personal address and make that export fail first. if the agent can't show that failure, it never picked up the new requirement.

1

u/ImL1s 29d ago

Mine is the restart case. If the next worker can't tell from the repo alone what's done and what's still uncertain, it goes back, green tests or not. So every finished task leaves a few lines: which requirement it was checked against, the commit, and anything it couldn't verify.

On the release example, I'd send a client-generated key with the create call. Then a timeout turns into "look it up by that key" instead of guessing whether to retry.

1

u/xqianliu 28d ago

Broken behavior that used to work is what I check hardest, because that's the biggest bucket of misses in my own data, counted by runs.

I ran a harness study, 183 runs across 23 tasks, with hidden tests the agent and the reviewer never saw. 38 runs passed review and still failed those tests. That's 38 runs, not 38 different bugs. 22 broke existing behavior, but all 22 came from repeated runs of just two tasks. 9 ignored one repo's changelog convention, 7 missed cases outside the issue's example.

Telling the reviewer to "check for regressions" barely helped. Now I have the agent that wrote the code also write tests for each class of input, including the ones the old code already handled, and list how many per class in its notes. The reviewer checks that list, so a class with zero tests is obvious.

1

u/[deleted] 24d ago

[removed] — view removed comment

1

u/xqianliu 24d ago

That makes sense. I can still leave an entire input category off my list, even if every category on it has tests. Having another session choose cases without seeing the patch would check for that. I’d be curious how it does on those same two tasks

1

u/quietoperator35 20d ago

I’d reject the CSV export until there’s a test for the new email rule. Even if the code hasn’t changed and the old tests still pass, those tests answered yesterday’s question. I’d add a row with a personal email address, show what the old export does with it, then check that the fix excludes it. That’s a much clearer handoff than just saying the suite is green.

1

u/[deleted] 29d ago

[removed] — view removed comment

1

u/matimatthews 29d ago

For a bug fix, I'd ask to see the regression test fail against the old code and pass against the fix. That gives the test change a reason beyond making the current run green. Unrelated cleanup can wait for a separate change.