Amit Upadhyay for AI Wisdom

Posted on Nov 14 Edited on Nov 14 Originally published at aiwisdom.dev

Amit Got Stuck Reviewing Agent Code: Green CI, Wrong Behaviour

#ai #programming #codereview #testing

Every check passed. Twelve new tests, green CI, two approvals. And the invoices were rounding down.

A story: the names and numbers are invented, the failure is real.

The pull request looked great. Tidy functions, good names, twelve new tests, all passing. An agent had written it. I read it, liked it, and approved it.

Two weeks later a customer told us their invoices were rounding down. I dug in and found the bug in the new code. Then I opened the new test for it, and the test asserted the wrong value.

I rebuilt what happened:

The agent wrote the code, and the code rounded down.
The agent then wrote a test, and the test asserted what the code did.
The two agreed with each other, so CI went green.
I read a tidy diff and saw nothing that looked like a mistake. Because it did not look like one.

Green CI. Passing tests. A clean diff. And the wrong behaviour shipped anyway. I stared at the test for a long time.

When I'm stuck, I open the one reference that does not hand out answers. AI Wisdom is not a friendly tutor. It is a guru, and it makes you earn the solution.

I had no answer.

I sat with that one for a while.

Draw the problem

Once I could say it in one sentence, I drew it. First the version I would have understood in my first year. Then the one for people who review pull requests for a living.

Beginner view: a student who marks their own exam. The agent wrote the questions and the answers. Then it marked itself. Of course it got full marks.

%%{init: {"themeVariables": {"fontSize": "16px", "lineColor": "#8B97A8"}, "flowchart": {"wrappingWidth": 180, "nodeSpacing": 20, "rankSpacing": 32, "padding": 10, "subGraphTitleMargin": {"top": 6, "bottom": 20}}}}%%
flowchart TD
  A(["The agent"]):::person --> W
  subgraph W["The student writes the exam"]
    direction TB
    Q["Writes the code: it rounds down"]:::bad --> K["Writes the test: it asserts the rounded-down value"]:::bad
  end
  subgraph M["Then marks their own paper"]
    direction TB
    G["Test passes. Full marks."]:::ok --> C["CI is green"]:::ok
  end
  W --> M
  M -->|looks fine to| R(["Amit, the reviewer"]):::person
  classDef ok fill:#DCFCE7,stroke:#16A34A,color:#14532D
  classDef bad fill:#FEE2E2,stroke:#DC2626,color:#7F1D1D
  classDef stop fill:#FEF3C7,stroke:#D97706,color:#78350F
  classDef note fill:#E0E7FF,stroke:#4F46E5,color:#1E1B4B
  classDef person fill:#FCE7F3,stroke:#DB2777,color:#831843
  style W fill:none,stroke:#A3AEC0,stroke-dasharray:5 4,color:#475569
  style M fill:none,stroke:#A3AEC0,stroke-dasharray:5 4,color:#475569

Technical view: the same pull request, step by step. Both pieces came from one author, so they are consistent with each other. Consistent is not the same as correct.

%%{init: {"themeVariables": {"fontSize": "16px", "lineColor": "#8B97A8"}, "flowchart": {"wrappingWidth": 180, "nodeSpacing": 20, "rankSpacing": 32, "padding": 10, "subGraphTitleMargin": {"top": 6, "bottom": 20}}}}%%
flowchart TD
  S(["Amit opens the pull request"]):::person --> PR
  subgraph PR["What was in the pull request"]
    direction TB
    c1["New code: fluent, tidy, wrong"]:::bad --> c2["Twelve new tests, written by the same agent"]:::note
  end
  PR --> CI
  subgraph CI["What CI proved"]
    direction TB
    t1["The tests pass"]:::ok --> t2["That is all it proved"]:::stop
  end
  CI --> AP["Two approvals: nobody read the tests against the spec"]:::stop
  AP --> X["Two weeks later: a customer finds the wrong invoices"]:::bad
  classDef ok fill:#DCFCE7,stroke:#16A34A,color:#14532D
  classDef bad fill:#FEE2E2,stroke:#DC2626,color:#7F1D1D
  classDef stop fill:#FEF3C7,stroke:#D97706,color:#78350F
  classDef note fill:#E0E7FF,stroke:#4F46E5,color:#1E1B4B
  classDef person fill:#FCE7F3,stroke:#DB2777,color:#831843
  style PR fill:none,stroke:#A3AEC0,stroke-dasharray:5 4,color:#475569
  style CI fill:none,stroke:#A3AEC0,stroke-dasharray:5 4,color:#475569

Do you have this problem?

Your team merges agent code and you are not sure how closely it's being read
Reviews feel like skimming because the volume went up
Tests pass, and bugs still get through
You approved a pull request without reading its tests against the spec

I ticked all four. How many do you tick?

Why it hid

Agent-written code fails differently from a junior engineer's, and my review habits were built for the junior engineer. An agent's mistakes are fluent, so the cues I had learned to rely on were gone. And there was a lot more of it.

The fix that is a trap

An agent grading its own work is a poor judge. It will usually say yes, and it will sound convinced. Ask a fresh session to find problems instead.

The fix: read the tests first

I changed the order I read an agent's pull request in, because the risk sits in different places:

The spec or ticket. Know what should have happened before you see what did.
The diff summary. Do the touched files make sense? Anything unexpected is the first question.
The tests. Read these before the code.
New dependencies, config and migrations. These have the largest blast radius.
Error handling and boundaries. This is where fluent code hides its mistakes.
The rest of the code. By now I know what to look for.

Then I make the tests prove something. The strongest check for a test is to break the code and see whether it fails. Mutation testing automates that:

# Does the test suite notice when the code is wrong?
# First time only: npm init stryker  (installs the test-runner plugin and writes a config)
npx stryker run --mutate "src/billing/**/*.ts"

# Every new dependency, for a human to check
git diff origin/main -- package.json pnpm-lock.yaml | grep '^+' | grep -v '^+++'

The cheap version is to ask a fresh agent session, with no memory of the implementation, to write the tests from the spec alone.

The fix as a flow. Generation and evaluation are separate sessions. A person still reads the tests, the risks and the diff.

%%{init: {"themeVariables": {"fontSize": "16px", "lineColor": "#8B97A8"}, "flowchart": {"wrappingWidth": 170, "nodeSpacing": 20, "rankSpacing": 32, "padding": 10, "subGraphTitleMargin": {"top": 6, "bottom": 20}}}}%%
flowchart TD
  SP(["The spec"]):::note --> SE
  subgraph SE["Three separate sessions"]
    direction TB
    I["1. Implementing session: build this<br/>2. Fresh test session: tests from the spec only<br/>3. Fresh review session: the diff, no reasoning attached"]:::ok
  end
  SE --> CI
  subgraph CI["CI flags two things"]
    direction TB
    F["New dependencies and changed assertions"]:::stop
  end
  CI --> HU
  subgraph HU["Then a person reads"]
    direction TB
    H["The tests first, then the risks, then the diff"]:::note
  end
  classDef ok fill:#DCFCE7,stroke:#16A34A,color:#14532D
  classDef bad fill:#FEE2E2,stroke:#DC2626,color:#7F1D1D
  classDef stop fill:#FEF3C7,stroke:#D97706,color:#78350F
  classDef note fill:#E0E7FF,stroke:#4F46E5,color:#1E1B4B
  style SE fill:none,stroke:#A3AEC0,stroke-dasharray:5 4,color:#475569
  style CI fill:none,stroke:#A3AEC0,stroke-dasharray:5 4,color:#475569
  style HU fill:none,stroke:#A3AEC0,stroke-dasharray:5 4,color:#475569

What else to look for

Invented packages are worth a special mention. Models sometimes suggest package names that sound right and don't exist. Attackers register those names and wait, which is known as slopsquatting. The install succeeds, and CI stays green. So every new dependency gets checked by hand.

Failure	What it looks like	How to catch it
Tests that prove nothing	Asserts what the code does, or mocks the thing under test	Read the tests first, against the spec
Invented dependencies	A package that doesn't exist, or one a squatter registered	Check every new dependency by hand
Scope creep	Unrelated refactors mixed into the change	Compare the diff with the spec
Reinvented helpers	A new utility that duplicates one you already have	Search before accepting new helpers
Silent failure handling	A broad catch that hides errors and returns a default	Look hard at every catch and fallback
Loosened checks	A test or lint rule weakened to make things pass	Flag any change to existing assertions

Package hallucination is studied in this research paper.

Prove it with a checklist

For a large agent-written change I use a short list, because fluent code makes it easy to lose focus:

Does this solve the problem in the ticket, or a slightly different one?
Would each new test fail if the feature were broken?
Is every new dependency real, maintained and needed?
Are errors handled, or just caught and hidden?
Does anything touch auth, money, or data deletion?
Is there new code that duplicates something we already have?
Could I debug this at 2 a.m.?

Which check catches which failure? CI answers one question. A person has to answer the rest.

%%{init: {"themeVariables": {"fontSize": "16px", "lineColor": "#8B97A8"}, "flowchart": {"wrappingWidth": 190, "nodeSpacing": 20, "rankSpacing": 32, "padding": 10, "subGraphTitleMargin": {"top": 6, "bottom": 20}}}}%%
flowchart TD
  subgraph CI["Green CI tells you one thing"]
    direction TB
    C["The tests pass"]:::ok
  end
  subgraph PE["Only a person can tell you"]
    direction TB
    P["Do the tests assert the right thing?<br/>Is every new dependency real?<br/>Is this the problem in the ticket?<br/>Could I debug this at 2 a.m.?"]:::bad
  end
  CI --> PE
  classDef ok fill:#DCFCE7,stroke:#16A34A,color:#14532D
  classDef bad fill:#FEE2E2,stroke:#DC2626,color:#7F1D1D
  classDef stop fill:#FEF3C7,stroke:#D97706,color:#78350F
  classDef note fill:#E0E7FF,stroke:#4F46E5,color:#1E1B4B
  style CI fill:none,stroke:#A3AEC0,stroke-dasharray:5 4,color:#475569
  style PE fill:none,stroke:#A3AEC0,stroke-dasharray:5 4,color:#475569

If the answer to the last question is no, send it back, even if everything else passes. Readability is a safety property. Code that nobody on the team understands is a liability that only looks finished. If you cannot say whether the tests would fail when the feature is broken, you have a pull request with a nice diff.

The guru's last word

What I do first now: add a check that flags new dependencies and any change to existing test assertions, since those are the two cheapest catches. Set a size limit on pull requests, because nobody reviews 3,000 lines well. Then agree one rule as a team: whoever opens a pull request can explain every line of it, whoever or whatever wrote it. "The agent wrote that" is not an answer in an incident review.

Be kind to your reviewers as well. If you opened the pull request, read it yourself first, and note where you're unsure. A reviewer shouldn't be the first human to see the code.

Go deeper

My problem is fixed. The rest is in the full guide:

The full table of what agent mistakes look like and how to catch each one
The reading order for a pull request and why the risk sits where it does
How to keep generation and evaluation in separate sessions
Review depth by risk tier, with path-based code owners (how CODEOWNERS works)
Mutation testing to check that tests can fail (StrykerJS getting started)

DEV Community