The Second Attempt Is a New Call: Reviewing Agent PRs That Add Retries

Agent pull requests often wrap a call in retry logic and label the change “more robust.” That label is not a review. A retry is a second invocation. It repeats every side effect the first invocation already performed.

Trust the wrapper only when the callee is idempotent, the budget is bounded, and the retried exceptions are named. Revert retries on POST, mail, charge, enqueue, or any other write that has no idempotency key. Test the duplicate path. The happy path the agent generated will not catch this.

The pattern that lands in review

Agents have seen more for attempt in range(3) snippets than duplicate-ledger incidents. The local diff looks like engineering. CI stays green because the new tests never fail after the first side effect.

A typical agent patch looks like the block below. Treat it as a labeled example, not as production code.

# agent-generated example — do not merge as-is
import time
import requests

def notify_billing(payload: dict) -> None:
    url = "https://billing.internal/v1/charges"
    last_error = None
    for attempt in range(5):
        try:
            response = requests.post(url, json=payload, timeout=2)
            response.raise_for_status()
            return
        except Exception as exc:  # noqa: BLE001
            last_error = exc
            time.sleep(0.5 * (2 ** attempt))
    raise last_error

Three defects sit in twelve lines. The catch-all exception retries programming errors. The POST has no idempotency key, so a timeout after a 201 still charges again. The sleep is CPU-themed backoff with no jitter and no retry-after handling.

Green unit tests usually mock a single success. That is not coverage of the new path.

Decision table for the retry site

Use the table before arguing about “best practice.” The row is the review, not the blog post the agent paraphrased.

Callee kind	Idempotent?	Agent default	Review action	Minimum test
GET of immutable bytes	Yes, if no logging side channel	Retry 3–5 times	Trust bounded retry on 429/503 only	Inject 503 then 200; assert one logical result
PUT with client document id	Yes, if server is replace-by-id	Retry on any exception	Keep, narrow the exception set	Two identical PUTs; assert one stored document
POST charge / mail / enqueue	No, unless an Idempotency-Key is stored	Retry on timeout	Revert or require a key + server dedupe	Timeout after send; assert one side effect
PATCH that increments	No	Retry on 500	Revert	Two applies; assert increment is 1, not 2
Local pure function	Yes	Retry “just in case”	Revert the wrapper	No retry test; delete the loop
DB transaction with rollback	Conditional	Retry serialization failures	Trust only named retryable SQLSTATEs	Force one serialization failure; assert one commit

If the PR cannot name the row, it cannot keep the retry.

Review protocol

What to trust

Trust a retry that names the failure class. requests.exceptions.ConnectionError and HTTP 429/503 are candidates. Exception is not.

Trust a budget you can read without executing the function. A maximum of three attempts, a wall-clock deadline, and a list of status codes can be reviewed statically. An open while True cannot.

Trust a callee that is already a replace or a read. PUT-by-id and GET-by-hash are the boring cases. They are the only cases agents should get for free.

RETRYABLE_STATUS = frozenset({429, 503})
MAX_ATTEMPTS = 3

def fetch_object(session, url: str) -> bytes:
    last_error = None
    for attempt in range(1, MAX_ATTEMPTS + 1):
        response = session.get(url, timeout=5)
        if response.status_code in RETRYABLE_STATUS:
            last_error = RuntimeError(f"retryable {response.status_code}")
            continue
        response.raise_for_status()
        return response.content
    raise last_error

That function still needs a test that returns 503 then 200. It does not need a lecture about resilience.

What to revert

Revert retries that sit on non-idempotent writes. A timed-out POST is not a known failure. The server may have applied the body.

Revert except Exception and except BaseException. Those clauses retry KeyboardInterrupt in some stacks and retry TypeError in all of them. A type error will not succeed on attempt four.

Revert time.sleep in request threads when the PR adds no jitter, no deadline, and no upper bound. Five attempts with exponential sleep is a hidden stall. Under load it is a synchronized retry storm.

Revert retries that the agent added around a function that already retries. Nested wrappers multiply attempts. 3 * 3 = 9 is a behavior change even when each layer looks conservative.

What to test

Do not accept the agent’s new test file as evidence. Ask for three injections.

1. Failure before any side effect. The retry may succeed. Assert one effect.
2. Failure after the side effect is visible (timeout after send, 500 after commit, dropped response). Assert the effect count stays at one. If it becomes two, the retry is a second write.
3. Exhausted budget. Assert the original error type surfaces. A wrapped RuntimeError("failed after retries") is a new API.

Labeled pytest sketch:

# proposal: tests a reviewer can require before merge
from unittest.mock import Mock

def test_retry_does_not_double_charge():
    sent = []

    def post(url, json, timeout):
        sent.append(json)
        response = Mock()
        if len(sent) == 1:
            response.status_code = 201
            response.raise_for_status.side_effect = TimeoutError("after 201")
            return response
        response.status_code = 201
        response.raise_for_status.return_value = None
        return response

    session = Mock()
    session.post.side_effect = post

    try:
        notify_billing_with_session(session, {"order_id": "ord_9", "cents": 1999})
    except TimeoutError:
        pass

    assert len(sent) == 1, f"duplicate charge payloads: {sent!r}"

If the production function cannot be taught to take a session, that is also a review finding. Hidden transport is a second problem. It is not solved by adding sleep.

Mechanical scan of the diff

Run this against the patch before reading prose in the PR description. Agents put the real change in the loop, not in the summary.

git diff origin/main...HEAD -- '*.py' '*.ts' '*.go' |
  rg -n "retry|backoff|tenacity|Retrying|time\.sleep|attempts?\s*=|except Exception"

Then classify each hit:

• Read path, named errors, bound ≤ 3: keep and add the 503-then-200 test.
• Write path, no key: revert or add server-side dedupe first.
• Sleep without a deadline: revert or replace with a clock you can fake.
• New dependency on a retry library: review the default policy. Library defaults are still defaults. They become production behavior.

For TypeScript agents the same scan targets axios-retry, p-retry, and setTimeout inside catch.

git diff origin/main...HEAD -- '*.ts' '*.tsx' |
  rg -n "p-retry|axios-retry|retries:\s*[0-9]|await new Promise\(.*setTimeout"

A hit is not an automatic reject. It is a mandatory row in the decision table.

Idempotency is a stored contract

If the product needs retries on a POST, the PR must add a key the server remembers. Header theater is not enough.

POST /v1/charges HTTP/1.1
Idempotency-Key: ord_9:charge:v1
Content-Type: application/json

Review the store, not the header name. Require a test that replays the same key and asserts one ledger row. Require a test that uses a second key and asserts a second row. If the agent only asserts status_code == 200, revert the client retry until the store exists.

Client-generated keys also need a documented collision domain. uuid4() per attempt is the opposite of idempotency. The key must be stable for the logical operation, not for the HTTP attempt.

Optional second pass with a local model

A structured review prompt can sit next to the scan when the diff is large. It does not replace the table.

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

MonkeyCode’s free model access and free server option are relevant when the patch cannot leave the network and a second pass is still useful. Feed the model the diff plus the decision table. Ask it to list retry sites, callee kinds, and missing tests. Discard any site it cannot map to a row. Keep human merge authority for writes.

The article’s protocol does not depend on that pass. If the model is unavailable, the grep, the table, and the three injections remain the review.

Limitations

Static scans miss retries built from recursion, message redelivery, and queue nack. They also miss retries performed by a sidecar or an SDK default you did not import in-repo.

A model pass misses semantic idempotency. create_or_update is not proven by a name. Only a stored key plus a duplicate-call test is proof.

Backoff tests that use real time.sleep are slow and flaky. Inject a clock. If the PR cannot accept a clock argument, the retry is not under test.

This protocol does not rate libraries. It rates whether this callee may be invoked twice.

Who should not use this approach

Do not merge retry wrappers as a substitute for fixing the error. A 400 caused by a bad payload will not become a 201 on attempt three.

Do not apply model-assisted review as the only gate on charge, authz, deletion, or migration paths. Those diffs need a named reviewer and a failing duplicate test before merge.

Do not keep an agent’s retry on a batch job that already has queue-level redelivery. You now have two multipliers. Measure attempts at the boundary you own, then delete the inner loop.

Do not use unbounded client retries as load shedding. They concentrate load onto a recovering dependency. That is the opposite of robustness.

Merge bar

A retry PR is done when every new loop maps to a table row, every write has a stored idempotency key or has no retry, and the three injections exist. Anything else is a second call pretending to be a safety feature.

If you want a local second pass over a retry-heavy diff, MonkeyCode’s free model access and free server option can host that loop. The decision table still decides the merge.