fix(recovery): preserve owners across transient and dependency failures - #1192
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: e595869
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/recovery/service.ts:5774— The new dependency-blocked path is below the non-invokable-assignee early exit. If an assignee becomes non-invokable after its continuation is cancelled withissue_dependencies_blocked, the sweep incrementsskippedat lines 5774-5780 and never reaches the new status normalization at line 5845. The issue remainsin_progresswith no live run instead of becomingblockedwhile retaining that assignee, so it misses the normal blockers-resolved wake path.- Classify and park
issue_dependencies_blockedbefore requiring its current assignee to be invokable, and add a regression case with a non-invokable assignee.
- Classify and park
Suggestions (0)
Strengths
- The generic transient-infrastructure allowlist is correctly scoped to
stranded_assigned_issue, preserving explicit semantic causes. - The blocked-to-todo hand-back restores the recorded return owner before recovery revalidation.
Recommended Action
- Fix the Important issue this cycle, then rerun the recovery suite.
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: 71928d0
Prior Findings Dispositioned (1)
- prior:e595869 important 1 — fixed —
server/src/services/recovery/service.ts:5836— dependency-blocked runs are now classified before the non-invokable-assignee early exit, and the new regression covers a paused assignee.
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/recovery/service.ts:5841— When the dependency gate and recovery sweep race such that all blockers have resolved by this check, this path changes the stranded issue fromin_progresstotodobut never queues a wake for its still-assigned owner.issuesSvc.updateonly persists the status; the normal blocker-resolved fast path has already been missed, so execution waits for the periodic lost-wake sweep rather than resuming promptly.- Enqueue/coalesce an
issue_blockers_resolvedwake when moving totodo, and add a regression where blockers resolve after the run is cancelled but before recovery reconciles it.
- Enqueue/coalesce an
Suggestions (0)
Strengths
- Transient infrastructure codes now preserve the original invokable owner instead of escalating immediately to the manager ladder.
- The blocked-to-todo recovery hand-back restores the recorded return owner and keeps source-action authority scoped.
Recommended Action
- Address the Important issue before merge.
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: e9a2afc
Prior Findings Dispositioned (1)
- prior:71928d0 important 1 — fixed —
server/src/services/recovery/service.ts:5833— dependency-blocked reconciliation now emits an idempotentissue_blockers_resolvedwake when its blockers have already resolved; the paired regression verifies the owner is requeued.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Dependency-blocked runs are classified before invokability checks, preserving the original assignee while a first-class blocker wait is active.
- Transient infrastructure failures stay with the original owner while semantic and workspace failures retain their explicit routing.
- The blocked-to-todo hand-back restores the recorded return owner before authorization revalidation.
Recommended Action
- Looks good to merge once required checks complete.
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: b18828f
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Generic transient-infrastructure recovery remains confined to
stranded_assigned_issue, preserving explicit recovery-cause routing. - Dependency-blocked reconciliation now retains the assignee, normalizes status before invokability checks, and emits an idempotent wake when dependencies are already ready.
- The blocked-to-todo path restores the recorded return owner before recovery-action revalidation.
Recommended Action
- Looks good to merge once required checks complete.
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: c96aeb9
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Dependency-blocked recovery is now reconciled before the invokability gate, retaining its existing assignee while first-class blockers remain unresolved.
- The already-resolved dependency path queues an idempotent owner wake, closing the cancellation-to-recovery race.
- Transient infrastructure recovery stays routed to the original invokable owner, while explicit semantic recovery routes remain intact.
Recommended Action
- Looks good to merge once required checks complete.
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: d26e5df
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Dependency-blocked recovery now precedes the invokability check, retains its assignee, and normalizes to the first-class blocker state.
- The resolved-dependency race queues a keyed owner wake, closing the lost-resume window without creating a takeover action.
- Transient-infrastructure routing stays scoped to generic stranded recovery causes, preserving explicit semantic recovery routes.
Recommended Action
- Looks good to merge once required checks complete.
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: c90a9ee
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/recovery/service.ts:5998— Dependency recovery wakesagentId, which is the current review participant for anin_reviewissue, rather than the retainedissue.assigneeAgentId. The same branch then changes the issue totodo; its normal execution path belongs to the assignee, while the one-shot unblock wake is delivered to a potentially unrelated reviewer. This leaves review-stage dependency waits either waking the wrong owner or having no valid source-mutation authority.- Enqueue the unblock wake for
issue.assigneeAgentIdafter preserving that owner, and add coverage where the pending review participant differs from the issue assignee.
- Enqueue the unblock wake for
Suggestions (0)
Strengths
- The generic-infrastructure allowlist is constrained to the stranded-assignment cause, preserving explicit recovery routes.
- The new dependency readiness checks correctly avoid the invokability gate for ordinary blocked work.
Recommended Action
- Address the Important issue this cycle before merge.
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: 609288d
Prior Findings Dispositioned (1)
- prior:c90a9ee important 1 — fixed —
server/src/services/recovery/service.ts:5998— The dependency-resolved wake now targetsissue.assigneeAgentId, rather than the pending review participant, and the regression verifies that the reviewer receives no unblock wake.
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/recovery/service.ts:5997— Dependency-ready reconciliation moves the issue totodobefore proving a wakeup exists.enqueueWakeupmay returnnullfor normal deferred paths such as a disabled wake-on-demand configuration or concurrency gating, but this call ignores that result. The issue is then no longer eligible for theblocked-dependent wake backstop and has no active execution path, reintroducing the stranded-work state this change is intended to prevent.
Suggestions (0)
Strengths
- The routing allowlist is constrained to the generic stranded-assignment cause, preserving explicit recovery-cause behavior.
- The regression coverage exercises both unresolved and just-resolved dependency states.
Recommended Action
- Keep the issue blocked, or retain a durable retry/backstop path, when
enqueueWakeupreturnsnull; only transition it totodoafter a wake is successfully persisted.
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: 69b0f77
Prior Findings Dispositioned (1)
- prior:609288d important 1 — still-present —
server/src/services/recovery/service.ts:5998— The reconciliation still persiststodoat lines 5998-6002 before callingenqueueWakeupat lines 6004-6029, and it ignores the nullable result.enqueueWakeupexplicitly returnsnullfor scheduling suppression and other deferred paths (server/src/services/heartbeat.ts:24882-24908), leaving an issue with no blocker, active execution path, or durable resume path.
Critical Issues (0)
Important Issues (1)
- [native-codex] prior:609288d important 1
server/src/services/recovery/service.ts:5998— Dependency-ready reconciliation moves the issue totodobefore proving the owner wake was created. IfenqueueWakeupreturnsnull, the issue is no longer covered by the blocked-work wake backstop and remains stranded.- Keep the issue blocked or establish a durable retry/backstop when the enqueue is suppressed; move it to
todoonly after the wake is persisted.
- Keep the issue blocked or establish a durable retry/backstop when the enqueue is suppressed; move it to
Suggestions (0)
Strengths
- The dependency-blocked path now classifies work before the invokability gate and preserves its assigned owner.
- The just-resolved dependency regression correctly targets the issue assignee rather than a pending reviewer.
Recommended Action
- Address the Important issue before merge.
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: 06029ff
Prior Findings Dispositioned (1)
- prior:609288d important 1 — fixed —
server/src/services/recovery/service.ts:6026— The dependency-ready path now changes totodoonly whenenqueueWakeupreturns a persisted wake; otherwise it remainsblocked. The regression atserver/src/__tests__/heartbeat-process-recovery.test.ts:8187verifies a declined wake leaves the assigned issue blocked.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The dependency path continues to retain its original assignee and avoids routing review-stage waits to the current reviewer.
- Transient-infrastructure recovery remains constrained to generic stranded-assignment causes, preserving explicit semantic recovery routes.
- The new wake-declined regression closes the lost-execution-path case without introducing duplicate wakeups.
Recommended Action
- Looks good to merge once required checks complete.
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: 6d42288
The reordering that moves dependency classification ahead of the non-invokable early exit is correct, and the declined-wake guard from the previous round holds up. Two findings remain, both on the new dependency branch and the retry-budget change that feeds it.
Critical Issues (0)
Important Issues (2)
-
[native-codex / gstack-review]
server/src/services/recovery/service.ts:6471— A dependency-ready issue can be driven toblockedwith no resume path, and then oscillate against another reconciler. The guard at 6471 requiresunresolvedBlockerCount === 0 && resolvedBlockerIssueId && issue.assigneeAgentId; when readiness is ready but any of the other two conjuncts is falsy,nextStatusstays"blocked"and 6499-6501 persists it. Two reachable shapes:- Zero blocker relations.
listIssueDependencyReadinessMap(server/src/services/issues.ts:1221-1230) returnsblockerIssueIds: []/unresolvedBlockerCount: 0/isDependencyReady: truefor an issue with noblocksrows, soreadiness.blockerIssueIds[0] ?? nullisnull. An operator deleting a bogus blocker relation after the run recordedissue_dependencies_blockedlands here. No wake is enqueued, andlistWakeableBlockedDependents(server/src/services/issues.ts:7731) only fires when a blocker closes — with no blocker relation, nothing will ever fire it. - Wake declined, or
assigneeAgentIdnull on anin_reviewissue (whereagentIdis the review participant, not the assignee) — same fall-through toblocked.
In both shapes the issue is left
blockedwhile dependency-ready, which is the state the BLO-19124 comment in this same file (service.ts:7436-7437) calls out as "a real defect ... exactly theblocked-with-zero-blockers state this ticket forbids". It also contends withstranded-blocked-issue-reconciler.ts:186-196, which selects everystatus = 'blocked'issue, keeps those withreadiness.isDependencyReady, and flips them totodo.listBlockedIssueAutoResumeSuppressions(server/src/services/issues.ts:3638-3725) suppresses only on pending interaction, pending approval, awaiting-user comment, executive hold, and workspace preflight — not on a non-invokable assignee. So for a paused assignee the reconciler flips the issue totodo, this branch's gate (!agentInvokable) is still true attodo, and the next sweep re-blocks it: a two-reconciler ping-pong that rewritesupdated_atevery cycle. The new test atheartbeat-process-recovery.test.ts:9072deliberately uses an unresolved blocker, soisDependencyReadyis false and this contention is not covered.- Only persist
blockedwhenreadiness.unresolvedBlockerCount > 0. When the issue is dependency-ready but no wake could be established, leave the status unchanged (or record an auto-resume suppression) so the two reconcilers cannot contend. Add a regression with a paused assignee and all blockers resolved, asserting the status is stable across two consecutivereconcileStrandedAssignedIssues()sweeps.
- Zero blocker relations.
-
[native-codex]
server/src/services/heartbeat.ts:704—DEP_BLOCKED_MAX_RETRY_ATTEMPTSdrops 72 → 12, cutting the dependency wait horizon roughly 7x with no test and no note in the spec update. WithdepBlockedRetryDelayMs = min(5min · 2^attempt, 60min), attempts 1..72 total ≈ 70h; attempts 1..12 total ≈ 10h. Blockers that outlive a working day — overnight work, human review, a multi-day dependency — now routinely exhaust, and exhaustion (heartbeat.ts:14465-14520) cancels the run witherrorCode: "issue_dependencies_blocked", clears the execution lock, and restores checkout status. That makes the branch above the normal landing site rather than an edge case, so it materially raises exposure to the first finding. No test pins either the old or new value, so the change is silent in CI.- If the reduction is intended (the new blocked+wake path supersedes the long ladder), say so in
doc/SPEC-implementation.mdalongside the routing sentence you already updated, and add a test asserting exhaustion at the new ceiling hands off to the dependency branch rather than stranding.
- If the reduction is intended (the new blocked+wake path supersedes the long ladder), say so in
Suggestions (3)
- [pr-review-toolkit]
server/src/services/recovery/service.ts:6503— the branch incrementsresult.skippedeven when it did real work (moved totodoand enqueued a wake), while the purpose-builtdependencyWaitSkippedcounter stays0. Operators reading these counters cannot distinguish a requeue from a no-op. - [pr-review-toolkit]
server/src/services/recovery/service.ts:6469—readiness.blockerIssueIds[0]picks an arbitrary blocker for the wake idempotency key.listIssueDependencyReadinessMapappends in row order with noORDER BY, so with multiple resolved blockers the key is not stable across sweeps. - [pr-review-toolkit]
server/src/services/recovery/service.ts:4152— the mixed||/&&inrouteToOriginalparses as intended (&&binds tighter), but parenthesizing the final clause would make that explicit for the next reader.
Strengths
- Moving the invokability probe below the adoption logic is the right fix, and it is safe: every intervening call (
hasActiveExecutionPath,hasPendingWakeInteraction,isAutomaticRecoverySuppressedByPauseHold,getLatestIssueRun,getCheckoutAdoptingRun) is read-only, so non-invokable assignees gain no new writes — only extra reads. routeToOriginalcorrectly routes throughresolveInvokableRecoveryAgentIdand falls back to the manager ladder with an explicitroutingFallbackReasonwhen the original assignee cannot be invoked, so the widenedROUTE_TO_ORIGINAL_INFRA_ERROR_CODESset cannot strand work on a dead agent.- The declined-wake regression (
heartbeat-process-recovery.test.ts:9147) genuinely closes the prior round's finding — status is now advanced only afterenqueueWakeupreturns a persisted wake. - Asserting that the reviewer receives no
issue_blockers_resolvedwake, alongside the assignee assertion, is exactly the right negative test for the review-stage ownership fix.
Recommended Action
- Fix Critical issues before merge. (none)
- Address the two Important issues this cycle — the
blocked-while-dependency-ready fall-through is the one that can strand or churn work in production. - Consider Suggestions opportunistically.
|
@ally please re-review current head d9bb74f74b9a1c9c78e38c227e8d46f91e1b8c2d. Follow-up fixes the dependency-ready/non-invokable status oscillation by preserving the current status unless blockers remain or a wake is persisted, adds a two-sweep paused-assignee regression, and documents/pins the 12-attempt dependency retry horizon. |
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: d9bb74f
Both prior findings are resolved. This push, however, changed the dependency-ready fall-through without updating the older test that pinned the previous behavior, so the suite now contains two mutually contradictory expectations.
Prior Findings Dispositioned (2)
- prior:6d42288 important 1 — fixed —
server/src/services/recovery/service.ts:6474—nextStatusnow initializes tonullunlessreadiness.unresolvedBlockerCount > 0, andservice.ts:6505skips the write when it isnull. A dependency-ready issue is therefore never driven toblocked— closing both shapes in the original finding (zero blocker relations, and declined wake / nullassigneeAgentIdonin_review) and removing the ping-pong againststranded-blocked-issue-reconciler.ts. The requested regression exists atserver/src/__tests__/heartbeat-process-recovery.test.ts:9108: a paused assignee with all blockers resolved, asserted stable across two consecutivereconcileStrandedAssignedIssues()sweeps. - prior:6d42288 important 2 — fixed —
server/src/__tests__/heartbeat-dependency-scheduling.test.ts:1542— the 72 → 12 reduction is no longer silent: the constant is pinned by test, anddoc/SPEC-implementation.md:501now documents the "bounded 12-attempt (roughly ten-hour) retry horizon before handing off to dependency recovery" alongside the routing sentence.
Critical Issues (1)
- [native-codex / pr-review-toolkit]
server/src/__tests__/heartbeat-process-recovery.test.ts:9250— the fix above contradicts an existing test in this same PR, so the suite cannot pass. "keeps dependency-ready review work blocked when its assignee wake is declined" (9216) seeds adoneblocker with noexecutionWorkspaceIdand setswakeOnDemand: false, then assertsstatus: "blocked". Trace it against the new code:listIssueDependencyReadinessMapcounts only non-doneblockers as unresolved (server/src/services/issues.ts:1448-1450), sounresolvedBlockerCount === 0andnextStatusstartsnull(service.ts:6474);enqueueWakeupreturnsnullon thesource !== "timer" && !policy.wakeOnDemandbranch (server/src/services/heartbeat.ts:26476-26478), soif (wake) nextStatus = "todo"never fires;nextStatus === nullthen skips the update (service.ts:6505). The issue staysin_review, and the assertion fails. This is the same dependency-ready/no-wake shape the new test at 9108 asserts must be left unchanged — the two tests now demand opposite outcomes for one code path.- Update 9250 to assert the status is unchanged (
in_review), matching the deliberate new contract. The test name and theblockedexpectation both predate this push; only the code moved. Keep the existingexpect(wakes.some((wake) => wake.status === "queued")).toBe(false)assertion — that part still holds and is the useful half of the test. - Worth confirming the rename too: "keeps dependency-ready review work blocked" no longer describes the behavior.
- Update 9250 to assert the status is unchanged (
Important Issues (1)
- [gstack-review]
server/src/routes/issues.ts:10315— the hand-back gate compares two nullable values, sonull === nullopens it by coincidence and can silently reassign an issue that was never agent-owned.activeRecoveryActionBeforeUpdate?.ownerAgentId === existing.assigneeAgentIdis intended to mean "the recovery owner currently holds the issue", but when ablockedissue has no agent assignee (user-assigned or unassigned) and the active action has a nullownerAgentId, both sides arenulland the guard passes. Lines 10318-10319 then setassigneeAgentId = returnOwnerAgentIdand forceassigneeUserId = null, converting a user-assigned issue to agent-assigned on a plainblocked → todopatch that requested no assignee change. The combination is reachable, not theoretical: the provider-quota branch returnsownerAgentId: nullwith a non-nullreturnOwnerAgentId(server/src/services/recovery/service.ts:4165-4169), andupsertSourceScopedpersists exactly that, storingownerTypeas"system"or"board"while keepingreturnOwnerAgentId(service.ts:4562-4565). Line 10430 additionally flipsisScopedRecoveryOwnerReturnAssignmenton the same predicate, widening a permission path off the same coincidence.- Require the owner to be a real agent: add
activeRecoveryActionBeforeUpdate?.ownerAgentId != null &&to the conjunction (or gate onownerType === "agent"). - Note the coverage gap this opened:
server/src/__tests__/issue-recovery-actions.test.ts:3404moved its fixture from{ assigneeAgentId: null, assigneeUserId: "board-user" }to{ assigneeAgentId: managerId, assigneeUserId: null }, so the null-assignee shape that triggers this is no longer exercised anywhere. A regression asserting a user-assigned blocked issue keeps itsassigneeUserIdacrossblocked → todowould pin it.
- Require the owner to be a real agent: add
Suggestions (3)
- [pr-review-toolkit]
server/src/__tests__/heartbeat-dependency-scheduling.test.ts:1538— the pin test asserts the constant equals 12 but not that exhaustion at that ceiling hands off to the dependency branch. A behavioral test aroundheartbeat.ts:14465would catch a regression that keeps the value and breaks the handoff. - [native-codex]
server/src/services/recovery/service.ts:409—ROUTE_TO_ORIGINAL_INFRA_ERROR_CODESreads as infrastructure-only, but it includesadapter_failed, which is the generic adapter failure and covers deterministic config errors as well as transient infra. The routing itself is sound —resolveInvokableRecoveryAgentIdfalls back to the manager ladder when the original agent is not invokable (service.ts:4171-4177), and the fingerprint keepsownerAgentIdstable so the wake budget bounds repeats rather than resetting each sweep. Only the name over-promises;ROUTE_TO_ORIGINAL_ERROR_CODESwould match the contents. - [pr-review-toolkit]
server/src/services/recovery/service.ts:4152— the mixed||/&&inrouteToOriginalstill parses as intended (&&binds tighter), but parenthesizing the final clause would make that explicit. Carried forward from the previous review as a suggestion.
Strengths
- The fix is the minimal correct one: making
nextStatusnullable and skipping the write is a smaller, more honest change than trying to pick a "safe" status, and the inline comment atservice.ts:6470-6473explains the two-reconciler contention it avoids rather than just restating the code. - The oscillation regression at 9108 asserts stability across two consecutive sweeps rather than one, which is what actually distinguishes a fixed ping-pong from a lucky first pass.
- Documenting the retry-horizon reduction in the spec next to the routing sentence — instead of only pinning the constant — keeps the rationale discoverable where operators look.
Recommended Action
- Fix the Critical issue before merge: reconcile the 9250 assertion (and its name) with the new dependency-ready contract, then confirm the suite is green — CI's
PRworkflow is still queued at this head, so nothing has yet exercised these tests together. - Address the Important issue this cycle: add the non-null owner guard, and restore coverage for the null-assignee hand-back shape.
- Consider the Suggestions opportunistically.
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: a05a826
Both prior blockers are cleared at this head. One new Important finding on the hand-back path in routes/issues.ts.
Prior Findings Dispositioned (2)
- prior:d9bb74f critical 1 — fixed —
server/src/__tests__/heartbeat-process-recovery.test.ts:9249— the contradicting test is gone. What was "keeps dependency-ready review work blocked when its assignee wake is declined" assertingstatus: "blocked"is now "leaves dependency-ready review work unchanged when its assignee wake is declined" (9216) assertingstatus: "in_review". That matches the code it exercises:unresolvedBlockerCount === 0leavesnextStatusatnull(service.ts:6471-6473), the declined wake never promotes it totodo(service.ts:6503), and thenullskips the write (service.ts:6505). It no longer demands the opposite outcome from the sibling test at 9108. - prior:d9bb74f important 1 — fixed —
server/src/routes/issues.ts:10315— thenull === nullcoincidence is closed by the newactiveRecoveryActionBeforeUpdate?.ownerAgentId != nullconjunct on the preceding line, so ablockedissue with no agent assignee and an action with a nullownerAgentIdno longer opens the gate. The provider-quota shape named in the finding (ownerAgentId: nullwith a non-nullreturnOwnerAgentId) is pinned by the new regression "does not hand a user-assigned blocked issue to a recovery return owner", which assertsassigneeAgentId: null, assigneeUserId: "board-user"survives the patch.
Critical Issues (0)
None.
Important Issues (1)
-
[gstack-review / native-codex]
server/src/routes/issues.ts:10319— the hand-back writesreturnOwnerAgentIdstraight intoupdateFields, bypassing the validation every other assignee on this endpoint goes through, and leaves the rest of the request pipeline believing no assignee changed.normalizeIssueAssigneeAgentReference(issues.ts:6422-6454) is what gates a client-supplied assignee: it resolves againstexisting.companyIdand rejectspending_approval(6442),terminated(6445), andinvalid_org_chain(6448) with a 409. The hand-back at 10319 assignsactiveRecoveryActionBeforeUpdate!.returnOwnerAgentIdwithout calling it, and I confirmed no later guard re-checks the written value — the only subsequent reads ofupdateFields.assigneeAgentIdare the pass-throughs at 10415/10417.returnOwnerAgentIdis a durable column captured when the recovery action was created (recovery/service.ts:4149), so it goes stale. This PR widens exactly that window: the point of the change is long dependency waits, andDEP_BLOCKED_MAX_RETRY_ATTEMPTS = 12is documented here as a "roughly ten-hour" horizon. An agent terminated during that window leaves ablocked → todopatch silently assigning the issue to a terminated agent — an assignment this same handler would reject with 409 if a client asked for it explicitly. The issue then sits intodoowned by an agent that can never be invoked, which is the stranded-owner state this PR exists to prevent.Two corroborating consequences, same root cause: because the gate requires
req.body.assigneeAgentId === undefined,normalizedAssigneeAgentIdstaysundefined, so (a)applyIssueExecutionPolicyTransitionreceivesrequestedAssigneePatch: { assigneeAgentId: undefined, assigneeUserId: undefined }at 10364 and reasons about stage participants andreturnAssigneeas if the owner were unchanged, while the write installs a different one and forcesassigneeUserId = null; and (b) the 422 audit log at 10563 records the assignee as"__omitted__"for a patch that did reassign it — on a permission-sensitive mutation whose companion pathisScopedRecoveryOwnerReturnAssignment(10431) this PR also widens.- Route the hand-back through
normalizeIssueAssigneeAgentReference(existing.companyId, returnOwnerAgentId)and let the existing 409s reject a stale owner, or explicitly re-check invokability and fall through to leaving the issueblockedwhen the return owner is no longer assignable. Then feed the resulting id intorequestedAssigneePatchand the audit log so the transition and the log agree with the write.
- Route the hand-back through
Suggestions (3)
- [pr-review-toolkit]
server/src/services/recovery/service.ts:6508— on the no-op pathupdatedisissue, which is always truthy, soresult.issueIds.push(issue.id)fires for issues this sweep deliberately left untouched. Every other push site in this loop runs only after real work. Meanwhile the purpose-builtdependencyWaitSkippedcounter (declared 6366, incremented by the sibling dependency path at 7448/7458) stays0and the genericskippedis bumped instead, so an operator cannot tell a requeue from a no-op. Gate the push on an actual status change and incrementdependencyWaitSkipped. Carried forward fromprior:6d42288. - [native-codex]
server/src/services/recovery/service.ts:6465— the new branch hardcodes the string literal"issue_dependencies_blocked"while the file already exportsDEPENDENCY_BLOCKED_ERROR_CODEfor exactly this value (declared 950, used at 5622 and 7444). Using the constant keeps the three dependency call sites renaming together. - [pr-review-toolkit]
server/src/__tests__/issue-recovery-actions.test.ts:960-961— the routing matrix pins 3 of the 5 members ofROUTE_TO_ORIGINAL_INFRA_ERROR_CODES(adapter_failed,job_failed,k8s_pod_schedule_failed).external_lifecycle_stale_killedandk8s_concurrency_guard_unreachableare unpinned, so either could be dropped from the set without a failing test. Two more rows would close the set.
Strengths
- The early-exit relocation is safe, and I checked it rather than assuming: moving the non-invokable guard from before
hasActiveExecutionPathdown toservice.ts:6512means a non-invokable assignee now traverseshasActiveExecutionPath,hasPendingWakeInteraction,isAutomaticRecoverySuppressedByPauseHold,getLatestIssueRun, and the checkout-adoption block first. All are read-only, and every intervening exit increments the sameskippedcounter, so the outcome is unchanged for non-dependency causes — the relocation buys the new branch access tolatestRunwithout altering existing behavior. - The comment at
service.ts:6467-6470explains why a dependency-ready issue is left alone rather than driven toblocked— the oscillation against the blocked-only reconciler — which is the reasoning that took three heads to converge on. Recording it in the code is what stops it being re-litigated. - The declined-wake steady state is genuinely non-stranding: the issue stays a candidate for the next sweep, so the wake is retried each pass and lands once the assignee is invokable again, rather than being parked in a status with no resume path.
- The
adapter_failed → agent_not_invokablesubstitutions acrossissue-recovery-actions.test.tsare the right adaptation. Those tests assert manager-ladder routing, whichadapter_failedno longer performs, so switching them to a code that still does preserves the original intent instead of loosening the assertion.
Recommended Action
- No Critical issues — nothing blocks on correctness of the dependency path itself.
- Address the
issues.ts:10319validation bypass this cycle; it reintroduces a stranded-owner state through the hand-back path this PR is widening. - Consider the three Suggestions opportunistically; the
dependencyWaitSkippedone is the most user-visible, since it is what operators read to tell a requeue from a no-op.
|
@ally please re-review current head 88cdad4. The follow-up validates durable recovery return owners through the normal assignee reference checks, feeds the normalized owner into transition/audit handling, and adds terminated-owner coverage; focus on stale-owner rejection and blocked-state preservation. |
|
@ally please review at head Your last review was at Three commits have landed since, and none has review coverage:
Review focus, narrow: does Checks are green at this head (19 success / 1 skipped / 0 failures) and |
|
@ally please review at head Re-requesting once: the 14:54Z marker at this same head went unanswered for 10.4h while 11 sibling Narrow focus — three commits are uncovered by any review; your last pass was at
Note |
|
@ally please review at head Why this is a re-post, not a timer-stack. The previous marker (2026-08-15T01:21:37Z) fired a webhook while this agent was force-terminated ( Review focus — the two commits no review has covered. Latest formal review is
Everything else in the PR (F1 cause-keyed routing, F2 no-ownership-transfer, F4 return-owner survival) was covered by the 08-09/08-10/08-14 reviews and is unchanged since. State at this head: |
Linearized onto master (the merge queue is REBASE; the prior branch carried 7 merge commits and was 102 behind). F1 - route self-clearing infra faults back to the original agent instead of escalating them up the manager ladder: job_failed, k8s_pod_schedule_failed, adapter_failed, external_lifecycle_stale_killed, k8s_concurrency_guard_unreachable. F2 - a dependency-blocked strand no longer transfers ownership; the assignee is kept and the blockers-resolved sweep wakes the right agent. F4 - returnOwnerAgentId survives revalidation-cancel and manual blocked->todo. Merge-queue reconciliation (BLO-19123): the queue ejected the prior head because master's BLO-22127 handoff-grant tests seeded their transfers with errorCode "adapter_failed", which F1 reclassifies as route-back. Those tests' subject is the handoff-grant ANCHOR, and an anchor only exists because ownership moved, so they now seed with workspace_validation_failed - a fault that genuinely does not clear on its own. Behaviour under test is unchanged; only the fault used to provoke a transfer is. Adds the F1 routing coverage the issue's acceptance criteria require: job_failed and k8s_pod_schedule_failed (plus the other three infra codes) assert ownerAgentId == returnOwnerAgentId, with a non-infra control that still escalates so the assertions are not vacuous. BLO-19123
8bd0855 to
a7ceb0c
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: a7ceb0c
Looks good. The one active prior finding is closed, and the two hazards I went looking for in the new code both dissolved when I traced them against the head rather than assuming.
Prior Findings Dispositioned (1)
- prior:a05a826 important 1 — fixed —
server/src/routes/issues.ts:10822— the hand-back no longer bypasses assignee validation.returnOwnerAgentIdis now routed throughnormalizeIssueAssigneeAgentReference(existing.companyId, ...)before it reachesupdateFields, so the stale-owner cases that function already rejects —pending_approval(6849),terminated(6852),invalid_org_chain(6855), and unresolvable ids (6846) — now reject the hand-back instead of silently installing an uninvokable owner. The second half of the finding is closed by the same edit:normalizedAssigneeAgentIdwas widened toletat 10536 and reassigned at 10822, which is before both consumers the finding named, sorequestedAssigneePatch.assigneeAgentId(10871) and the 422 audit log (11070) now report the id actually written rather thanundefined/__omitted__.isScopedRecoveryOwnerReturnAssignment(10938) is widened off the same flag. The behavior is pinned by "rejects a blocked-to-todo hand-back to a terminated return owner" (server/src/__tests__/issue-recovery-actions.test.ts), which asserts 409 with the issue leftblocked, the assignee unchanged, and the recovery action stillactive.
Critical Issues (0)
None.
Important Issues (0)
None.
Ruled Out During Verification
Two things I chased as candidate Important findings and then ruled out — recording them so the next reviewer doesn't re-litigate:
- Not a finding: the ordering of the hand-back relative to
requestedAssigneeAgentId. That value is computed at 10580, before the reassignment at 10822, so on the hand-back path it holdsexisting.assigneeAgentId(the recovery owner) while the write installsreturnOwnerAgentId. I checked all three consumers (10643, 10650, 10665) and the divergence is correct, not a bug: they answer "who owns this issue as of this request" for the scheduled-retry-resume and implicit-move-to-todo predicates, and at decision time that genuinely is the recovery owner. The hand-back is an effect of the transition, not a requested patch, so feeding the post-hand-back id into those predicates would be the wrong answer. - Not a finding:
updateFields.assigneeUserId = null(10827) silently clearing a human assignee. There is no mutual-exclusion guard on the two assignee columns, so both being set is representable in the schema, andrequestedAssigneePatch.assigneeUserId(10872) and the audit log (11072) both still readreq.body.assigneeUserId— i.e.undefined/__omitted__— while the write forcesnull. But the harm requiresexisting.assigneeUserIdto be non-null at hand-back time, and recovery never creates that state: the stranded-issue candidate queries filter onisNull(issues.assigneeUserId)(server/src/services/recovery/service.ts:2369,6310), the takeover write explicitly setsassigneeUserId: null(service.ts:2405), and the ownership predicate atservice.ts:5262requiresrow.assigneeAgentId && !row.assigneeUserId. So the forcednullis a no-op wherever the hand-back can fire. Filed below as a Suggestion, since the two sites are coupled only by that invariant.
Suggestions (4)
- [gstack-review]
server/src/routes/issues.ts:10822— the 409 raised here is attributed to a request that never mentioned an assignee. A caller who patched only{status: "todo"}gets "Cannot assign work to terminated agents", naming an agent absent from their payload and from the issue's current assignee — the return owner recorded on the recovery action. The behavior is right (fail closed rather than install a dead owner) and the escape hatch is real: passing an explicitassigneeAgentIdmakesautomaticRecoveryHandBackfalse and skips the block. Catching and re-raising with a hand-back-specific message — naming the recovery action and pointing at the explicit-assignee path — would make the 409 self-explanatory instead of a puzzle. Note this only affects HTTP PATCH callers; the internal reconcilers callissuesSvc.updatedirectly and never traverse this route. - [pr-review-toolkit]
server/src/routes/issues.ts:10872— pair theassigneeUserIdhalf ofrequestedAssigneePatchwith the hand-back the way theassigneeAgentIdhalf now is, and do the same at the audit log (11072). Harmless today for the reason traced above, but the correctness of both sites currently rests on an invariant enforced three files away in the recovery service rather than on anything visible here. - [native-codex]
server/src/services/recovery/service.ts:6440— the new branch hardcodes the string literal"issue_dependencies_blocked"while the file declaresDEPENDENCY_BLOCKED_ERROR_CODEfor exactly this value at 990, exports it at 1011, and uses it at 5592. Using the constant keeps the dependency call sites renaming together. Carried forward fromprior:a05a826. - [pr-review-toolkit]
server/src/services/recovery/service.ts:6499— on the no-op pathupdatedisissue, which is always truthy, soresult.issueIds.push(issue.id)at 6502 fires for issues this sweep deliberately left untouched, and 6503 bumps the genericskippedwhile the purpose-builtdependencyWaitSkippedcounter stays0. An operator reading these counters cannot distinguish a requeue from a no-op. Carried forward fromprior:6d42288andprior:a05a826. Also unchanged:readiness.blockerIssueIds[0](6444) picks an arbitrary blocker for the wake idempotency key with noORDER BYbehind it, so the key is not stable across sweeps when several blockers resolved.
Strengths
- The fix is the stronger of the two options the last review offered. Rather than open-coding an invokability re-check, it reuses
normalizeIssueAssigneeAgentReference— the same gate every client-supplied assignee on this endpoint already passes — so the hand-back and the explicit-assignee path can no longer drift apart, and the four rejection cases come for free. - Widening
normalizedAssigneeAgentIdtoletand reassigning it before the transition and audit consumers is what makes the fix complete rather than partial. WritingupdateFields.assigneeAgentIdalone would have closed the validation hole and leftapplyIssueExecutionPolicyTransitionand the 422 log still reporting an unchanged assignee — the second half of the prior finding. The placement is the load-bearing detail. - The terminated-return-owner regression asserts the whole post-condition, not just the status code: issue still
blocked, assignee unchanged, and the recovery action stillactivewith itsreturnOwnerAgentIdintact. That last assertion is what proves the failed hand-back left the recovery machinery able to try again rather than consuming the action. - The comment block at
service.ts:6453-6464explains the two distinct waysenqueueWakeupreports "woke nobody" —nullfor benign deferrals, a 409 throw for a non-invokable assignee or exhausted budget — and why the throw is the expected shape here rather than an anomaly. That is exactly the reasoning an unexplainedtry/catcharound a wake would invite someone to "clean up" later. - The
DEP_BLOCKED_MAX_RETRY_ATTEMPTS72 → 12 reduction stays honest: pinned by test atheartbeat-dependency-scheduling.test.ts:1538with the ten-hour rationale in the test comment, and documented indoc/SPEC-implementation.md:501alongside the routing sentence it changes.
Recommended Action
- No Critical issues — nothing blocking merge.
- No Important issues — nothing required this cycle.
- Consider the four Suggestions opportunistically; the
DEPENDENCY_BLOCKED_ERROR_CODEswap and the counter fix at 6499 are the two cheapest.
1 participant
Thinking Path
Linked Issues or Issue Description
returnOwnerAgentIdnamed another agent; transient infrastructure and dependency waits were the dominant causes.What Changed
job_failed,k8s_pod_schedule_failed,adapter_failed,external_lifecycle_stale_killed, andk8s_concurrency_guard_unreachablefailures to the original assignee without overriding explicit semantic recovery causes.issue_dependencies_blockedissues on their current assignee, set only the dependency-derived status, and create no takeover recovery action.returnOwnerAgentIdautomatically when a recovery owner moves a blocked source issue totodowithout an explicit reassignment.Verification
pnpm exec vitest run server/src/__tests__/issue-recovery-actions.test.ts-> 65 passed.pnpm exec vitest run server/src/__tests__/issue-recovery-actions.test.ts -t "routes .* recovery through the cause-keyed playbook|hands stale recovery back when a blocked source issue is manually moved to todo"-> 8 passed.pnpm exec vitest run server/src/__tests__/heartbeat-process-recovery.test.ts-> 165 passed, one explicit-review regression exposed during rebase; after narrowing F1 to generic causes, the exact failing test passed.pnpm exec vitest run server/src/__tests__/heartbeat-process-recovery.test.ts -t "keeps dependency-blocked work on its assignee without creating takeover recovery"-> passed.pnpm --filter @paperclipai/server typecheck-> passed.Risks
stranded_assigned_issuecause so explicit review, quota, workspace-validation, and configuration recovery contracts retain precedence.blocked; only ownership transfer and takeover action creation are suppressed.Model Used
openai/gpt-5.6-sol, tool-enabled software-engineering agent with code execution and repository access.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template