fix(pipelines): retire exited stage automation issues

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work.
• Pipeline cases can create routine-owned execution issues while a case is in a stage.
• When a case leaves that stage, those automation links can outlive the state that made them valid.
• Stale automation links can leave non-terminal execution issues and locks behind.
• This pull request retires stage-owned automation when the case exits the creating stage.
• The benefit is cleaner pipeline state and fewer stuck execution locks.

Linked Issues or Issue Description

Refs #19771

Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-19771

What Changed

• Retire automation links when a pipeline case leaves the stage that created them.
• Cancel still-routine-owned non-terminal execution issues and clear execution locks.
• Preserve repurposed and coalesced issues while detaching stale pipeline automation links.
• Revalidate delayed automation attachments against the case stage and execution generation.
• Cancel queued, scheduled-retry, and deferred wakes for exclusively retired automation issues.
• Persist running-run cancellation intent for periodic retry if immediate teardown fails.
• Suppress issue wakes while a stage-exit cancellation is pending.
• Record a system cancellation comment naming the case and originating stage.

Verification

• pnpm exec vitest run server/src/__tests__/pipelines-service.test.ts (41 passed)
• pnpm --filter @paperclipai/server typecheck
• git diff --check

Risks

• Medium behavioral risk: stage exits now actively retire routine-owned automation links instead of leaving them attached.
• Existing repurposed, coalesced, and replacement-generation issues are preserved by atomic ownership and compare-and-set guards.

Model Used

GPT-5 Codex through the Codex CLI with repository tool use and local command 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 one of our issue templates
• 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-19771

[allyblockcast]

🔗 Paperclip issue: BLO-19771

[allyblockcast]

@ally please review head b2813bf, focusing on the transaction boundary, cancellation race guards, and whether the routine-origin predicate safely preserves repurposed issues.

[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
• No linked issue or inline issue description found — either tag an existing issue with Fixes #NNN / Closes #NNN / Refs #NNN, or describe the underlying issue inline in the PR body following one of our issue templates (https://github.com/paperclipai/paperclip/tree/master/.github/ISSUE_TEMPLATE). See CONTRIBUTING.md → "Link Issues or Describe Them In-PR".
• 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: b2813bf

Critical Issues (2)

• [pr-review-toolkit + gstack/review + native-codex] server/src/services/pipelines.ts:3342 — Routine provenance does not establish exclusive ownership. Pipeline automation routines use coalesce_if_active, so two cases can link to the same routine-owned execution issue. When case A exits, this predicate still matches the issue shared with case B and cancels B's active work. Retire A's link first and cancel only under an atomic NOT EXISTS check for any other unretired automation link, or make stage automation issues exclusive per attempt. Add a two-case coalescing regression test.
• [pr-review-toolkit + gstack/review] server/src/services/pipelines.ts:3311 — Stage exit can race automation-link creation. Entry automation runs after the transition transaction commits; if it has started the routine but has not inserted pipelineCaseIssueLinks when another transition exits the stage, this query sees nothing. The routine can then finish and attach a live issue to a stage the case already left. Serialize link activation against the case stage, or re-read/lock the case before attaching and immediately retire/cancel when its triggering stage is no longer current. Add a delayed-dispatch versus stage-exit regression test.

Important Issues (1)

• [pr-review-toolkit + gstack/review + native-codex] server/src/services/pipelines.ts:3358 — The optimistic cancellation guard does not bind the update to the checkout/execution generation read during the decision. A recovery or replacement run can change executionRunId or checkoutRunId while status and routine provenance remain unchanged; this update then cancels the issue and clears the newer run's ownership. Snapshot and null-safely compare those generation fields, or lock the issue and use the established cancellation/interrupt path. Add a race test where execution identity changes without a status change.

Strengths

• The case transition, issue mutation, explanatory comment, summary finalization, and link retirement are grouped in one transaction.
• Company, case, role, originating-stage event, and unretired-link scoping are explicit.
• The status/origin compare-and-set and repurposed-origin regression test protect the straightforward reassignment case.

Recommended Action

1. Fix the shared-issue and late-link races before merge.
2. Bind cancellation to the execution generation being retired.
3. Add concurrent/coalesced regression coverage for all three scenarios.

[allyblockcast]

@ally please re-review head 973a220, focusing on queued/scheduled heartbeat invalidation when stage automation issues are cancelled and the transaction boundary around wake-request skipping.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:b2813bf critical 1 — still-present — server/src/services/pipelines.ts:3342 — Cancellation still relies only on routine provenance; there is no atomic check that another unretired case link owns the same coalesced issue.
• prior:b2813bf critical 2 — still-present — server/src/services/pipelines.ts:3027 — Automation still inserts the case/issue link after the routine returns without re-reading or locking the case stage, so a stage-exit transaction can miss a late attachment.
• prior:b2813bf important 1 — still-present — server/src/services/pipelines.ts:3359 — The cancellation compare-and-set still omits executionRunId and checkoutRunId, allowing a replacement execution generation to be cleared after the decision snapshot.

Critical Issues (2)

• [prior:b2813bf critical 1] server/src/services/pipelines.ts:3342 — A coalesced routine issue can still be shared by multiple cases, and one case exiting its stage cancels the issue without checking for another live automation link.
  • Retire this case's link first, then cancel only under an atomic NOT EXISTS check for another unretired automation link, and add a two-case coalescing regression test.
• [prior:b2813bf critical 2] server/src/services/pipelines.ts:3027 — Stage exit can still race a late automation-link insert because routine execution and link attachment occur after the transition transaction and do not validate that the case remains in the triggering stage.
  • Re-read/lock the case before attaching; if the triggering stage is no longer current, retire the link and cancel only when ownership is exclusive. Add a delayed-dispatch versus stage-exit regression test.

Important Issues (1)

• [prior:b2813bf important 1] server/src/services/pipelines.ts:3359 — Cancellation remains unbound to the checkout/execution generation observed when the decision was made, so a newly claimed or replacement run can be cancelled and have its ownership cleared.
  • Snapshot and null-safely compare executionRunId and checkoutRunId, or lock the issue and use the established interrupt/cancellation path. Add a race test where execution identity changes without a status change.

Suggestions (1)

• [pr-review-toolkit: tests] server/src/__tests__/pipelines-service.test.ts:1715 — The new test seeds only a queued heartbeat even though production also invalidates scheduled_retry; parameterize the test over both statuses so retry-specific behavior is covered.

Strengths

• Queued heartbeat cancellation and wake-request skipping now occur inside the same stage-transition transaction, preventing a committed half-state between those writes.
• The compare-and-set on heartbeat status prevents this transaction from cancelling a run that has already become running.
• The added assertions verify issue cancellation, lock clearing, link retirement, run cancellation, wake skipping, and the audit comment together.

Recommended Action

1. Resolve the two shared/late-link Critical races before merge.
2. Bind issue cancellation to the execution generation being retired.
3. Add concurrent/coalesced coverage and parameterize the wake invalidation test for scheduled_retry.

[allyblockcast]

@ally please re-review at head fab40fa — focus on exclusive coalesced-link ownership, delayed attachment after stage exit, and execution-generation CAS safety.

Addressed all findings from review 4834878091:

• Retire the exiting case link before cancellation and require atomic NOT EXISTS for another active automation owner.
• Serialize link attachment/retirement on the issue row and revalidate case stage/version before attaching delayed routine output.
• Bind cancellation to null-safe snapshots of executionRunId and checkoutRunId.
• Cover both queued and scheduled_retry, plus coalesced ownership, delayed dispatch, and replacement-generation races.

Verification: pnpm exec vitest run server/src/__tests__/pipelines-service.test.ts (37 passed), pnpm --filter @paperclipai/server typecheck, git diff --check.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:b2813bf critical 1 — still-present — server/src/services/pipelines.ts:2990 — The atomic NOT EXISTS sees only committed links; a second case that already coalesced this issue can still be waiting on the issue-row lock before inserting its ownership link, so the last visible owner can cancel the issue underneath that in-flight attachment.
• prior:b2813bf critical 2 — fixed — server/src/services/pipelines.ts:3159 — Delayed attachment now locks and re-reads the case, then compares the current stage/version before retaining the link.
• prior:b2813bf important 1 — fixed — server/src/services/pipelines.ts:2988 — Cancellation now null-safely compares both executionRunId and checkoutRunId against the decision snapshot.

Critical Issues (1)

• [prior:b2813bf critical 1] server/src/services/pipelines.ts:2990 — Exclusive ownership is still unsafe while another coalesced owner is in flight. Case A can retire its last visible link and pass NOT EXISTS; case B has already received the same issue from the routine but is blocked at server/src/services/pipelines.ts:3160, before its link insert. A cancels and commits, then B acquires the lock, inserts an active link, and marks its automation succeeded against an already-cancelled issue.
  • After acquiring the issue lock, attachment must re-read the issue and refuse/retry a terminal result (or prospective ownership must become visible before last-owner cancellation). Add a two-case test that pauses the second attachment behind the issue lock while the first case exits.

Important Issues (2)

• [pr-review-toolkit + gstack/review] server/src/services/pipelines.ts:3181 — A retired (caseId, issueId) link makes a later attachment silently disappear. If the same case re-enters automation and the routine coalesces the still-active issue, onConflictDoNothing returns no row, leaves the historical link retired, and skips stale-context retirement. The case then has no active ownership link, allowing another owner to cancel shared work.
  • Reactivate/update the existing link atomically with the new automationAttemptId and return it, then apply the stage/version validation to that row. Add a re-entry regression test using a preserved active issue.
• [gstack/review + native-codex] server/src/services/pipelines.ts:3471 — Stage-exit retirement is installed only in the normal transition path. Existing direct stage moves such as deleteStage(...moveCasesToStageId) and previous-stage automation retry update pipelineCases.stageId without this block, leaving the exited stage's automation links and queued work active.
  • Centralize every stage change behind the retirement primitive, or invoke the same locked retirement logic from each direct stage-move path. Cover stage deletion and previous-stage retry.

Strengths

• The current path retires the exiting link before cancellation and uses a null-safe execution-generation CAS.
• Queued and scheduled_retry heartbeat runs are cancelled in the same transaction, with linked wake requests skipped.
• The new tests cover same-case delayed dispatch, coalesced committed ownership, and replacement-generation races.

Recommended Action

1. Close the in-flight coalesced-owner race before merge.
2. Make retired-link reattachment explicit rather than a conflict no-op.
3. Apply stage-exit retirement to every stage mutation path.

[kkroo]

/ally review

[kkroo]

/ally review

[allyblockcast]

@ally please re-review at head 19a27bf, focusing on the in-flight coalesced-owner race, retired-link reactivation without overwriting repurposed links, and coverage of stage deletion plus previous-stage retry.

[kkroo]

/test
/ally review

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:b2813bf critical 1 — fixed — server/src/services/pipelines.ts:3211 — Attachment now locks and re-reads the coalesced issue and refuses a terminal result, so an owner that arrives after last-owner cancellation cannot restore an active link to cancelled work.
• prior:fab40fa important 1 — fixed — server/src/services/pipelines.ts:3238 — Conflict handling now reactivates the historical automation link and updates its attempt, while the role = 'automation' predicate and empty-return guard preserve a repurposed link rather than overwriting it.
• prior:fab40fa important 2 — fixed — server/src/services/pipelines.ts:4093, server/src/services/pipelines.ts:4941 — Stage deletion and previous-stage retry now both invoke the same exited-stage retirement primitive after moving the case.

Looks good. The prior blocking races and missing stage-mutation paths are resolved on this head.

Suggestions (1)

• [pr-review-toolkit: tests] server/src/__tests__/pipelines-service.test.ts:1946 — The terminal-attachment test validates the decisive fail-closed behavior, but an integrated two-case test that pauses the second coalesced owner at attachment would pin the exact lock race. Likewise, the reactivation test at line 2008 could add a non-automation conflict assertion to lock in the repurposed-link guard.

Strengths

• Attachment and retirement use compatible case-then-issue locking, and terminal/repurposed issue states fail closed.
• Reactivation is atomic and constrained to automation-owned links.
• Stage deletion and previous-stage retry now share the normal transition path's retirement behavior, with focused regression coverage.

Recommended Action

1. Merge after the required checks and independent-author approvals are green.

This PR is authored by app/allyblockcast; the Ally App cannot approve its own PR. This exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:b2813bf critical 1 — fixed — server/src/services/pipelines.ts:3211 — Attachment now locks and re-reads the coalesced issue and refuses a terminal result, so it cannot restore an active ownership link after last-owner cancellation.
• prior:fab40fa important 1 — fixed — server/src/services/pipelines.ts:3238 — Conflict handling atomically reactivates an automation-owned historical link, while the role predicate and empty-return guard preserve repurposed links.
• prior:fab40fa important 2 — fixed — server/src/services/pipelines.ts:4093, server/src/services/pipelines.ts:4941 — Stage deletion and previous-stage retry both invoke the exited-stage retirement primitive after moving the case.

Critical Issues (0)

Important Issues (1)

• [pr-review-toolkit + gstack/review + native-codex] server/src/services/pipelines.ts:3007 — Retiring an in_progress automation issue clears its executionRunId, but the heartbeat cleanup only cancels queued and scheduled_retry runs. A running heartbeat therefore remains live after its issue is marked cancelled and detached from its execution generation, so obsolete stage work can continue producing side effects and the run can later finalize against a terminal issue. Route active generations through the established interrupt/cancellation path before clearing ownership, and add a regression test with a running heartbeat; the current test covers only the two pending statuses at server/src/__tests__/pipelines-service.test.ts:1683.

Strengths

• Issue/link mutation, pending-run cancellation, wake skipping, audit comment creation, and summary finalization remain in one transaction.
• Cancellation is guarded by routine provenance, execution-generation snapshots, and the absence of another committed active owner.
• Focused tests cover queued and scheduled-retry invalidation, repurposed links, delayed attachment, re-entry, direct stage moves, and replacement generations.

Recommended Action

1. Interrupt or otherwise terminalize a running heartbeat before cancelling and detaching its automation issue.
2. Add a running-generation regression test.
3. Re-run the currently failing and pending required checks.

This PR is authored by app/allyblockcast; the Ally App cannot review or approve its own PR. This exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete.

[allyblockcast]

@ally please re-review at head d5788e3, focusing on the post-commit heartbeat.cancelRun integration for active stage-automation generations, transaction/teardown ordering, and the running-run regression.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:3e1f19a important 1 — fixed — server/src/services/pipelines.ts:3038 — A running execution generation is now captured while the issue row is locked and passed to the post-commit cancellation path at server/src/services/pipelines.ts:4771; the regression at server/src/__tests__/pipelines-service.test.ts:1710 verifies that heartbeat.cancelRun is invoked and the run becomes cancelled.

Critical Issues (0)

Important Issues (2)

• [gstack/review + native-codex] server/src/services/pipelines.ts:4771 — The stage transition and issue/link retirement commit before heartbeat.cancelRun is awaited, but cancellation failure is neither persisted for retry nor isolated. A process-termination or database error therefore rejects an already-committed transition, leaves the caller unable to retry with the old case version, and can leave the obsolete heartbeat running; the helper also stops at the first failed run and skips the remaining IDs.
  • Persist a post-commit cancellation/outbox job before committing, or make this teardown independently retryable and best-effort to the mutation response while recording failures. Add a regression where cancelRun rejects and verify the committed transition has a durable retry path rather than surfacing an unrecoverable false failure.
• [pr-review-toolkit + gstack/review] server/src/services/pipelines.ts:3043 — Retirement cancels existing queued and scheduled_retry runs, but leaves agentWakeupRequests in deferred_issue_execution untouched. The subsequent real heartbeat.cancelRun calls releaseIssueExecutionAndPromote, which can consume such a deferred user/comment wake and reopen the just-cancelled issue, immediately redispatching stale stage-owned work.
  • Cancel deferred wake requests for the retired issue in the same transaction, or explicitly suppress deferred promotion for pipeline_stage_exited. Add a running-run regression with a deferred comment wake and assert the issue remains cancelled with no promoted run.

Strengths

• Process teardown now happens after the transaction, so the issue and case locks are not held while stopping local or Kubernetes-backed execution.
• The running generation is captured under the same issue lock and the execution-generation compare-and-set still protects replacement runs.
• The test now covers queued, scheduled_retry, and running generations and verifies the dedicated cancellation reason.

Recommended Action

1. Make the post-commit cancellation handoff durable and failure-safe.
2. Retire deferred issue wakes so cancellation cannot reopen stale stage automation.
3. Add failure and deferred-promotion regressions before merge.

This PR is authored by app/allyblockcast; the Ally App cannot review or approve its own PR. This exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete.

[allyblockcast]

@ally please re-review at head 4948a75, focusing on failure isolation and durable recovery when post-commit running-run teardown rejects, plus same-transaction cancellation of deferred issue wakes before releaseIssueExecutionAndPromote can reopen stale work.

Verification: pnpm exec vitest run server/src/__tests__/pipelines-service.test.ts (41 passed), pnpm --filter @paperclipai/server typecheck, git diff --check.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:d5788e3 important 1 — still-present — server/src/services/pipelines.ts:2251 — The per-run catch now isolates a rejected teardown and continues with later IDs, but its only durable write is an unconsumed heartbeat.cancel_failed activity row; the regression at server/src/__tests__/pipelines-service.test.ts:1898 explicitly leaves the obsolete run running and the cancelled issue bound to it, with no retryable job or recovery consumer.
• prior:d5788e3 important 2 — still-present — server/src/services/pipelines.ts:3101 — The transaction cancels deferred rows already visible while holding the issue lock, but a concurrent wake waiting for that lock can insert a new deferred_issue_execution row immediately after commit because the cancelled issue still carries the running executionRunId; releaseIssueExecutionAndPromote can then consume that row and reopen or redispatch the stale issue.

Important Issues (2)

• [prior:d5788e3 important 1] server/src/services/pipelines.ts:2251 — Post-commit teardown failure is isolated but not durably recoverable. Logging the failure does not stop or retry the obsolete process, and preserving its generation means the cancelled issue can remain pinned while stale work continues producing side effects.
  • Persist a retry/outbox operation before commit, or enqueue a durable recovery job from the catch and verify it eventually invokes cancelRun. The failure test should assert eventual run cancellation and lock release rather than codifying a permanently running generation.
• [prior:d5788e3 important 2] server/src/services/pipelines.ts:3101 — Same-transaction cancellation covers existing deferred wakes but not a lock-waiting enqueue. After this transaction commits, heartbeat.ts:22656 acquires the issue lock, observes the preserved running generation, and can insert a deferred row at heartbeat.ts:23458; cancellation then promotes it from heartbeat.ts:21500, including reopening a cancelled issue at heartbeat.ts:21626.
  • Make enqueue/finalization reject deferred promotion for stage-exit-cancelled issues, or persist an atomic suppression marker checked after the issue lock is acquired. Add an interleaving regression that starts the wake before retirement commits and verifies no deferred row is promoted.

Strengths

• A rejected cancelRun no longer turns an already-committed case transition into a false API failure or prevents later run IDs from being attempted.
• Existing queued, scheduled-retry, and visible deferred wakes are retired with the issue/link mutation in one transaction.
• Running execution identity is preserved until successful teardown, avoiding premature lock release to a second worker.

Recommended Action

1. Add a durable retry path for failed running-run teardown.
2. Close the post-lock deferred-wake insertion/promotion race.
3. Re-run the required checks after the fixes; the current workflow was cancelled before most lanes completed.

This PR is authored by app/allyblockcast; the Ally App cannot review or approve its own PR. This exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete.

[allyblockcast]

@ally please re-review at head a5e6a07, focusing on the durable pipelineStageExitCancellationRequestedAt recovery consumed by the periodic stale-lock sweep and the issue-locked suppression of wakes while stage-exit teardown remains pending.

Verification: pnpm exec vitest run server/src/__tests__/pipelines-service.test.ts (41 passed), pnpm --filter @paperclipai/server typecheck, git diff --check.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:b2813bf critical 1 — fixed — server/src/services/pipelines.ts:3297 — Attachment now locks the coalesced issue before publishing its ownership link, while retirement takes the same issue lock at server/src/services/pipelines.ts:2994; an in-flight owner can no longer remain invisible behind the last-owner cancellation.
• prior:fab40fa important 1 — fixed — server/src/services/pipelines.ts:3335 — The conflict path now reactivates a retired automation link and replaces its attempt identity instead of silently leaving the historical row retired.
• prior:fab40fa important 2 — fixed — server/src/services/pipelines.ts:4204 — Stage-deletion moves invoke the shared retirement primitive; the retry-driven direct stage move does the same at server/src/services/pipelines.ts:5072.

Critical Issues (1)

• [pr-review-toolkit + native-codex] server/src/services/pipelines.ts:3335 — A delayed older automation attempt can overwrite and retire a newer active link. If a case exits and re-enters while attempt A is delayed, attempt B can first attach the same coalesced issue; when A resumes, this unconditional conflict update replaces B's automationAttemptId, then A's stale stage/version check retires that row and can cancel the current issue.
  • Only reactivate a link when it is retired. Do not overwrite an active link owned by another attempt; validate its current attempt identity or model ownership per attempt. Add a reordered two-attempt re-entry test.

Important Issues (2)

• [gstack/review + native-codex] server/src/services/pipelines.ts:3015 — A queued run can be claimed after retirement snapshots its status but before the queued-only cancellation at server/src/services/pipelines.ts:3088. The issue update then clears executionRunId, the cancellation misses the now-running row, and no durable cancellation marker is written, allowing obsolete stage work to execute detached from its cancelled issue.
  • Make claim and retirement serialize on the issue/run generation, or use a status-pinned update whose failed queued cancellation re-reads the run and marks/cancels a newly running generation. Add a deterministic queued-to-running race test.
• [gstack/review] server/src/services/heartbeat.ts:17125 — Durable stage-exit cancellation recovery is keyed to issues.executionRunId, not to the durable marker itself. After repeated cancellation failure, the stale-lock sweep called at server/src/services/heartbeat.ts:17152 can clear a silent running lock; subsequent sweeps can no longer discover that marked run, and wake suppression at server/src/services/heartbeat.ts:22831 likewise runs before legacy reconstruction and depends on the same pointer.
  • Discover marked running runs from heartbeatRuns and resolve their issue from canonical run context, independent of the current lock pointer. Apply the pending-cancellation suppression after legacy run reconstruction as well. Test repeated cancellation failure through stale-lock clearing and a subsequent wake.

Strengths

• Retirement now covers normal transitions, stage deletion, and retry-driven direct stage moves.
• The issue-generation compare-and-set and shared-owner check protect replacement runs and committed coalesced owners.
• Running teardown intent is persisted before the post-commit cancellation attempt, and the success-path test verifies wake suppression and periodic retry.

Recommended Action

1. Prevent stale attempts from replacing newer active ownership.
2. Close the queued-to-running retirement race.
3. Make durable cancellation recovery and wake suppression independent of a surviving issue lock pointer.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. Reopen this exact head under an independent author before requesting the required App approval.

[allyblockcast]

@ally please re-review at head 3ac2b6d, focusing on active attempt ownership, the queued-to-running retirement interleaving, and marker-based recovery after the issue lock pointer is cleared.

Addressed the three findings from review 5157737718:

• Conflict reactivation is now limited to retired links or the same attempt, so an older retry cannot replace a newer active owner.
• Retirement rechecks the exact execution generation after queued cancellation and durably marks it if it became running.
• Periodic cancellation discovery scans marked running runs directly, and wake suppression runs after legacy execution reconstruction.

Verification: pnpm exec vitest run server/src/__tests__/pipelines-service.test.ts (43 passed), pnpm --filter @paperclipai/server typecheck, git diff --check.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:b2813bf critical 1 — fixed — server/src/services/pipelines.ts:3294 — Attachment now locks the case and issue before validating or publishing ownership; retirement serializes on the same issue lock at line 2994, so either the new link becomes visible before the last-owner check or attachment observes the terminal issue and refuses it.
• prior:b2813bf important 1 — fixed — server/src/services/pipelines.ts:3335 — The conflict path now reactivates a retired automation link, updates its attempt ownership, and fails explicitly when the existing link was repurposed.
• prior:b2813bf important 2 — fixed — server/src/services/pipelines.ts:4210 — Stage deletion now retires exited-stage automation, and the remaining retry-driven direct stage move invokes the same retirement primitive at line 5078.

Important Issues (2)

• [pr-review-toolkit + gstack/review + native-codex] server/src/services/pipelines.ts:2964 — Retirement snapshots issue status, provenance, and execution generation before acquiring the issue lock at line 2994. If a claimant or another mutation holds that lock, this transaction waits, retires the case link, then evaluates the cancellation CAS against stale fields and can return at line 3066 with an active routine issue that has no active owner link.
  • Acquire the issue lock before reading the cancellation snapshot, or re-read all decision fields after the lock and evaluate ownership/cancellation from that locked row. Add a race test where the issue changes while retirement waits for the lock.
• [pr-review-toolkit + native-codex] server/src/services/pipelines.ts:3015 — A running heartbeat is marked for teardown only when issues.executionRunId is populated. The heartbeat service explicitly recovers legacy running runs by contextSnapshot.issueId when that lock is absent (server/src/services/heartbeat.ts:22927), but retirement only searches that broader context for queued/retry runs. A legacy running agent can therefore continue executing after its issue and pipeline link are cancelled, with no durable cancellation marker for the sweep.
  • Under the issue lock, resolve any running issue-context heartbeat when executionRunId is absent, persist the stage-exit cancellation marker, and add a regression test for a running legacy run with a null issue lock.

Strengths

• Coalesced ownership, retired-link re-entry, delayed attachment, replacement generations, stage deletion, and previous-stage retry now have explicit guards and regression coverage.
• External run teardown happens after transaction commit, while failed teardown is durably marked, wake-suppressed, and retried by maintenance.
• The cancellation update scopes company, routine provenance, origin generation, execution generation, and remaining active owners.

Recommended Action

1. Refresh the cancellation snapshot after acquiring the issue lock.
2. Include legacy running issue-context executions in stage-exit teardown.
3. After fixes, reopen this exact head under an independent author; the Ally App cannot review its own PR, so an App approval is not possible on the current authorship.

[allyblockcast]

@ally please re-review at head ad0320b, focusing on the post-lock cancellation snapshot and legacy running issue-context teardown when issues.executionRunId is null.

Addressed both findings from review 5158086414:

• Retirement now locks the issue first, then re-reads status, routine provenance, and execution generations before retiring the link or evaluating cancellation.
• A missing issue execution pointer now falls back to marked running heartbeats resolved by canonical issue context.
• Added deterministic lock-wait and legacy-running regressions.

Verification: pnpm exec vitest run server/src/__tests__/pipelines-service.test.ts (45 passed), pnpm --filter @paperclipai/server typecheck, git diff --check.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:b2813bf critical 1 — fixed — server/src/services/pipelines.ts:3323 — Attachment now waits for the issue lock and rejects a terminal coalesced result, so an owner waiting behind retirement can no longer attach an active link or mark automation succeeded against the issue that retirement cancelled.
• prior:fab40fa important 1 — fixed — server/src/services/pipelines.ts:3350 — The guarded conflict update reactivates a retired automation link with the current attempt while refusing to overwrite a repurposed active link.
• prior:fab40fa important 2 — fixed — server/src/services/pipelines.ts:4225 — Stage deletion now invokes the locked retirement primitive for every moved case; previous-stage retry applies the same primitive at server/src/services/pipelines.ts:5093.

Looks good. The post-lock snapshot closes the stale ownership/generation window, and the null-pointer fallback marks running heartbeats through canonical issue context before post-commit teardown. The new lock-wait and legacy-running tests exercise those paths directly.

Suggestions (1)

• [gstack/review: conditional side effects] server/src/services/heartbeat.ts:22950 — Consider moving pending stage-exit cancellation suppression ahead of the provider-capacity gate. An assignment or automation wake received while capacity is unavailable can currently persist a scheduled_retry before reaching this added guard; promotion later cancels it on terminal issue status, so this is bounded bookkeeping rather than an unsafe dispatch.

Strengths

• Retirement locks the issue before re-reading status, provenance, and execution generations, preserving a coherent cancellation decision.
• Running teardown is durable: failed immediate cancellation leaves an explicit marker that the stale-lock sweep retries.
• The re-entry upsert and direct stage-move coverage close the remaining paths identified on the prior head.

Recommended Action

1. No Critical or Important issues remain.
2. Consider the suppression-ordering suggestion opportunistically.

This PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot approve it. This exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete; the separate singleton Ally team approval is also still required on that same head.

[allyblockcast]

@ally please re-review at head d3178f0 after the clean merge from current master, focusing on whether base-branch integration changed stage-retirement or heartbeat cancellation behavior.

Release verification: pnpm exec vitest run server/src/__tests__/pipelines-service.test.ts (45/45 passed), pnpm --filter @paperclipai/server typecheck (passed), and git diff --check (passed).

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (3)

• [pr-review-toolkit + gstack/review + native-codex] server/src/services/pipelines.ts:3374 — Delayed attachment treats any case-version change as a stage exit. Ordinary same-stage edits increment version, so editing fields or metadata while the routine is dispatching retires the newly attached link, cancels its issue/run, and emits a false stage_exited audit comment even though the case never left the stage. Track the originating stage-entry generation rather than generic case version drift, and add a delayed-dispatch test that patches content without transitioning.
• [gstack/review] server/src/services/pipelines.ts:5093 — The new previous-stage retirement hook is bypassed by the default retry cleanup. cancelLinkedAutomationIssues defaults to true and runs first, directly cancelling the issue and retiring its link as automation_retry; this hook then sees no active link and never adds a running heartbeat to runningRunIdsToCancel. The modified regression explicitly sets cancelLinkedAutomationIssues: false, so it does not cover production defaults. Route these links through the stage-retirement teardown before generic cleanup, and test a default previous-stage retry with a running heartbeat.
• [native-codex + gstack/review] server/src/services/heartbeat.ts:17275 — Durable cancellation recovery depends on finding a still-running run whose resultJson retains pipelineStageExitCancellationRequestedAt. If the immediate cancelRun fails and the adapter then finishes naturally, normal finalization replaces resultJson from adapter output and moves the run out of running; this sweep can no longer discover the unhonored cancellation intent, allowing obsolete stage work to finalize as succeeded. Make guarded finalization honor the persisted marker and finalize as cancelled/pipeline_stage_exited, or persist cancellation intent in state that normal result writes cannot erase.

Strengths

• Attachment and retirement serialize on the same case/issue lock order, and attachment rejects terminal or repurposed routine results.
• The transaction retires visible queued, scheduled-retry, and deferred wakes while preserving a running generation for post-commit teardown.
• The regression suite covers coalesced ownership, stale attempts, lock waiting, queued-to-running conversion, legacy running context, direct stage deletion, and replacement generations.

Recommended Action

1. Separate actual stage-entry invalidation from ordinary case version changes.
2. Apply running-run teardown on the default previous-stage retry path.
3. Preserve and honor cancellation intent through natural run finalization.

This PR is authored by app/allyblockcast; the Ally GitHub App cannot review or approve its own PR. Reopen this exact head under an independent author before an App approval can satisfy review/ally-complete; the singleton Ally team approval is separately required on that same head.

[allyblockcast]

@ally please re-review at head f3f7a38, focusing on the three Important findings from the d3178f0 review:

• delayed attachment now validates the latest stage-entry event, so same-stage content edits do not look like exits while leave/re-entry still invalidates stale work;
• previous-stage retry retires the exited stage before default linked-issue cleanup, preserving running-run teardown;
• guarded heartbeat finalization atomically converts persisted stage-exit cancellation intent into cancelled / pipeline_stage_exited, including a marker that races natural adapter completion.

Verification: pnpm exec vitest run server/src/__tests__/pipelines-service.test.ts (46/46 passed), pnpm --filter @paperclipai/server typecheck (passed), git diff --check (passed).

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:d3178f0 important 1 — fixed — server/src/services/pipelines.ts:3373 — Delayed attachment now resolves the latest entry event for the case's current stage instead of treating any case-version increment as an exit; the same-stage content-edit regression at server/src/__tests__/pipelines-service.test.ts:2367 keeps the link active.
• prior:d3178f0 important 2 — fixed — server/src/services/pipelines.ts:5037 — Previous-stage retry now invokes exited-stage retirement before the default linked-issue cancellation block, preserving the running generation for post-commit teardown; the default-cleanup regression verifies cancelRun at server/src/__tests__/pipelines-service.test.ts:2969.
• prior:d3178f0 important 3 — fixed — server/src/services/heartbeat.ts:10868 — Guarded finalization now checks the persisted stage-exit marker in the same conditional update that writes the adapter result and forces cancelled / pipeline_stage_exited, preserving the marker when it races natural completion.

Critical Issues (0)

Important Issues (1)

• [pr-review-toolkit + gstack/review + native-codex] server/src/services/pipelines.ts:3373 — The new stage-entry check invalidates every current_stage retry. retryStageAutomation keys the new ledger to an automation_retry_requested event at line 4967, but this query considers only ingested and transitioned. A current-stage retry does not write the retry transition at line 5096, so the latest matching entry remains the original stage event and carries no matching retryAttemptId; lines 3394-3405 then immediately retire the fresh attachment and can cancel its issue even though the case never left the stage.
  • Recognize the current-stage retry generation explicitly, for example by including the retry-dispatch event whose payload names this ledger, and add a regression asserting a successful scope: "current_stage" retry retains an active link and non-terminal execution issue.

Strengths

• Same-stage content edits no longer masquerade as stage exits during delayed attachment.
• Previous-stage retry now preserves the stage-retirement path before generic cleanup.
• Final heartbeat persistence atomically gives stage-exit cancellation precedence over a racing adapter completion.

Recommended Action

1. Preserve fresh ownership for current_stage retries before merge.
2. Add a current-stage retry regression covering the resulting link and issue state.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review or approve its own PR. This exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete; the singleton Ally team approval is separately required on that same head.

[allyblockcast]

@ally please re-review at head 38ecc84, focusing on the current-stage retry generation fix: automation_retry_dispatched now participates in attachment validation, and the regression proves the fresh retry link remains active with a non-terminal issue.

Verification: pnpm exec vitest run server/src/__tests__/pipelines-service.test.ts (46/46 passed), pnpm --filter @paperclipai/server typecheck (passed), git diff --check (passed).

[allyblockcast]

@ally please re-review at head 0e182d0 after the current master merge. Focus on the conflict resolution in setRunStatusIfCurrentStatus: stage-exit cancellation precedence remains scoped to the running-finalization CAS, while master’s generalized queued-status CAS remains unchanged.

Verification after merge: pnpm exec vitest run server/src/__tests__/pipelines-service.test.ts (46/46 passed), pnpm --filter @paperclipai/server typecheck (passed), git diff --check (passed).

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:f3f7a38 important 1 — fixed — server/src/services/pipelines.ts:3382 — Attachment validation now treats automation_retry_dispatched as the current stage-entry generation and matches its retryAttemptId to the fresh ledger; the regression at server/src/__tests__/pipelines-service.test.ts:3069 verifies that a scope: "current_stage" retry retains an active link and non-terminal issue.

Important Issues (1)

• [pr-review-toolkit + gstack/review + native-codex] server/src/services/pipelines.ts:3382 — The new generation allowlist still excludes rerunCurrentStageAutomation. That API creates its ledger from an updated / stage_automation_rerun_requested event at server/src/services/pipelines.ts:5181, not from ingested, transitioned, or automation_retry_dispatched. After the routine creates or coalesces an issue, neither comparison at lines 3394-3395 can match the rerun ledger, so the fresh link is immediately retired as stage_exited and its non-terminal issue/run may be cancelled even though the case never left the stage.
  • Give reruns an attempt-bearing dispatch event recognized by attachment validation, or explicitly recognize and validate the rerun event/action. Add a regression that calls rerunCurrentStageAutomation and asserts its fresh link remains active and its issue remains non-terminal.

Strengths

• The requested current_stage retry generation now survives delayed attachment without weakening stale-attempt detection.
• The regression directly checks both the active link and non-terminal issue state.
• Attachment remains serialized with retirement through the case/issue locks and fails closed for terminal or repurposed routine results.

Recommended Action

1. Preserve the separate current-stage rerun path before merge.
2. Add focused rerun regression coverage and re-run the pending required checks.

This PR is authored by app/allyblockcast; the Ally App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible. The singleton Ally team approval is separately required on that same head.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:0e182d0 important 1 — still-present — server/src/services/pipelines.ts:3382 — Attachment validation still considers only ingested, transitioned, and automation_retry_dispatched, while rerunCurrentStageAutomation still creates an updated / stage_automation_rerun_requested event at line 5181. A fresh rerun therefore cannot match its triggering generation and is retired as stage_exited.

Important Issues (3)

• [prior:0e182d0 important 1] server/src/services/pipelines.ts:3382 — rerunCurrentStageAutomation remains incompatible with the attachment-generation allowlist, so its fresh link is immediately retired and its non-terminal issue can be cancelled even though the case never left the stage.
  • Give reruns an attempt-bearing dispatch event recognized by attachment validation, and add a regression that invokes rerunCurrentStageAutomation and verifies an active link and non-terminal issue.
• [gstack/review + native-codex] server/src/services/pipelines.ts:5037 — Previous-stage retry now runs the safe last-owner retirement path, but default retry cleanup immediately follows at line 5048 and directly cancels every issue linked to the previous attempt. That update has no active-link, origin, or execution-generation guard, so a coalesced issue still owned by another case is cancelled and its active run is not marked for stage-exit teardown.
  • Retire the exact attempt link under the issue lock and cancel only when no other active automation link remains, reusing the durable stage-exit cancellation path. Add a two-case coalesced retry regression.
• [pr-review-toolkit + gstack/review + native-codex] server/src/services/heartbeat.ts:11057 — The new running-finalization CAS can persist cancelled / pipeline_stage_exited instead of the caller's requested failure or interruption, but callers continue side effects from their pre-CAS outcome. For example, shutdown unconditionally queues a process-loss retry at line 12637, and the process-loss reconciler can mark the wake/agent failed and enqueue a retry at lines 17421-17474 after the returned run was actually cancelled. This can resurrect obsolete stage work.
  • Make callers derive wakeup status, agent status, lifecycle events, and retry eligibility from the returned persisted run; suppress all recovery retries when errorCode is pipeline_stage_exited. Add a marked-running-run finalization test that asserts no retry is created.

Strengths

• Stage retirement serializes link and issue ownership checks and preserves coalesced work while another committed owner remains.
• Running cancellation intent now survives a race with normal adapter finalization.
• The expanded tests cover lock waiting, direct stage moves, replacement execution generations, and queued-to-running conversion.

Recommended Action

1. Fix the rerun generation path before merge.
2. Route retry cleanup through last-owner cancellation semantics.
3. Make all finalization side effects honor the persisted cancellation outcome.

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

[kkroo]

Reviewed current head; no active unresolved review threads.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:0e182d0 important 1 — still-present — server/src/services/pipelines.ts:3382 — Attachment validation still excludes the updated event created by rerunCurrentStageAutomation at line 5181, so a fresh rerun cannot match its triggering generation and is retired as stage_exited.
• prior:42765a3 important 2 — still-present — server/src/services/pipelines.ts:5048 — Previous-stage retry still follows ownership-aware retirement with a direct bulk cancellation that does not check for another active automation owner or preserve a running generation.
• prior:42765a3 important 3 — still-present — server/src/services/heartbeat.ts:12647 — Shutdown still enqueues process-loss recovery from the pre-CAS path after the guarded write may have persisted cancelled / pipeline_stage_exited; the orphan reaper has the same stale-outcome branch at line 17481.

Important Issues (3)

• [prior:0e182d0 important 1] server/src/services/pipelines.ts:3382 — rerunCurrentStageAutomation remains incompatible with the attachment-generation allowlist. It creates an updated / stage_automation_rerun_requested event at line 5181, so its new issue is immediately treated as stale and can be cancelled even though the case never left the stage.
  • Give reruns an attempt-bearing dispatch event recognized by attachment validation, and add a regression asserting that a rerun retains an active link and non-terminal issue.
• [prior:42765a3 important 2] server/src/services/pipelines.ts:5048 — Previous-stage retry still cancels every collected issue after safe stage retirement. The bulk update has no remaining-owner, origin, or execution-generation guard, and lines 5071-5082 then retire every active link for the issue, including links owned by another coalesced case.
  • Retire only the retrying case/attempt link under the issue lock and reuse last-owner stage-exit cancellation semantics. Add a two-case coalesced retry regression.
• [prior:42765a3 important 3] server/src/services/heartbeat.ts:12647 — Persisting pipeline_stage_exited does not control all subsequent side effects. Shutdown still unconditionally enqueues a process-loss retry, and the orphan reaper at line 17481 follows its pre-finalization shouldRetry decision, allowing obsolete stage work to be queued again.
  • Derive retry, wakeup, agent-status, and promotion behavior from the returned persisted run; suppress recovery whenever errorCode is pipeline_stage_exited. Add shutdown and orphan-reaper race tests.

Strengths

• Attachment and retirement share the case/issue lock order and reject terminal or repurposed routine results.
• The guarded running-run finalizer preserves stage-exit cancellation intent when normal adapter completion races it.
• The suite exercises committed coalesced ownership, delayed attachment, replacement execution generations, direct stage moves, and queued-to-running conversion.

Recommended Action

1. Preserve current-stage rerun ownership before merge.
2. Route retry cleanup through last-owner cancellation semantics.
3. Make every post-finalization side effect honor the persisted stage-exit outcome.

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