审查意见都标成已解决之后，部分退款仍然没有把货加回库存（中英切换）

Three people, seven computers. In the afternoon, machine 4 is reviewing a refund pull request. The full-refund path is clean: the money goes back, and both units return to stock. The failure is on a partial refund. A customer bought two units of WID-1 and returns one of them for 1000 cents. refund accepts return_lines=[("WID-1", 1)], records the money, sets the status to partial_refund, and then drops the restock list on the else branch. The unit is already back in the warehouse. The system still shows 8 in stock, not 9.

Machine 4 pins a comment on that else branch in shop/refund.py: the partial refund returns the money and does not restock the returned unit. The agent on machine 1 replies “fixed” and marks the conversation resolved. The tests are still 2 passed. Those two tests check that a full refund restocks, and that a partial refund returns the money. They never ask whether stock is 9. If machine 6 only reruns the same tests, it will be green too. This lab did not run a Windows job, so I am not reporting a machine-6 exit code as something I measured. If machine 7 treats “every conversation is resolved, and the tests are green” as a reason to merge, this change lands on main.

This essay asks whether a review thread marked resolved is the same thing as a review thread that was addressed; which pretend fixes a “resolve conversations before merge” rule will allow; and what a merge gate still has to check. It is not the same problem as the earlier piece on who picks up the next change when the session that opened the pull request is already gone. That one is about binding a living pull request to a machine that is still awake. This one is about follow-up that looks finished while stock is still short one unit.

I built a toy shop repository on this machine. The git commits, pytest, the three merge gates, and the fast-forward or refusal after the gate were actually run. Review-thread state lives in local JSON; the GitHub API was not called. GitHub’s “Require conversation resolution before merging” is used only as a semantic counterpart. This machine did not enable that rule on a real repository. The workflow YAML was checked for structure and was not run on GitHub Actions. The numbers from this run come first; the gate comes after.

How the lab was built

The toy repository’s main is 16cdaff. Stock starts at 10. The order reserves 2 units, leaving 8. A full refund of 2000 cents restocks both units, stock returns to 10, and the matching test passes. A partial refund of 1000 cents that also declares one returned unit records the money and never calls restock. Running that case prints stock=8 refunded=1000 status=partial_refund. The correct stock is 9.

The review comment is stored as one blocking thread, T1: the path is shop/refund.py, commented_sha is bound to main at 16cdaff, and follow-up is required to introduce tests/test_refund.py::test_partial_refund_restocks. This is not a live GitHub thread. It is the fixture that drives the gate. In production, fill the same structure from gh api or from Actions, and put “which regression this comment requires” in a field on the thread, or on a follow-up list from machine 4.

The gate has three modes, kept separate on purpose, so it is visible what “resolved” will allow.

resolution only asks whether every blocking thread has resolved=true. That is the local counterpart of GitHub’s conversation-resolution rule. No real ruleset was turned on; every number comes from this local implementation.

file_touched adds a second question, once the thread is resolved: after commented_sha, does the commented file have a diff? A log line is enough.

addressed is the mode I would install. When the resolution is “fixed”, the commented file must have changed, the named regression test must appear in the diff after the comment, and that test must pass on the SHA you would merge. When the resolution is wontfix, only a review role (machine-4-reviewer or machine-5-reviewer in this lab) may say so, with a reason at least eight characters long. The author cannot wontfix their own blocking thread.

merge_if_threads.sh places the target SHA in a temporary worktree, runs pytest there, runs thread_gate.py, and only then allows a fast-forward of main. The tests have to run on the commit that would be merged. If they run in the current worktree — often main — a test that only exists on D is reported as missing. That is a real misstep from this lab, and it was fixed before the numbers below were recorded.

Resolve with no code change, and the resolution gate allows it

A is the control: the thread is still open, and the code is still main. All three gates are REFUSE. Tests are 2 passed, and stock is still 8. This is the case GitHub’s rule is meant to stop: unresolved conversations must not merge.

B marks the same thread resolved. The agent writes “fixed” and does not change a line of code. Tests are still 2 passed. resolution becomes ALLOW. file_touched refuses, because shop/refund.py did not change after 16cdaff. addressed refuses too. So if the merge policy only asks whether conversations are resolved and whether tests are green, B is already allowed onto main, and stock is still 8.

The agent does not have to be malicious. When review is moving at agent speed, “fixed” plus a click on Resolve is a very ordinary way to close a thread. GitHub’s resolved mark records that someone — or some agent — clicked the button. It does not record that the unit was restocked.

After a log line, “the file changed” allows it too

C is closer to the pretend fix that actually gets pushed. The agent adds a log line on the else branch, in commit f0f07d8, marks the thread resolved, and writes “fixed: added a partial-refund log.” Tests are still 2 passed. The partial refund still prints stock 8.

resolution is ALLOW, and file_touched is ALLOW — the file did change. addressed refuses, because test_partial_refund_restocks does not appear in the diff after 16cdaff.

merge_if_threads.sh in addressed mode keeps main at 16cdaff: the tests are green, the gate is still REFUSE. The control is to switch the mode to resolution: the script fast-forwards, main moves to f0f07d8, and the same partial refund still prints stock=8. Switching the mode to file_touched does the same thing: main moves to f0f07d8, stock is still 8. The lab then reset main to 16cdaff so later cases stay independent.

G is C with a different closing ritual. The agent marks the thread outdated, writes “outdated, code moved,” and resolves it. The three gates give the same answers as C. Outdated only means the line number may have moved. It does not mean restock happened. Bulk-resolving outdated conversations will satisfy resolution. It will not satisfy addressed.

A real fix changes the file and brings a regression that can fail

D does the two things that were required. The else branch calls restock for return_lines. The test file gains test_partial_refund_restocks, which asserts that stock is 9. The commit is e30e480. pytest is 3 passed. The same partial refund prints stock=9 refunded=1000 status=partial_refund. All three gates are ALLOW. merge_if_threads.sh in addressed mode fast-forwards main from 16cdaff to e30e480. After the merge, the same case still reports stock 9. main was then reset to 16cdaff.

H is the reverse control. The agent only adds a function of the required name in the test file. That function asserts that 1000 cents were refunded, and it does not touch shop/refund.py. Tests become 3 passed; stock is still 8. resolution allows, file_touched refuses, and addressed refuses. It is not enough for the required test name to appear. The production file must also have changed after the comment. H is stopped not because the gate understood the assertion, but because the refund file did not move.

An author wontfix does not count; a reviewer’s written scope can

E and F stop pretending the code was fixed. In E, the author machine-1-agent marks T1 as wontfix, with the reason that partial restock can wait for a later iteration. resolution allows, and file_touched allows — wontfix does not require a file change. addressed refuses, because the author cannot close their own blocking thread this way.

In F, the same scope decision is written by machine-4-reviewer: this pull request is only about refund copy, and restock will be a separate ticket. All three gates are ALLOW. Stock is still 8, but that is machine 4 taking the scope, not an agent writing “fixed” on the thread. If restock still needs to happen, it belongs on another ticket. It should not be disguised as this pull request having addressed the comment.

The gate does not read assertions: a log line plus a weak test still lands

I stacks C and H: the same kind of log line as f0f07d8, plus a test still named test_partial_refund_restocks that only checks the refund amount, as in 3a44515. The commit is d2b4221. pytest is 3 passed. The file changed, the required test name appears in the post-comment diff, and the test passes. addressed returns ALLOW, and merge_if_threads.sh fast-forwards main to d2b4221. The same partial refund still prints stock 8. main was reset again.

This remainder has to be stated clearly. The gate checks that the file moved, and that a named test was introduced and is green. It does not check whether the assertion actually asks about stock. A weak assertion under the required name still reaches main. Machine 4 still has to open that test and see whether it asserts 9, or only asserts the money. The gate will catch B, C, G, H, and an author’s own wontfix. It will not catch a follow-up that is willing to touch both the production file and the test file, and then weaken the test. That is a weakened test, and it needs a review of the test itself. This gate should not be asked to read the assertion.

The mechanism: resolved records a click, not a behavior

A conversation marked resolved answers “this thread is folded up in the UI.” The repository needs to ask something else: did the behavior the comment named actually change, and will it go red if someone changes it back?

Those two questions get bound to the same button because a human reviewer usually reads the diff first, and only then decides to resolve the thread. An agent reverses the order. It resolves first, and then keeps the tests green. If the existing tests never covered the comment, the green check does not object. GitHub treats comments from accounts such as Copilot as conversations that can be resolved; the rule itself does not care whether a person or an agent clicked the button. This lab did not attach those accounts, so I am not reporting live product behavior as something I measured. B and C on this machine are already enough: the button and the restock are not the same evidence.

“The file changed” looks stricter, and is thin. A log line, a comment, or a rename can put shop/refund.py in the diff. That is how C and G pass. To pin the review comment down, you also need a regression that appears only after the comment, and that regression has to run on the SHA you would merge. The gate uses a temporary worktree at that SHA so the current worktree’s test results cannot stand in.

wontfix has to exist, or every style comment will stall the merge. The opening cannot belong to the author. The difference between E and F is not how long the reason is. It is who takes responsibility for “we will not change this now.”

When you should not install this gate

If the comments are wording, formatting, or naming, requiring a regression for every thread will stall machine 4 and the merge queue together. Those comments should not be blocking, and they should not carry requires_test. The gate is for comments that change money, stock, permissions, or where data goes.

If the team’s rule is that unresolved conversations cannot merge, and machines 4 and 5 always resolve the threads themselves after they have opened the follow-up diff and the new test, then the local addressed mode is mostly a backup for the case where an agent clicks Resolve first. On a three-person, seven-machine team shipping at agent speed, that premise usually fails.

If the repository has no convention for “which test this blocking comment requires,” the gate will refuse every “fixed” claim because requires_test is empty. That is intentional. Have machine 4 put the test node on the comment, or on a follow-up list, before you turn the gate on. Do not let the agent invent a test name.

This gate also does not replace “Require conversation resolution.” A pull request like A, where nobody has resolved the thread, should still be blocked by that rule. Use both: first require the conversation to be handled, then require a “fixed” claim to be real.

It does not replace a person reading a weak test either. I already measured that remainder.

What to copy tomorrow

Turn on, or keep, GitHub’s “require conversation resolution before merging.” It will stop A. It will not stop B or C.

Put a requires_test on blocking comments that change behavior, down to the function name. In AGENTS.md, say that agents must not resolve a blocking thread unless that test is in the diff after the comment and is passing. Leave wontfix to machines 4 and 5.

Make thread_gate.py --mode addressed a required check. Run the tests in a worktree of the target SHA; do not use the current worktree’s results. merge_if_threads.sh should fast-forward only after both the tests and the gate pass.

In production, fill the JSON from GitHub review threads, and do not treat the workflow robot as resolved_by. The required check name must match the name your ruleset expects. The workflow in this lab was inspected for structure; it was not run on Actions.

The scripts, the workflow, and the AGENTS.md snippet are in review-thread-kit.tar.gz. Call the gate like this:

python3 tools/thread_gate.py --repo . --head HEAD --threads threads.json --mode addressed

The smallest step today: open a pull request you already merged whose review conversations are all resolved, and see whether any comment was closed with only a log line, a remark, or a same-named test that does not assert the original problem. If you find one, turn that regression into an assertion that would have failed, and then decide whether the pull request should still be on main.

What was actually run

Run: git 2.47.3, Python 3.13.5, and pytest 9.1.1 on this machine; the commits and branches in the toy repository; pytest on main and on each case; the three gate modes; merge_if_threads.sh refusing an addressed merge of C, and fast-forwarding C under resolution and file_touched to f0f07d8 (stock 8, then reset); fast-forwarding D to e30e480 (stock 9, then reset); fast-forwarding I to d2b4221 (stock 8, then reset); the author-wontfix and reviewer-wontfix decisions.

Not run: the GitHub API; a real branch protection rule or ruleset; a merge queue; a second GitHub account to click merge; GitHub Actions (the YAML was checked for structure, and PyYAML was not installed on this machine); any Windows job. The short SHAs come from this run. They will change if the repository is rebuilt; the field relationships and the decisions will not.