fix(authz): make stranded-execution recovery reachable and race-safe (BLO-21947)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agent runs are dispatched by the heartbeat service and can strand in queued for hours; the control plane's repair levers for that are POST /heartbeat-runs/:id/cancel and a monitor re-arm via PATCH /issues/:id
• Both were reachable only by the run's own assignee or a human board user — but the assignee's wake path is precisely what is broken in this failure class, so a manager that detected a strand could not fix it
• feat(authz): scoped stranded-execution recovery for managing agents (BLO-21947) #1229 (kkroo) opens both levers to a managing agent under auditable preconditions. Ally raised two findings against it: a Critical TOCTOU on the cancel, and an Important one that the monitor half is unreachable for the manager it was written for
• This pull request fixes both, on top of feat(authz): scoped stranded-execution recovery for managing agents (BLO-21947) #1229 rather than as a competing PR
• The benefit is that the recovery grant actually works for its intended actor, and that a cancel authorized because a run never started cannot destroy live work if the dispatcher wins the race

Linked Issues or Issue Description

Refs BLO-21947 · Refs BLO-20396 (generalizes the CAS primitive that issue added)

Targets kkroo/blo-21947-stranded-run-recovery, not master — this is a follow-up onto #1229, deliberately kept on a human-authored PR so it stays independently reviewable. A fresh App-authored PR onto master recreates the review gate that stranded #1161. Take it, cherry-pick it, or tell me to drop it.

Related PRs searched and linked: #1229 (base, supersedes #1161), #1161 (my earlier attempt — left open and unmodified at its author's request; carries the identical ordering defect fixed here).

What Changed

• heartbeat.ts: cancelRunInternal gains requireUndispatched — compare-and-swaps the cancel on status IN (queued, scheduled_retry) AND started_at IS NULL, throwing 409 when it loses.
• heartbeat.ts: the claim is taken before the process teardown. Teardown runs ahead of the status write, so swapping in the write's usual place would let a lost race kill a freshly-dispatched process and only then refuse — reporting "conflict, nothing happened" over work already destroyed.
• heartbeat.ts: setRunStatusIfCurrentStatus (BLO-20396) generalized to accept a status set plus an extra SQL condition, so this reuses the existing primitive instead of adding a parallel one. setRunStatusIfUndispatched is the new thin wrapper.
• agents.ts: the stranded-recovery cancel passes requireUndispatched; board cancels stay unconditional.
• issues.ts: assertAgentIssueMutationAllowed gains the opt-in allowConvergenceStallMonitorRecovery, set only by PATCH /issues/:id, plus the shape predicate isConvergenceStallMonitorRecoveryPatch.
• stranded-run-recovery.ts: exports UNDISPATCHED_HEARTBEAT_RUN_STATUSES so the CAS and the route predicate share one definition.
• Tests: 3 new in issue-execution-policy-routes.test.ts, 3 new in agent-permissions-routes.test.ts.

Why the monitor half needed anything at all. assertCanManageIssueMonitor grew a manager convergence-stall branch in #1229, but a manager never reaches it: PATCH /issues/:id calls assertAgentIssueMutationAllowed first, issue:mutate denies there (allow_manager_chain is gated to issue:comment), and the guard 403s before the monitor check runs.

I confirmed this finding on 08-10, then retracted it on 08-11 after finding that hasActiveCheckoutManagementOverride does admit a manager. It does — but it sits below the !boundaryDecision.allowed early return, so it is only reachable once issue:mutate has already been allowed by some other path. If you acted on my retraction, it was wrong, and I'm sorry for the noise. Both my confirmation and my retraction were traces, which is why they could disagree; this settles it by execution instead.

The new guard is shape-gated, mirroring allowCoordinationMetadata:

• opt-in per route, so the ~two dozen other callers of that shared helper (most pointedly DELETE /issues/:id) are structurally unaffected — the PR fix(authz): wire allow_manager_chain + allow_issue_creator into issue:comment/issue:mutate (BLO-18797) #814 lesson;
• the monitor must already be cleared for convergence_stalled, re-derived from stored state and never asserted by the caller;
• the body may carry nothing but the re-arm, plus an optional comment (a manager already holds issue:comment here, so that widens nothing);
• fails closed when the issue's existing policy carries stages / reviewPreset / authorizationPolicy — a policy write replaces rather than merges, so a monitor-only body would otherwise silently erase another agent's review configuration as a side effect of recovering a monitor. Board stays available for those.

Implementation note worth keeping: the route normalizes req.body.executionPolicy in place before the guard runs, so mode / commentRequired / stages are present with schema defaults even when the caller sent only monitor. A shape predicate that counts keys there silently never matches.

Verification

npx tsc --noEmit -p server/tsconfig.json          # clean

npx vitest run \
  src/__tests__/issue-execution-policy-routes.test.ts \
  src/__tests__/issue-agent-mutation-ownership-routes.test.ts \
  src/__tests__/authorization-service.test.ts \
  src/__tests__/stranded-run-recovery.test.ts \
  src/__tests__/agent-permissions-routes.test.ts
# Test Files 5 passed (5) · Tests 383 passed (383)

The load-bearing test is issue-execution-policy-routes.test.ts → "lets a managing agent re-arm a monitor cleared for convergence_stalled", which drives the full PATCH route with a managing agent. Against #1229 unmodified it fails with exactly:

403 {"error":"Issue is outside this actor's authorization boundary (grant)",
     "details":{"reason":"deny_missing_grant","boundary":"grant"}}
  at assertAgentIssueMutationAllowed (issues.ts:5332)
  at issues.ts:9825            # the PATCH call site

assertCanManageIssueMonitor never appears in that stack. The authorization-service tests call access.decide directly and cannot observe this composition, which is how the gap survived review in the first place.

Controls included so the bypass can't quietly widen: a non-stalled monitor still 403s; a manager may not ride the bypass to also rewrite description; board cancels are asserted to stay unconstrained; a lost CAS is asserted to surface as 409.

Not verified, stated plainly: no DB-level test that a concurrent dispatch actually loses the CAS — race safety currently rests on the SQL predicate plus the route tests. The embedded-Postgres harness exists (execution-lock-orphan-cleanup.test.ts) and I'll write that test if this is taken. A full local suite run was still in progress when this was opened; I am not claiming it green.

Risks

• Medium — authorization widening. Two narrow grants for a non-board agent. Mitigated by: opt-in per route; preconditions re-derived from stored state rather than caller assertion; body shape allowlisted; both actions left unmapped in permissionForAction so the generic grant fallback cannot satisfy them.
• Low — shared helper. setRunStatusIfCurrentStatus is used by four existing cancel paths. The signature change is additive (string | readonly string[], optional condition) and all existing callers pass a single status with no extra condition, so their SQL is byte-identical.
• Behavioral change on 409. The recovery cancel can now fail where it previously always succeeded. That is the point — the previous success could have been a killed live run.
• No migrations. No UI changes.

Model Used

Claude Opus 4.5 (claude-opus-5[1m], 1M context), extended thinking, with tool use and code execution — running as the Paperclip CTO agent.

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 — n/a, server-only
• I have updated relevant documentation to reflect my changes — code comments; no user-facing docs affected
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-20396
🔗 Paperclip issue: BLO-21947

[allyblockcast]

🔗 Paperclip issue: BLO-20396
🔗 Paperclip issue: BLO-21947

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Verification
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Strengths

• Claims the undispatched run before any cancellation teardown, turning a dispatcher race into a no-op conflict rather than terminating newly started work.
• Limits the manager recovery path to a persisted convergence-stalled monitor and a monitor-only PATCH; policy configurations that could be overwritten remain board-only.
• Adds route-level authorization and conflict coverage for both recovery paths.

Recommended Action

1. No blocking issues found.