Repository navigation
Conversation
A directly linked PR with green CI, mergeable MERGEABLE, mergeStateStatus BLOCKED and reviewDecision REVIEW_REQUIRED only waits on a human review. getEffectiveDirectPullRequestGateCondition still turned its 'ready' base condition into 'action_required', because BLOCKED is in ACTION_REQUIRED_GITHUB_PULL_REQUEST_MERGE_STATE_STATUSES. isActionableDirectPullRequestGate then counted it as follow-through work: the first sync took the issue from its current assignee mid-run (Paperclip cancelled that run as issue_reassigned), assigned it to the PR's followThroughAssigneeAgentId and woke them with nothing to do. The direct/issue-linked gate now classifies a PR as 'waiting' when isGitHubPullRequestPendingExternalWaitForSync holds (unfinished CI behind a BLOCKED/UNSTABLE merge state) or when it is a BLOCKED maintainer-approval wait with reviewDecision REVIEW_REQUIRED. The remote-action fingerprints read the same remoteCondition, so status, owner selection and wake routing stay consistent. The 'waiting' status mapping keeps an already-blocked external wait blocked (it was protected when the gate was actionable) and keeps an in_review required-review wait in_review. The exemption is narrower than isGitHubPullRequestBlockedMaintainerApprovalWaitForSync, which also accepts a null review decision: GitHub returns null when no review is required, so BLOCKED there comes from another rule and still wakes the executor, as the existing merge-state routing test expects. CHANGES_REQUESTED needed no change. A summary-only "Request changes" review on a branch that requires reviews leaves the PR BLOCKED with CHANGES_REQUESTED, which is outside the exemption and still action_required, so it wakes the owner (covered by the new test). On a branch without required reviews the PR stays CLEAN and maps to in_review; SPEC.md already requires that an aggregate CHANGES_REQUESTED alone, which GitHub keeps after the author pushes fixes, does not move the issue back to active execution.
After hand-back a ticket usually belongs to another agent, typically the product owner, who gates it in in_review. Agents that set no executionPolicy or executionState lose it there: on a transition to or within in_review, resolvePaperclipIssueReviewAssignee returns null and shouldClearTransitionAssignee clears the agent. That happened for a required-review wait (green CI, BLOCKED, REVIEW_REQUIRED) and for the CLEAN mapping without required reviews. shouldKeepAgentAssigneeDuringPullRequestWait keeps an existing agent assignee, with no wake, whenever the effective gate is 'waiting' or 'ready' and the issue is not done or cancelled, in both the direct-PR and the mapped-issue paths. This mirrors what sync already did for a human assignee. A done issue restored to in_review still has its stale agent cleared, and an execution policy's reviewer still wins when Paperclip exposes one. Two existing tests asserted that an agent is unassigned on a healthy in_review wait; they now expect the agent to stay. A new test covers the product owner staying on in_review for BLOCKED/REVIEW_REQUIRED and CLEAN, with 0 wakes over two passes.
…reviewer A reviewer requests changes with inline threads. The author pushes, replies on every thread and hands the ticket back. GitHub keeps the threads unresolved and reviewDecision CHANGES_REQUESTED until the reviewer looks again, so every sync classified the PR as review_actionable (or action_required for the BLOCKED merge state), treated the hand-back as local drift, moved the ticket back to the author and woke them with nothing to do, every pass until the reviewer approved. Sync now loads, for PRs with unresolved review threads, the PR author, the last comment of each unresolved thread and the latest reviews (GitHubPullRequestReviewThreadsReplyState, one small query per such PR, sync paths only). The threads wait on the reviewer when the author wrote the last, submitted comment on every unresolved thread and no other user submitted a CHANGES_REQUESTED or APPROVED review, or a review with a summary body, after the author's latest thread reply or review. Such threads stop counting as actionable, and a BLOCKED PR with green CI and no conflict is a 'waiting' gate whatever its review decision, so the ticket keeps its current holder and nobody is woken, as for the required-review wait. Red CI and merge conflicts keep their priority and still wake the owner. A reviewer reply in a thread, a new change request, an approval (the author then resolves the threads and finishes) or a review with a body after the reply makes the threads actionable again; the gate fingerprint changes, so the owner is woken once. While the threads wait on the reviewer, the PR author's new comments are the answers, so the trusted-comment rule no longer re-arms the work for that author; without that, the replies themselves bounced the ticket back once. A deliberately blocked issue stays blocked during the wait, and the external-state hash records the flag only when it is set, so hashes already persisted for other PRs stay valid. Replies alone count; a push newer than the latest change request is not required. An author may answer a change request by disagreeing, explaining, or asking a question, all without a commit, and requiring a push would put the ticket back on the author in exactly those cases. GitHub also has no reliable push time to compare: Commit.pushedDate is deprecated and committedDate is rewritten by a rebase. If the reply does not satisfy the reviewer, the reviewer's next comment or review hands the work back. An empty COMMENTED review is not reviewer activity: GitHub creates one around every thread comment, and the threads already show those.
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
1 participant
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.
Summary
What breaks today. For a directly linked pull request, sync treats three states as work for the PR author although the author has nothing to do. Each time, it takes the ticket from whoever holds it, assigns the PR's
followThroughAssigneeAgentIdand wakes them. The woken agent finds nothing to do and hands the ticket back. If the holder is an agent with a run in progress, Paperclip cancels that run asissue_reassigned.MERGEABLE,BLOCKED,REVIEW_REQUIRED, no threadsBLOCKEDis an action-required merge state, so the gate becomesaction_requiredand wakes the authorin_reviewwait (required review, orCLEANwithout required reviews)shouldClearTransitionAssigneeunassigns the agent gating the ticket (for example a product owner after hand-back), because no execution policy names a reviewerCHANGES_REQUESTEDstays until the reviewer re-reviewsreview_actionable, so the author's hand-back counts as local drift and is reverted on every passScenario (live, third case). A reviewer requests changes with two inline threads. The author pushes a fix, replies on both threads and hands the ticket back to the product owner in
in_review. On every following sync (every 15 minutes) GitHub Sync moves the ticket back to the author with "unresolved review threads" and wakes them. The author replies that the threads are answered and hands back, and the next pass repeats it. This goes on until the reviewer approves.What changes. A PR that waits on someone else becomes a
waitinggate. The ticket keeps its holder and nobody is woken:fix(sync): treat a PR waiting only on a required review as a wait.BLOCKEDwithREVIEW_REQUIRED, green CI and no unresolved threads is a wait. Anullreview decision is excluded: GitHub returns it when no review is required, soBLOCKEDcomes from another rule and still wakes the executor. A summary-only "Request changes" review staysaction_required.fix(sync): keep the agent holding a ticket while its PR waits. On awaitingorreadygate an existing agent assignee stays assigned, the same way a human assignee already did. Adoneissue restored toin_reviewstill has its stale agent cleared, and an execution policy's reviewer still wins when Paperclip exposes one.fix(sync): treat review threads the author answered as a wait on the reviewer. For PRs with unresolved threads, sync makes one small extra GraphQL query (author,reviews(last: 20), and the last comment of each thread). The threads wait on the reviewer when the PR author wrote the last submitted comment on every unresolved thread, and no one else submitted aCHANGES_REQUESTEDorAPPROVEDreview, or a review with a body, after the author's latest reply or review. A reply counts without a push, because an author may answer by disagreeing. While the threads wait, the author's own new comments are the answers, so the trusted-comment rule does not re-arm the work for that author; without this, the replies themselves bounce the ticket back once. Red CI and merge conflicts still wake the owner. A reviewer reply, a new change request, an approval (the author then resolves the threads and finishes) or a review with a body after the author's answer makes the threads actionable again, and the owner is woken once.Evidence
All tests are in
tests/plugin.spec.ts.sync.runNow hands a direct PR to its follow-through owner only when the author has work to do, scenario green CI waiting only on a required review: the ticket moves toengineer-agentwith 1 wake.After: it stays with
po-agentwith 0 wakes. Summary-only change request, unresolved thread and red CI still go toengineer-agentwith 1 wake.sync.runNow keeps a product owner on an in_review direct PR ticket while the PR waits(BLOCKED/REVIEW_REQUIREDandCLEAN):po-agentis unassigned.After:
po-agentstays, 0 wakes over two passes. Two existing tests that expected the unassignment now expect the agent to stay.sync.runNow leaves a change request the author answered with the ticket holder until the reviewer asks again: the answered scenario moves the ticket toengineer-agenton pass 1.After:
a PR blocked only on a required review is a waiting gate, not follow-through workanda PR whose review threads the author answered waits on the reviewer until the reviewer asks againcover the classifier (emptyCOMMENTEDreviews, approvals, unsubmitted replies, unknown author, resolved threads), gate, status mapping, blocked preservation and the external-state hash.pnpm typecheckandpnpm testpass after each of the three commits.Merge Danger
Door: two-way
No persisted state changes shape. The external-state hash records the answered flag only when it is set, so hashes already stored for other PRs stay valid. Reverting the commits restores the old routing.
Blast Radius: direct-PR routing
This affects tickets with directly linked or issue-linked open PRs in the three wait states. Those tickets stop being reassigned and stop waking the follow-through owner. One case trades a missed wake for no loop: an author who answers a thread with a promise ("will fix") and hands back is not woken again until the reviewer replies. PRs with unresolved threads cost one more small GraphQL query per sync.