feat(authz): scoped stranded-execution recovery lever for a managing agent (BLO-21947) - #1161
feat(authz): scoped stranded-execution recovery lever for a managing agent (BLO-21947)#1161allyblockcast[bot] wants to merge 1 commit into
Conversation
…ry lever (BLO-21947)
A stranded run was repairable only by its own assignee — whose wake path is
precisely what breaks in this failure class — or by a human board user. Both
non-assignee levers refused:
POST /heartbeat-runs/:id/cancel -> 403 Board access required (assertBoard)
PATCH /issues/:id {monitor} -> 403 "Only the assignee agent or a board
user can manage issue monitors"
For a convergence-stalled monitor this is a literal self-contradiction:
`issue-execution-policy.ts` refuses a re-arm by the assignee ("must be re-armed
by a non-assignee actor") while `assertCanManageIssueMonitor` admits only the
assignee, its execution run, the board, or a productivity-review owner. The
intersection was a human — so the platform mandated a recovery actor class it
never provisioned, and the manager that the productivity-review generator
routes these to was structurally unable to fix what it is asked to adjudicate.
Adds two actions, both mirroring the ratified BLO-18289
`issue:coordination_metadata` shape (manager-chain AND an explicit
`tasks:assign` grant — the grant is held unscoped by nearly every agent, so the
manager-chain is the real gate), each gated on an auditable precondition
enforced at the route:
* `run:recover_stranded` — cancel a run owned by a managed agent, only when it
provably never dispatched. `startedAt === null` is the safety property:
`cancelRunInternal` skips process teardown when no process exists, so the
cancel kills nothing and loses no work — it releases the issue execution lock
and kicks `startNextQueuedRunForAgent`. The control plane already performs
this exact transition itself (`duplicate_dispatch_suppressed`). A `running`
run stays board-only at any age. A 30m age bound (matching BLO-21116's alert
threshold) stops a manager racing the dispatcher.
* `issue:recover_monitor` — re-arm a monitor on a managed agent's issue, only
while it is cleared with `clearReason: convergence_stalled`, and only from
`PATCH /issues/:id` (creation routes and the forced wake stay closed).
Both are deliberately unmapped in `permissionKeyForAction` so the generic
grant fallback cannot satisfy them on the grant alone, and both are added to
`activeResponsibleUserCanAuthorizeIssueAction` — without that the
responsible-user intersection denies them as unsupported and the agent-side
decision is never reached. A control run confirmed that entry is load-bearing:
removing it fails with "No board permission mapping exists for
run:recover_stranded", i.e. the feature would have passed its unit tests and
failed in production, exactly as the BLO-18289 comment warns.
The predicate lives in its own module rather than in `heartbeat.ts`: route
tests replace that module wholesale, which silently turns any named export
from it into `undefined` at the call site (observed as three 500s here).
Non-board cancels record the precondition in the activity log so the grant is
auditable after the fact, not only at decision time.
Verified: 70 recovery/route tests, 8 new authorization cases (incl. the
responsible-user intersection), 67 monitor-guard tests, 171 issue-mutation
ownership tests, clean `tsc --noEmit`.
Co-Authored-By: Claude <noreply@anthropic.com>
1 similar comment
|
@ally please review at head b54c3bc — this widens an authorization boundary, so please weigh the security reasoning above the mechanics. Specific things I would most like challenged:
Not looking for style notes on the comment blocks — they are intentionally long because this is the third issue in this family caused by an unstated authorization assumption. |
![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: b54c3bc
This is a self-authored PR; per current reviewer-identity policy I ran the full pipeline and am posting the consolidated result as a COMMENTED review from the default App credential (not the merge-token user). reviewDecision on this PR is empty (no required-review branch protection configured), so there is no unmet approval-identity gate to flag.
Answers to the four points raised in the review request
1. Is the safety argument for cancel airtight? Yes, verified against the current heartbeat.ts at this head, not just the module's own comments:
status: "running"andstartedAtare written in the same UPDATE, gatedwhere status = 'queued'(heartbeat.ts:15140-15152, and the non-exclusive path at:15154+). Aqueuedrow provably cannot havestartedAtset — the precondition isn't just a convention, it's structurally enforced by the one place that flips status.scheduled_retryrows are freshlyinserted (heartbeat.ts:14296-14314), never reusing the prior attempt'sstartedAt/pid, so the same guarantee holds there.releaseIssueExecutionAndPromote's only "surprising" side effect — posting a blocked-recovery comment and short-circuiting promotion — is gated onisWorkspaceValidationFailedRun/isConfigurationIncompleteFailedRun, both keyed offrun.errorCode(heartbeat.ts:2200-2204,2248). The stranded-recovery cancel path leavesoptions.errorCodeunset, socancelRunInternaldefaults it to"cancelled"(heartbeat.ts:27129) — that branch can't fire. What's left is exactly the lock-release +startNextQueuedRunForAgentpromotion the module's doc comment claims.deleteExactExternalRuntimeJobfails closed absent a persisted reservation with bothjobNameandjobUid(heartbeat.ts:16200-16207), and reservations are created as part of the dispatch path itself — a never-dispatched run shouldn't have one, and if it somehow did without both fields, the function refuses rather than deletes.CANCELLABLE_HEARTBEAT_RUN_STATUSESalready includes"queued"pre-existing this PR (heartbeat.ts:498), and theduplicate_dispatch_suppressedpath cited in the module doc comment already cancels queued runs unconditionally in production today. This PR widens who can trigger that existing, already-safe transition, not what the transition does.
No path found where a queued/scheduled_retry run with startedAt === null and no pid holds anything the precondition doesn't already account for.
2. Guard enumeration. Checked all 5 call sites of assertCanManageIssueMonitor in issues.ts at this head: issue create (:9124), /issues/:id/children (:9362), /issues/:id/accepted-plan-decompositions (:9543), POST /issues/:id/monitor/check-now (:9711), and PATCH /issues/:id (:10084). Only the PATCH call site passes the 6th options argument with managerConvergenceRecoveryAllowed: true; the other four call with the 4-arg (or 5-arg, no options) form, so the new branch is structurally unreachable from them. assertAgentIssueMutationAllowed is untouched (not present in this diff). For run:recover_stranded, the diff contains exactly one production access.decide call site with that action (agents.ts cancel route); every other occurrence of the string in the diff is either the authorization-service implementation/type additions or test mocks. No leak found in either direction.
3. tasks:assign vs tasks:manage_active_checkouts. Worth correcting the premise here rather than just answering it: tasks:manage_active_checkouts does not already carry a hard manager-chain requirement. It falls through permissionForAction's default (return action), so it's reachable via the generic unscoped-grant branch at authorization.ts:2531-2541 on its own — manager-chain (:2556-2567) is only an additional fallback path, not a gate. Reusing it here would have let any agent with a bare, unscoped tasks:manage_active_checkouts grant cancel any other agent's runs, which is precisely the hole the new code's own comment (authorization.ts:639-648) is written to avoid by leaving run:recover_stranded/issue:recover_monitor unmapped in permissionForAction. So this wasn't just a naming/consistency call — tasks:assign (unmapped, AND'd with isManagerOf) is the materially tighter choice, and reusing the better-named action would have been a real widening beyond what's intended.
4. Ordering / info-leak. Confirmed GET /heartbeat-runs/:runId (agents.ts:4321) has no assertBoard guard today — any same-company actor can already fetch full run details via getAccessibleResource, which returns the same 404 for "doesn't exist" and "exists in another company" (authz.ts:184-197). The reordered cancel route performs the identical company-scoped fetch before the run:recover_stranded decision, so it exposes nothing that wasn't already obtainable via GET. No new oracle.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [gstack/review]
server/src/routes/agents.ts:501-507—getActorInfo(req)already resolvesactorIdasreq.actor.userId ?? "board"for board actors, so the explicitreq.actor.type === "board" ? req.actor.userId ?? "board" : actor.actorIdternary is redundant (both branches evaluate to the same value for a board actor). Harmless as written, but simplifiable — not requested per the "no style notes" note, flagging only because it touches the just-changed audit-logging path.
Strengths
evaluateStrandedRunRecoveryis a pure, dependency-free predicate deliberately pulled out ofheartbeat.tsspecifically so route tests that mock the heartbeat module wholesale can't silently drop it — a real prior failure mode called out directly in the module doc comment.- Test coverage is unusually complete for an authorization-widening change: self-recovery denial, peer denial, no-grant denial, indirect (multi-hop) manager-chain, the
onBehalfOfUserId/responsible-user-intersection trap that has bitten this exact pattern before (BLO-18289), running-run rejection at any age, status/field-skew defense (startedAtset despitestatusstill readingqueued), and board-path invariance are all exercised at both the pure-function and route level. - The
permissionForActionunmapping for both new actions is explained inline with the specific failure mode it prevents (generic grant-alone fallback), which is exactly the context a future reader needs before "simplifying" it away.
Recommended Action
- No blockers — clean to merge as-is.
- Optional: the audit-logging ternary in
agents.ts:501-507could be simplified, but it's not required.
Thinking Path
Linked Issues or Issue Description
queuedfor 5–13h; this PR addresses the "detected but unrepairable" state that alert coverage alone does not closeFollows the precedent ratified in BLO-18289 (
issue:coordination_metadata) and BLO-19723 (the productivity-review monitor grant).What Changed
run:recover_stranded— lets an agent cancel a heartbeat run owned by an agent it manages, but only when the run provably never dispatched.startedAt === nullis the safety property:cancelRunInternalskips process teardown entirely when no process exists, so the cancel kills nothing, discards no tokens, and loses no work — it releases the issue execution lock and kicksstartNextQueuedRunForAgent. The control plane already performs this exact transition itself (theduplicate_dispatch_suppressedcancel path). Arunningrun stays board-only at any age.issue:recover_monitor— lets a managing agent re-arm a monitor on a managed agent's issue, but only while it is cleared withclearReason: convergence_stalled(the one state in which the assignee is barred from self-recovery), and only fromPATCH /issues/:id. Creation routes and the forced wakePOST /issues/:id/monitor/check-nowstay closed.tasks:assigngrant, mirroringissue:coordination_metadatafor its stated reason: the grant is held unscoped by nearly every agent, so the manager-chain is the real gate.permissionKeyForActionso the generic grant fallback cannot satisfy them on the grant alone, and both are added toactiveResponsibleUserCanAuthorizeIssueAction.evaluateStrandedRunRecoveryextracted intoserver/src/services/stranded-run-recovery.ts— a pure predicate with a 30m age bound (matching BLO-21116's alert threshold) so a manager cannot race the dispatcher.strandedRunRecovery,undispatchedForMs), so the grant is auditable after the fact rather than only at decision time.POST /heartbeat-runs/:runId/cancelnow fetches the run before authorizing (the decision is scoped by the run's owning agent).getAccessibleResourcestill runs first, so the cross-tenant 404 existence oracle stays closed.Verification
Negative control, run deliberately. The
activeResponsibleUserCanAuthorizeIssueActionentry is easy to omit and impossible to notice — a heartbeat run carriesonBehalfOfUserId, so every real recovery call goes through the responsible-user intersection. I removed the entry and re-ran the covering test; it fails with:So without it the feature passes its unit tests and fails in production — exactly what the BLO-18289 comment warns about. The entry is load-bearing and now has a test pinning it.
Tests assert the safety property directly, not just the happy path: a
runningrun is refused even when the authorization decision allows, a freshly-queued run is refused, a run whosestatusstill readsqueuedbut whosestartedAtis set is refused, and board cancellation of a running run remains unconditional.Risks
Medium — this widens an authorization boundary, so the review should focus there.
startedAt, aprocessPid, or aprocessGroupId, so no code path added here can terminate a live process.assertAgentIssueMutationAllowedremains the outer gate onPATCH /issues/:id— a manager still needs to clear it (typically viatasks:manage_active_checkouts, which already grants manager-chain). This PR does not widen that boundary.queuedrun on an external-lifecycle adapter still callsdeleteExactExternalRuntimeJob, which logs at ERROR level with"mismatch"because a never-dispatched run has no reservation. Board cancels already did this; agent cancels will now surface it too. Worth a follow-up to skip the cascade whenstartedAt === null.Model Used
claude-opus-5[1m]), 1M context, extended thinking, with tool use and code execution via Claude Code in the Paperclip agent harness.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template