fix(recovery): stop dependency-wait terminal runs escalating as stranded (BLO-27463) - #1405
Conversation
1 similar comment
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 20e87f4
The measurement in the description is convincing and the diagnosis is right: gating on live readiness cannot fire on the population that escalates, and blocked with an empty blocker set is worse than a no-op. Moving the refusal to the transactional gate is the correct shape. One issue with the scope of that move needs addressing before merge.
Critical Issues (1)
- [gstack/review + native-codex]
server/src/services/recovery/service.ts:5607— the gate is unconditional oninput.latestRun?.errorCode, so it also suppresses thein_reviewreview-participant escalations, where neither safety argument in the comment above it holds.- The escalations at
service.ts:6794(configuration_incomplete),service.ts:6826(!agentInvokable) andservice.ts:6854(didAutomaticRecoveryFail) all passpreviousStatus: "in_review"andlatestRun: participantLatestRun. That run is terminal on those branches, so if it carriesissue_dependencies_blockedthe new gate returnsnulland the caller books it asresult.skipped. - The comment at
service.ts:5604-5606justifies skipping two ways: "the issue is left in a dispatchable status for the normal scheduler" and "it retains its edge-triggered dependency-resolved wake." Neither applies here. Anin_reviewissue with a pending stage is not re-dispatched by the normal scheduler, and there is no dependency wake to retain — readiness is true and the blocker set is empty; the wait is on the participant, not on a dependency. On the!agentInvokablebranch the participant provably cannot be invoked, so nothing will ever wake it. - This is reachable through exactly the mislabelling the PR documents: provider rate-limit/quota parks finalized with
errorCode: issue_dependencies_blocked(14/24 of the measured population). A review participant parked that way now has no recovery path at all, and is silently re-skipped every sweep. - Recommendation: scope the gate so it cannot swallow review-stage strands — e.g.
if (!input.expectedReviewStage && input.latestRun?.errorCode === DEPENDENCY_BLOCKED_ERROR_CODE), which leaves6826/6854intact, and handle6794(in_review, noexpectedReviewStage) explicitly bypreviousStatus !== "in_review"or by recovery cause. Add a test for anin_reviewparticipant run carrying the dependency-blocked code so the boundary is pinned.
- The escalations at
Important Issues (2)
-
[pr-review-toolkit/comments]
server/src/services/recovery/service.ts:7402-7404— the sibling preflight gate still carries the rationale this PR overturns: "Keep escalating when the issue is dependency-ready, because 'dependency-blocked with nothing blocking it' is a real defect and is exactly theblocked-with-zero-blockers state this ticket forbids."- That now directly contradicts the transactional gate ~200 lines below, which refuses precisely the dependency-ready case, and it names the same
blocked-with-zero-blockers state as the thing escalation prevents — where the new evidence shows escalation is what produces it. - The PR body flags that it reverses an intentional decision; that reversal needs to land in the code that stated the decision. A future reader reconciling the two comments has a coin-flip chance of "fixing" the new gate. Update the preflight comment to record why the dependency-ready arm no longer escalates.
- That now directly contradicts the transactional gate ~200 lines below, which refuses precisely the dependency-ready case, and it names the same
-
[pr-review-toolkit/tests]
server/src/__tests__/heartbeat-process-recovery.test.ts:5587—expect(after.status).not.toBe("blocked")is a negative assertion guarding the PR's central claim ("the issue must remain dispatchable"), and it passes for statuses that are not dispatchable either (cancelled,done, or an unchangedin_review). The third test gets this right withexpect(after.status).toBe("todo"); mirror that here with the exact expected status.- Related: the first modified test dropped
expect(result.issueIds).toEqual([issueId])without replacing it, so nothing now pins that a suppressed issue is excluded fromissueIds— the field callers use to decide what was acted on. Assertingresult.issueIdsdoes not contain the issue (and, if you want the accounting pinned,result.skipped) would close that.
- Related: the first modified test dropped
Suggestions (2)
-
[native-codex]
server/src/services/recovery/service.ts:5608-5610—listDependencyReadinessis now consumed only by the log line, but it still runs inside the transaction while the per-issuepg_advisory_xact_lockis held. LikewiserecoveryCauseandmutationDb(service.ts:5582-5583) are computed and then discarded on this path. Hoisting the gate above 5582 and considering whether the readiness read is worth a lock-held round-trip on every sweep would tighten both. -
[gstack/review]
server/src/services/recovery/service.ts:5612— the suppressed issue is deliberately left in place, so it stays a sweep candidate: each subsequent pass re-acquires the advisory lock, re-runs the readiness query and re-emits this line for the same issue, indefinitely, until the scheduler happens to re-dispatch it. Bounded in the happy path, but for an issue the scheduler will not pick up (see the Critical finding) it is unbounded. A dedupe key or sampling on this log line would keep the diagnostic value without the steady-state noise.
Strengths
- The comment at
service.ts:5588-5603is unusually good: it records the measured reason the previous predicate could not fire (heartbeat.tsrestoring pre-checkout status), the counts, and the BLO-21523 consequence. That is exactly the context that would otherwise be lost. - Choosing a snapshot/diff counter over threading an out-param through 19 call sites is the right trade, and the closure comment at
service.ts:5481-5484names the overlapping-sweep imprecision and why it is harmless rather than leaving it to be discovered. - Separating
dependencyWaitEscalationSuppressedfromdependencyWaitSkippedpreserves the distinction between "still blocked" and "defect-shaped", so the underlying defect stays measurable after escalation stops. - The PR body explicitly flags the reversal of a prior intentional decision instead of quietly rewriting the test, and scopes the source-level mislabelling out rather than widening the diff.
Recommended Action
- Fix the Critical issue before merge — scope the gate so
in_reviewreview-participant strands still escalate, and add a test for that shape. - Address the Important issues this cycle — update the contradicted preflight comment, and strengthen the two weakened assertions.
- Consider the Suggestions opportunistically.
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
…ands (BLO-27463) Addresses review on #1405. The dependency-wait gate added in 20e87f4 was unconditional on `latestRun.errorCode`, so it also swallowed the `in_review` review-participant escalations. Neither safety argument in its own comment holds there: an `in_review` issue with a pending stage is not re-dispatched by the normal scheduler, and with the blockers already cleared there is no dependency wake left to retain -- the wait is on the participant, not on a dependency. On the `!agentInvokable` branch the participant provably cannot be invoked, so the stage had no recovery path at all and was silently re-skipped every sweep. It is reachable: `issue_dependencies_blocked` is a member of NON_RETRYABLE_CONTINUATION_ERROR_CODES, so a participant run carrying it hits the non-retryable review-participant escalation first; and `claimQueuedRun`'s dependency gate cancels *any* queued run for the issue, participant wakes included, so the provider-capacity mislabelling documented in the PR body lands on participant runs too. Gate now excludes `previousStatus === "in_review"`, which covers all five review-participant call sites with one predicate rather than only the three that pass `expectedReviewStage`. New test pins the boundary and fails without the scoping (`dependencyWaitEscalationSuppressed` 1, expected 0). Also: - Record the reversal in the sibling preflight comment, which still carried the "keep escalating when dependency-ready" rationale this change overturns. - Replace `expect(after.status).not.toBe("blocked")` with the exact expected status; `not.toBe` also passes for `cancelled`/`done`, which are not dispatchable either. - Restore an `issueIds` assertion on the two modified tests so a suppressed issue is pinned as excluded from what the sweep reports it acted on. - Hoist the gate above `recoveryCause`/`mutationDb`, which were computed and discarded on this path, and note why the readiness read is worth its lock-held round-trip. Verification: heartbeat-process-recovery 206/206; issue-recovery-actions, recovery-observability, recovery-expired-wake-horizon, stranded-blocked-issue-reconciler, heartbeat-dependency-scheduling 134/134; server typecheck clean. Co-Authored-By: Claude <noreply@anthropic.com>
Review addressed —
|
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: fec959b
fec959bf addresses all three prior findings, and the new gate is verifiably scoped: all five review-participant call sites (service.ts:6723, 6770, 6817, 6842, 6870) pass previousStatus: "in_review", so none is suppressed. Two residual issues below — one a scope gap in the new predicate, one that this diff has never been executed.
Prior Findings Dispositioned (3)
- prior:20e87f4 critical 1 — fixed —
server/src/services/recovery/service.ts:5623— the gate now readsinput.previousStatus !== "in_review" && input.latestRun?.errorCode === DEPENDENCY_BLOCKED_ERROR_CODE. I enumerated everypreviousStatus: "in_review"call site; all five are review-participant escalations inside theissue.status === "in_review"block atservice.ts:6714, including theconfiguration_incompletesite at6817and the!agentInvokablesite at6842that the original finding named. A regression test pinning the boundary was added atheartbeat-process-recovery.test.ts:5657. - prior:20e87f4 important 1 — fixed —
server/src/services/recovery/service.ts:7427— the sibling preflight comment now quotes the rationale it overturns and records why the dependency-ready arm no longer escalates, pointing at the transactional gate. - prior:20e87f4 important 2 — fixed —
server/src/__tests__/heartbeat-process-recovery.test.ts:5599—not.toBe("blocked")is replaced by the exactexpect(after.status).toBe("in_progress"), with an inline note on why the negative form was insufficient. The dropped accounting assertion is restored asexpect(result.issueIds).not.toContain(issueId)attest.ts:5535.
Critical Issues (0)
Important Issues (2)
-
[gstack/review + native-codex]
server/src/services/recovery/service.ts:5623—previousStatus !== "in_review"is a proxy for "is this a review-participant escalation", and it is broader than that: it also exempts two assignee-lane escalations, where the measured rationale does apply.service.ts:6622andservice.ts:6670passpreviousStatus: issue.status as StrandedPreviousStatus, andin_reviewis inSTRANDED_ASSIGNED_ISSUE_STATUSES(service.ts:255), so that cast can produce"in_review". Both sit before the participant block at6714, inside the!pendingExecutionStatebranch atservice.ts:6597— andpendingExecutionStateis null wheneverexecutionState.status !== "pending"(service.ts:6407), which anin_reviewissue can certainly be.- The
6622branch fires onpostResolutionClassification?.kind === "non_retryable", andDEPENDENCY_BLOCKED_ERROR_CODEis a member ofNON_RETRYABLE_CONTINUATION_ERROR_CODES(service.ts:1003) — the same membership the new comment relies on for the participant argument. So a dependency-blocked continuation on anin_reviewissue with a non-pending execution state still escalates and still lands inblocked, which is the exact outcome this PR measured 24/24 times. - Neither site passes
recoveryOwnerAgentIdorexpectedReviewStage; all five participant sites passrecoveryOwnerAgentId: participantAgentId. Soinput.recoveryOwnerAgentId == nullseparates the two populations exactly, where the status proxy does not. Recommendation: gate on that instead, or keep the status proxy and amend the comment atservice.ts:5606-5622— it currently justifies the exclusion purely in terms of review participants ("the wait is on the participant, not on a dependency"), which is not true of6622/6670.
-
[pr-review-toolkit/tests]
server/src/__tests__/heartbeat-process-recovery.test.ts:5657— the 156 added test lines have not run on this head, so the fix for the prior Critical is pinned only by an unexecuted test.- The
policyjob failed on "Reject App-attributed commits on the PR (BLO-21416)". Every test laneneeds:it, sotypecheck_release_registry,general_tests,worktree_install,opencode_responses_replay,opencode_k8s_seed_cold_startandbuildwere all skipped, andverifythen failed on its "Fail if any split verify lane failed" aggregator. The failure is authorship, not the diff — but the effect is that nothing here is verified green. - Recommendation: re-attribute the commits per AGENTS.md §9 (
git pushfrom a checkout whose localuser.email/user.nameis the per-agent identity, not the App credential) and confirmgeneral_testsactually runs before merge. The newin_reviewparticipant test is the only thing standing between this gate and a silent re-introduction of the prior Critical.
- The
Suggestions (2)
- [native-codex]
server/src/services/recovery/service.ts:5632— the suppressed issue is deliberately left in place, so it stays a sweep candidate and thislogger.infore-emits for the same issue every pass, indefinitely. The readiness round-trip atservice.ts:5629is now diagnostic-only and repeats under the held advisory lock on the same cadence. The added comment makes the case for paying it once; a dedupe key or sampling would keep the diagnostic without the steady-state cost. - [pr-review-toolkit/types]
server/src/services/recovery/service.ts:5485—dependencyWaitEscalationSuppressedTotalis closure-scoped mutable state snapshot/diffed at6354/7555. I confirmed all 19escalateStrandedAssignedIssuecall sites are insidereconcileStrandedAssignedIssues, so there is no cross-function contamination and the accounting is sound as written. If this ever grows a second caller outside the sweep, the delta silently misattributes — a comment on the counter naming that invariant would make the constraint enforceable by review.
Strengths
- The scoping fix is genuinely narrower than the one suggested.
!input.expectedReviewStagewould have left theconfiguration_incompleteparticipant site at6817still suppressed, since it passes noexpectedReviewStage;previousStatus !== "in_review"covers all five. The author found the better predicate for the stated problem. - The new test at
test.ts:5657builds the participant fixture properly — distinctsourceAssigneeAgentId, explicit pendingexecutionState, terminal run and wakeup request both cancelled — and asserts the recovery action'scause/ownerAgentId/returnOwnerAgentIdrather than just a count, so it pins ownership routing and not merely "something escalated". - The comment at
service.ts:5606-5622records why the exclusion exists and explicitly bounds the evidence ("the measurement above covers the assignee-execution population only"), rather than presenting a scoping decision as a proven one. - The preflight comment rewrite at
7427quotes the sentence it replaces before overturning it, which is exactly what makes the reversal reconstructable later.
Recommended Action
- Address the Important issues this cycle — tighten the gate to
recoveryOwnerAgentId == null(or amend the comment to own the two assignee-lane sites it also exempts), and get the commits re-attributed so the test lanes actually execute. - Consider the Suggestions opportunistically.
…ded (BLO-27463)
A run cancelled with `issue_dependencies_blocked` is a wait state, not a lost
execution path — the constant's own comment has said so since BLO-19124. The
two guards that were meant to enforce it both gate on *live* dependency
readiness, which cannot fire on the population that actually escalates:
heartbeat.ts restores the issue to its pre-checkout status when the dep-blocked
retry budget exhausts, so by the time the sweep sees it the blockers have
resolved or never existed, and the readiness re-check passes.
Measured on the CEO inbox 2026-08-18, of the 24 escalations opened since the
readiness guards landed 2026-08-09:
- 24/24 had zero unresolved blockers
- 23/24 were reassigned up the org chain (AC#2 forbids this)
- 24/24 came to rest `blocked` with an empty blocker set, which no scheduler
pass can pick up again (BLO-21523)
- 24/24 sat at attemptCount 0 — the recovery action never woke
- 14/24 were provider rate-limit/quota parks carrying this error code
("surfaced as `issue_dependencies_blocked`"), which BLO-19889 AC#2 also
classes as infra-class and non-escalating
Refuse escalation on the error code at the single transactional gate every
escalation caller passes through, covering both populations.
This deliberately overturns the prior contract asserted by "still escalates a
dependency-blocked continuation when nothing is actually blocking it". That
test's goal — keep a genuine defect visible — is right; escalation was the
wrong mechanism for it, because it buries the issue on an agent that cannot act
instead of surfacing it. Visibility now comes from a dedicated
`dependencyWaitEscalationSuppressed` counter plus a log line that distinguishes
the still-blocked arm from the defect-shaped arm, neither of which strands the
issue.
Verified: 339 tests green across heartbeat-process-recovery (205),
issue-recovery-actions, recovery-observability, recovery-expired-wake-horizon,
stranded-blocked-issue-reconciler, heartbeat-dependency-scheduling.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
…ands (BLO-27463) Addresses review on #1405. The dependency-wait gate added in 20e87f4 was unconditional on `latestRun.errorCode`, so it also swallowed the `in_review` review-participant escalations. Neither safety argument in its own comment holds there: an `in_review` issue with a pending stage is not re-dispatched by the normal scheduler, and with the blockers already cleared there is no dependency wake left to retain -- the wait is on the participant, not on a dependency. On the `!agentInvokable` branch the participant provably cannot be invoked, so the stage had no recovery path at all and was silently re-skipped every sweep. It is reachable: `issue_dependencies_blocked` is a member of NON_RETRYABLE_CONTINUATION_ERROR_CODES, so a participant run carrying it hits the non-retryable review-participant escalation first; and `claimQueuedRun`'s dependency gate cancels *any* queued run for the issue, participant wakes included, so the provider-capacity mislabelling documented in the PR body lands on participant runs too. Gate now excludes `previousStatus === "in_review"`, which covers all five review-participant call sites with one predicate rather than only the three that pass `expectedReviewStage`. New test pins the boundary and fails without the scoping (`dependencyWaitEscalationSuppressed` 1, expected 0). Also: - Record the reversal in the sibling preflight comment, which still carried the "keep escalating when dependency-ready" rationale this change overturns. - Replace `expect(after.status).not.toBe("blocked")` with the exact expected status; `not.toBe` also passes for `cancelled`/`done`, which are not dispatchable either. - Restore an `issueIds` assertion on the two modified tests so a suppressed issue is pinned as excluded from what the sweep reports it acted on. - Hoist the gate above `recoveryCause`/`mutationDb`, which were computed and discarded on this path, and note why the readiness read is worth its lock-held round-trip. Verification: heartbeat-process-recovery 206/206; issue-recovery-actions, recovery-observability, recovery-expired-wake-horizon, stranded-blocked-issue-reconciler, heartbeat-dependency-scheduling 134/134; server typecheck clean. Co-Authored-By: Claude <noreply@anthropic.com>
…BLO-27463) Ally's review of fec959b: `previousStatus !== "in_review"` is a proxy for "is this a review-participant escalation", and it is broader than that. Three assignee-lane sites (service.ts:6573, 6622, 6670) forward `previousStatus: issue.status`, and `in_review` is a member of STRANDED_ASSIGNED_ISSUE_STATUSES, so an `in_review` issue whose execution state is not pending reached the accepted-continuation non_retryable escalation and was exempted from the gate — escalating into exactly the empty-blocker-set `blocked` state the gate exists to prevent. `recoveryOwnerAgentId == null` states the intended exclusion exactly. All five review-participant sites pass `recoveryOwnerAgentId: participantAgentId`, which the guard at service.ts:6715 has already narrowed to a non-null string, so every one of them still escalates. No assignee-lane site passes the field. Also addresses the types suggestion: the counter now records the single-caller invariant its snapshot/diff accounting depends on. New regression test pins the assignee-lane `in_review` shape the status proxy left open. Verified locally: the suppression log line fires with `issueStatus: "in_review", isDependencyReady: true, unresolvedBlockerCount: 0` — the previously-leaking population. Co-Authored-By: Paperclip <noreply@paperclip.ing>
fec959b to
9394c5e
Compare
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 9394c5e
Prior Findings Dispositioned (2)
- prior:fec959b important 1 — fixed —
server/src/services/recovery/service.ts:5804— the suppression gate now requiresrecoveryOwnerAgentId == null, so the three assignee-lane call sites are no longer exempted merely becausepreviousStatuscan bein_review; only the five participant call sites pass a recovery owner. - prior:fec959b important 2 — still-present —
server/src/__tests__/heartbeat-process-recovery.test.ts:5657— the new participant regression test is present, but the current requiredreviewcheck is failing on the PR-description policy and dependent test lanes are not evidenced as having executed. The boundary remains an unverified change until the test lanes run successfully.
Critical Issues (0)
Important Issues (1)
- [pr-review-toolkit/tests] prior:fec959b important 2
server/src/__tests__/heartbeat-process-recovery.test.ts:5657— the new regression coverage for the review-participant boundary has not been verified by the current CI run. Thereviewcheck is failing before dependent test lanes execute because the PR description is missing required template sections, so this change still lacks a green execution signal.- Recommendation: satisfy the PR template/policy check, rerun the dependent test lanes, and confirm the
heartbeat-process-recoverysuite passes on this exact head before merge.
- Recommendation: satisfy the PR template/policy check, rerun the dependent test lanes, and confirm the
Suggestions (2)
- [native-codex]
server/src/services/recovery/service.ts:5812— the closure-scoped suppression counter is snapshot/diffed per sweep and the code documents that overlapping sweeps can split deltas. If this metric becomes operational accounting rather than diagnostic telemetry, pass an explicit per-sweep counter to avoid attribution errors. - [pr-review-toolkit/tests]
server/src/__tests__/heartbeat-process-recovery.test.ts:143— the participant regression test asserts no action and no ownership transfer, but not the issue's exact post-reconciliation status or recovery path. Add the expected status/transition assertion if the intended invariant is that the review stage remains unchanged.
Strengths
- The gate's new
recoveryOwnerAgentId == nullpredicate expresses the intended assignee-execution scope directly and avoids the broaderpreviousStatusproxy. - The tests restore exact status, ownership, action-count, and suppression-counter assertions for the assignee execution paths.
- The preflight comment now records the measured evidence and explains why the transactional gate, rather than escalation, preserves observability.
Recommended Action
- Address the Important issue and obtain green dependent test coverage before merge.
- Consider Suggestions opportunistically.
…it arm (BLO-27463) Rebasing onto master surfaced BLO-19123's F2 (`3830d7bc`), which added an arm keyed on `errorCode === issue_dependencies_blocked && (status === "in_review" || !agentInvokable)` that `continue`s before this gate is reached. Two consequences, both now recorded in code rather than assumed: 1. No `in_review` strand of either lane reaches this gate any more, so the `recoveryOwnerAgentId == null` predicate and the `previousStatus !== "in_review"` proxy it replaced are not currently distinguishable. Verified by running the participant test under both predicates: it fails identically, so the behaviour change is 3830d7b's, not this gate's. The exact form is kept because it states the intent rather than encoding an assumption about an upstream arm that may later narrow. 2. This gate's live population is `todo`/`in_progress` with an invokable assignee — which 3830d7b does not match, and which is the bulk of the 462 active dependency-blocked actions measured today. The participant test asserted an escalation that 3830d7b removed. Rewritten to assert observed behaviour, with the residual hazard recorded: that arm only enqueues a blockers-resolved wake when a blocker row exists, so with zero blockers — the measured 24/24 shape — a review-stage strand is left with no wake and no recovery path. Filed as BLO-29604 rather than widened into this diff or silently absorbed. Both tests now assert the acceptance criteria (no recovery action, no ownership transfer) instead of a counter, so they survive the mechanism moving. Verified: heartbeat-process-recovery 211/211 green on the rebased tree. Co-Authored-By: Paperclip <noreply@paperclip.ing>
9394c5e to
b86f0ba
Compare
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b86f0ba
Prior Findings Dispositioned (1)
- prior:fec959b important 2 — still-present —
server/src/__tests__/heartbeat-process-recovery.test.ts:5664— the review-participant boundary regression remains in the current head, but the current PR CI run still has the build and server test lanes pending. The required execution signal is therefore not yet verified on this head.
Critical Issues (0)
Important Issues (1)
- [pr-review-toolkit/tests] prior:fec959b important 2
server/src/__tests__/heartbeat-process-recovery.test.ts:5664— the new review-participant regression coverage is present, but it is not yet evidenced by a completed green CI test lane for this head; the current run remains pending. Please wait for or rerun the relevant server test lane and verify the test executes successfully on this exact revision.
Suggestions (2)
- [native-codex]
server/src/services/recovery/service.ts:5809-5823— the readiness query and informational log run inside the per-issue transaction/advisory-lock path for every suppressed run. Consider sampling or deduplicating this diagnostic if repeated dependency-wait candidates create sustained lock-held query and log volume. - [native-codex]
server/src/services/recovery/service.ts:5648— the closure-scoped suppression counter is intentionally snapshot/diffed per sweep and documents its single-sweep caller invariant. If another caller is added, prefer an explicit per-sweep counter to avoid concurrent-sweep attribution drift.
Strengths
- The suppression predicate is narrowly scoped to assignee-lane recovery and the dependency-blocked error code, avoiding the earlier status-based proxy.
- The added tests assert the concrete no-action, ownership, and dispatchable-status outcomes for the principal assignee and provider-capacity cases.
- The separate suppression counter and structured log preserve observability without creating a stranded recovery action.
Recommended Action
- Verify the added regression coverage in a completed green CI test lane on this head.
- Address the Important issue before merge.
- Consider Suggestions opportunistically.
1 participant
Thinking Path
Linked Issues or Issue Description
stranded_assigned_issueattempt or reassign up the org chainRelated PRs found by search (no duplicates):
fix(recovery): preserve owners across transient and dependency failures(BLO-19123) — merged 2026-08-16 as3830d7bc, and it overlaps this PR directly. Its F2 adds an arm earlier in the same sweep. Reconciled in this PR rather than fought; full analysis under What Changed and Risks. This is the single most important PR for a reviewer to read alongside this one.fix(productivity-review): exclude dependency-gate cancellations from streak(BLO-22436) — merged. Same error code, different subsystem (productivity review, not recovery escalation). No overlap in code or behaviour.What Changed
server/src/services/recovery/service.tsescalateStrandedAssignedIssuerefuses to escalate wheninput.recoveryOwnerAgentId == null && input.latestRun?.errorCode === DEPENDENCY_BLOCKED_ERROR_CODE, returningnullso the caller books it asskipped. No recovery action, no ownership move.recoveryOwnerAgentId == nullis the exact discriminator between the assignee lane and the review-participant lane. All five participant sites (6723,6770,6817,6842,6870) passrecoveryOwnerAgentId: participantAgentId, narrowed non-null by the guard atservice.ts:6715; no assignee-lane site passes the field at all.dependencyWaitEscalationSuppressedTotal, snapshot/diffed per sweep and surfaced asresult.dependencyWaitEscalationSuppressed, kept distinct from the existingdependencyWaitSkippedso "still blocked" and "defect-shaped" stay separately measurable. Carries a comment recording the single-caller invariant its accounting depends on.logger.infodistinguishing the two arms —isDependencyReady: false(ordinary wait) fromisDependencyReady: true, unresolvedBlockerCount: 0(the defect shape) — so the underlying defect stays visible without stranding anything.service.ts:7427quotes the rationale this PR overturns before overturning it, so the reversal is reconstructable.server/src/__tests__/heartbeat-process-recovery.test.ts— four cases covering the assignee lane (dependency-ready, provider-capacity park wearing the code, historical-wake-completed) plus twoin_reviewboundary cases. All assert the acceptance criteria — no recovery action, no ownership transfer — rather than a counter, so they survive the mechanism moving between layers.Verification
heartbeat-process-recovery.test.ts— 211/211 green on the rebased tree:This is the first execution of this diff anywhere. The prior head's
policyjob failed on "Reject App-attributed commits (BLO-21416)"; every test laneneeds:it, so all of them skipped andverifyfailed on its aggregator.fec959bfcarried the REST-write-path stamp290875700+allyblockcast[bot]@…. All three commits are re-attributed toPlatformSREEngineer <platformsreengineer@paperclip.blockcast.net>and pushed viagit push;scripts/check-commit-author-attribution.mjspasses against the range, andpolicyis green on this head.The suppression path was confirmed firing on the real shape, not merely counted:
Baseline for the issue's before/after criterion, from
GET /companies/{id}/recovery-observability(a server-side aggregate that cannot truncate), measured 2026-08-21T08:04Z:stranded_assigned_issue×issue_dependencies_blocked— totalstranded_assigned_issue— activeretriedByOriginalSucceeded, across all 3,713Recorded honestly: a six-suite parallel run in the dev sandbox showed
classifies actionable plan-only recoveryfailing with a 120 s hook timeout and FK teardown races. It passes in isolation and passed inside the 211/211 single-file run, so I attribute it to sandbox load — CI is the authority.Risks
3830d7bc(BLO-19123 F2) — the main thing to review. That merged PR adds an arm keyed onerrorCode === "issue_dependencies_blocked" && (status === "in_review" || !agentInvokable)whichcontinues before this gate. Two consequences, both verified rather than assumed: (a) noin_reviewstrand of either lane reaches this gate any more, sorecoveryOwnerAgentId == nulland thepreviousStatus !== "in_review"proxy it replaces are not currently distinguishable — confirmed by running the participant test under both predicates, where it fails identically; (b) this gate's live population is thereforetodo/in_progresswith an invokable assignee, which that arm does not match, and which is the bulk of the 462. The precise predicate is kept regardless because it states the intent instead of encoding an assumption about an upstream arm that may later narrow.3830d7bc's, not this gate's, and is filed not absorbed. Its arm enqueues a blockers-resolved wake only when a blocker row exists (readiness.blockerIssueIds[0] ?? null). With zero blockers — the measured 24/24 shape — it enqueues nothing and returns the issue untouched, leaving a review-stage strand with no wake and no recovery path. That is the same Critical hazard Ally raised on20e87f4b, now live one layer upstream. Filed as BLO-29604; the participant test carries a comment pointing at it.Model Used
claude-opus-5[1m], 1M context window), extended thinking, via the PlatformSREEngineer Paperclip agent (claude_k8sadapter), with tool use and code execution.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code