In January a coding agent opened about a fifth of the pull requests on our main service, usually with a person driving it. By May it was over half. The agent was better at the work than we'd expected, and because a change got cheaper to make, we made more of them. That part was fine.
Review wasn't. Our process was three years old and rested on an assumption nobody had written down: whoever opened the PR understood every line of it, and review was a conversation between two people who both knew what the change was for. Neither half was true any more. Often the author hadn't read every line, and for some of the code the reviewer was the first human to look at it.
In April we got a production incident out of it. The agent made a schema change correctly, and the author approved it without noticing it dropped a default. After that we changed the process, and this post is what changed and why.
The failure modes were specific
The first suggestion was to read more carefully, and it was wrong. Reviewers were already spending longer on agent PRs than on human ones, and the incident PR had two approvals. Attention wasn't the problem. Reviewers were looking at the wrong things, because agent-written code fails in different ways from human-written code.
Human PRs usually make a few deliberate changes, and the bugs are in the logic of those changes. Agent PRs usually have the change you asked for plus a halo of small changes next to it that seemed reasonable to the agent: a renamed variable, a reformatted block, a default removed because it looked unused, a null check added that changes behaviour, a test updated to assert the new behaviour instead of the old. Each one is plausible, and in a 400 line diff you can't see any of them.1
The second failure was in the tests. The agent writes tests, they pass, and reviewers took that as evidence. But the agent wrote the tests and the code from the same understanding, so a misunderstanding in the code shows up in the test too, and the test passes because it checks that the code does what the code does. A green suite written by the code's author is weaker evidence than one written by someone else, and with agents it's always the same author.
The third was scope. Ask a human to fix a bug and they fix the bug. Ask an agent and sometimes it fixes the bug and also refactors the module it lives in, because the refactor made the fix cleaner. The refactor might be good, but it's a second change riding along inside the first one's review.
What a PR has to contain now
The fix for all three was to change what a pull request needs before a reviewer gets assigned. We didn't write guidelines. We added checks in CI, and they fail.
The first check is a diff budget by intent. The PR description has to declare the intended scope as a list of files or directories, and the diff has to stay inside it. Anything outside fails the check, which lists the files that strayed. The author can either widen the scope, which is a visible edit to the description that the reviewer sees, or tell the agent to drop the extra changes.2 The halo mostly stopped appearing, because the agents we use read the check's failure output and learn what scope means in that repository.
# .github/workflows/scope.yml
- name: Enforce declared scope
run: |
scope=$(gh pr view "$PR" --json body -q .body \
| sed -n '/^## Scope/,/^## /p' | grep '^- ' | sed 's/^- //')
changed=$(git diff --name-only origin/main...HEAD)
out=$(echo "$changed" | grep -v -F -f <(echo "$scope") || true)
if [ -n "$out" ]; then
echo "Files outside declared scope:"; echo "$out"; exit 1
fi
The second check is that behaviour changes have to be listed. The description has a section for every change in behaviour someone could observe: a default changed, an error now thrown, a response field added, a migration. The agent writes it when it opens the PR, and the reviewer's first job is checking that list against the diff. Our incident PR would have had "removes the default on orders.currency" in it, or the reviewer would have asked why it didn't, since the schema file was in the diff.
This check is weaker. It only verifies that the section exists and isn't empty when certain paths change. What helps is making the list a required artefact. Agents are good at writing it, and reviewers are good at spotting a diff that contradicts it.
The third is that test changes get reviewed separately from code changes. Our review tool now opens the diff with tests collapsed and a banner saying tests changed, 4 files. The reviewer reads the code first, decides what they think it does, then opens the tests and checks whether the tests agree with them, rather than with the code. Of everything we did, that reordering helped most. Read the test first and you're primed to accept the code. Read the code first and the test becomes a claim you have to check.
When a test was modified and not just added, the description needs a one line reason for each one. "Updated assertion to match new behaviour" is a red flag sentence, and reviewers treat it like one.
What a reviewer does now
With those artefacts in place, the review itself looks different.
Read the declared scope and the behaviour list before the diff, and ask whether the scope fits the task. A bug fix that declares eleven files needs a question answered before it gets a review.
Read the code and form a view of what it does, then read the tests as claims and check them. For every modified test, decide whether the change is a correction or a capitulation.
Look for the halo. Even with the scope check, there can be incidental changes inside the scope. A reformatted block is fine. A removed line never is unless the description gives a reason.
Run it.3 For any PR that touches persistence, a queue or an external call, the reviewer pulls the branch and exercises the change by hand, or through the agent with a specific instruction to demonstrate the behaviour on the list. It takes ten to twenty minutes. The case for it is that a passing suite from the code's own author isn't evidence, and a person watching the thing happen is.
Don't review what you can't review. A 1,200 line PR doesn't get a review. It gets a comment asking for it to be split, the agent splits it, and that takes five minutes, where splitting a PR took a human two hours in 2023. The limit we settled on is 400 changed lines, not counting generated files and lockfiles, and a check enforces it.
What we stopped doing
We stopped trusting green. A passing suite is the floor, and a PR where the agent ran the tests it wrote and reports them passing is that same floor described twice.
We stopped giving agent PRs extra reviewers. The incident PR had two approvals, and the second approver assumed the first had read the schema change. Two half reviews add up to one review where nobody quite feels responsible. Now there's one named reviewer who owns the approval.
We stopped reviewing style. Biome does that, and the agent follows Biome. Every comment about naming or formatting is one that wasn't spent on the behaviour list.
Where the author fits
The person who drove the agent is still the author, and what we ask of them changed most of all. Their job used to be writing the code. Now it's having read the code, having written or checked the scope and the behaviour list, and being able to answer questions about any line in the diff. If the answer to a review question is "I'll ask the agent", the PR went up too early.
We put that in the template: by opening this PR you confirm you've read every changed line and can explain it. It sounds heavy. It's exactly the bar we always had for human-written code, and it only needs writing down now because the tool made it easy to skip.
What the agent sees
One more change, and it cost nothing. The checks above print failure output, and the agents we use read failure output and adjust, so we wrote the messages for the agent as much as for the human. "Files outside declared scope" lists the files and says to either add them to the Scope section with a reason or revert the changes to them.4
After a month of that, most PRs show up with the scope declared and the behaviour list filled in before a human has seen them, because the agent learned what the repository wants from the checks that told it. The process taught the tool, and the people got to spend their attention on the part of review that needs a person.
Numbers, for what they are worth
From May to August, with the checks in place, median time to first review dropped from 5 hours to about 2, mostly because the scope and behaviour sections make a PR easier to start on. Median PR size went from 310 lines to 160. Reverts within a fortnight of merging went from 6 in the quarter before the change to 1 in the quarter after, and that one was a human PR.
None of the checks are clever. Our old process assumed the author understood their diff, and you can't take that for granted any more, so it has to be something you can verify.
Originally published at zeybek.dev.
-
The one that dropped our default was three lines in a 380 line PR that was mostly a correct feature. ↩
-
Nine times out of ten it's the second. ↩
-
This was controversial, and it's the change the team disagrees on most. ↩
-
"Behaviour changes section is empty but the diff touches a migration" names the migration and asks for one line per observable change. ↩
Top comments (2)
The part that bit me: when the same model writes and reviews, they share the same blind spots. When I let one model check its own roasts in my side project, it was flaky and made up files that weren't in the PR. Splitting the writer and the checker fixed most of it. How do you handle that?
interesting approach