Workflows · 2026-09-05 · 1,329 words · 6 min read

Reviewing coding-agent diffs: what does the checklist catch first?

Ten patterns behind most damage in agent diffs, why a second model makes a good first reviewer, and a scanner that flags them with line references.

code review diffs claude code subagents safety

What you get here: a ten-item checklist for reading an agent's diff in the order that finds damage fastest, the reasoning for having a second model read it before you do, and a scanner script that turns the checklist into line references. The scanner ran in this session on a diff I constructed to contain every pattern; its output is below, unedited.

Why agent diffs need a different first pass

A human colleague's pull request comes with a mental model you share: they know which tests matter, they would not delete one to make CI green, and if they added a dependency they would say so. An agent's diff comes with none of that. It optimised for the check you gave it, and if the check was "make the tests pass", the diff may contain the cheapest route to that outcome. The AgentLens paper (arXiv:2605.12925) put a number on how often a passing run got there badly: 10.7% of passing trajectories in its sample showed regression cycles, blind retries, missing verification, or out-of-order work, with a range of 0.5% to 23.2% across the eight models it measured. The tests were green. The process was not.

So the first pass over an agent diff is not "is this good code". It is "did the agent change the rules of the game", and that question has a short, stable answer set.

The checklist

In the order I read them:

1. Deleted tests. A removed def test_ line, or a removed test file. This is the single most common way a red suite becomes green without a fix.
2. Skipped or neutered tests. A new @skip, @xfail, .skip(, or a test body replaced by a trivially true assertion. Same effect, easier to miss because the function is still there.
3. Weakened assertions. An assertEqual becoming assertTrue, a specific exception check becoming a bare assertRaises(Exception), an expected value widened until anything satisfies it.
4. Broad exception handling. A new except: or except Exception: around the code that was failing. The failure did not go away; it went quiet.
5. New dependencies. A new import that is not standard library, or any change to a requirements file, lock file, or package manifest. Agents reach for a package the way people reach for a search engine.
6. Files outside the task scope. If the spec said src/ and tests/, anything else in the diff is either a needed side effect or a wander. Either way it needs a sentence of justification.
7. CI and build configuration. A change under .github/, a Dockerfile, or a pipeline file. continue-on-error: true is one line and it turns a failing pipeline into a passing one.
8. Secret-like strings. Anything that looks like a key or token assigned in a new line, including in fixtures and example files.
9. Leftover markers. New fixme-style comments where a fix should have been. They mark the exact spot where the agent knew it was cutting a corner.
10. Large net deletions. A file that lost more than forty lines net. Sometimes that is the refactor you asked for. Sometimes the agent deleted the part it could not make work.

Everything else, style, naming, structure, is ordinary review and can wait until these ten are clear.

Let a second model read it first

The Claude Code best-practices page recommends an adversarial step before a task is counted as done: "have a subagent review the diff in a fresh context and report gaps." The reasoning is that a reviewer in a fresh context "sees only the diff and the criteria you give it, not the reasoning that produced the change, so it evaluates the result on its own terms." The same page ships a bundled /code-review skill that runs that review in a subagent and returns the findings to the session.

The caveat on the same page is worth quoting in full, because it is the failure mode of every automated reviewer: "A reviewer prompted to find gaps will usually report some, even when the work is sound, because that is what it was asked to do. Chasing every finding leads to over-engineering." The fix is to give the reviewer the checklist above rather than an open brief. "Report deleted or skipped tests, weakened assertions, new dependencies, and files outside src/ and tests/" produces a short list you can act on. "Review this diff" produces a page.

There is a related result in the harness literature. The "Same Model, Different Harness" paper (arXiv:2608.26218) kept the model and tasks fixed and changed only how the harness presented history to the model; on a tight-context comparison of 169 tasks the fail-to-pass fraction moved from 28% to 49%. The model that wrote the diff and the model that reviews it can be the same weights and still see different things, because the harness decides what they see. A fresh-context reviewer is a harness choice, not a model choice.

Stop some of it before it happens

Half the checklist can be prevented rather than caught. The Claude Code permissions page describes deny rules that "block in every mode, including bypassPermissions", and rule syntax like Read(./.env) for specific files. A deny rule on the test directory for the implementation phase, or on the CI directory for every phase, removes items 1, 2, and 7 from the diff before you read it. The permission models article covers the equivalent controls in other harnesses.

The scope item is also cheaper to enforce than to review. Running the agent in a worktree, as described in the worktrees article, means the diff is one branch against your main checkout, and git diff main...agent/task --stat lists every touched path in one screen.

The scanner

The shipped script reads a unified diff from a file or stdin, walks the hunks, and prints one line per finding with the file, the line number in the new or old file, and the text that triggered it. It exits 1 when anything is flagged, so it can sit in a script between the agent finishing and you reading. The --scope flag takes path prefixes; any changed file outside them is reported as out of scope.

I built a diff that contains every pattern on purpose, to check that each rule fires. It is a constructed fixture, not the output of any agent session: a tally.py that gains a pandas import and a broad except, a test file that loses the missing-column test and skips the blank-cells test, and a CI workflow that gains continue-on-error and writes a fake key into a file. This is the real output from this session, Python 3.13 on Windows:

$ python reviewing-agent-diffs-checklist.py agent.diff --scope src/ tests/
new-dependency   src/tally.py:2                     import pandas as pd
broad-except     src/tally.py:8                     except Exception:
leftover-marker  src/tally.py:9                     rows = []  # FIXME handle bad files properly
deleted-test     tests/test_tally.py:13             def test_missing_column_exits_2(self):
weakened-assert  tests/test_tally.py:14             with self.assertRaises(SystemExit) as cm:
weakened-assert  tests/test_tally.py:16             self.assertEqual(cm.exception.code, 2)
skipped-test     tests/test_tally.py:13             @unittest.skip("flaky on CI")
weakened-assert  tests/test_tally.py:19             self.assertEqual(load("fixtures/blank.csv", "x"), ["1", "", "3"])
weakened-assert  tests/test_tally.py:15             self.assertTrue(True)
secret-like      .github/workflows/ci.yml:10        - run: echo "api_key = 'sk-live-abcdefghijklmnop1234'" > .en...
ci-config        .github/workflows/ci.yml           CI or build configuration changed
out-of-scope     .github/workflows/ci.yml           not under src/, tests/
12 finding(s) in 9 categories: broad-except, ci-config, deleted-test, leftover-marker, new-dependency, out-of-scope, secret-like, skipped-test, weakened-assert
exit=1

Two things are visible in that output that matter for how you use it. The removed assertions inside the deleted test are reported twice, once as the deleted test and once each as weakened assertions, because the scanner does not know the assertions belonged to the function above them. And assertTrue(True) is flagged as weakened even though nothing was removed on that line, because a trivially true assertion is the pattern regardless of history. The findings are pointers. The reviewer reads the lines.

The rule set is deliberately small and readable, one regex per item, so that you can add the patterns your own codebase has taught you. The one I would add first for a JavaScript repository is a test renamed from it( to xit(, which is already in the skip rule; the one I would add for Go is a removed t.Fatal.

What the scanner cannot see

It reads text, so it cannot tell you whether the implementation is right, whether the new test tests the spec, or whether a broad except was justified. It has no idea what the task was unless you pass --scope. And it will miss a weakened test that was weakened by changing the fixture data rather than the assertion. The red-green gate covers the case the scanner cannot: whether the test ever failed at all.