A Catch-All Is a Policy Change: Reviewing Agent PRs That Swallow Exceptions

Agent-generated pull requests often turn failures into successful returns. That is not a style nit. It is an API change.

A catch that logs and continues, or an except Exception: return None, rewrites every caller. Reviewers should treat the handler as a product decision: trust it only at an explicit boundary, revert it when the type is generic, and prove the rest with tests that fail closed.

The pattern in the diff

Agents are optimized to make the branch compile. A timeout, a missing key, or a type mismatch gets wrapped so the happy path type-checks. The suite stays green. Production then loses the failure mode.

# Typical agent-generated handler
def fetch_invoice(client, invoice_id: str) -> dict | None:
    try:
        payload = client.get(f"/invoices/{invoice_id}")
        return payload.json()
    except Exception as exc:
        print(f"failed to fetch invoice: {exc}")
        return None

Short. Compiles. Wrong.

Callers can no longer tell "no invoice" from "timeout" from "auth expired" from "JSON is not an object". An HTTP 500 becomes None. The next agent PR adds if data: and skips billing. Two diffs later, silence is the system.

A labeled alternative for the same function:

class InvoiceFetchError(Exception):
    def __init__(self, invoice_id: str, cause: BaseException):
        super().__init__(f"invoice {invoice_id}: {cause!r}")
        self.invoice_id = invoice_id
        self.cause = cause

def fetch_invoice(client, invoice_id: str) -> dict:
    try:
        response = client.get(f"/invoices/{invoice_id}", timeout=5.0)
        response.raise_for_status()
        payload = response.json()
    except (OSError, TimeoutError, ValueError, RequestException) as exc:
        raise InvoiceFetchError(invoice_id, exc) from exc

    if not isinstance(payload, dict) or "id" not in payload:
        raise InvoiceFetchError(invoice_id, ValueError("invalid invoice payload"))
    return payload

The second version still handles errors. It does not hide them. That distinction is the whole review.

Why the model emits this

The training mix rewards snippets that "keep going". Catch-all examples are common in tutorials. Typed failure is rarer in those snippets, so the model copies the tutorial.

Three pressures show up repeatedly in agent diffs:

1. Green path bias. A raised exception fails the demo. A swallowed one does not.
2. Type-narrowing by deletion. Returning None is cheaper for the model than introducing an error type and updating callers.
3. Log-as-proof. print or console.error is treated as handling. It is only I/O.

None of those pressures are malicious. They are still a contract change. Review the contract, not the model's intent.

Trust, revert, or prove

Run a three-bucket pass before reading the feature description. Do not negotiate with the rest of the diff until every new handler is labeled.

Trust

Trust a handler only when every item below holds:

• The except / catch list is closed and named. No Exception, no error, no any.
• Failure becomes a typed error or a result object callers already understand.
• Side effects exist: a metric, a structured log without secrets, a span status.
• The handler sits at a documented boundary: HTTP adapter, queue consumer, process edge, or FFI.
• Tests show both the wrapped cause and the outward type.

If any item is missing, it is not trust. Move the line to another bucket.

Revert

Revert immediately when the PR:

• Adds except Exception, catch (e), catch (...), or empty catch {}.
• Returns a sentinel (None, {}, [], 0, false, HTTP 200 plus "ok") after a failure.
• Logs a request body, Authorization header, cookie, or token.
• Retries inside the same catch. That is a new call graph. Do not review it as "error handling".
• Changes a function that used to raise into one that returns optional, with no caller and docs update.

Do not fix-forward a catch-all in the same review. The blast radius is the call graph, not the line.

Prove

Prove the middle cases. Mapping FileNotFoundError to 404, mapping a unique-constraint failure to 409, or isolating a third-party SDK that raises RuntimeError for business cases all belong here.

Require a test that failed on the old code and still fails if the handler is widened again. One mapping, one unexpected-error case. Not a narrative comment.

Decision table

Diff signal	Bucket	Reviewer action
except Exception / catch (e) / empty catch	Revert	Restore raise; demand a named type
except SpecificError → typed wrapper + from exc	Trust if tests exist	Check log fields and caller updates
return None / return {} on failure	Revert	A sentinel is a new API
Map DB unique violation → 409	Prove	Test 409 and test unexpected DB errors still 500
except: log and continue in a loop	Revert	Partial success must appear in the return type
Catch at HTTP edge, re-raise as Problem+JSON	Prove	Snapshot status and type URI; do not leak stacks
Catch in a helper used by many call sites	Revert	Policy does not belong in a helper

This table is the review artifact. Apply it before the rest of the PR.

A reproducible test plan

Do not accept "we logged it". Logs are not assertions. The suite below is a proposed gate, not an execution report.

# test_invoice_fetch_errors.py
import pytest
from unittest.mock import Mock
from requests import Timeout, HTTPError

from invoices import fetch_invoice, InvoiceFetchError

def test_timeout_does_not_become_none():
    client = Mock()
    client.get.side_effect = Timeout("read timed out")
    with pytest.raises(InvoiceFetchError) as ei:
        fetch_invoice(client, "inv_1")
    assert ei.value.invoice_id == "inv_1"
    assert isinstance(ei.value.cause, Timeout)

def test_http_500_does_not_become_empty_dict():
    response = Mock()
    response.raise_for_status.side_effect = HTTPError("500")
    client = Mock()
    client.get.return_value = response
    with pytest.raises(InvoiceFetchError):
        fetch_invoice(client, "inv_2")

def test_invalid_json_object_is_not_success():
    response = Mock()
    response.raise_for_status.return_value = None
    response.json.return_value = ["not", "an", "object"]
    client = Mock()
    client.get.return_value = response
    with pytest.raises(InvoiceFetchError):
        fetch_invoice(client, "inv_3")

Run them against the merge base, not as a local courtesy:

git fetch origin main
git diff --name-only origin/main...HEAD
pytest -q test_invoice_fetch_errors.py

If the agent also edited tests to expect None, revert those tests. A test that documents silence is not coverage. It is a permission slip.

TypeScript carries the same smell on full-stack agent PRs:

// Agent version
export async function fetchInvoice(id: string): Promise<Invoice | null> {
  try {
    const res = await fetch(`/invoices/${id}`);
    return await res.json();
  } catch (e) {
    console.error(e);
    return null;
  }
}

// Review target
export class InvoiceFetchError extends Error {
  constructor(readonly invoiceId: string, readonly cause: unknown) {
    super(`invoice ${invoiceId} fetch failed`);
  }
}

export async function fetchInvoice(id: string): Promise<Invoice> {
  let res: Response;
  try {
    res = await fetch(`/invoices/${id}`);
  } catch (cause) {
    throw new InvoiceFetchError(id, cause);
  }
  if (!res.ok) {
    throw new InvoiceFetchError(id, new Error(`http ${res.status}`));
  }
  const payload: unknown = await res.json();
  if (!isInvoice(payload)) {
    throw new InvoiceFetchError(id, new Error("invalid payload"));
  }
  return payload;
}

null is a value. An exception is control flow. Mixing them is how a billing job skips invoices without paging anyone.

Commands that rank the smell

These are heuristics. They rank files. The table still decides.

# Python catch-alls in this branch
git diff -U0 origin/main...HEAD | rg -n "except Exception|except:|except BaseException"

# TypeScript / JavaScript
git diff -U0 origin/main...HEAD | rg -n "catch \(e\)|catch \(err\)|catch \{"

# Sentinels returned near a handler (rough)
git diff -U0 origin/main...HEAD | rg -n -A2 "catch|except" | rg "return (None|null|\[\]|\{\}|false|0)"

# Added lines only
git diff origin/main...HEAD | rg '^\+' | rg "except Exception|catch \(e\)"

Second pass: call sites. If fetch_invoice used to raise, list every caller. Zero try around those callers means the PR just changed them all to implicit None handling.

rg -n "fetch_invoice\(" --type py
rg -n "fetchInvoice\(" --type ts

Record the count. If the handler widened and the caller count is greater than one, the PR is not local. Split it or revert it.

HTTP mapping is still a policy

Agents like this adapter:

def to_http(exc: BaseException) -> tuple[int, dict]:
    try:
        raise exc
    except InvoiceFetchError:
        return 404, {"error": "not_found"}
    except Exception:
        return 200, {"error": "ignored"}

The second branch is a product incident. Unexpected errors belong on 5xx, with a stable error code and no stack in the body. 404 is for a proven missing resource, not for a timeout.

Proposed mapping tests:

def test_unknown_error_is_500_not_200():
    status, body = to_http(RuntimeError("disk"))
    assert status == 500
    assert body["error"] != "ignored"

def test_missing_invoice_is_404():
    status, _ = to_http(InvoiceFetchError("inv_9", FileNotFoundError()))
    assert status == 404

If the PR adds the mapper and not these two tests, it is still prove, not trust.

Where a local review assistant fits

A rubric without a second pair of eyes drifts. Some teams run a constrained pass over the diff and allow only three labels: TRUST, REVERT, PROVE.

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

MonkeyCode is an open-source coding assistant. Operator-supplied availability includes free model access and a free server option, which is enough to run that labeled pass on the branch. That is the only reason it belongs in this workflow: it can apply the table. It cannot certify that a catch block is correct. If the assistant and the table disagree, keep the table.

Redact .env, tokens, and production payloads before any assistant sees the patch.

git diff origin/main...HEAD > /tmp/pr.diff
# Review /tmp/pr.diff against the decision table.
# Allowed labels: TRUST, REVERT, PROVE.
# Require a named exception type or a revert.

Limitations

This protocol does not ban try. Isolation at process edges is real work. Queue consumers, HTTP adapters, and batch jobs need a poison-message policy.

It does not replace on-call data. A typed error with no metric is still silent, just louder in the process dump.

It does not decide product questions such as "is a missing invoice a 404 or a 204". That is a spec. The agent PR is not a spec.

Retries, caches, fallback providers, and empty test assertions are separate reviews. Do not fold them into this pass.

Who should not use this approach

• Do not use it to reject every error-mapping PR. Named mappings with tests belong in prove.
• Do not apply it to generated protobuf or OpenAPI stubs you do not own. Review the wrapper, not the generated catch.
• Do not use it as a substitute for a linter if CI already fails on except Exception. The table is for repos where those lines still land.
• Do not run an assistant review on diffs that contain credentials, customer data, or proprietary weights.

If the repo already has an exception budget, a typed Result convention, and a forbid-catch-all lint, this article is redundant. Keep the lint.

Close the loop on the contract

The interesting part of an agent PR is rarely the feature. It is the control flow the model invented so the feature would compile.

A catch-all is a policy change. Revert the generic ones. Prove the specific ones. Trust nothing that returns success after a failure. The merge decision stays with the reviewer who can name the outward type.