feat(engine,client): session fork with lazy provider re-binding - #524
feat(engine,client): session fork with lazy provider re-binding#524Zerlight wants to merge 9 commits into
Conversation
…d carry claude subagent transcripts
…history while the source exists
…prefix, and keep it out of notifications until it commits
…in flight and carry the child's MCP warnings
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Minor suggestions inline.
Reviewed changes — the full 48-file diff at 27ac29b, plus the surrounding source for every claim below: fork-service.ts end to end, session-record-registry.ts, checkpoint-service.ts, lineage-attribution.ts, lineage.ts, use-workbench-sessions.ts, both adapter changes, and the installed pi SDK .d.ts/.js to check the session-ref emission. Scoped gates run green: vitest run packages/host/engine packages/host/agent-adapter/src/__tests__ packages/presentation/ui/src/chat packages/client/workbench packages/foundation/schema → 223 files, 2058 tests, 0 failures.
The saga holds up
The fork critical section is the part most likely to go wrong, and it doesn't. operationId idempotency, the per-session semaphore, expectedGraphRevision optimistic concurrency, and the Effect.onExit → abandon compensation together mean a failed fork leaves no listed session and no persisted row. Provisional records being in-memory, unlisted, unannounced and unpersisted until the fork transaction commits is the right shape for that — the child only becomes real when the graph copy does.
Two things I specifically checked rather than assumed:
- The all-or-nothing backfill gate in
checkpoint-service.ts:124-131is sound.copiedPrefixonly suppresses durable binding writes;attributionis still returned, so rendering is unaffected and a partially-attributed copied prefix never gets half its turns permanently bound to the wrong provider messages. - The provisional record identity makes the orphan log resolve.
registerProvisionalstores the same object referencebindHistoryIdlater mutates in place, sochild.runs[0]?.historyIdinabandonreads the bound id rather thanundefined.
Wire 82 is correctly additive
WIRE_PROTOCOL_VERSION 81 → 82 with MIN_COMPATIBLE_WIRE_VERSION held at 76 is right: session.fork is a new variant, nothing was removed, renamed, or re-meaned. The client gates on SESSION_FORK_WIRE_VERSION rather than assuming, so an older daemon degrades to "no fork affordance" instead of a dropped frame.
Things I chased and cleared
Recording these so they don't get re-litigated on the next pass:
- The fork affordance is enabled, not dead.
lineage.ts:157-161writes alineageVersionsentry for every path turn, withstate: nullfor completed ones, so the gate resolves true on a normal agent reply. Wherelineageis undefined,onForkTurnis also absent, so both gates fail together — consistent, not a half-wired button. - Fork errors do surface.
useMutation(forkSession, { onError })matches every sibling mutation in the file, and the doc comment ("Rejections propagate to the caller … and reachonError") is accurate. The.catch(noop)suppresses only the already-handled secondary rejection, not the primary one. - The pi
session-refemission change is required. Wideningif (this.resumeFrom)toif (manager)looks like it might announce on fresh sessions, butmanageris truthy only on the branch or resume paths. The old code emitted nothing on the branch path, which violated theAGENTS.mdcontract that pi "announce the resumed/branched id at start".AgentSession.sessionIddelegates tosessionManager.getSessionId(), which preserves the file's header id onopenand assigns a new one on branch — so resumes still announce the same id and branches announce the new one.
Scope and follow-ups
This is stacked on ruocheng/code-638, so the diff is this PR's commits only and merge order matters. Deferred work that I'd want tracked rather than forgotten: opencode forkAfterTurn (CODE-633), the codex binding copy, provider-native lineage metadata, and mobile. One behavioral consequence worth naming in the PR description — an open fork operation blocks submit and stop on the source session via hasOpenOperation for up to LAUNCH_TIMEOUT_MS (300000 ms / 5 min) if the child launch hangs. That's the correct trade for consistency, but it is a five-minute user-visible freeze in the bad case.
The one coverage gap I noticed: nothing exercises editing a turn inside a copied prefix after re-binding against a real provider. The unit tests cover the binding logic and the fork saga separately, but not that specific interaction.
Nitpicks
engine-session-fork.test.tsre-declaresstartedHarness,twoCheckpointedTurns,ForkingAdapter,cursorRow,assistantRow, andHarness, all of which already exist inengine-turn-submit.test.tsorengine-conversation-read.test.ts. A shared test-helper module would keep the three suites from drifting apart.
Claude Opus ｜ 𝕏
| protected async copySubagentTranscripts(sourceId: string, childId: string): Promise<void> { | ||
| try { | ||
| await copyClaudeSubagentTranscripts( | ||
| path.join(homedir(), '.claude', 'projects'), | ||
| sourceId, | ||
| childId, | ||
| ); | ||
| } catch (error) { | ||
| this.emitError( | ||
| `claude-code: subagent transcripts were not copied into the fork: ${extractErrorMessage(error)}`, | ||
| 'fork_subagents_not_copied', | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
copyClaudeSubagentTranscripts returns a documented boolean ("Returns whether anything was copied"), and this discards it. Only a thrown error reaches emitError(..., 'fork_subagents_not_copied') — the three early return false paths stay silent.
Two of those are benign (no subagents/ to copy, id fails SAFE_SESSION_ID), but childDir === undefined is not: the child project directory should exist by the time this runs, so its absence means the copy silently did not happen. AGENTS.md promises the opposite — "a silent copy failure would be a silently empty subagent card, so it is reported as a recoverable error event instead".
The new claude-code-fork-subagents.test.ts:74 pins exactly the false that production throws away, which makes the mismatch concrete. Either branch on the return value here, or drop the boolean from the helper so the contract reads honestly.
| Effect.logWarning('Abandoned a session fork; its provider child history is orphaned', { | ||
| sessionId: child.forkOrigin?.sourceSessionId, | ||
| historyId: child.runs[0]?.historyId, | ||
| }), |
There was a problem hiding this comment.
This logs the source session id under the sessionId key, while the sibling logError twelve lines above logs child.sessionId under the same key. The child's own id — the one an operator needs to correlate the orphaned provider history back to a record — never appears in this warning at all.
| Effect.logWarning('Abandoned a session fork; its provider child history is orphaned', { | |
| sessionId: child.forkOrigin?.sourceSessionId, | |
| historyId: child.runs[0]?.historyId, | |
| }), | |
| Effect.logWarning('Abandoned a session fork; its provider child history is orphaned', { | |
| sessionId: child.sessionId, | |
| sourceSessionId: child.forkOrigin?.sourceSessionId, | |
| historyId: child.runs[0]?.historyId, | |
| }), |
1 participant
Summary
Phase 5 of CODE-627 — Conversation turn graph & immutable attachment store. Linear: https://linear.app/arcbox/issue/CODE-639/featengine-session-fork-with-lazy-provider-re-binding
Stack: #517 ← this PR (
ruocheng/code-639, baseruocheng/code-638) ← #525. Merge bottom-up; this PR's diff is only its own commits.session.fork { sourceSessionId, throughTurnId, operationId, expectedGraphRevision }forks a session through a completed turn onto a provider-native copy: the child holds the chosen prefix (the selected turn included, its suffix excluded), shares current file state, and never re-executes history. The saga mirrorsturn.submit: replay by operation id → admit under the source's critical section (typedbusywhile a turn runs or another operation is open; the through turn must be completed with a usable checkpoint; the graph revision must match) and persist the open operation → provider fork and child adapter start outside it under the launch budget, the child record held provisionally so itssession-refand status bind before it exists durably → one transaction commits the child record, its copied prefix (new turn ids, shared prompt and attachment references) and the operation. The source is never stopped or switched. Copied turns start with no child-history binding; one cold read re-derives bindings only when the user-row count matches exactly and per-position prompt fingerprints agree — any mismatch leaves all bindings absent, because a wrong binding forks at a wrong cut. The copied prefix renders from the source history while it exists (claude'sforkSessionre-stamps the copied rows) and from the child's own copy once the source is gone. Adapters: claude announces the child session-ref at fork and copiessubagents/; pi announces its resumed or branched id at start. Client:forkSessionbehind a wire-version gate, "Fork a new thread from here" on completed turns (withheld while a fork is in flight), dev mock parity.Commits
Verification
Every commit passed
pnpm check:ciandpnpm testat its own tip; the tip (27ac29b9) is atpnpm check:ci0 errors,pnpm test3463 passed / 1 skipped. Adversarial review pair (engine/store/saga axis and client/adapter axis, isolated read-only worktrees): both ACCEPT-WITH-FINDINGS, no P1; each finding was reproduced with a failing test before its fix — the record is on CODE-639. Real development daemon with live Claude Code, no paid turn for the fork itself: a wire probe forked an existing thread through its second turn in 1.7 s, and a headless-Chrome probe of the webview drove the same fork from the UI; a later run on CODE-640 forked a live source on a managed worktree against the real provider (distinct claude history, copied prefix rendered from the source while it existed and from the child's copy after the source was deleted). Not exercised on a real provider: editing a copied-prefix turn after re-binding; a fork while the source is mid-turn is refused typedbusyby design. Deferred, recorded on the issue: opencode tip-fork staysunsupporteduntil CODE-633 verifies it; codex binding copy; provider-native lineage metadata; mobile.Checklist
pnpm check:ciandpnpm testboth pass (no Rust changes)session.fork/session.forkedframes and thesession.forkoperation kind); floor unchanged at 76packages/host/agent-adapter/AGENTS.mdand module docs in this branch)