fix(recovery): let a recovery owner comment, and bound its wake budget (BLO-18996)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• The recovery subsystem detects stranded work and mints a source_scoped_recovery_action that names an ownerAgentId and wakes it to restore a live execution path
• Nothing in that path checks that the owner it names can actually write to the issue the action is about — and issue:comment admits only three actors (assignee, unassigned, mention-granted peer), none of which is "recovery-action owner"
• So the owner wakes, gets 403 Issue is outside this actor's authorization boundary, cannot discharge, the action stays active, and every subsequent sweep re-fires the same wake forever with nothing surfaced to anyone who could fix it — a silent, unbounded, billable loop
• This pull request admits that owner for comments on its own source issue, and gives owner-wake recovery actions a wake budget so an undischargeable one stops re-firing and says so
• The benefit is that a stuck recovery becomes a visible terminal state a human can act on, instead of an invisible loop

Linked Issues or Issue Description

• Closes BLO-18996
• Refs BLO-18145 (where this was first observed, on BLO-18142)

Related PRs found in the dedup search — please read this before reviewing:

• #827 (BLO-18906) — closest neighbour, complementary not duplicate. fix(authz): comment-only handoff grant for a recovery-reassigned previous owner (BLO-18906) #827 grants comment access to the previous owner (the agent recovery took the issue from, so it can hand off its diagnosis). This PR grants comment access to the named recovery owner (the agent recovery gave it to, so it can discharge the action it was woken for). Opposite ends of the same transfer; neither subsumes the other. They do touch the same two files and both add a member to the AuthorizationDecision["reason"] union, so whichever lands second needs a small textual rebase.
  • One structural question for the reviewer: fix(authz): comment-only handoff grant for a recovery-reassigned previous owner (BLO-18906) #827 implements its grant inside authorization.ts; this PR implements its grant in the route-level comment helper. I chose the route level deliberately — assertAgentIssueCommentAllowed has exactly one call site, whereas agentIssueDecision in authorization.ts is shared by issue:read/issue:comment/issue:mutate and by every access.decide caller including filterIssuesForActor. Given BLO-18996 explicitly asks for minimum blast radius, narrower won. If the maintainers would rather have both grants in one place for consistency, say so and I will move mine — I would rather that be an explicit decision than a silent divergence.
• #814 (BLO-18797) — the creator / manager-chain allow-paths. Explicitly out of scope here and not depended on; it is back in draft over an active-run-guard bypass, and BLO-18996 says not to assume it lands.
• #824 (BLO-18860) — recovery checkout-adoption; different code path, no overlap.
• #388 / #521 — merged, and the direct precedent this reuses: they established recovery-owner mutation and checkout admissions. This is the missing comment leg of the same idea.

What Changed

(A) Admit the recovery owner for comments — server/src/routes/issues.ts

• assertAgentIssueCommentAllowed now admits the named owner of an open (active/escalated) recovery action on that action's own sourceIssueId, via a new actorOwnsActiveRecoveryActionOnIssue helper. This mirrors the admission assertAgentIssueMutationAllowed already makes for the same pairing.
• The grant surfaces as a distinct allow_source_scoped_recovery_owner decision reason (added to the union in server/src/services/authorization.ts) so it is auditable rather than masquerading as a mention grant.
• isIssueMentionGrantDecision becomes isCommentOnlyPeerGrantDecision, covering both grants. Effect: on a closed source issue the recovery owner may comment but a reopen/resume still has to clear assertAgentIssueMutationAllowed on its own — same treatment a mention-granted peer already gets.

(C) Bound the re-fire — server/src/services/recovery/service.ts

• New STRANDED_RECOVERY_MAX_OWNER_WAKE_ATTEMPTS (default 5, env-overridable, floored at 2) and a pure strandedRecoveryWakeAttemptsExhausted predicate, both re-exported from server/src/services/recovery/index.ts.
• ensureSourceScopedStrandedRecoveryAction sets maxAttempts to that budget only for causes that actually wake an owner. Provider-quota monitor waits and workspace/config manual-repair holds keep maxAttempts: null, because they return early from the wake path by design and are expected to sit open for a long time — giving them a ceiling would manufacture a spurious exhaustion.
• enqueueSourceScopedStrandedRecoveryWake returns without waking anyone once the budget is spent.
• On exhaustion the source issue gets one system comment (deduped on the action id) naming the action, the attempt count, the cause, and the human next step — including that reassigning resets the budget, since the recovery-action fingerprint includes the assignee.
• The escalation activity-log entry gains recoveryActionMaxAttempts and recoveryWakeBudgetExhausted.

Tests

• New describe("source-scoped recovery owner comment grant (BLO-18996)") in issue-agent-mutation-ownership-routes.test.ts.
• New wake-budget test in issue-recovery-actions.test.ts.
• heartbeat-process-recovery.test.ts asserted maxAttempts: null on the shared owner-wake action shape; that expectation encoded the unbounded behaviour this PR removes, so it now asserts the budget.

Verification

Both new tests were run against master and confirmed to fail there, then to pass on this branch:

Test	On master
issue-agent-mutation-ownership-routes.test.ts › source-scoped recovery owner comment grant (BLO-18996) › lets the owner of an active recovery action comment on its source issue	403 Issue is outside this actor's authorization boundary (grant) — the reported error verbatim
issue-recovery-actions.test.ts › stops waking the recovery owner once the wake budget is spent and says so on the source issue	7 escalations produce 7 wakes; the < 7 assertion fails

The first test reproduces the reported shape exactly: owner agent ≠ assignee, and the owner's runId deliberately does not match the issue's checkoutRunId/executionRunId (the "checked out against a different issue" half), which is what makes isCurrentIssueExecutionRun fall through to the boundary check. Four negatives ship alongside it — non-owner agent, board-owned (ownerAgentId: null) action, reopen-on-closed, and cross-company — and all four already pass on master, which is the evidence that the grant does not widen anything they cover.

The second test asserts 7 escalations produce fewer than 7 wakes, exactly one exhaustion comment, and that a further sweep is a no-op on both counts.

Local runs on this branch:

vitest run issue-agent-mutation-ownership-routes + issue-recovery-actions + heartbeat-process-recovery
  → 287 passed (3 files)
vitest run authorization-service + issue-comment-reopen-routes
  → 155 passed (2 files)
vitest run recovery-classifiers + issue-liveness + low-trust-red-team-routes
            + issue-blocker-attention + recovery/service.pause-durability
  → 85 passed (5 files)
vitest run recovery-observability + attention-service
  → passed
pnpm --filter @paperclipai/server typecheck
  → exit 0, 0 errors

Dashboard / log query. The stuck class is select count(*) from issue_recovery_actions where status in ('active','escalated') and max_attempts is not null and attempt_count > max_attempts, and the escalation activity log now carries the same two fields so it can be counted without a join. I have not run this against production and it would not be meaningful yet — max_attempts has never been populated for stranded actions, so it returns 0 on both sides today. Real before/after numbers are owed on the issue once this deploys; flagging rather than claiming the AC satisfied. No UI change is needed to see it: IssueRecoveryActionCard already renders attempt N of M whenever maxAttempts is set.

Risks

Blast-radius enumeration — BLO-18996 explicitly asks for this, because #814 shipped a cancel-any-agent's-run bypass through a fully green pipeline by widening a boundary without tracing everything on its branch and in its helper.

• Routes sharing the helper being edited. assertAgentIssueCommentAllowed has exactly one call site: POST /issues/:id/comments. assertAgentIssueMutationAllowed — which backs ~two dozen mutation routes including DELETE /api/issues/:id — is not touched. Neither is decideIssueAccess.
• Guards sharing the branch being bypassed. The new admission sits inside if (!boundaryDecision.allowed). The only guard after that block is the in_progress + assigneeAgentId === actorAgentId run-id requirement, which is unreachable on this path: an actor that is the assignee would have been allowed by allow_self and never reached the deny branch. The guards before it (assertTaskWatchdogScopedIssueMutationAllowed, isCurrentIssueExecutionRun) still run first, unchanged.
• in_progress active-run 409 guard. Untouched — it lives in assertAgentIssueMutationAllowed and comments never reach it. A recovery owner attempting reopen/resume on a closed source issue is still routed through that helper and denied; covered by a test.
• Scope of the grant. getActiveForIssue(companyId, issueId) filters on sourceIssueId = issue.id AND status IN ('active','escalated'); the helper additionally requires ownerAgentId === actorAgentId and a matching company, and returns false when ownerAgentId is null. No other issue is reachable. Company isolation in fact fires earlier (getAccessibleResource 404s a foreign-company actor) — also covered by a test.
• authorization.ts change is type-only. One new member on the reason union. authorizationBoundaryLabel switches only on deny_* and funnels the rest to unlabelledBoundary, whose parameter type is Exclude<..., deny_${string}> — an allow_* reason is assignable, so no build break and no behavioural change. (This is the line that will conflict with fix(authz): comment-only handoff grant for a recovery-reassigned previous owner (BLO-18906) #827.)
• maxAttempts had no prior readers outside issue-recovery-actions.ts; packages/shared validates it as a positive int ≤ 100, and 5 satisfies that.

Behavioural risk of the wake budget. A legitimately slow recovery that genuinely needs more than 5 sweeps would now stop being woken. Mitigations: a discharge resolves the action so the next escalation starts a fresh one at attempt 1; a reassignment changes the action fingerprint and also starts fresh; attemptCount only increments when the sweep re-escalates a still-stranded issue, so reaching 5 means five consecutive failures to make progress; and the exhaustion comment tells a human exactly what to do. The budget is env-overridable if 5 proves too tight in practice.

Base branch. Branched from master (81308c700), not from the BLO-18829 branch the assigned workspace was detached on. BLO-18829 (#818/#820, both draft) refactors the same wake-decision function into a plan/dispatch split, so stacking would have made this unmergeable until those land. Whichever merges second needs a small rebase; the three edits port cleanly either direction.

Out of scope, unchanged: BLO-18145's exit-128 / stderr-capture work, job_missing durability (BLO-18106), and merging or unblocking #814. No credentials, permissions beyond the single path above, runner labels, caches, or agent instructions change.

Model Used

Claude Opus 5 (claude-opus-5), 1M context, extended thinking, with tool use and code execution — running as a Paperclip claude_k8s 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, no UI change (the existing recovery card renders the newly-populated maxAttempts with no code change)
• I have updated relevant documentation to reflect my changes — no user-facing doc covers recovery-action authorization; the rationale lives in code comments at both call sites
• 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

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-18829
🔗 Paperclip issue: BLO-18142
🔗 Paperclip issue: BLO-18145
🔗 Paperclip issue: BLO-18106
🔗 Paperclip issue: BLO-18996

[allyblockcast]

🔗 Paperclip issue: BLO-18829
🔗 Paperclip issue: BLO-18142
🔗 Paperclip issue: BLO-18145
🔗 Paperclip issue: BLO-18106
🔗 Paperclip issue: BLO-18996

[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: ## 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: 8555702

Important Issues (2)

• [gstack/review] server/src/services/recovery/service.ts:3770 — Reassignment does not reset an exhausted action's wake budget. upsertSourceScoped looks up the active row by source issue, then overwrites its fingerprint/owner while incrementing the existing attemptCount; therefore a changed assignee still reaches this new exhaustion guard with the old spent counter and the replacement owner is never woken. This also contradicts the exhaustion notice's claim that reassignment resets the budget. Resolve/create a fresh action when the fingerprint changes, or atomically reset the counter and notification state before evaluating the new owner.
• [gstack/review] server/src/routes/issues.ts:3622 — The recovery-owner capability replaces every failed issue:comment authorization decision, including deny_low_trust_boundary and policy-restricted denials, rather than only the intended missing-grant case. An owner of an active recovery action can consequently comment outside an explicit trust boundary. Preserve hard scope/policy/trust-boundary denials and apply this narrow override only to the ordinary peer-agent missing-grant decision (with a regression test for a low-trust actor).

Suggestions (1)

• [pr-review-toolkit] server/src/__tests__/issue-recovery-actions.test.ts:1019 — Exercise exhaustion through the production reconciliation lifecycle, including reassignment after exhaustion. The current test repeatedly calls the escalation helper directly, so it does not catch active-row fingerprint reuse or prove that the real sweep path resets and wakes a replacement owner.

Strengths

• The comment grant is otherwise narrowly scoped to the active action's source issue and owner, and the tests cover non-owner, unset-owner, cross-company, and reopen denial cases.
• The wake ceiling and one-time operator-facing exhaustion notice address the original unbounded-cost failure mode directly.

Recommended Action

1. Address the Important issues before merge.
2. Add the lifecycle regression coverage in this cycle.

[allyblockcast]

Both Important issues accepted — fixes scheduled, not landing in this pass

Thanks. I've verified (2) directly against the diff and accept (1) on the mechanism.

(2) routes/issues.ts:3622 — you're right, and the miss is worth naming precisely. The admission sits inside if (!boundaryDecision.allowed) { … }, so it fires on every denial reason, deny_low_trust_boundary and policy denials included — not just the ordinary missing-grant case.

BLO-18996 explicitly required enumerating the blast radius after #814, and my PR body did enumerate two of the three dimensions: every guard sharing the branch, and every route sharing the helper (assertAgentIssueMutationAllowed's ~two dozen routes, DELETE /api/issues/:id, the in_progress 409). What it never enumerated was the set of denial reasons that reach the branch I hooked into. Widening an authz boundary means enumerating what flows into the bypass, not only what sits around it. Green CI missed it again.

Fix will gate on the specific peer-agent missing-grant reason and preserve hard scope/policy/trust-boundary denials, with the low-trust-actor regression test you asked for.

(1) service.ts:3770 — accepted. upsertSourceScoped reusing the active row and incrementing the existing attemptCount across a fingerprint/owner change means a reassigned owner inherits a spent budget and is never woken. That also makes my own exhaustion notice wrong where it tells the operator reassignment resets the budget — so this is a correctness bug and a false instruction to a human. Fix will either resolve/create a fresh action on fingerprint change or atomically reset counter + notification state before evaluating the new owner.

Suggestion accepted too — the current test calls the escalation helper directly, so it cannot catch active-row fingerprint reuse. I'll drive exhaustion through the real reconciliation lifecycle including post-exhaustion reassignment.

Not pushing these in this pass, deliberately. (2) is a security-relevant narrowing and (1) changes shared recovery-action upsert semantics; both deserve a full verify cycle rather than an end-of-run patch. #814 is the cautionary case — it shipped a cancel-any-agent's-run bypass through 19/19 green precisely because a boundary change looked small. This PR stays open and unmerged until both land with tests. Tracked on BLO-18996.

Unrelated but worth flagging: the ready_for_review webhook that woke me for this PR carried #824's review body rather than this one's, which cost a detour to untangle. Filing that separately.

[kkroo]

Pushed 674daf5ec to bring this branch up to current origin/master and clear the merge conflict.

What changed:

• combined the BLO-18996 source-scoped recovery owner comment grant with the newer recovery handoff comment-only grant in routes/issues.ts / services/authorization.ts;
• kept recovery handoff grants explicitly comment-only for reopen/resume and excluded them from approval-shaped comment auto-approval;
• hardened agent-hires-instructions-materialize.test.ts cleanup with retrying fs.rm to address the transient ENOTEMPTY cleanup failure.

Local verification passed:

• pnpm exec vitest run server/src/__tests__/agent-hires-instructions-materialize.test.ts server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts server/src/__tests__/issue-recovery-actions.test.ts --reporter=dot
• pnpm --filter @paperclipai/server typecheck
• git diff --check

[allyblockcast]

Both Important issues landed at 9feef246 — with one correction to the proposed fix for (1)

Pushed rather than deferred. One of the two remediations had to diverge from what we both agreed to, and that divergence is the substantive content of this comment.

(1) service.ts wake budget — fixed, but not by keying on the fingerprint

We both proposed "resolve/create a fresh action on fingerprint change." That fix is wrong, and it fails in the direction that silently removes the bound.

The stranded fingerprint ends in issue.assigneeAgentId (recovery/service.ts:3604-3611), and escalateStrandedAssignedIssue reassigns the issue to the recovery owner as part of escalating. So the fingerprint changes on every sweep of a single unresolved failure — not only on reassignment. Keying the reset on it restarts attemptCount every sweep, maxAttempts is never reached, and the unbounded re-fire loop this PR exists to stop comes back — while the tests still look green, because the exhaustion assertions are about a bounded count.

I implemented it that way first and it did exactly that. Instrumenting two consecutive escalations of one issue:

inFp:  …:stranded_assigned_issue:f94131bd-…   ← assignee segment changed
exFp:  …:stranded_assigned_issue:2407af7e-…
reset: true                                   ← every sweep
inOwner: 14ac1a42-…  exOwner: 14ac1a42-…      ← owner held steady

It also broke the pre-existing attemptCount: 2 assertion in "reuses the same source-scoped action when latest run IDs change" — which is what surfaced it.

So the reset keys on ownerAgentId. That is the better key on the merits, not just the one that works: the wake budget counts times we woke this agent about this action, so the agent is what the count belongs to, and replacing the agent is what must start a fresh sequence. It also matches the failure you described exactly — "a reassigned owner inherits a spent budget." Computed in the same UPDATE as the owner write, so no sweep can observe a new owner carrying an old counter.

Two follow-on corrections, both of which were also lying to operators:

• The exhaustion notice told operators reassignment restores the budget. It didn't. Now it says handing the action to a different owner does, and re-running against the same owner does not.
• The notice's dedup marker was keyed on action id alone. Since the row is reused across reassignments, a later owner's exhaustion would never have been announced. Marker now carries the owner.

I also left a note at the wake idempotencyKey (service.ts:3809): attemptCount is now unique per (action, owner sequence, attempt), not per (action, attempt). Safe today — enqueueWakeup coalesces on (companyId, agentId, taskKey), never on idempotencyKey, and the replacement owner is a different agent — but anyone adding idempotency dedup there must put the owner in the key first, or they reintroduce this exact deadlock.

(2) routes/issues.ts:3622 grant over-reach — fixed as proposed

Now gated on deny_missing_grant alone, the ordinary peer-agent fall-through at authorization.ts:2142. Terminal denials stay terminal: deny_low_trust_boundary and deny_policy_restricted (authorization.ts:1012-1019), plus deny_scope, deny_missing_membership, deny_company_boundary.

Five parametrized route tests, one per reason, each with the actor genuinely being the named recovery owner so only the reason differs. Each returns 201 instead of 403 without the fix — I verified by reverting the gate. Plus the complement, asserting the grant still admits on deny_missing_grant, so a future narrowing can't quietly disable the thing this PR is for.

Suggestion — taken

The exhaustion regression now drives exhaustion → reassignment → re-exhaustion through the real escalateStrandedAssignedIssue path. It fails pre-fix. Two things it caught that the helper-level test structurally could not:

• Wake counts must be asserted per owner: the assignee-fallback branch (service.ts:3776-3807) also calls enqueueWakeup, so total call count over-counts.
• Ownership routes through the assignee's reportsTo, so the sweep walks up to the CTO unless the assignee is held on the second reporting line. My first version measured owner churn while appearing to measure the budget.

One thing you didn't flag, found while merging

Post-rebase my grant could still reach the in_review comment auto-approval — the mutation-without-issue:mutate path #827's finding 3 had just closed for its own grant. A comment-only grant must not reach a done transition, so I excluded it on the same reasoning, scoped to callers admitted solely by this grant (an owner who is also the assignee is allow_self and unaffected). The mention grant stays included, as #827 intended.

Branch state — please note

This was CONFLICTING: #827 merged into the same issue:comment block and the same AuthorizationDecision["reason"] union. I had rebased onto master locally, but while I was working @omarramadan pushed 674daf5ec ("Merge origin/master into BLO-18996 branch") to this branch, which also brings in more of master than my rebase base did (#834, issue-run-holding.ts).

I discarded my rebase and re-applied on top of that merge as a fast-forward, so his commit and his conflict resolution are intact — including his closedCommentGrantPeerAgentCommentOnly naming, which I deliberately did not rename. No force-push.

Verification

server typecheck clean (0 errors). 326 tests green on the new base across issue-recovery-actions, issue-agent-mutation-ownership-routes, issue-comment-reopen-routes, authorization-service, issue-run-holding; 609 green across the wider set on the pre-merge base.

Not self-approving and not merging. Back to you — the parts most worth a second pair of eyes are the deny_missing_grant allow-list (is one reason the right width?) and whether owner-keyed reset is the semantics we want when ownership legitimately alternates between two agents, since each switch grants a fresh budget.

[allyblockcast]

⚠️ Correction to my previous comment — the owner-keyed reset does NOT preserve the bound. Do not review 9feef246 as final.

I claimed above that "the restored budget is still a budget." That is wrong, and I found it by testing the open question I had just asked instead of leaving it open. Flagging immediately rather than after a review cycle.

I probed a 4-level org (CEO → CTO → EM → Eng) and logged the owner and attemptCount on the action row across 30 consecutive sweeps of one unresolved issue:

[OWNERSEQ] EM#1 CTO#1 CEO#1 CTO#1 CEO#1 CTO#1 CEO#1 CTO#1 CEO#1 CTO#1 CEO#1 ...
[WAKES]    EM CTO CEO CTO CEO CTO CEO CTO CEO CTO CEO ...  total=30  budget=5

30 sweeps, 30 wakes, budget 5. attemptCount never exceeds 1.

The mechanism is a pre-existing routing ping-pong that my reset turns into an unbounded loop:

• Sweep N: assignee is CTO → resolveStrandedIssueRecoveryOwnerAgentId takes assignee.reportsTo = CEO → escalation reassigns the issue to CEO.
• Sweep N+1: assignee is CEO → CEO.reportsTo is null → falls through to the cto/ceo role candidates, which order cto first → CTO → reassigns to CTO.
• Repeat forever.

Every sweep is an owner change, so keying the reset on ownerAgentId restarts the counter every sweep — the same failure shape as the fingerprint key I rejected, just with a longer period. My lifecycle test missed it because it pins the assignee back onto one reporting line before each sweep (deliberately, to isolate the budget), which suppresses exactly this. The two-agent seed in the other tests also can't show it: there the CTO is the top of the chain, so routing stabilizes on the first hop.

So both candidate keys are unsafe, for the same underlying reason: anything derived from current issue/owner state churns, because escalation itself mutates that state. A correct bound has to be anchored to something escalation does not move.

What I'm doing

Keeping (2) — the deny_missing_grant narrowing — which is independent, verified, and unaffected by any of this.

For (1) I'm adding an outer bound that owner churn cannot reset, using the existing issue_recovery_actions.timeoutAt column (set once at insert, preserved across updates, so no migration): the action stops waking anyone once it is past that horizon, regardless of owner. The per-owner attemptCount reset stays as the inner bound, which is what makes a genuinely reassigned owner reachable again — Ally's original finding, still valid. strandedRecoveryWakeAttemptsExhausted becomes "per-owner budget spent or past the horizon," and the ping-pong regression above becomes a test.

Please hold off reviewing until I push that — I'll comment when it's in. Sorry for the extra cycle; better here than in prod.

Worth naming as the general lesson, since it has now bitten this PR twice: for a row that a sweep rewrites on every pass, "did X change?" is not a safe reset trigger. Ask instead what the counter is protecting against, and anchor the bound outside the sweep's own writes.

[allyblockcast]

Horizon bound pushed at 9d32d197 — ready for review now

Follow-up to my correction above. The owner ping-pong is fixed; both of your Important findings remain addressed.

What changed

timeoutAt is now a second, independent bound, anchored to the action's creation — the one thing about the row that no sweep rewrites. strandedRecoveryWakeAttemptsExhausted becomes "per-owner budget spent or past the horizon", still gated on maxAttempts !== null so the monitor-only and manual-repair shapes are untouched. Existing column, no migration.

The load-bearing detail is in upsertSourceScoped: it preserves an existing timeoutAt instead of taking the input. Escalation re-derives the horizon from now on every sweep, so adopting the input would push it forward each pass and make it exactly as useless as the counter it backstops. That one line is what the whole bound rests on.

The per-owner attemptCount reset stays as the inner bound — that is what makes a genuinely reassigned owner reachable again, your original finding, and it is still verified by the lifecycle test.

Cross-caller check on timeoutAt

Preserving a shared column changes who wins, so I enumerated the writers rather than assuming (the #814 lesson, and the reason this PR needed a second pass at all):

• pr_review_non_convergence (service.ts:7427) — maxAttempts: null, so the maxAttempts === null early return fires before the horizon is read. Inert.
• Provider-quota scheduler (service.ts:3983) — writes timeoutAt = retryAt via a direct UPDATE; a later upsert used to clear it and now preserves it. Also maxAttempts: null, so also inert for wake bounding.
• Nothing else in the codebase reads this column.

Noted inline so the next person doesn't have to re-derive it.

Test

Reproduces the 4-level ping-pong (CEO → CTO → EM → Eng) and asserts the wakes stop. It first asserts that ownership really is churning and that no single owner ever spent its budget — otherwise it could pass for the wrong reason and prove nothing. Verified to fail without the horizon: 40 wakes where 20 are expected.

The exhaustion notice now distinguishes which bound fired, because the operator's next move differs — a spent per-owner budget is restored by handing the action to a different owner, the horizon is not restored at all. Dedup keys differ per bound for the same reason: the attempt notice keys on the owner (a later owner may legitimately exhaust again), the horizon notice keys on the horizon instant (it stays fired while ownership churns underneath, so an owner key would re-announce every sweep).

Verification

server typecheck 0 errors. 452 tests green across issue-recovery-actions, issue-agent-mutation-ownership-routes, heartbeat-process-recovery, issues-service. Both regression suites confirmed failing without their respective fixes.

Note CI on 8555702b had one unrelated failure — ENOTEMPTY tmpdir teardown in agent-hires-instructions-materialize.test.ts (1034 passed, 1 failed). Omar's merge touches that file and cleanup-heartbeat-test-state.ts, so it may already be handled; worth a glance if it recurs on this head.

STRANDED_RECOVERY_OWNER_WAKE_HORIZON_MS defaults to 6h, env-overridable. That number is a judgement call rather than a measured one — if you have a better sense of how long a legitimately-slow recovery can take, say so and I'll change it.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:8555702 important 1 — fixed — server/src/services/issue-recovery-actions.ts:221 — an owner change now atomically resets the reused action's attemptCount to 1, so a replacement owner no longer inherits the immediately previous owner's exhausted counter.
• prior:8555702 important 2 — fixed — server/src/routes/issues.ts:3636 — the recovery-owner override now applies only to deny_missing_grant; explicit trust-boundary, policy, scope, membership, and company denials remain terminal.

Important Issues (2)

• [gstack/review] server/src/services/issue-recovery-actions.ts:221 — Resetting the only counter on every owner change does not preserve a spent budget when an owner returns. The production routing can naturally move from a manager to CTO/CEO as escalation reassigns the source issue, then select a previous role candidate again; an A -> B -> A sequence gives A a fresh counter each time and can keep every action below the exhaustion guard indefinitely. The new regression test checks A then B but never routes back to A, despite claiming A's budget remains spent.
  • Persist attempt usage per owner, impose a non-resetting per-action/deadline ceiling in addition to the per-owner sequence, or create a fresh action whose lifecycle makes the reset explicit; add an alternating-owner test that runs past the configured ceiling.
• [native-codex] server/src/services/recovery/service.ts:3754 — Provider-quota actions are always assigned maxAttempts: null, but the existing routing at lines 3298-3305 assigns a manager-ladder ownerAgentId when the quota-hit agent is not invokable, and enqueueSourceScopedStrandedRecoveryWake then wakes that owner. That fallback remains an unbounded billable loop, contradicting the nearby assumption that every provider-quota action is monitor-only and never wakes an owner.
  • Exempt only the actual monitor-only shape (provider_quota && !ownerAgentId); any action with wakePolicy.type === "wake_owner" should receive the same bounded budget.

Suggestions (1)

• [pr-review-toolkit] server/src/services/recovery/service.ts:4583 — The "one-time" exhaustion notice is deduped only against the latest 50 system comments. Once newer automation comments push the marker out of that window, another sweep can emit it again. Prefer a durable action field or an exact unbounded/indexed marker lookup.

Strengths

• The authorization fix now preserves hard trust and tenancy denials and includes focused regression coverage for each denial class.
• The replacement-owner test exercises the real escalation path and closes the original immediate counter-inheritance defect.
• The exhaustion telemetry and operator-facing notice make a genuinely spent budget observable.

Recommended Action

1. Address the Important issues before merge.
2. Consider the durable exhaustion-notice marker in this cycle.

[allyblockcast]

Review response — 2 fixed, 1 already fixed before the review landed (a48009e3)

Thanks — both Important findings were real. One note on scope before the dispositions.

A timing artifact worth flagging. This review reports Reviewed head: 9feef246, but 9d32d197 was pushed at 19:02:34Z and the review was submitted at 19:04:43Z — about two minutes later. So the review did not see the newer commit, and its first Important finding was already fixed by it. GitHub attributes the review to 9d32d197, which makes this hard to spot from the API. Calling it out so the next pass doesn't re-report it.

---

Important 1 — alternating owner A -> B -> A resets the budget forever

Already fixed in 9d32d197, before this review was submitted. No change in a48009e3.

That commit added exactly what this finding asks for: a non-resetting per-action ceiling alongside the per-owner sequence. timeoutAt becomes a creation-anchored horizon, and upsertSourceScoped preserves it rather than re-deriving it from now on every sweep — which is the property that makes it immune to owner churn. strandedRecoveryWakeAttemptsExhausted is now "per-owner budget spent OR past the horizon."

The diagnosis in the finding is right, including the mechanism: escalation reassigns the source issue to the recovery owner, routing then reads the new assignee's reportsTo, and in an org deeper than two levels that ping-pongs CTO → CEO → CTO because the CEO has no reportsTo and the role fallback orders cto first.

On "the new regression test checks A then B but never routes back to A" — correct about that test, and 9d32d197 added a second one rather than extending it, which is why it wasn't visible here. bounds the wakes even when recovery ownership ping-pongs and never spends one owner's budget runs a 4-level org through STRANDED_RECOVERY_MAX_OWNER_WAKE_ATTEMPTS * 4 sweeps and first asserts that ownership really is churning (distinctOwnersWoken.size > 1) and that no single owner ever spent its budget (attemptCount <= maxAttempts) — so it cannot pass for the wrong reason — then asserts the wakes stop. Verified to fail without the horizon: 40 wakes where 20 are expected.

Important 2 — provider-quota with a manager-ladder owner is unbounded ✅ fixed

Real, and the sharper framing is that the budget and the wake path were two separately-written conditions that had to agree, and disagreed on exactly one shape. provider_quota is monitor-only only when it has no owner; resolveStrandedRecoveryRouting (service.ts:3298-3305) hands it a manager-ladder owner whenever the quota-hit agent isn't invokable, and that shape takes the wake_owner branch and clears every early return in enqueueSourceScopedStrandedRecoveryWake.

Rather than exempt the monitor-only shape by name, both now derive from one wakesOwner predicate — the same one those early returns implement. Writing the condition twice is what let them drift, so the fix is to write it once.

Cross-caller re-check, since this widens who reads timeoutAt as a horizon: the quota scheduler's timeoutAt = retryAt write is gated on isProviderQuotaWait, which requires !action.ownerAgentId — exactly the shape that still carries maxAttempts: null and returns false from strandedRecoveryWakeAttemptsExhausted before the horizon is ever read. The two shapes can't collide on that column. The comment in issue-recovery-actions.ts asserting the older, broader claim ("provider-quota actions carry maxAttempts: null") is now false and has been corrected rather than left to mislead.

Tests: the new case asserts the harm first (wake count bounded) rather than just maxAttempts being populated, so a regression reports the billable loop instead of a config value. Verified to fail without the fix — expected null to be 5. A companion test pins the ownerless monitor-wait as still unbounded, so a future tightening can't manufacture a spurious exhaustion on a normal quota wait.

Suggestion — durable exhaustion marker ✅ taken

Agreed, and the 50-comment window was worse than it looks: the notice fires at the end of an action's life, which is precisely when the issue carries the most automation chatter, so the marker aging out wasn't a corner case.

Took the second of your two options — an exact unbounded marker lookup, filtered by issue + author in SQL and capped at one row. It's also cheaper than the 50-row fetch it replaces. I skipped the durable action field because it needs a migration for no additional guarantee.

---

Verification. issue-recovery-actions 48/48, issue-agent-mutation-ownership-routes 106/106, recovery service + heartbeat-process-recovery 202/202 — all green locally. CI is running on a48009e3.

Still open from your earlier review and unchanged: the structural question in the PR description about whether this grant belongs in authorization.ts alongside #827's rather than in the route helper. Happy to move it if you'd prefer both grants in one place.

[allyblockcast]

Review disposition — all 3 findings addressed

Thanks — both Important findings were real. Note on sequencing: the review body reads Reviewed head: 9feef246, and 9d32d197 landed at 19:02:34Z, ~2 min before the review was submitted — so it was not in scope for this pass. Mapping each finding to the commit and test that closes it:

Important 1 — owner ping-pong resets the only counter (issue-recovery-actions.ts:221)

Fixed in 9d32d197. Correct diagnosis: an A -> B -> A sequence gave A a fresh attemptCount each time, and the regression test never routed back to A.

Rather than persisting per-owner usage, the row now carries a creation-anchored timeoutAt horizon that owner churn cannot reset — the upsert preserves the existing value instead of adopting the sweep's freshly-derived input (timeoutAt: existingTimeoutAt ?? input.timeoutAt ?? null). Every sweep re-derives input.timeoutAt from "now", so taking it would push the horizon forward on each pass and make it exactly as resettable as the counter it backstops. The per-owner counter stays as the fast bound; the horizon is the non-resetting ceiling underneath it.

Test: bounds the wakes even when recovery ownership ping-pongs and never spends one owner's budget — alternates owners and runs past the configured ceiling, which is the coverage gap you named.

One cross-caller interaction checked while making the horizon sticky: the provider-quota scheduler writes timeoutAt = retryAt on this row via a direct UPDATE, and a later upsert previously cleared it. It is now preserved — see the next finding for why that is no longer load-bearing.

Important 2 — provider-quota actions get maxAttempts: null even when they wake an owner (recovery/service.ts:3754)

Fixed in a48009e3. This was two expressions that disagreed on exactly one shape. The budget predicate is now the single wakesOwner = Boolean(ownerAgentId) && …, matching the early returns, so the exemption applies only to the genuinely monitor-only shape (provider_quota && !ownerAgentId) — precisely your recommended narrowing. Any action that reaches wakePolicy.type === "wake_owner" now gets STRANDED_RECOVERY_MAX_OWNER_WAKE_ATTEMPTS, including the manager-ladder fallback at 3298-3305 that made this an unbounded billable loop.

Tests: bounds provider-quota recovery once it falls through to a manager-ladder owner and keeps the ownerless provider-quota monitor unbounded, and lets its retry horizon stand — the second pins the exemption so the narrowing can't silently widen back.

Suggestion — exhaustion notice deduped against the latest 50 comments

Taken, in a48009e3. Agreed the window was self-defeating: the notice fires at the end of an action's life, exactly when automation traffic is most likely to have pushed the marker out. Replaced with an exact unbounded marker lookup filtered by issue + author in SQL and capped at one row (LIKE … ESCAPE), rather than scanning latest-N.

---

CI is re-running on 7cc19d2a (merge of master). Re-requesting review via the draft→ready toggle.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:9feef24 important 1 — fixed — server/src/services/recovery/service.ts:214 — wake exhaustion now includes a creation-anchored timeoutAt ceiling, and the current-head ping-pong regression at server/src/__tests__/issue-recovery-actions.test.ts:1225 verifies owner churn cannot keep waking past that horizon.
• prior:9feef24 important 2 — fixed — server/src/services/recovery/service.ts:3770 — the budget is now derived from whether the action actually wakes an owner, so manager-owned provider-quota recovery receives maxAttempts while the ownerless monitor-only shape remains exempt.

Important Issues (2)

• [gstack/review] server/src/services/issue-recovery-actions.ts:246 — Preserving every existing timeoutAt can suppress a newly assigned recovery owner before its first wake. An ownerless provider-quota action stores its retry deadline in this column with maxAttempts: null; if the source agent later becomes non-invokable, the same active row gains a manager owner and a non-null budget, but this line retains the old quota deadline instead of the new six-hour wake horizon. Once that retry deadline is past, strandedRecoveryWakeAttemptsExhausted immediately rejects the manager wake. Replace the old timeout when transitioning from an unbounded action (existing.maxAttempts === null) to a bounded owner-waking action, or store the wake horizon separately, and cover ownerless quota monitor -> expired deadline -> manager-owner transition.
• [native-codex] server/src/services/recovery/service.ts:3882 — The exhaustion guard counts committed upsert attempts rather than successful wakes. recoveryActionsSvc is closed over the outer db, so its attempt increment commits outside escalateStrandedAssignedIssue's transaction before deps.enqueueWakeup runs; if enqueue throws, the issue transaction rolls back but the attempt remains spent. Five transient enqueue failures can therefore consume the budget without waking anyone, after which this guard permanently skips enqueue and the notice falsely says the owner was woken five times. Increment the wake count only after enqueue succeeds, or make the attempt reservation and enqueue failure rollback/compensation atomic; add a throwing-enqueue regression.

Strengths

• The recovery-owner comment grant is limited to deny_missing_grant, preserving tenancy, scope, policy, membership, and low-trust denials.
• The exact-head tests cover owner churn, owned versus ownerless provider-quota recovery, authorization denial classes, and one-time exhaustion notices.
• The operator-facing exhaustion telemetry makes stopped recovery actions visible instead of silently dropping them.

Recommended Action

1. Fix the two Important state-accounting issues before merge.
2. Add transition and enqueue-failure regressions in the existing recovery-action suite.

[allyblockcast]

Both Important findings fixed at 8102227

Both were real, both reachable, and both reinstated the exact deadlock this PR exists to close — my own comments at those sites argued they were inert, and the argument was wrong in the same way twice: it reasoned about the row's state at the moment of one write instead of over the row's lifetime. These rows are long-lived and change shape underneath you.

1 — timeoutAt inherited across the unbounded → bounded transition (issue-recovery-actions.ts)

Confirmed the mechanism end to end. The quota scheduler writes timeoutAt = retryAt on this row via a direct UPDATE (recovery/service.ts:4049) and does not touch maxAttempts, so the row keeps maxAttempts: null. My comment then claimed the two shapes "never collide on this column" — true only while the budget stays null. When the quota-hit agent stops being invokable, resolveStrandedRecoveryRouting gives that same active row a manager owner and a budget, and a quota retryAt is minutes out, so it is in the past by then. strandedRecoveryWakeAttemptsExhausted (:210-215) short-circuits on the null budget, but once the budget is non-null it reads the stale horizon and returns true on the new owner's first wake.

Fix: preserve the horizon only once the row is actually bounded; adopt the fresh wake horizon on the existing.maxAttempts === null → input.maxAttempts !== null transition. The anti-ping-pong property is unchanged — after the bound begins the horizon is never rewritten, so owner churn still cannot push it forward. Staying unbounded still preserves, which keeps the quota retryAt intact and leaves pr_review_non_convergence (also maxAttempts: null) untouched.

2 — budget spent on enqueues that woke nobody (recovery/service.ts)

Also confirmed: recoveryActionsSvc closes over the outer db, so upsertSourceScoped's attemptCount increment commits on its own connection before deps.enqueueWakeup runs, and escalateStrandedAssignedIssue's rollback cannot claw it back.

Fix: enqueueOrRefundAttempt wraps both wake enqueues (owner and assignee-fallback), refunds the attempt via a new releaseWakeAttempt (floored at 0, scoped to active statuses + company), then rethrows so the escalation still fails loudly. The refund's own error is swallowed so it can never mask the enqueue's. I went with compensation rather than a separate wakeCount column to stay migration-free, consistent with the rest of this PR — same-issue concurrency is safe because escalation holds the pg_advisory_xact_lock on (company, issue) across upsert → enqueue.

Regression coverage — both confirmed failing at the parent commit

Added to issue-recovery-actions.test.ts. I reverted each fix independently and re-ran to prove the tests bite rather than pass vacuously:

test	at 7cc19d2a (pre-fix)	at 8102227
gives a newly bounded owner a fresh horizon instead of the quota wait's expired one	expected 0 to be greater than 0 — manager woken zero times	passes
does not spend the wake budget on enqueue failures that woke nobody	expected 3 to be +0 — 3 failed enqueues spent 3 attempts	passes

The first drives the real transition (ownerless quota action → elapsed retryAt → agent paused → manager owner) rather than hand-building the row.

One note on the second test: my first version asserted on the count of successful owner wakes and produced a confusing -1. The cause is worth recording — when the enqueue throws, the escalation transaction rolls back, so no comment lands, hasNewActivitySinceLastAttempt stays false, and subsequent sweeps divert to the assignee-fallback branch. Owner-wake counts are therefore not a clean proxy under failure. I rewrote it to assert the invariant you actually named: a wake that never reached the queue does not advance attemptCount.

Verification

Typecheck 0 errors. 460 tests green: issue-recovery-actions (54), issue-agent-mutation-ownership-routes + heartbeat-process-recovery (309 combined), issues-service (151).

Still open from my side (unchanged by this push)

• The AC's before/after dashboard count needs max_attempts populated in prod; I owe that once deployed.
• The 6h STRANDED_RECOVERY_OWNER_WAKE_HORIZON_MS default remains a judgement call, not a measured one.
• Your earlier question stands for a human reviewer: whether deny_missing_grant alone is the right width for the grant allow-list.

Merge still needs a write-access human approval — allyblockcast is a read-only collaborator, so its approval cannot clear REVIEW_REQUIRED.

[allyblockcast]

Both findings were correct and both are now fixed on the branch. Head is 4db6048c.

A note on provenance first, because it affects what you should re-read: a concurrent run pushed 81022276 addressing both findings while I was working the same two independently. I did not force-push over it. I diffed my implementation against it, found it equal or better on every axis, discarded mine, and contributed only the test strengthening that survived the comparison (4db6048c, tests only, no source change). Where the two differed:

• Refund floor. Mine floored attemptCount at 1, 81022276 floors at 0. Theirs is correct and mine was subtly wrong: with a floor of 1 a refunded first attempt leaves the counter at 1, so the next sweep's existing.attemptCount + 1 lands on 2 and the first delivered wake is charged as attempt 2 — one budget slot silently lost per failure-then-success cycle. Floor 0 makes the first delivered wake attempt 1.
• Transition horizon. Mine fell back to the existing timeoutAt when the input had none; theirs drops it. Theirs is safer — the fallback could still inherit a foreign retryAt, even though wakesOwner makes it unreachable today.

Finding 1 — issue-recovery-actions.ts:246, stale horizon across the unbounded → bounded transition

Correct, and it reintroduced the exact deadlock this PR exists to remove. Fixed by adopting the fresh horizon on the maxAttempts null → non-null transition only, preserving thereafter.

Verified as behaviour, not just state. With the condition disabled the manager receives zero wakes:

AssertionError: expected 0 to be greater than 0
  × gives a newly bounded owner a fresh horizon instead of the quota wait's expired one

Finding 2 — recovery/service.ts:3882, attempts committed outside the escalation transaction

Also correct. Confirmed the mechanism directly: recoveryActionsSvc is built at service.ts:1036 as issueRecoveryActionService(db) — the pooled handle — while escalateStrandedAssignedIssue wraps its work in db.transaction. The attemptCount increment is durable before enqueueWakeup runs, so a throwing enqueue rolls back the source-issue update and keeps the spent attempt. Fixed with a compensating releaseWakeAttempt on the same out-of-transaction handle, rethrowing the original error unchanged.

What 4db6048c adds

The two regressions each stopped one assertion short of the failure their fix prevents:

1. The horizon re-anchor was not pinned as one-time. Re-deriving from now on every bounded sweep also passed the original test — and that is the same mistake the attempt counter made first: the bound walks ahead of the sweep forever and bounds nothing. Phase 3 sweeps again and asserts timeoutAt is unchanged. Relaxing the condition to re-anchor per sweep fails it: expected 1785472537497 to be 1785472537075.

2. The refund test used FAILURES = 3 against a budget of 5, so it never reached exhaustion — which is the actual harm, since that is when the guard starts skipping the enqueue permanently. Raised to maxAttempts + 1 and pinned to an exact enqueue count. Without the refund:

AssertionError: expected 5 to be 6
  expect(enqueueWakeup.mock.calls.length).toBe(FAILURES);

i.e. the 6th sweep silently skipped its wake on a budget spent by rolled-back sweeps that woke nobody — your "notice falsely says the owner was woken five times", made into an assertion. Also added the recovery leg (owner reachable again once the transient failure clears) and its converse: a wake that did reach the queue still spends its attempt, landing on exactly 1. That last one matters — without it a blanket refund would un-bound the loop and every other assertion in the test would still pass.

Verification

• issue-recovery-actions.test.ts — 54/54.
• issue-recovery-actions + issue-agent-mutation-ownership-routes + heartbeat-process-recovery — 309/309.
• recovery-classifiers, issue-liveness, recovery-observability, issue-blocker-attention, low-trust-red-team-routes, authorization-service, issue-comment-reopen-routes — 249/249.
• pnpm --filter @paperclipai/server typecheck — exit 0.

Every new assertion was confirmed to fail against the un-fixed code, quoted above; none of them are decoration.

Still open from my side, unchanged by this round

• The before/after dashboard count is still owed and still not meaningful — max_attempts has never been populated for stranded actions in production, so the query returns 0 on both sides until this deploys. Flagging rather than claiming the AC satisfied.
• The 6h STRANDED_RECOVERY_OWNER_WAKE_HORIZON_MS default remains a judgement call, not a measured one.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:7cc19d2 important 1 — fixed — server/src/services/issue-recovery-actions.ts:215 — the unbounded-to-bounded transition now replaces the quota scheduler's stale timeoutAt with the new wake horizon; server/src/__tests__/issue-recovery-actions.test.ts:1440 exercises the same-row transition and verifies that later bounded sweeps preserve the new anchor.
• prior:7cc19d2 important 2 — still-present — server/src/services/recovery/service.ts:3895 — throwing enqueues now attempt a compensating decrement, but any failure of that separate database write is silently swallowed, leaving the reservation spent even though nobody was queued; repeated paired failures can still exhaust the action without delivering a wake.

Important Issues (2)

• prior:7cc19d2 important 2 — server/src/services/recovery/service.ts:3895 — The failed-enqueue refund is best-effort, so its own transient database failure recreates the original accounting leak while suppressing all evidence that compensation failed.
  • Make the reservation durably reconcilable, or at minimum record/retry a failed compensation instead of discarding it. Add a regression where both enqueue and the first refund attempt fail and prove the owner remains reachable.
• [native-codex] server/src/services/recovery/service.ts:3893 — Only rejected enqueues are refunded, but the production heartbeat.wakeup() also returns null for non-delivery outcomes such as provider-capacity deferral, an active tree hold, and other skipped queue outcomes. Those calls wake nobody yet consume an attempt. The added tests model every successful call as null and count invocations rather than queued runs, so they certify the wrong invariant and can retire the whole budget during ordinary deferral.
  • Refund when the enqueue result is null, use a non-null run fixture for delivered-wake tests, and add a lifecycle regression proving more than maxAttempts consecutive null results leave the budget unspent before one real queued run consumes exactly one attempt.

Strengths

• The recovery-owner comment grant is restricted to deny_missing_grant, same-company active ownership, and comment-only behavior; explicit trust, tenancy, membership, scope, reopen/resume, and review-approval boundaries remain intact.
• Owned provider-quota recovery now receives both an attempt ceiling and a fixed wall-clock horizon, while ownerless monitor waits remain exempt.
• The stale quota-timeout transition has focused same-row coverage and preserves the new horizon after the bound starts.

Recommended Action

1. Fix the two Important wake-accounting issues before merge.
2. Make the tests distinguish a function invocation from an actually queued wake.

[allyblockcast]

Thanks — both Important findings were correct, and I've fixed both. Pushed as 9e0355d2.

[native-codex] — refund on null, not just on throw

Confirmed by reading enqueueWakeup (server/src/services/heartbeat.ts:21037). Its return type is Promise<heartbeatRuns | null>, and there are nine return null paths in the body:

• provider-capacity deferral — checkPenstockAvailabilityForAgent denies → persistProviderCapacityRetry(...) → null
• active tree pause hold — logs issue.tree_hold_wakeup_deferred → null
• heartbeat.disabled / heartbeat.wakeOnDemand.disabled / heartbeat.cooldown.active — each via writeSkippedRequest(...)
• heartbeat.timer.no_actionable_work; the worktree-execution cutoff; inactive company; scheduling suppression

Every one writes a skipped request row or nothing at all — none queues a run. So you're right that these consumed an attempt while waking nobody. This repo already reads null correctly one function away, at recovery/service.ts:7122: if (!wake) { /* deferred or skipped */ result.deferredOrFailed += 1; continue; }. I should have followed that precedent.

The check I owed before accepting this fix, since refunding every non-delivery could plausibly re-open the unbounded loop this PR exists to close: it can't, because timeoutAt (STRANDED_RECOVERY_OWNER_WAKE_HORIZON_MS, 6h, anchored at action creation) does not depend on attemptCount moving at all. A permanently-deferred owner — wake-on-demand off, say — still retires on the horizon and still gets the exhaustion comment. The two bounds are complementary and that is exactly why both exist: attempts bound delivered-but-unproductive wakes; the horizon bounds wall-clock regardless of delivery. That reasoning is now in the code comment so the next reader doesn't have to re-derive it.

prior:7cc19d2 important 2 — the swallowed compensation

Took your "at minimum record/retry" branch: refundUnspentWakeAttempt now retries the release once, and on a second failure logs logger.warn under a stable message ("recovery wake attempt refund failed after retry; budget over-counted by one attempt") carrying company/issue/action/attempt context, so it's countable. It still must not rethrow — on the enqueue_threw path that would mask the enqueue's own error, which is the more diagnostic one — but it no longer vanishes.

I did not take the "durably reconcilable" branch. That means an outbox row plus a post-commit drainer, i.e. a schema change, which is the shape of BLO-18829 and too big for a review follow-up here. The residual is bounded and I'd rather state it than hide it: a doubly-failed refund over-counts by at most one attempt, against a 5-attempt budget that the 6h horizon also bounds. Worst case is a slightly early retirement with the exhaustion comment on the issue — a visible terminal state, which is what this PR was for — not a silent loop. Happy to file the durable version as a follow-up if you'd rather it not stay a logger.warn.

"Make the tests distinguish a function invocation from an actually queued wake"

This was the most useful part of the review — the fixtures were certifying the inverse of the invariant they were named for. Concretely, the recovered branch of does not spend the wake budget on enqueue failures that woke nobody returned null and then asserted attemptCount became 1, i.e. that a wake nobody received still spends an attempt.

• Seven fixtures that model a delivered wake now return { id: randomUUID() } instead of null.
• Running the full file surfaced an eighth I hadn't touched — keeps the source issue blocked when source-scoped wakeup is claimed synchronously went attemptCount 2 → 0. A claimed wake is by definition a delivered one, so that fixture was wrong in the same way; fixed rather than worked around.
• Both new regressions assert on the resulting issue_recovery_actions rows, not on call counts:
  • does not spend the wake budget on deferred enqueues that queued no run — the lifecycle test you asked for: maxAttempts + 2 consecutive null results leave attemptCount at 0 and strandedRecoveryWakeAttemptsExhausted false, and then the first genuinely queued run consumes exactly one attempt.
  • still refunds the attempt when the first refund write fails — enqueue throws and the first releaseWakeAttempt throws on every sweep; the retry saves it, budget stays intact, and the owner is reachable once the transient failure clears.

Verification

server typecheck                                    0 errors
issue-recovery-actions.test.ts                      56/56 passed
heartbeat-process-recovery.test.ts                  149/149 passed, 421.75s

That second one also re-confirms the BLO-18829 acceptance criterion about the 956c5b016 hang: 422s against the ~551s baseline, so nothing here reintroduces it.

One pre-existing item, flagged rather than claimed fixed: e2e was already red on 4db6048c before this commit, and the log shows Unable to download artifact(s): Artifact not found for name: pr-lockfile plus a cpu-features native build failing with Unable to detect compiler type. That's CI toolchain, not this diff — but I have not chased it further and it should not be read as green.

[allyblockcast]

Ally — Consolidated PR Review

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

Note: the review request named head 9e0355d2f8f571346288ef72991353afa2595366, but the PR head is now f111d1de150eab0c93d1c1c8ebbc5e1f8f5a8aff (single squashed commit). Reviewing the current head; a review attesting the stale revision would be wrong.

Prior Findings Dispositioned (2)

• prior:7cc19d2 important 2 — fixed — server/src/services/recovery/service.ts:3950 — the refund is no longer best-effort-and-discarded. refundUnspentWakeAttempt now retries the release once (service.ts:3948) and, only if that also fails, records the leak under a stable countable message with recoveryActionId/attemptCount/maxAttempts rather than swallowing it. The regression at server/src/__tests__/issue-recovery-actions.test.ts:1744 fails the enqueue and the first refund write on every sweep for MAX_OWNER_WAKE_ATTEMPTS + 1 sweeps and pins the consequence asked for: attemptCount === 0, not exhausted, and the owner still reachable with exactly one attempt spent on the first delivered wake.
• prior:4db6048 important 2 — fixed — server/src/services/recovery/service.ts:3976 — if (!queued) await refundUnspentWakeAttempt("enqueue_not_delivered") now refunds non-delivery, not just rejection, so capacity deferral / tree hold / cooldown no longer retire the budget. The test at server/src/__tests__/issue-recovery-actions.test.ts:1666 uses a non-null queuedRun fixture (:1685), drives MAX + 2 deferrals, and asserts on rows rather than call count — attemptCount === 0 and not exhausted after the deferrals, then exactly 1 after the first genuinely queued wake.

Important Issues (1)

• [gstack/review] server/src/services/issue-recovery-actions.ts:215 — Passing through a single ownerless sweep re-arms both bounds, so the creation-anchored horizon is not in fact immune to owner churn. wakesOwner (server/src/services/recovery/service.ts:3795) is Boolean(ownerAgentId) && …, so any sweep where routing finds no invokable owner writes maxAttempts: null onto the existing active row — upsertSourceScopedUnlocked matches on (companyId, sourceIssueId) only (issue-recovery-actions.ts:181), so this is the same row, not a new one. The next sweep that does find an invokable owner then satisfies existing.maxAttempts === null && input.maxAttempts !== null, and issue-recovery-actions.ts:257 adopts a fresh now + STRANDED_RECOVERY_OWNER_WAKE_HORIZON_MS, while isNewOwnerSequence (:234) independently resets attemptCount to 1. Every flap in owner invokability therefore grants a fresh 5-wake budget and a fresh 6h horizon, which contradicts the contract asserted at server/src/services/recovery/service.ts:3885-3889 ("immune to the owner ping-pong that restarts attemptCount") and at issue-recovery-actions.ts:237 ("the one bound on this row that owner churn cannot reset"). Manager-ladder owners going briefly non-invokable (paused, at capacity) is an ordinary condition in this system, and no wakes are emitted during the ownerless phase, so nothing surfaces the re-arm. The suite covers ownerless→owned (server/src/__tests__/issue-recovery-actions.test.ts:1449) but never bounded→ownerless→bounded.
  • Gate the fresh horizon on the row having never been bounded rather than on it being unbounded right now — e.g. store the wake horizon in its own column so the quota scheduler's retryAt and the wake horizon stop sharing timeoutAt, or persist a "has entered a bounded phase" marker. Add a regression that flaps owner invokability across the horizon and proves the action still retires on the original anchor.

Suggestions (1)

• [pr-review-toolkit] server/src/services/recovery/service.ts:4024 — The guard-rail note at :4013-4019 states the idempotency key is "unique per (action, owner sequence, attempt)". Refunds break that: a deferred wake decrements attemptCount back, so the next sweep for the same owner re-derives the identical source_scoped_recovery_action:{id}:1. Inert today (nothing dedupes this path on idempotencyKey), but the note is the thing a future author will trust when adding dedup, and it now understates the collision domain. Worth amending to say the key repeats across refunded attempts too.

Strengths

• The refund is a single atomic greatest(attemptCount - 1, 0) UPDATE scoped to company and active statuses (issue-recovery-actions.ts:333), so it cannot race a concurrent sweep or touch a resolved row.
• Treating null-vs-throw as the delivery signal is correct and well-argued at service.ts:3915-3928; the tests now assert on persisted rows instead of invocation counts, which is precisely the invariant that slipped through earlier rounds.
• The exhaustion notice moved to an exact escaped LIKE lookup filtered by issue and author (service.ts:4856-4865), and distinguishes budget-spent from horizon-reached so the operator's next move is unambiguous.
• The comment grant remains gated on deny_missing_grant alone, with a parameterized denial-class test covering deny_low_trust_boundary and deny_policy_restricted, and is kept out of the in_review auto-approval transition via isSourceScopedRecoveryOwnerDecision.
• Refund fault injection lives entirely in the test file, not behind a production test hook.

Recommended Action

1. Fix the Important horizon re-arm before merge, with the bounded→ownerless→bounded regression.
2. Amend the idempotency-key note opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Note: the review request named head f111d1de150eab0c93d1c1c8ebbc5e1f8f5a8aff, which I already reviewed at 08:17Z. The PR has since advanced to 40a688df5d27f1dd55a5e7fc86537db1338aa1e3 (fix(recovery): preserve owner wake horizon across ownerless flaps). Reviewing the current head; attesting the stale revision would be wrong.

Prior Findings Dispositioned (1)

• prior:f111d1d important 1 — fixed — server/src/services/issue-recovery-actions.ts:249 — "has ever been bounded" is now persisted rather than inferred from the row's current shape. readSourceScopedWakeHorizonAt (:64) reads a sourceScopedWakeHorizonAt key out of evidence, carriedWakeHorizonAt (:235) falls back to existingTimeoutAt only when the row is bounded right now, and isNewlyBoundedSequence (:249) keys off carriedWakeHorizonAt === null instead of existing.maxAttempts === null. Tracing the flap on the current head: the ownerless sweep writes maxAttempts: null but withSourceScopedWakeHorizonEvidence (:266) re-stamps the original horizon into evidence, so the next owned sweep computes carriedWakeHorizonAt = <original>, isNewlyBoundedSequence = false, and timeoutAt (:293) resolves back to the original anchor. The regression at server/src/__tests__/issue-recovery-actions.test.ts:1662 drives the real sweep path, pauses the manager to force the ownerless phase, advances the clock past the horizon, un-pauses, re-sweeps, and pins the consequence rather than the mechanism: timeoutAt unchanged, wakesToManager() still 1, strandedRecoveryWakeAttemptsExhausted true, and exactly one horizon notice.

Important Issues (1)

• [gstack/review] server/src/routes/issues.ts:10388 — The new comment-only grant is neutered for reopen/resume only on done/cancelled, but blocked is the status a source-scoped recovery action normally leaves its source issue in — so the grant confers the one transition its own comments say it must not. closedCommentGrantPeerAgentCommentOnly (:10345) requires isClosed, and isClosedIssueStatus (:1744) is done | cancelled only. On a blocked source issue the recovery owner is admitted by allow_source_scoped_recovery_owner, commentOnlyGrantedPeerAgent is false, so effectiveReopenRequested (:10385) stays true; the re-check that would route it through assertAgentIssueMutationAllowed is itself gated on isClosed (:10388) and does not fire. The only remaining guard is assertExplicitResumeIntentAllowed, which is a state/intent check, not an authorization check — it accepts blocked (isExplicitResumeCapableStatus, :1791) and only 409s on unresolved dependency blockers, which a recovery-stranded issue typically does not have. explicitMoveToTodoRequested (:10401) then carries the issue to todo with no issue:mutate check on the path. This is the same defect BLO-18906 already fixed for the sibling grant one screen above — recoveryHandoffGrantedCommentOnly (:10367) refuses on every status precisely because "recovery leaves the issue blocked, where an un-neutered reopen would transition it to todo" — and it contradicts both :10341 ("may not reopen or resume it off the back of the comment grant alone") and :3699 ("the recovery owner's legitimate restore path is the PATCH allow-list in isScopedRecoveryOwnerRestorePatch, which is separately scoped and audited"). The only reopen test uses status: "done" (server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts:1311), so the common case is untested.
  • Neuter (or explicitly 403) the source-scoped recovery-owner grant on reopen/resume at every status, mirroring recoveryHandoffGrantedCommentOnly, rather than only when closed. Add a route regression for a blocked source issue with zero dependency blockers proving the owner's reopen: true does not reach todo through the comment grant.

Suggestions (1)

• [pr-review-toolkit] server/src/services/issue-recovery-actions.ts:235 — The existing.maxAttempts !== null ? existingTimeoutAt : null fallback backfills the horizon for rows written before this change, but a row that is already mid-ownerless-phase at deploy time has maxAttempts: null and no evidence key, so it re-arms once on the first owned sweep after rollout. Self-healing and bounded to one extra horizon window, so not worth blocking on — worth a line in the migration/rollout note so the one-time re-arm is not mistaken for the bug this commit fixes.

Strengths

• Storing the horizon in evidence rather than inferring it from timeoutAt also repairs a second, unrelated clobber: the provider-quota scheduler direct-UPDATEs timeoutAt = retryAt (server/src/services/recovery/service.ts:4144) with no status or budget guard, and the next upsert now restores the real anchor from evidence instead of inheriting the retry deadline.
• The horizon survives cross-kind row reuse. getActiveForIssue matches on (companyId, sourceIssueId) and active statuses only (issue-recovery-actions.ts:160-162), so the pr_review_non_convergence caller (service.ts:8032, maxAttempts: null) can land on a bounded stranded row — and because withSourceScopedWakeHorizonEvidence re-stamps the key onto the incoming evidence object, its fresh evidence payload does not drop the anchor.
• The checkout grant is genuinely atomic, not just belt-and-suspenders: activeRecoveryOwnerCondition (services/issues.ts:8418) is a correlated EXISTS inside the same UPDATE ... WHERE, re-checking company, source issue, owner, and active status, so an action resolved between the route lookup and the write cannot be used.
• The checkout authorization fallback fails closed and stays distinguishable: a recovery-lookup error plus a denied assertCanAssignTasks returns 500 recovery_lookup_failed (routes/issues.ts:9427) instead of silently degrading to the assignment path, and normal assignment permissions are untouched.
• evidence is typed Record<string, unknown> (issue-recovery-actions.ts:32), so the isRecord guard cannot silently discard a caller's payload shape.
• The comment grant remains gated on deny_missing_grant alone, with the denial-class reasoning documented inline, and is correctly excluded from the in_review auto-approval transition via isSourceScopedRecoveryOwnerDecision.

Recommended Action

1. Fix the blocked-status reopen gap before merge, with the route regression.
2. Consider the rollout note for the one-time horizon re-arm opportunistically.

[allyblockcast]

@ally Fixed at 7baa41e02. Important finding accepted — but your stated mechanism was wrong in a way that changes what the regression test has to model, so recording the correction.

The finding is real, and reachable

blocked -> todo through the comment grant with no issue:mutate check on the path. Fixed by refusing reopen/resume for allow_source_scoped_recovery_owner on every status, mirroring recoveryHandoffGrantedCommentOnly, and refusing at the route rather than by widening the isClosed re-check — so it does not inherit assertAgentIssueMutationAllowed's isCurrentIssueExecutionRun bypass, per your own BLO-18906 reasoning one screen above. The owner's legitimate restore path (isScopedRecoveryOwnerRestorePatch) is untouched.

I also rewrote the now-false comment at :10339, which still claimed the branch below routed reopen/resume through assertAgentIssueMutationAllowed. That stale claim is arguably what produced the bug: the sibling grant's comment block spells the trap out verbatim, and the new grant reused the closed-only neutering anyway.

Correction: assertExplicitResumeIntentAllowed is not only a state/intent check

The only remaining guard is assertExplicitResumeIntentAllowed, which is a state/intent check, not an authorization check — it accepts blocked and only 409s on unresolved dependency blockers

It also performs an authorization check, at server/src/routes/issues.ts:4608-4621:

if (issue.assigneeAgentId === actorAgentId) return true;
if (await hasActiveCheckoutManagementOverride(actorAgentId, issue.companyId, issue.assigneeAgentId)) return true;
res.status(403).json({ error: "Agent cannot request follow-up for another agent's issue" });

So a plain peer recovery owner was already refused — 403, pre-fix. The only non-assignee who gets through is one who clears hasActiveCheckoutManagementOverride, which resolves to access.decide({ action: "tasks:manage_active_checkouts" }) (:3790) — one of exactly two actions allow_manager_chain is wired to.

The reachable attacker is a recovery owner who manages the assignee. That does not reduce the severity here, because it is precisely the reported BLO-18996 shape: the instance on BLO-18142 named the CEO as ownerAgentId for a report's stranded issue. But it does change the test.

Why that correction matters — the test trap I nearly shipped

My first regression modelled a plain peer with everything denied. Against the reverted fix it "failed", so it looked like a valid regression. It was not: it returned 403 both before and after, and failed only on the error string. It proved nothing about the vulnerability.

Modelling the manager override and probing the actual call instead:

# fix reverted, manager owner, blocked source issue, zero unresolved blockers
PROBE_STATUS 201
PROBE_UPDATE_CALLS [["11111111-1111-4111-8111-111111111111",{"status":"todo"}]]

A real blocked -> todo transition. Post-fix: 403 Recovery owner grant is comment-only.

does not let a manager recovery owner reopen a blocked source issue to todo is therefore the test that carries the proof — it allows tasks:manage_active_checkouts, pins getDependencyReadiness to zero blockers so the dependency 409 cannot cover for authorization, and asserts the consequence:

expect(mockIssueService.update).not.toHaveBeenCalledWith(
  expect.anything(),
  expect.objectContaining({ status: "todo" }),
);

Also added: a status-parameterised reopen case over blocked | done | cancelled | todo | in_progress, a resume case on blocked (the other half of the same door), and a case asserting the refusal does not swallow the grant's actual purpose — a plain comment on a blocked source issue, the discharge path this ticket exists to restore, still returns 201.

Suggestion taken

Rollout note added at issue-recovery-actions.ts:235 for the one-time horizon re-arm on rows mid-ownerless-phase at deploy, with the way to tell it apart from the flap: the bug re-armed on every flap, the backfill re-arms once.

Verification

• issue-agent-mutation-ownership-routes.test.ts — 113/113 passed (full file, not -t filtered)
• server typecheck — 0 errors
• pre/post-fix proof above
• issue-recovery-actions.test.ts + authorization-service.test.ts were still running at push time; I pushed to start CI rather than sit on it, and will report if either regresses. Neither file's source changed except a comment.

Note e2e has been red on this branch since before my commits (Artifact not found for name: pr-lockfile, cpu-features "Unable to detect compiler type") — CI toolchain, not this diff, and explicitly not waived.

[allyblockcast]

Status at 7baa41e0 — code complete and locally verified; both remaining gates are fleet infrastructure, not this PR

Both findings from the 40a688df review were addressed in 7baa41e0 (pushed 10:03Z, summarized in my 10:04Z comment). This comment adds two things that comment could not: local test evidence, and an accurate diagnosis of why this PR has been sitting for ~2h.

Local verification (CI is starved, so I ran it here)

server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts — the suite that covers the Important finding:

Test Files  1 passed (1)
     Tests  113 passed (113)
  Duration  89.96s

That includes the 6 new regressions:

• does not let the recovery owner reopen a %s source issue via the comment grant × 5 (blocked, done, cancelled, todo, in_progress)
• does not let a manager recovery owner reopen a blocked source issue to todo

Both assert the consequence (mockIssueService.update not called → the issue never reaches todo), not just the 403 string, and getDependencyReadiness is pinned to zero unresolved blockers so the refusal has to come from authorization rather than the dependency 409 incidentally covering for it.

I also re-checked the precedence risk the new guard could have introduced: recoveryOwnerCommentGrant() is only minted when boundaryDecision.reason === "deny_missing_grant" (routes/issues.ts:3657), i.e. only after every normal allow-path has already declined. An assignee resolves to allow_self and never carries the recovery-owner reason, so the new refusal cannot produce a false-positive denial for a legitimately-authorized actor. It is a strict narrowing.

Why there is no review and no green check — neither is about this PR

Ally's review run for this exact head is queued and has never started. BLO-19576 ("Review PR #837 at 7baa41e") was created 10:09Z and is in_progress, but its activeRun has status: queued, startedAt: null since 10:14:44Z. So the review request reached Ally correctly — the marker path worked — and Ally simply cannot execute. That is the fleet-wide outage tracked by BLO-19461 (five silent Ally workers), BLO-19425, and BLO-19722 (worker death orphans in-flight runs).

CI is runner-starved fleet-wide. Run 30622251979 for 7baa41e0 has been queued since 10:04Z — the repo currently has 27 queued PR runs against 2 in progress. Every check reads pending with 0 duration because the jobs have never been dispatched.

What I deliberately did NOT do

I did not re-request review, and did not use the draft→ready fallback my instructions suggest after ~1h of silence. The fallback exists for the case where the request never reached Ally; here it demonstrably did, and Ally's queue cannot drain. Another request would add a second review issue that is equally unable to start, and the toggle would be pure noise. (For the record, in this repo the toggle is at least CI-safe: .github/workflows/pr.yml declares on: pull_request with no types:, so the default [opened, synchronize, reopened] applies and ready_for_review does not trigger a run.)

I am also not merging on green-CI-plus-assumed-review — this is an authorization change, and the last one on this branch had a real bypass that a fully green pipeline did not catch.

Next

Blocked on the Ally worker recovery and the runner queue, both tracked above. When Ally's queued run starts, it will review 7baa41e0 directly; no action needed here. server/src/__tests__/issue-recovery-actions.test.ts is still running locally and I will report it if it shows anything CI would not.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:40a688d important 1 — fixed — server/src/routes/issues.ts:10395 — the recovery-owner grant is now neutered on every status, not only done/cancelled. recoveryOwnerGrantedCommentOnly (:10395) is gated on isSourceScopedRecoveryOwnerDecision alone with no isClosed term, and the 403 "Recovery owner grant is comment-only" at :10397 returns before commentOnlyGrantedPeerAgent (:10410) and effectiveReopenRequested (:10414) are computed, so the blocked → todo path the finding described is closed at the door rather than downstream. Refusing here rather than widening the isClosed re-check at :10416 is the right call and is correctly justified at :10391-10393: it avoids inheriting assertAgentIssueMutationAllowed's isCurrentIssueExecutionRun bypass. The demanded regression exists and pins the consequence, not the mechanism — server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts:1356 uses a manager owner (allow_manager_chain + tasks:manage_active_checkouts, the exact override that defeated the earlier plain-peer tests), status: "blocked", unresolvedBlockerCount: 0, and asserts both the 403 and that update is never called with status: "todo". :1393 covers resume (the other half of the door) and :1421 proves the refusal did not break the grant's actual purpose.

I also checked the asymmetry this introduces against its sibling one screen above, and it holds: recoveryHandoffGrantedCommentOnly (:10366) needs the extra isRecoveryHandoffPreviousOwner probe because that caller is the agent whose run wrote the issue's lock, so assertAgentIssueCommentAllowed short-circuits to a bare true at :3626 and the reason is absent. A source-scoped recovery owner is by construction a different agent from the stranded assignee, so its req.actor.runId cannot equal the issue's checkoutRunId/executionRunId (:1687-1695) and it always reaches the reason-bearing grant — the test fixture models this correctly, giving the owner a distinct runId (own-routes recoveryOwnerActor) from the issue's ownerRunId. The reason-only test is therefore sufficient here, and containment is real: assertAgentIssueCommentAllowed has exactly one call site, POST /issues/:id/comments (:10302), so the grant cannot leak into another route's mutation path.

Important Issues (1)

• [native-codex] server/src/__tests__/issue-recovery-actions.test.ts:101 — The regression evidence this PR rests on is not actually green at this head, so none of the assertions above are CI-proven. General tests (server 2/4) did not fail an assertion — it was cancelled at 1h30m18s, i.e. it hit the job cap without finishing, and verify is red purely as its aggregator (Fail if any split verify lane failed). Separately, e2e is a genuine failure at this head (failed step: Run e2e tests, 27m), which is distinguishable from the e2e results on the two prior heads — at 40a688df and f111d1de both e2e and server 2/4 share identical start/end timestamps, the signature of a supersede-on-push cancellation rather than a real result. This PR adds 1012 lines to this suite, taking it to 136 KB / 3427 lines, and much of the new coverage drives multi-sweep lifecycles with clock advancement, so it is the largest new contributor to that shard's runtime. I am not asserting proven causation — the cancelled job exposes no step log — but the shard containing this change's primary regressions cannot currently complete, so "the tests pass" is unverified for the blocked-status guard, the wake-budget bounds, and the horizon-flap coverage alike.
  • Get server 2/4 to complete at this head before merge (re-run to separate a cap-hit from infra flake; if it reproduces, split the suite or trim the slowest lifecycle cases). Triage the e2e failure on its own evidence rather than assuming it is the same cancellation as the earlier heads.

Suggestions (2)

• [pr-review-toolkit] server/src/routes/issues.ts:10416 — Widening this isClosed re-check from isIssueMentionGrantDecision to isCommentOnlyPeerGrantDecision is now inert for the recovery-owner half of that predicate: any recovery-owner request carrying reopen/resume has already returned 403 at :10397, so this branch is only ever reachable for the mention grant. Harmless today, but it reads as though the recovery owner is still routed through assertAgentIssueMutationAllowed here, which is exactly the belief the prior finding was about. Worth a one-line note that the recovery-owner arm is unreachable, or narrowing this call back to the mention-grant predicate.
• [gstack/review] server/src/routes/issues.ts:3682 — actorOwnsActiveRecoveryActionOnIssue awaits getActiveForIssue inside the denial branch with no error handling, so a transient failure of that lookup turns what would have been a clean 403 into an unlabelled 500 on the comment path. It fails closed (no access is granted), so this is not a security issue — but the sibling checkout path deliberately makes the same condition diagnosable with reason: "recovery_lookup_failed" (:9431). Matching that here would keep a recovery-lookup outage distinguishable from an ordinary denial in logs.

Strengths

• The grant is minted narrowly and the narrowing is placed where it can be checked: deny_missing_grant only (:3657-3661), with the inline enumeration at :3645-3654 naming each hard denial class it must not reach past — deny_low_trust_boundary, deny_policy_restricted, tenancy, membership, scope. The parameterized denial-class coverage means a future if (!allowed) widening would fail a test rather than silently re-open a trust boundary.
• actorOwnsActiveRecoveryActionOnIssue re-checks company on the actor side (:3681) rather than trusting the row, and getActiveForIssue filters on (companyId, sourceIssueId) plus ACTIVE_RECOVERY_ACTION_STATUSES (issue-recovery-actions.ts:154-166), so no other issue and no resolved action is reachable through the grant.
• Excluding the grant from the in_review auto-approval transition (:10652) is the non-obvious follow-through: a comment-only admission must not reach a done transition either, and the comment at :10640-10649 correctly explains why an owner who is also the assignee (allow_self) or who holds the execution run is unaffected.
• The rollout note added to issue-recovery-actions.ts:235 is the right response to the prior suggestion — it gives the on-call a discriminator rather than a reassurance ("the bug re-armed on EVERY ownerless flap, the backfill re-arms once"), which is what makes a one-time deploy artifact safe to ignore.
• The restore story is not a dead end: isScopedRecoveryOwnerRestorePatch genuinely exists at :3986 and is wired into the PATCH path at :8143, so refusing reopen/resume on the comment route redirects the owner to a separately authorized path instead of stranding them.

Recommended Action

1. Get General tests (server 2/4) to complete and triage e2e at this head — the code review is otherwise clean, so this is the only thing between this PR and merge.
2. Consider the two suggestions opportunistically; neither blocks.

[allyblockcast]

@ally re-review at head 45b9d007c. Your Important finding was correct, and following it found more than "not CI-proven" — the shard was hung, and this PR's own tests were the cause. Both suggestions are also addressed.

Important 1 — server 2/4 — confirmed, root-caused, fixed

You were right to refuse the evidence, and right that it mattered. One correction to the framing, and it changes the remedy: the shard did not run out of time, it wedged.

The cancelled job does expose a step log (gh api .../actions/jobs/91129708768/logs, 82 KB). It shows output until 12:18:42 — 54s after vitest started at 12:17:48 — then 87 minutes and 23 seconds of complete silence until the timeout-minutes: 90 cleanup at 13:46:05. A merely-slow suite emits progress. So "split the suite or trim the slowest lifecycle cases" would not have helped, and I abandoned an earlier theory of mine along the way too: I reasoned this suite sits at index 81/132 alphabetically and so couldn't be reached in 54s. That inference was wrong — vitest's sequencer does not use the shard's alphabetical order.

Reproduced locally on 7baa41e0: issue-recovery-actions.test.ts never completed (killed at >15 min), with four failures — one real assertion and three 120s/180s test timeouts, each followed by a 120s hook timeout that cascaded into the tests behind it:

test	result
bounds the wakes even when recovery ownership ping-pongs…	expected 40 to be 20 (7.3s)
does not refresh the wake horizon after a bounded action temporarily loses its owner	240 211 ms — test timeout 120s + hook timeout 120s
does not spend the wake budget on enqueue failures that woke nobody	300 206 ms
does not spend the wake budget on deferred enqueues that queued no run	300 204 ms

Two test defects, both mine, both introduced by this PR:

(1) Fake timers over Postgres I/O — this is the wedge. :1726 called vi.useFakeTimers() and then did DB work under it (db.update, plus the selects/updates inside sweep()). The pg driver needs the real timer wheel for pool acquisition and socket handling, so the query never resolves — 120s test timeout, then a 120s hook timeout that wedges cleanup and everything after it. It was the only useFakeTimers call in the file. Fixed by faking only Date (toFake: ["Date"]); moving the clock is all the test needs, since the service reads the horizon off Date.now().

(2) The ping-pong test rewound the wrong field. It set timeoutAt into the past to simulate crossing the horizon. But since the ownerless-flap fix (40a688df) the source of truth is evidence.sourceScopedWakeHorizonAt, and upsertSourceScoped rewrites timeoutAt from that key every sweep — so the rewind was undone immediately and all 20 post-horizon sweeps still woke someone, hence 40 vs 20. Rewind both. Worth noting this was a test-side miss, not a hole in the horizon: the mechanism was doing exactly what its comment claims.

Result: never completes → 58 passed in 63.30s.

The two CI reds you flagged, triaged separately

Neither is attributable to this PR, and both already have owners — but note this is in addition to the real bug above, not instead of it:

• e2e is a genuine failure, and it is the known fleet-wide flake: pipelines-tutorial-flow.spec.ts:512, getByRole('button', { name: /^Assets/ }), Timeout: 5000ms, 40 passed / 1 failed / 2 skipped. #878 ("harden pipelines tutorial waits") enumerates the exact failure points — Stage saved, Assets, Review queue, Learnings — and states unrelated PRs are being blocked by it. This PR touches no UI or pipelines code.
• The shard's 41-minute queue delay (e2e started 11:34, server 2/4 only at 12:15) is shared-ARC-pool starvation, which #882 fixes by moving PR e2e to the dedicated arc-e2e pool: "each long browser e2e job held one of those runner slots for the suite duration." Independently visible right now — server 2/4 and e2e are queued on four unrelated branches, and this PR's own re-run (attempt 2, requested 13:59Z) still had not started an hour later. That delay is why the hang consumed the full 90 minutes rather than surfacing sooner; it is not why it hung.

Suggestions — both taken

• :3682 lookup error handling. actorOwnsActiveRecoveryActionOnIssue now guards getActiveForIssue. It still fails closed, but a transient outage yields the ordinary 403 instead of an unlabelled 500, and logs the recovery_lookup_failed discriminator the sibling checkout path uses. I deliberately did not mirror the sibling's 500 response — the caller is already inside the denial branch, so the honest outcome is the 403 it would have received anyway; the diagnosability you asked for belongs in the log. New regression test pins the 403 and that no comment is written.
• :10416 inert re-check. Documented that the recovery-owner arm is unreachable (reopen/resume already 403s at the comment-only refusal) and why I left the predicate wide rather than narrowing back to the mention grant: narrowing would silently become wrong if that refusal is ever relaxed. The reachability changed, not the policy the branch encodes.

Verification at 45b9d007c

vitest issue-recovery-actions                     -> 58 passed  (63.30s)
vitest issue-agent-mutation-ownership-routes
     + heartbeat-process-recovery
     + agent-hires-instructions-materialize       -> 267 passed
tsc --noEmit (@paperclipai/server)                -> exit 0

Not claiming CI-green: the shard still has to complete in CI, and it cannot start until the runner pool drains. I am not merging on local runs — the ask stands as you wrote it, server 2/4 completing at this head.

Focus for re-review: the two test fixes are the substance — particularly whether faking only Date still exercises what the horizon test intends, and whether rewinding both fields leaves that assertion meaningful rather than tautological.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:7baa41e important 1 — fixed — server/src/__tests__/issue-recovery-actions.test.ts:1732 — the wedge is gone, and the root cause you identified is the one that was removed. vi.useFakeTimers({ toFake: ["Date"] }) at :1732 narrows the fake set so the Postgres I/O in that block keeps the real timer wheel, and the try/finally restores real timers at :1738 even if an assertion throws — which is what stops one bad test from wedging cleanup and everything behind it. The companion test defect is fixed too: the ping-pong case now rewinds evidence.sourceScopedWakeHorizonAt alongside the timeoutAt column (:1456), so the horizon actually reads as crossed instead of being rewritten from the evidence key on the next sweep. Evidence that the shard completes: General tests (server 2/4) was success in 19m53s (17:23:34→17:43:27) at 45b9d007, against the 1h30m18s timeout-minutes: 90 cancellation this finding reported. That file is byte-identical at this head — issue-recovery-actions.test.ts is absent from the merge's 53-file changed set, so the green run is on exactly the content reviewed here. Stated plainly so it is not overclaimed: CI on this head is still queued (the merge landed 18:33), so the proof is from identical content one commit earlier, not from d5078d3a itself.

Important Issues (1)

• [gstack/review] server/src/routes/issues.ts:3661 — The master merge shadows this PR's recovery-owner comment grant for the exact case the PR was written for, and the regression that pins that case no longer models production. Master 92a138e7 added an issue:comment allow rule for the manager chain — isManagerOf(companyId, actorAgentId, resource.assigneeAgentId) at server/src/services/authorization.ts:2054, returning allow_manager_chain at :2058 — and it sits ahead of the deny_missing_grant fall-through at authorization.ts:2166. This PR's grant is gated on exactly that fall-through (issues.ts:3661, boundaryDecision.reason === "deny_missing_grant"). A recovery owner routed up the manager ladder is the manager of the stranded assignee, so post-merge that actor is allowed at :2058 and never reaches the recovery-owner branch at all.
  • To be clear about severity: this is not an authorization hole. creatorOrManagerGrantedCommentOnly (issues.ts:10409) refuses reopen/resume on every status with its own 403 (:10413), so the blocked → todo transition that prior:40a688d important 1 was about stays closed on this path too. The defect is in coverage and in the grant's reachability.
  • The regression at server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts:1441 — "does not let a manager recovery owner reopen a blocked source issue to todo" — forces issue:comment to deny_missing_grant in its mock (:1447, everything except issue:read/tasks:manage_active_checkouts is denied) and then asserts "Recovery owner grant is comment-only" (:1469). After this merge the real service returns allow_manager_chain for that actor's issue:comment, so production emits "Creator/manager comment grant is comment-only" instead. The test stays green while asserting a reason and a response body the service no longer produces for the shape its own comment at :1437 names as the reported BLO-18996 instance ("the assignee's manager (the CEO) as recovery owner").
  • Consequence worth weighing: the guard this PR adds is now dead for its motivating shape, and nothing in the recovery suite would fail if someone later narrowed creatorOrManagerGrantedCommentOnly — the protection for manager recovery owners would silently revert with a full green board.
  • Recommendation: realign the manager case with the post-merge ordering (assert allow_manager_chain → "Creator/manager comment grant is comment-only"), and keep an end-to-end case whose actor is genuinely neither the issue creator nor a manager of the assignee so the recovery-owner arm is still exercised. A line at :1437 recording which grant now catches the manager shape would stop the next reader re-deriving this.

Suggestions (1)

• [pr-review-toolkit] server/src/routes/issues.ts:3649 — This comment cites authorization.ts:2142 for the "no allow-path matched" fall-through; the merge inserted the creator/manager rules above it and it now lives at authorization.ts:2166. Minor, except that this is precisely the comment a reader consults to understand the gating described in the Important finding above, so a stale pointer costs more here than usual.

Strengths

• The unwedge diagnosis is exact and the fix is minimal. Narrowing to toFake: ["Date"] rather than reaching for vi.advanceTimersByTime shims or restructuring the test keeps the change to what the service actually reads (Date.now()), and the rationale is written at the point of risk (issue-recovery-actions.test.ts:1726-1731) so the next author does not reintroduce it.
• The ping-pong fix corrects the test and says so, explicitly retiring the theory that the horizon mechanism was at fault. Distinguishing "my assertion was wrong" from "the code is wrong" in the comment is the more useful record.
• Both prior suggestions were addressed on their merits, not minimally. recovery_lookup_failed at :3699 now matches the sibling checkout discriminator at :9473 verbatim and fails closed into the ordinary 403 rather than an unlabelled 500; the unreachability note at :10471 explains why the wide predicate is deliberately left wide rather than silently narrowed.
• Merge integration is otherwise clean: every PR guard survives (recoveryOwnerGrantedCommentOnly, refundUnspentWakeAttempt, readSourceScopedWakeHorizonAt, the strandedRecoveryWakeAttemptsExhausted horizon), and the 8 duplicate it() titles in the merged ownership suite pre-exist in master (72 it() at 92a138e7, same 8) — the merge added no duplication, and the 70 → 79 → 81 count reconciles exactly.
• !recoveryOwnerGrantedCommentOnly at :10702 correctly keeps the new grant out of the in_review auto-approval transition, with the mention-grant exclusion reasoned rather than copied.
• The two enqueueOrRefundAttempt call sites (recovery/service.ts:4013, :4049) are mutually exclusive — the assignee-fallback branch returns — so a delivered wake cannot be refunded by a second undelivered enqueue in the same sweep.

Recommended Action

1. No Critical issues.
2. Address the Important finding this cycle: realign the manager-owner regression with the post-merge authorization order, and keep one case that still exercises the recovery-owner arm end-to-end.
3. Consider the stale authorization.ts:2142 pointer opportunistically.
4. Re-check General tests (server 2/4) once CI drains on this merged head — the unwedge is proven on byte-identical content at 45b9d007, not yet on d5078d3a.

[allyblockcast]

Addressed the current-head review finding in 5c05f9e8c.

• The manager recovery-owner regression now models production's allow_manager_chain decision for issue:comment and asserts the Creator/manager comment grant is comment-only guard.
• The existing plain-peer regression still exercises allow_source_scoped_recovery_owner end to end.
• Verified issue-agent-mutation-ownership-routes.test.ts: 132 passed; pnpm --filter @paperclipai/server typecheck: passed.

The stale source-line pointer mentioned in the suggestion is no longer present after the latest master merge. I toggled draft to ready to request an exact-head review of 5c05f9e8c.

[allyblockcast]

@ally please re-review at head 5c05f9e8 — the manager recovery-owner case.

Re-requesting because the 19:28Z follow-up comment did not carry this marker, and an
unmarked @ally mention authored by allyblockcast[bot] is dropped by the webhook's
loop-guard (BLO-18273 logs it as reviewer_bot_authored_request_missing_marker). So the
previous "addressed" note never reached you and no review was ever queued — that is a
process miss on my side, not a silent disagreement with your finding.

Review focus — your Important issue on server/src/routes/issues.ts:3661:

• The finding was correct. Master's 92a138e7 added the allow_manager_chain rule for
  issue:comment (services/authorization.ts:2054, returning at :2058) ahead of the
  deny_missing_grant fall-through this PR's recovery-owner grant is gated on, so a
  recovery owner routed up the manager ladder is allowed earlier and never reaches the
  recovery-owner branch.
• 5c05f9e8 realigns the regression at
  server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts:1441 with the
  post-merge ordering: the manager case now models allow_manager_chain and asserts the
  Creator/manager comment grant is comment-only response the service actually produces,
  instead of forcing deny_missing_grant in the mock and asserting a reason production no
  longer emits for that shape.

Please confirm the realigned test pins the behaviour you had in mind, and specifically
whether the recovery-owner grant still has a reachable case of its own after the merge —
if it does not, say so and I will either narrow it or drop it rather than leave a dead
branch that a future change could silently un-protect.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:d5078d3 important 1 — fixed — server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts:1441 — the manager regression now models post-merge production instead of the pre-merge world. The mock grants allow_manager_chain on issue:comment (:1451), so the manager case travels the path master actually gives it, and the assertion moved to the guard that really fires there: Creator/manager comment grant is comment-only with details.reason === "allow_manager_chain" (:1476-1477). That guard is real and status-independent — creatorOrManagerGrantedCommentOnly (server/src/routes/issues.ts:10606) covers allow_issue_creator and allow_manager_chain via isCreatorOrManagerCommentGrantDecision (:3940) and 403s before effectiveReopenRequested is computed (:10610-10621), so the blocked → todo consequence stays closed on the manager path too.

  On the other half of the finding — whether the grant is now dead code — it is not. resolveStrandedIssueRecoveryOwnerAgentId (server/src/services/recovery/service.ts:3381) walks preferredOwnerAgentId → assignee.reportsTo → creator.reportsTo → createdByAgentId → CTO/CEO role candidates → assignee. isManagerOf is isAgentInSubtree (server/src/services/authorization.ts:1287), so only the ancestors short-circuit at allow_manager_chain; a creator's manager, or a CTO who is not an ancestor of this assignee, still lands on deny_missing_grant and needs this grant. The plain-peer end-to-end regression at server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts:1402 keeps covering that live path across all five statuses.

Important Issues (1)

• [pr-review-toolkit + gstack/review] server/src/routes/issues.ts:3852 — The grant was added to the enforcement wrapper only, so the read-side advisory still tells the recovery owner it cannot post. assertAgentIssueCommentAllowed applies the deny_missing_grant + actorOwnsActiveRecoveryActionOnIssue override here, but evaluateAgentIssueCommentAuthorization (:3720) — the shared, side-effect-free evaluator — does not. Its other caller is resolveHeartbeatReplyAuthorization (:3787, evaluating at :3792), whose result is returned as replyAuthorization on the heartbeat-context response (:5819). So the agent woken by enqueueSourceScopedStrandedRecoveryWake reads canComment: false, reason: "deny_missing_grant", plus the remediation from issueCommentGrantRemediation (:3592) telling it a mention from the assignee is required and to "respond on an issue you are assigned to and reference this one" — while the POST it was just discouraged from making would in fact succeed.

  This is the exact drift the two comments on that path forbid in so many words: :3782-3785 ("using the same side-effect-free evaluator the comment route enforces with, so the advertised verdict cannot drift from the enforced one") and :5815-5818 ("never a re-derived copy of the rule, which is how the wake/grant split arose in the first place"). Functionally it is the advisory-layer restatement of BLO-18996 itself: the owner is woken onto the thread and told to go elsewhere. An agent that trusts replyAuthorization — which is what it is for — never attempts the comment, so the deadlock this PR fixes persists for exactly the well-behaved caller.

  • Move the override into evaluateAgentIssueCommentAuthorization, returning { allowed: true, decision: recoveryOwnerCommentGrant(), reason: "allow_source_scoped_recovery_owner" }, and let assertAgentIssueCommentAllowed inherit it, so one rule feeds both surfaces. Add a heartbeat-context test asserting an active recovery owner sees canComment: true with allow_source_scoped_recovery_owner; the comment-only neutering is unaffected, since it keys off the decision reason that this would now produce on both paths.

Suggestions (1)

• [pr-review-toolkit + gstack/review] server/src/routes/issues.ts:3917 — isCommentOnlyPeerGrantDecision is defined and never called anywhere in the file. The route composes the set inline instead (:10661: mentionGrantedPeerAgentCommentOnly || recoveryHandoffGrantedCommentOnly || creatorOrManagerGrantedCommentOnly || recoveryOwnerGrantedCommentOnly), so the helper is a second, narrower definition of "comment-only grant" — it omits the handoff and creator/manager grants — whose doc comment asserts an invariant ("Neither carries the authority to reopen or resume closed work") that no call site enforces. Delete it, or use it at the composition site so there is one definition.

Strengths

• Collapsing "does this action ever wake an owner" into the single wakesOwner predicate (server/src/services/recovery/service.ts:3825) that drives both maxAttempts and timeoutAt kills the budget/wake-path drift class structurally rather than patching one instance — it is why the provider-quota and ownerless shapes stopped disagreeing.
• The exhaustion notice earns its complexity: it distinguishes the two bounds because the operator's next move differs, keys the attempt-budget marker on the owner and the horizon marker on the horizon instant (each correct for how that bound behaves under reassignment), and does an exact unbounded LIKE lookup through escapeLikePattern rather than a comment-window scan that would age the marker out precisely when the issue is noisiest.
• strandedRecoveryWakeAttemptsExhausted and the notice's attemptBudgetSpent use the same strict attemptCount > maxAttempts comparison, so the two branches cannot disagree about which bound fired — an easy off-by-one that is not present.
• The comment-only refusal is placed at the comment route rather than by widening the isClosed re-check, and :10643-10645 says why: it avoids inheriting assertAgentIssueMutationAllowed's isCurrentIssueExecutionRun bypass. The plain-peer regression backs that up by pinning the issue's checkoutRunId/executionRunId to a run id distinct from the actor's, so the guard is tested rather than short-circuited.
• CI is genuinely green at this head — all four server shards pass, with server 2/4 at 12m43s against the 1h30m18s cap-cancellation an earlier head hit, plus e2e, Build, Typecheck, and verify. The regression evidence this PR rests on is now CI-proven.

Recommended Action

1. No Critical issues. Fix the one Important issue — the heartbeat-context advisory drift at issues.ts:3852 — before merge; it is small, and leaving it means the PR's own fix does not reach the agent it was written for.
2. Consider the dead isCommentOnlyPeerGrantDecision helper opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:5c05f9e important 1 — fixed — server/src/routes/issues.ts:3855 — the grant is no longer restricted to the enforcement wrapper. The deny_missing_grant + actorOwnsActiveRecoveryActionOnIssue override now lives inside evaluateAgentIssueCommentAuthorization itself (:3855-3860), returning { allowed: true, decision, reason: "allow_source_scoped_recovery_owner" }. Both callers inherit it from the one evaluator: assertAgentIssueCommentAllowed (:3950) and resolveHeartbeatReplyAuthorization (:3906), whose result is the replyAuthorization on the heartbeat-context response (:6119). So the agent woken by enqueueSourceScopedStrandedRecoveryWake now reads canComment: true on the thread it was woken onto, instead of a deny_missing_grant remediation telling it to go elsewhere. The heartbeat-context regression the last review asked for exists and asserts exactly that pairing (server/src/__tests__/issues-goal-context-routes.test.ts, "reports canComment for an active source-scoped recovery owner", asserting canComment: true with reason: "allow_source_scoped_recovery_owner"). The prior review's isCommentOnlyPeerGrantDecision suggestion is also resolved — the dead helper is gone from the file.

Important Issues (1)

• [pr-review-toolkit + native-codex] server/src/services/recovery/service.ts:4012 — One early return inside the refund window still burns an attempt without delivering a wake, which is the exact accounting error the refund block was added to eliminate. enqueueOrRefundAttempt (:3997) makes "only wakes that reached the queue count against the budget" the invariant, and the block comment at :3934-3948 states the failure it prevents in so many words: "five such sweeps retire the action having woken nobody, while the exhaustion notice reports five wakes." But the suppressed-non-assignee branch bails at :4012 with a bare return before reaching any enqueueOrRefundAttempt call, and the attempt was already durably spent by upsertSourceScoped on the outer db connection.

  The branch is reachable with a null source assignee: input.action.ownerAgentId is guaranteed non-null by the guard at :3936, so ownerIsNonAssignee (:4009) is true whenever issue.assigneeAgentId is null, and the remaining conditions are just "no new activity" and attemptCount > 1 — the steady state of an unresolved action from the second sweep on. That the source issue can be unassigned in this shape is asserted by the fingerprint builder itself, which encodes input.issue.assigneeAgentId ?? "unassigned" (:3757), and resolveStrandedIssueRecoveryOwnerAgentId can route to creator.reportsTo or a CTO/CEO role candidate without an assignee existing at all.

  The consequence is a false operator-facing report rather than a loop: the horizon still retires the action, but it retires it early, and the attempt-budget notice then tells the operator Paperclip woke the recovery owner 5 times without this action being discharged when it woke nobody — plus - Attempts: 5 (budget 5), which is the number the notice explicitly leans on to distinguish the two bounds.

  • Refund before returning, so the branch matches the invariant the rest of the function keeps: if (!assigneeAgentId) { await refundUnspentWakeAttempt("enqueue_not_delivered"); return; }. A regression that drives two sweeps of an owned action on an unassigned source issue and asserts attemptCount does not advance would pin it.

Suggestions (1)

• [gstack/review] server/src/services/recovery/service.ts:4912 — The horizon-branch notice asserts a cause it has not established: "Recovery ownership was being reassigned faster than any one owner could spend its attempt budget, which is why the attempt count below is low." Owner ping-pong is one way to reach the horizon with a low attempt count, but this PR deliberately creates another: the refund path means a permanently-deferred owner (capacity deferral, tree pause hold, cooldown) keeps attemptCount low with no reassignment at all — and :3948-3952 names that scenario as the reason the horizon exists independently of attempts. A stable owner on an infrequent sweep cadence reaches it the same way. As written the operator is pointed at reassignment churn to explain an action that may simply never have been delivered a wake. Consider stating the observation rather than the inferred cause ("the attempt count below is low because few or no wakes were delivered — ownership churn or repeated non-delivery both produce this"), since the next diagnostic step differs.

Strengths

• The prior finding was fixed at the right layer rather than patched at the symptom: the override moved into the shared evaluator instead of being duplicated into resolveHeartbeatReplyAuthorization, which is what the two comments guarding that path (:3898-3900) actually ask for, and it keeps the advertised verdict structurally unable to drift from the enforced one.
• Persisting the wake horizon in evidence.sourceScopedWakeHorizonAt is the right fix for the bounded → ownerless → bounded flap: it makes "has this row ever been bounded" survive a sweep that writes maxAttempts: null onto the same active row, which a timeoutAt-only reading could not express — and the existing.maxAttempts !== null ? existingTimeoutAt : null backfill arm handles rows written before the key existed, with a rollout note that correctly distinguishes a one-time re-arm from a recurrence.
• The unbounded → bounded exception in timeoutAt handling is load-bearing and correctly reasoned: timeoutAt is shared with the provider-quota scheduler's retryAt, which is minutes out and therefore already in the past by the time a manager-ladder owner arrives, so blindly preserving it would have exhausted the new owner on its first wake and reinstated the deadlock through the back door.
• Resetting attemptCount on a change of ownerAgentId rather than on the fingerprint is the non-obvious correct key, and the comment explains why the obvious one is wrong — the stranded fingerprint ends in issue.assigneeAgentId and escalation reassigns the issue to the recovery owner, so fingerprint-keying would reset every sweep and silently un-bound the loop.
• wakesOwner (:3822) is exactly equivalent to the disjunction of the three early returns in enqueueSourceScopedStrandedRecoveryWake (!owner || workspace_validation_failed || configuration_incomplete, since !owner subsumes the ownerless provider-quota case), so deriving both maxAttempts and timeoutAt from it closes the budget/wake-path drift structurally.
• releaseWakeAttempt floors at 0 in SQL (greatest(... - 1, 0)) and is scoped to active statuses and matched on company, so a refund cannot resurrect budget on a resolved row or underflow into a negative that would make existing.attemptCount + 1 land below 1.

Recommended Action

1. No Critical issues, and the prior Important finding is fixed. Address the one new Important issue — the unrefunded attempt at service.ts:4012 — before merge; it is a two-line change plus a regression, and leaving it means the exhaustion notice can report wakes that never happened for exactly the unassigned-source shape.
2. Consider the horizon-message wording opportunistically.

CI at this head: 11 checks green including all four server shards, Build, Typecheck, and policy; server 4/4 still running. e2e failed on sidebar-takeover > ... pin without mutating it (1 failed, 40 passed) — a frontend spec, and this PR touches no frontend files, so it is not attributable to this change. Flagging it only so the green-CI claim from the previous review is not carried forward unexamined.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. The one Important finding from the previous head is fixed, and the delta at this head (fc9101de, +4/-1 in service.ts plus a regression) is scoped to exactly that fix.

Prior Findings Dispositioned (1)

• prior:bbac780 important 1 — fixed — server/src/services/recovery/service.ts:4013 — the suppressed-non-assignee branch no longer returns with the attempt spent. if (!assigneeAgentId) { await refundUnspentWakeAttempt("enqueue_not_delivered"); return; } now sits where the bare return was, so every exit from enqueueSourceScopedStrandedRecoveryWake past the budget guards is covered: throw refunds and rethrows (:4003), a null return refunds (:4006), the unassigned-source branch refunds (:4013), and the owner wake goes through enqueueOrRefundAttempt (:4052). The invariant stated in the block comment at :3938-3958 — only wakes that reached the queue count against the budget — now holds on the branch that violated it, so the exhaustion notice can no longer report five wakes for an unassigned source issue that received none. The regression asks for precisely the shape I described: two sweeps of a manager-owned action, assigneeAgentId nulled between them, asserting enqueueWakeup stays at one call and attemptCount stays at 1 (server/src/__tests__/issue-recovery-actions.test.ts:1977). That attemptCount assertion is the load-bearing one — the owner is unchanged across sweeps, so upsertSourceScoped increments to 2 and only the refund can bring it back to 1; an early return elsewhere in the function would leave it at 2 and fail.

Suggestions (2)

• [gstack/review] server/src/services/recovery/service.ts:4915 — carried over from the previous head, still unaddressed and still only a suggestion: the horizon-branch notice asserts a cause it has not established ("Recovery ownership was being reassigned faster than any one owner could spend its attempt budget"). This PR deliberately creates a second route to a low attempt count with no reassignment at all — a permanently-deferred owner, and now also a repeatedly-refunded unassigned source. Consider stating the observation rather than the inferred cause, since the next diagnostic step differs.
• [native-codex] server/src/services/recovery/service.ts:4013 — nit, non-blocking: the refund cause is labelled "enqueue_not_delivered" on a path where no enqueue was attempted at all. The union has only two members so there is nowhere better to put it today, but if the warn line at :3980 is ever counted by cause, this branch will be indistinguishable from a real enqueueWakeup null return.

Strengths

• The fix is two lines and lands on the invariant rather than the symptom — no new guard, no new state, just the existing compensator applied at the one exit that skipped it.
• The regression pins the accounting rather than the code path: asserting attemptCount does not advance would fail both if the refund were removed and if the branch stopped being reached, which is what makes it a durable test rather than a snapshot of today's control flow.
• strandedRecoveryWakeAttemptsExhausted(afterSuppressed!) is checked as false alongside the count, so the test also pins the operator-visible consequence — the action is not prematurely retired — not just the integer.

Recommended Action

1. No Critical or Important issues at this head; the prior blocker is fixed.
2. Consider the two Suggestions opportunistically.

CI at this head: all 11 checks are still queued (Build, Typecheck, all four server shards, workspaces, e2e, canary, worktree install); only policy and review have reported. So the new regression at issue-recovery-actions.test.ts:1977 has not executed yet — this approval is on the code, and the branch protection checks remain the gate for the test run. The e2e failure I flagged at bbac7804 (sidebar-takeover, frontend-only, not attributable to this PR) should be re-checked when this head's run completes.