代理说“已修复”，线程也关了：怎么核实每条审查意见真的改了？（中英切换）

Same three-person team, same seven computers: machines 1–3 develop, 4 and 5 review, 6 validates on Windows, 7 merges.

Tuesday morning the agent on machine 2 opened PR #41: partial refunds plus percentage coupons in the cart. The review agent on machine 4 left nine inline comments. Refunds have no cap, so two partial refunds can add up to more than was paid. Amounts go through float and get truncated after multiplying by 100, so a 0.29 refund becomes 28 cents. Coupons use Python's round(), which is banker's rounding; finance wants half-up, and the cart preview must show the same number as the invoice. Any logged-in user can refund anyone's order. The log line prints the full card number. A variable is called amt2. A zero refund creates an empty record. Two concurrent refunds can both pass the cap. Every refund needs an audit event.

Forty minutes later the agent pushed one commit and replied under each comment: "Fixed, test added." "Switched to precise rounding." "Done." Then it resolved all nine threads. Branch protection had "require conversation resolution before merging" on; that turned green. CI was green, 8 tests passing. Machine 5 approved, machine 7 merged.

I went through the final code one comment at a time. Four of the nine were not fixed. A 0.29 refund still refunds 28 cents. The invoice now rounds half-up, but the copy of the same code in the cart preview was untouched: 2005 cents at 10% off shows 1804 in the preview and charges 1805. The agent created an assert_owner() helper and unit-tested it, but the refund endpoint never calls it, so mallory can still refund alice's order. For the audit event, the only change is a comment: # audit event is recorded by the caller. The caller records nothing.

My claim: on GitHub, "Resolved" records who clicked the button, not what changed in the code. Review has two halves: writing comments, and checking that each one was really dealt with. Most teams staff the first half and leave the second to the author's good faith. When the author was a person making a 20-line follow-up, a reviewer could skim the delta. When the author is an agent pushing dozens of commits a day, replying politely to every comment and closing the threads as it goes, a review machine that rereads the whole diff every round falls behind fast. Below are the verification methods I tried in a toy repo, which ones worked, which failed in their first version, and how to wire the result into the seven machines.

First, who clicks "Resolved" in a real repo

To make sure this isn't a problem I invented, I used the GitHub GraphQL API (read-only) to pull the last 60 merged PRs of streamlit/streamlit on 2026-10-25. The repo uses AI review. 24 of those PRs had inline review threads, 174 threads in total, and 158 were opened by AI reviewers (a review workflow running as github-actions, Greptile, Cursor and others). At merge time 145 threads were resolved, and 125 of those were resolved by the PR author. In 83 resolved threads a reply contains a commit hash, along the lines of "Added in 29887f9170: …". In 54 resolved threads, the commented lines were unchanged at merge. Another 29 threads were still unresolved when the PR merged.

None of that says how many comments went unfixed. Unchanged lines may mean the author declined with a reason, or fixed it in another file. My point is different: on GitHub, not one of those 145 "resolved" flags tells you which case you're looking at. "Added in 29887f9170" is a claim, and the platform does not check it. Streamlit's own PR-fixing agent rules say to auto-resolve only threads opened by bots and leave human comments for the reviewer to resolve, which shows they see the same issue: who gets to click Resolve matters.

The toy repo and nine comments

The lab repo is billing, in Python, with three commits: main, PR #41's commit R (what the reviewer saw), and the agent's follow-up F. The nine threads and their true outcomes are in review-41.json.

One disclosure: there is no logged-in coding agent on my box, so I wrote F by hand. I copied follow-up patterns I've seen from agents: fix only the commented line and ignore copies of the same pattern elsewhere (C3); create a helper, unit-test it, never wire it in (C4); replace the implementation with a comment (C9); add a test that would have passed before the fix too (C2). Some fixes are real: the refund cap (C1), zero refunds (C7), the rename (C6), and card masking, where the agent left the commented log line alone and added a logging filter in another file (C5), a reasonable fix. It declined C8, saying refunds for one order are already serialized by the queue's partition key, so no lock is needed. Whether that holds is the reviewer's call.

So the answer key: C1, C5, C6, C7 really fixed; C2, C3, C4, C9 not fixed; C8 needs a reviewer decision.

Experiment 1: "resolved" and "this line changed"

The first method is the status quo: every thread is resolved, so everything passes. All four unfixed comments get through, and C8, a decline that needed the reviewer, was closed by the agent itself.

The second is an intuition many reviewers really use: GitHub marks a thread Outdated once the commented line changes, and Outdated gets read as "fixed." I wrote line_touch.py to model that: if the commented line is modified or deleted between R and F, it counts as changed. The result is bad. C2, C3 and C9 all count as changed. C2's line did change, from float(amount) to round(float(amount), 2), with the same behavior. C3's invoice line changed and the preview didn't. C9's line changed because a comment was added. In the other direction, C1 and C5 were fixed correctly but show as untouched: C1's cap check was inserted above the commented line, and C5 was fixed in another file. Of the four unfixed comments it caught only C4, and it flagged two of the four real fixes.

So "which line changed" and "was the comment dealt with" are unrelated things. A comment points at a problem. The fix doesn't have to sit on that line, and a changed line doesn't mean the problem is gone.

Experiment 2: make the agent submit evidence per comment

If line numbers can't be trusted, ask for evidence. The agent keeps a ledger in the branch, .review/41.json, with one entry per thread. fixed must cite a test; declined and style give a reason and wait for the reviewer. The check borrows the old rule for bug fixes: the cited test must fail on R, the commit the reviewer looked at, and pass on the current commit. To do that I check R out into a temporary worktree, copy the branch's whole tests/ directory over it, and run the test. Then I run it again on the current commit.

That rule caught two right away. C2's test, refund("10.50") == 1050, also passes on R, because 10.5 happens to be exact in floating point. C9 cites a test that already existed and passes before and after. A test that passes before the fix proves no fix.

But the first version let C4 through. The agent's test file starts with from billing.auth import assert_owner. That module doesn't exist on R, so collection fails with ImportError. That counts as "fails before." It passes now. Rule satisfied, and the refund endpoint still has no permission check. The problem is that "fails" was too loose: on R the test never got to run any old code, it just failed to import. Version 2 tightens it: the result before the fix must be an assertion failure inside the test function (including a pytest.raises that never saw its exception). Collection errors, ImportError, AttributeError and NameError don't count. Now C4 is blocked, and the only way through is a test that goes through the refund endpoint, which can only flip from failing to passing if the endpoint actually calls the check.

The tightening has a cost: a legitimate fix that really needs a new interface also gets its test rejected. My answer is to have the agent test through the code path the reviewer pointed at, and test the new interface separately. In this repo that requirement is exactly right, because C4's bug is that the new interface was never wired in.

Version 2 still let C3 through. The agent's test, invoice_total([2005], 10) == 1805, fails with an assertion on R and passes now, fully compliant. And it ignores the reviewer's "the cart preview must show the same number." That is the ceiling for this kind of check: the agent picks the test, so the evidence only covers the part of the problem the agent understood. If the agent misunderstood, its evidence goes wrong with it.

Experiment 3: the reviewer writes the comment as a failing test

To get past that ceiling, the evidence has to come from whoever raised the comment. Before commenting, machine 4 pushes tests/review/test_pr41_review.py to the PR branch, one test per behavioral comment, and puts a line test: <test name> in the comment body. The key is to write the test as the rule, not as the line. C3's test loops over 1 to 2999 cents and five discounts and asserts that both invoice_total and preview_total equal the half-up result. C4's test calls create_refund("mallory", ...) through the endpoint and asserts it is rejected and no refund is recorded on the order. C9's test asserts that after calling the endpoint, audit.EVENTS holds exactly one refund event. Seven behavioral comments as tests: 55 lines.

I replayed the agent's same follow-up commit unchanged on top of that. Result: C2, C3, C4 and C9 all fail, C1, C5 and C7 pass, C6 and C8 wait for the reviewer, and none of the four real fixes is flagged. The author must not be able to edit the reviewer's tests, and that also has to be checkable. In a variant, the agent deletes the preview assertion from C3's test with the commit message "fix flaky review test." The verifier sees that a file under tests/review/ changed after the reviewer's commit and fails it.

My first version of that was too coarse. It judges "edited" per file, so all seven items in that file failed, including C1, C5 and C7, which had passed. As a gate that's fine, since nobody should touch the reviewer's file. But the report gets noisy, and the agent's to-do list picks up three items that make no sense to it. A finer approach is one test file per thread, or comparing the AST of each test function. I didn't build that; I'm noting it here.

Then comes the second half of the loop. The verifier writes its failures to next-prompt.md: the original comment and the verifier's reason for each failed thread, plus an explicit line saying replies and Resolve don't count. That file is the next input for machine 2's agent. In round two it moved amount parsing and discount rounding into one money.py shared by the invoice and the preview, made the refund endpoint call assert_owner, and recorded an audit event on refund: 5 files, 24 lines added. The verifier runs again: all seven behavioral comments pass, exit code 2, meaning only C6 and C8 are waiting for the reviewer. Machine 4 replies /accept on those two threads and the exit code becomes 0. That round, the review machine looked at two items instead of rereading the whole diff.

Wiring it into seven machines

The kit has three scripts and a workflow. review_fetch.py uses GraphQL to pull every review thread on a PR, each with the commit the comment was written against (originalCommit). That is the "before" commit, so nobody has to fill it in. verify_ledger.py runs the three checks above. ledger_report.py writes the PR comment and the next prompt for the dev machine; with --apply it reopens threads that are resolved but failed verification and replies with the reason. I ran review_fetch.py read-only against a merged PR in shuizhengqi1/pages and against public streamlit PRs, and used GraphQL introspection to confirm the arguments of unresolveReviewThread and addPullRequestReviewThreadReply. In this post, reopening threads was only a dry run; it was not executed against real threads.

.github/workflows/review-ledger.yml

name: review-ledger
# Required check: every review thread must be verified, not just resolved.
on:
  pull_request:
    types: [opened, synchronize, reopened]
  pull_request_review:
    types: [submitted]
  pull_request_review_comment:
    types: [created]   # a reviewer replying "/accept" re-runs the gate

permissions:
  contents: read
  pull-requests: write

concurrency:
  group: review-ledger-${{ github.event.pull_request.number }}
  cancel-in-progress: true

jobs:
  ledger:
    runs-on: ubuntu-latest
    timeout-minutes: 15
    env:
      PR: ${{ github.event.pull_request.number }}
      GH_TOKEN: ${{ github.token }}
      # GitHub logins allowed to accept "style" / "declined" items (reply "/accept" in the thread)
      REVIEWERS: ${{ vars.REVIEW_LEDGER_REVIEWERS }}
    steps:
      - uses: actions/checkout@v4
        with:
          ref: ${{ github.event.pull_request.head.sha }}
          fetch-depth: 0
      - uses: actions/setup-python@v5
        with:
          python-version: "3.12"
      - run: pip install -r requirements-dev.txt
      - name: Fetch review threads
        run: python3 tools/review_fetch.py "$GITHUB_REPOSITORY" "$PR" > threads.json
      - name: Verify ledger
        id: verify
        run: |
          set +e
          python3 tools/verify_ledger.py --threads threads.json --ledger ".review/$PR.json" \
            --head HEAD --reviewers "$REVIEWERS" --json verdicts.json | tee verdicts.txt
          echo "code=${PIPESTATUS[0]}" >> "$GITHUB_OUTPUT"
      - name: Report, re-open unverified threads, label for the dev machine
        if: always() && hashFiles('verdicts.json') != ''
        run: |
          python3 tools/ledger_report.py threads.json verdicts.json \
            --comment ledger-comment.md --prompt next-prompt.md --apply
          gh pr comment "$PR" --body-file ledger-comment.md --edit-last --create-if-none
          if [ "${{ steps.verify.outputs.code }}" = "1" ]; then
            gh pr edit "$PR" --add-label review:unverified
          else
            gh pr edit "$PR" --remove-label review:unverified || true
          fi
      - uses: actions/upload-artifact@v4
        if: always()
        with:
          name: review-ledger-${{ github.event.pull_request.number }}
          path: |
            threads.json
            verdicts.json
            next-prompt.md
      - name: Gate
        run: |
          case "${{ steps.verify.outputs.code }}" in
            0) echo "all review threads verified" ;;
            2) echo "::error::waiting for the reviewer on style/declined items"; exit 1 ;;
            *) echo "::error::review threads not verified, see the ledger comment"; exit 1 ;;
          esac

The first version of the workflow was wrong. I wanted to re-verify whenever a thread was resolved or unresolved, so I used pull_request_review_thread. actionlint rejected it: that's a webhook event, not an Actions trigger. So the reviewer's decision became a /accept reply in the thread. Replies are pull_request_review_comment events, and those can trigger a re-run. That turned out better: anyone can click Resolve, but /accept at least leaves a signed reply. Also, replies posted by the workflow with GITHUB_TOKEN don't trigger workflows, so "reopen the thread and reply" can't turn into a loop.

/accept only counts from logins listed in REVIEW_LEDGER_REVIEWERS. Many small teams will hit a trap here: if the agents on all seven machines use one GitHub account, resolvedBy and every reply author are the same person, and the verifier can't tell whether machine 4 accepted or machine 2's agent accepted its own decline. For this rule to mean anything, the review machines need a different identity from the dev machines, either a separate machine account or a GitHub App. If you can't do that, keep /accept for humans and forbid agents from posting it.

Rules for the agents go into AGENTS.md:

AGENTS-snippet.md

## Review threads (dev machines 1–3)

- Never resolve a review thread yourself. Resolving is the reviewer's decision; the `review-ledger` check ignores it anyway.
- For every review thread on your PR, add an entry to `.review/<pr>.json` keyed by the thread id (`PRRT_...`, see `threads.json` from `tools/review_fetch.py`):
  - `{"status": "fixed", "evidence": "tests/...::test_name"}`: the test must FAIL on the commit the reviewer commented on, with an assertion, and PASS now. A test that imports a helper you just added does not count; go through the code path the reviewer pointed at.
  - `{"status": "declined", "note": "..."}`: say why in one or two sentences. The reviewer decides by replying `/accept`.
  - `{"status": "style", "note": "..."}`: for naming/formatting. Also needs the reviewer's `/accept`.
- Never edit files under `tests/review/`. Those are the reviewer's tests; make them pass by changing code.
- If the reviewer comment covers a pattern (rounding, permission checks, logging), fix every place the pattern occurs, not only the line with the comment.
- When `review:unverified` is on your PR, read `next-prompt.md` from the latest `review-ledger` run and work only on the items listed there.

## Review threads (review machines 4–5)

- For a behaviour comment, push a failing test under `tests/review/test_pr<N>_review.py` first, then comment and put `test: <nodeid>` on its own line in the comment body.
- Phrase the test as the rule, not the line: if the comment is about rounding, assert every function that rounds, not only the one in the diff.
- Comments you cannot turn into a test (concurrency, design, naming) stay normal comments; you will be asked to `/accept` or reject the dev agent's answer.

Costs, and when not to do this

The biggest cost lands on the reviewer. Writing "the preview must match too" takes ten seconds. Writing a test that loops over five discounts and asserts two functions takes minutes. A review agent can write it for you, but a person still needs to check that it's written as the rule. In my experience not every comment is worth this. The ones that are worth it are the ones where a wrong fix causes an incident: money, permissions, data, public interfaces. Naming and formatting go through style plus /accept. Concurrency and design calls like C8 can't become a stable test anyway; their value is in being forced in front of the reviewer instead of being closed by the agent.

Second, tests freeze design. C9's test requires exactly {"kind": "refund", "order": "o1", "cents": 500}. If the author has a better event shape, they have to go back to the reviewer first. Sometimes that friction is exactly what you want, sometimes it just slows things down. Review tests should assert what must be true, not how the reviewer would have written it.

Third, fail-before/pass-after runs tests twice. Each verification on the toy repo takes under 5 seconds. For a suite that runs ten minutes, run only the tests the ledger cites, not the whole suite. Tests that depend on a browser or external services are flaky to begin with, and a failure before the fix may be luck, so those comments are better verified by hand.

Fourth, this workflow runs PR code with a token that can write. That's acceptable on a private repo where every branch belongs to your team. A public repo that takes fork PRs must not copy it as is. Split running tests and posting comments into two workflows and pass results through workflow_run.

Finally, if your PRs rarely get more than one or two comments and the author is the colleague sitting next to you, this whole thing is overkill. It targets one specific combination: authors are agents that close their own threads, and there are too many PRs for reviewers to reread each round.

The strongest objection

The objection goes like this. Review is a conversation, not a contract. Turn comments into mandatory tests and reviewers will write fewer comments because of the cost, leaving only the easy-to-test ones. Agents will learn to game the tests too: Goodhart's law. The natural fix is a second review agent that reads only the delta and judges whether each comment was handled, without changing how anyone works.

Half of that is right. The number of comments does drop, and you feel it in the first week. My view is that what drops is mostly the comments nobody was checking anyway. Fewer behavioral comments that each have teeth beat nine comments where four quietly go nowhere. As for gaming: the review tests are written by whoever raised the comment and the author can't edit them, so there's far less room than when the agent picks its own evidence. What's left is a review test that isn't thorough enough, and that's a review-quality problem, not a flaw in the mechanism.

I agree with adding a delta-only second review, but it doesn't replace this step. I didn't run a real review agent on the box, so this part is structural reasoning, not measured. C3's delta contains only the invoice file; the preview file isn't in the diff, so a second reviewer never sees it. C4's delta contains a new function and a passing unit test, which looks more complete than before. A delta reviewer reads what the agent chose to show, and shares the agent's blind spot. It belongs after the verifier, for the comments that can't become tests.

What you can copy tomorrow

1. Run review_fetch.py once on your recently merged PRs and count how many resolved threads were resolved by the PR author and how many commented lines were unchanged at merge. That number tells you how big the problem is.
2. In the dev machines' AGENTS.md, say: never resolve threads yourself; write one entry per thread in .review/<pr>.json; fixed must cite a test that fails with an assertion before the fix and passes after.
3. Have review machines push a failing test under tests/review/ before commenting on money, permissions or data, written as the rule and covering every copy of the pattern.
4. Make review-ledger a required check. Treat the branch-protection "conversations resolved" setting as a reminder, not as evidence.
5. Give review machines and dev machines different GitHub identities, or neither /accept nor Resolve says who agreed.

My conclusion: the agent isn't deliberately lying to you. It treats "replied" as "handled," and the platform lets it. Verification can't be left to the author's good faith, and it can't depend on reviewers rereading everything each round. It has to be a check that judges each thread on its own: the reviewer's test passes, or the agent shows a test that fails before and passes after, or the reviewer accepts in their own words. Today you can unpack the kit below, run lab/run_all.sh to see the nine threads under all five methods, then run review_fetch.py against your last ten merged PRs.

Full scripts, workflow, toy-repo builders, every raw output, and the streamlit sample: review-loop-kit.tar.gz.