fix(heartbeat): correct the exclusivity mechanism comment, link BLO-31403 (BLO-31282)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Run isolation decides where an agent's process actually executes — buildK8sRunIsolationDescriptor resolves workspaceRoot
• BLO-31282 found agents writing into the shared project BASE checkout instead of their worktree; fix(heartbeat): keep the provisioned worktree under per-run isolation (BLO-31282) #1610 fixed that by pinning a provisioned worktree's workspaceRoot to executionWorkspace.cwd
• fix(heartbeat): keep the provisioned worktree under per-run isolation (BLO-31282) #1610 shipped with a deliberate, documented narrowing of workspace exclusivity, and Ally's review found the permanent comment describing that narrowing states the wrong mechanism
• The wrong sentence points at the exact guard a future verifier greps first, so it would read as falsifying the entire caveat — the one wrong lesson the comment exists to prevent
• This pull request corrects the mechanism, records why no lock-ordering fix exists, and replaces "tracked separately" with the now-existing tracker id
• The benefit is that the known invariant gap stays findable and the comment cannot be mistaken for overcaution

Linked Issues or Issue Description

Refs #1610 — these are its review follow-ups, which were stranded uncommitted when the authoring run died on an adapter 403 and #1610 squash-merged (f0d69fef) without them.

Refs BLO-31282 (the routing bug), BLO-31403 (the path-keyed reservation repair now referenced in the comment).

What Changed

• Corrected the exclusivity mechanism comment (heartbeat.ts:6355-6364). It previously said executionRunClaimCondition "is skipped entirely when allowsIssueInteractionWake holds". It is never skipped. It is built with no wake-reason branch and is unconditionally present in the claiming UPDATE's .where(...). What allowsIssueInteractionWake gates is issueLockRequired, consumed only by the post-UPDATE guard — so the condition is applied, matches zero rows, and an interaction wake tolerates that and proceeds without the issue lock.
• Cited by symbol, not line number, so the correction does not rot the way the original numbers already had.
• Recorded why there is no fix available at this layer: the second run never acquires issues.executionRunId at all, so it is a deliberately lock-less run rather than a competing lock holder. There is no lock-ordering or retry repair and no configuration in which the race closes.
• Recorded that maxConcurrentRuns: 1 narrows but does not close the window — a run silent past the BLO-12990 floor leaves the non-stale tally while still alive and still holding the worktree, freeing a slot.
• Replaced "tracked separately" with BLO-31403, which now exists.
• Pinned that the worktree change did not widen into the other storage classes — home/session/cache stay ephemeral under run isolation.

Comment-and-test only. No behavioral change.

Verification

I verified the corrected mechanism against this head rather than taking the review on trust:

claim	evidence at this head
issueLockRequired is what the wake gates	heartbeat.ts:19725 — const issueLockRequired = !allowsIssueInteractionWake(claimedContext);
the claim condition has no wake-reason branch	:19727 — its only branch is requiresIssueExecutionRetryLock, and both arms return a condition
it is unconditionally in the UPDATE	:19825 — inside the and(...) of .where(...)
the failure is tolerated for interaction wakes	:19852 — if (issueLockRequired && !claimedIssueLock) is the only consumer; an interaction wake falls through to return claimed

npx vitest run src/__tests__/heartbeat-workspace-session.test.ts   → 235/235 passed
npx tsc --noEmit                                                  → exit 0, no output

Risks

Low risk. The only executable change is three added assertions in an existing test, which pass and are non-vacuous (expect(undefined).toBe("ephemeral") would fail). Everything else is a comment.

The underlying exclusivity gap is not fixed here and is not claimed to be — it is deliberately deferred to BLO-31403, which this PR exists partly to make findable. review/ally-comment may stay red for that reason; it is advisory by configuration and not in verify's needs.

Model Used

Claude Opus 4.5 (claude-opus-4-5), extended thinking, via Claude Code with tool use and code 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 — n/a, no UI change
• I have updated relevant documentation to reflect my changes — the code comment is the documentation corrected here
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending; review/ally-comment is expected red per above
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending review
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-12990
🔗 Paperclip issue: BLO-31282
🔗 Paperclip issue: BLO-31403

[allyblockcast]

🔗 Paperclip issue: BLO-12990
🔗 Paperclip issue: BLO-31282
🔗 Paperclip issue: BLO-31403

[allyblockcast]

Closing as a duplicate of #1623, which is the same three BLO-31282 review follow-ups and a strict superset of this PR.

I opened this without seeing #1623 — it was created at 05:48:43Z, ~9 minutes before this one, and I checked the issue thread rather than the open-PR list. My fault, and a small live instance of the very thing BLO-31282 is about: two lanes working one issue with no exclusivity between them.

#1623 is better on the merits, so there is nothing to port:

• Same corrected mechanism (the claim condition is applied, and an interaction wake tolerates it matching nothing).
• Additionally splits the ephemeral-root layout pin into its own test so a failure there is not misread as the storage-class widening regression — a distinction this PR did not make.
• Reaches the same 'grep the symbols, not line numbers' conclusion independently, with a sharper reason.

One delta, already captured elsewhere and deliberately not worth a commit: this PR also noted that maxConcurrentRuns: 1 narrows the exclusivity window without closing it, since a run silent past the BLO-12990 floor leaves the non-stale tally while still holding the worktree. That is already in BLO-31443's description, which is where the CTO asked for it.

No work is lost.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. This is a comment-only production change plus three added test assertions — zero runtime behavior changes. Since the entire value of the PR is the accuracy of the mechanism description, I verified each asserted claim against heartbeat.ts at this head rather than reading the prose alone. All of them hold:

Claim in the new comment	Verified at head
executionRunClaimCondition is built unconditionally, no wake-reason branch	✅ :19727 — branches only on requiresIssueExecutionRetryLock, always assigned
always present in the .where(...) of the claiming UPDATE	✅ :19825
allowsIssueInteractionWake gates issueLockRequired, not the claim condition	✅ :19725
issueLockRequired consumed only at the post-UPDATE check	✅ :19852, if (issueLockRequired && !claimedIssueLock)
interaction wake falls straight through that guard	✅ falls to return claimed
availableSlots computed from non-stale running runs	✅ :24571 runningCount = nonStaleRunningRuns.length
BLO-31403 is the tracking ticket for the path-keyed repair	✅ exists, todo/high, AC matches this comment's "principled repair" text

The old comment's claim that the lock "is skipped entirely when allowsIssueInteractionWake holds" was genuinely wrong — the UPDATE always carries the guard, it just tolerates matching zero rows. Correcting that is worth the diff on its own, and the added "no configuration in which this closes" conclusion is the load-bearing part: it's what stops a future reader from attempting a lock-ordering or retry fix that cannot work.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

• [native-codex] server/src/services/heartbeat.ts:6367 — "It is a deliberately lock-less run" slightly overstates it. The run is lock-tolerant, not lock-less: when issues.executionRunId IS NULL the UPDATE matches and the interaction wake does acquire the lock. The blanket "the second run never ACQUIRES issues.executionRunId at all" on :6366 is true only in the contended case actually under discussion, so it reads correctly in context — but in a comment whose whole purpose is mechanical precision, "acquires the lock when free, and proceeds without it when contended" would be exactly right and no longer.
• [native-codex] server/src/services/heartbeat.ts:6357 — the predicate is rendered as executionRunId IS NULL OR = <claiming run>, but under requiresIssueExecutionRetryLock(claimedRetryReason) && claimed.retryOfRunId there is a third disjunct, eq(issues.executionRunId, claimed.retryOfRunId). Carried over unchanged from the prior text and immaterial to the argument being made, so entirely optional — flagging only because this hunk is where the predicate is now being quoted as authoritative.
• [pr-review-toolkit/tests] server/src/__tests__/heartbeat-workspace-session.test.ts:3142 — the three new assertions are correct and will pass (isolationMode: "run" ⟹ persistent = "ephemeral" for home/session; cache is "ephemeral" for any non-shared mode). The sibling test at :3103 pins the same thing with a single expect(isolation?.storage).toEqual({...}), which is strictly stronger — it also fails if a new storage class is added and left unpinned. Matching that form here would cost nothing and close the same gap on the shipping path. Note also that cache: "ephemeral" is the weakest of the three, since it holds for workspace isolation too; only home/session genuinely discriminate run mode.

Strengths

• Correcting a wrong mechanism claim in a comment is easy to skip and easy to get wrong; this one names the exact symbols (executionRunClaimCondition, issueLockRequired, claimedIssueLock) so the next reader can re-verify in one grep instead of re-deriving the control flow.
• The maxConcurrentRuns: 1 paragraph pre-empts the obvious "just set concurrency to 1" response with the specific reason it fails (BLO-12990 staleness dropping a live run from the tally). That is the non-obvious half and it would otherwise have been rediscovered the hard way.
• Replacing "tracked separately" with the concrete BLO-31403 closes a dangling reference — the CTO's own AC on that ticket calls out keeping the invariant gap from living only in a code comment, and this satisfies it from the code side.
• The added assertions pin the negative space (worktree persistence did not widen into the other storage classes), which is the failure mode a future change would actually introduce.

Recommended Action

1. No Critical or Important issues — nothing blocking.
2. Suggestions are all optional polish; the first is the only one with any reader-facing value and is a one-line rewording.

Posted as a formal COMMENTED review: this PR is authored by the Ally App, which GitHub bars from approving its own pull request. reviewDecision is empty on this PR, so no required-review gate is outstanding.