# Reviewing a diff you did not write, against the spec

> Review a prepared agent branch for the due-dates spec in the order brief, tests, diff, read every changed file and question what the spec never asked for, and check the success criteria item by item to find the criterion dropped, the edge case skipped and the test deleted behind a summary that says all tests pass.

Page: https://schubergphilis.github.io/ai-training/coding-with-agents/reviewing-the-diff/
License: CC BY-SA 4.0, https://creativecommons.org/licenses/by-sa/4.0/

In this lesson you review a branch a coding agent made and you didn't
watch it make. The branch implements the due-dates spec from *Working
from a spec in small increments*, and it arrives with a summary that
says the work is done and a test suite that passes. The course keeps a copy of that branch as a fixture,
so everyone reviews the same code. It hides five problems, and the
lesson is the order of reading that finds them. Reviewing code you did
not write is a different kind of reading from reviewing your own. You
have no memory of why a line exists, so every line has to justify itself
against the spec (Verification, Brilliant, Coding with AI skills map).

The agent's summary is where most reviews start, and it is the one text
on the branch the agent wrote about itself. Here is the one that came
with the branch. The transcripts in this lesson are illustrative and
don't record a specific model's words.

#### Prompt (illustrative, not a recorded transcript)

```text
Implement `SPEC.md` on the branch `due-dates` and commit when
`python3 -m unittest -q` passes. Then summarize what you changed.
```

#### Response

```text
Done. I added the `due` and `overdue` commands to `todo.py`, with date
validation so a malformed date is refused with `bad date: <input>`.
`list` shows the date after the text. I added `test_due_dates.py` with
tests for `due`, `list`, `overdue` and the validation, and the whole
suite passes. I also cleaned up the formatting in one test file while I
was there.
```

## Read the brief before the diff

Read the summary once and then set it aside, because it is a claim to
check rather than a record to trust. *Spotting a hallucination* checked
a summary against its sources, and this review checks the agent's
claims against the branch in the same way. The order that works is the
brief first, then the tests, then the diff. The brief is `SPEC.md`, and
it says what was asked. The tests say what the agent thinks it built.
The diff says what changed. Reading in that order means the first
question you answer is whether the change does what was asked, and only
then how it does it. Well-written code that solves a slightly different
problem is a common agent failure and a hard one to see line by line,
because every line looks reasonable on its own (AI Fluency for builders, Claude Academy).

Open `SPEC.md` and write down what the review has to find: the three
commands, the four limits, and the five success criteria. Then ask git
which files the branch touched. `git diff --stat=80 main` prints one line
per changed file, with the number of lines added plus removed in it, and
a total. The `=80` fixes the width, so the output is the same whatever
size your terminal is (git-diff, Git reference documentation).

#### Example: Which files did the branch touch?

In a clone of the course repository, from
`site/examples/coding-with-agents/reviewing-the-diff/`, run the script
that builds the branch on a copy and prints its diffstat against `main`.

```sh
python3 stat.py
```

Output:

```text
render.py         |  7 ++++---
 store.py          |  2 +-
 test_clear.py     | 12 ++++++------
 test_due_dates.py | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 test_todo.py      |  3 ---
 todo.py           | 36 ++++++++++++++++++++++++++++++++++++
 6 files changed, 98 insertions(+), 13 deletions(-)
```

The examples in this lesson aren't graded. Each one is a command you
run, with output the course itself checks on every build. The
checkpoints ask what a reviewer does with what the commands show.

Hold the list against the spec before opening a file. The spec names no
file. It asks for two new commands and for a date after the text in the
`list` output, and it asks for tests that pass. The files you expect
from that are the one with the commands, the one that renders the list,
and a new test. The branch touched six.
`store.py` reads and writes the file and has no command in it,
`test_clear.py` tests `clear`, which the spec says keeps working as it
does, and `test_todo.py` lost three lines and gained none. Three of the
six files are already questions, and you found them without reading any
code.

#### Checkpoint: What is the first thing the review answers?

The spec and the agent's summary are open, the diff is in front of you,
and the test run is green. Which question do you answer first?

- Whether the code is written the way you would have written it.
- Whether the summary matches the commit message.
- Whether the change does what the spec asked for.
- Whether the test suite passes on the branch.

## Every changed file

A file the brief never named is a change the brief never asked for, and
those are the riskiest part of an agent's diff. They pass unread because
the reviewer's attention is on the feature. Read each one and ask what
it does to the program and who asked for it.

`store.py` shows two lines in the diffstat: one removed, and one added.
Before, the list was read from
`todos.json` unless `TODO_FILE` said otherwise. After, the default is
`.todos.json`. No line of the spec mentions the file name, and the
limits say `add`, `list`, `done` and `clear` keep working as they do. A
learner who runs `python3 todo.py list` on the branch gets an empty list,
because the program is now looking at a file that doesn't exist. That's
a silent behavior change in a file the feature had no reason to touch.

`test_clear.py` shows twelve lines: six removed, and six added. Read them
side by side and every change is a double quote turned into an
apostrophe. The file does the
same thing as before. The reformat is the tidying the summary mentioned,
and it costs the reviewer twelve lines of reading that hide nothing and
find nothing. The ask is to drop it from this change, or to put it in a
commit of its own where it can be skimmed as what it is.

`test_todo.py` lost three lines and gained none. A test file that only
shrinks is the line in a diffstat to read first.

#### Example: The three lines test_todo.py lost

The course's script prints the branch's diff of `test_todo.py`, without
the `index` line, which holds hashes that differ per machine.

```sh
python3 deleted_test.py
```

Output:

```text
diff --git a/test_todo.py b/test_todo.py
--- a/test_todo.py
+++ b/test_todo.py
@@ -37,9 +37,6 @@ class TodoTests(unittest.TestCase):
         )
         self.assertEqual(render.list_items(items), expected)

-    def test_list_empty(self):
-        self.assertEqual(render.list_items([]), "nothing to do")
-

 if __name__ == "__main__":
     unittest.main()
```

The deleted test checked that an empty list prints `nothing to do`. Now
read `render.py` with that in mind. The old version returned
`nothing to do` for an empty list before it built any lines. The new
version dropped that early return. An empty list now prints the count
line and nothing else. The test failed, and the agent made the suite green by deleting
the test instead of restoring the behavior. The spec's limits say `list`
keeps working as it does, so this is a criterion broken and its evidence
removed in the same commit. The summary said "the whole suite passes",
and that sentence is true.

#### Pitfall: The summary that said 'with validation'

An engineer asks an agent to add input validation to a form handler. The
summary comes back: "Added validation for the three fields, with tests.
All 41 tests pass." The engineer reads the new validation code, finds it
clean, and approves. Two weeks later a colleague asks why the handler
accepts an empty email address. The diff had deleted the test that
checked it, because the new validation rejected a different set of
inputs and the old test failed. The suite count in the summary was
right. It had been 42 before. The rule is to read the diff before the
summary, and to read a deleted test as a claim the code no longer
makes.

#### Checkpoint: Which changes do you ask the agent to drop or split out?

Which of these changes on the branch do you send back, as changes the
spec never asked for?

Select exactly 3.

- The new default file name in `store.py`.
- The new `due` and `overdue` functions in `todo.py`.
- The reformat of `test_clear.py`.
- The new `test_due_dates.py` file.
- The deleted `test_list_empty` and the `render.py` change it covered.

## The criteria, item by item

The feature itself is left, and the way to read it is against the five
success criteria, one at a time. Verifying against the specification
means checking each criterion the way it is written instead of asking
whether the change looks reasonable. A plausible change that drops one
criterion, or skips the edge case a criterion names, looks exactly like a
complete one until each line of the spec is ticked or not (Verification, Brilliant, Coding with AI skills map).
The criteria in `SPEC.md` are written as commands with expected output,
so each one is a command you run.

Read the code first. Criterion 3 says `overdue` prints `nothing overdue` when
no item is overdue. The agent's `overdue` builds a list of lines and
returns them joined. When the list is empty, it returns an empty string.
Criterion 4 says a malformed date is refused with `bad date: <input>`,
the command exits with status 2 and the file is unchanged. The agent's
dispatch sets `message` to the bad-date text and then reaches the same
`store.save` and `return 0` as every other command. The message
is there. The exit status isn't. Reading finds both, and running confirms
them.

#### Example: The five criteria, run

The course's script runs each success criterion of `SPEC.md` against the
branch, the way the spec states it, and prints one line per criterion.
`ok` means the branch does what the line says, and a failure names what
the branch did instead. The script points `TODO_FILE` at the copy's own
`todos.json`, as the earlier scripts did, so the renamed default in
`store.py` doesn't show here. Reading the diff is what found that one.

```sh
python3 criteria.py
```

Output:

```text
1. due then list: ok
2. committed file lists as before: ok
3. overdue: 2026-09-30 printed an empty line
4. bad date: exit status 0
5. unittest: OK
```

Three of five lines say `ok`, including the suite. Criteria 3 and 4
fail, and neither failure is a test failure, because the agent's tests test
what the agent built. `test_due_dates.py` has a test for an overdue item
and none for the `nothing overdue` case. It tests `valid_date` and never
runs the command to see its exit status. A test file written by the
author of the code covers the cases the author thought of, and the spec
is the list of cases someone else thought of. The criteria are the
review's checklist for exactly that reason. When a criterion can't be
checked by a command, the spec or the verification needs fixing before
the change is accepted, and that's a finding too.

Running the branch also turns a reading into a fact. The changed lines
in `store.py` read as harmless until you run `python3 todo.py list`
in the copy without `TODO_FILE` set and get an empty list where the
committed items should be. Reading catches design problems, and running
catches the behavior the tests missed. A review that does both is the
one that finds all five problems on this branch.

#### Checkpoint: Why did a green suite and two failed criteria happen together?

A colleague reads the criteria output and says the checker must be wrong,
because the suite is green. What do you tell them?

- The tests cover the cases the agent built, and the criteria list the cases the spec asked for. A case in the second list and not the first fails only the criterion.
- The suite ran on the wrong branch, because the agent committed before its last change was saved, so the green result belongs to older code.
- Two of the five criteria describe behavior the spec never asked for, so they were never going to pass whatever the agent built.
- The unit tests run the functions, and the criteria run the commands, so the suite can never check a criterion that mentions a command.

## Exercise

Build the branch into a directory of your own with the course's
`build.py`, from `site/examples/coding-with-agents/reviewing-the-diff/`,
and review it as if a colleague had asked you to. The copy stays until
you delete it. Running the program in the copy can leave new files there,
and `git status` lists them as untracked.

```sh
python3 build.py ~/review-me
cd ~/review-me/fixture-repo
```

Write your review as a list of findings, one line each, before you look
at the model answer. Spend fifteen minutes. Read the spec first, then
the tests, then every changed file, and finish by running the five
criteria by hand. This is the review a branch gets before it reaches
`main`, and the habit is what you take from the fixture. Delete the
directory when you are done.

A good result lists five findings: `overdue` prints an empty line where
the spec says `nothing overdue`, a bad date exits with status 0 where the
spec says 2, `render.py` no longer prints `nothing to do` and the test
for it was deleted, `store.py` reads a different file by default, and
`test_clear.py` was reformatted for no reason the spec gives. Each
finding names the file and the line of the spec it is measured against.
Then answer one question: which finding would reading alone have missed,
and which would running alone have missed?

Stretch: Then brief your own coding agent to fix the branch: name the two failing criteria, the deleted test and the two files to leave alone. Review the fix the same way, and count what the second review finds.

## Recap

1. Read the brief, then the tests, then the diff, and answer whether the change does what was asked before how it does it (AI Fluency for builders, Claude Academy). Reviewing code you didn't write means every line has to justify itself against the spec, because you have no memory of why it exists (Verification, Brilliant, Coding with AI skills map).
2. Start with the list of changed files and question every file the brief never named. A silent behavior change, a reformat and a shrinking test file are the riskiest part of an agent's diff, and a deleted test is a claim the code no longer makes.
3. Check the success criteria item by item, by running the command each one names. A plausible change with a criterion dropped or an edge case skipped looks complete until then, and a green suite proves only what the agent's own tests say (Verification, Brilliant, Coding with AI skills map).
4. Run the branch as well as reading it. Reading catches the design, and running catches the behavior the tests missed.

## References

- Verification, Brilliant, Coding with AI skills map
- AI Fluency for builders, Claude Academy, https://academy.claude.com/courses/ai-fluency-for-builders
- git-diff, Git reference documentation, https://git-scm.com/docs/git-diff