The pull request is titled "improve error handling in payment sync." It touches 38 files. CI is green. The description is three confident paragraphs about resilience. And somewhere in the middle, a rescue block that used to return a 500 now returns a 200 with {"status": "ok"}.
Nothing in the diff looks wrong in isolation. That's the point. Reviewing AI-generated pull requests is not the same job as reviewing human ones, and the difference isn't quality — it's the shape of the failure. A human PR tends to fail loudly: half-finished, inconsistent, obviously risky in the places the author was unsure. An AI PR tends to fail quietly. It's internally consistent, stylistically clean, tests pass, and the wrongness is in a decision nobody explicitly made.
Rubber-stamping isn't a discipline problem. It's what happens when you apply human-PR review habits to a volume of plausible diffs. This article is about the habits that actually change the outcome.
Why reading the files view fails at this volume
The default review interface shows changed files one at a time. For a human PR of 200 lines, that's fine — you hold the shape of the change in your head and check the details against it.
A 38-file, 1,400-line diff defeats that. You can't hold the shape, so you either skim for anything that looks alarming (and find nothing, because nothing looks alarming) or you read carefully for forty minutes and still miss the cross-file consequence — the caller that now receives a 200 instead of an exception and stops retrying.
Two habits fix this, and both are mechanical:
Review the change as a sequence of commits, not a file list. If the PR is one squashed commit, ask for it to be split, or reconstruct intent from the diff hunks in order. Order reveals the story: schema change, then handler, then test, then the tweak that broke it.
Diff the behavior, not the text. For every changed public function, note its old contract and its new one — return types, thrown errors, status codes, side effects, ordering guarantees. A table of contract changes is short and it's where the real risk lives.
# Behavior-oriented diff: signatures and exception surfaces only
git diff origin/main...HEAD -- '*.py' \
| grep -E '^[-+].*(def |class |raise |return |status_code|throw )' \
| head -200
That command is crude and it's still more informative than scrolling 1,400 lines. It puts every changed return and every changed raise on one screen.
🧠 The important part: you're not looking for bugs in the diff. You're looking for decisions that were made implicitly. The diff only tells you a decision happened; you have to go find out whether anyone meant it.
Ask for the failing test before you look at the fix
A correct fix has a reproduction: something that failed before and passes after. If a PR claims to fix a bug and doesn't include a test that fails on main, that's the first thing to raise — before reading a single line of the implementation.
This matters more for generated code than for human code, because the model has no memory of the bug. It has the description, the surrounding code, and a plausible narrative. It will produce a patch that makes the description true in the places it looked. Without a reproduction, you have no way to distinguish "fixed the bug" from "changed the code in a way that sounds like a fix."
The check is cheap to automate:
# Does the PR add or modify a test that fails without the source change?
git checkout -q origin/main
git checkout -q -- . # keep only the test changes
pytest path/to/affected_tests -q # expect: failures
If the tests pass on main with only the test changes applied, the test doesn't test the fix. That's not always fatal — sometimes you're adding coverage for an already-correct path — but it should be stated in the description, not discovered by a reviewer.
For generated PRs specifically, also check whether the test asserts the thing the description claims. "Improve error handling" with a test that asserts response.status_code == 200 is a test of the happy path, which was never the problem.
Read the tests adversarially
Tests are where generated PRs most often go wrong in a way that hides itself. A passing suite is evidence, but only if the assertions are real.
Look for these in the test diff, in rough order of how often they appear:
| Pattern | What it usually means |
|---|---|
Deleted or @skipped test |
The change broke an expectation; the test was removed to make CI pass |
assert x is not None where a value was asserted before |
The assertion was weakened until it passed |
pytest.raises(Exception) instead of a specific type |
Broadened to accommodate a new exception path |
try: ... except: pass inside a test |
The test now swallows the failure it was written to catch |
| Mock added around the code under test | The test no longer exercises the changed logic |
| Snapshot/fixture regenerated | The expected output was rewritten to match new behavior without discussion |
The last one deserves emphasis. Regenerating a snapshot in the same PR as the code change is indistinguishable from "I changed the behavior and updated the expectation." That's sometimes correct and always worth an explicit question: why did this output change? If the answer isn't in the description, the reviewer is the only person who will ever ask.
A small script catches most of this before a human looks:
import re
import subprocess
DIFF = subprocess.run(
["git", "diff", "origin/main...HEAD", "--", "tests/", "*_test.py", "*_test.go"],
capture_output=True, text=True, check=True,
).stdout
PATTERNS = {
"skipped test": re.compile(r"^\+\s*(@pytest\.mark\.skip|t\.Skip|@Ignore|//\s*skip)", re.M),
"deleted assertion": re.compile(r"^-\s*assert ", re.M),
"broad exception": re.compile(r"^\+\s*(except\s*:|except Exception|pytest\.raises\(Exception\))", re.M),
"swallowed error in test": re.compile(r"^\+\s*except[^\n]*:\s*$\n^\+\s*pass", re.M),
"assert is not None only": re.compile(r"^\+\s*assert .* is not None\s*$", re.M),
"snapshot regenerated": re.compile(r"^[-+]{3} .*(snapshot|__snapshots__|\.snap)", re.M),
}
for name, pattern in PATTERNS.items():
hits = pattern.findall(DIFF)
if hits:
print(f"{name}: {len(hits)} occurrence(s)")
This doesn't judge. It just makes the questionable parts of the test diff visible in ten seconds instead of requiring a careful read of forty files.
⚠️ Gotcha: some of these are legitimate. A broad
exceptin a test can be correct when you genuinely don't care about the exception type. The point is that it should be a decision a reviewer saw, not a detail buried on line 900.
Verify that the "why" exists outside the code
Human PRs carry intent in the review thread: the author explains the trade-off they considered, the thing they tried first, the reason they didn't touch the adjacent module. Generated PRs carry intent in a description written from the diff, which means the description can only ever justify what was done. It cannot tell you what was rejected.
So reconstruct the missing half yourself, from artifacts that exist independently:
- The issue. Does it describe symptoms or a proposed solution? Symptom-based issues are checkable; solution-based issues let a patch satisfy the words without addressing the problem.
- The reproduction. Was it actually reproduced, or is the fix speculative?
- Prior art in the repo. Is there an existing helper, pattern, or module that already solves this? Generated patches frequently reimplement something that exists three directories away, because the context window didn't include it.
That last one is the highest-yield check in the whole review, and it's mechanical:
# Does something similar already exist?
git grep -n -i "retry_with_backoff\|with_backoff\|backoff(" -- '*.py' | grep -v test
If the PR adds a retry helper and the repo already has two, the review comment isn't "this helper has a bug." It's "which of the three is canonical, and can we delete one?" That's a conversation a human author would have had with themselves. The generated patch had no reason to.
Treat scope creep as a signal about the model's context
A generated PR that touches files unrelated to its title is usually telling you something about what the model saw, not about what it intended. Common causes:
- The prompt included a lint or format pass, so every file the agent touched got reformatted.
- A refactor was attempted to make the target code testable, and the refactor leaked.
- The agent followed an error message into an unrelated module and "fixed" something it found there.
Case three is the dangerous one, because the unrelated fix is often plausible and untested. The rule I'd apply: every changed file must be justifiable in one sentence that references the PR's stated goal. If it can't be, it comes out of this PR, whether or not it's a good change.
Splitting is cheap. Reviewing a mixed PR is not, because the reviewer has to hold two unrelated risk profiles at once and will do neither well.
Hunt for APIs that don't exist
This is the failure mode people expect from generated code, and it does happen — but not usually as an invented function name. Modern models are good at plausible-looking calls. The realistic failures are subtler:
- A real method with an argument that doesn't exist in this version (
client.messages.create(..., cache_ttl="1h")where the SDK expectscache_control). - A real config key at the wrong nesting level.
- A real library function imported from the wrong module path.
- A framework convention from a different major version.
None of these fail at review time; they fail at runtime, or worse, at the one code path that isn't covered.
The cheapest defense is to actually execute the import surface and the type checker, not just the test suite:
python -c "import app.services.payment_sync" # import errors surface immediately
mypy app/services/payment_sync.py --strict # unknown attributes surface here
ruff check app/ --select F821,F401 # undefined names, unused imports
F821 (undefined name) catches the invented-variable class. Mypy catches calls to attributes that don't exist on a typed client. Neither replaces review, but together they eliminate the category that's most embarrassing to ship.
For dependency changes, be stricter than you'd be for a human: check that the added version actually exists, and check the changelog between the pinned version and the new one for breaking changes the patch didn't account for.
Review the error paths, because that's where the model is blindest
Generated code is optimized for the path described in the prompt. If the prompt says "add retry logic," you get retry logic on the success path, and the error handling is whatever pattern appeared nearby.
Read every changed error path with three questions:
- What used to happen, and what happens now? Changed status codes, swallowed exceptions, and new fallback values all shift the contract for callers.
- Is the error still observable? A
rescuethat logs atinfoand returns a default is a decision to stop alerting. Sometimes right, often not, never stated. - Does the retry actually retry the right thing? Retrying a non-idempotent write is a data-corruption bug that will pass every test.
That last one is worth a dedicated look because retry logic is a favorite of generated patches:
# What the patch looks like
for attempt in range(3):
try:
return client.charge(customer_id, amount)
except TransientError:
time.sleep(2 ** attempt)
If charge isn't idempotent — no idempotency key, no dedupe on the provider side — attempt two can double-charge. The correct patch passes an idempotency key derived from something stable:
key = f"charge:{invoice_id}:{attempt_epoch}" # stable across retries, unique across invoices
for attempt in range(3):
try:
return client.charge(customer_id, amount, idempotency_key=key)
except TransientError:
time.sleep(min(2 ** attempt, 30))
A reviewer who only checks "does it retry" approves the first version. A reviewer who asks "what happens on the second attempt" catches it. That question is the whole job.
Separate the mechanical review from the judgment review
Rubber-stamping happens when the same person, in the same pass, has to check both "does this compile" and "is this the right design." The mechanical checks are tedious, so they get skimmed, and then the judgment is made on a diff nobody actually verified.
Split them:
Mechanical pass (automate, or 2 minutes): builds, type checks, lints, tests pass, no skipped tests, no deleted assertions, no new dependencies without a version bump note, no secrets, no debug prints, file count consistent with the stated goal.
Judgment pass (human, 10–20 minutes): is this the right layer for the change, does it duplicate existing code, is the error contract intentional, does the test actually reproduce the bug, what happens on the second attempt.
Put the mechanical checks in CI so they can't be skipped, and keep the human pass focused on the four or five questions that need a person. The result is fewer approvals per hour and far fewer reverts.
Automate the parts that are actually automatable
A workable CI gate for generated PRs, in the order I'd add it:
name: pr-guards
on: pull_request
jobs:
guards:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
with: { fetch-depth: 0 }
- uses: actions/setup-python@v5
with: { python-version: "3.12" }
- run: pip install ruff mypy pytest
- name: Lint for undefined names
run: ruff check . --select F821,F811,F401
- name: Type check changed modules
run: |
git diff --name-only origin/${{ github.base_ref }}...HEAD -- '*.py' \
| xargs -r mypy --strict
- name: No skipped or deleted tests
run: python scripts/pr_guards.py --check-tests
- name: Diff size budget
run: python scripts/pr_guards.py --max-files 20 --max-lines 600
The diff-size budget is the one people resist and the one that changes behavior most. A 600-line cap doesn't prevent large changes; it forces them to arrive as a sequence that can be reviewed in order. Generated patches split easily because they're usually additive — asking for the split costs one prompt.
None of these guards knows whether the change is correct. They exist so that the human review starts from "plausible and mechanically sound" rather than "unknown."
Two diffs that pass every check and are still wrong
Mechanical gates don't catch everything, and it's worth having concrete examples of what slips through so you know what the human pass is actually for.
The orderly refactor that changes evaluation order. A PR reorders a validation pipeline "for readability," moving the authorization check after input parsing. Every line is cleaner. Tests pass, because the tests never exercised a case where the two orderings differ — an unauthorized user sending malformed input. Before: 403. After: 400 with a validation message that describes the schema to someone who shouldn't see it. No test asserted the 403, so nothing broke. The reviewer's job here isn't to spot the reorder in a 400-line diff; it's to see the phrase "reorder validation steps" in the description and ask what order the checks were in and why. That question takes ten seconds and only gets asked if the description is read before the diff.
The helpful null guard that hides a data bug. A patch adds return [] if rows is None else process(rows) to a reporting function. Defensive, tested, harmless-looking. But rows is never supposed to be None — the query returns [] for no matches. None means the connection pool gave out and the query silently failed. The new guard converts a loud outage into an empty report that an analyst will screenshot into a board deck.
The common thread is that both changes are locally reasonable and globally wrong. Locally-reasonable is exactly what a generated patch optimizes for, because it has no model of the system's invariants — only of the code in front of it. The human pass exists to supply the invariants.
A practical way to do that: before reading the diff, write down three invariants the touched module is supposed to uphold. For the reporting function: a null result means infrastructure failure, never empty data; unauthorized callers get 403 before any parsing; every write path is idempotent. Then read the diff looking for violations of those three. This is faster and more reliable than open-ended reading, because it converts review into a search for specific contradictions rather than a general impression of quality.
🔍 Why this matters: most review processes ask "does this look right?" A model-generated diff is specifically optimized to look right. Asking "does this violate a stated invariant?" is a question the diff can't pre-satisfy.
Know which PRs came from an agent
You can't apply a different review standard to a category you can't identify. Label generated PRs — a generated label, a trailer in the commit message, or a bot author on the branch — and keep that label accurate even when a human edits the result afterward.
Two reasons this is worth the friction. First, it lets you route: generated PRs go to the checklist-plus-invariants review, human PRs to the normal one, and neither gets the wrong treatment. Second, it gives you a feedback loop. If you track revert rate and time-to-first-incident by category, you'll find out within a quarter whether your generated patches are actually cheaper to own, and the answer is rarely uniform across modules. Auth and billing tend to look much worse than CRUD and test scaffolding, which is useful information for deciding where to allow generation at all.
The label also helps the humans doing the work. A reviewer who knows a diff was generated reads it differently — checking the test delta first, looking for reimplemented helpers, scanning error paths — and that prior is worth more than any generic process instruction.
Read the description as a hypothesis, then test it
A generated PR description is written from the diff, which makes it circular: it describes what changed and asserts that the change is correct. Treat each sentence as a claim to verify rather than a summary to trust.
"Adds retry logic to handle transient failures" — verify: which failures, how classified, how many attempts, what happens on the last one.
"Improves test coverage" — verify: coverage of which lines, and did any existing assertion weaken to get there. A coverage number that rises while assertion strength falls is a net loss.
"Minor refactor, no behavior change" — verify: search the diff for every changed return, raise, status code, and default value. "No behavior change" is the most load-bearing claim in the description and the easiest to falsify with two lines of grep.
"Follows the existing pattern in orders/" — verify: open orders/ and compare. If the pattern isn't there, the model pattern-matched something else, and the same mistake probably recurs elsewhere in the diff.
This takes about ninety seconds and it consistently finds the one sentence in the description that doesn't survive contact with the code. That sentence is where the review should start, because it marks the place where the author — model or person — believed something without checking.
A review checklist you can finish in fifteen minutes
| # | Check | Fails when |
|---|---|---|
| 1 | Test fails on main without the source change |
No reproduction, or a test of the happy path |
| 2 | No assertion weakened, no test skipped | Behavior changed and expectation was edited to match |
| 3 | Every changed file justifiable in one sentence against the PR goal | Refactor or format noise mixed into a fix |
| 4 | No duplicate of an existing helper | Patch reimplemented something it couldn't see |
| 5 | Changed return/raise/status contracts listed and intentional | Callers silently get a different result |
| 6 | Error paths keep failures observable | rescue → default value → no alert |
| 7 | Retries target idempotent operations | Double charge, duplicate email, double write |
| 8 | No invented API surface | Method, param, or config key from a different version |
| 9 | Dependency changes have a version and a changelog read | Lockfile bump with unread breaking changes |
| 10 | Diff under budget, or split requested | Review quality collapses with size |
Ten items, most of them mechanical, most fixable with CI. That's the realistic target: not heroic review, but a floor that generated PRs can't fall through.
Where to hold the line
Generating code is now cheap, and reviewing it isn't. That asymmetry is the actual constraint, and it means the useful question isn't "how do we review faster" but "what do we refuse to merge without a person understanding it."
I'd refuse without genuine review: anything touching authorization, billing, migrations, or data deletion; anything changing a public API contract; anything that modifies a test expectation without a stated reason; and anything whose author — human or agent — can't explain what happens on the second attempt.
For everything else, the checklist above is enough, and the honest goal is to make the mechanical part boring so the judgment part gets the attention. A green build is not an argument. It's a precondition, and the difference between the two is the entire job.
There's a useful test for whether your review process is working: can a reviewer describe, a week later, what a merged PR changed about the system's behavior? If the answer is a file list, you reviewed text. If it's a contract change, a new failure mode, or a deliberate trade-off, you reviewed the change. Generated code makes the first outcome very easy to reach and the second one still entirely human.