How to Review an Agent’s Pull Request When You Did Not Type the Code

The pull request that taught me how to review agent code added retries to our outbound webhook sender. A background agent had picked up the ticket overnight: when a customer’s endpoint returned a 5xx, retry up to five times with exponential backoff, then mark the delivery as failed. In the morning there was a tidy PR waiting. A new RetryPolicy class, a config entry for max_retries, eleven new tests, all green. The description was clearer than most humans write. I read the diff, it looked right, I approved it.

Three weeks later a customer asked why their webhook deliveries were failing on the first 503 with no retry. Retries had never happened in production. The config key the agent added lived under webhooks.max_retries, but our settings loader reads webhook config from delivery.webhooks. The loader silently ignores unknown keys, so the policy fell back to its default of zero retries. The tests passed because every test constructed RetryPolicy(max_retries=5) directly and mocked the HTTP client. Not one of them went through the real config path.

I had reviewed the code the way I review a colleague’s code. That was the mistake. When a colleague writes a PR, I can lean on things I know about them: they ran it locally, they understand the settings loader, they would have noticed that retries were not happening. With an agent’s PR, none of that can be assumed. Nobody on the team had typed the code, and nobody had run it outside the test suite. Here is how I review these now.

Start with what was asked, not what was built

Before opening the diff, I read the task the agent was given: the ticket, the prompt, any follow-up instructions. This sounds obvious, and I skipped it for months. The PR description is the agent’s account of what it did. The task is what someone actually wanted. They differ more often than you would expect, and the difference is where scope creep and misunderstood requirements live.

For the webhook PR, the ticket said “retry on 5xx and on connection timeouts.” The PR only handled 5xx. The description did not mention timeouts at all. A human author would probably have said “timeouts to follow in a separate PR.” The agent simply did not do it, and its description was confidently complete about what it had done.

Read the file list before the code

The list of changed files tells you the shape of the change before any detail can distract you. For each file, I ask whether I would have expected it to change given the task. Missing files are as telling as extra ones. The webhook PR changed the sender, added the policy class, added tests, and touched config/defaults.yaml. It did not touch the settings loader or its schema. For a change that introduces new configuration, that absence should have been a question.

Extra files matter too. If there is one you did not expect, find out why before reading anything else. Often it is harmless tidying on the path. Sometimes it is a design decision nobody asked for, and understanding why agents rewrite files you never mentioned makes it easier to decide whether to keep that change or send it back.

Read the tests first, and ask whether they could fail

With human PRs, I usually read implementation first and tests second. With agent PRs, I reverse that. The tests tell you what the agent believed “working” means, and they are where an agent’s misunderstanding is most likely to be encoded as a guarantee.

The question I ask of each test is not “does this test pass?” but “what would have to be broken for this test to fail?” For the webhook tests, the answer was: only the RetryPolicy class itself. The tests constructed the policy by hand, injected a mock client, and checked the number of calls. The integration between config and policy, which is where the real bug was, had no test at all. Eleven tests, all testing the one part that was least likely to be wrong.

A quick practical check: run the new tests against the main branch with the implementation reverted. If they still pass, they are not testing the change. If they fail for the wrong reason, like an import error because the class does not exist yet, that tells you little. I look for at least one test that fails on the old code for the reason the ticket describes.

Look for things that look right but were never checked

Agents are very good at producing code that follows the patterns of the code around it. They are much less reliable at verifying that those patterns connect to reality. The categories I now look at specifically:

Configuration keys and environment variables. Does the key the code reads match the key the loader provides? Does the environment variable exist in the deployment config? This is exactly the class of bug that sank the webhook PR, and it is common because the agent often writes both sides of the contract in one PR, consistently wrong.

Library APIs and options. Does the function actually accept that keyword argument? Does the library version you pin support it? Agents sometimes use an option that exists in a newer version, or in a similar library, or nowhere. The code reads naturally either way.

New helpers that duplicate existing ones. Before accepting a new utility function, I search for whether one already exists. Agents frequently write a fresh format_currency or retry_with_backoff when the codebase already has one. We had an existing backoff helper. The agent wrote a second one inside RetryPolicy.

Error handling that swallows. Broad try/except blocks, fallback defaults, silent returns. These make tests pass and hide failures in production. The zero-retry default was a silent fallback. A missing config key should have raised an error at startup.

Run it outside the tests

This is the step that would have caught the webhook bug in five minutes, and it is the one people skip most, because the green tests feel like evidence it works.

For any change with observable behaviour, I now run the code in a realistic path at least once. For the webhook sender, that would have meant pointing a test delivery at an endpoint that returns 503 and watching the logs. With the real config loaded, the policy would have logged zero retries on the first failure, and the missing key would have been obvious.

When nobody typed the code, nobody has run it. The test suite ran it in a controlled, mocked world. Someone needs to run it in the real one before it merges, and that someone is the reviewer.

Ask the agent, but treat answers as leads, not evidence

One real advantage of reviewing agent code is that you can interrogate the author at any time without waiting for them to wake up. I often ask the agent to explain a specific choice: “why does the policy default to zero retries?” or “where is webhooks.max_retries read in production?”

Those questions are useful, but the answers need checking. An agent asked where a config key is read will sometimes describe how it should be read rather than tracing how it is actually read. When I later asked the question about the webhook key, the agent confidently said the settings loader “merges all keys under webhooks into the delivery config.” It does not. The answer pointed me at the right file, and reading that file showed me the bug.

The useful framing: an explanation from the agent tells you where to look. It does not tell you what you will find.

Keep agent PRs small enough to review this way

Everything above takes time. For a hundred-line change, it is maybe twenty minutes. For a thousand-line change, it is not realistic, and the honest outcome of a large agent PR is usually a rubber stamp. I now send back agent PRs that are too large to review properly and ask for them to be split, the same way I would with a human colleague.

We also changed how tasks are written for background agents. Each ticket names the integration points the change must touch (“config is loaded via settings/loader.py, schema in settings/schema.py“), states the behaviour that must be verified end to end, and asks for at least one test that goes through the real configuration path. That last requirement alone has caught two config mismatches before review.

The checklist I actually use

1. Read the task. Note anything the PR description does not mention.
2. Read the file list. Question every unexpected file and every expected one that is missing.
3. Read the tests. For each, ask what would have to break for it to fail. Run them against the old code.
4. Read the implementation for unchecked contracts: config keys, library options, environment variables, duplicated helpers, silent fallbacks.
5. Run the change once in a realistic path, with real configuration.
6. Ask the agent about anything surprising, then verify its answer in the code.
7. If the PR is too large to do all of this, send it back to be split.

Whose code it is

The retry fix, once we found the bug, was two lines: move the key under delivery.webhooks and make the loader reject unknown keys. We also added a test that loads the real settings file and asserts the webhook policy has five retries. That test would have failed on the original PR.

What stayed with me is that I was the reviewer of record on code nobody on the team wrote or ran. The agent was not accountable for the customer’s lost webhooks. I was. Reviewing an agent’s PR is not a lighter version of reviewing a colleague’s. It is the only point where a human actually checks that the code does what the ticket asked, in the system it will actually run in, and it deserves to be done as if that is true.