A Default Return Is a New API: Reviewing Agent PRs That Swallow Failures

Agent-generated pull requests often look correct because they stop throwing. That is the defect. When a helper that used to raise KeyError, TimeoutError, or PermissionError starts returning [], None, or 0, callers inherit a new API without a version bump.

This note is a review workflow, not a memoir. It assumes the suite already runs. Green tests are not the signal. The signal is a control-flow change that turns a failure into a value.

The pattern to flag

Agents optimize for a compiling happy path. A frequent edit wraps a call in try/except Exception and returns a sentinel.

# proposed change (agent PR) — example, not executed
def load_user_roles(user_id: str) -> list[str]:
    try:
        payload = store.fetch(user_id)
        return payload.get("roles", [])
    except Exception:
        return []

The diff is small. The contract is not. Callers that branched on exceptions now treat "user has no roles" and "store is down" as the same state.

Trust the intent. Revert the silence. Test the failure modes the agent deleted.

Why this shows up in agent diffs

Three local incentives produce the same hunk. None of them are malicious.

• Tutorial shape. Snippets teach .get(key, default) and bare except Exception as politeness.
• Flaky-test pressure. A raising fixture makes CI red. Returning 0 makes CI green.
• Type erasure. list[str] still type-checks when the body returns [] on fire.

The reviewer is not scoring style. The reviewer is deciding whether empty is now a documented success.

What to trust

Keep these pieces when they are local, named, and already in the module's error vocabulary:

• A narrower exception that callers already handle (json.JSONDecodeError, KeyError, TimeoutError)
• An explicit mapping from a domain miss to a typed result (Result, Optional, a custom NotFound)
• Logs that include exception class and a stable error code, without bodies, cookies, or tokens
• Tests that assert both the success payload and the raised type

Trust is not merge approval. It is permission to leave those lines in the working tree while the rest of the diff is still on trial.

What to revert

Revert, or send the agent back with a closed list, when any of the following appear:

1. except Exception or except: around I/O, auth, parsing, or money
2. Default returns that collide with real data ([], {}, "", 0, False)
3. payload.get(key, default) on a field that used to be required
4. A comment that says "fail safe" without naming the safe state
5. Tests rewritten to expect the sentinel instead of the error

# revert this shape
try:
    return decode(raw)
except Exception:
    return {}

An empty dict is valid JSON. It is also how a truncated file disappears.

If the product actually wants degradation, make it a named policy. load_user_roles_or_empty is a different API from load_user_roles. Rename first. Then the default is honest.

Sentinel collision table

Empty is not neutral. Match the return to a production reading before you keep it.

Sentinel	Legitimate success	Hidden failure
[]	User has no roles	Store timeout, authz miss
{}	Object with no fields	Parse error, truncated body
0	Zero-dollar invoice	Missing row, replica lag
False	Feature flag off	Check never ran
None	Optional field absent	Lookup failed mid-flight
""	Empty display name	Encoding error

If both columns are possible, the sentinel is a new public type. Require a tagged result (stale=True, error="not_found") or restore raise.

Review checklist (use as a gate)

Walk the diff with this list. Fail the PR on the first hit unless the ticket asked for that behavior.

• [ ] Does any new except catch a superclass of errors callers already handle?
• [ ] Does a return value now overlap a legitimate success value?
• [ ] Did the agent delete a raise or replace it with return?
• [ ] Did tests lose a pytest.raises / assertThrows assertion?
• [ ] Is the default visible in the type (Optional, Result, union) rather than only in the body?
• [ ] Would on-call see this failure in metrics, or only as an empty result?

Paste the list in the review comment. Agents respond to a closed list. They do not respond to "please handle errors properly."

Artifact: decision table

Diff shape	Trust?	Action	Test you add
except KeyError: raise NotFound	Yes	Keep	pytest.raises(NotFound)
except Exception: return []	No	Revert	Fault injection on the store
.get("roles", []) on a required payload	No	Make the key required	Missing-key test
except TimeoutError: raise	Yes	Keep	Deadline test
except TimeoutError: return cache	Conditional	Require an explicit stale flag	Assert stale=True
Tests updated to drop raises	No	Restore original asserts	Re-run the deleted test

The except Exception: return [] row is the usual agent PR. It is also the row that hides incidents.

Concrete review: before and after

Suppose production code is:

def invoice_total(order_id: str, db) -> int:
    row = db.get_order(order_id)
    if row is None:
        raise OrderMissing(order_id)
    return row.cents

An agent "fixes" a flaky test by doing this:

def invoice_total(order_id: str, db) -> int:
    try:
        row = db.get_order(order_id)
        if row is None:
            return 0
        return row.cents
    except Exception:
        return 0

The flaky test goes green. Billing now emits zero-dollar invoices when the replica lags. Zero is a legal amount. Do not negotiate it away in review.

Review comment template (paste, then fill paths):

This PR converts OrderMissing and transport errors into 0.
That is a billing API change.

Please:
1. Restore raise OrderMissing for missing rows.
2. Do not catch Exception.
3. If the test is flaky, fix the fixture (transaction, clock, isolation),
   not the production return.
4. Add test_invoice_total_missing_order_raises and
   test_invoice_total_db_error_propagates.

Keep retries, caches, and fallbacks out of this PR. Those are separate releases.

What to test (reproducible plan)

Label: this plan is a proposal you can run locally. It does not claim production metrics.

1. Restore a raising fixture. Point db.get_order at a stub that raises ConnectionError once.
2. Assert propagation. The public function must not return a number.
3. Assert the missing-row path. None from the store must raise OrderMissing, not return 0.
4. Assert the success path. One valid row, exact cents.
5. Check the test diff. If the agent removed pytest.raises, put it back in the same PR.

# proposed tests — example, not executed against a live service
import pytest

def test_invoice_total_missing_order_raises():
    db = FakeDb(rows={})
    with pytest.raises(OrderMissing):
        invoice_total("ord_1", db)

def test_invoice_total_db_error_propagates():
    db = FakeDb(error=ConnectionError("replica"))
    with pytest.raises(ConnectionError):
        invoice_total("ord_1", db)

def test_invoice_total_success():
    db = FakeDb(rows={"ord_1": Order(cents=1999)})
    assert invoice_total("ord_1", db) == 1999

If you only add the success test, you have accepted the agent's contract.

Commands that keep the review honest:

git diff origin/main -- '*.py' | rg -n \
  'except Exception|except:|return \[\]|return \{\}|return 0|return None|return False'
pytest -q tests/test_invoice_total.py --tb=short

rg over the diff is faster than reading every hunk for this class of bug. Fail the review if the test file is the only place raises disappeared.

Typed alternative when degradation is the ticket

When the ticket really wants a non-raising API, do not hide it in int. Name the miss.

# proposed shape — example, not executed
from dataclasses import dataclass

@dataclass(frozen=True)
class InvoiceTotal:
    cents: int
    missing: bool = False

def invoice_total(order_id: str, db) -> InvoiceTotal:
    row = db.get_order(order_id)
    if row is None:
        return InvoiceTotal(cents=0, missing=True)
    return InvoiceTotal(cents=row.cents)

Callers can no longer confuse a free order with a missing one. The type change is the release. That is the point.

Isolated second pass

When the patch is large, regenerate the same task in a clean tree and diff the two error paths. You are not looking for identical code. You are looking for whether a second sample also deletes raise.

Disclosure: This article was prepared as part of MonkeyCode's product outreach.

MonkeyCode is an open-source coding environment with free model access and a free server option. Those two are enough to apply the same prompt to a throwaway checkout, then compare except hunks without pointing either run at production credentials. Use it as a reproduction box, not as an authority. If the second sample still swallows errors, the prompt is the defect.

Keep the original PR as source of truth. Merge neither sample until the decision table is green.

Limitations

This workflow misses silent successes that never raise: off-by-one totals, swapped currency codes, truncated pagination. It also over-flags libraries that document [] as the empty-but-valid result.

It does not replace type checkers. Optional[int] would have made a 0 return a review smell earlier. Turn them on.

It does not measure upstream latency or correctness. Fault injection is only as good as the stub. A stub that never raises will bless the agent's catch-all.

Who should not use this

• Do not apply a blanket "never catch Exception" rule to process supervisors, CLI entrypoints, or batch workers whose job is to isolate a failed item and continue.
• Do not use sentinel returns on money, authz, or deletion paths.
• Do not outsource the merge decision to a second agent run. The reviewer still owns the contract.
• Skip this checklist on pure comment or formatting PRs.

Close

Treat every new default as a release. If the ticket did not ask for degradation, restore the raise, restore the test, and only then discuss retries, caches, or fallbacks as a separate change.