fix(heartbeat): correct the exclusivity mechanism comment, link BLO-31443 - #1623
Conversation
…1443 Follow-ups from Ally's review of #1610, none of which gate that merge. The permanent comment at heartbeat.ts:6358 stated the wrong mechanism, and pointed at the exact guard a verifier checks first. It read "that lock is skipped entirely when allowsIssueInteractionWake holds", naming executionRunClaimCondition. That condition is never skipped: it is built unconditionally at :19714 and is always present in the claiming UPDATE's .where(...). What allowsIssueInteractionWake gates is issueLockRequired (:19712), consumed only at :19839. The UPDATE runs with the guard, matches zero rows when another run holds executionRunId, and that failure is tolerated -- control falls through to `return claimed` at :19853. A reader following the comment's own instruction not to re-derive this would grep executionRunClaimCondition, find it unconditionally applied, and reasonably conclude the whole narrowing caveat was overcautious. That is the one wrong lesson the comment exists to prevent. The correction strengthens the caveat. The second run never acquires issues.executionRunId at all -- it is a deliberately lock-less run, not a competing lock holder -- so there is no lock-ordering or retry fix available and no configuration in which the race closes. Also links the "tracked separately" pointer to BLO-31443, and pins that keeping the workspace persistent did not leak into the sibling roots. Verified by negative control: widening usesEphemeralWorkspace to cover home/session fails the new assertion with "expected 'persistent' to be 'ephemeral'". heartbeat-workspace-session.test.ts: 235/235 pass. tsc error count is identical to pristine master (39, all pre-existing, none in either changed file). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Omar Ramadan <omar@blockcast.network>
|
🔗 Paperclip issue: BLO-31443 |
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6dec3ef
The substantive mechanism correction is right, and I verified it independently rather than taking the PR body's word for it. At this head: executionRunClaimCondition is built with no wake-reason branch (heartbeat.ts:19717, ternary keys only on requiresIssueExecutionRetryLock), it is unconditionally present in the claiming UPDATE's and(...) (:19815), issueLockRequired = !allowsIssueInteractionWake(...) (:19715) makes the cancel at :19842 a no-op for an interaction wake, and control does fall through to return claimed (:19856). The old comment's "that lock is skipped entirely" was false; the replacement is accurate. All five new test assertions also check out against the implementation.
The findings below are both about the pointers, not the mechanism.
Critical Issues (0)
Important Issues (2)
-
[comments/native-codex]
server/src/services/heartbeat.ts:6357— All three line citations in the new comment are wrong at this head, and this PR's own edit is what broke them. The comment is +11/-8 = net +3 lines, and it cites lines ~13,000 below itself. Measured both sides:comment says correct at base f0d69fefactual at head 6dec3eff`:19714`claim condition built19714 ✅ 19717 `:19839`skips the cancel19839 ✅ 19842 `:19853`return claimed19853 ✅ 19856 Every number was correct before the edit and is off by exactly +3 after it. The worst of the three is
:19839: at this head that line isreturn null;inside the routine-execution-duplicate cancel ("cancelled duplicate routine execution run after lock race") — a genuinely different cancel path. A reader who follows the pointer lands on a cancel that is not the one being described and can reasonably conclude the comment is documenting the routine-duplicate branch.That defeats this PR's own stated purpose. The comment exists to stop a reader re-deriving the mechanism; sending them to the wrong branch is the same failure mode as the original wrong-mechanism text, one level down.
- Recommendation: drop the bare line numbers. The symbol names are already in the comment and each resolves to exactly one definition site (
grep -n "const issueLockRequired"→ 1 hit;const executionRunClaimCondition→ 1 hit), so the numbers add nothing the symbols don't while carrying guaranteed decay — any future edit above:19715re-breaks them. If you want to keep them, anchor them to a SHA the way BLO-31443's own description already does ("Verified atf0d69fef"): the ticket got this right, the source comment didn't, and the comment is the copy that lives in the file whose lines move.
- Recommendation: drop the bare line numbers. The symbol names are already in the comment and each resolves to exactly one definition site (
-
[comments]
server/src/services/heartbeat.ts:6359— The rewrite drops the greppable predicate and the design rationale. The removed text namedallowsIssueInteractionWakeand explained why the behavior is deliberate: "an issue-interaction wake carrying a comment id is deliberately allowed to run while another run holds the issue, so a human can talk to the assignee mid-flight." The replacement says only "an interaction wake", with no symbol to grep and no reason given.Combined with the finding above, the net trade is durable anchors (a unique symbol name, the intent) for fragile ones (three line numbers that are already stale). The intent sentence is the part a future reader most needs and least able to reconstruct — it is the difference between "this race is a bug someone should close" and "this race is the price of a feature". It survives in BLO-31443, but the reader is in the file, not the tracker.
- Recommendation: keep
allowsIssueInteractionWakeby name and restore the one-clause rationale. Both fit inside the lines the numbers would vacate.
- Recommendation: keep
Suggestions (2)
- [tests]
server/src/__tests__/heartbeat-workspace-session.test.ts:3143— the comment attributes all five assertions to one regression ("if someone later widensusesEphemeralWorkspaceto cover them too"), but only the threestorage.*assertions would catch it.homeRoot/sessionRootnever readusesEphemeralWorkspace— they key purely offisolationMode(heartbeat.ts:6386,:6391), so that widening cannot move them. Those two pin a different property: the ephemeral root layout. The PR body's Risks section says exactly this and calls it deliberate; worth putting that split in the code comment, so a future reader who breaks one assertion knows which invariant they actually broke. - [tests]
server/src/__tests__/heartbeat-workspace-session.test.ts:3147— the test is named "keeps the provisioned worktree when the isolation identity is precomputed by dispatch", and now also asserts sibling-root ephemerality and two exact paths. AhomeRootmismatch reported under that name is harder to place. Consider a siblingit(...)for the root assertions, sharing the same descriptor call.
Strengths
- The correction is verified, not asserted. I re-derived all four claims against the head file and they hold, including the non-obvious one — that the claim condition is applied and its zero-row result tolerated, rather than bypassed.
- BLO-31443 is real and substantive. I fetched it: correct title, full mechanism, four acceptance criteria, a negative control, and the
maxConcurrentRunsanalysis showing the race survives at effective concurrency 1 via the BLO-12990 staleness floor. Replacing "tracked separately" with a live id was the right call, and the id points somewhere worth arriving at. - The negative control on the test is the right instinct — injecting the regression and confirming the new assertion fails is what makes the five
expects worth their maintenance cost. hasProvisionedWorktreeis left alone. Correcting the comment without touching the guard it describes keeps the diff honestly reviewable as documentation.
Recommended Action
- No Critical issues.
- Address the two Important issues this cycle — both are edits to the same comment block and neither touches runtime. Fixing them is strictly less work than the analysis that produced them.
- Consider the test-comment split opportunistically.
Not approving: the two Important findings are in the exact artifact this PR exists to correct, so a green verdict would attest that the comment is now accurate when its pointers are not. The underlying mechanism analysis is sound and I'd expect this to clear quickly on a follow-up push.
…nd rationale Addresses both Important findings from Ally's review of 6dec3ef. 1. The line citations were wrong, and this PR's own edit broke them. The comment is net +3 lines and cited lines ~13,000 below itself, so every number was correct at f0d69fe and off by exactly +3 afterwards. The worst was the cancel cite: at the previous head that line was `return null;` in the routine-execution-duplicate branch, a genuinely different cancel path. A reader following it would have landed on the wrong branch and concluded the comment documented something else -- the same failure mode as the original wrong-mechanism text, one level down. Bare line numbers are now gone. Each symbol the comment names resolves to exactly one definition site (verified: `const issueLockRequired` 1 hit, `const executionRunClaimCondition` 1 hit, `function allowsIssueInteractionWake` 1 hit), so the numbers bought nothing while carrying guaranteed decay. The comment now says so, citing its own near miss. 2. The rewrite had dropped `allowsIssueInteractionWake` -- the greppable predicate -- and the design rationale for why the behavior is deliberate. Both restored: the wake is allowed to run while another run holds the issue so a human can talk to the assignee mid-flight. Also splits the test per Ally's suggestion. The three `storage.*` assertions catch a widening of `usesEphemeralWorkspace`; `homeRoot`/`sessionRoot` cannot, because they key off `isolationMode` and never consult it. They pin the ephemeral root layout instead, so they now live in their own `it(...)` sharing one descriptor builder. Verified by negative control: injecting the widening fails the two storage tests and leaves the sibling-roots test passing, which is the split Ally described. 236/236 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Omar Ramadan <omar@blockcast.network>
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 464e323
Looks good. Both Important findings from my earlier review are genuinely fixed, and fixed in the durable direction rather than by patching the stale numbers — which would have re-decayed on the next edit above :19715. The diff to heartbeat.ts is comment-only (all 22 added and 8 removed lines are //), so the runtime surface is unchanged and the review reduces to: is the prose accurate, and are the new assertions correct? I re-derived both against the files at this head. They are.
Prior Findings Dispositioned (2)
- prior:6dec3ef important 1 — fixed —
server/src/services/heartbeat.ts:6372— all three absolute line citations (:19714,:19839,:19853) are gone, replaced by "Grep the three symbols named above rather than trusting line numbers." I verified the grep promise actually holds rather than taking it on faith:executionRunClaimCondition→ 1 definition (:19728),allowsIssueInteractionWake→ 1 (:6779),issueLockRequired→ 1 (:19726). Each resolves uniquely, so the symbols carry everything the numbers did with none of the decay. - prior:6dec3ef important 2 — fixed —
server/src/services/heartbeat.ts:6358—allowsIssueInteractionWakeis named again and greppable, and the design rationale is restored verbatim at:6362("That is deliberate — an issue-interaction wake carrying a comment id is allowed to run while another run holds the issue, so a human can talk to the assignee mid-flight"). That intent clause was the part a future reader was least able to reconstruct; it is back in the file rather than only in the tracker.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
-
[comments/tests]
server/src/__tests__/heartbeat-workspace-session.test.ts:3155— the justification for the test split is true but non-distinguishing, and my prior review is where the error came from — flagging it so it stops here. I wrote that "only the threestorage.*assertions would catch it" whilehomeRoot/sessionRoot"never readusesEphemeralWorkspace". The second half is right; the first half is wrong.usesEphemeralWorkspacehas exactly three references in the file — its definition (:6393),workspaceRoot(:6394), andstorage.workspace(:6428). It is not read by the storage trio either:homeandsessionare bothpersistent(:6429,:6430), which isisolationMode === "run" ? …(:6414), andcachekeys offisolationMode === "shared"(:6431). So all five values — the trio and the two roots — key purely offisolationMode, and none consultsusesEphemeralWorkspace.The consequence is only in the prose, not the assertions: because
:3155states "never consultusesEphemeralWorkspace" of the roots and not of the trio, a reader comparing the two comments infers the trio does consult it. It doesn't. The split itself is still worth keeping — ahomeRootfailure and a storage-class failure really are different invariants, which was the useful half of the suggestion.- Either drop the non-distinguishing "never consult … key purely off
isolationMode" clause, or state the distinction that does hold: the trio are siblings ofstorage.workspaceinside one object literal, so the plausible regression is someone tidying that literal onto a single predicate, whereas the roots live outside it. One clause either way.
- Either drop the non-distinguishing "never consult … key purely off
-
[comments]
server/src/services/heartbeat.ts:6373— the last two lines of the grep paragraph narrate this comment's own review history ("An earlier draft of this very comment cited absolute lines and its own +3-line edit silently moved them onto a different cancel branch"). The instruction at:6372is worth keeping — it tells a future editor not to re-add line numbers. The provenance is PR and ticket material: a reader in the file has no way to see the earlier draft, and BLO-31443 already holds the history. Trimming it also takes ~4 lines off a comment block that is now 43 lines above a 4-lineconst. -
[comments]
server/src/services/heartbeat.ts:6372— "the three symbols named above" leaves the reader to work out which three. The block backticks six names before that line (executionRunClaimCondition,allowsIssueInteractionWake,issueLockRequired,executionRunId,return claimed,issues.executionRunId); three of those are uniquely greppable identifiers and three aren't. Naming them inline costs one clause and removes the guess. Worth it in a comment whose whole subject is pointer precision. Relatedly,claimQueuedRunis no longer named anywhere in the block — not a defect, sinceexecutionRunClaimConditionresolves inside it, but it was a free anchor to the enclosing function.
Strengths
- Both prior findings were fixed at the root, not papered over. The cheap fix was to bump three numbers by +3; that would have been correct for exactly one commit. Removing them and leaning on unique symbols is the fix that survives, and it is the one taken.
- The mechanism text is still accurate at the new head — I re-verified all four claims rather than assuming they survived the rewrite.
executionRunClaimConditionis built with a ternary keying only onrequiresIssueExecutionRetryLock(…) && claimed.retryOfRunId, with no wake-reason branch (:19728); it sits unconditionally inside the claiming UPDATE'sand(…)(:19826);issueLockRequired = !allowsIssueInteractionWake(claimedContext)(:19726) makes the:19853lock-not-acquired cancel a no-op for an interaction wake; and control does fall through toreturn claimed(:19867). The subtle claim — that the guard is applied and its zero-row result tolerated, rather than bypassed — is the one the original comment got backwards, and the replacement states it correctly. - The new "no configuration in which the race closes" claim holds up against the two obvious levers. This is a strong universal, so I tried to break it:
concurrencyEnabled: falsepins the effective ceiling to exactly 1 regardless ofmaxConcurrentRuns(:4189), which looks like it should close the race — but the BLO-12990 floor explicitly does not count a stale run toward the slot gate (:2050), so a second run still dispatches. Combined with the lock-less interaction-wake path, the race really does survive at effective concurrency 1. The claim earns its strength. - All five new assertions are correct against the implementation.
ephemeralIsolationRootis/runtime-cache/paperclip-runs/run-1(:6317), so both root paths match exactly;isolationModeis"run"sopersistentis"ephemeral"forhome/session;cacheis"ephemeral"since the mode isn't"shared"; andhasProvisionedWorktreeis true, sostorage.workspaceis"persistent"andworkspaceRootis the worktreecwd. No assertion is aspirational. - The helper extraction is a clean no-op on inputs.
buildPrecomputedIdentityDescriptorlifts the fixture verbatim, so the seconditshares the exact dispatch-shaped input rather than a drifting copy — and the BLO-31282 rationale at:3111now sits above the fixture both tests use, which is where it belongs. - Splitting the roots into their own
itwas the right call even though my stated reason for it was wrong. A failure named "keeps the sibling roots under the per-run ephemeral root" is placeable; the same failure under the worktree-pinning name was not.
Recommended Action
- No Critical issues.
- No Important issues — the two from the previous head are both resolved.
- Take the three Suggestions opportunistically; all are single-clause comment edits and none touches runtime. The first one matters most, since it corrects a wrong dependency fact I introduced.
Approving: the mechanism correction is accurate and independently verified, the new assertions are all correct, and both prior blockers are genuinely closed. The remaining items are comment-precision polish, including one error of my own that I'd rather flag than have inherited silently.
Branch was based on f0d69fe; master has since added four test(db) commits for BLO-31354. Merging to satisfy the strict up-to-date requirement and to re-run policy on current master -- the previous policy failure was two 1-minute-budget steps (bounded PR-check polling, helm chart render) that pass on this branch's base, on master, and on four other open PRs, so it reads as runner slowness rather than anything in this diff.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c6ee189
Looks good. This head is a pure up-to-date merge — it carries no new PR content, and I established that from object identity rather than by eyeballing the diff. Both files this PR touches are byte-identical to the head I approved at 464e3230:
| file | blob @ 464e3230 |
blob @ c6ee1894 |
|---|---|---|
server/src/services/heartbeat.ts |
fc4bc3be |
fc4bc3be |
server/src/__tests__/heartbeat-workspace-session.test.ts |
ed6bf8ff |
ed6bf8ff |
The merge is clean in both directions, which is the check that actually matters on a merge commit — a mis-resolution would show up as a file matching neither parent:
- vs first parent
464e3230→ onlypackages/db/src/heartbeat-dispatch-query-plan.test.ts, blob4f68b7eb, identical to master'sa3fba29a. - vs second parent
a3fba29a(master) → only this PR's two files, identical to464e3230.
So the merge tree is exactly the union of its two parents with zero manual resolution. No conflict markers, nothing smuggled in alongside the merge.
Critical Issues (0)
Important Issues (0)
I re-derived the mechanism against the merged tree instead of assuming it survived, since "the blob didn't change" guarantees the text is intact but not that the code it points at still is. All four claims hold at this head: executionRunClaimCondition is built with a ternary keying only on requiresIssueExecutionRetryLock(claimedRetryReason) && claimed.retryOfRunId, no wake-reason branch (heartbeat.ts:19728); it sits unconditionally inside the claiming UPDATE's and(…) (:19826); issueLockRequired = !allowsIssueInteractionWake(claimedContext) (:19726) makes the lock-not-acquired cancel at :19853 a no-op for an interaction wake; and control falls through to return claimed (:19867).
I also checked the one thing a merge from master could plausibly have broken without touching a single byte of this PR: the comment's "grep the three symbols" promise is a claim about the whole file, so a merge that added a reference elsewhere would silently degrade it. It holds — each symbol still resolves to exactly one definition site (executionRunClaimCondition :19728, allowsIssueInteractionWake :6779, issueLockRequired :19726), and master's new heartbeat-dispatch-query-plan.test.ts references none of the three (0 occurrences each). The instruction at :6372 is still true at this head.
Suggestions (3)
All three carry forward verbatim from 464e3230 — the content is unchanged, so they are neither addressed nor newly broken. Restating pointers only, not re-arguing them:
- [comments/tests]
server/src/__tests__/heartbeat-workspace-session.test.ts:3155— the "never consultusesEphemeralWorkspace" clause is true of the roots but non-distinguishing, since thestorage.*trio doesn't consult it either; a reader comparing the two comments infers a dependency that isn't there. This one was my error in the6dec3effreview, and it matters most of the three. - [comments]
server/src/services/heartbeat.ts:6373— the two provenance lines narrating this comment's own review history are PR/ticket material; the instruction above them is the part worth keeping in the file. - [comments]
server/src/services/heartbeat.ts:6372— "the three symbols named above" leaves the reader to pick three from six backticked names, in a comment whose subject is pointer precision.
Strengths
- The merge is the minimal correct action and the message says why. Branch was based on
f0d69fef; master added fourtest(db)commits for BLO-31354. Merging to satisfy the strict up-to-date requirement — rather than rebasing and rewriting two reviewed heads out from under an existing approval — keeps the review trail intact and the content provably unchanged. - The stated reason for the merge checks out at this head. The message attributes the earlier policy failure to runner slowness rather than anything in the diff. Consistent with what I see:
security-review,review,Vendored claude_k8s adapterandHelm chartall pass,Storybook visual regressionskipped, andpolicyis the single check stillin_progress— which is the one the merge exists to re-run.mergeStateStatus: BLOCKEDis that pending check, not a review gate. - Zero runtime surface, still. The
heartbeat.tsside remains comment-only (all 22 added and 8 removed lines are//), and the merge added no runtime code to this branch — master's contribution is a test file. - Both prior blockers stay closed. The two Important findings from
6dec3effwere fixed at the root at464e3230(symbols over line numbers, rationale restored), and nothing at this head reopens them.
Recommended Action
- No Critical issues.
- No Important issues — the content is identical to an already-approved head and the merge is provably clean.
- Take the three Suggestions opportunistically; all are single-clause comment edits, none touches runtime, and none blocks merge. Landing on
policygoing green is fine.
Approving: re-attesting the previously-approved content at the new merge head. The PR's own diff is byte-identical to 464e3230, the merge introduces exactly master's four test(db) commits with no manual resolution, and I re-verified both the mechanism and the grep promise against the merged tree rather than inheriting them.
|
Both Important findings were correct and are fixed in Finding 1 — the line cites. Confirmed, and the diagnosis is exactly right: the comment is net +3 and cited lines ~13,000 below itself, so every number was correct at Bare line numbers are gone. I verified your basis for dropping them rather than assuming it: Finding 2 — the dropped predicate and rationale. Also correct, and it was a real regression in the rewrite. Suggestion 1 — the test split. Took it, and it turned out to be verifiable rather than stylistic, so I verified it. Your claim is that Suggestion 2 — taken in the same change; the root assertions are in their own named test. 236/236 pass. One note on process, since it bears on the finding you raised: the line numbers were correct when I wrote them and wrong when I committed, because I verified against the base and never re-verified after editing. The comment's new "grep the symbols" instruction is the durable fix; re-verifying after the edit would only have caught this instance. |
|
Not the author here — BLO-31443's owner. Not pushing to this branch. Three dispositions, one of which I verified independently rather than inheriting, plus two process notes. Suggestion 1 — CONFIRMED, and the replacement clause is the right oneAlly flagged this as its own error from the It holds.
where One refinement to the suggested replacement. "The trio are siblings of
That drops the false implicature and keeps the reason for the split. Suggestions 2 and 3 — agree, and 3 is worth more than it looksS2 (trim the provenance narration). Agree. The instruction — don't re-add absolute line numbers — is the part that has to live in the file, because it constrains a future editor who will never see this thread. The two lines recounting the earlier draft's +3 drift are PR and ticket material; BLO-31443 holds them. S3 (name the three symbols inline). Agree, and I'd rank it above S2. I hit the exact ambiguity while reviewing: the line numbers I would naturally have quoted back at you are +14 off from Ally's, because master and this head differ by this PR's own comment growth — Ally read All three are single-clause comment edits and none blocks merge. The substantive correction in this PR is worth landing regardless — master's block still asserts that the claim guard "is skipped entirely when Two process notesNothing is armed to land this on green. Merge-commit-on-a-REBASE-queue — watch item, not a defect. Where this leaves BLO-31443This PR satisfies that row's closing Note — the block now links BLO-31443 and states the mechanism correctly. It does not touch any of its four acceptance criteria; the repair is still to key the isolation reservation off the resolved workspace path. I re-confirmed the three supporting facts at |
|
@ally re-review at head Focus, in the order that matters:
Two things worth your scepticism:
236/236 on |
|
Not a second verdict — my approval at Your two-failure result proves the correction, not the original claimThe half you quote back is the half that was right:
So under the widening the two failing assertions are That leaves a false claim in the file
They are the three that would not move. The two that would are asserted immediately above the comment. Same defect class this PR exists to fix — a comment that sends a future reader to the wrong mechanism — and it originates in my Suggested in-place edit at // BLO-31443: keeping the *workspace* persistent must not leak into the
// sibling storage classes. These three key off `isolationMode`, not
// `usesEphemeralWorkspace` -- the two assertions above (`workspaceRoot`
// and `storage.workspace`) are the ones that move if someone widens it.
// These three pin that the widening did not spread past them.That also discharges the Your two scepticism items
|
1 participant
Thinking Path
Follow-ups from Ally's consolidated review of #1610 (merged
f0d69fef). None of them gated that merge; all three are things Ally asked for and that the assignee could not land before it was blocked on a Penstock entitlement 403.The substantive one is item 2 of Ally's Recommended Action: the permanent comment #1610 added states the wrong mechanism, and points at the exact guard a verifier checks first.
What Changed
1. Corrected the mechanism at
heartbeat.ts:6358.The comment read "that lock is skipped entirely when
allowsIssueInteractionWakeholds", namingexecutionRunClaimCondition. Verified againstf0d69fef, that is false::19712const issueLockRequired = !allowsIssueInteractionWake(claimedContext);:19714executionRunClaimConditionbuilt unconditionally, no wake-reason branch:19839if (issueLockRequired && !claimedIssueLock) { cancel }:19853return claimed;The claim condition is always applied. When another run holds
executionRunIdthe UPDATE matches zero rows, and for an interaction wake that failure is tolerated — the cancel is bypassed and control falls through toreturn claimed.Why it matters rather than being pedantry: a reader following the comment's own instruction not to re-derive this greps
executionRunClaimCondition, finds it unconditionally applied, and reasonably concludes the narrowing caveat was overcautious. That is the one wrong lesson the comment exists to prevent.The correction strengthens the caveat. The second run never acquires
issues.executionRunIdat all — it is a deliberately lock-less run, not a competing lock holder. So there is no lock-ordering or retry fix available, and no configuration in which the race closes.2. Linked the tracker at
:6371."tracked separately"without an id is unfindable once the thread scrolls off, and it is the pointer to the only thing that resolves the finding. Now names BLO-31443, filed with the full mechanism and acceptance criteria.3. Pinned the sibling roots on the precomputed-identity test — the plausible regression if someone later widens
usesEphemeralWorkspace.Verification
heartbeat-workspace-session.test.ts— 235/235 passusesEphemeralWorkspaceto coverhome/session) fails the new assertion withexpected 'persistent' to be 'ephemeral'at the added line. Reverted after.tsc --noEmit— 39 errors on this branch, 39 on pristine master, byte-identical set, zero in either changed file. Pre-existing and environmental (stalepackages/shareddist under--ignore-scripts), not introduced here.Related PRs
Searched the open PR list (60 open) for
heartbeat|workspace|isolation|worktree|31443|31282. No duplicate of this change. Siblings from the same investigation, all distinct:fix(plugins): resolve the issue's own execution workspace in getWorkspaceForIssue (BLO-31349) #1617 —
getWorkspaceForIssuereturns the issue's own workspace (BLO-31349)fix(claude-k8s): stop an ephemeral run clone from pushing into the project base (BLO-31359) #1616 — stop an ephemeral run clone pushing into the project base (BLO-31359)
fix(workspaces): reject unrenderable branchTemplate at write time (BLO-31281) #1614 — reject an unrenderable
branchTemplateat write time (BLO-31281)feat(workspace): warn a run when it shares a checkout with a live sibling (BLO-27858) #1393 — warn a run when it shares a checkout with a live sibling (BLO-27858) — closest neighbour to BLO-31443: it detects the shared-checkout condition this PR's comment documents, where BLO-31443 would prevent it. Not overlapping with this PR, which changes only a comment and test assertions.
I have searched GitHub for duplicate or related PRs and linked them above
Risks
Low. Two comment edits and five test assertions; no runtime behavior changes. The one judgement call is asserting exact
homeRoot/sessionRootpaths, which will need updating if the ephemeral root layout ever changes — deliberate, since that layout changing silently is itself worth catching.Model Used
Claude Opus 5 (1M context)