Worktree V2 Phase 4: isolated conversation forks - #50
Merged
gannonh merged 30 commits intoAug 8, 2026
Conversation
added 26 commits
August 7, 2026 16:29
Add the Worktree V2 Phase 4 isolated-conversation-fork contract to the shared package: shared-worktree | isolated-worktree strategies, typed blocker codes, renderer-safe preview/confirm DTOs, and the durable pending provider-fork intent that structurally cannot claim a child provider ID before first Send. Wire WORKTREE_FORK_* wire error codes, git:fork* RPC channels, and routing classification. Add the strict provider capability gate (resolveIsolatedForkCapability) and the credential-free deterministic strict fork adapter with failure injection and full four-category destination-execution proofs, mirroring the Phase 3 handoff capability surface. Refs #43
The conversation-fork status contract must represent both pending and established child provider identity: before first Send it displays as pending, and after the strict adapter establishes the native fork the persisted child provider ID is exposed. Replace the fixed pendingProviderIdentity flag with a pending | established discriminated union so the 'established' state is representable. Refs #43
Mirror the handoff contract: the blocker tuple is the single source of truth for its union, so adding or removing a code must be a deliberate contract change visible in the test. Refs #43
- ConversationForkConfirmInput.worktreeNameSuffix is optional (required for isolated-worktree only; server enforces); absent for shared-worktree. - Clarify that fork recovery outcomes surface via recovery-required results and the recovery-state union, not a separate outcome-code set. - Pin recovery-state union length (13) in the contract test. - Rename the deterministic-adapter idempotency test to match what it actually asserts (stable child ID on retry; persist-once dedupe is the fork service's responsibility). Refs #43
IsolatedConversationForkService (Worktree V2 Phase 4): the eligibility preview surface and fingerprinted seed capture that Task 3's confirm transaction will build on. - preview(): serialized evaluation returning typed blocked previews (never throws) for unsupported-provider, non-head-source, source-active, path-unleased, invalid-name, name-collision, flags-disabled, cleanup-in-progress, fork-in-progress, missing-source, git-operation-in-progress, unsupported-snapshot, and oversized-capture; eligible previews bind source/destination/name/capability/owner/lease facts into a previewFingerprint and register a pending isolated transaction through the durable fork journal. Shared-worktree previews stay unblocked without a provider capability (the existing path) and own no transaction. - captureForkSeed(): captures a fingerprinted Phase 2 seed at the source checkout's exact captured HEAD (staged + unstaged + eligible untracked + .worktreeinclude, CAS-pinned via the snapshot service's hidden ref) without cleaning or changing the source; limit failures map to a typed FORK_SEED_LIMIT error. removeSeed() verifies, CAS-deletes the owned ref, and drops only the payload. - Composed into GitServices as svc.fork with forkHooks (session/capability resolution, activity, quiescence); 'fork' added to the journal op union. - Session DTO gains optional isolatedForkCapable (parallel to handoffCapable), computed through the strict fork capability gate. TDD: 27 vertical-slice tests (real temp repos, deterministic strict adapters) covering the blocker matrix on current/managed/shared sources, current-head enforcement, canonicalized current-checkout leases, seed capture fidelity and source immutability, and seed removal. Refs #43
Shared-worktree forks capture no seed, so detached-HEAD and snapshot-state feasibility checks must not narrow shared eligibility. Review finding from task2-quality-review (Important): gate assertSupportedState and the detached-HEAD check on isIsolated; add a shared-strategy regression test. Refs #43
…able journal Implement IsolatedConversationForkService.confirm: the durable target/child transaction core for Worktree V2 Phase 4 isolated conversation forks. Under the common-directory mutation lock + registry runExclusive the confirm revalidates the preview fingerprint exactly (identity-drift blocker), re-checks source-active/path-unleased (every source owner must hold a stable lease)/name-collision/ fork-in-progress/git-op/cleanup blockers, journals the target reservation and seed id before materialization, captures the seed at the source head, materializes and restores the managed target, verifies the restored content byte-for-byte, creates the child session through the new ConversationForkHooks.createForkChildSession contract (typed hook-not-wired error when absent), commits the child as sole registry owner, and writes the journal commit marker before the child is visible. Pre-publication failures compensate only transaction-owned artifacts with CAS proof (branch only while it still points at the journaled OID, target only beneath the server root with the created owner set, seed and child only when created by this transaction) and journal the rollback. A repeated confirm with the same transactionId resumes from the journal - in-progress entries replay forward, rolled-back entries re-run once, committed entries return the committed summary - never double-creating target/child/owner. Extend gcOrphanedSnapshots retention to in-progress/failed fork journal entries so an interrupted confirm's seed survives until commit. Refs #43
…erialization An interrupted fork journal whose destination was already materialized was permanently unresumable: confirm-time revalidation treated the transaction's own destination as a name-collision (and its own lease as fingerprint drift), then failed the entry. Revalidation now treats the transaction's own materialized destination as the as-of-preview value (collision checks skipped, own lease excluded from the fingerprint), so replay completes exactly once instead of stranding the target record and seed. Regression test drives the journal through target-verified with a real materialized target and re-confirms on a fresh service instance. Review finding from task3a-quality-review (Important). Refs #43
Add the recovery and cancellation surface to IsolatedConversationForkService, mirroring WorktreeHandoffService: - status(): active:false when no in-memory transaction and no in-progress fork journal entry exists; otherwise the active transaction with its strategy, state (durable journal metadata state preferred), startedAt timestamp, retained seed snapshot authority, and pending provider identity. Rehydrates from the journal when the in-memory transaction is gone (restart). - cancel(): only a durable PENDING preview may be cancelled. Recovers the journal entry with a preview-cancelled marker and drops the in-memory transaction so fences release and re-preview stays possible after restart (closes the Task 2 review gap where a dismissed preview left a durable in-progress entry that blocked future previews). An in-progress confirm (any journaled step past pending) is refused and the active status returned. - recover(): resolves through the same confirm transaction resolution (in-memory or journal), returns the committed summary idempotently for a committed entry, re-enters the shared confirm/resume machinery for an in-progress entry using the transaction's journaled fingerprint, and starts a fresh attempt for a rolled-back entry. Unknown transactions throw the typed FORK_TRANSACTION_UNKNOWN error. confirm() and recover() share the hook validation + mutation-lock preamble (enterLockedResume) and confirmLocked resume core. No SessionManager wiring, RPC registration, or first-Send changes: the new methods are public seams for the later phases. Refs #43
Spec-compliance review findings (task3b): - resolveConfirmTransaction's in-memory branch silently reset a failed (recovery-required) transaction via beginFreshAttempt and re-ran the whole confirm on top of possibly uncompensated artifacts, while the same durable entry after restart threw FORK_TRANSACTION_UNKNOWN. The in-memory branch now mirrors journal-only resolution: committed returns its summary, rolled-back starts a fresh attempt, and every other terminal state is not resumable — recover and confirm throw the typed error consistently before and after restart. - statusFor now prefers the durable journal metadata state for any unresolved entry, so a compensation failure surfaces as recovery-required instead of the stale in-memory step state. - Tests: compensation-failed fixture helper; status recovery-required; cancel refusal on failed entries (in-process + restart); recover/confirm typed error on failed entries (in-process + restart). Refs #43
Code-quality review finding (Important): cancel did not take the mutation lock, so a cancel landing in a confirm's pre-journal revalidation window marked the entry preview-cancelled while the in-flight confirm durably committed a child the journal no longer recorded — breaking the commit-marker-before-child-visibility invariant and enabling double-creation after restart. - cancel now runs its pending-only guard + write under the same common-directory mutation lock as confirm; a queued cancel therefore sees the confirm's first journal step and refuses. - confirmLocked re-checks its own durable journal entry (by journalId) after acquiring the lock and aborts with FORK_TRANSACTION_UNKNOWN if a cancel or superseding preview recovered it first. - Two deterministic FIFO race tests: cancel-first aborts the queued confirm with no child; confirm-first makes the queued cancel refuse and the entry stays committed. - statusFor validates journal state against the new CONVERSATION_FORK_RECOVERY_STATES tuple (single source of truth for the recovery-state union) so journal-internal markers can never leak onto the wire; stale comment fixed. Refs #43
… fork RPCs Task 3c/3d of Worktree V2 Phase 4 (#43): - SessionManager wires the full ConversationForkHooks surface (session facts with conversation head, strict capability via resolveIsolatedForkCapability, live adapter, activity, shared quiesce helper, create/delete child session). - createForkChildSession creates the durable pending child through the real createSession branch path: messages copied through the fork point, TARGET checkout bound (isolated = new record), pendingFork intent persisted on the stored record (strict parent identity, immutable transcriptCwd, executionCwd, idempotencyKey — no child provider ID), checkoutStrategy provenance ('shared' for ordinary branches, 'isolated' for fork children), and the backend preflight skipped entirely. - deleteForkChildSession compensates un-published children best-effort. - Fencing: fork transactions and pending children block Send/agent creation/Git mutations with the typed WORKTREE_FORK_PENDING code; session deletion cancels pending previews and blocks in-progress confirms (recovery-required stays deletable). - RPC: FORK_PREVIEW/CONFIRM/STATUS/RECOVER/CANCEL registered with WORKTREE_FORK_FAILED error mapping and V2 gating; headless parity test. - Tests: RPC routing/mapping/V2-gate, headless fork surface, SessionManager pending-child creation semantics + Send fence. i18n git.fork.pendingFence added in all seven locales. Refs #43
…d-creation tests
Spec-compliance review findings (task3cd):
- deleteSession blocked deletion for EVERY active fork state except
recovery-required, contradicting the pending-preview-cancellation
requirement. A pure pending preview is now cancelled (cancel serializes
with confirm under the mutation lock; if it refuses, the confirm is
genuinely in flight and deletion blocks with the typed pending code);
in-progress confirms still block; recovery-required stays deletable.
- The SessionManager child-creation tests live under src/sessions/, which
.gitignore excludes ('sessions/' matches Kata local data); force-add
isolated-fork-child.test.ts so the child creation + Send fence + three-way
deletion behavior ships with the change.
- New three-way deleteSession test: pending preview cancels and the session
deletes (journal preview-cancelled), in-flight confirm blocks with
WORKTREE_FORK_PENDING and the source survives.
Refs #43
Code-quality review finding (Important, task3cd): the internal pendingFork option is reachable from the untyped sessions:create RPC payload; the managed-session application block was not guarded by validatedBranch like the stored block, so a forged payload without a branch could fabricate a checkout/workingDirectory/sdkCwd/pendingFork plus a path lease on an arbitrary path. The managed block now requires validatedBranch, matching the stored block; production fork children always carry a valid branch, so behavior is unchanged. Also narrows the deleteSession test title to what it exercises. Refs #43
Failed/unverified first-Send provider establishments are appended to a durable append-only orphan ledger (next to the registry) so an unlinked provider child SDK session is never silently attached; Task 5 reconciles. IsolatedConversationForkService.markEstablished records the child provider identity as a metadata-only update on the committed fork journal entry (the child session record stays authoritative). The worktree journal now accepts metadata-only updates on committed entries. Refs #43
… adapter Replaces the pending-child Send fence with the real establish flow: the first Send persists the user message, then drives the strict provider adapter (deterministic seam; production adapters stay disabled) to create the native fork at the recorded source head with the PERSISTED idempotency key, persists the child provider ID exactly once, retires the pendingFork metadata, records the establishment in the fork journal, and only then dispatches the message. Missing/malformed anchors, absent/incomplete adapters, throwing establishes, and malformed results are typed WORKTREE_FORK_FAILED errors with no fallback; the child stays pending and retryable, the message is never duplicated, and failed or unverified attempts are recorded in the orphan ledger. getOrCreateAgent keeps the pending-child fence for every caller except the establish path. Refs #43
Code-quality review finding (Important, task4): two concurrent first-sends to a pending child both passed the prelude (isProcessing is set only after establishment) and both dispatched — the persisted idempotency key dedupes the provider artifact, but nothing prevented double dispatch. A per-session forkEstablishing set now refuses a second send during establishment with the typed pending code; the establishment body moved to a locked helper that also completes the defensive validation (idempotencyKey/executionCwd/ transcriptCwd emptiness -> typed anchor error, no fallback). Concurrency test: a gated establish adapter holds the first send while the second send is fired; asserts one establish call, one dispatch, pendingFork retired, child SDK id persisted. Refs #43
Startup reconciliation for interrupted isolated-conversation-fork transactions: committed entries stay committed (with the established marker backfilled from the durable child session when a crash between the child-session flush and markEstablished lost it); pre-child in-progress entries stay resumable; child-created in-progress entries without a live pending child are classified recovery-required; failed entries stay failed. The orphan ledger now retires entries with an append-only resolution marker when the same fork transaction later establishes, surfaces stale unresolved entries beyond the retention window, and never attaches an orphan to a session. Both reconciles run in the git.ts startup block before journal compact and the lifecycle-ready gate, fed by a new SessionManager resolveSessionForkState hook. Refs #43
Code-quality review finding (Critical, task5): compact() dropped committed fork entries, so a crash between a failed establish attempt and the later successful one left the orphan ledger permanently unresolvable (markEstablished finds no entry; isEstablished can never match) and deleted backfilled establishment markers in the same startup that wrote them. - compact() now retains committed op 'fork' entries (their post-commit establishment metadata and the orphan resolution that depends on it must survive restarts); failed entries stay; other committed/recovered entries are still dropped. - Regression test: commit -> compact -> updateMetadata(established) -> fresh instance still sees the marker. - Rename ForkJournalReconcileReport.recovered to flagged (it counts newly flagged recovery-required entries, not recoveries) and fix the startup log line; declare resolveSessionForkState on ISessionManager to drop the guarded cast in git.ts. Refs #43
…surfaces Worktree V2 Phase 4 isolated conversation forks — renderer surfaces: - Branch action opens a ForkDialog when Worktree V2 is effective (shared default, isolated only when eligible); V2 off keeps the immediate shared branch byte-identical to today. - ForkDialog drives FORK_PREVIEW/CONFIRM/STATUS/RECOVER/CANCEL through the new ElectronAPI channels, renders the sanitized source/destination/ capability/ignored-policy preview, disables isolated with the typed blocker reason when blocked, and navigates to the committed child. Shared-worktree confirm falls back to the existing onCreateSession branch flow (server-side shared confirmation is FORK_NOT_IMPLEMENTED). - Pending provider identity: the checkout badge shows PENDING for a published-but-not-established fork child (session DTO forkPending) and never claims a child provider ID before first Send. - Retryable establishment failure: the typed WORKTREE_FORK_FAILED error event surfaces a chat-input retry banner that re-sends the persisted message via existingMessageId (additive SendMessageOptions field). - ForkRecoveryBadge polls FORK_STATUS and opens the fork dialog in recovery mode, mirroring the handoff recovery affordance. - i18n: all new strings under git.fork.* in all 7 locales (parity + sorted). Refs #43
Spec-compliance review findings (task6-renderer):
- Blocker: the existingMessageId retry path in SessionManager.sendMessage
never fired onAck, so every renderer retry of a failed fork establishment
rejected with 'sendMessage completed without persisting a user message'
even on success — breaking the working-retry AC and never clearing the
pending banner. The retry path now acks the already-durable message id
exactly like the fresh path (RPC resolves { accepted, messageId } at
persistence time); a re-failed establish still routes through the typed
WORKTREE_FORK_FAILED post-persist error event. The retry idempotency test
now also asserts the ack contract.
- Minor: ForkDialog preview now renders the source session id + conversation
head turn id and the destination repository root (the DTO already carried
them), completing the mandated preview facts. Two new git.fork.* i18n keys
in all seven locales.
Refs #43
Deleting an isolated fork child (delete-with-worktree) is verified to use only the child's lifecycle: the child's own record is removed snapshot-first (session-delete journal entry committed), while the source session, source record, and source branch/HEAD/index stay untouched and the committed fork journal entry is retained. Deleting without the removal choice leaves the child's record unowned and manageable (auto-delete applies as for any record). Shared children keep dropping exactly one owner. Inspection is provenance-correct (child's own record, no shared-worktree language. rollbackFailedBranchCreation is structurally unreachable for pendingFork children (the preflight gate requires !options.pendingFork; the fork service compensates), verified and documented. No production behavior change. Refs #43 EOF )
…nt errors Install KATA_FORK_DETERMINISTIC_ADAPTER=1 (non-production only, mirroring the handoff seam) in SessionManager.getOrCreateAgent so the real Electron app can exercise preview/confirm and first-Send native-fork establishment without a live provider. Sweep the fork fence/establish error strings through i18n (git.fork.*, all 7 locales) following the handoff pattern; fork service blocker reasons stay literal exactly like handoff. Also add the missing trailing newline to isolated-fork-child.test.ts. Refs #43
Mirror the handoff spec structure: create a repo, start a session, send a message, then exercise the fork dialog (shared default; isolated eligible via the deterministic strict fork seam), preview rendering, name edit, confirm to a committed child bound to kata-agent/<name> at source HEAD, PENDING identity before first Send, first-Send establishment retires forkPending, and child deletion cleanup. The run tier needs a provider credential to create the agent (isolatedForkCapable only exists with a live agent), so it is deferred to credentialed UAT exactly like the handoff spec (issue #47 model). Refs #43
…tes, and OKF logs Record the Phase 4 fork engine decision (pending provider-fork intent with no child provider ID claim before first Send, strict cross-CWD native fork gate with no fallback, journaled fork transaction with compensation and an orphan ledger, checkout-strategy provenance for cleanup, first-Send establishment with a persisted idempotency key) as an ADR following the Phase 3 handoff ADR format. Update the docs roadmap, ADR index/log, architecture system overview + log, the online git-worktrees page (shared vs isolated, current-head restriction, pending identity, blockers), release notes, and the root/specs OKF logs. Refs #43
Final-review polish: the fork hooks, seed capture, confirm leases, and shared-worktree confirmation routing are all implemented on this branch; the comments now describe the actual wiring. Refs #43
16 tasks
📝 WalkthroughWalkthroughThis PR adds Worktree V2 isolated conversation forks. It defines fork protocols, provider capability checks, transactional Git operations, deferred first-send establishment, recovery and retry flows, Electron RPC wiring, renderer UI, persistence, tests, localization, and documentation. ChangesIsolated conversation fork workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
13 tasks
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d2bf84f38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (6)
apps/electron/src/renderer/components/app-shell/__tests__/fork-controls.test.ts (1)
379-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for two consecutive name edits.
Every
name-changedaction in this test starts from thepreviewphase. Fast typing sends a secondname-changedwhile the phase is alreadyloading, which the reducer currently ignores. A regression test locks the fix infork-controls.ts.💚 Proposed regression test
+ it('keeps the latest name while a debounced re-preview is still in flight', () => { + let state = reduceForkDialog(initialForkDialogState(), { + type: 'open', + strategy: 'isolated-worktree', + }) + state = reduceForkDialog(state, { type: 'preview-ready', preview: previewFor() }) + state = reduceForkDialog(state, { type: 'name-changed', value: 'a' }) + state = reduceForkDialog(state, { type: 'name-changed', value: 'ab' }) + expect(state.nameInput).toBe('ab') + expect(state.phase).toBe('loading') + })🤖 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 `@apps/electron/src/renderer/components/app-shell/__tests__/fork-controls.test.ts` around lines 379 - 401, Extend the fork dialog reducer coverage around the existing “keeps confirm disabled…” test to perform two consecutive name-changed actions, including the second while preview loading, and assert the latest name is retained and confirmation remains disabled until its matching preview arrives. Update the reducer logic in fork-controls.ts so name-changed actions are processed from the loading phase rather than ignored, while preserving the existing preview-matching confirmation behavior.packages/server-core/src/handlers/rpc/git.test.ts (1)
754-784: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the startup drain wait time-bounded instead of iteration-bounded.
The loop yields 25 macrotasks. The startup path awaits several promises whose count can grow, so a future change can exhaust the iterations before
lifecycle.markReadyruns and produce a flaky failure. Wait against a deadline instead.♻️ Deadline-based drain
- for (let i = 0; i < 25 && !startupCalls.includes('lifecycle.markReady'); i++) { - await new Promise((resolve) => setTimeout(resolve, 0)) - } + const deadline = Date.now() + 2000 + while (!startupCalls.includes('lifecycle.markReady') && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 1)) + }🤖 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/server-core/src/handlers/rpc/git.test.ts` around lines 754 - 784, Update the startup reconciliation drain in the test around makeHarness and lifecycle.markReady to wait until a time deadline rather than limiting the loop to 25 iterations. Repeatedly yield while markReady has not been recorded and the deadline has not expired, then retain the existing ordered startupCalls assertion.packages/server-core/src/sessions/isolated-fork-child.test.ts (1)
42-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an env-scoped config directory for this test.
beforeEachwritesws_testintojoin(CONFIG_DIR, 'config.json'), which the config module derives fromprocess.env.KATA_CONFIG_DIR || ~/.kata-agents. Set that env var to a temp config directory before importing the config module so the workspace registration cannot mutate or race with the developer’s real Kata config.🤖 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/server-core/src/sessions/isolated-fork-child.test.ts` around lines 42 - 87, The test currently modifies the developer’s real configuration because configFile uses the default directory. Update isolated-fork-child.test.ts to create a temporary config directory, assign it to KATA_CONFIG_DIR before importing the config module, and point the test’s configFile at that directory; restore or remove the environment variable during cleanup while preserving the existing workspace setup.packages/shared/src/agent/backend/deterministic-fork-adapter.ts (1)
19-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
EXECUTION_CWD_PROOF_CATEGORIESinstead of restating the category list.
types.tsalready exportsEXECUTION_CWD_PROOF_CATEGORIESwith the same four values, and derivesExecutionCwdProofCategoryfrom it. The localPROOF_CATEGORIEScopy can drift. If a fifth proof category is added to the canonical list, this fixture keeps proving only four, and the tests that assert full-proof coverage keep passing while the real first-Send gate expects the new category.Import the canonical tuple and type so the fixture tracks the contract automatically.
♻️ Proposed refactor
import type { ConversationForkEstablishInput, ConversationForkEstablishResult, + ExecutionCwdProofCategory, ExecutionCwdProof, StrictConversationForkCapability, } from './types' - -const PROOF_CATEGORIES = ['file', 'shell', 'mcp', 'provider'] as const +import { EXECUTION_CWD_PROOF_CATEGORIES } from './types' export interface DeterministicStrictForkAdapterOptions { /** Adapter identity stamped into capabilities and proofs. */ adapterId?: string /** establishNativeFork throws when true (native anchor missing/malformed). */ failEstablish?: boolean - /** Proof categories to omit; the strict fork requires all four. */ - missingChecks?: (typeof PROOF_CATEGORIES)[number][] + /** Proof categories to omit; the strict fork requires every category. */ + missingChecks?: ExecutionCwdProofCategory[]Then update the filter to use the shared tuple:
- const checks = PROOF_CATEGORIES.filter((category) => !missing.has(category)).map((category) => { + const checks = EXECUTION_CWD_PROOF_CATEGORIES.filter((category) => !missing.has(category)).map((category) => {🤖 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/shared/src/agent/backend/deterministic-fork-adapter.ts` around lines 19 - 27, Replace the local PROOF_CATEGORIES declaration in DeterministicStrictForkAdapterOptions with the canonical EXECUTION_CWD_PROOF_CATEGORIES tuple and its derived ExecutionCwdProofCategory type imported from types.ts. Update missingChecks and any proof-coverage iteration to use the shared symbols so the fixture automatically tracks all required proof categories.packages/server-core/src/git/worktree-journal.ts (1)
233-249: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRetire established fork entries, and read the file once.
Committed
forkentries are now kept forever. After the fork is durably established and the orphan ledger resolved it, the entry has no reader left, but it stays in the journal and everyentries()scan parses it. Add a retirement condition so compaction drops a committed fork entry oncemetadata.state === 'established'.
compactalso callsreadAll()twice inside the lock, which parses the whole file two times.♻️ Proposed refactor
compact(): void { this.lock.runSync(() => { - const entries = this.readAll().filter( - (entry) => entry.status === 'failed' || (entry.status === 'committed' && entry.op === 'fork'), - ) - if (entries.length === this.readAll().length) return + const all = this.readAll() + const entries = all.filter( + (entry) => + entry.status === 'failed' || + (entry.status === 'committed' && + entry.op === 'fork' && + entry.metadata?.state !== 'established'), + ) + if (entries.length === all.length) return this.writeAll(entries) }) }If the orphan ledger must still match established fork entries after retirement, keep the entry and instead move retirement into the ledger reconciliation path.
🤖 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/server-core/src/git/worktree-journal.ts` around lines 233 - 249, Update compact() to read the journal once, retain failed entries and committed fork entries only while metadata.state is not "established", and write the filtered result only when its length differs from the original entries array. Ensure ledger reconciliation still has access to established fork entries; if it requires them after compaction, perform retirement only after that reconciliation completes.packages/server-core/src/git/fork-orphan-ledger.ts (1)
162-198: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePlan a size bound for the ledger file.
The ledger keeps every attempt line and every resolution marker forever, and
entries()parses the whole file on each call and on eachreconcile. On a long-lived install the read cost grows without limit. Add a rotation or archive step that keeps resolved attempt/marker pairs outside the hot file, or cap the retained history after the retention window is reported.Concurrent reconciles in two processes can also append duplicate markers for one
attemptId, because the markers are computed before the lock is taken.entries()dedupes through a Set, so the result stays correct; only the file grows faster.🤖 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/server-core/src/git/fork-orphan-ledger.ts` around lines 162 - 198, Add bounded ledger maintenance around ForkOrphanLedger.reconcile and entries: archive or rotate resolved attempt/marker pairs, or prune history after the retention window is reported, while preserving unresolved retained entries and reconciliation results. Perform marker existence checks and append/rotation decisions inside the existing lock so concurrent processes cannot duplicate resolution markers for the same attemptId; ensure entries() continues reading the active ledger correctly after maintenance.
🤖 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.
Inline comments:
In `@apps/electron/src/renderer/components/app-shell/fork/ForkAction.tsx`:
- Around line 31-49: Guard the result of
window.electronAPI?.getGitCapabilities?.() before chaining .then() in the
React.useEffect. When the capability method is unavailable, set v2Effective to
false and clear v2Pending; preserve the existing cancellation handling and
promise success/error behavior when the call returns a promise.
In `@apps/electron/src/renderer/components/app-shell/input/fork-controls.ts`:
- Around line 255-262: Update the name-changed handler in the reducer so it also
accepts actions when state.phase is loading, preserving the latest action.value
in nameInput while keeping phase loading; retain the existing preview and
preview-blocked behavior.
In `@e2e/tests/git/worktree-v2-fork.spec.ts`:
- Around line 70-98: Update readForkChild so session.checkout.checkoutPath and
session.checkout.expectedBranch are constructed in Node scope before
page.evaluate; pass the resulting checkout data into the page callback or merge
it afterward, leaving only the electronAPI session lookup inside evaluate and
preserving the returned ForkChildSession shape.
In `@packages/server-core/src/git/__tests__/isolated-conversation-fork.test.ts`:
- Around line 522-533: Update dirtySource to await the git add operation for
staged.txt before performing the subsequent rev-parse calls and returning.
Remove the discarded-promise usage so staging completes deterministically and
failures propagate to dependent tests.
In `@packages/server-core/src/git/isolated-conversation-fork-service.ts`:
- Around line 1784-1806: Introduce a shared helper that resolves the
authoritative journal entry for a transactionId by searching matching fork
entries newest-first and preferring committed over in-progress entries. Use this
helper at
packages/server-core/src/git/isolated-conversation-fork-service.ts:1784-1806 so
repeated confirm/recover returns the committed summary instead of starting
another attempt, and at
packages/server-core/src/git/isolated-conversation-fork-service.ts:423-438 so
establishment markers target the committed entry; update both call sites to
preserve their existing downstream handling.
- Around line 423-438: Update markEstablished to select the newest matching fork
journal entry for transactionId rather than the first entries() match, so
repeated attempts resolve the latest committed entry. Preserve the existing
committed-status guard and metadata update behavior after selecting that newest
entry.
In `@packages/server-core/src/handlers/rpc/git.ts`:
- Around line 388-429: Guard the startup reconciliation calls around
reconcileForkJournal and forkOrphans.reconcile so omitted optional Git services
do not throw. Use optional access with empty/default reconciliation reports when
git.fork or git.forkOrphans is unavailable, then always continue to
git.journal.compact() and git.lifecycle.markReady().
In `@packages/server-core/src/sessions/SessionManager.ts`:
- Around line 5689-5709: Move the forkEstablishing in-flight check from
establishPendingFork to the Send-path fences before user-message persistence,
alongside the existing send guards. Reject concurrent first-sends with
WORKTREE_FORK_PENDING_CODE before writing, flushing, or acknowledging the
message, while retaining establishPendingFork’s establishment and cleanup
behavior.
- Around line 3010-3027: Validate the parent anchor before creating or
committing an isolated fork child: require non-empty parentSdkSessionId and
parentSdkTurnId derived from conversationHead?.turnId and the parent session
data. If either anchor is missing, return the existing confirmation-blocker
response with reason `missing-parent-anchor` and do not call
createForkChildSession or persist the child. Keep valid fork confirmation
behavior unchanged.
In
`@packages/shared/src/agent/backend/__tests__/conversation-fork-capability.test.ts`:
- Around line 126-143: Remove the inert backend and sdkCwdWrites assertion from
the direct RecordingForkAdapter test, and rename the test to describe only input
pass-through, child session ID, and proof destination verification. Simplify
makeBackend or its sdkCwdWrites spy if it is unused by the remaining gate tests;
otherwise move the transcript-identity assertion into the fork-service path that
invokes updateSdkCwd.
In `@packages/shared/src/i18n/locales/de.json`:
- Line 552: Update the German git.fork.* translations to use correct German
grammar, including the dative ownership construction in the message containing
“Server {{serverId}} gehört”. Standardize all git.fork.* values at the affected
entries to one form of address, preferably the informal “du” form, replacing
formal “Sie” wording consistently.
In `@packages/shared/src/i18n/locales/hu.json`:
- Line 552: Update all git.fork.* translations in the Hungarian locale to use
consistent informal address, replacing formal phrasing in git.fork.* keys such
as the restore and view messages. Correct the sourceSession translation to use
“munkamenet” instead of “szakasz”, while preserving the existing flat keys and
required ellipses.
In `@packages/shared/src/i18n/locales/ja.json`:
- Around line 537-602: Standardize terminology throughout the git.fork
translations: use one Japanese term consistently for “isolated” across keys such
as anchorMissing, cwdInvalid, committedDetail, strategy.isolated, and
strategy.isolatedNote, and use one consistent rendering for “Fork” across the
entire namespace. Preserve the existing ellipses, English brand names,
placeholders, and concise wording.
In `@packages/shared/src/i18n/locales/zh-Hans.json`:
- Around line 537-602: Update the git.fork locale values so the conversation
fork concept consistently uses 派生 (or 分叉), including titles, actions, status,
errors, and descriptions currently translated as 分支. Keep 分支 exclusively for Git
branch references such as destination branch, committed branch, and branch
names, while preserving placeholders and unrelated Git terminology.
---
Nitpick comments:
In
`@apps/electron/src/renderer/components/app-shell/__tests__/fork-controls.test.ts`:
- Around line 379-401: Extend the fork dialog reducer coverage around the
existing “keeps confirm disabled…” test to perform two consecutive name-changed
actions, including the second while preview loading, and assert the latest name
is retained and confirmation remains disabled until its matching preview
arrives. Update the reducer logic in fork-controls.ts so name-changed actions
are processed from the loading phase rather than ignored, while preserving the
existing preview-matching confirmation behavior.
In `@packages/server-core/src/git/fork-orphan-ledger.ts`:
- Around line 162-198: Add bounded ledger maintenance around
ForkOrphanLedger.reconcile and entries: archive or rotate resolved
attempt/marker pairs, or prune history after the retention window is reported,
while preserving unresolved retained entries and reconciliation results. Perform
marker existence checks and append/rotation decisions inside the existing lock
so concurrent processes cannot duplicate resolution markers for the same
attemptId; ensure entries() continues reading the active ledger correctly after
maintenance.
In `@packages/server-core/src/git/worktree-journal.ts`:
- Around line 233-249: Update compact() to read the journal once, retain failed
entries and committed fork entries only while metadata.state is not
"established", and write the filtered result only when its length differs from
the original entries array. Ensure ledger reconciliation still has access to
established fork entries; if it requires them after compaction, perform
retirement only after that reconciliation completes.
In `@packages/server-core/src/handlers/rpc/git.test.ts`:
- Around line 754-784: Update the startup reconciliation drain in the test
around makeHarness and lifecycle.markReady to wait until a time deadline rather
than limiting the loop to 25 iterations. Repeatedly yield while markReady has
not been recorded and the deadline has not expired, then retain the existing
ordered startupCalls assertion.
In `@packages/server-core/src/sessions/isolated-fork-child.test.ts`:
- Around line 42-87: The test currently modifies the developer’s real
configuration because configFile uses the default directory. Update
isolated-fork-child.test.ts to create a temporary config directory, assign it to
KATA_CONFIG_DIR before importing the config module, and point the test’s
configFile at that directory; restore or remove the environment variable during
cleanup while preserving the existing workspace setup.
In `@packages/shared/src/agent/backend/deterministic-fork-adapter.ts`:
- Around line 19-27: Replace the local PROOF_CATEGORIES declaration in
DeterministicStrictForkAdapterOptions with the canonical
EXECUTION_CWD_PROOF_CATEGORIES tuple and its derived ExecutionCwdProofCategory
type imported from types.ts. Update missingChecks and any proof-coverage
iteration to use the shared symbols so the fixture automatically tracks all
required proof categories.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 913c8640-ee3b-4cc1-8604-8cac18fb666f
⛔ Files ignored due to path filters (9)
apps/electron/resources/release-notes/next.mdis excluded by!**/*.mddocs/adrs/2026-08-08-isolated-conversation-forks.mdis excluded by!**/*.mddocs/adrs/index.mdis excluded by!**/*.mddocs/adrs/log.mdis excluded by!**/*.mddocs/architecture/log.mdis excluded by!**/*.mddocs/architecture/system-overview.mdis excluded by!**/*.mddocs/index.mdis excluded by!**/*.mddocs/log.mdis excluded by!**/*.mddocs/specs/log.mdis excluded by!**/*.md
📒 Files selected for processing (55)
apps/electron/src/renderer/App.tsxapps/electron/src/renderer/atoms/sessions.tsapps/electron/src/renderer/components/app-shell/ChatDisplay.tsxapps/electron/src/renderer/components/app-shell/__tests__/fork-controls.test.tsapps/electron/src/renderer/components/app-shell/__tests__/worktree-removal.test.tsapps/electron/src/renderer/components/app-shell/fork/ForkAction.tsxapps/electron/src/renderer/components/app-shell/fork/ForkDialog.tsxapps/electron/src/renderer/components/app-shell/input/WorkspaceCheckoutBadge.tsxapps/electron/src/renderer/components/app-shell/input/__tests__/checkout-controls.test.tsapps/electron/src/renderer/components/app-shell/input/checkout-controls.tsapps/electron/src/renderer/components/app-shell/input/fork-controls.tsapps/electron/src/renderer/components/right-sidebar/git-changes/ChangesPanel.tsxapps/electron/src/renderer/context/AppShellContext.tsxapps/electron/src/renderer/playground/PlaygroundAppShellProvider.tsxapps/electron/src/shared/__tests__/ipc-channels.test.tsapps/electron/src/shared/types.tsapps/electron/src/transport/channel-map.tsapps/online-docs/core-concepts/git-worktrees.mdxe2e/tests/git/worktree-v2-fork.spec.tspackages/server-core/src/git/__tests__/fork-orphan-ledger.test.tspackages/server-core/src/git/__tests__/isolated-conversation-fork.test.tspackages/server-core/src/git/__tests__/worktree-journal.test.tspackages/server-core/src/git/fork-orphan-ledger.tspackages/server-core/src/git/index.tspackages/server-core/src/git/isolated-conversation-fork-service.tspackages/server-core/src/git/worktree-journal.tspackages/server-core/src/git/worktree-lifecycle-service.tspackages/server-core/src/handlers/rpc/git.test.tspackages/server-core/src/handlers/rpc/git.tspackages/server-core/src/handlers/rpc/headless-server-flow.test.tspackages/server-core/src/handlers/rpc/sessions.tspackages/server-core/src/handlers/session-manager-interface.tspackages/server-core/src/sessions/SessionManager.tspackages/server-core/src/sessions/isolated-fork-child.test.tspackages/shared/src/agent/backend/__tests__/conversation-fork-capability.test.tspackages/shared/src/agent/backend/__tests__/deterministic-fork-adapter.test.tspackages/shared/src/agent/backend/conversation-fork-capability.tspackages/shared/src/agent/backend/deterministic-fork-adapter.tspackages/shared/src/agent/backend/index.tspackages/shared/src/agent/backend/types.tspackages/shared/src/i18n/locales/de.jsonpackages/shared/src/i18n/locales/en.jsonpackages/shared/src/i18n/locales/es.jsonpackages/shared/src/i18n/locales/hu.jsonpackages/shared/src/i18n/locales/ja.jsonpackages/shared/src/i18n/locales/pl.jsonpackages/shared/src/i18n/locales/zh-Hans.jsonpackages/shared/src/protocol/__tests__/conversation-fork-contracts.test.tspackages/shared/src/protocol/channels.tspackages/shared/src/protocol/conversation-fork.tspackages/shared/src/protocol/dto.tspackages/shared/src/protocol/index.tspackages/shared/src/protocol/routing.tspackages/shared/src/protocol/types.tspackages/shared/src/sessions/types.ts
…ck chain The environment is credentialed (codex OAuth harness + KATA_*_API_KEY chain in .env); deferring UAT and wiring deterministic provider seams was wrong. This removes the fakes and makes the agent E2E tier run real providers: - Remove the KATA_FORK_DETERMINISTIC_ADAPTER and KATA_HANDOFF_DETERMINISTIC_ ADAPTER env seams from SessionManager (production foot guns). - Relocate the deterministic adapters out of the production export surface into the test-only @kata-sh/shared/agent/testing subpath (new package export); update all test importers; production code no longer references them. - env.ts: parse the ordered provider fallback chain — primary KATA_E2E_AGENT_PROVIDER/MODEL plus numbered _02.._NN fallbacks (openai-codex OAuth-preferred with key fallback; opencode-go/openrouter/deepseek/anthropic api-key). Prerequisite passes when any candidate is ready; unready candidates are skipped with logged reasons. - onboarding.ts: configureAgentConnection re-runnable per candidate (OAuth path with key fallback; programmatic setupLlmConnection + setDefault for api-key candidates); remove the one-shot wizard path it supersedes. - agentChat.ts: runWithAgentProviderFallback walks the chain, logs every attempt, and throws an aggregated AgentProviderChainExhaustedError naming each option and failure; agentSuiteTimeoutMs budgets one agent window per candidate. - reply/fork/handoff specs: real provider prologue via the fallback chain; fork asserts shared-default works and isolated is typed-blocked (unsupported-provider) without a production adapter; handoff asserts the control is absent for unsupported providers. - AGENTS.md: Credentials and E2E provider UAT section (never assume non-credentialed; never defer UAT or wire fake seams without asking). - #49 (deferred UAT) closed as superseded. Refs #43
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
e2e/tests/git/worktree-v2-handoff.spec.ts (1)
113-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnchor the absence assertion with a positive check.
The test asserts only that
handoff-open-buttonhas count 0. It also passes when the Changes panel renders no controls at all, for example after an unrelated regression. Assert one control that the panel must always render, then assert the absence of the handoff button.🤖 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 `@e2e/tests/git/worktree-v2-handoff.spec.ts` around lines 113 - 115, Update the test around the git-changes-panel in the handoff flow to first assert a known control that the panel must always render, then retain the toHaveCount(0) assertion for handoff-open-button. Use an existing stable control test ID from the panel rather than adding or relying on an unrelated element.e2e/tests/git/worktree-v2-fork.spec.ts (1)
168-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the blocker by test id, not by the English copy.
The expectation embeds the literal string "This provider can't establish an isolated fork yet." User-facing strings go through
t()and exist in seven locale files. A copy edit or a locale default change then breaks this test for a reason unrelated to fork behavior. Assert a stabledata-testidon the blocker element, or compare against the imported locale key value.As per coding guidelines: "Route all user-facing strings through
t()ori18n.t(); add keys to all seven locale files".🤖 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 `@e2e/tests/git/worktree-v2-fork.spec.ts` at line 168, Update the expectation around the isolated-fork blocker to assert its stable data-testid instead of the hard-coded English message. Locate the blocker element rendered by the fork flow and use its existing test id, preserving the assertion that the blocker is shown without depending on localized copy.Source: Coding guidelines
packages/shared/package.json (1)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a guard that blocks production imports of
./agent/testing.The export map makes the test-only adapters importable from any package, including production code. The JSDoc header in
packages/shared/src/agent/backend/testing.tsis the only protection today. Add an ESLintno-restricted-importsrule (or a CI grep) that allows@kata-sh/shared/agent/testingonly in**/__tests__/**,**/*.test.ts, ande2e/**. This keeps the deterministic adapters test-only after future refactors.As per coding guidelines: "Use deterministic adapters from
@kata-shared/shared/agent/testingonly as test doubles; never import them into production code".#!/bin/bash # Find any non-test import of the testing subpath. rg -n --glob '!**/__tests__/**' --glob '!**/*.test.ts' --glob '!e2e/**' \ "`@kata-sh/shared/agent/testing`" .🤖 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/shared/package.json` at line 20, Add a repository lint or CI guard for the `@kata-sh/shared/agent/testing` import path, restricting it to **/__tests__/**, **/*.test.ts, and e2e/** while rejecting imports from production code. Configure no-restricted-imports or an equivalent grep check, and ensure the existing test-only export remains usable in the allowed locations.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.
Inline comments:
In `@e2e/src/flows/onboarding.ts`:
- Around line 131-162: Update the onboarding setup flow around
setupLlmConnection so candidate.apiKey is not passed as a page.evaluate argument
while tracing is enabled. Redact the credential before evaluate or inject it
through the authenticated page context, while preserving the existing
targetSlug, defaultModel, provider, and setup behavior.
- Around line 192-213: Update the authenticated OAuth branch in the onboarding
flow around completeConfiguredChatGptOnboarding so it handles an already-ready
app when `#app-ready` is visible instead of waiting exclusively for
`#onboarding-wizard`. Skip waitForOnboardingWizard or transition to the ready
shell before setupLlmConnection for ChatGPT/Codex, while preserving the existing
OAuth configuration and setDefaultConnection behavior.
In `@e2e/src/harness/env.ts`:
- Line 101: Update the API-key resolution around firstNonEmpty to read only the
canonical KATA_* environment variable, removing the un-prefixed fallback; ensure
keySource reports the actual resolved canonical variable name so logs and
exhausted-chain errors identify it correctly.
---
Nitpick comments:
In `@e2e/tests/git/worktree-v2-fork.spec.ts`:
- Line 168: Update the expectation around the isolated-fork blocker to assert
its stable data-testid instead of the hard-coded English message. Locate the
blocker element rendered by the fork flow and use its existing test id,
preserving the assertion that the blocker is shown without depending on
localized copy.
In `@e2e/tests/git/worktree-v2-handoff.spec.ts`:
- Around line 113-115: Update the test around the git-changes-panel in the
handoff flow to first assert a known control that the panel must always render,
then retain the toHaveCount(0) assertion for handoff-open-button. Use an
existing stable control test ID from the panel rather than adding or relying on
an unrelated element.
In `@packages/shared/package.json`:
- Line 20: Add a repository lint or CI guard for the
`@kata-sh/shared/agent/testing` import path, restricting it to **/__tests__/**,
**/*.test.ts, and e2e/** while rejecting imports from production code. Configure
no-restricted-imports or an equivalent grep check, and ensure the existing
test-only export remains usable in the allowed locations.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 80c980d6-7a72-4f53-9106-18d96debdafc
⛔ Files ignored due to path filters (1)
AGENTS.mdis excluded by!**/*.md
📒 Files selected for processing (16)
e2e/src/flows/agentChat.tse2e/src/flows/onboarding.tse2e/src/harness/env.tse2e/tests/agent/reply.spec.tse2e/tests/git/worktree-v2-fork.spec.tse2e/tests/git/worktree-v2-handoff.spec.tspackages/server-core/src/git/__tests__/handoff-runtime-gate.test.tspackages/server-core/src/git/__tests__/isolated-conversation-fork.test.tspackages/server-core/src/git/__tests__/worktree-handoff.test.tspackages/server-core/src/sessions/SessionManager.tspackages/server-core/src/sessions/isolated-fork-child.test.tspackages/shared/package.jsonpackages/shared/src/agent/backend/__tests__/deterministic-fork-adapter.test.tspackages/shared/src/agent/backend/__tests__/deterministic-handoff-adapter.test.tspackages/shared/src/agent/backend/index.tspackages/shared/src/agent/backend/testing.ts
💤 Files with no reviewable changes (2)
- packages/shared/src/agent/backend/index.ts
- packages/server-core/src/sessions/SessionManager.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shared/src/agent/backend/tests/deterministic-fork-adapter.test.ts
- packages/server-core/src/sessions/isolated-fork-child.test.ts
- packages/server-core/src/git/tests/isolated-conversation-fork.test.ts
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Fence unresolved journal transactions after restart, preserve source backend identity, validate execution proofs and anchors, recover durable retry state, and make first-send and startup reconciliation fail closed. Tighten E2E credential handling, locale wording, and regression coverage.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/shared/src/protocol/dto.ts (1)
254-261: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftValidate
existingMessageIdand wire up the retry path.
packages/server-core/src/sessionsis missing, and the onlysendMessageimplementation found only passesexistingMessageIdtoSessionManager.sendMessage(...)without using it. Add the shared runtime path and make it verify thatexistingMessageIdbelongs to the requested session, points to the expected persisted user message, and retains the required fork idempotency state before reusing it.🤖 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/shared/src/protocol/dto.ts` around lines 254 - 261, Wire existingMessageId through the shared sendMessage runtime path and implement validation before reuse. In SessionManager.sendMessage, verify the ID belongs to the requested session, references the expected persisted user message, and has the required fork idempotency state; reject invalid or mismatched IDs, otherwise reuse the existing message instead of creating a new one.apps/electron/src/renderer/components/app-shell/ChatDisplay.tsx (1)
603-623: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLocalize the shared-branch name.
Branch ofandUntitledbecome visible session names. Route both strings throught()and add the required locale entries. Addtto the callback dependency list.As per coding guidelines, “Route all user-facing strings through
t()ori18n.t().”🤖 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 `@apps/electron/src/renderer/components/app-shell/ChatDisplay.tsx` around lines 603 - 623, The createSharedBranch callback currently hardcodes user-facing “Branch of” and “Untitled” text; replace both with translations via t(), add the corresponding locale entries, and include t in the callback dependency list while preserving the existing branch naming 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.
Inline comments:
In `@apps/electron/src/renderer/atoms/sessions.ts`:
- Around line 733-736: Update the retry-target selection around the session
message lookup to find the first post-branch user message without filtering by
string content; if that message cannot populate ForkRetryState, return null
rather than selecting a later message. Preserve the exact first user payload,
and add a regression test covering structured first content followed by a later
text user message.
In `@apps/electron/src/renderer/components/app-shell/ChatDisplay.tsx`:
- Around line 566-590: Update the retry hydration tracking in the effect around
forkRetryHydratedSessionRef to use a Set<string> of hydrated session IDs rather
than a single session ID. Check the set before processing each session, and only
add session.id after the full snapshot has loaded; preserve the existing
forkPending and retry-message checks so dismissed banners are not rehydrated
when revisiting a session.
In `@packages/server-core/src/sessions/isolated-fork-child.test.ts`:
- Around line 303-304: In both affected sites in
packages/server-core/src/sessions/isolated-fork-child.test.ts (lines 303-304 and
642-643), replace the early return after confirmChild with an assertion that
childId is defined before continuing to load the stored child or test
execution-proof rejection. Keep the subsequent test logic unchanged so missing
child creation fails the test.
---
Outside diff comments:
In `@apps/electron/src/renderer/components/app-shell/ChatDisplay.tsx`:
- Around line 603-623: The createSharedBranch callback currently hardcodes
user-facing “Branch of” and “Untitled” text; replace both with translations via
t(), add the corresponding locale entries, and include t in the callback
dependency list while preserving the existing branch naming behavior.
In `@packages/shared/src/protocol/dto.ts`:
- Around line 254-261: Wire existingMessageId through the shared sendMessage
runtime path and implement validation before reuse. In
SessionManager.sendMessage, verify the ID belongs to the requested session,
references the expected persisted user message, and has the required fork
idempotency state; reject invalid or mismatched IDs, otherwise reuse the
existing message instead of creating a new one.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d3fbb2ed-f8bb-4f0a-8128-597a85cac13a
⛔ Files ignored due to path filters (5)
docs/adrs/2026-08-08-isolated-conversation-forks.mdis excluded by!**/*.mddocs/adrs/log.mdis excluded by!**/*.mddocs/architecture/log.mdis excluded by!**/*.mddocs/log.mdis excluded by!**/*.mddocs/specs/log.mdis excluded by!**/*.md
📒 Files selected for processing (24)
apps/electron/src/renderer/App.tsxapps/electron/src/renderer/atoms/__tests__/sessions.test.tsapps/electron/src/renderer/atoms/sessions.tsapps/electron/src/renderer/components/app-shell/ChatDisplay.tsxapps/electron/src/renderer/components/app-shell/__tests__/fork-controls.test.tsapps/electron/src/renderer/components/app-shell/fork/ForkAction.tsxapps/electron/src/renderer/components/app-shell/input/fork-controls.tse2e/src/flows/onboarding.tse2e/src/flows/shell.tse2e/src/harness/env.tspackages/server-core/src/git/__tests__/isolated-conversation-fork.test.tspackages/server-core/src/git/isolated-conversation-fork-service.tspackages/server-core/src/handlers/rpc/git.test.tspackages/server-core/src/handlers/rpc/git.tspackages/server-core/src/sessions/SessionManager.tspackages/server-core/src/sessions/isolated-fork-child.test.tspackages/shared/src/agent/backend/__tests__/conversation-fork-capability.test.tspackages/shared/src/i18n/locales/de.jsonpackages/shared/src/i18n/locales/hu.jsonpackages/shared/src/i18n/locales/ja.jsonpackages/shared/src/i18n/locales/zh-Hans.jsonpackages/shared/src/protocol/__tests__/conversation-fork-contracts.test.tspackages/shared/src/protocol/conversation-fork.tspackages/shared/src/protocol/dto.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/shared/src/i18n/locales/hu.json
- packages/shared/src/i18n/locales/zh-Hans.json
- packages/shared/src/i18n/locales/ja.json
- apps/electron/src/renderer/App.tsx
- packages/shared/src/agent/backend/tests/conversation-fork-capability.test.ts
- packages/shared/src/i18n/locales/de.json
- apps/electron/src/renderer/components/app-shell/tests/fork-controls.test.ts
- apps/electron/src/renderer/components/app-shell/input/fork-controls.ts
- packages/server-core/src/handlers/rpc/git.ts
- packages/server-core/src/handlers/rpc/git.test.ts
- packages/shared/src/protocol/conversation-fork.ts
- packages/server-core/src/git/isolated-conversation-fork-service.ts
- apps/electron/src/renderer/components/app-shell/fork/ForkAction.tsx
- packages/server-core/src/git/tests/isolated-conversation-fork.test.ts
- packages/server-core/src/sessions/SessionManager.ts
gannonh
deleted the
feat/worktree-v2-phase-4-isolated-conversation-forks
branch
August 8, 2026 20:41
This was referenced Aug 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #43
Scope
Worktree V2 Phase 4: isolated conversation forks — an explicit current-head alternative to shared conversation branches. A fork child gets its own named managed worktree (
kata-agent/<name>at the source checkout's exact captured HEAD), its own Kata session with a durable pending provider-fork intent, its own execution runtime, and a strict first-Send native-fork establishment — with source immutability, typed blockers, journaled compensation, and an orphan ledger throughout.The credential-requiring UAT tiers (E2E UI UAT run + credentialed provider UAT) are deferred to #49 (mirroring the Phase 3 handoff deferral #47). Production provider adapters stay disabled —
KATA_FORK_DETERMINISTIC_ADAPTER=1is the non-production credential-free seam.Tasks (each with spec-compliance + code-quality review gates)
shared-worktreedefault /isolated-worktree), 14-code blocker tuple, preview/confirm/status/recover/cancel DTOs, pending intent that structurally cannot carry a child provider ID, error codes, channels; strict fork capability gate + deterministic adapter.kata-agent/<name>at captured HEAD, restore/verify, child session via SessionManager hooks, sole-owner commit through the fork journal, CAS compensation, idempotent replay (incl. crash-after-materialization resume).@worktree-v2 fork, lists cleanly), ADR, docs, release notes, OKF logs; deferred-UAT issue Deferred: Worktree V2 Phase 4 fork UI UAT and credentialed provider UAT #49.Verification
packages/shared:bun test src/git src/sessions86 pass;tsc --noEmitclean; full suite 3072 passpackages/server-core: focused issue suite (fork/snapshot/lifecycle/managed/mutation-lock/reconcile/git RPC/headless) 230 pass;src/git src/handlers/rpc src/sessions567 pass; full server-core 645 pass;typecheckcleanapps/electron: app-shell + right-sidebar 238 pass;typecheckcleanlint:i18n:parity+lint:i18n:sortedpass (1819 keys × 7 locales)bun run e2e --list --grep '@worktree-v2.*fork'→ 1 spec listedKnown deferred items
sessions:create(the existing branch flow);FORK_CONFIRMwithshared-worktreethrows typedFORK_NOT_IMPLEMENTEDby design.Build completion report: posted on #43.
Summary by CodeRabbit
Greptile Summary
The isolated conversation-fork flow rejects incomplete, mismatched, and stale provider-execution proofs before attaching a child provider session or sending a child turn. The previously reported destination-validation and stale-timestamp concerns were exercised through
SessionManager.sendMessage: invalid proofs leave the fork pending, do not persist the child provider identity, and do not dispatch chat.Confidence Score: 5/5
Safe to merge based on the exercised provider-fork establishment paths.
No blocking failure remains.
What T-Rex did
Reviews (5): Last reviewed commit: "fix(worktree): reject stale fork executi..." | Re-trigger Greptile