Repository navigation
fix(#203): the queue stalls because drift is treated as a lifecycle exit - #214
Merged
Merged
Conversation
The report was "prereview stops making progress once there are 5+ comments; the
Done count never reaches the total". There is no cap, no batch size and no `5`
anywhere: five is where the agent has edited enough of the document that its own
edits start invalidating the anchors of the comments it has not reached yet.
Four defects all key off AnchorOutdated(), and each turns that drift into lost
progress. One rule fixes them: the queue drops an item only on an explicit human
resolve. Never on drift.
- QueueState() tested AnchorOutdated() BEFORE Processed, so a comment the agent
had already marked done left the Done count the moment the agent's own edit
broke its anchor. suggestionQueueState has always ordered it the other way and
says why ("checked FIRST, since an applied suggestion is also anchor-outdated");
the comment path had it backwards. Done now beats drift.
- An enqueued, un-done comment whose anchor is gone returned "" — it left BOTH
the queued and the done count. The panel renders no total, so a row leaving
looked exactly like a row being finished. It is now `blocked`: badged, counted,
named in the legend, and included in HasQueue() so the panel body cannot
collapse and hide it.
- `done --all-open` sourced its ids from the AGENT'S actionable set, which drops
outdated and agent-replied comments — precisely the ones it has just finished.
It skipped them, printed "marked N comment(s) as worked on", and exited 0. It
now marks every enqueued, unresolved comment, skips ids already marked (whose
duplicates were quietly making the reviewer's re-enqueue button need N clicks),
and says out loud what it held back.
- `prereview done` did not settle a comment in the agent's snapshot:
actionableComments never consulted Processed, and emitSnapshot/LoadComments
never loaded processed.jsonl, so the field was always false there. Every later
snapshot re-handed the agent work it had already done, and on a growing queue it
re-chewed the front of the backlog and never reached the tail. SKILL.md has
always documented the opposite. The #164 reply override is untouched: a trailing
reviewer reply still reopens a done item, and re-enqueue still returns it.
Also: SKILL.md's launch line now redirects stdout. Snapshots stream to stdout for
the whole session; on an undrained pipe the buffer fills, the write blocks while
holding the emit lock, and emission stops for good with the browser still
responsive. Not observed in the wild, but the documented command was the exposed
path. A write deadline on the live channel is filed separately.
Tests: the projection at N=8 with mid-run drift, the blocked bucket, the
bucket-conservation invariant across files, done-settles + reply-reopens +
re-enqueue-returns, --all-open over a drifted store, and two browser e2e — a
blocked row renders, and the real binary + real browser reach Done = 8 after a
restructuring edit. Both e2e fail without the fix (0 rows; Done = 7).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016NH17Zerr7gujxZUSRTQh3
… path Follow-ups from review of the previous commit: - suggestionQueueState now takes the Suggestion rather than its id. The blocked branch was reachable but untested, and looking the suggestion back up by id meant a second linear scan on a path countQueue already walks per suggestion. Every caller had the value in hand. TestSuggestionBlockedOnDrift covers the state it exists for: an ACCEPTED, not-yet-applied suggestion whose OriginalText has vanished. AnchorStatus is re-derived from OriginalText on every load, so that is reachable independently of Applied — the file only has to move under an accept the agent has not written yet. - TestApplyProcessedMarks_DuplicatesOutvoteOneTombstone pins the claim the last commit message made without evidence: done is `marks > tombstones`, so three duplicate marks mean the reviewer's re-enqueue button needs three clicks. That is what made re-marking in --all-open worth fixing, and it is now asserted rather than asserted-about. - The launch snippet writes /tmp/prereview-$$.log, not a single shared /tmp/prereview.log. SKILL.md is copied verbatim by every agent, and four docs pointed at the same literal path — two concurrent reviews on one box would have interleaved their event streams into it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NH17Zerr7gujxZUSRTQh3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #203.
The rule: the queue drops an item only on an explicit human resolve. Never on drift.
Anchor drift is something that happens to a comment — and it is usually the agent's own edit that causes it. Treating it as a lifecycle transition meant a comment could leave the queue without anyone deciding it should, and the panel has no total, so a row leaving looked exactly like a row being finished.
Measured before the fix: eight comments, all marked done, five anchors invalidated →
DoneCount = 3, want 8.There is no cap, no batch size and no
5anywhere in the source. Five is simply where the agent has edited enough of the document that its own edits start invalidating the anchors of the comments it has not reached yet.Four defects, all keyed off
AnchorOutdated()QueueState()testedAnchorOutdated()beforeProcessed. A comment the agent had already marked done left the Done count the moment the agent's own edit broke its anchor.suggestionQueueStatehas always ordered it the other way and says why — "checked FIRST, since an applied suggestion is also anchor-outdated". The comment path had it backwards.""— it left both the queued and the done count. It is nowblocked: badged, counted, named in the legend ("N need re-anchoring"), and included inHasQueue()so the panel body cannot collapse and hide it. An accepted-but-unapplied suggestion whose target text is gone gets the same treatment.done --all-opensourced its ids from the agent's actionable set, which drops outdated and agent-replied comments — precisely the ones it has just finished. It skipped them, printedmarked N comment(s) as worked on, and exited 0. It now marks every enqueued, unresolved comment, skips ids already marked, and says out loud what it held back.prereview donedid not settle a comment in the agent's snapshot.actionableCommentsnever consultedProcessed, andemitSnapshot/LoadCommentsnever loadedprocessed.jsonl, so the field was always false there. Every later snapshot re-handed the agent work it had already done; on a growing queue it re-chewed the front of the backlog and never reached the tail. SKILL.md has always documented the opposite. The Replies should also appear as work in Queue #164 reply override is untouched — a trailing reviewer reply still reopens a done item, and re-enqueue still returns it.Also
SKILL.md's launch line now redirects stdout to
/tmp/prereview-$$.log. Snapshots stream to stdout for the whole session; on an undrained pipe the buffer fills, the write blocks while holding the emit lock,inEmitnever clears, and emission stops for good with the browser still responsive. Not observed in the wild — but the documented command was the exposed path. A write deadline on the live channel is filed separately as #215.Verification
Full browser e2e suite green (499s, exit 0), full unit suite and
go vetclean,make tmpl-checkgreen with the signature golden regenerated — its diff was exactly the two intended insertions, and the render goldens moved only the button'stitlestring.Both new browser tests fail without the fix:
TestE2E_QueueBlockedRow—queue has 0 rows, want 1(and the legend came back empty, becauseHasQueue()was false and the whole panel body collapsed).TestE2E_QueueDoneReachesTotalAfterDrift— real binary, real browser, eight comments,--all-open, then a restructuring edit:Done = 7, want 8.One thing worth knowing
Writing the e2e turned up something the static reading missed: a straight in-place paragraph rewrite does not produce
outdated. #196'sanchorEditedclaims it — both neighbours still match — and it stays actionable. The bug only reproduces once the edit restructures the document (five paragraphs merged into one), which shifts the neighbours.That is exactly what SKILL.md tells the agent to do: "drive a single, coherent change across the affected files — each comment is an input to that holistic change, not a standalone instruction." The instructions steer the agent into the edit pattern that broke its own queue.
🤖 Generated with Claude Code
https://claude.ai/code/session_016NH17Zerr7gujxZUSRTQh3