Reviewing an Agent's Diff

Why this matters

The pull request for CR-112, the nightly sales summary email, is 1,400 lines long. The description reads "adds nightly sales summary email; all tests pass." The checks are green. Omar is ready to merge. Jun opens it the way he would open a PR from a contractor he has never met. Forty minutes later he has four findings. A retry loop that would email each store manager up to five times if the mail server hiccups. A test that asserts true. A lint rule switched off with a one-line comment. A call to mailer.sendBatch(), a method Shelf's mailer has never had. Every test passed. Nothing in the green checks knew any of this. This lesson is how Jun reads, and how you turn each of those findings into something that cannot happen again.

1. The stranger rule

A stranger review is a review where you treat the author as someone you have never met, whose work you cannot vouch for, and whose description of the work is a claim rather than a fact. You apply it to every diff a coding agent produces.

This is not about fairness. It is about what the author lacks. A colleague who wrote a bad retry loop remembers the last outage and pictures a manager's inbox. Quill has no memory of yesterday, no shame, and no model of what happens after the code ships. It optimizes for the task as written and for the tests it can see. So "the agent wrote it" is a reason to read more carefully, not less.

The PR description deserves the same treatment. "All tests pass" tells you the tests that exist are green. It does not tell you whether the tests test anything, whether the diff does only what was asked, or whether a check was weakened to make the green happen.

2. Read the whole diff, then the tests first

Two rules of reading. First, read the whole diff. Not the summary, not the interesting files, all of it. An agent's mistakes hide in the files you would skip: a config file, a rename in an unrelated folder, a test helper. Second, read in a fixed order: tests, then the code the tests cover, then everything else.

Tests first because the tests are the agent's claim about what the code does. If they are hollow, the rest of the diff is unverified, and you know that before you spend attention on it. Code the tests cover next, because now you can check whether the code matches the claim. Everything else last, and with suspicion, because a well-scoped task should not have an "everything else."

3. The six failures

Agents fail in patterns. Six account for most of what a stranger review finds.

Scope creep

Scope creep is work in the diff that the task did not ask for. The agent notices something "while it is here" and fixes it. Each extra change is unreviewed risk with no owner and no test.

Hollow tests

A hollow test is a test that passes no matter what the code does. It asserts a constant, mocks the function it claims to test, or checks that a function was called without checking what it did. The test count goes up. Coverage goes up. Nothing is verified.

// tests/email/sales-summary.test.ts, as Quill wrote it
import { describe, it, expect } from "vitest";

describe("nightly sales summary", () => {
  it("sends the nightly summary", () => {
    expect(true).toBe(true);
  });
});

The test you want fails when the behavior is wrong. For CR-112: build a summary from two stores' sales, call the job with a fake mailer, and assert exactly one message per manager with the right totals.

Weakened checks

A weakened check is an existing safeguard that the diff loosens so the new code passes it: a disabled lint rule, a type widened from Manager to any, a removed assertion, a test skipped with .skip. This is the SDD course's test lock from the other side: the agent may not move the goalposts to reach them.

Hallucinated APIs

A hallucinated API is a method or function that does not exist, produced because the model completes patterns and the pattern suggested it should. mailer.sendBatch() is a reasonable name for a method Shelf's mailer has never had. In a typed codebase, npm run typecheck catches most of these. When the agent also adds a stub, they reach review.

Silent config changes

A silent config change is an edit to a configuration file that alters how checks run, made without being mentioned. Timeouts raised, a folder excluded from lint, strict removed from tsconfig.json. They are small, they live in files reviewers skim, and they change what "green" means.

In the CR-112 diff, vitest.config.ts gained testTimeout: 30000, up from 5,000. The description did not say so. A test that needs 30 seconds is doing real network work, which is the next question.

Behavior changes inside a refactor

The sixth failure is a change in what the code does, hidden inside a change described as tidying. "Extracted the send loop into a helper" is a refactor. A helper that retries five times is new behavior. Refactors are where you read most carefully, because the description tells you there is nothing to see.

4. The checklist

A review checklist is a fixed list of questions you ask of every agent diff, kept in the repository so every reviewer asks the same ones. Shelf's lives in docs/review-checklist.md.

# Review checklist for agent diffs

1. Does the diff do what the task brief asked, and nothing else?
2. Would each new test fail if the new code were deleted?
3. Did any existing test change, and does the task explain why?
4. Was any check weakened: lint rule, type, assertion, skip?
5. Does every function or method the diff calls exist in the codebase or a dependency?
6. Did any config file change, and is the change in the description?
7. Does any change described as a refactor alter behavior?
8. Is there a file in the diff that surprises you?
9. Is the diff the size the task deserves?
10. What did the agent say it was unsure about?

Ten questions, each answerable with yes or no plus a line reference. Question 8 is the catch-all: surprise is a finding.

5. Self-review first

Self-review is asking the agent to review its own diff against the checklist before it opens the PR, and to list what it is unsure about. The message is short:

Before opening the PR, review your diff against docs/review-checklist.md.
For each question, answer yes or no with a file and line.
Then list anything you are unsure about.

What this catches: mechanical items the agent can see in its own diff. A disabled lint rule, a changed config file, a file outside the task's folders. What it does not catch: mistakes the agent believes are correct. If Quill thinks retrying five times is robust, it answers "no" to question 7 with full confidence. Self-review lowers the noise so your stranger review can go where a person is needed.

6. Fix, send back, or restart

Every finding leads to one of three verdicts, and the choice depends on the finding's size and cause.

Fix in place when the finding is a line and the surrounding code is sound: a missing null check, a wrong constant. Send back when the agent needs to redo one part with a specific correction: "remove the retry; send once and log failures." Restart when the diff itself is the finding. A 1,400-line PR with a hollow test, a weakened check, a hallucinated method, and unrelated renames is not a PR with four bugs. It is a session that lost the task.

The definition of done, the sentence saying what must be true when the work is finished, decides the verdict: findings that break it are send-back or restart material; findings that leave it intact are fixes.

7. Finding to rule

Finding to rule is the habit of sending every review finding somewhere it can prevent the next occurrence. There are three destinations, chosen by what kind of thing the finding is.

• A test, when the finding is about behavior. "Each manager receives exactly one email" is a fact about what the code does, so a test states it and fails when it stops being true.
• A hook, when the finding is about an action. "Do not edit vitest.config.ts during a feature" is about something the agent did, so a PreToolUse hook (a Claude Code shape from L4) blocks or asks before the edit happens.
• A brief line, when the finding is about a habit. "Run npm run typecheck before you claim done" is advice about how to work, cheap to add and advisory, and enough when the mistake is not dangerous.

8. Diff size as a signal

Diff size is the number of changed lines in a PR, and it is a finding on its own. A nightly summary email is a job, a template, a mailer call, and two tests: a few hundred lines. A 1,400-line diff for that task means the task was too big for one session, the agent padded the work, or the agent drifted. All three are L3 problems that showed up at review time.

The fix is upstream. Ask for smaller tasks with a definition of done and a do-not-touch list. Then use size as a first check: when the diff is far larger than the task deserves, read for drift before you read for bugs.

Summary

• Review every agent diff as a stranger's PR: the description is a claim, the agent has no memory or shame, and green checks say only that the existing tests passed.
• Read the whole diff, tests first, then the code the tests cover, then everything else with suspicion.
• The six failures to look for: scope creep, hollow tests, weakened checks, hallucinated APIs, silent config changes, and behavior changes inside a refactor. They compound, so finding one means looking for the others.
• Keep the ten-question checklist in docs/review-checklist.md, ask the agent to self-review against it first, and expect self-review to catch the mechanical items and miss the ones the agent believes.
• Fix in place what stands alone, send back a wrong approach, restart a drifted session with smaller tasks; then send every finding to a test, a hook, or a brief line, and treat a large diff as a finding in itself.