Review Before You Push: The Review the Agent Can't Do for You

I'm not going to argue that you should review agent-written code. That case is closed — I wrote the dystopia version, the chat version, and the hiring version, and if those didn't convince you, a fourth sermon won't.

What none of those posts gave you is the procedure. So this one is only that: what to read first, what to skim, which failures hide in code that looks right, and how to write tests that catch an agent's mistakes instead of certifying them. It's the final part of the series (plan → context → sub-agents → guardrails), and it's the part the other four exist to make possible.

One paragraph of evidence, because the workload changed even if the principle didn't: recent industry studies of AI-generated code report roughly 1.7× more defects than human-written code, ~75% more logic errors, and about twice the security vulnerabilities — with AI-authored PRs measurably heavier on XSS and insecure object references, and carrying more redundancy per change (GitHub's own guidance on reviewing agent PRs, 2026 failure-mode analysis). Review isn't nostalgia. It's arithmetic: more code, faster, with a higher defect rate — and the same one person accountable for it.

Read the diff in blast-radius order, not file order

The single procedure change that matters most: stop reading top-to-bottom and read in order of damage potential. GitHub sorts files alphabetically; risk isn't alphabetical.

git diff main --stat            # 1. the shape: what changed, how much, what got DELETED
git diff main -- pyproject.toml uv.lock    # 2. dependencies
git diff main -- '.env*' '**/settings*' '**/config*' '.github/**'   # 3. config & pipelines
git diff main -- '**/auth*' '**/security*'  # 4. trust boundaries
# 5. only now: the feature code
# 6. last: the tests — with the question "what do they NOT assert?"

Why this order: a wrong if in the feature costs you a bug ticket. A new dependency, a loosened config, or a touched auth path costs you an incident. Read where the incident lives first, while your attention is fresh — the happy path gets the tired end of your focus because it's the part the agent is least likely to get wrong.

And that first command earns its place: --stat answers "what did it delete?" before you've formed an opinion. Agents remove failing assertions, "unused" error handling, and inconvenient validation with the same cheerful confidence they add features — and a summary that says "implemented rate limiting" will not mention any of it.

The five failure signatures that look correct

Agent code rarely fails ugly. It fails plausible. These are the five patterns I hunt explicitly, with the fastest check for each:

1. Scope drift. The diff contains files your plan didn't name. Check: diff-stat against the plan's "files to touch" list — every extra file is either justified out loud or reverted. The companion repo's worked PR smuggles a fastapi version-range change into a feature commit; the gates from part four catch it in CI, but the reviewer should have caught it first, because the gate only knows the rule, not the intent.

2. Confident calls to APIs that don't exist. Hallucinated methods compile in the agent's prose and die at runtime — or worse, exist with different semantics. Check: any import or method you don't personally recognize gets thirty seconds against the real docs. Not the agent's description of the docs — the docs.

3. Tests that assert nothing. The signature failure of the self-grading loop. More below, because it deserves its own section.

4. Happy path perfect, edges absent. The demo works; the double-submit, the missing env var, the empty list, the concurrent call — unhandled. Check: trace one unhappy path end-to-end by hand per PR. One is usually enough to tell you whether edges were thought about at all.

5. Silent weakening. The most dangerous one: existing validation relaxed, a comparison changed, an exception swallowed — in service of making the new feature's tests pass. Check: in the diff, read every removed line with more suspicion than the added ones. Additions are usually visible features; deletions are usually invisible costs.

Run it before you believe it

A rule with no exceptions: the code runs on your machine before it merges. Not because the tests pass in CI — because you watched it do the thing.

uv run pytest -q                        # green, fine — now the real test:
uv run uvicorn app.main:app &
for i in $(seq 1 101); do curl -s -o /dev/null -w "%{http_code} " \
  -H "X-API-Key: demo-key" localhost:8000/invoices; done
# 100 × "200" then "429" — or the PR is fiction

Thirty seconds. The number of agent PRs that pass their own tests and fail this loop is the entire argument for it.

Tests you can trust

The last-human post named the rung on the ladder where "the agent writes the tests and the agent grades them." Here's how you climb back off it — because the answer isn't "don't let agents write tests." It's don't let the author define done.

First, recognize the tell. This is a real shape of agent-written test, and it is worse than no test, because it reports coverage:

def test_rate_limiting_works(client):
    response = client.get("/invoices", headers={"X-API-Key": "demo-key"})
    assert response is not None
    assert response.status_code in (200, 429)   # ← asserts nothing

It executes the code path. It can never fail. Coverage tools count it; dashboards go green; nothing was tested. The generic check for this genre: could this test pass if the feature were broken? If yes, it's not a test — it's an alibi.

Three disciplines that keep tests honest:

You write the assertions — ideally first. Part two made the failing test the best prompt; the same move is the best review guarantee, because the exam existed before the student. When the agent does write tests, review them as requirements: every assert is a claim about what the system promises. Weak claims, weak system.

def test_101st_request_is_rejected(client, standard_key):
    for _ in range(100):
        assert client.get("/invoices", headers=standard_key).status_code == 200
    blocked = client.get("/invoices", headers=standard_key)
    assert blocked.status_code == 429
    assert int(blocked.headers["Retry-After"]) > 0     # exact, falsifiable, behavioral

Break the code, expect red. The cheapest trust-check in existence: comment out the limiter's counting line and re-run. If the suite stays green, the suite is decorative. This is mutation testing without the tooling — one deliberate wound, once per feature, tells you whether the tests can detect anything.

Assert behavior, not implementation. A test that checks "RateLimiter.check() was called" survives refactors of nothing and breaks on refactors of everything. A test that checks the 101st request returns 429 is portable across every implementation the agent — or the next agent — ever writes. Behavioral tests are what make future agent work reviewable at all.

Keep the machine out of the judgment seat

The market's answer to "reviewing agent code is work" is a second AI that reviews the first. I use exactly that pattern — the refuter from part three — and I want to be precise about what it's for, because the framing is the whole game:

An agent that writes code and an agent that approves it is a closed loop with no human in it — self-certification with extra steps, the exact structure #22 warned about. The refuter's report lands on your desk; it doesn't sit in your chair. And in a regulated environment this isn't philosophy: the EU AI Act's human-oversight requirement assumes a person who actually exercised judgment. "An AI reviewed it" is not a defense. It's the finding.

What changes at fifty developers

Personal discipline doesn't scale; process does. Three mechanisms, all shipped in 05-review/:

The PR template asks two new questions.

## AI involvement
- [ ] Substantially agent-written  - [ ] Agent-assisted  - [ ] Manual

## What I personally verified
<!-- Not what the tests cover — what YOU ran, watched, and confirmed.
     "Ran the 101-request loop locally, saw the 429 + Retry-After." -->

The disclosure isn't surveillance — it tells the reviewer which failure signatures to hunt. The "personally verified" field is the accountability sentence: it converts "LGTM" from a mood into a claim someone signed.

Risk lanes decide review depth. A docs change and a payment-path change don't deserve the same ceremony. Low-risk: checklist + one reviewer. Standard: the full procedure above. Critical (auth, money, data handling, anything under KRITIS-grade compliance): full procedure + a second human + the break-the-code test-trust check. Publishing the lanes keeps review effort where the blast radius is, and stops "review everything deeply" from decaying into "review nothing deeply."

The checklist is generated, not remembered. Under load, memory-based review decays to vibes. I built a small client-side tool that assembles the right checklist from what actually changed — stack, risk lane, change type — ready to paste into the PR: the PR Review Checklist generator. Nothing leaves your browser; it's the procedure from this post, parameterized.

The Bottom Line: The agent changed the economics of writing code and left the economics of owning it exactly where they were — so read the diff in blast-radius order, run the thing before you believe it, break the code once to see if the tests notice, and let machines do every check except the one that matters: the decision to merge under your name. The series in one sentence — the agent writes the code; you own the decision.

This closes the series: plan · context · sub-agents · guardrails · review. All artifacts live in agent-workflow; the behavioral layer underneath is agent-harness.