fix(issues): atomically claim pending review stages

[allyblockcast]

Thinking Path

• Paperclip coordinates autonomous agents through issues, heartbeats, and run-scoped execution locks.
• Atomic checkout is the concurrency boundary that prevents two runs of one agent from doing the same work.
• Pending review and approval stages intentionally remain in_review because approval-by-comment depends on that state.
• The existing runtime rejected unlocked assigned in_review checkout, leaving review work without an atomic claim path.
• PR fix(agents): tell the truth about inbox-lite's status filter (BLO-18858) #821 demonstrated that simply admitting checkout was insufficient: direct mutations could bypass the new lock and a drifted stage participant could not claim it.
• This pull request adds a participant-aware claim that preserves general assignment and enforces lock ownership through compare-and-swap write preconditions.
• The benefit is one atomic owner for review work without breaking the execution-policy state machine or human review handoffs.

Linked Issues or Issue Description

• Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-22666
• Refs fix(agents): tell the truth about inbox-lite's status filter (BLO-18858) #821
• Related contract-only split: fix(agents): tell the truth about inbox-lite's status filter and the in_review checkout contract (BLO-18858) #1116

What Changed

• Allow unlocked assigned in_review work to acquire checkout and execution run locks; pending execution-policy stages remain in_review.
• Allow only the pending stage's currentParticipant to claim a drifted review stage while preserving the issue's existing assignee fields.
• Enforce every populated pending-review run lock before agent PATCH and comment mutations, including approval-shaped comments and stage decisions.
• Carry checkout run IDs and execution state into atomic update preconditions so ownership cannot change between route authorization and the write.
• Serialize approval transitions before FK-backed comment inserts to avoid PostgreSQL lock-upgrade deadlocks.
• Update paperclipCheckoutIssue defaults and description to include in_review.
• Preserve master's paperclipCreateIssue duplicateCandidates description and test; no unrelated revert from fix(agents): tell the truth about inbox-lite's status filter (BLO-18858) #821 was carried forward.

Verification

• pnpm exec vitest run server/src/__tests__/issue-stale-execution-lock-routes.test.ts server/src/__tests__/issues-service.test.ts packages/mcp-server/src/tools.test.ts --reporter=dot (240 tests passed)
• pnpm --filter @paperclipai/server typecheck
• pnpm --filter @paperclipai/mcp-server typecheck
• git diff --check
• The route regression fails if any of the lock guard, approval-comment guard, owner-success path, participant claim, assignment preservation, or second-run conflict behavior is reverted.

Risks

• Moderate concurrency-sensitive change. The update path adds optional compare-and-swap predicates only for locked pending-review agent writes; other issue updates retain their existing behavior.
• Pending-stage checkout now refuses the general assignee when a different participant owns the stage, preventing an assignment-level actor from preempting the stage reviewer.
• No schema or migration changes.

For core feature work, check ROADMAP.md first and discuss it in #dev before opening the PR. Feature PRs that overlap with planned core work may need to be redirected - check the roadmap first. See CONTRIBUTING.md.

Model Used

• OpenAI openai/gpt-5.6-sol, tool-enabled software-engineering model with repository, terminal, GitHub, Paperclip, and structural code-review access.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-22666

[allyblockcast]

🔗 Paperclip issue: BLO-22666

[allyblockcast]

@ally please review head 46ec8a7 with focus on pending in_review lock ownership, participant-aware checkout, and PATCH/comment CAS behavior.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 46ec8a7

Critical Issues (0)

Important Issues (2)

• [code/concurrency] server/src/routes/issues.ts:4162 — The CAS is omitted when authorization observed an unlocked pending review stage. Run A can read and authorize against the unlocked row, run B can then check out and acquire both locks, and run A will still PATCH or approve without expected-null lock predicates; a plain comment also skips the guarded transaction because isLockedPendingInReviewExecutionStage(currentIssue) remains false on its stale snapshot. Run A can therefore mutate or complete run B's claimed stage and clear B's locks. Build preconditions for every pending in_review agent write, including expectedCurrentCheckoutRunId: null and expectedCurrentExecutionRunId: null when the authorization snapshot was unlocked, and add a paused-request concurrency test that checks out from run B between run A's read and write.
• [gstack/concurrency] server/src/routes/issues.ts:5317 — The shared ownership check is snapshot-only for the many mutation routes backed by assertAgentIssueMutationAllowed; only PATCH and comment writes receive write-time predicates. A request from run A can pass while it owns the stage, run A can terminate and run B can claim the same stage, and run A's delayed document, work-product, attachment, comment-deletion, or issue-deletion request will still mutate run B's issue. Revalidate the lock owner transactionally at each protected write, or provide a shared lock-owner CAS primitive that these routes use at the mutation boundary.

Suggestions (0)

Strengths

• The checkout SQL revalidates the current participant and preserves existing assignee fields for drifted stage ownership.
• The new tests cover sequential competing-run rejection, human-review handoffs, stale-lock adoption, and approval-comment atomicity.

Recommended Action

1. Close both run-ownership race windows before merge and add deterministic interleaving tests.

This PR is authored by app/allyblockcast, so the Ally App cannot review its own PR. The exact reviewed head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Addressed both Important concurrency findings at head 98dcd2257.

• Pending in_review agent writes now carry a transaction-scoped ownership receipt that pins status, execution state, and both run-lock columns, including expected NULL locks from an unlocked authorization snapshot.
• Every route backed by assertAgentIssueMutationAllowed consumes the guard at its principal persistence boundary; external resolvers stay outside the row lock, and accepted-plan decomposition revalidates in each resumable transaction.
• Added deterministic A-authorizes / B-checks-out / A-writes regressions for PATCH, plain comments, and a non-PATCH work-product write.

Verification: 559 affected server tests passed; pnpm --filter @paperclipai/server typecheck; git diff --check. Current-head CI is running. No self-approval.

[allyblockcast]

@ally please review exact head 98dcd22 with focus on the pending in_review lock ownership receipts, expected-null CAS coverage, participant-aware checkout, and every assertAgentIssueMutationAllowed persistence boundary. Confirm zero Critical findings.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 98dcd22

Prior Findings Dispositioned (2)

• prior:46ec8a7 important 1 — fixed — server/src/services/issues.ts:9170 — the ownership receipt now converts an observed null checkout/execution run into IS NULL predicates while taking the row lock; update has the same expected-null CAS at lines 8994-9007.
• prior:46ec8a7 important 2 — still-present — server/src/routes/issues.ts:10950 — the guarded PATCH transaction ends before a supplied comment is persisted through the outer service, so an unlocked pending-review request can pass the receipt check, commit its issue update, lose ownership to a checkout, and then write the bundled comment from the stale run.

Critical Issues (1)

• [gstack/concurrency] server/src/routes/issues.ts:4199 — the ownership guard is enabled only for executionState.status === "pending", but this PR also makes ordinary unlocked in_review issues checkoutable. A request can authorize against an ordinary unlocked in_review snapshot, another run can check out and move the row to in_progress, and the first request then takes the no-receipt branch and performs its mutation after the claim. This reaches destructive routes such as issue deletion because they all rely on the same helper.
  • Issue a status/run-lock receipt for every agent mutation of a checkoutable in_review row, not only pending execution stages, and add a deterministic ordinary-in_review authorize/checkout/write interleaving test.

Important Issues (1)

• [prior:46ec8a7 important 2] server/src/routes/issues.ts:10950 — PATCH protects the issue update at lines 10397-10451, but persists the request's bundled comment only after that transaction commits. For an initially unlocked pending stage, another run can claim the row between those two writes, after which the stale request still inserts its comment and runs its reference synchronization.
  • Persist the PATCH update, optional comment, and required comment-side synchronization under one ownership receipt transaction, or revalidate the same receipt at the comment persistence boundary.

Suggestions (0)

Strengths

• Expected-null checkout and execution ownership are now represented explicitly in both the row-lock receipt and update CAS.
• Participant-aware checkout preserves drifted assignee fields and the tests cover competing participant runs and approval-shaped comments.
• Deterministic hooks exercise the original unlocked pending-stage races for PATCH, plain comments, and work-product creation.

Recommended Action

1. Extend the receipt to ordinary checkoutable in_review rows.
2. Keep every principal write in a request, including PATCH's bundled comment, inside the same ownership boundary.

This PR is authored by app/allyblockcast, so the Ally App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

@ally please re-review exact head bca2e0b. The prior Critical is addressed by issuing status/run-lock receipts and CAS preconditions for every agent-observed in_review row, including ordinary non-stage issues. The prior Important is addressed by revalidating the post-update receipt while inserting PATCH's bundled comment through the transaction-bound issue service. Please focus on both deterministic authorize/checkout/write interleavings and confirm zero Critical findings.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: bca2e0b

Prior Findings Dispositioned (2)

• prior:46ec8a7 important 2 — fixed — server/src/routes/issues.ts:10951 — PATCH's bundled comment now re-enters withPendingInReviewRunOwnershipGuard, whose transaction locks and checks the post-update receipt before addComment; a checkout between the field update and comment therefore prevents the stale comment write.
• prior:98dcd22 critical 1 — fixed — server/src/routes/issues.ts:4177 — every agent-observed in_review row now receives a status/execution-state/run-lock receipt, and destructive persistence boundaries such as issue deletion consume it at server/src/routes/issues.ts:11338, closing the reported ordinary-review authorize/checkout/write bypass.

Critical Issues (1)

• [code/database] server/src/services/issues.ts:9176 — ordinary in_review rows store no execution state as SQL NULL, but the new receipt guard compares that nullable column with execution_state = 'null'::jsonb. SQL NULL cannot equal JSONB null, so a legitimate agent mutation of an ordinary unlocked in_review issue always fails with 409 even when no checkout races; the update CAS has the same mismatch at server/src/services/issues.ts:9010. Use isNull(issues.executionState) when the expected state is null, retain JSONB equality otherwise, and add a successful ordinary-in_review mutation test alongside the losing-race test.

Important Issues (2)

• [gstack/concurrency] server/src/routes/issues.ts:10951 — the second guard prevents the stale bundled comment, but the issue update has already committed in the earlier transaction at server/src/routes/issues.ts:10397. The new regression test codifies a 409 response with the title still changed at server/src/__tests__/issue-stale-execution-lock-routes.test.ts:479; clients can therefore retry a request reported as failed after part of it took effect. Persist PATCH fields and its bundled comment under one ownership transaction, or define and expose an explicit partial-success contract.
• [native-codex/authorization] server/src/routes/issues.ts:4159 — the ownership receipt pins status, execution state, and run locks but omits the assignee fields used by assertAgentIssueMutationAllowed. After agent A authorizes an unlocked assigned review, a board request can reassign it to agent B without changing any receipt field; A's guard still matches and allows the stale mutation against B's issue. Include assigneeAgentId and assigneeUserId in the receipt and transactional predicate, with a deterministic authorize/reassign/write regression.

Suggestions (0)

Strengths

• Expected-null checkout and execution run-lock predicates now cover ordinary as well as staged in_review rows.
• The current-head tests use deterministic hooks for authorize/checkout/write orderings rather than timing sleeps.
• Participant-aware checkout preserves drifted assignee fields and the second PATCH guard prevents the previously reported stale comment insertion.

Recommended Action

1. Fix SQL-null execution-state matching before merge.
2. Make bundled PATCH failure semantics atomic and pin reassignment-sensitive authorization fields in the receipt.

This PR is authored by app/allyblockcast, so the Ally App cannot review its own PR. Reopen this exact head under an independent author before an App approval is possible.

[allyblockcast]

Structural review at exact head bca2e0b85727b8236256ef2f0ecc800d0aecb9c7: changes required before landing.

1. Critical: SQL NULL is compared as JSONB null. server/src/services/issues.ts:9010 and :9176 use JSONB equality for a nullable execution_state. Ordinary in_review rows store SQL NULL, so legitimate unchanged mutations fail 409. CI confirms the production-shaped regression: General tests (server 2/4) has ten failures in done-gate-durable-artifact.test.ts where expected 200/422 became 409. Use IS NULL for a null expected state and JSONB equality otherwise; add a successful ordinary-in_review mutation regression.
2. Important: PATCH with bundled comment is partially committed on failure. The field update commits before the second ownership guard at server/src/routes/issues.ts:10951; the new test at issue-stale-execution-lock-routes.test.ts:470-483 explicitly expects a 409 while retaining the changed title. Keep the fields and comment in one ownership transaction so a failed request has no durable partial effect.
3. Important: the ownership receipt omits reassignment-sensitive assignee fields. A board reassignment can occur after authorization without invalidating the receipt, allowing the stale agent mutation against the new assignee's issue. Pin assigneeAgentId and assigneeUserId in the receipt/predicate and add deterministic authorize/reassign/write coverage.

This disposition matches Ally's exact-head review on both-surface verification: formal reviews are empty, while comment review 5212682851 targets this exact head. No additional review request was posted. Re-run the affected server suites and request exact-head review only after all three findings are fixed.

[allyblockcast]

Closing as architecturally stranded — not as wrong, and not because the work was low quality. This PR cannot be rebased forward, and leaving it open reports live work that no longer exists.

What changed under it

#1117 (kkroo, merge 8d586d450b88, merged 2026-08-11T00:23:30Z) landed the same concern by another route while this PR sat at bca2e0b8 from 08-07. git cherry origin/master bca2e0b8 still reports all +, so this content is not upstream — but the ground it was written against is gone.

Why rebase is not the answer

git rebase origin/master conflicts in 3 files on the first of the three commits (46ec8a75f, "atomically claim pending review stages") — the checkout half that #1117 has since implemented differently. Underneath that is a genuine design fork, not a textual conflict:

machinery	this branch	origin/master
withPendingInReviewRunOwnershipGuard	28 call sites	absent
isPendingInReviewExecutionStage	3	absent
currentRunMutationOwnershipSnapshot	absent	5
currentRunMutationPreconditions	absent	3

Two incompatible substrates for one concern. Porting 28 guard call sites onto master's snapshot/CAS mechanism is a re-architecture, and doing it inside a conflicted 3-commit rebase is the worst possible venue for it — this predicate already produced four consecutive Criticals across 10 review rounds.

Two things here must NOT land as-is

• packages/mcp-server/src/tools.ts removes in_review from the paperclipCheckoutIssue expectedStatuses default. Under fix(agents): tell the truth about inbox-lite status filter (BLO-18858) #1117 that is now wrong: master deliberately ships ["todo","backlog","blocked","in_review"] (tools.ts:609, pinned by tools.test.ts:192) because unlocked in_review rows are now checkoutable.
• server/src/services/external-objects.ts (+45/-35) is explicitly out of scope for BLO-22666.

What is preserved, and what happens next

Nothing is lost — branch staff/blo-22666-in-review-checkout stays on the remote, and its routes/issues.ts work remains the reference implementation for the run-ownership fences.

The Critical this PR was parked on — SQL NULL vs JSONB null on currentParticipant — is already fixed upstream: master's preserveInReviewExecutionStageCheckoutCondition (services/issues.ts:167-179) uses the null-safe coalesce(executionState -> 'currentParticipant', 'null'::jsonb) <> 'null'::jsonb.

Remaining BLO-22666 scope is now only the run-ownership fences on live-locked in_review mutations and the currentParticipant claim path, re-cut on master's substrate. Tracked on BLO-22666 with the full per-AC audit.