fix(recovery): persist job-missing invocation evidence (BLO-18106)

[allyblockcast]

Thinking Path

• Paperclip manages durable agent execution across local and external lifecycle adapters
• The heartbeat reconciler terminalizes runs when their external Kubernetes Job disappears
• Pre-invocation disappearance is already persisted as process_lost; job_missing is emitted only after adapter.invoke
• Missing Jobs and broad k8s_pod_schedule_failed outcomes may follow non-idempotent side effects, so neither is safe to replay without durable never-invoked evidence
• The reconciler now persists invocation evidence for missing Jobs, while both automatic and continuation recovery remain fail-safe for production-reachable job_missing
• The benefit is deterministic recovery without duplicate reviews or other duplicate external side effects

Linked Issues or Issue Description

• Refs BLO-18106
• Refs BLO-19889
• Problem: real job_missing finalizations did not persist the invocation evidence consumed by retry safety checks. Pre-invocation loss is process_lost; production job_missing records must remain fail-safe because invocation already started.
• Duplicate search: GitHub PR search for job_missing adapterInvocationStarted k8s_pod_schedule_failed returned only this PR.

What Changed

• Persist externalLifecycleRecovery.adapterInvocationStarted for reconciler-produced job_missing records, matching the existing job_failed path.
• Assert through the real reapOrphanedRuns integration fixture that production writes true and does not schedule an automatic retry.
• Keep production-reachable job_missing on the default continuation path; pre-invocation process_lost remains the bounded transient retry path.
• Require durable never-invoked evidence before automatic retry of job_missing.
• Stop automatically retrying ambiguous k8s_pod_schedule_failed outcomes, which current adapters may emit after the main container starts.

Verification

• pnpm exec vitest run server/src/services/recovery/service.infra-class-continuation.test.ts server/src/__tests__/heartbeat-retry-scheduling.test.ts (53 passed)
• pnpm exec vitest run server/src/__tests__/heartbeat-process-recovery.test.ts -t "records that adapter invocation started" (1 passed, 158 skipped)
• pnpm --filter @paperclipai/server typecheck (clean)

Risks

• Safety-biased behavior: ambiguous missing/failed Job states no longer trigger automatic replay unless durable evidence proves invocation never began.
• A scheduler failure without safe evidence now relies on normal issue disposition/escalation instead of risking duplicate external side effects.
• No migrations, API schema changes, UI changes, or documentation changes.

Model Used

• OpenAI openai/gpt-5.6-sol, tool-enabled coding and test execution.

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
• I have updated relevant documentation to reflect my changes
• 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

[allyblockcast]

🔗 Paperclip issue: BLO-16182
🔗 Paperclip issue: BLO-18669
🔗 Paperclip issue: BLO-18106
🔗 Paperclip issue: BLO-19889

[allyblockcast]

🔗 Paperclip issue: BLO-16182
🔗 Paperclip issue: BLO-18669
🔗 Paperclip issue: BLO-18106
🔗 Paperclip issue: BLO-19889

[allyblockcast]

@ally please review PR #1048 (BLO-19889).

Review focus:

1. Is k8s_pod_schedule_failed genuinely safe to re-dispatch unconditionally? I claim the pod never bound to a node so the adapter was never invoked. Challenge that: is there any path where this code is emitted after the container started?
2. Is the job_missing evidence gate correct? I gated on resultJson.externalLifecycleRecovery.adapterInvocationStarted === false, mirroring the job_failed gate at heartbeat.ts:916-919. Is that field populated on the job_missing path at all, or is my gate dead code that always falls through to default? That is the failure mode I most want checked.
3. Retry-budget interaction. process_lost needed a broadened streak counter (BLO-16182, isContinuationAttemptRetryReason) so two retry engines could not each grant a fresh budget. Does k8s_pod_schedule_failed have a second retry engine I have missed? I believe not — shouldScheduleAutomaticRunRetry only re-queues it for pr_review — but a missed engine would mean an uncapped retry loop.

[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: 92b3492

Critical Issues (1)

• [gstack/review] server/src/services/recovery/service.ts:634 — k8s_pod_schedule_failed is not a reliable "pod never ran" signal, so retrying it unconditionally can repeat non-idempotent work. The exact adapters pinned by this tree also emit this code after scheduling: the Claude adapter maps phase === "Failed", init-container exits, and main-container CrashLoopBackOff through this error, while its phase === "Failed" diagnostic can include a terminated main container (paperclip-adapter-claude-k8s at pinned 3ad3370, src/server/execute.ts:879-933,1643-1654). The OpenCode adapter likewise maps init/container startup failures through the same code (paperclip-adapter-opencode-k8s at pinned 42384fd, src/server/execute.ts:397-440,925-934). In those states a container may have started and performed external side effects before failing.
  • Split the adapter error into a truly pre-schedule/pre-main-start code, or persist and require durable evidence that the main agent container never entered running before classifying it as transient infra. Add producer-level tests for fast main-container failure and CrashLoopBackOff, not only a synthetic classifier input.

Important Issues (1)

• [code/tests] server/src/services/recovery/service.ts:646 — the new job_missing branch is dead for real reconciler-produced runs. heartbeat.ts:15998-16000 calls hasAdapterInvocationEvent(...) only when terminalOutcome.errorCode === "job_failed"; for job_missing it sets the local value to null, and heartbeat.ts:16019 therefore omits externalLifecycleRecovery.adapterInvocationStarted. The unit test manufactures a result shape production never writes, so every real job_missing run still falls through to default.
  • Capture and persist the invocation event for job_missing as well, then test the reconciler-finalized run record end to end. Keep missing/non-boolean evidence on the fail-safe default path.

Strengths

• The classifier remains fail-safe for absent or malformed job_missing evidence.
• The change is narrowly scoped and includes controls for work-class and non-retryable failures.
• The continuation retry budget for ordinary issue-backed k8s_pod_schedule_failed runs has only the sweep engine; the existing PR-review retry engine is scoped to reviewer contexts, so I did not find a second engine on the normal issue path.

Recommended Action

1. Narrow or evidence-gate k8s_pod_schedule_failed before enabling unconditional re-dispatch.
2. Populate adapterInvocationStarted on the actual job_missing finalization path and cover that path with an integration test.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Addressed both findings at head 2559a7357f9ef19c2935e7d61209a9755355a38a.

• Removed unconditional transient replay for k8s_pod_schedule_failed; it remains fail-safe/default because current producers may emit it after main-container execution.
• Persisted adapterInvocationStarted for real reconciler-produced job_missing records and added a reapOrphanedRuns integration assertion.
• Kept absent/non-boolean/started evidence on the default path.
• Targeted tests and server typecheck pass locally; the GitHub review workflow is queued.

I am not requesting Ally again because this PR is authored by the Ally GitHub App and cannot receive an App self-review. The exact head needs an independent-author replacement before an Ally approval can exist.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:92b3492 critical 1 - no-longer-applicable - server/src/services/recovery/service.ts:913 - k8s_pod_schedule_failed is no longer in the transient continuation set, so it now reaches the fail-safe default classification instead of being replayed by this recovery path.
• prior:92b3492 important 1 - still-present - server/src/services/heartbeat.ts:16977 - the producer now persists invocation evidence, but missing-Job finalization is only evaluated inside the externalLifecycleStarted branch, which was derived from the same adapter.invoke event. A reconciler-produced job_missing therefore records adapterInvocationStarted: true; the new false classifier case remains unreachable.

Important Issues (1)

• [prior:92b3492 important 1] server/src/services/heartbeat.ts:16977 - The production reconciler still cannot produce the job_missing evidence value that enables transient recovery. The missing-Job path at heartbeat.ts:17143 is nested under externalLifecycleStarted, while the unit test manufactures adapterInvocationStarted: false directly. Consequently, real pre-invocation disappearance continues through the existing process_lost path, and real job_missing records still classify as default.
  • Add an end-to-end producer test for the intended never-invoked scenario and route that actual persisted outcome into the bounded classifier. If pre-invocation disappearance is intentionally always process_lost, remove the unreachable job_missing === false branch rather than presenting it as recovery coverage.

Strengths

• The unsafe k8s_pod_schedule_failed continuation widening has been removed.
• Missing, malformed, and started-invocation evidence remain fail-safe.
• The real missing-Job integration fixture now verifies the value production actually writes.

Recommended Action

1. Make the never-invoked producer and classifier agree, with an end-to-end test proving the retryable persisted state is reachable.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Addressed the current-head finding in b51414583ac5402178b28173f85a69d0418a6962.

• Confirmed pre-invocation disappearance is produced as process_lost; production job_missing is only finalized after adapter.invoke.
• Removed the unreachable job_missing === false continuation classification and kept real job_missing fail-safe/default.
• Gated automatic job_missing retry on durable never-invoked evidence and stopped automatic replay of ambiguous k8s_pod_schedule_failed outcomes.
• Extended the real reapOrphanedRuns fixture to prove persisted adapterInvocationStarted: true does not schedule a retry.

Local verification: continuation/retry suites 53 passed; missing-Job producer integration 1 passed (158 skipped); server typecheck clean. CI is running on the new head.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:92b3492 important 1 — fixed — server/src/__tests__/heartbeat-process-recovery.test.ts:1673 — The exact missing-Job producer fixture now persists adapterInvocationStarted: true and asserts that automatic retry is disabled; the unreachable continuation classifier arm for job_missing === false was removed, while pre-invocation loss remains the process_lost path.

Critical Issues (1)

• [gstack/review] server/src/services/recovery/service.infra-class-continuation.test.ts:45 — The new test treats kind: "default", maxAttempts: 1 as fail-safe for production job_missing, but the continuation reconciler interprets that state as permission for one automatic re-dispatch. On the original failed run, isAutomaticContinuationRecoveryRun(...) is false, so reconcileStrandedAssignedIssues reaches enqueueStrandedIssueRecovery(...); only a subsequent failed continuation consumes the single-attempt cap and escalates. Since the producer now proves real job_missing occurs after adapter.invoke, that first continuation can repeat non-idempotent external side effects. Classify production job_missing as non-retryable in continuation recovery, or add an explicit no-requeue branch, and cover an issue-backed missing-Job finalization through the reconciliation sweep to assert that no continuation run is queued.

Suggestions (1)

• [code/tests] server/src/services/heartbeat.ts:916 — The synthetic job_missing plus adapterInvocationStarted: false arm uses generic transient_failure retry options because resolveAutomaticRunRetryOpts recognizes only job_failed. If this currently unreachable state is retained for future producers, route it through the external-job retry family and its issue execution lock/status gates, with a scheduling-level test; otherwise remove the dead arm rather than testing behavior production cannot emit.

Strengths

• Ambiguous k8s_pod_schedule_failed outcomes no longer replay automatically.
• Missing-Job finalization now records the invocation evidence production actually has.
• The automatic retry predicate fails safe for missing, malformed, or started-invocation evidence.

Recommended Action

1. Prevent the continuation sweep from re-dispatching production job_missing runs.
2. Add an issue-backed end-to-end assertion that neither retry engine queues work after a missing Job whose adapter invocation started.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[kkroo]

Reviewed the recovery changes. The final behavior keeps ambiguous k8s_pod_schedule_failed/job_missing on the fail-safe no-retry path, persists adapterInvocationStarted evidence for missing Jobs, and preserves process_lost as the bounded retry path for work that provably never started. Focused classifier tests pass locally and server typecheck is clean; embedded Postgres suites are skipped in this local environment and are left to CI.

[allyblockcast]

@ally please re-review head f9ab2b8. Focus on the continuation and automatic retry gates for job_missing: production missing-Job outcomes must never enqueue a duplicate continuation, while pre-invocation process_lost remains the bounded retry path. The new issue-backed sweep test asserts zero continuation requeues and an explicit blocked disposition.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:b514145 critical 1 — fixed — server/src/services/recovery/service.ts:665 — job_missing is now classified as non-retryable in the in_progress continuation path, and the issue-backed sweep test at server/src/__tests__/issue-recovery-actions.test.ts:419 proves that path queues no continuation and blocks the issue immediately.

Critical Issues (1)

• [gstack/review] server/src/services/recovery/service.infra-class-continuation.test.ts:34 — The test says k8s_pod_schedule_failed must not be replayed after possible main-container execution, but asserts the default classification. For the original failed run, isAutomaticContinuationRecoveryRun(...) is false, so reconcileStrandedAssignedIssues reaches enqueueStrandedIssueRecovery(...); maxAttempts: 1 only stops a subsequent failed recovery run. This still permits one duplicate execution after an ambiguous scheduling outcome.
  • Classify k8s_pod_schedule_failed as non-retryable unless durable producer evidence proves the main container never started, and add an issue-backed sweep test asserting zero queued continuation work.

Important Issues (2)

• [native-codex] server/src/services/heartbeat.ts:913 — The new job_missing no-retry policy is evaluated after the generic persisted transient-recovery contract. Missing-Job finalization merges the previous resultJson, so a stale or contradictory errorFamily: "transient_upstream", rate_limit_exhausted, or provider_quota makes shouldScheduleAutomaticRunRetry(...) return true before it examines errorCode. That can replay a run whose adapter invocation already started.
  • Reject job_missing before reading the transient contract, or strip incompatible transient metadata during missing-Job finalization. Add a contradictory-evidence regression case.
• [code/recovery] server/src/services/recovery/service.ts:665 — Adding job_missing only to NON_RETRYABLE_CONTINUATION_ERROR_CODES protects the in_progress continuation branch. The same sweep's todo assignment-recovery and pending in_review participant-recovery branches do not call classifyContinuationFailure, so either status can still enqueue a replacement run after a missing Job.
  • Apply the job_missing non-retryable gate before status-specific recovery, or mirror it in every branch. Cover todo and pending in_review cases with zero-enqueue assertions.

Strengths

• Reconciler-produced missing-Job records now persist the actual adapter-invocation evidence.
• The in_progress continuation path now blocks production job_missing without a retry.
• The producer, automatic-retry predicate, and issue-backed recovery outcome all have targeted tests.

Recommended Action

1. Close all automatic recovery routes for outcomes that may follow external side effects.
2. Re-enable retries only from durable evidence that execution never began.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:f9ab2b8 critical 1 — fixed — server/src/services/recovery/service.ts:668 — k8s_pod_schedule_failed is now explicitly non-retryable, and the status-specific recovery paths no longer enqueue deliverable continuation work for that outcome.
• prior:f9ab2b8 important 1 — fixed — server/src/services/heartbeat.ts:878 — job_missing and k8s_pod_schedule_failed are rejected before merged transient recovery metadata is inspected, so stale errorFamily values cannot re-enable the automatic retry engine.
• prior:f9ab2b8 important 2 — fixed — server/src/services/recovery/service.ts:5742 — Pending review participants now classify terminal failures before requeueing; the corresponding assigned-todo gate is present at server/src/services/recovery/service.ts:5909.

Critical Issues (1)

• [gstack/review] server/src/services/recovery/service.ts:668 — The non-retryable classification still does not cover accepted-interaction continuation recovery. That branch runs before status-specific handling and unconditionally reaches enqueueStrandedIssueRecovery(...) at service.ts:5687 when no successful post-resolution run exists. If that latest post-resolution run ended with job_missing or k8s_pod_schedule_failed, Paperclip can therefore launch deliverable work again after the adapter may already have produced non-idempotent external side effects.
  • Classify latestPostResolutionRun before the accepted-interaction enqueue and escalate non-retryable outcomes without a deliverable wake. Add a regression test with an accepted interaction followed by job_missing, asserting zero continuation requeues.

Important Issues (2)

• [code/recovery] server/src/services/recovery/service.ts:5909 — The new todo and in_review non-retryable branches escalate before calling latestRunPredatesLatestUnblock(...). An operator who deliberately unblocks an issue can have the next sweep immediately return it to blocked using the same pre-unblock failure. The existing freshness invariant at service.ts:1317 and the in_progress path at service.ts:6237 already guard against this stale-evidence loop.
  • Apply the latest-unblock freshness gate before both new status-specific escalations and test blocked -> todo and blocked -> in_review transitions whose latest failed run predates the unblock.
• [types/errors] server/src/services/recovery/service.ts:5748 — The new review-participant escalation supplies recoveryOwnerAgentId but omits recoveryCause. resolveStrandedRecoveryCause(...) consequently falls back to stranded_assigned_issue, unlike adjacent review-participant branches that use execution_review_participant_recovery. This records the wrong action cause and emits the wrong recovery diagnostics/instructions; the test masks it by using the same agent as assignee and reviewer and never asserting the action cause.
  • Pass recoveryCause: EXECUTION_REVIEW_PARTICIPANT_RECOVERY_REASON and test with distinct assignee/reviewer agents while asserting the persisted recovery action cause.

Strengths

• Both ambiguous external-lifecycle outcomes now fail closed in the automatic retry predicate.
• The three status-specific recovery paths prevent deliverable-work replay for the common stranded-run flow.
• Production job_missing finalization persists the invocation evidence used by the recovery policy.

Recommended Action

1. Gate accepted-interaction continuation recovery with the same non-retryable policy.
2. Preserve explicit operator unblocks by applying the existing freshness guard consistently.
3. Record review-participant recovery under the correct cause.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

@ally please re-review head 0b19409. Focus on the prior current-head findings: accepted-interaction continuation now classifies only the matching interaction run and blocks non-retryable outcomes without deliverable replay; todo/review-participant escalation respects newer operator unblocks; and cross-agent review-participant escalation persists execution_review_participant_recovery. Full heartbeat process recovery suite: 163 passed; server typecheck clean.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:bbaa11b critical 1 — still-present — server/src/services/recovery/service.ts:1629 — Accepted-interaction recovery now matches the interaction ID, but it also restricts failure lookup to the current assignee. If ownership changes after the matching prior assignee's job_missing, that non-retryable run is invisible and the accepted interaction is dispatched again.
• prior:bbaa11b important 1 — fixed — server/src/services/recovery/service.ts:5952 — Assigned-todo escalation now checks the latest unblock before re-blocking; the equivalent review-participant guard is present at server/src/services/recovery/service.ts:5783.
• prior:bbaa11b important 2 — fixed — server/src/services/recovery/service.ts:5792 — Non-retryable review-participant escalation now persists execution_review_participant_recovery and identifies the participant as the recovery owner.

Critical Issues (2)

• [gstack/review] server/src/services/heartbeat.ts:16001 — A missing PR-review Job can still be replayed after adapter invocation. The PR evidence override replaces job_missing with pr_review_output_missing before invocation evidence is captured, so adapterInvocationStarted is omitted and shouldScheduleAutomaticRunRetry deliberately retries the rewritten code. The integration test at server/src/__tests__/heartbeat-process-recovery.test.ts:1768 currently asserts that unsafe retry. Preserve the underlying lifecycle outcome/evidence and refuse replay when the missing Job followed adapter.invoke.
• [prior:bbaa11b critical 1] server/src/services/recovery/service.ts:1629 — Accepted-interaction no-replay protection remains assignee-scoped. If agent A's interaction-correlated run ends in job_missing and the issue is reassigned to agent B before the sweep, the query excludes A's failure and enqueueStrandedIssueRecovery runs the accepted work again as B. Correlate non-retryable evidence by issue and interaction before selecting the current execution owner, and add a reassignment regression test.

Important Issues (1)

• [native-codex] server/src/services/recovery/service.ts:5778 — Review-participant non-retryable classification uses the latest run for that reviewer and issue, without correlating it to the current execution stage. Reusing a reviewer in a later pending stage can therefore consume an earlier stage's job_missing and immediately block the new stage before its run starts. Scope the selected run to the current stage/request evidence and test two sequential stages with the same reviewer.

Strengths

• Automatic retry now rejects raw job_missing and k8s_pod_schedule_failed before stale transient metadata is considered.
• The direct accepted-interaction path classifies matching same-assignee failures before enqueueing.
• todo and review-participant escalation now honor the existing unblock freshness guard and record the reviewer-specific recovery cause.

Recommended Action

1. Preserve and enforce adapter-invocation evidence through PR-review error rewriting.
2. Correlate accepted-interaction failure evidence independently of assignee changes.
3. Correlate review-participant failures to the current execution stage.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Addressed the current-head review findings in 39599d0edec93ab20976565922744fd62ac4becc.

• Preserve job_missing as the authoritative lifecycle outcome after adapter.invoke, retain PR-verification detail in recovery metadata, and block both bounded retry and immediate deliverable continuation replay.
• Correlate accepted-interaction outcomes by issue + interaction across assignee changes.
• Correlate review-participant recovery to the current execution stage, supporting both nested initial-stage and top-level recovery context shapes.
• Added regressions for reassignment, reused reviewers across sequential stages, and missing-Job PR review evidence.

Verification: six changed integration cases pass; retry/classifier suites pass (55 passed); server typecheck and git diff --check are clean. The earlier full process-recovery run reached 162/165; its three failures were obsolete retry/error-code expectations, and all three pass in the subsequent targeted run. CI is now running on the pushed head.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:0b19409 critical 1 — fixed — server/src/services/heartbeat.ts:16005 — Missing PR-review Jobs now retain job_missing as the authoritative lifecycle outcome, preserve adapterInvocationStarted: true, and store the PR-verification failure only as recovery metadata, so the bounded retry predicate cannot reinterpret the run as safe.
• prior:bbaa11b critical 1 — fixed — server/src/services/recovery/service.ts:1633 — Accepted-interaction recovery now selects post-resolution evidence by issue and exact interaction ID without restricting it to the current assignee, so reassignment no longer hides the prior owner's non-retryable failure.
• prior:0b19409 important 1 — still-present — server/src/services/heartbeat.ts:23134 — The periodic sweep now scopes review-participant evidence to the current stage, but immediate finalization still decides recovery from participant identity and terminal status alone. A late failure from stage A can therefore act on stage B when the same reviewer is reused.

Critical Issues (1)

• [gstack/review] server/src/services/heartbeat.ts:23287 — The new job_missing / k8s_pod_schedule_failed no-replay gate is reached only by the ordinary todo/in_progress branch. An in_review participant is handled earlier at heartbeat.ts:23134, where shouldBlockReviewRecovery does not classify either code, so run finalization immediately queues another deliverable reviewer run before the stage-scoped sweep can block it. This can repeat non-idempotent review-side effects after adapter.invoke already started.
  • Apply the same non-retryable lifecycle gate in immediate review-participant recovery and add a finalization-level in_review test asserting zero queued recovery runs for both codes.

Important Issues (1)

• [prior:0b19409 important 1] server/src/services/heartbeat.ts:23134 — Immediate review-participant recovery is still not correlated to executionState.currentStageId. If stage A's run finalizes after the issue advances to stage B with the same reviewer, the old run satisfies the participant/status checks and can queue or block recovery for the new stage. The added regression calls only reconcileStrandedAssignedIssues, so it never exercises this finalization race.
  • Compare the run's nested executionStage.stageId or top-level currentStageId with the current execution stage before immediate recovery, and test late stage-A finalization while stage B is pending.

Strengths

• Missing-Job PR-review evidence now remains fail-closed through terminalization.
• Accepted-interaction failures are correlated across assignee changes by exact interaction ID.
• The periodic sweep now scopes reviewer failures to the active execution stage and preserves explicit operator unblocks.

Recommended Action

1. Close the immediate review-participant replay path for non-retryable external-lifecycle outcomes.
2. Apply stage correlation at finalization time, not only in the later reconciliation sweep.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:39599d0 critical 1 — still-present — server/src/services/heartbeat.ts:23163 — Immediate review-participant recovery still blocks only for an unavailable agent or an already-failed recovery run. It does not classify job_missing or k8s_pod_schedule_failed, so the branch can still enqueue deliverable reviewer work after adapter invocation; the new non-retryable checks at heartbeat.ts:23287 apply only to the later ordinary todo/in_progress branch.
• prior:0b19409 important 1 — still-present — server/src/services/heartbeat.ts:23134 — The immediate eligibility predicate still matches only issue status, participant identity, wake kind, and terminal status. Although the fixture now persists executionStage.stageId, this branch never compares that value with executionState.currentStageId, so a late stage-A failure can act on stage B when the reviewer is reused.

Critical Issues (1)

• [prior:39599d0 critical 1] server/src/services/heartbeat.ts:23163 — A terminal review-participant run with job_missing or k8s_pod_schedule_failed still reaches the immediate recovery enqueue at heartbeat.ts:23178-23228. This can duplicate non-idempotent review-side effects before the stage-scoped periodic sweep runs.
  • Apply the non-retryable lifecycle classification before immediate review-participant enqueue and add a finalization-level test proving both codes queue zero deliverable recovery runs.

Important Issues (1)

• [prior:0b19409 important 1] server/src/services/heartbeat.ts:23134 — Immediate recovery remains uncorrelated with the active execution stage. Adding stage data to the seeded run does not enforce it; the predicate must compare the run's nested executionStage.stageId or top-level currentStageId with the issue's current stage before either blocking or queueing recovery.
  • Add a finalization-level regression where stage A finishes late after stage B becomes pending with the same reviewer, and assert stage B remains untouched.

Strengths

• Missing PR-review Jobs preserve job_missing and durable adapter-invocation evidence instead of being rewritten into a retryable review error.
• Accepted-interaction recovery is correlated by interaction across assignee changes.
• The periodic reconciliation sweep now scopes reviewer evidence to the active stage and preserves explicit operator unblocks.

Recommended Action

1. Close the immediate review-participant replay path for ambiguous external-lifecycle outcomes.
2. Enforce stage correlation during finalization, not only during the periodic sweep.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

@ally please re-review head 6282bf009f54cf00b7adf87daef85328609a98afc. Focus on the prior 39599d0ed findings: immediate review-participant finalization now blocks job_missing and k8s_pod_schedule_failed before a newer status-only lock can hide the outcome, and it requires the finalized run stage ID to match the active execution stage before any reviewer recovery. Regressions cover both lifecycle codes and late stage-A failure after stage B starts. Full heartbeat process recovery: 168 passed; issue recovery actions: 65 passed; retry/classifier suites: 55 passed; server typecheck clean.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:39599d0 critical 1 — fixed — server/src/services/heartbeat.ts:22815 — Immediate review-participant finalization now intercepts both job_missing and k8s_pod_schedule_failed for the exact active stage before the newer execution-lock guard, returning a blocked recovery disposition instead of enqueueing deliverable reviewer work.
• prior:0b19409 important 1 — fixed — server/src/services/heartbeat.ts:23164 — Ordinary immediate review recovery now requires the finalized run's nested executionStage.stageId or top-level currentStageId to equal executionState.currentStageId; the earlier non-retryable branch applies the same match at heartbeat.ts:22819-22823.

Critical Issues (0)

Important Issues (1)

• [gstack/review] server/src/services/recovery/service.ts:5793 — Active-stage correlation is not revalidated inside escalateStrandedAssignedIssue. The sweep selects a stage-matching run and checks it at service.ts:5549-5556, then calls escalation later; escalation opens a separate transaction and rechecks only that the status is still in_review or blocked at service.ts:4849-4871. If stage A advances to stage B in that window, the stale stage-A job_missing can still block and reassign stage B. The immediate-finalization path has the same check-then-escalate gap because releaseIssueExecutionAndPromote commits its stage check before invoking escalation at heartbeat.ts:23444.
• Recommendation: carry the expected stage ID and participant into escalation and revalidate both under the escalation transaction's issue lock before changing status or ownership; add a race regression that advances the stage between candidate selection and escalation.

Suggestions (1)

• [comments/errors] server/src/services/heartbeat.ts:22685 — The new no-replay branch uses a recovery comment saying Paperclip “retried ... once,” even when it intentionally blocks the first job_missing or k8s_pod_schedule_failed without retrying. Use a separate non-retryable message so the durable issue history reflects the actual safety decision.

Strengths

• Ambiguous external-lifecycle outcomes are rejected before stale transient metadata can make them retryable.
• Immediate finalization now covers both lifecycle codes before a newer lock can hide the terminal outcome.
• Finalization-level regressions exercise both lifecycle codes and late stage-A completion after stage B becomes active.

Recommended Action

1. Make the stage/participant freshness check atomic with recovery escalation.
2. Correct the non-retryable recovery audit message.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Addressed the current-head Ally findings in f65d9135b.

• Revalidates the expected review stage and agent participant inside the escalation transaction before status/ownership changes.
• Carries that expectation from both sweep and immediate-finalization recovery paths.
• Uses an accurate no-retry audit message for ambiguous external-lifecycle failures.
• Added a stage-A to stage-B race regression; targeted recovery suites and server typecheck pass.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:6282bf0 important 1 - still-present - server/src/services/recovery/service.ts:4853 - The stage and participant are re-read inside the escalation transaction, but the issue row is selected without FOR UPDATE; ordinary stage transitions do not take the advisory lock, and the eventual block update runs through the outer issuesSvc after the recovery action and wake are created. A concurrent stage advance can still land after this check and be overwritten by stale escalation.

Critical Issues (0)

Important Issues (3)

• [prior:6282bf0 important 1] server/src/services/recovery/service.ts:4853 - Review-stage freshness is still not atomic with recovery escalation. The advisory lock only serializes callers that take the same advisory lock, while stage transitions can update the unlocked issue row between validation and issuesSvc.update; the code can also create a recovery action and enqueue its wake before discovering a conflicting update.
• [gstack/review] server/src/services/heartbeat.ts:22827 - The new non-retryable review branch executes before the newer-execution guard at line 22849. If run B already owns issue.executionRunId for the same reviewer and stage when late run A finalizes as job_missing or k8s_pod_schedule_failed, A still returns blocked; stage-only revalidation then passes and blocks active review work.
• [tests/errors] server/src/services/heartbeat.ts:880 - PR-review runs with job_missing or k8s_pod_schedule_failed now terminate without entering bounded retry, but retry exhaustion is the only caller of queueExhaustedPrReviewGateStatus. A required review status can therefore remain pending indefinitely after this deliberate no-replay decision.

Strengths

• Missing-Job finalization now preserves the authoritative lifecycle outcome and durable adapter-invocation evidence.
• Accepted-interaction recovery is correlated by interaction rather than current assignee.
• The added tests cover stage changes and the common status-specific no-replay paths.

Recommended Action

1. Lock or conditionally update the exact expected review stage, participant, and execution owner before creating recovery side effects.
2. Do not let an older terminal run block a newer live run in the same stage.
3. Queue a guarded terminal failure status for non-retryable PR-review outcomes.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

@ally please re-review head 83d32767e. The three current-head findings are addressed:

• stage, participant, and execution ownership are atomically claimed before recovery actions or wakes are created;
• older terminal runs yield to a newer active execution in the same stage;
• non-retryable job_missing / k8s_pod_schedule_failed PR reviews enqueue the guarded terminal failure status instead of leaving the gate pending.

Verification: focused heartbeat regressions 4 passed; full issue-recovery-actions suite 66 passed; server typecheck and git diff --check clean.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:6282bf0 important 1 — fixed — server/src/services/recovery/service.ts:4850 — Escalation now locks the issue row, revalidates the exact stage, participant, and execution owner, and writes the blocking claim in the same transaction.
• prior:f65d913 important 2 — still-present — server/src/services/heartbeat.ts:22710 — A terminal run still queues the PR gate failure before the locked issue read at heartbeat.ts:22842 discovers and yields to a newer active execution, so the stale run can incorrectly fail the current review gate.
• prior:f65d913 important 3 — fixed — server/src/services/heartbeat.ts:22710 — job_missing and k8s_pod_schedule_failed PR-review runs now enqueue a guarded terminal failure delivery instead of leaving the required status pending.

Critical Issues (0)

Important Issues (3)

• [prior:f65d913 important 2] server/src/services/heartbeat.ts:22710 — The non-retryable PR failure delivery is queued before execution ownership is validated. If old run A finalizes after run B owns the same issue and stage, A enqueues a red status and only later yields at heartbeat.ts:22842-22847; the newer-run regression checks issue recovery state but not the already-created GitHub delivery.
  • Move gate terminalization after the locked newer-execution check, or atomically claim applicability before inserting the delivery, and assert that the stale-run case creates no delivery.
• [errors/tests] server/src/services/heartbeat.ts:16006 — A failed Job can be rewritten to pr_review_output_missing or pr_review_verification_unavailable while durable metadata records adapterInvocationStarted: true. shouldScheduleAutomaticRunRetry then refuses replay at heartbeat.ts:903-909, but the new gate-failure path at heartbeat.ts:22710 handles only job_missing and k8s_pod_schedule_failed, leaving these non-retryable review outcomes without a review or terminal status.
  • Centralize terminal non-retryable PR-review classification for gate delivery and add cases for both review-evidence codes with adapterInvocationStarted: true.
• [gstack/review] server/src/services/recovery/service.ts:4886 — The locked review-stage claim commits status = blocked, then recovery-action creation, wake enqueueing, reassignment, and blocker updates occur in a second transaction at service.ts:4901-4969. A failure or process exit between those phases leaves a blocked issue with no recovery action or wake, and the normal sweep excludes blocked issues at service.ts:5476-5484, so it cannot self-repair.
  • Keep the claim and durable recovery action in one transaction, or persist a repairable intermediate state that reconciliation includes; add failure-injection coverage between claim and action creation.

Suggestions (0)

Strengths

• Missing Jobs retain authoritative lifecycle and adapter-invocation evidence instead of being misclassified as safe to replay.
• Accepted-interaction recovery is correlated by issue and interaction across assignee changes.
• Review-stage recovery now validates stage, participant, and execution ownership under a row lock.

Recommended Action

1. Prevent stale runs from terminalizing the PR gate before execution ownership validation.
2. Terminalize every non-retryable PR-review outcome, not only the two external-lifecycle codes.
3. Remove or make repairable the gap between blocking a review stage and creating its recovery surface.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

@ally please re-review at head 495685dd1 with focus on the three findings from the 83d32767e review: stale-run gate delivery ordering, terminal status coverage for non-retryable evidence outcomes, and the review-escalation transaction gap.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:f65d913 important 2 - still-present - server/src/services/heartbeat.ts:23487 - The newer-execution check now precedes gate terminalization, but its transaction commits before the failure delivery is queued. A replacement review can claim the issue in that gap, after which the older terminal run still inserts a red status for the same head.
• prior:83d3276 important 2 - fixed - server/src/services/heartbeat.ts:2189 - The shared non-retryable classifier now includes invocation-started pr_review_output_missing and pr_review_verification_unavailable, and server/src/services/heartbeat.ts:23489 routes those outcomes through terminal gate delivery.
• prior:83d3276 important 3 - still-present - server/src/services/recovery/service.ts:4904 - The stage check is syntactically inside a transaction, but action creation, wake enqueueing, and the issue update still use services bound to the outer database connection. The code explicitly confirms this at server/src/services/recovery/service.ts:4088, so the fence and side effects do not share the transaction.

Critical Issues (0)

Important Issues (2)

• [prior:f65d913 important 2] server/src/services/heartbeat.ts:23487 - Gate terminalization still has a post-check race and crash window. releaseIssueExecutionAndPromote commits before queueFailedPrReviewGateStatus; a newer execution can claim the issue between those operations, or the process can exit before any durable delivery row exists. The regression at server/src/__tests__/heartbeat-process-recovery.test.ts:6295 covers only a newer run that already existed before the transaction.
  • Persist a stage/execution-fenced delivery intent in the same transaction as the applicability decision, or make delivery revalidate that no newer matching execution exists before publishing.
• [prior:83d3276 important 3] server/src/services/recovery/service.ts:4849 - Review-stage escalation remains non-atomic. The transaction holds an advisory lock, but normal stage transitions need not take that lock; after the check at service.ts:4876, outer-connection calls create the action and wake, then issuesSvc.update at service.ts:4940 can block a newly advanced stage. Partial failures can also leave committed action/wake state without the intended issue transition.
  • Thread tx through recovery-action creation, wake persistence, comments, and issuesSvc.update, and keep a row lock or conditional stage/participant/execution-owner update until all durable recovery state is committed. Add a race/failure-injection regression across the fence and issue update.

Suggestions (0)

Strengths

• Non-retryable review-evidence outcomes now share one classifier and terminal status path.
• An already-active newer execution suppresses the stale run's gate delivery, and the regression asserts no delivery is created in that case.
• Recovery action creation now precedes the blocking update, avoiding the earlier permanently blocked-without-action ordering.

Recommended Action

1. Make PR gate terminalization atomic or durably fenced against newer executions.
2. Execute review-stage validation and all recovery mutations on the same transaction.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

@ally please re-review exact head 646cd31fc with focus on the two prior important findings: gate-delivery outbox atomicity under the issue lock, and review-stage escalation fencing/transaction scope.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:f65d913 important 2 — fixed — server/src/services/heartbeat.ts:22879 — The terminal gate-delivery intent is now inserted through the same transaction that holds the issue row lock and validates executionRunId; a newer execution cannot claim the issue between applicability validation and the durable outbox write.
• prior:83d3276 important 3 — fixed — server/src/services/recovery/service.ts:4914 — Review-stage escalation now routes recovery-action creation, blocker reads, the blocking issue update, and comments through the row-locking transaction; wake dispatch is deliberately deferred until that durable state commits.

Critical Issues (0)

Important Issues (2)

• [gstack/review] server/src/services/recovery/service.ts:5215 — Review-stage wake dispatch is not recoverable after commit. The transaction has already moved the issue to blocked and committed the recovery action before enqueueSourceScopedStrandedRecoveryWake runs. If enqueue throws or returns no delivery, the attempt is refunded, but reconcileStrandedAssignedIssues only scans todo, in_progress, and in_review at service.ts:5504-5518; there is no dispatcher for active actions on blocked issues. The comment at service.ts:5202-5206 therefore promises a next-sweep retry that cannot occur.
• [code/errors] server/src/services/recovery/service.ts:5144 — The review-stage transaction still calls logActivity(db, ...) through the outer connection and publishes immediately. If the enclosing escalation transaction later rolls back or fails to commit, the activity row and live/plugin event survive as a phantom issue.updated escalation even though the recovery action, comment, and blocked transition did not commit.

Suggestions (0)

Strengths

• Gate failure delivery is now durably coupled to the issue-lock ownership decision.
• Review-stage validation, recovery-action creation, issue mutation, and comments share one row-locked transaction.
• The stale-run regression asserts that a run already superseded before finalization creates no gate delivery.

Recommended Action

1. Add a durable wake outbox/dispatcher or include blocked issues with active recovery actions in a bounded retry path.
2. Write activity through the escalation transaction with deferred publication after commit.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval is possible.