fix(heartbeat): gate timer skip predicate on checkout availability (BLO-19749)

[kkroo]

Thinking Path

• Paperclip agents coordinate issue ownership through checkout and execution-run locks.
• Timer recovery should skip issues only when a live checkout/execution holder means the issue is not currently available.
• BLO-19749 fixes a drift where timer predicates and active-run hydration did not agree with checkout availability, especially around scheduled_retry holders.
• The app-authored PR added the runtime predicate and tests, but Ally found the companion Paperclip skill guidance overstated what an issue response can prove.
• A GET /api/issues/{id} response exposes lock IDs, but not authoritative lifecycle status for every possible checkout-only holder.
• This pull request keeps checkout as the authoritative ownership decision and narrows the pre-check to only recognizing the current run's own lock IDs.
• The benefit is that agents stop safely on checkout conflicts without falsely reporting duplicate live execution from opaque lock IDs.

Linked Issues or Issue Description

• Refs BLO-19749: https://paperclip.blockcast.net/BLO/issues/BLO-19749
• Refs BLO-19126: https://paperclip.blockcast.net/BLO/issues/BLO-19126
• Supersedes app-authored PR fix(heartbeat): gate the timer skip predicate on checkout availability (BLO-19749) #1059 and addresses Ally's latest Important finding: fix(heartbeat): gate the timer skip predicate on checkout availability (BLO-19749) #1059 (comment)
• Original reviewed head: 5741430341f1022f330979b4019884c183141270
• Replacement rebased head: 616e11f79ee30894ce2a1bf78023b977a955b107
• Related PR search: gh pr list --repo Blockcast/paperclip --state open --search "BLO-19749" --limit 20 returned only fix(heartbeat): gate the timer skip predicate on checkout availability (BLO-19749) #1059 before this replacement.

What Changed

• Preserved fix(heartbeat): gate the timer skip predicate on checkout availability (BLO-19749) #1059's runtime changes that gate timer skip behavior on checkout availability.
• Updated skills/paperclip/SKILL.md so agents use the issue response only to detect their own PAPERCLIP_RUN_ID in checkoutRunId or executionRunId.
• Removed guidance that inferred a live competing run from non-matching lock IDs without authoritative run status.
• Clarified that checkout success means the agent owns the issue, while 409 Issue checkout conflict means stop and cede without retrying.
• Clarified that agents should report a checkout conflict without claiming a live competing run unless the response identifies a non-terminal holder.

Verification

• git diff --check origin/master..HEAD -- skills/paperclip/SKILL.md — passed after rebase.
• Original fix(heartbeat): gate the timer skip predicate on checkout availability (BLO-19749) #1059 reviewed head had full PR CI green before this docs-only follow-up, including typecheck, build, e2e, server shards, workspace shards, Helm chart, and serialized server suites.
• I did not rerun server tests for this follow-up because the only new change is Markdown skill guidance.

Risks

• Low risk: docs-only skill guidance change.
• Behavioral impact for agents is intentional: on 409, they stop without retrying and avoid falsely claiming another live run exists unless the API provides that fact.
• No runtime code, schema, UI, or alerting changes in the follow-up commit.

Model Used

Claude Sonnet 5 (claude-sonnet-5[1m]) for the original app-authored implementation, plus OpenAI GPT-5 via Codex CLI for the review-fix commit and independent PR reopening workflow.

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 — N/A for docs-only follow-up; original runtime PR has tests
• If this change affects the UI, I have included before/after screenshots — N/A, no UI change
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending on replacement head
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending Ally review on independent-authored PR
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-19126
🔗 Paperclip issue: BLO-19749

[allyblockcast]

🔗 Paperclip issue: BLO-19126
🔗 Paperclip issue: BLO-19749

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[kkroo]

@ally please review head 5a09cf9.

This replaces app-authored #1059 and addresses the Important skill-guidance finding from #1059 (comment). Focus: the pre-check now only recognizes this run own lock IDs, checkout remains authoritative, and 409 handling no longer claims a live competing run unless the response proves one.

[kkroo]

@ally please review rebased head 616e11f.

This supersedes the earlier #1108 review-request comment after rebasing onto current master. Same fix: the pre-check only recognizes this run own lock IDs, checkout remains authoritative, and 409 handling no longer claims a live competing run unless the response proves one.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. I found no Critical or Important issues.

Critical Issues (0)

Important Issues (0)

Suggestions (2)

• [tests] server/src/services/issues.ts:2260 — Add service or route coverage that seeds executionRunId with a scheduled_retry run and asserts list/detail hydration returns it as activeRun; the shared-set test pins membership but not the changed API behavior.
• [tests] server/src/services/heartbeat.ts:14764 — Add a timer-availability case with only checkoutRunId populated. The new query correctly checks both lock columns, but current fixtures that set a checkout lock also set executionRunId, so dropping the checkout-only branch would not be detected.

Strengths

• The company-scoped correlated NOT EXISTS checks both lock columns and preserves the intended PostgreSQL NULL behavior.
• Centralizing canonical holding statuses removes the concrete scheduled_retry drift across timer availability, execution paths, and activeRun hydration.
• Coverage exercises live holders, retry holders, terminal stale locks, mixed held/free work, and the canonical status partition.
• The skill guidance now limits the cheap pre-check to the current run's own lock IDs, keeps checkout authoritative, and avoids inferring a live competing run from a generic conflict.

Recommended Action

1. Merge when required CI checks pass.
2. Consider the regression-test suggestions opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. I found no Critical or Important issues.

Critical Issues (0)

Important Issues (0)

Suggestions (2)

• [tests] server/src/services/issues.ts:2260 — Add service or route coverage that seeds executionRunId with a scheduled_retry run and asserts list/detail hydration returns it as activeRun; the shared-set test pins membership but not the changed API behavior.
• [tests] server/src/services/heartbeat.ts:14764 — Add a timer-availability case with only checkoutRunId populated. The new query correctly checks both lock columns, but current fixtures that set a checkout lock also set executionRunId, so dropping the checkout-only branch would not be detected.

Strengths

• The company-scoped correlated NOT EXISTS checks both lock columns and preserves the intended PostgreSQL NULL behavior.
• Centralizing canonical holding statuses removes the concrete scheduled_retry drift across timer availability, execution paths, and activeRun hydration.
• Coverage exercises live holders, retry holders, terminal stale locks, mixed held/free work, and the canonical status partition.
• The skill guidance now limits the cheap pre-check to the current run's own lock IDs, keeps checkout authoritative, and avoids inferring a live competing run from a generic conflict.

Recommended Action

1. Merge when required CI checks pass.
2. Consider the regression-test suggestions opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. Re-review after a merge-from-master sync (fddddc44 is a merge commit; the PR's own diff against the two prior commits fd953bb8 + 616e11f7 is unchanged — confirmed via gh pr diff matching the PR description exactly and via the commit list showing no new PR-authored commits). No Critical or Important issues.

Critical Issues (0)

Important Issues (0)

Suggestions (2)

• [tests] server/src/services/issues.ts (ACTIVE_RUN_STATUSES / getActiveRun) — Add service or route coverage that seeds executionRunId with a scheduled_retry run and asserts list/detail hydration returns it as activeRun; the shared-set test pins membership but not the changed API behavior.
• [tests] server/src/services/heartbeat.ts (hasActionableTimerWork) — Add a timer-availability case with only checkoutRunId populated. The new query correctly checks both lock columns, but current fixtures that set a checkout lock also set executionRunId, so dropping the checkout-only branch would not be detected.

Strengths

• The company-scoped correlated NOT EXISTS/exists(...) check in hasActionableTimerWork covers both lock columns (checkoutRunId OR executionRunId) and correctly preserves PostgreSQL's NULL-comparison semantics — a null lock column can't spuriously match.
• issue-execution-lock.ts centralizes the previously drift-prone "holds vs. terminal" partition (three open-coded literal arrays collapsed to one derived pair), and issue-execution-lock.test.ts pins the partition invariant directly against HEARTBEAT_RUN_STATUSES rather than a hardcoded list.
• New heartbeat tests exercise live holders, scheduled_retry holders, terminal stale locks, mixed held/free work, and the any-not-all-or-nothing availability semantics.
• The skill guidance update correctly narrows the cheap self-check to comparing the run's own $PAPERCLIP_RUN_ID against the lock columns, defers to checkout() as the sole ownership authority, and no longer infers a live competing run from an opaque non-matching lock ID.

Recommended Action

1. Merge when required CI checks pass.
2. Consider the regression-test suggestions opportunistically.

[kkroo]

@allyblockcast please re-review the current head fddddc441fe3d43a9765d0b89826fe59769e9a4b for the formal App evidence lane.

The current head is clean, required verify is green, and the existing User-seat approval is already at this head.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. I found no Critical or Important issues.

Critical Issues (0)

Important Issues (0)

Suggestions (2)

• [tests] server/src/__tests__/heartbeat-stale-queue-invalidation.test.ts:37 — Add a checkout-only lock fixture (checkoutRunId set while executionRunId is null) to pin the new OR condition independently of the normal dual-column path.
• [tests] server/src/services/issues.ts:2371 — Add API-facing coverage showing that a scheduled_retry executionRunId hydrates as activeRun, rather than testing only the shared status partition.

Strengths

• The correlated NOT EXISTS check is company-scoped, checks both lock columns, and only suppresses a timer wake when every candidate would conflict with a live holder.
• Centralizing holding and terminal status definitions prevents the previous scheduled_retry drift between checkout behavior, timer availability, and active-run hydration.
• The regression suite covers live, retry, terminal-stale, and mixed available/held issue states.

Recommended Action

1. Merge when required CI checks pass.
2. Consider the regression-test suggestions opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

• [code] server/src/services/heartbeat.ts:15053 — The new availability query treats a lock as live only when its run status appears in ISSUE_EXECUTION_LOCK_HOLDING_RUN_STATUSES. That is not the checkout predicate: clearExecutionRunIfTerminal / clearCheckoutRunIfTerminal retain every non-terminal status, including an unrecognized persisted value. The new helper test explicitly defines some_future_live_status as lock-holding while this inArray query ignores it, so a timer wake is still dispatched only to receive a checkout conflict. Use the terminal-status predicate (or otherwise make the SQL and checkout definitions identical) and add a regression fixture for a non-canonical non-terminal status.

Suggestions (0)

Strengths

• The correlated availability check covers both checkout and execution lock columns.
• The regression suite covers retry, terminal-stale, and mixed held/free paths.

Recommended Action

1. Address the Important issue before merge.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:0b1dd47 important 1 — still-present — server/src/services/heartbeat.ts:15058 — Timer availability recognizes a holder only when its status is in the canonical ISSUE_EXECUTION_LOCK_HOLDING_RUN_STATUSES array, while checkout retains every status absent from TERMINAL_HEARTBEAT_RUN_STATUSES. A persisted non-terminal status outside the canonical union therefore dispatches a timer wake which checkout rejects with a lock conflict.

Critical Issues (0)

Important Issues (1)

• [prior:0b1dd47 important 1] server/src/services/heartbeat.ts:15058 — The availability NOT EXISTS filter does not match checkout's lock predicate for unknown non-terminal run statuses. runStatusHoldsIssueExecutionLock("some_future_live_status") explicitly returns true, but this SQL enumeration treats that same holder as available, so the timer still starts a run that cannot claim any work.
  • Use a terminal-status-based predicate that matches checkout, or explicitly normalize persisted statuses before this query; add a timer regression fixture for a non-canonical non-terminal holder.

Suggestions (0)

Strengths

• The correlated subquery correctly scopes runs to the issue company and checks both lock columns.
• The shared canonical status set correctly adds scheduled_retry to timer availability and activeRun hydration.

Recommended Action

1. Address the Important issue before merge.

[kkroo]

@ally please submit the formal App and User reviews for current head 4b8007c337620effeb126b6f9cd7178a1a5912c3.

verify is green and the PR is cleanly mergeable. Earlier approvals are on older heads.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:0b1dd47 important 1 — fixed — server/src/services/heartbeat.ts:15066 — The correlated lock query now excludes only statuses in TERMINAL_HEARTBEAT_RUN_STATUSES, so an unrecognized persisted non-terminal status remains a holder rather than making the issue look timer-actionable.

Critical Issues (0)

Important Issues (1)

• [native-codex] server/src/services/heartbeat.ts:15054 — The timer predicate suppresses issues held by every non-terminal run, but checkout can reclaim a queued or scheduled_retry holder that has never started. isReapableHeartbeatRunRow() treats those holders as stale and adoptStaleCheckoutRun() cancels and adopts them once the new timer run is running. The new regression fixtures create exactly such holders without startedAt, yet expect the timer to skip, permanently leaving the issue behind a reclaimable lock until another non-timer wake happens.
  • Align the predicate with checkout availability by treating only non-reapable holders as busy, and add coverage for a never-started queued/scheduled-retry holder that a timer wake successfully adopts.

Suggestions (0)

Strengths

• The terminal-complement SQL predicate correctly fixes the prior unknown-status mismatch.
• The shared lock-status module and focused regression tests make the intended lock semantics easier to trace.

Recommended Action

1. Address the Important issue before merge.

[kkroo]

@ally please submit the formal App and User reviews for current head 26c89e89870ffd6e161938baee2d863d797e4b8b.

Addresses the last Important finding:

• timer availability now uses the same terminal-status predicate as checkout;
• the regression covers a checkout-only some_future_live_status holder;
• focused Vitest: 33/33; server typecheck: passed.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:cee347b important 1 — still-present — server/src/services/heartbeat.ts:15077 — hasActionableTimerWork excludes every issue named by a non-terminal lock holder before a new timer run exists, but checkout subsequently reaps a never-started queued or scheduled_retry holder and adopts the issue.

Critical Issues (0)

Important Issues (1)

• [prior:cee347b important 1] server/src/services/heartbeat.ts:15077 — The timer availability predicate suppresses a wake for every non-terminal lock owner. That is stricter than checkout availability: isReapableHeartbeatRunRow() classifies a queued or scheduled_retry run with startedAt == null as stale, and adoptStaleCheckoutRun() cancels it and transfers the issue to the current running timer run. The added fixtures seed precisely those never-started statuses but now assert no wake, leaving a reclaimable issue without its timer execution path.
  • Treat only non-reapable holders as busy, or ensure timer dispatch can perform the same reclaim before deciding there is no actionable work. Add a timer regression that successfully adopts a never-started queued/scheduled_retry holder.

Suggestions (0)

Strengths

• The terminal-status SQL predicate now correctly handles a checkout-only holder with a future nonterminal status.
• The added partition tests and checkout-only regression make the terminal-complement behavior explicit.

Recommended Action

1. Address the Important issue before merge.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. I found no Critical or Important issues.

Prior Findings Dispositioned (1)

• prior:cee347b important 1 — fixed — server/src/services/heartbeat.ts:15066 — The correlated lock query now treats a lock as busy only when its holder is non-terminal and not a never-started queued or scheduled_retry run, matching checkout reclamation.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The timer predicate now mirrors checkout availability for terminal, unknown non-terminal, and never-started reclaimable holders.
• The regression suite covers both reclaimable statuses and checkout-only locks.
• The shared lock-status definitions prevent the previously observed status drift.

Recommended Action

1. Merge when required CI checks pass.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. I found no Critical or Important issues.

Prior Findings Dispositioned (1)

• prior:cee347b important 1 — fixed — server/src/services/heartbeat.ts:15066 — The correlated lock query now treats a lock as busy only when its holder is non-terminal and not a never-started queued or scheduled_retry run, matching checkout reclamation.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The timer predicate now mirrors checkout availability for terminal, unknown non-terminal, and never-started reclaimable holders.
• The regression suite covers both reclaimable statuses and checkout-only locks.
• The shared lock-status definitions prevent the previously observed status drift.

Recommended Action

1. Merge when required CI checks pass.

[kkroo]

@ally please submit the formal App and User reviews for current head 928b7e6e3e7f88c5e8ec510b5230744b24193a53.

Addresses the last Important finding:

• timer availability now treats only non-reapable non-terminal locks as busy;
• queued and scheduled-retry holders with startedAt: null are covered;
• the regression advances the new timer run to running, verifies atomic checkout adoption, and asserts the prior owner is cancelled.

Checks: targeted Vitest 2/2 adoption cases passed; server typecheck passed.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. I found no Critical or Important issues.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The timer availability query now treats only non-terminal, non-reapable lock holders as busy, matching the checkout adoption path.
• Regression coverage exercises checkout-only unknown-status locks, terminal locks, live holders, mixed availability, and never-started queued and scheduled_retry holders.
• The shared lock-status module removes the prior status-list drift among checkout, timer availability, and activeRun hydration.
• The Paperclip skill now uses lock columns only for a self-check and leaves ownership decisions to checkout().

Recommended Action

1. Merge when required CI checks pass.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. I found no Critical or Important issues.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The timer availability query now treats only non-terminal, non-reapable lock holders as busy, matching the checkout adoption path.
• Regression coverage exercises checkout-only unknown-status locks, terminal locks, live holders, mixed availability, and never-started queued and scheduled_retry holders.
• The shared lock-status module removes the prior status-list drift among checkout, timer availability, and activeRun hydration.
• The Paperclip skill now uses lock columns only for a self-check and leaves ownership decisions to checkout().

Recommended Action

1. Merge when required CI checks pass.

[kkroo]

@ally please provide formal App and User reviews for exact head 3e33745.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. I found no Critical or Important issues.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [tests] server/src/__tests__/issue-execution-lock.test.ts:19 — Add an API-facing assertion that an issue whose executionRunId points to a scheduled_retry run hydrates it as activeRun; the shared-status test verifies the set, but not the changed detail/list response behavior.

Strengths

• The correlated timer query checks both lock columns and excludes only non-terminal, non-reapable holders, matching checkout's terminal cleanup and never-started adoption paths.
• The terminal tuple provides a non-empty SQL-safe source for the availability predicate while the shared module eliminates the prior scheduled_retry status drift.
• The regression suite covers live holders, unknown non-terminal statuses, terminal stale locks, checkout-only locks, mixed availability, and reclaimable queued/retry owners.

Recommended Action

1. Merge when required CI checks pass.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. I found no Critical or Important issues.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [tests] server/src/__tests__/issue-execution-lock.test.ts:19 — Add an API-facing assertion that an issue whose executionRunId points to a scheduled_retry run hydrates it as activeRun; the shared-status test verifies the set, but not the changed detail/list response behavior.

Strengths

• The correlated timer query checks both lock columns and excludes only non-terminal, non-reapable holders, matching checkout's terminal cleanup and never-started adoption paths.
• The terminal tuple provides a non-empty SQL-safe source for the availability predicate while the shared module eliminates the prior scheduled_retry status drift.
• The regression suite covers live holders, unknown non-terminal statuses, terminal stale locks, checkout-only locks, mixed availability, and reclaimable queued/retry owners.

Recommended Action

1. Merge when required CI checks pass.

[kkroo]

@ally please provide formal App and User reviews for exact head 70329e2.

[allyblockcast]

CI is red at 70329e213 — one type error, and I've verified the fix without touching this branch

Flagging rather than pushing: this is your branch and you're actively driving it, so I've left the head alone. The verified commit is on cto/blo-19749-typecheck-fix-proposed (1c9194f19) for you to take or ignore.

The failure

Three of the five red checks are the same error, and it's the only thing standing between this PR and merge:

server/src/services/heartbeat.ts(15072,15): error TS2345:
Argument of type 'SQL<unknown> | undefined' is not assignable to parameter of type 'SQLWrapper'.

Typecheck + Release Registry (job), Build, Canary Dry Run — byte-identical message in all three. verify is the aggregator.

70329e213 ("make terminal lock predicate type-safe") fixed the adjacent line — notInArray(..., TERMINAL_HEARTBEAT_RUN_STATUS_VALUES) needed the non-empty tuple — but the not(and(...)) immediately below still fails: drizzle's and() is SQL | undefined (it returns undefined when every argument is undefined), and not() demands a non-optional SQLWrapper.

The fix

De Morgan rather than !, so the same class of error can't be silently swallowed if a leg becomes optional later:

-            not(
-              and(
-                inArray(
-                  heartbeatRuns.status,
-                  [...ISSUE_EXECUTION_LOCK_REAPABLE_NEVER_STARTED_RUN_STATUSES],
-                ),
-                isNull(heartbeatRuns.startedAt),
-              ),
-            ),
+            or(
+              notInArray(
+                heartbeatRuns.status,
+                [...ISSUE_EXECUTION_LOCK_REAPABLE_NEVER_STARTED_RUN_STATUSES],
+              ),
+              isNotNull(heartbeatRuns.startedAt),
+            ),

isNotNull/notInArray are already imported, so no import change. Both legs are NOT NULL booleans (status is non-nullable; IS NULL never yields NULL), so this is an exact equivalent — not a three-valued-logic approximation that would quietly change which rows the correlated EXISTS matches.

Verified, not asserted

• pnpm run typecheck:build-gaps (the exact CI command) — exit 0, zero diagnostics.
• Reverted the one hunk and re-ran: reproduces heartbeat.ts(15072,15): error TS2345 exactly, exit 2. So the fix is load-bearing, not incidental.
• issue-execution-lock.test.ts + heartbeat-stale-queue-invalidation.test.ts — 37/37 passed, including allows a timer wake to adopt a never-started scheduled_retry lock, which is precisely the leg the rewrite touches.

To take it: git cherry-pick 1c9194f19. Delete the branch after — it exists only so the verification isn't lost.

One thing that is not downstream of the type error

General tests (workspaces-b) is a separate failure and won't clear when the type error does:

FAIL @paperclipai/adapter-utils src/execution-target-sandbox.test.ts
  > ignores unauthenticated connections to the process session bridge
  Error: Timed out waiting for process session proxy.

That's a 5s socket-wait timeout in a sandbox test with no connection to this diff. Reads as load-sensitive flake, but I haven't reproduced it, so I'm reporting it rather than classifying it — worth a re-run to see if it sticks before you enqueue.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. I found no Critical or Important issues.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [tests] server/src/services/issues.ts:2404 — Add API-facing coverage that asserts an issue whose executionRunId names a scheduled_retry run hydrates it as activeRun; the new shared-status tests prove the set but not the detail/list response behavior.

Strengths

• The correlated timer query checks both lock columns and excludes only non-terminal, non-reapable holders, matching checkout's terminal cleanup and stale-lock adoption paths.
• The regression suite covers live and unknown non-terminal holders, terminal stale locks, checkout-only locks, mixed availability, and both reclaimable never-started statuses.
• Shared status definitions prevent scheduled_retry from drifting between checkout behavior, timer availability, and active-run hydration.

Recommended Action

1. Merge when required CI checks pass.
2. Consider the regression-test suggestion opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. I found no Critical or Important issues.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [tests] server/src/services/issues.ts:2404 — Add API-facing coverage that asserts an issue whose executionRunId names a scheduled_retry run hydrates it as activeRun; the new shared-status tests prove the set but not the detail/list response behavior.

Strengths

• The correlated timer query checks both lock columns and excludes only non-terminal, non-reapable holders, matching checkout's terminal cleanup and stale-lock adoption paths.
• The regression suite covers live and unknown non-terminal holders, terminal stale locks, checkout-only locks, mixed availability, and both reclaimable never-started statuses.
• Shared status definitions prevent scheduled_retry from drifting between checkout behavior, timer availability, and active-run hydration.

Recommended Action

1. Merge when required CI checks pass.
2. Consider the regression-test suggestion opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

• [native-codex] server/src/services/issues.ts:672 — ACTIVE_RUN_STATUSES enumerates only the currently known holding states, while the new lock contract intentionally treats every unknown nonterminal status as holding. A row in a newly introduced status will therefore block checkout() and be excluded by hasActionableTimerWork, but hydrate as activeRun: null; this recreates the API/checkout disagreement the PR is meant to remove.
  • Make activeRun use the same terminal-complement predicate as the checkout and timer paths, or make the helper's unknown-status behavior match the API contract, and add coverage for an unknown persisted nonterminal status.

Suggestions (0)

Strengths

• The timer availability query correctly checks both lock columns and handles never-started reclaimable owners.
• The new focused tests cover live, scheduled-retry, terminal, and mixed-availability cases.

Recommended Action

1. Address the Important issue before merge.

[allyblockcast]

Thanks — the Important finding is correct, and it is now tracked in BLO-25410. It cannot be addressed on this PR: the review was submitted at 2026-08-11T02:49:51Z, 2m28s after this PR merged at 02:47:23Z, so there is no branch left to push a follow-up commit to.

Confirming the finding

The asymmetry is real. The three consumers of the lock notion do not express it the same way:

site	form	out-of-union non-terminal status
checkout() — runStatusHoldsIssueExecutionLock	!TERMINAL.has(status)	holding → 409
hasActionableTimerWork — heartbeat.ts:15164	notInArray(status, TERMINAL_…_VALUES)	holding → wake suppressed
activeRun hydration — issues.ts:2337, used at :2479	inArray(status, ACTIVE_RUN_STATUSES)	not matched → activeRun: null

ACTIVE_RUN_STATUSES derives from HEARTBEAT_RUN_STATUSES.filter(s => !TERMINAL.has(s)) — an enumeration over the canonical union only. A status persisted in the column but absent from that union therefore cannot appear in it, and GET /api/issues/{id} would return activeRun: null while POST /checkout 409s naming that run. That is exactly the disagreement this PR set out to remove, reproduced for the unknown-status case. issue-execution-lock.ts already articulates the rule the site violates ("SQL call sites … must use the same terminal-status predicate"), so this is an internal inconsistency, not a design disagreement.

Worth adding that it is not purely theoretical: that same module records error and adapter_failed as statuses "not in the canonical union but … observed in the column". Both happen to be terminal and are explicitly handled, so nothing is broken today — the exposure is the next out-of-union status that isn't terminal.

Two corrections to the review

1. The line citation is wrong. server/src/services/issues.ts:672 is comment-attribution code at the reviewed head; ACTIVE_RUN_STATUSES is at :2337 and the inArray call site is :2479. Worth checking, since a wrong anchor sends the next reader to unrelated code.
2. Severity is Important-leaning-Suggestion. The defect is latent — reachable only once a non-terminal status is persisted outside HEARTBEAT_RUN_STATUSES. Correct to flag, but it did not warrant blocking the merge, which is moot here anyway.

hasActionableTimerWork I read as not being called out as broken — it already uses the terminal-complement form and agrees with checkout. Flagging in case that was intended otherwise.

Fix and regression test (insert a run row in a non-terminal out-of-union status, assert activeRun hydrates it) are specified in BLO-25410's acceptance criteria.