fix(issues): fence a second run off a live-locked pending in_review stage (BLO-22666) - #1366
Conversation
…tage (BLO-22666) AC2: run-ownership was never asserted on an `in_review` row. ensureAgentCheckoutOwnership early-returns on `status !== "in_progress"`, and the approval-shaped-comment branch gates on identity only, so the SAME agent's second run could decide the stage run A was sitting on — the BLO-18858 duplicate-work defect, reintroduced through the review path that #1117 opened up. New `assertPendingReviewRunOwnership` is the `in_review` counterpart of assertCheckoutOwner, kept separate rather than widening that one: assertCheckoutOwner is keyed on `in_progress` in eight places and owns the stale-lock adoption ladder. The fence is deliberately narrow — live-locked AND `assigneeAgentId === actor` AND the stage is pending. By the time control reaches it the actor may legitimately be a mention-granted peer, a manager-chain actor, a recovery owner, a human, or a drifted `currentParticipant`; none hold the checkout and all must still approve. Widening past the assignee term would break the approval-by-comment path this issue exists to protect, which is a worse regression than the bug. AC3: a pending stage pinned to a `currentParticipant` whose id has drifted off `assigneeAgentId` could never take the lock, so its already-authorized stage decision raced whoever held it. The claim writes ONLY the lock columns — assignee, status and startedAt are untouched — so it cannot become a back door to general issue ownership, and the participant pin is re-asserted inside the UPDATE as a compare-and-swap.
1 similar comment
Requesting a human review — @kkroo — because the automated reviewer is down, not slow@kkroo asking you specifically because you authored #1117, which shipped the checkout half of BLO-22666. This PR is the remaining AC2 + AC3 half, re-cut on the substrate #1117 established. You have the most context on it of anyone. Deliberately not requesting Ally. Ally is pinned to
So Ally's silence here is not a per-PR delivery drop — I checked that first, and the repo-wide sweep is what ruled it out. Posting a State at head
|
1 participant
Thinking Path
Linked Issues or Issue Description
What Changed
isForeignRunOfLockedPendingReview(routes/issues.ts) — predicate identifying the one actor that must be fenced: a second run of the issue's own assignee, on a live-locked, pendingin_reviewstage.assertPendingReviewRunOwnership(services/issues.ts) — thein_reviewcounterpart ofassertCheckoutOwner, kept as a separate entry point rather than widening that one. It clears locks held by terminal runs first (otherwise a dead run A would fence its own agent's run B forever), re-reads, and only then fences on a live foreign run.ensureAgentCheckoutOwnership(PATCH), the approval-shaped-comment branch, and the execution-stage decision path.currentParticipantwhose id has drifted offassigneeAgentIdcan now take the lock. The claim writes only the lock columns;assigneeAgentId,assigneeUserId,statusandstartedAtare untouched, so it cannot become a back door to general issue ownership. The participant pin is re-asserted inside theUPDATEas a compare-and-swap.issue-agent-mutation-ownership-routes.test.ts, four of them negative controls.Verification
Each guard was verified load-bearing — neutered individually, then re-run:
isForeignRunOfLockedPendingReview→falseallowExecutionStageParticipantClaim→falseThe four negative controls stayed green under both reverts, which is the point — they must be insensitive to the fence:
in_reviewrow is still freely checkoutable (does not re-close the door fix(agents): tell the truth about inbox-lite status filter (BLO-18858) #1117 opened)One flake seen and run to ground:
issue-stale-execution-lock-routes.test.tsfailed once inafterEachwith an FK violation onagent_wakeup_requests— an async wakeup insert racing teardown. Re-run 3× on this branch: 53/53 each time. It is not caused by this change; the affected test's fixture hasassigneeAgentId !== actor, so the new fence provably does not arm on it.Risks
The regression to fear is over-fencing, not under-fencing. By the time control reaches this predicate the actor has already cleared the authorization boundary, so it may legitimately be a mention-granted peer reviewer, a manager-chain actor, a recovery owner, a human, or a drifted
currentParticipant— none of whom hold the checkout, and all of whom are supposed to be able to approve. TheassigneeAgentId === actorterm is what keeps them out of the fence and must not be widened; doing so would deadlock every review stage. Four prior Criticals on this predicate came from exactly that over-reach, which is why four of the eleven tests exist only to pin the non-fenced paths.Placement in
ensureAgentCheckoutOwnershipis deliberate: after the recovery-owner and unassigned early-returns so no rescue path can be fenced, and before the blocked-correction and participant-decision returns, which are the two ways run B would otherwise slip past.Not rebased from #1212 or #1120. Both predate #1117 and are built on the
withPendingInReviewRunOwnershipGuardsubstrate that master replaced; I tested it rather than assuming —git rebase origin/masteron #1212's head conflicts inroutes/issues.tsandservices/issues.ts, the exact two files #1117 rewrote, and #1212 is 225 commits behind with its checkout half now redundant and an out-of-scopeexternal-objects.tschange. No migration, no API-shape change.Model Used
claude-opus-5[1m], 1M context), extended thinking, via Claude Code with tool use.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatepaperclipCheckoutIssuedescription already shipped with fix(agents): tell the truth about inbox-lite status filter (BLO-18858) #1117