fix(core): mint the CLI session id instead of inferring it (#357) - #431
fix(core): mint the CLI session id instead of inferring it (#357)#431edspencer wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThe PR adds UUID-based CLI session IDs, expected-transcript discovery with exit-aware fallback, fake CLI support for explicit IDs, and deterministic session attribution based on ChangesCLI session identity and discovery
Deterministic session attribution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant cliRuntime
participant claudeCli
participant sessionPath
cliRuntime->>claudeCli: Pass minted --session-id
claudeCli->>sessionPath: Write transcript with explicit ID
cliRuntime->>sessionPath: Wait for expected transcript
sessionPath-->>cliRuntime: Return match or fallback result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying herdctl with
|
| Latest commit: |
96150c1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://479dba24.herdctl.pages.dev |
| Branch Preview URL: | https://fix-357-minted-session-id.herdctl.pages.dev |
edspencer
left a comment
edspencer
left a comment
There was a problem hiding this comment.
This PR fixes co-located session-id collisions by minting the CLI session id (--session-id <uuid>) instead of inferring the transcript file, with a deadline fallback to the old inference and an HERDCTL_CLI_MINT_SESSION_ID=0 escape hatch. It also makes the incremental AttributionIndexBuilder resolve doubly-claimed session ids deterministically (newest started_at wins, tie-break on job id) instead of by Promise.all completion order.
I read cli-session-path.ts, cli-runtime.ts (spawn/mint/watch flow), session-attribution.ts, the JobMetadataSchema (confirmed started_at/id are always present, so claimWins never dereferences undefined), and job-metadata.ts's listJobs ordering. The mint logic, the "keep waiting for exactly our file while polling" invariant, and the new tie-break all look correct and well-tested. Two lower-severity gaps below.
| if (held && !claimWins(entry, held)) continue; | ||
| winners.set(entry.sessionId, { startedAt: entry.startedAt, jobId: entry.jobId }); | ||
| index.set(entry.sessionId, { agent: entry.agent, triggerType: entry.triggerType }); | ||
| } |
There was a problem hiding this comment.
LOW · correctness — Non-incremental buildJobIndex still resolves collisions by order (oldest-wins, non-deterministic on ties)
This PR makes the incremental AttributionIndexBuilder resolve a doubly-claimed session id deterministically (newest started_at wins, tie-break on job id). But its sibling buildJobIndex (lines 100-115), used by the exported buildAttributionIndex, was left unchanged: it iterates result.jobs and does last-write-wins via index.set.
listJobs sorts jobs by started_at descending (job-metadata.ts:443-447), so last-write-wins there resolves a collision to the oldest claim — the opposite of the new newest-wins rule this PR argues is correct (adopt/promote should win). And for exact started_at ties the result still depends on V8's stable sort preserving readdir order, i.e. the same filesystem-order non-determinism this PR set out to remove.
The two builders are documented as parallel (createAttributionIndex is "Shared by the full buildAttributionIndex and the incremental AttributionIndexBuilder"), so having them disagree on collision resolution is a latent surprise. buildAttributionIndex currently has no production callers besides being a public export, which is why this is low rather than higher — but it's still reachable API. Consider applying the same claimWins ordering in buildJobIndex, or documenting that the full builder is deprecated in favor of the incremental one.
| // file was ours (issue #357). A plain resume keeps its existing id and needs | ||
| // nothing. Opt out with HERDCTL_CLI_MINT_SESSION_ID=0. | ||
| const mintedSessionId = | ||
| (!options.resume || options.fork) && process.env.HERDCTL_CLI_MINT_SESSION_ID !== "0" |
There was a problem hiding this comment.
LOW · bug — --session-id defaults on; an older CLI that errors on the unknown flag fails every new-session turn
mintedSessionId is enabled by default (HERDCTL_CLI_MINT_SESSION_ID !== "0") and pushed as --session-id. The deadline fallback in waitForNewSessionFile only helps if the CLI ignores an unknown flag. If an older claude binary instead errors on the unknown --session-id flag, the subprocess exits non-zero and no fallback runs — every fresh/fork turn fails loudly, by default, with no minimum-CLI-version guard in the repo.
The author acknowledged this in the PR body ("One thing I could not verify"), and the env escape hatch exists — recording it here so it isn't lost in prose. Worth either documenting a minimum Claude Code version, or gating the flag behind a capability probe, before this ships as the default.
|
Verified the three spawn shapes core produces:
The key subtlety — writing to the sessionId-named file but reading prior text from the resume source — is handled correctly, and Reviewed 1 changed file (3 hunks); 0 findings. |
|
Pushed The harness's fake minted its own uuid and ignored Verified load-bearing rather than assumed: with the fake reverted, that one job test fails and takes 48 s; with the fix, the whole suite is 27 passed in 55 s. (One flaky —
This is the same companion change paddock's |
edspencer
left a comment
edspencer
left a comment
There was a problem hiding this comment.
Companion test-fixture change: the fake claude CLI used by @herdctl/web's UI/integration tests now honours --session-id, matching core's new mint-and-pass behaviour (#357). Without it, every new-session turn in the web tests would burn the full 60s timeout waiting for a file named after the minted id the fixture ignored.
I read the full fixture and cross-checked its flag handling against how core actually passes --resume/--fork-session/--session-id (verified in the sibling core PR): new session, plain resume, and fork all resolve to the correct transcript filename and continuity source. No issues found — the change is correct and well-commented.
waitForNewSessionFile worked out which transcript a freshly-spawned claude had created by inspecting the session directory. #357 fixed half of that with a pre-spawn snapshot, so a co-located agent APPENDING to its own session can no longer be mistaken for ours. The other half stayed a guess — 'if several appeared, the newest is ours' — and the newest is not ours, it is whoever spawned last. Two agents sharing a working directory share one session dir, so two concurrent resume:null spawns trade session ids. Claude Code accepts --session-id <uuid> and names the transcript after it (composing with --fork-session), so a new-session turn now mints its own id and passes it; the file is known by name. While polling, the inference paths are disabled — taking whichever brand-new file shows up is the collision we are removing. Only if the deadline passes without our file appearing (a CLI that ignores the flag) do we warn and fall back. HERDCTL_CLI_MINT_SESSION_ID=0 opts out. Also implement #357's proposed secondary hardening: AttributionIndexBuilder filled its map in Promise.all completion order, so when two records claimed one session id the winner was whichever file's stat+parse finished last — decided by record size and machine load (enlarging either record flips it, 10/10 each way). Order claims explicitly: newest started_at wins, ties broken on job id. Newest rather than first-owner because adopt/promote legitimately change a session's owner and write a newer record. Found via paddock#548. Co-Authored-By: Claude <noreply@anthropic.com>
The web UI harness drives a fake `claude` that minted its own uuid and ignored --session-id. Now that the runtime passes the id it minted, the fake wrote a differently-named transcript, so every new-session turn waited out the full 60s timeout before falling back to inference — the job-trigger specs failed and the suite took 8.5 minutes. --session-id now names the transcript whenever supplied, winning over --resume so a fork lands in a new file instead of appending to its source; continuity is read from the resumed session, which for a fork is that source. Full web UI suite: 55s, down from 8.5 minutes. Co-Authored-By: Claude <noreply@anthropic.com>
Rebasing onto main surfaced three more test files (herdctl#423 CLAUDE_CONFIG_DIR / claude-home threading) whose fake spawners write a session id of their own choosing and ignore --session-id. They did not fail because the fix was wrong; they failed because waiting out the whole 60s timeout is an unreasonable cost to impose on every fake-CLI harness in the ecosystem — this was the third one. So stop waiting on the clock and start waiting on evidence. While the process is alive we still refuse to infer, which is the guarantee that matters: a co-located agent's brand-new file must not be claimed just because it landed first. But once the CLI has EXITED without writing the file we asked for, it never will, so fall back immediately (after a 500ms grace for a late flush). All three #423 files now pass UNMODIFIED — no other author's tests touched. Co-Authored-By: Claude <noreply@anthropic.com>
0682236 to
96150c1
Compare
|
Rebased onto The conflict was trivial; what it revealed was notThe only conflict was additive-vs-additive in But the rebase also pulled in three test files from #423 ( They were not failing because the fix is wrong. They were failing because waiting out the full 60 s timeout is an unreasonable cost to impose on every fake-CLI harness — and this was the third one (herdctl's web-UI fake, these, and paddock's still to come). So: wait on evidence, not on the clock
All three #423 files now pass unmodified — no other author's tests touched. Two new tests cover it: the fast fallback completes in <5 s against a 30 s timeout, and the co-located race still resolves to our file while the process is alive.
The |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/core/src/state/__tests__/session-attribution.test.ts (3)
931-939: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse one
AttributionIndexBuilderin the loop.The loop creates a new builder each time. It does not exercise cached claim metadata. Create the builder before the loop, then call
buildrepeatedly on that instance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/state/__tests__/session-attribution.test.ts` around lines 931 - 939, Update the “is stable across repeated builds” test to instantiate a single AttributionIndexBuilder before the loop and reuse that instance for every build(tempDir) call, so repeated builds exercise cached claim metadata while preserving the existing seen-set assertion.
896-912: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit helper types.
Define an interface for
opts. Add thePromise<void>return type towriteCompeting.As per coding guidelines, “Use strict TypeScript with explicit types” and “Prefer
interfaceovertypefor defining object shapes in TypeScript”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/state/__tests__/session-attribution.test.ts` around lines 896 - 912, Update the writeCompeting helper to return Promise<void> explicitly, and define its opts parameter through a dedicated interface rather than an inline object type. Use the interface for the keeperFirst property while preserving the helper’s existing behavior.Source: Coding guidelines
914-978: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMock file-system operations in these unit tests.
These tests create directories and files through
node:fs/promises. Mock the file-system dependency and control the directory-entry order in the test fixture.As per coding guidelines, “Mock external dependencies (SDK, file system, GitHub API) in tests”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/state/__tests__/session-attribution.test.ts` around lines 914 - 978, Mock the node:fs/promises operations used by the attribution tests instead of creating real directories and files through writeCompeting and writeJobFile. Configure the mock fixture to return controlled directory-entry orders for each build, while preserving the existing assertions for newest claims, repeated-build stability, re-attribution, and timestamp tie-breaking in AttributionIndexBuilder.Source: Coding guidelines
packages/core/src/state/session-attribution.ts (1)
396-402: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine named interfaces for claim metadata.
Lines 396-421 and Line 543 define related object shapes inline. Define interfaces such as
SessionClaimOrderandCachedSessionClaim. Use them forclaimWins,CachedJobFile.entry, andwinners.As per coding guidelines, “Prefer
interfaceovertypefor defining object shapes in TypeScript”.Also applies to: 413-421, 542-549
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/state/session-attribution.ts` around lines 396 - 402, Define named interfaces for the related claim metadata object shapes, such as SessionClaimOrder and CachedSessionClaim, near the existing claim logic. Replace the inline shapes used by claimWins, CachedJobFile.entry, and winners with the appropriate interfaces while preserving their current fields and behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/state/__tests__/session-attribution.test.ts`:
- Around line 931-939: Update the “is stable across repeated builds” test to
instantiate a single AttributionIndexBuilder before the loop and reuse that
instance for every build(tempDir) call, so repeated builds exercise cached claim
metadata while preserving the existing seen-set assertion.
- Around line 896-912: Update the writeCompeting helper to return Promise<void>
explicitly, and define its opts parameter through a dedicated interface rather
than an inline object type. Use the interface for the keeperFirst property while
preserving the helper’s existing behavior.
- Around line 914-978: Mock the node:fs/promises operations used by the
attribution tests instead of creating real directories and files through
writeCompeting and writeJobFile. Configure the mock fixture to return controlled
directory-entry orders for each build, while preserving the existing assertions
for newest claims, repeated-build stability, re-attribution, and timestamp
tie-breaking in AttributionIndexBuilder.
In `@packages/core/src/state/session-attribution.ts`:
- Around line 396-402: Define named interfaces for the related claim metadata
object shapes, such as SessionClaimOrder and CachedSessionClaim, near the
existing claim logic. Replace the inline shapes used by claimWins,
CachedJobFile.entry, and winners with the appropriate interfaces while
preserving their current fields and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e57c3b1-a3fb-4e79-a0ff-b7d6a5d42fa2
📒 Files selected for processing (7)
.changeset/cli-minted-session-id.mdpackages/core/src/runner/runtime/__tests__/cli-session-path.test.tspackages/core/src/runner/runtime/cli-runtime.tspackages/core/src/runner/runtime/cli-session-path.tspackages/core/src/state/__tests__/session-attribution.test.tspackages/core/src/state/session-attribution.tspackages/web/test-ui/fixtures/bin/claude
1 participant
Reopens and finishes #357. Found via paddock#548.
What #357 left behind
#357 fixed the case where a co-located agent is appending to its own session:
a pre-spawn snapshot identifies the new transcript by set difference instead of
by mtime. It did not fix the case where a co-located agent creates a file,
and said so in a comment:
The newest is not ours — it is whoever spawned last. Two agents sharing a
working_directoryshare one~/.claude/projects/<encoded-cwd>/, so twoconcurrent
resume:nullspawns produce two brand-new files that areindistinguishable by name or mtime. The agents trade session ids: each
adopts the other's transcript and writes a job record claiming it.
Reproduced against a build of
main(5.27.0), no paddock and no vitest —pre-existing file, snapshot, write
bbbb.jsonl, sleep 20 ms, writecccc.jsonl:The fix: stop inferring
Claude Code accepts
--session-id <uuid>and names the transcript after it. Iverified both that the file is named exactly as requested and that it composes
with
--fork-session(the fork wrote the requested id).So a turn that starts a new session — a fresh run, or a fork — now mints its own
id and passes it. The transcript is known by name: no snapshot, no mtime, no
tie-break. A plain resume already knows its id and is untouched.
Two details that matter:
whichever brand-new file shows up" mid-wait would reintroduce exactly the
collision this removes. Only once the deadline passes without our file
appearing — a CLI too old to know the flag — do we warn and fall back, so the
turn still runs rather than failing.
HERDCTL_CLI_MINT_SESSION_ID=0restores the old behaviour.Verified under the hostile ordering: with the co-located agent's file created
first and newest by mtime, we still take ours.
Also: #357's "secondary hardening", which was never implemented
#357 proposed it in as many words:
AttributionIndexBuilderfilled its session→agent map by iterating a cachepopulated inside an unordered
Promise.all, thenindex.set(sessionId, …)—last writer wins. So when two records claim one session id, the winner is
whichever file's stat+parse finished last.
That is not random, which is worse than it sounds — it is a function of record
size and machine load. With the same two records on disk, enlarging one flips the
result:
keeper10/10sweeper10/10Claims are now ordered explicitly: newest
started_atwins, ties broken on jobid, so the answer is a pure function of what is on disk.
Newest-wins rather than the literal "first owner keeps it" #357 suggested,
because adopting or promoting a session is a legitimate change of owner —
paddock's
writeAgentAdoptionJobandreattributeSessiondo exactly that, and ahard owner lock would break them. (
reattributeSessionrewrites every matchingrecord in place, so a promote leaves no conflict at all; the ordering rule is
what covers adoption and any accidental collision that still gets through.)
Tests
7 new cases, all of which fail against the current source and pass here:
packages/core: 3488 passed, 1 pre-existing failure(
directory.test.ts"parent directory is not writable" — fails identically on aclean checkout when the suite runs as root, since root can write to the
restricted dir). Typecheck and lint clean.
paddock's integration suite drives a fake
claude(test/bin/claude) thatmints its own uuid and ignores
--session-id. Against this version the expectedfile would never appear, so every new-session turn would burn the full 60 s
timeout before falling back — technically passing, practically unusable.
test/bin/claudeneeds to honour--session-idwhen present. Happy to open thatPR alongside; it should land before paddock bumps
@herdctl/core.One thing I could not verify
I don't know which Claude Code version introduced
--session-id(confirmedpresent in 2.1.220), and herdctl declares no minimum. If an older CLI errors on
an unknown flag rather than ignoring it, the deadline fallback never gets a
chance and the turn fails loudly. That is the reason for the env escape hatch. If
you know the introducing version, a documented minimum would be better than the
fallback.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes