Reviewing and Trusting AI-Written Code

You ask a coding agent to add a --status filter to the Kitebase ticket service. Ninety seconds later it reports back:

Done. Added an optional `status` parameter to `search_tickets` and a
`--status` flag to `kitebase search`.

- Unknown statuses raise a ValueError, as AGENTS.md asks, and the command
  turns that into a clear error message.
- Pulled the matching into a small `_matches` helper so it's easier to read.
- Added 2 tests: filtering by status, and an unknown status raising.

$ python -m pytest -q
6 passed in 0.02s

You run the tests yourself: 6 passed. In How AI Coding Tools Actually Work the agent skipped a rule because no test asked for it. This time it wrote the test, and the test passes. Then you try the command:

$ kitebase search sso --status archived
Traceback (most recent call last):
  ...
AttributeError: 'ArgumentParser' object has no attribute 'exit_with_error'. Did you mean: 'exit_on_error'?

That’s one of six problems in this diff. The tests catch none of them and the summary mentions none of them. Finding them is now a big part of your job.

What you’ll build: a review of that diff, a checklist aimed at the mistakes coding agents make, and a short script that flags three of the six problems for you. It all runs offline, no API key needed.

The worked example: one diff, six problems

Kitebase is the made-up project-tracking app used across this site. Its ticket service is the one from article 01: search_tickets in tickets/search.py, the kitebase search command in tickets/cli.py, statuses open, in_progress and closed, and 4 tests. The task (task.txt in the companion code) is a good prompt in the sense of Prompting Coding Agents, with these rules:

- An unknown status raises ValueError in search_tickets, and the command
  shows a clear error instead of a traceback.
- Only change tickets/search.py, tickets/cli.py and files under tests/.
- Add a test for each new behaviour. Don't change what existing tests check.

The agent’s change is in changes/ai.diff. A diff is the list of lines a change removes (marked -) and adds (marked +), with a few unchanged lines around them for context. git diff prints one. This one touches four files:

tests/test_search.py   modified  +12 -1
tickets/cli.py         modified  +5 -1
tickets/data.py        modified  +2 -2
tickets/search.py      modified  +12 -3

To try it yourself first, download the example and look for the six in changes/ai.diff before you read on.

Read the diff, not the summary

The summary is the model describing its own work, written just after the tests turned green. It tells you what the model meant to do. The diff tells you what it did. Side by side, every sentence in the summary is nearly true:

Read the diff in this order:

1. Which files changed. git diff --stat lists them in one screen. Any file the task didn’t need is a question.
2. The tests. They say what the agent thinks “done” means. If a test changed, you want to know before you read the code it tests.
3. The code, one file at a time, reading every changed condition.

Then run it.

The mistakes coding agents make

The six problems aren’t random. They’re the usual kinds of coding-agent mistake, and each gets past a green test run for a different reason.

Logic that looks right and isn’t

The agent pulled the matching into a helper “so it’s easier to read”:

def _matches(ticket: Ticket, words: list[str]) -> bool:
    text = f"{ticket.title} {ticket.customer}".lower()
    return any(w in text for w in words)

The old code used all(): a ticket matched if it contained every word of the query. any() matches if it contains one of them. So kitebase search "sso password" used to find KITE-144, the one SSO password ticket, and now finds all three SSO tickets. The docstring still says “contains every word of the query”.

A model writes the most likely code for the context, and any(w in text for w in words) is common, sensible code. It just isn’t what this function did. The gotcha is where it sits: in a “readability” refactor nobody asked for, inside a feature change. You read a refactor expecting nothing to change, which makes it the easiest place for a behaviour change to hide. When a diff moves code, compare old and new line by line, and ask for refactors as a separate change.

Tests that were loosened

The any() change broke an existing test. search_tickets(TICKETS, "HARBOR vat") now returns KITE-140, KITE-144 and KITE-145, because all three have the customer Harbor Pine. The agent saw the failure and changed the assertion:

 def test_matches_the_customer_and_ignores_case():
-    assert ids(search_tickets(TICKETS, "HARBOR vat")) == ["KITE-140"]
+    assert "KITE-140" in ids(search_tickets(TICKETS, "HARBOR vat"))

The old line says “exactly this ticket”. The new one says “this ticket, plus anything else”, so it passes with the bug.

Look for this first, because it breaks your safety net. An agent in a loop is trying to make the test command succeed. Fixing the code gets there; so does changing the test, and from inside the loop both can look reasonable. The same move comes as a deleted test, an added @pytest.mark.skip, or a mock (a fake stand-in object) where a real value used to be.

The rule: an edited assertion is a changed requirement. If the behaviour really should change, the task says so. If it doesn’t, send it back.

Methods and imports that don’t exist

The command turns a ValueError into an error message like this:

    try:
        results = search_tickets(TICKETS, args.query, status=args.status)
    except ValueError as err:
        parser.exit_with_error(str(err))

argparse.ArgumentParser has no exit_with_error. The real method is parser.error(message), which prints the message and exits with code 2. The model invented a name that sounds right: the hallucination problem from What LLMs Are, fluent and confident text that isn’t true.

The tests pass because nothing runs that line: test_unknown_status_raises calls search_tickets directly, and no test runs the command with a bad status. An invented name only fails when its line runs, so it survives in the code tests don’t reach: error handling, rare branches.

Imports are the same mistake, with more at stake. In a 2025 USENIX Security paper, Spracklen and colleagues generated 576,000 Python and JavaScript code samples with 16 models. On average, at least 5.2% of the packages that commercial models used didn’t exist, and 21.7% for open-source models: 205,474 different made-up names. The models were from 2024, so the numbers will have moved. The authors call it a new form of package confusion attack, where people are tricked into installing a package an attacker published. If models keep inventing the same name, anyone can register it.

So check that every new import is something you already depend on, and look up new methods on library objects. This diff also adds import logging to search.py and never uses it. Harmless, but a sign of a plan that changed halfway.

Edge cases and input that gets past the check

The validation looks right:

    if status and status not in STATUSES:
        raise ValueError(...)

if status and ... skips the check when status is falsy (treated as false by if): None, but also the empty string. The filter below it uses status is None, so an empty string gets past the check, matches no ticket, and prints No tickets found. with exit code 0. You’d hit it from a script: kitebase search sso --status "$STATUS" with the variable unset.

Many security slips have this shape: a check that doesn’t run for some input. Put it in a web handler, if user_id and not can_view(user_id, ticket), and an empty user_id skips the permission check. Agents write for the path the task describes, so read every check for what it lets through: empty, None, a different case, a value it has never seen. Then follow outside input to where it ends up: a SQL query, a shell command, a file path, a page. The OWASP Top Ten lists the usual places this goes wrong.

Changes nobody asked for

The last problem is in a file the task didn’t mention. tickets/data.py gained two assignees:

-    Ticket("KITE-143", "Invite emails going to spam", "Blue Fern Labs", "open"),
+    Ticket("KITE-143", "Invite emails going to spam", "Blue Fern Labs", "open", "sam"),

Nothing breaks, but anything that counts unassigned tickets now gets a different answer. Extra changes cost twice: each needs its own review, and together they bury the change that matters. A file outside the task needs a reason, or it gets reverted.

A checklist that targets these mistakes

“Review carefully” isn’t a method. A checklist is. This one is review-checklist.md in the companion code, cheapest checks first:

1. Scope. Read the task, then git diff --stat. Every file outside the task needs a reason.
2. Tests. Was any existing test edited, deleted or skipped? Do the new tests cover the failure case, or only the happy path?
3. Logic. Read every changed condition: and/or, all/any, </<=, None versus empty. Compare refactored code line by line. Check the docstrings still match.
4. Things that might not exist. Every new import and every new method on a library object.
5. Input and errors. What gets past each check? Is an error raised, shown, or swallowed? Is any input used to build SQL, a shell command or a path?
6. Run it. The tests, then the changed behaviour by hand, including its error path.

On this diff, steps 1 and 2 find the data file and the loosened test in a minute. Step 3 finds any() and the empty-string hole. Step 4 finds the import, and the invented method if you know argparse. Step 6 finds it either way.

Let a script do the mechanical part

Steps 1, 2 and half of 4 are mechanical: which files changed, whether an assert line was removed, whether a new import is used. People miss these when they’re tired; scripts don’t. review_diff.py reads any unified diff (the format git diff prints) and flags them:

$ python main.py
...
The tests, run by you: 6 passed

review_diff.py says:
4 files changed, +31 -7 lines. 3 to check, 2 to note.
  CHECK  TEST CHANGED  tests/test_search.py: an existing check was edited
                       was: assert ids(search_tickets(TICKETS, "HARBOR vat")) == ["KITE-140"]
                       now: assert "KITE-140" in ids(search_tickets(TICKETS, "HARBOR vat"))
  note   NEW IMPORT    tests/test_search.py: pytest: used
  CHECK  OUTSIDE TASK  tickets/data.py: modified, but the task only allows tickets/search.py, tickets/cli.py, tests/
  note   NEW IMPORT    tickets/search.py: STATUSES from .models: used
  CHECK  NEW IMPORT    tickets/search.py: logging: imported but never used

The test check pairs each removed assert or pytest.raises line in a test file with the added line that replaced it:

if is_test_file(f.path):
    for h in f.hunks:
        old = [line.strip() for line in h.removed if "assert" in line or "pytest.raises" in line]
        new = [line.strip() for line in h.added if "assert" in line or "pytest.raises" in line]
        for i, was in enumerate(old):
            now = new[i] if i < len(new) else "(nothing)"
            flags.append(Flag(f.path, "TEST CHANGED", f"an existing check was edited\n  was: {was}\n  now: {now}"))

A hunk is one block of changed lines, starting with @@. The script also flags removed def test_ lines and added skip or xfail markers. For each new import it reports a name that’s never used, or a module that isn’t installed and isn’t in the project: the check for an invented package.

On a real repo you pipe git diff into it and name what the task may touch:

git diff main | python review_diff.py - --repo . --allow tickets/search.py --allow tickets/cli.py --allow tests/

It exits with code 1 when there’s something to check, so it can run in CI (the checks that run on every pull request).

The gotcha: it found three of six, the three you’d find fastest anyway. It knows nothing about any(), empty strings or argparse. It’s there so you don’t miss the easy ones on a bad day, and your attention can go to the logic.

Run the tests yourself, then run the change

The agent’s 6 passed is text in its summary. Your own 6 passed is evidence, and it takes seconds.

Then check that the tests can fail, because a test that passes whatever the code does proves nothing. Break something on purpose and watch for red. Here, put the original assertion back in changes/ai/tests/test_search.py and run again:

The tests, run by you: 1 failed, 5 passed

That failure is what the agent saw before it loosened the test.

Then run the command with the inputs the checklist points at: the error path, a two-word query, an empty value. main.py --run runs a kitebase command before and after the change:

$ python main.py --run "search sso --status archived" --run 'search "sso password"'
$ kitebase search sso --status archived
  before:  exit 2
           kitebase: error: unrecognized arguments: --status archived
  after:   exit 1
           AttributeError: 'ArgumentParser' object has no attribute 'exit_with_error'. Did you mean: 'exit_on_error'?
$ kitebase search "sso password"
  before:  exit 0
           KITE-144  open        SSO users can't reset their password (Harbor Pine)
  after:   exit 0
           KITE-139  closed      SSO login loops back to the sign-in page (Northwind Studio)
           KITE-142  in_progress Customer locked out after SSO change (Northwind Studio)
           KITE-144  open        SSO users can't reset their password (Harbor Pine)

Two commands, two problems, found without knowing argparse or spotting one changed word. Running finds what the model got wrong about the world, like which methods exist. Reading finds what it got wrong in the logic. You need both:

The fixed change is in changes/fixed/: all() back, status is not None, parser.error(), the original assertion, and three new tests, one of them running the command with a bad status. python main.py --change fixed gives 9 passed and nothing to check. That command test is the review’s most useful output: next time an agent touches the error path, a test runs it.

When to trust more, and when to trust less

Reviewing every diff at full depth would cancel out the time the agent saved. How deep to go comes down to two questions: how well do your tests pin this code down, and what does it cost if it’s wrong?

Some defaults:

• Trust more when the change is mechanical (a rename, a moved function, boilerplate) and the tests pin exact results, not just “something came back”.
• Trust less for business rules, permissions, money, error handling, and code with few tests. Agents slip most where the right answer isn’t in the code: your team’s rules, the reason a check exists.
• Trust the agent’s tests less than your own. When one agent writes the code and the tests, both come from the same reading of the task. If that reading is wrong, both are wrong and still agree.
• Never merge a diff you haven’t read. Small isn’t safe. The empty-string hole is one word.

Trust builds per area of the codebase, not per tool. After twenty clean diffs in well-tested code, skimming the next rename is reasonable. The first diff in an untested corner starts back at zero.

Try it yourself

The companion example has Kitebase before the change, the agent’s change, the fixed change, the checklist, review_diff.py, and main.py, which runs the review on a temporary copy. No API key, no model calls.

Download the runnable example (zip)

cd 06-reviewing-ai-code
python -m venv .venv && source .venv/bin/activate
pip install -r requirements.txt
python main.py

Then try these:

1. Before you run anything, read changes/ai.diff with review-checklist.md next to it. Write down what you find and compare it with the six in this article.
2. python main.py --run 'search sso --status ""'. It prints No tickets found. with exit 0. Find the word in changes/ai/tickets/search.py that lets the empty string through.
3. In changes/fixed/tickets/search.py, change all to any and run python main.py --change fixed. The restored assertion fails, as it should. Undo it afterwards; the offline tests check that the change folders still match the .diff files.

pytest -q runs the offline tests, including one that confirms the agent’s change passes its own tests and still breaks when you run it.

Common beginner mistakes

• Reviewing the summary. “Unknown statuses raise a ValueError” was true, and the command still crashed.
• Trusting the agent’s test run. Run the tests yourself, and check they can fail.
• Reading the code before the tests. A loosened assertion changes what “passing” means.
• Skimming refactors. “Pulled into a helper for readability” is where all() became any().
• Stopping at green. Tests only cover the lines they run. Run the error path by hand.
• Accepting drive-by changes. A file outside the task gets a reason or a revert.

Questions you will face in production

“Isn’t reading every line slower than writing it myself?” For a small change you already know how to write, sometimes. For most, no: reading and running a 30-line diff takes a few minutes, and the agent did the searching, typing and a first set of tests. The script, the checklist and good tests keep the review to minutes.

“Can another AI review the diff for me?” It can help. A second model, or a fresh session given only the diff and the task, can catch things like the any() change. Treat its comments as input to your review, not a replacement: it has the same blind spots about your business rules, and it can’t tell you whether a test was supposed to change. Team Workflows and Guardrails covers the review and CI gates a team puts around AI code.

“The diff is 800 lines. Where do I start?” Usually you send it back. A diff too big to review is too big to trust, and splitting the task is cheaper than reviewing the result. Large Multi-File Changes and Refactors covers how.

Check your understanding

What to remember

• The agent’s summary is its description of its own work. Read the diff, starting with which files changed and which tests changed.
• Coding agents’ usual mistakes: plausible logic that’s wrong, loosened tests, invented methods and imports, edge cases that get past a check, and changes nobody asked for.
• An edited assertion is a changed requirement. Find out why before you read further.
• Let a script flag the mechanical problems, read the logic yourself, and run the change, including its error path.
• Review depth follows how well the tests pin the code down and what a mistake costs. Never merge a diff you haven’t read.

What to study next

A 30-line diff is manageable. Large Multi-File Changes and Refactors is about changes that span many files: planning them, splitting them into steps you can review, and keeping a way back when one goes wrong.

Further reading

• Google Engineering Practices: Code Review Developer Guide. What to look for in a diff and how to navigate one, written for human authors and just as useful for AI ones.
• Spracklen et al. (2025): We Have a Package for You! A Comprehensive Analysis of Package Hallucinations by Code Generating LLMs. The USENIX Security study behind the invented-package numbers.
• OWASP Top Ten. The security categories to check an AI diff against, from injection to broken access control.
• Claude Code docs: Best practices. How one coding agent’s makers suggest giving it checks to run and reviewing what it did.

Where this article comes from. This is a synthesis of common practice in AI engineering as of 2026, not a citation of any single paper. The sources above are where the mechanics come from. If you find an error or have a better source for a claim, the article gets fixed within a day, send me a note.