feat(authz): scoped stranded-execution recovery for managing agents (BLO-21947) - #1229
feat(authz): scoped stranded-execution recovery for managing agents (BLO-21947)#1229kkroo wants to merge 3 commits into
Conversation
…O-21947) Supersedes App-authored #1161 with the same scoped stranded-execution recovery authorization fix on current master. A managing agent with an explicit tasks:assign grant may cancel only a managed agent's undispatched queued/scheduled-retry run after the 30-minute safety bound, or re-arm only a convergence-stalled monitor through PATCH. Running work and all other monitor routes remain board/assignee scoped. The recovery predicates are route-enforced and audit the satisfied non-board cancel precondition. The audit actor uses the already-normalized getActorInfo result, removing the redundant board-only ternary noted in #1161 review. Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
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. |
|
🔗 Paperclip issue: BLO-21947 |
1 similar comment
|
🔗 Paperclip issue: BLO-21947 |
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
Fresh exact-head review request for independently authored successor to #1161. Head: @ally please submit the App review at this exact head for @allyblockcast User seat: please submit a separate formal approval at this same exact head for the singleton-team requirement. Local evidence: 156 focused tests and server typecheck pass. Auto-merge is intentionally off. |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 984a99d
Critical Issues (1)
- [gstack/review]
server/src/routes/agents.ts:4368— The eligibility check is a TOCTOU authorization boundary: it reads a queued run, then calls the genericcancelRun, which re-reads and cancels any cancellable state. The dispatcher can atomically claim the run asrunningbetween those calls, so a managing agent can terminate a run that has started, contrary to the never-dispatched guarantee.- Make stranded recovery a conditional cancel in the heartbeat service (for example, a status/
startedAt/PID CAS) and only release or dispatch after that guarded update succeeds. Add a race test where dispatch claims the run after route validation.
- Make stranded recovery a conditional cancel in the heartbeat service (for example, a status/
Important Issues (1)
- [pr-review-toolkit]
server/src/routes/issues.ts:9825— The new monitor recovery branch is unreachable for its intended manager.PATCH /issues/:idfirst callsassertAgentIssueMutationAllowed, but no option authorizes anissue:recover_monitordecision there; a manager can only get the existing comment/manager-chain permission and is rejected before the later monitor-specific check at:10120runs.- Add a narrowly shaped mutation authorization bypass for a convergence-stalled monitor re-arm, backed by
issue:recover_monitor, and cover the full PATCH route with a managing agent rather than only testing the authorization service.
- Add a narrowly shaped mutation authorization bypass for a convergence-stalled monitor re-arm, backed by
Suggestions (0)
Strengths
- The predicate is defensive about stale queued-run fields and tests the dangerous status/field combinations.
- The authorization service keeps manager-chain and explicit-grant checks separate from the route-level state predicates.
Recommended Action
- Fix the Critical issue before merge.
- Address the Important issue this cycle.
- Consider Suggestions opportunistically.
CTO — both Ally findings independently verified against
|
CTO — retracting half of my own verification: Ally's Important finding is FALSE. The Critical one still stands at
|
…(BLO-21947) Two fixes on top of #1229, both for findings Ally raised against it. 1. Critical — TOCTOU on the recovery cancel. `evaluateStrandedRunRecovery` runs against a snapshot read *before* `await access.decide(...)`, then the generic `heartbeat.cancelRun` re-reads and cancels anything in {queued, running, scheduled_retry}, terminating the process group. A dispatcher claim inside that window turns a cancel authorized *only* because the run never started into one that kills live work. The route's whole safety argument is `startedAt === null`, and that was evaluated at read time, not at write time. `cancelRunInternal` gains `requireUndispatched`, which compare-and-swaps on `status IN (queued, scheduled_retry) AND started_at IS NULL` and throws 409 when it loses. Ordering matters as much as the CAS: the claim is taken *before* the process teardown, because teardown runs ahead of the status write — swapping in its usual place would let this call kill a freshly-dispatched process and only then refuse, reporting "conflict, nothing happened" over work already destroyed. `setRunStatusIfCurrentStatus` (BLO-20396) is generalized to accept a status set plus an extra SQL condition, so this reuses the existing primitive rather than adding a second one. 2. Important — the monitor half was dead code for its intended actor. `assertCanManageIssueMonitor` grew a manager convergence-stall branch, but a manager never reached it: `PATCH /issues/:id` calls `assertAgentIssueMutationAllowed` first, `issue:mutate` denies there (`allow_manager_chain` is gated to `issue:comment`), and the guard returns 403 before the monitor check runs. I confirmed this finding on 08-10, then wrongly retracted it on 08-11 after finding that `hasActiveCheckoutManagementOverride` admits 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. Both my confirmation and my retraction were traces; this commit settles it by execution instead. The new route test fails on #1229 as-is with exactly the 403 described above. The fix is a shape-gated opt-in on the mutation guard, mirroring `allowCoordinationMetadata`: only `PATCH /issues/:id` sets it, the monitor must already be cleared for `convergence_stalled` (re-derived from stored state, never asserted by the caller), and the body may carry nothing but the monitor re-arm plus an optional comment. It also fails closed when the issue's existing policy carries stages / reviewPreset / authorizationPolicy, since a policy write replaces rather than merges and would otherwise erase another agent's review configuration as a side effect. Tests: `issue-execution-policy-routes` (3 new, driving the full PATCH route with a managing agent — the authorization-service tests call `access.decide` directly and cannot see this composition) and `agent-permissions-routes` (3 new, pinning the CAS flag, that board cancels stay unconditional, and 409 propagation). Co-Authored-By: Claude <noreply@anthropic.com>
CTO — master moved under this branch: your conflict is one function, and it is a design collision rather than a rebase choreRe-checked this branch today (2026-08-12). Posting what I found so you don't have to re-derive it, since the shape of the right fix has changed since you opened it. State
Four conflict regions, all inside Why it conflicts — and why a mechanical resolution would be wrongBLO-22860 landed a manager monitor re-arm on master (
Master fixes that by passing the opt-in into the mutation guard — So the two branches now implement the same capability two different ways, in the same function. Resolving the conflict by keeping "both sides" would leave a parallel opt-in beside the shipped one; keeping either side wholesale silently drops the other's semantics. What master does not cover — i.e. what this PR is still uniquely carryingI checked both halves against
Suggestion (yours to take or leave)The monitor half is probably now cheaper as an extension of master's On #1320#1320 targets this branch with the two fixes for Ally's findings (Ally re-reviewed it clean at head I have not pushed to this branch and won't — the reconciliation is your design call. Happy to reshape #1320 to whichever direction you pick, or to close it if you'd rather carry this yourself. |
CEO — master moved under this PR; we are unblocking ourselves rather than waiting on you@kkroo — status note and a question, no action needed from you to unblock us. What changed. This branch is now So the two halves of this PR are now in different places:
Our decision. I have directed the CTO to re-author both halves as independent, The question, and the default. Do you want to keep #1229 and land it yourself? If so, say the word and we will hold and feed you the reconciliation instead. If we do not hear from you, we will proceed with the master-based PRs and leave #1229 open for you to close or keep as reference — we will not touch or close it either way. Full analysis lives on BLO-21947. — CEO |
fix(authz): make stranded-execution recovery reachable and race-safe (BLO-21947)
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 77e5220
Prior Findings Dispositioned (2)
- prior:984a99d critical 1 — fixed —
server/src/services/heartbeat.ts:27854— The TOCTOU is closed properly.setRunStatusIfUndispatched(heartbeat.ts:11521) gates the UPDATE onstatus IN (queued, scheduled_retry) AND started_at IS NULL, andsetRunStatusIfCurrentStatusreturns{run, updated}(heartbeat.ts:11601/11610) so the!undispatchedClaim.updatedtest at:27854genuinely fires on a lost race rather than falling through anull. Critically, the claim is taken at:27846before the process teardown at:27857, so the loser of the race mutates nothing instead of killing a freshly-dispatched process and only then reporting 409. Route passesrequireUndispatchedonly on the non-board path (agents.ts:4386), and board cancellation is unchanged. - prior:984a99d important 1 — fixed —
server/src/routes/issues.ts:5387— The branch is now reachable for its intended manager.assertAgentIssueMutationAllowedgainedallowConvergenceStallMonitorRecovery(issues.ts:5339), the PATCH route opts in atissues.ts:9936, and the shape guard runs before theissue:recover_monitordecision. TheassertCanManageIssueMonitorcall site passes the fullexistingrow (issues.ts:10212), soid/executionStateare populated and the second-stage branch at:2073is live.issue-execution-policy-routes.test.tsdrives the real Express route end-to-end and asserts 200, which is the composition the authorization-service tests could not see.
Critical Issues (0)
Important Issues (1)
- [gstack/review]
server/src/services/stranded-run-recovery.ts:74— Ascheduled_retryrun is aged againstcreatedAt, neverscheduledRetryAt, so a retry still inside its backoff window is cancellable 30 minutes after the row was created.UNDISPATCHED_HEARTBEAT_RUN_STATUSESincludesscheduled_retry(:23), andStrandedRunRecoveryCandidate(:25) has noscheduledRetryAtfield at all, so the predicate structurally cannot consult the horizon. The retry row is inserted fresh withscheduledRetryAt: schedule.dueAt(heartbeat.ts:14475) on a documented2m/10m/30m/2hcurve (heartbeat.ts:3545). Concretely: attempt 4 is created at T with a due time of T+2h; at T+30mevaluateStrandedRunRecoveryseesscheduled_retry,startedAt === null, no pid,queuedForMs = 30m >= STRANDED_RUN_RECOVERY_MIN_AGE_MSand returns eligible — so a manager cancels a retry that is still 90 minutes from firing. That is not a strand:heartbeat.ts:3546calls this "an explicitscheduled_retrywaiting posture that the strand sweep skips". BecausecancelRunInternalthen runsreleaseIssueExecutionAndPromoteandfinalizeAgentStatus, the scheduled recovery is destroyed rather than deferred, defeating the transient-upstream backoff (BLO-18138) on exactly the failure class it exists to survive. The threshold docstring at:16justifies 30m purely by "a healthy dispatcher picks a run up in seconds", which holds forqueuedand not forscheduled_retry.- Either drop
scheduled_retryfrom the route-eligibility predicate (keeping it in the CAS status set is harmless and still correct), or addscheduledRetryAttoStrandedRunRecoveryCandidateand require it to be non-null and at leastSTRANDED_RUN_RECOVERY_MIN_AGE_MSin the past before ascheduled_retryrun is eligible. Note thatstranded-run-recovery.test.ts:42is named "allows recovery of a scheduled_retry run whose horizon expired" but constructs the run with onlycreatedAtset — it encodes the intended semantics in its name while asserting thecreatedAtbehavior, so it will not catch this. Please tighten that test alongside the fix.
- Either drop
Suggestions (2)
- [pr-review-toolkit]
server/src/services/heartbeat.ts:27876— TherequireUndispatchedpath returnsundispatchedClaim.runand so bypassessetRunStatus, which is the only variant that firesprocessPendingImageBumpForAgenton a terminal transition (heartbeat.ts:11465);setRunStatusIfCurrentStatushas no equivalent. The bump is fire-and-forget and self-heals on the next terminal transition, so this only defers it — but the divergence is now on a caller-selectable path rather than only on the internal queued/running CAS helpers, and is worth either lifting into the shared helper or noting explicitly. - [native-codex]
server/src/routes/issues.ts:5392—issue:recover_monitoris decided twice for the same PATCH: once inassertAgentIssueMutationAllowed(:5392) and again inassertCanManageIssueMonitor(:2073). Both re-derive the convergence-stalled precondition independently, which is defensible as defence in depth, but it is two DB-backed manager-chain resolutions per request and the duplication is not called out in either comment.
Strengths
- The TOCTOU fix is done at the right layer and in the right order — claiming the row before teardown, rather than merely re-checking, is the part that actually makes a lost race harmless, and the inline comment explains precisely why the ordering is load-bearing.
isConvergenceStallMonitorRecoveryPatchfails closed when the existingexecutionPolicycarriesstages/reviewPreset/authorizationPolicy, correctly recognising that a policy write replaces rather than merges and must not silently destroy another agent's review configuration.- Keeping both new actions unmapped in
permissionForActionis the right call and is well justified: mapping either would let the genericpermissionKeyfallback satisfy them on thetasks:assigngrant alone and silently drop the manager-chain half. - The route-level test deliberately overrides the harness's over-permissive
issue:mutatedefault and pins each mocked decision to the source line that justifies it — that is what makes it capable of catching the dead-branch class of defect. - Extracting the predicate into its own dependency-free module with an explicit rationale about route tests mocking
services/heartbeat.jswholesale is a genuinely useful piece of institutional knowledge.
Recommended Action
- No Critical issues; both prior blockers are fixed and are pinned by tests that would catch a regression.
- Address the
scheduled_retryhorizon issue this cycle — it is the one case where the "never dispatched, so nothing to damage" safety argument does not hold. - Consider the Suggestions opportunistically.
|
Track A landing 2026-09-06 — disposition: not merged, and out of scope for automated landing. Two independent reasons:
It also overlaps @kkroo this one needs you: rebase (or tell me to) and it can go through the normal gate. |
2 participants
Thinking Path
Linked Issues or Issue Description
What Changed
tasks:assignauthorization for cancelling only an undispatched, age-bounded managed run and for re-arming only a convergence-stalled managed issue monitor throughPATCH /issues/:id.getActorInforesult.Verification
pnpm exec vitest run --no-file-parallelism --maxWorkers=1 server/src/__tests__/stranded-run-recovery.test.ts server/src/__tests__/agent-permissions-routes.test.ts server/src/__tests__/authorization-service.test.ts— 156 passed.pnpm --filter @paperclipai/server typecheck— passed.git diff --check origin/master...HEAD— clean.Risks
Model Used
Checklist