[codex] fix(issues): supersede in-review ownership races

[kkroo]

Thinking Path

• Paperclip coordinates autonomous agents through issues, heartbeats, and run-scoped execution locks.
• Pending review stages remain in_review so their participant and approval handoff are visible to the control plane.
• Original PR fix(issues): atomically claim pending review stages #1120 made review-stage checkout ownership explicit, but later review found that route authorization could still drift before a mutation persisted.
• The successor reconciles that work with current master checkout finalization and fences every affected mutation using one null-safe receipt compare-and-swap.
• It keeps issue ownership, execution policy, and optional comment persistence in the same transaction.
• The benefit is a single durable owner for review-stage work without regressing normal review checkout behavior.

Linked Issues or Issue Description

• Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-22666
• Refs fix(issues): atomically claim pending review stages #1120. This successor intentionally leaves fix(issues): atomically claim pending review stages #1120 open.
• Refs fix(agents): tell the truth about inbox-lite's status filter (BLO-18858) #821 and fix(agents): tell the truth about inbox-lite's status filter and the in_review checkout contract (BLO-18858) #1116.

What Changed

• Reapplies the in-review pending-stage checkout and ownership behavior from fix(issues): atomically claim pending review stages #1120 on current master.
• Adds null-safe receipt CAS fencing for assignee agent, assignee user, execution state, and execution policy; nullable JSONB values use SQL IS NULL rather than JSON equality.
• Keeps PATCH fields, execution decisions, and an optional bundled comment in one ownership-guard transaction.
• Preserves execution snapshot and current-run preconditions, including accepted-plan decomposition receipt revalidation.
• Preserves current master checkout-finalization behavior for ordinary in_review issues.
• Adds regression coverage for null execution state mutation, reassignment races, bundled-comment atomicity, and checkout finalization.

Verification

• pnpm --filter @paperclipai/server typecheck
• pnpm vitest run server/src/tests/issue-stale-execution-lock-routes.test.ts: 60 passed
• pnpm vitest run server/src/tests/issue-agent-mutation-ownership-routes.test.ts server/src/tests/issue-comment-reopen-routes.test.ts server/src/tests/issue-execution-policy-routes.test.ts packages/mcp-server/src/tools.test.ts: 369 passed
• pnpm vitest run server/src/tests/issues-service.test.ts -t issueService.addComment idempotency: 4 passed
• Full issues-service suite: 187 passed and 4 skipped; its only failure was an unrelated embedded-Postgres beforeAll timeout under concurrent host load, and the isolated affected group passed on rerun.
• git diff --check

Risks

• Moderate concurrency-sensitive change. Receipt predicates intentionally reject a stale agent mutation with 409 rather than allow a write after ownership changes.
• No schema or migration changes.
• The changed behavior is covered by route, service, and stale-lock regressions.

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 Codex, GPT-5, tool-enabled coding agent. The runtime does not expose a more specific model identifier or context-window value.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used with available 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 linked existing related issues and pull requests
• I have run focused tests locally and they pass
• I have added or updated tests where applicable
• This change does not affect the UI, so screenshots are not applicable
• Relevant documentation remains accurate because this is an internal concurrency correction with no new public endpoint or command
• I have considered and documented 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

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[kkroo]

@ally please review exact head 67c966b862788a24a918872adf2f371a4f29ae05.

Supersedes #1120. Focus on null-safe receipt CAS, assignee/policy receipt fencing,
atomic PATCH+decision+comment persistence, and checkout-finalization regressions.

[allyblockcast]

Superseded in practice by #1366 — flagging rather than closing, since this is your PR and the call is yours.

This PR and #1117 were in flight at the same time and solved overlapping halves of BLO-22666 differently. #1117 merged first (2026-08-11, 8d586d450b88) and rewrote services/issues.ts + the checkout path, which leaves this branch stranded rather than merely stale. Measured just now against current master:

• 225 commits behind, 1 ahead, not upstream (git cherry → +).
• git rebase origin/master conflicts in server/src/routes/issues.ts and server/src/services/issues.ts — precisely the two files fix(agents): tell the truth about inbox-lite status filter (BLO-18858) #1117 rewrote. Same substrate fork that stranded fix(issues): atomically claim pending review stages #1120: this branch carries withPendingInReviewRunOwnershipGuard, master evolved currentRunMutationOwnershipSnapshot / currentRunMutationPreconditions.
• The tools.ts half is now redundant — this branch's expectedStatuses: [..., "in_review"] and the reworded tool description are byte-identical to what master already ships via fix(agents): tell the truth about inbox-lite status filter (BLO-18858) #1117. (Worth stating plainly: an earlier note of mine on BLO-22666 claimed a tools.ts change here was wrong and would revert shipped behaviour. That was about fix(issues): atomically claim pending review stages #1120, which went the other direction. It does not apply to this PR — this one agrees with master.)
• It also carries an out-of-scope services/external-objects.ts change (+80).

#1366 re-cuts only the genuinely-remaining scope — AC2 (run-ownership fences) and AC3 (the currentParticipant claim path) — as a single commit on current master, with the checkout half left to #1117. Your route-level ownership work here was the reference for it, in particular the null-safe JSONB handling.

No action needed from you beyond closing this if you agree.

[allyblockcast]

Ally — Consolidated PR Review

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

Note: this PR carried an explicit review request from 2026-08-09T12:34Z and had zero Ally reviews on either surface (pulls/1212/reviews and issues/1212/comments) for ~8 days. That is a dropped review wake, not a slow one — tracked separately; flagging it here so the gap is visible on the PR itself.

The CAS fencing is the right shape and the SQL is careful in the places that usually go wrong. Both findings below are about a predicate that exists in two forms — once in SQL, once in TypeScript — which have drifted apart, and about an invariant the code asserts that this same PR falsifies.

Critical Issues (0)

Important Issues (2)

• [code/gstack] server/src/services/issues.ts:9850 — The TS gate currentHasPendingExecutionStage omits the currentParticipant check that its SQL counterpart requires, so a subset of in_review rows can become permanently un-checkoutable.

  preserveInReviewExecutionStageCheckoutCondition() (issues.ts:147) requires three conjuncts — status = 'in_review', executionState->>'status' = 'pending', and currentParticipant non-null. The TS predicate at 9850 checks only the first two. For a row with executionState = {"status":"pending"} and no currentParticipant, the two disagree by construction:

  • SQL classifies it as non-pending, so the primary UPDATE treats it as an ordinary checkout;
  • TS classifies it as pending, so currentCheckoutActorIsEligible (9855) demands allowPendingExecutionParticipant === true. The route only sets that flag when actorMatchesExecutionParticipant(...) succeeds against currentParticipant — which is absent — so it is never set.

  If such a row also holds a stale executionRunId, the primary UPDATE misses on executionLockCondition, and both stale-lock repair branches (9944, 10036) are gated off by currentCheckoutActorIsEligible. Nothing clears the lock and no actor can ever claim the issue. It fails closed rather than corrupting state, but the terminal state is a permanently stranded issue.

  That the SQL bothers with coalesce(... -> 'currentParticipant', 'null'::jsonb) <> 'null'::jsonb is itself evidence the authors consider the participant-absent state reachable.

  • Derive both from one source. Simplest fix: add the currentParticipant presence check to 9850 so the TS predicate matches issues.ts:147 exactly, and add a regression case for {"status":"pending"} with no currentParticipant plus a stale executionRunId — the existing suite always populates currentParticipant, so this hole is untested.

• [code/tests] packages/mcp-server/src/tools.ts:576 — Adding in_review to the default expectedStatuses falsifies an invariant issues.ts documents and depends on, making unassigned ordinary in_review issues claimable.

  The comment at server/src/services/issues.ts:10107 states: "Ordinary in_review is intentionally not claimable via checkout (it is excluded from every caller's expectedStatuses…)". This PR makes in_review the default for every MCP caller, so that premise no longer holds.

  Trace an ordinary (non-pending-stage) in_review row with assigneeAgentId IS NULL AND assigneeUserId IS NULL: nonPendingExecutionStageCondition is TRUE, so unassignedCheckoutCondition matches, the primary UPDATE succeeds, and checkoutStatusForCurrentRow() (issues.ts:156) resolves to 'in_progress'. The issue is pulled out of review by any agent calling checkout with defaults.

  The 422 guard at 10112 does not catch this — it fires only when current.assigneeAgentId === agentId, i.e. for the assignee, never for the unassigned case. The tool description ("Ordinary work moves to in_progress; pending execution-policy review/approval stages stay in_review") does not describe this either.

  Reachability depends on in_review rows with both assignee columns null, which is not the common shape — hence Important rather than Critical. But the widening is unconditional and the guarding invariant is now stale.

  • Either narrow the primary UPDATE so ordinary in_review requires the pending-stage path regardless of expectedStatuses, or keep the widening and update the 10107 comment to state the new invariant. Add a test for unassigned ordinary in_review + the new default list — 1264/1416 exercise the new default only against an in_progress row, so this path is uncovered.

Suggestions (2)

• [types] server/src/services/issues.ts:4764, server/src/services/external-objects.ts:778 — dbOrTx: any appears on every handle threaded through the guard. The whole correctness argument here is "this write executes inside the guarded transaction", and any is precisely the annotation that stops the compiler from checking it: a callback that ignores dbOrTx and closes over the module-level db compiles clean, runs outside the transaction, and looks guarded at the call site. Drizzle exposes a usable union for this; typing it would make the invariant load-bearing rather than conventional. Current call sites are correct — this is about the next one.
• [code] server/src/services/external-objects.ts:904 — refreshForIssue passes the same persist into a per-object loop, and each persist opens its own db.transaction. So an N-object refresh is N transactions, each re-acquiring the ownership lock; if ownership changes mid-loop, earlier objects have committed and later ones throw 409, leaving a partially-applied refresh. Probably acceptable, but it is a different guarantee than the single-transaction framing in the PR description ("keeps issue ownership, execution policy, and optional comment persistence in the same transaction").

Strengths

• The JSONB CAS is correct in the way this usually gets wrong: jsonb = is semantic (normalized key order and numerics), and nullable columns route to IS NULL instead of a JSON compare, which would otherwise silently evaluate to NULL and never match. The coalesce(..., 'null'::jsonb) guard shows the SQL-NULL vs JSON-null distinction was thought about deliberately.
• The assertCanAssignTasks bypass for a pending participant (routes/issues.ts) reads alarming but is sound: the route-level read is only a fast path, and the real authorization is re-verified atomically inside the UPDATE's WHERE via pendingExecutionParticipantCondition. The TOCTOU window is not load-bearing.
• publishLiveEvent fires after await persist(...), which resolves post-commit — so no live event announces a state change that later rolls back.
• SELECT … FOR UPDATE under READ COMMITTED re-qualifies after the lock is granted, so the receipt compare-and-swap is genuinely atomic against a concurrent writer rather than merely optimistic.
• +753 lines of regression coverage on the stale-lock routes is real investment, and the reassignment-race and bundled-comment-atomicity cases are the right ones to have written.

Recommended Action

1. Reconcile the two forms of the pending-stage predicate (issues.ts:147 vs issues.ts:9850) before merge — that one is a stranding bug, and this codebase pays a high price for stranded issues.
2. Decide whether ordinary unassigned in_review should be claimable, then make the code and the 10107 comment agree. Cover both paths with tests.
3. Consider the typing and per-object-transaction notes opportunistically.