代理交来一个改了上千行的 PR，审查机上的人该读哪几行？（中英切换）

Thursday morning, the agent on machine 2 opens a PR for "prorate by day when a customer changes plan mid-cycle". On the way it does three bits of tidying: it splits the 264-line billing/invoice.py into five files under billing/invoice/, runs a formatter over money.py, and regenerates the lockfile. The description is neat. All 11 tests pass.

Machine 4 gets it for review. GitHub shows 12 files and 374 changed lines. Git pairs invoice.py => invoice/tax.py as a rename with 110 red lines under it; four more files are solid green. The first job for the reviewer (human, or the review agent on machine 4) is to match 150 deleted lines against 150 added lines in their head and convince themselves it is "just a move". By the third file most people are skimming.

The PR hides two behaviour changes, three lines in total:

-    return user.is_admin or invoice.owner_id == user.id
+    return user.is_admin or invoice.owner_id is not None

-    return invoice.subtotal() - d + tax_amount(invoice, d)
+    return invoice.subtotal() - d + tax_amount(invoice)

The first lets any logged-in user refund someone else's paid invoice. The second taxes the pre-discount amount. Both sit inside "moved" code, surrounded by dozens of lines that really were moved unchanged. Why are the tests green? Because the same PR deletes test_stranger_cannot_refund, changes the expected value in test_total from 110.55 to 111.98, and regenerates the golden file. The golden file changes one line, TOTAL 110.55 to TOTAL 111.98, while the three lines above it (subtotal 109.98, discount -11.00, tax 11.57) stay the same and still add up to 110.55. The invoice no longer adds up, and that line is buried in 374.

This is a toy repo I built on my box, and I planted both bugs. But "refactor + feature + test edits in one PR" is the shape agents hand over most often: they fix whatever annoys them on the way, then edit tests until they pass. A team of three people on seven machines, with a dozen of these a day, cannot line-diff every one on the review machines. The real question is how to let them read only the few dozen lines that matter, and be sure the other few hundred really do not need reading.

What public repos look like

I wanted to know how much review large PRs get on real teams. Using GitHub search, I pulled PRs opened by the Copilot coding agent in the dotnet, microsoft and Azure orgs and merged between 2026-07-01 and 10-27. I took the first 400 search results per org and kept repos with at least 100 stars: 906 PRs across 146 repos. About nine in ten had some human review activity before merge, so this is a sample where someone was reviewing.

Grouped by changed lines, counting inline review threads by humans (not bots, not conversation comments):

As PRs grow from tens of lines to thousands, comments per PR go from 0.4 to about 2 and stop. Per line, attention drops by more than a hundredfold. That does not prove nobody reads big PRs: people read offline, discuss in the conversation tab, or trust CI. It does show one concrete thing: reviewer effort on big PRs does not grow with the code.

But how many of those 1,000 lines need a human at all? Lockfiles, generated SDK code, moves and reformatting are a waste of human reading. If most of a big PR is that, few comments may be fine. So the next step was to take the diffs apart.

Splitting the diff into two piles

I wrote review_map.py, stdlib only. It reads git diff output and puts every changed line in a bucket:

• Machine-provable: generated files (lockfiles, dist/, */generated/*, *.pb.go and so on), code moved unchanged, whitespace-only reformatting, docs, blank lines, import lines, added test code (skim the test names against the claim; don't line-read).
• Needs a human: ordinary source and config changes, plus three kinds to read first: lines edited while being moved (EDIT-INSIDE-MOVE), deleted or changed assertions (TEST-ASSERT-REMOVED), and changed snapshot/golden files.

"Lines needing a human" is the size of the second pile. The script writes a Markdown review map: the read-first lines, then logic lines per file.

How it decides "moved unchanged": strip leading, trailing and repeated whitespace from removed and added lines, fingerprint every run of 4 consecutive lines, and if a fingerprint occurs exactly once on each side, those lines were moved untouched. "Edited inside a move" means: a changed line where at least 60% of the same-side changed lines within 6 lines either way are moved lines, the line itself is not blank or a lone bracket, and its text does not appear anywhere on the other side. That last condition matters; more below.

On the toy repo, 374 lines become 33 that need a human. The read-first list is 8 lines:

EDIT-INSIDE-MOVE    billing/invoice/discounts.py:30  +return invoice.subtotal() - d + tax_amount(invoice)
EDIT-INSIDE-MOVE    billing/invoice/refunds.py:13    +return user.is_admin or invoice.owner_id is not None
EDIT-INSIDE-MOVE    billing/invoice/tax.py:256       -return user.is_admin or invoice.owner_id == user.id
TEST-ASSERT-REMOVED tests/test_invoice.py:13         -assert total(make()) == Decimal("110.55")
TEST-ASSERT-REMOVED tests/test_invoice.py:23         -assert not can_refund(User(2, "stranger"), make())
snapshot-changed    tests/golden/invoice_basic.txt:10 -TOTAL 110.55
snapshot-changed    tests/golden/invoice_basic.txt:10 +TOTAL 111.98
test-removed        tests/test_invoice.py:22         -def test_stranger_cannot_refund():

Both behaviour changes are in the first three lines, followed by the deleted test and the changed expectation. The other 25 lines are the proration feature itself (24 lines in proration.py) and one deleted old line. 152 moved lines, 44 lockfile lines, 16 reformatted lines and 21 import lines need no reading, but that does not mean nobody checks them. The next section covers who does.

Then I replayed the 96 public PRs with 500+ lines (2 more had diffs over 300 files, which GitHub will not serve, and 2 docs repos were dropped). Median changed lines: 1,116. Median lines needing a human: 324. 27 fell to 100 or fewer, mostly lockfiles, generated code and moves. 41 were still over 400 after removing those; they really were too big. Of the 20 PRs needing a human on 1,500+ lines, 18 merged with zero human inline comments.

So the public data mixes two cases. Some big PRs only look big and become readable with a map. Others really are big, and no map makes them readable; they have to be split. A limit on raw line count blocks both, so agents learn to split lockfile bumps into three PRs while genuinely large logic changes get in some other way.

Two PRs: one for the machines, one for people

Mapped onto three people and seven machines:

1. When an agent on machines 1–3 gets a task that needs both a refactor and a behaviour change, it opens two PRs. PR A carries the mechanical label and contains only moves, renames, formatting and regenerated files. PR B is based on PR A's branch and contains the feature and its tests.
2. On PR A, CI runs review_map.py --assert-mechanical: if any line needs a human, exit code 3 and the check fails. Then the project's own formatter check, lockfile check and full test suite. All green, and machine 7 merges it without putting it in the queue for machines 4–5.
3. On PR B, CI runs review_map.py --max-eyes 400 and posts the map as a PR comment (found again via a hidden marker and edited in place, so no spam). Over 400 lines needing a human, the check fails and the comment says how to split.
4. People on machines 4–5 read only the mapped lines. For a big change that genuinely cannot be split, someone on the list adds the split-waived label.

I ran all four branches in the toy repo:

Rows two and three side by side tell the story. The same refund change in a pure-move PR is caught by the tests, because test_stranger_cannot_refund is still there. Mixed into a feature PR, the agent has a reason to "update the tests too", and the tests stop catching it. --assert-mechanical does not depend on the tests: any line in a move PR that was not moved verbatim fails it, whatever the tests say.

The agent applies the mechanical label itself, and that is fine. The label is a claim, and CI checks it; if the agent lies, the check goes red. The waiver label is different. It means "a human looked and owns this", so the workflow checks who added it: it takes the actor of the last split-waived labelled event from the issue events API and ignores it unless that login is in HUMANS. The agents' GitHub accounts must not be in that list.

The rule also has to be in AGENTS.md, or the agent's first reaction to being blocked is to edit the tests or the threshold. The snippet in the kit is short: refactors and behaviour changes go in separate PRs; no logic edit while moving code, not even an "obvious fix"; deleting an assertion or regenerating a snapshot in the same PR requires a line in the description like "test X expected 110.55, now 111.98, because..."; never add split-waived yourself.

Where I got it wrong

The first version fingerprinted 3-line runs and counted a line as moved if the run appeared anywhere on the removed side. On the 96 public PRs it flagged 2,289 "edited inside a move" lines in 43 PRs. I looked at a few: builder.Append(c);, /// </param>, a lone }. These lines occur dozens of times in large files; repeated boilerplate around them matched somewhere else, so they looked edited. The other direction was worse: for the same reason 35,469 lines were counted as "moved unchanged" and taken out of human view. Generated TypeScript models, JSON baselines and workflow lock files are full of boilerplate that matches everywhere.

The second version used 4-line runs that must be unique on each side. "Moved unchanged" fell to 19,853 lines and the median lines needing a human rose from 257 to 324. Better to make people read more than to hide code that was not actually moved. Versions three and four require an edited-inside-move line to not be blank or a bracket, and its text to be absent from the other side. What remained: 141 lines in 21 PRs. I picked 10 at random and read them in context; all 10 were real edits: a new struct field, a deleted guard condition, a UI component gaining verticalAlign, and in wassette's sandbox code DirPerms::all(), FilePerms::all() becoming FsPerms::ReadWrite during a move. I am not judging whether those edits were right, only that a reviewer should see them first.

Where it stops working

It does not read semantics. Comparing with whitespace removed means re-indentation counts as unchanged. I tried it: delete an if user.is_admin: and dedent the four lines under it. The script counts the four lines as moved unchanged and flags only the removed if as edited inside a move, first in the list. That worked out, because the deleted line is itself the evidence. But in Python or YAML, where indentation is meaning, "moved verbatim into another function" can still change behaviour; same text is not same meaning. So the full test suite on the mechanical PR is not optional.

It trusts paths. Anything named dist/ or */generated/* is classed as generated. An agent that puts logic there hides it. So everything counted as generated must be regenerated in CI and checked with git diff --exit-code. The workflow in the kit has a placeholder step for this; the real commands (ruff format --check, uv lock --check, npm ci, your code generator) are yours to fill in.

Some repos do not fit. In a repo like the Azure SDKs where most code comes from a generator, review the generator input (TypeSpec, OpenAPI), not the output. In CSS or SQL migrations, order is meaning and "moved verbatim" means nothing. In a docs repo the docs are the product and cannot be skipped.

400 is my guess. The often-cited SmartBear study of code review at Cisco says reviews of up to 200–400 lines at a time work best. I did not verify that figure; I treat it as a starting point. Run report-only for a week, see where the PRs your team actually reviews well land, then pick a number.

The strongest objection: if review can be handed to agents, who never tire, why split at all? I did not test AI reviewers this time; Claude Code on my box is not logged in and I am not going to work around that. Two things I can say: the map helps a review agent too, because it decides what gets looked at first; and a human presses the merge button, and GitHub records a human as the approver. Handing someone a PR they cannot read and asking for a signature produces a record that says "reviewed" when nobody did.

There is a cost: two PRs mean one more merge and one base change. When GitHub deletes a merged branch automatically, it retargets PRs based on it to main, so PR B needs no manual rebase of its base. If your repo has auto-delete turned off, do that step yourself.

What I did not do. The workflow passes actionlint and shellcheck, and the jq that reads "who added the label" was tried against the events API of a public repo, but the workflow has never run in real GitHub Actions; I did not set up a sandbox repo to run it for this post. The lockfile in the toy repo is fake, only there to demonstrate the bucketing. The public data counts inline threads only, and the sample is the first 400 search results, not a random draw.

Take-aways

1. Size limits count lines a human must read, not lockfiles, generated code or verbatim moves.
2. Refactors and behaviour changes go in separate PRs. CI proves the refactor PR is "just a move"; a person does not.
3. Deleted assertions, changed expectations and regenerated snapshots in the same PR are the first things to read, not the last.
4. Post the review map on the PR and edit it in place; the size waiver counts only when a listed human adds it.
5. Anything exempted by path must be regenerated by CI to prove it.

The script, workflow and AGENTS.md snippet: review-map-kit.tar.gz. Change nothing today. Run scripts/review-last-prs.sh 20 in your repo, count how many of the last 20 merged PRs needed a human on more than 400 lines, and check how many comments they got at the time.