How to review code you did not write

The agent says it is done. git diff --stat reports 912 lines across fourteen files, the tests are green, and you have read roughly none of it. At three in the morning, when the refund endpoint starts double-charging, you are the person on call for those 912 lines.

Reviewing this is a different job from reviewing a colleague's pull request, and treating it the same way is why it goes badly.

Why this is not a normal review

With a colleague, half the review is a conversation about intent: why this shape, why not the other approach, what made them extract that helper. The answers reveal whether the design holds. With agent output there is nothing behind the choices to interrogate. Ask why a function takes an options object and you get a fluent justification generated after the fact, which tells you nothing about whether it was right.

The second difference is worse. The code is fluent. Names are apt, structure is conventional, comments read well, error handling is present. That is exactly what correct code looks like, so wrongness arrives well dressed. A human writing something dubious leaves fingerprints: an awkward name, a hedge in a comment. Here there are none, and plausibility is not evidence.

Third: nobody else has read it either. No author ran it locally, no second reviewer will catch it. Your pass is the only pass.

The order that makes a big diff reviewable

A 900-line diff read top to bottom is unreviewable. You lose concentration around file five and start skimming, and skimming a fluent diff finds nothing. Order fixes this: it front-loads the questions that can kill the whole change.

1. Does it actually run, and do the tests touch the new path

Green tests mean less than usual, because the same session that wrote the code wrote the tests. Run it yourself. Then check that a test genuinely exercises the new branch, rather than covering the file it lives in. The quickest checks: coverage on that one file, or the deliberate break below.

2. The shape of the diff, before any of its contents

Four commands, thirty seconds, and they catch the changes you never asked for:

git diff --stat main...HEAD                        # size and spread
git diff main...HEAD --diff-filter=D --name-only   # anything deleted
git diff main...HEAD --diff-filter=A --name-only   # anything new
git diff main...HEAD -- package.json '*.lock' '*.yaml' '*.env*'

You are looking for surprises rather than defects: unexpected deletions, a new dependency for a small problem, a config value changed to make something pass, a migration nobody asked for. New dependencies deserve a real pause: a library pulled in to parse one date is a supply-chain decision made by something optimising for a green test.

3. The dangerous boundaries

Now open files, but not all of them. Go straight to anything touching authentication or authorisation, money, personal data, external I/O, or the database schema. That is usually two or three files out of fourteen, and it holds most of the risk. Read those line by line, in full context rather than as a diff.

Everything else can be read at speed. Attention is finite. Spend it where a mistake is expensive.

4. Error paths and failure modes

Happy paths in agent-written code are usually fine. It is the second call failing after the first succeeded that gets missed. For each new side effect, ask what happens if it throws halfway, whether a retry double-applies it, and whether a failure leaves data in a state no code path expects.

export async function refundOrder(orderId: string, amountPence: number) {
  const order = await db.order.findUnique({ where: { id: orderId } });
  if (!order) throw new NotFoundError("Order not found");

  await payments.refund(order.paymentIntentId, amountPence);
  await db.order.update({
    where: { id: orderId },
    data: { status: "refunded", refundedPence: amountPence },
  });
}

That reads well: a null check, a typed error, sensible names. It also refunds any amount you pass regardless of the order total, refunds again on every call, and if the process dies between the two awaits the money is gone with no record of it. No test that calls it once with valid input notices any of that.

5. The "why does this exist" pass

Last, sweep the parts you skimmed and stop at anything you cannot explain. An abstraction with one caller. A configuration option nothing sets. A try/catch around something that cannot throw. These are usually residue from an approach abandoned mid-session, and they confuse whoever reads the file next: you, plus a fresh agent.

Things to grep for

Mechanical, fast, and worth doing on every agent-written branch:

rg -U -n 'catch\s*\([^)]*\)\s*\{\s*\}' src        # swallowed exceptions
rg -n 'as any|@ts-ignore|@ts-expect-error' src
rg -n '\.only\(|\.skip\(|xit\(|xdescribe\(' src
rg -n 'TODO|FIXME|XXX|for now|temporary' src
rg -n 'process\.env\.' src | rg -v '\.test\.'

And two on the diff itself, the highest-yield check here:

# every assertion the branch removed
git diff main...HEAD -- '*.test.ts' '*.spec.ts' | rg '^-.*(expect|assert)'

# and any test file it deleted outright
git diff main...HEAD --diff-filter=D --name-only -- '*.test.ts' '*.spec.ts'

Deleted or weakened assertions are the most common way a failing suite becomes a passing one, and they are invisible in a summary that says "fixed the failing test".

Read the tests first, then break the code

Open the test files before the implementation. Tests are a compact statement of what the author believed the code should do, and you can read them in a fraction of the time. If they describe behaviour you did not want, stop there. If they only assert that the function returns something truthy, you now know the green suite means nothing.

Then break it on purpose. Pick the most important new behaviour, invert a condition or return a constant, and run the tests. If they still pass, the test does not cover what its name claims, and that took forty seconds to find out.

// in the implementation, temporarily:
if (amountPence > order.totalPence) {
  // guard deliberately removed to see whether any test fails
  // throw new ValidationError("Refund exceeds order total");
}

Revert afterwards. It is the fastest way to tell a real test from a test-shaped object, and worth doing wherever a bug costs money. More on why the suite is what lets you move quickly in tests make you faster.

Use the agent as a reviewer, but check its homework

Asking an agent to review its own diff is useful, as long as you do not treat the output as a verdict. It is good at recall: the missing null check, the unhandled rejection, the endpoint without validation.

Do not ask "is this correct". Ask questions with checkable answers:

• "In refundOrder, what happens if payments.refund succeeds and the database update throws? Quote the lines that handle it."
• "List every route added in this diff and the line that checks the caller owns the record."
• "Which assertions did this branch delete, and why?"

Each produces a claim with a file and a line number, which you verify in seconds. When the answer cites a check that is not there, you have found your bug.

The size rule

If a diff is too big to review, the task was too big. That is not a statement about your stamina. Nine hundred lines you have not read carry the same operational risk as nine hundred lines nobody wrote down, and merging them because the tests are green just defers the reading to an incident. Send it back, ask for the change split into pieces you can hold in your head, and review each one properly. An agent regenerates work far more cheaply than you debug it at three in the morning.