Reviewing agent-written code

You asked an agent to add one endpoint to the FastAPI service behind your dashboard. It hands back a change touching fourteen files, all of it confident, well-formatted and plausible. Reading every line is not the answer — you would still not know what to look for. What you need is a checklist that goes after the specific ways agent-written code fails, in the order that matters.

The order matters because the expensive mistakes are not syntax errors. Those fail loudly at start-up. The expensive mistakes are code that runs perfectly and does the wrong thing quietly.

1. Did it do what you asked — and only that?

Before any logic, re-read your own prompt, then read the diff against it. Agent code is rarely broken; it is often correct code answering a slightly different question.

Then look at the file list before the code. If you asked for one endpoint and eleven files changed, ask why each one is there. Typical scope creep: an unrelated module reformatted, a dependency version bumped, variables renamed “for consistency”, a config option nobody requested, a helper extracted while it was in the area.

Bundled changes are worse than the sum of their parts. They make your actual feature unreviewable, and they enlarge the blast radius — everything that breaks if the change is wrong. Ask for a split. It costs one prompt, and you get to merge the part you wanted.

2. Does everything it calls exist?

A hallucinated import is a package, function or keyword argument that looks exactly right and does not exist. The plausible ones are the danger: an argument a library dropped two major versions ago, or a helper the agent assumed your repo has because most repos do.

# Is the package actually a dependency, not just imported?
grep -rn "httpx" pyproject.toml package.json
# Does this helper exist anywhere in the repo?
grep -rn "def normalise_response_date" app/
# What version is actually installed?
python -c "import httpx; print(httpx.__version__)"

A missing module fails at start-up, which is fine. A wrong keyword argument on a rarely-taken branch fails in three weeks, at 2 a.m., on one customer’s export.

3. Is this logic already in the repo?

Agents write what they need where they need it. So you get two date parsers, two retry wrappers, two shapes for the same response — all agreeing today. The problem is the next fix, which lands in one copy while the other silently diverges.

Search by behaviour, not by name, because the agent will have picked a different name:

# Looking for an existing retry wrapper under any name
grep -rn "max_retries\|backoff\|tenacity\|for attempt in" app/

4. Are errors handled, or hidden?

This is the single highest-value check. Compare what agents commonly emit with what you want:

# Agent version: every failure becomes a silent success
def import_responses(survey_id: str) -> int:
    try:
        rows = fetch_rows(survey_id)
        save(rows)
        return len(rows)
    except Exception:          # expired token, renamed column, typo — all identical now
        pass
    return 0

# Better: handle what you expect, let the rest crash loudly
def import_responses(survey_id: str) -> int:
    try:
        rows = fetch_rows(survey_id)
    except httpx.TimeoutException:
        logger.warning("survey api timed out", extra={"survey_id": survey_id})
        raise                  # the scheduler must see this fail, so it can retry
    save(rows)
    return len(rows)

The blanket version does real damage in your world: a nightly job that reports success every night while importing nothing. Exit code zero, no error logs, no alert. The only symptom is data that stopped arriving, noticed weeks later by whoever reads the dashboard.

Linters catch the pattern: S110 for try/except/pass, BLE001 for a blind except Exception, E722 for a bare except. If you really do want to ignore one specific error, say so explicitly with contextlib.suppress(FileNotFoundError) — that reads as a decision rather than an accident.

The mirror-image smell is no error handling on a network call:

# Agent version: no timeout, no status check
r = await client.get(f"{BASE}/surveys/{survey_id}/responses")
data = r.json()["responses"]          # KeyError when the body is an error payload

# Better: bounded wait, explicit failure, then parse
r = await client.get(f"{BASE}/surveys/{survey_id}/responses", timeout=20.0)
r.raise_for_status()                  # 4xx/5xx becomes an exception here, not a KeyError later
data = SurveyResponses.model_validate(r.json()).responses   # Pydantic v2 validates the shape

Without a timeout, a request can hang for as long as the connection stays open, holding a worker the whole time. Enough of those and your API stops answering while every health check looks fine.

5. Does it grant more access than the feature needs?

Agents optimise for “the thing works”. The cheapest way to make something work is usually to open it up.

# Agent version: makes the CORS error go away, and every site on the internet a client
app.add_middleware(CORSMiddleware, allow_origins=["*"], allow_credentials=True)

# Better: name the origins that should be able to call you with a session
app.add_middleware(
    CORSMiddleware,
    allow_origins=["https://dashboard.iff.example", "http://localhost:5173"],
    allow_credentials=True,
    allow_methods=["GET", "POST"],
)

That wildcard version is also self-defeating: the CORS specification forbids a wildcard origin together with credentials, so the browser blocks the request anyway.

Things to grep for in any diff: an asterisk in a permissions or origins list, public, Contributor or a role assigned at subscription rather than resource scope, a database user given write access for a read-only report, and — most important — a secret that moved into a variable name your bundler ships to the browser. Anything prefixed VITE_ or PUBLIC_ is visible to anyone who opens dev tools.

The other half of this check is what was removed. If the endpoints around the new one carry an auth dependency and the new one does not, that is the finding:

@router.get("/exports/{export_id}")                       # no dependency: anyone can read any export
async def get_export(export_id: str) -> ExportOut: ...

@router.get("/exports/{export_id}")
async def get_export(
    export_id: str,
    user: Annotated[User, Depends(current_user)],         # matches every sibling route
) -> ExportOut: ...

6. Do the tests test anything?

# Agent version: green forever
def test_compute_score():
    result = compute_score(SAMPLE_ROWS)
    assert result is not None          # any wrong number passes this

# Better: states the expected answer
def test_compute_score_weights_recent_waves_higher():
    assert compute_score(SAMPLE_ROWS) == pytest.approx(0.75)

def test_compute_score_rejects_empty_input():
    with pytest.raises(ValueError):
        compute_score([])

The fastest way to judge a test suite you did not write: change one number in the implementation and run it. If nothing goes red, the tests are decoration. Watch too for tests that assert on a mock the test itself configured — those verify the mock, not your code.

7. Are the numbers explained?

An unexplained literal — a 0.72 confidence threshold, timeout=30, text[:2000], sleep(5) — hides a decision nobody can audit. You cannot tell whether 0.72 came from analysis, a guess, or an example in the model’s training data. A named constant with one line of comment turns it into a claim you can check:

# Responses below this model confidence go to human coding.
# Set from the 2026 pilot: 0.72 gave 4% false positives on 1,200 hand-coded answers.
AUTO_CODE_CONFIDENCE_FLOOR = 0.72

Truncation constants deserve special suspicion, because they throw data away in silence.

8. Is the SQL answering the right question?

Wrong SQL is the hardest thing on this list to catch, because it runs and returns believable rows.

-- Agent version: the WHERE clause on the right-hand table makes this an inner join
SELECT c.name, COUNT(*) AS responses
FROM clients c
LEFT JOIN responses r ON r.client_id = c.id
WHERE r.submitted_at >= '2026-01-01'      -- NULL for clients with no responses, so they disappear
GROUP BY c.name;

-- Better: filter inside the join, and count rows that exist
SELECT c.name, COUNT(r.id) AS responses
FROM clients c
LEFT JOIN responses r
  ON r.client_id = c.id
 AND r.submitted_at >= '2026-01-01'       -- condition belongs to the join, not the result
GROUP BY c.id, c.name;

Other survivors of review: a COUNT inflated by a one-to-many join, a date range that drops the boundary day, SELECT * relied on by column position, and a != filter on a nullable column that also silently excludes the NULL rows. The only reliable check is running the query against a handful of known rows and counting the answer by hand.

How to ask an agent to justify a change

Ask for evidence, not reassurance. “Are you sure?” will get you a yes. These get you something checkable:

• Which file in this repo already does this, and why did you not use it?
• Quote the lines this change depends on.
• Link the documentation page for that keyword argument, for the version we have installed.
• Walk through what happens if that call times out.
• Write the test that fails without this change, and show it failing.

Each one produces a claim you can verify with a grep, a doc page, or a test run — and a wrong answer to any of them is a finding.

What to check in your own code

• grep -rn "except Exception" app/ and read every hit: does it log and re-raise, or hide?
• grep -rn "allow_origins\|VITE_\|PUBLIC_" . for wildcards and leaked secrets.
• grep -rn "\.get(\|\.post(" app/ and confirm each call has a timeout and a status check.
• grep -rn "assert .* is not None\|status_code == 200" tests/ for tests that cannot fail.
• grep -rn "LEFT JOIN" -A5 . and check whether a WHERE clause has quietly made it an inner join.
• Open the last three agent changes and count the files that had nothing to do with the request.