feat(worktree): add named managed worktree V2 - #44
Merged
gannonh merged 28 commits intoAug 5, 2026
Merged
Conversation
added 19 commits
August 4, 2026 18:58
Preserve V1 intent and persistence shapes while carrying V2 identity metadata through summaries and session storage.\n\nRefs #40
Implement exact V2 branch names, safe display fragments, V2 checkout metadata, and compare-and-swap compensation while preserving V1 preparation and routing.\n\nRefs #40
Expose server-capability-gated named worktree creation in the composer and add the Worktrees settings page for local and connected remote servers. Persist all user-facing copy across locales and retain V1 intent behavior when V2 is unavailable.\n\nRefs #40
Add a real headless named-worktree/settings flow and an offline Electron UAT covering custom names, server-owned root changes, restart recovery, and status routing.\n\nRefs #40
Document exact named branches, per-server root settings, fixed registry authority, and failure behavior across the architecture, ADR, online Git guide, and OKF logs.\n\nRefs #40
Restart the Electron process through the E2E harness while retaining the isolated Vite server and complete deferred setup again before checking persisted worktrees.\n\nRefs #40
Compare the new branch against the captured base OID before marking it request-owned, and verify the checkout HEAD matches that branch. Retain externally changed refs during cleanup with a regression covering the first ownership read.\n\nRefs #40
Exercise exact and case-colliding named branches without creating registry or filesystem residue.\n\nRefs #40
Refresh the roadmap and OKF logs now that the Electron artifact is built and the local Worktree V2 UAT passes.\n\nRefs #40
Convert human-readable V2 names to lowercase kebab-case in the composer while preserving nested refs and server-side Git validation. Add renderer and real-Electron coverage plus localized guidance.\n\nRefs #40
Record the human-readable named worktree normalization behavior in the OKF logs.\n\nRefs #40
📝 WalkthroughWalkthroughWorktree V2 adds versioned checkout contracts, named managed worktrees, server-owned materialization-root settings, cross-process registry coordination, capability-aware RPCs, Electron controls, settings UI, localization, documentation, and end-to-end validation while retaining V1 fallback behavior. ChangesWorktree V2 contracts and persistence
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![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: c6b77fccf4
ℹ️ 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".
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/electron/src/renderer/components/app-shell/input/WorkspaceCheckoutBadge.tsx (1)
278-288: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd
worktreeV2Enabledto thehandleSelectModedependency list.
handleSelectModereadsworktreeV2Enabledbut its dependency list is[intentKind, refs.length, worktrees.length, loadRefs, loadWorktrees].worktreeV2Enabledderives fromserverV2Available, which the capability effect sets asynchronously after the first render. The memoized callback therefore keepsworktreeV2Enabled === falsefrom the first render.Result: the user opens the menu and selects "New worktree" after capability discovery completes. The stale callback runs
setWorktreeNameSuffix(() => null)and clears the seeded name. The render path uses the fresh value, so the name field appears empty and the Create button stays disabled at Line 793 until the user types a name. The reset effect at Line 199 already seeded a valid default, so the clearing is unintended.🐛 Proposed fix
- [intentKind, refs.length, worktrees.length, loadRefs, loadWorktrees], + [intentKind, worktreeV2Enabled, refs.length, worktrees.length, loadRefs, loadWorktrees],🤖 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/input/WorkspaceCheckoutBadge.tsx` around lines 278 - 288, Update the handleSelectMode callback in WorkspaceCheckoutBadge to include worktreeV2Enabled in its dependency list so it always uses the latest capability state. This callback currently branches on worktreeV2Enabled when handling the “new” choice, so keep the existing refs.length/worktrees.length loading behavior intact while preventing the stale closure from clearing the seeded worktree name after capability discovery.
🧹 Nitpick comments (17)
packages/server-core/src/git/mutation-lock.ts (1)
394-397: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUse a static import for
mkdir.
mkdirAsyncperforms a dynamicimport('node:fs/promises')on every asynchronous acquisition attempt. The module resolution result is cached, but the promise and microtask cost repeat on each retry inside the acquisition loop. Importmkdironce at module scope.♻️ Proposed refactor
-async function mkdirAsync(path: string, recursive: boolean): Promise<void> { - const { mkdir } = await import('node:fs/promises') - await mkdir(path, { recursive }) -} +async function mkdirAsync(path: string, recursive: boolean): Promise<void> { + await mkdir(path, { recursive }) +}Add the top-level import:
import { dirname, join, resolve as resolvePath } from 'node:path' +import { mkdir } from 'node:fs/promises'🤖 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/mutation-lock.ts` around lines 394 - 397, Update mkdirAsync to use a module-scope static import of mkdir from node:fs/promises, removing the per-call dynamic import while preserving the existing recursive mkdir behavior.packages/server-core/src/git/__tests__/worktree-settings.test.ts (3)
98-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the typed error code instead of the message text.
These assertions match message substrings with
/protected/i,/repository/i, and/checkout/i.WorktreeSettingsErrorcarries a stablecodefield for exactly this purpose. Message text is not part of the contract, and/repository/ialso matches the checkout-overlap message if the wording changes.♻️ Proposed refactor
- expect(() => settings.update({ materializationRoot: join(root, 'snapshots', 'nested') })).toThrow(/protected/i) - expect(() => settings.update({ materializationRoot: join(repositoryRoot, 'nested') })).toThrow(/repository/i) - expect(() => settings.update({ materializationRoot: join(checkoutPath, 'nested') })).toThrow(/checkout/i) + const codeOf = (fn: () => unknown): string | undefined => { + try { + fn() + } catch (error) { + return (error as WorktreeSettingsError).code + } + return undefined + } + expect(codeOf(() => settings.update({ materializationRoot: join(root, 'snapshots', 'nested') }))) + .toBe('WORKTREE_SETTINGS_PROTECTED_PATH') + expect(codeOf(() => settings.update({ materializationRoot: join(repositoryRoot, 'nested') }))) + .toBe('WORKTREE_SETTINGS_REPOSITORY_OVERLAP') + expect(codeOf(() => settings.update({ materializationRoot: join(checkoutPath, 'nested') }))) + .toBe('WORKTREE_SETTINGS_CHECKOUT_OVERLAP')🤖 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/__tests__/worktree-settings.test.ts` around lines 98 - 100, Update the three `settings.update` assertions in `worktree-settings.test.ts` to capture the thrown `WorktreeSettingsError` and assert its stable `code` field for the protected, repository, and checkout cases. Remove the message-regex checks while preserving the existing path inputs and error expectations.
125-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo test names claim cross-instance serialization, but the calls are sequential. Each test constructs two service or registry instances on the same path and then calls them one after another on a single thread. No lock contention occurs, so the tests verify version monotonicity and state transitions rather than serialization under contention. The PR states concurrency safety as an objective, so these names imply coverage that does not exist.
packages/server-core/src/git/__tests__/worktree-settings.test.ts#L125-L145: rename the test to describe version monotonicity across instances, or spawn a second process the waypackages/server-core/src/git/__tests__/mutation-lock.test.tsdoes.packages/server-core/src/git/__tests__/worktree-registry.test.ts#L243-L266: rename the test to describe owner-bind and removal-claim transitions across instances, or add a real contended case.🤖 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/__tests__/worktree-settings.test.ts` around lines 125 - 145, Rename the test at packages/server-core/src/git/__tests__/worktree-settings.test.ts:125-145 to describe version monotonicity across separate WorktreeSettingsService instances, or replace its sequential calls with genuine cross-process contention modeled on mutation-lock.test.ts. Rename the test at packages/server-core/src/git/__tests__/worktree-registry.test.ts:243-266 to describe owner-bind and removal-claim transitions across instances, or add a genuinely contended case; ensure neither test name claims serialization without concurrent execution.
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
validateForCreation.The suite covers snapshots, persistence, path validation, overlap rejection, and default-root reset. It does not exercise
validateForCreation, which is the guard that runs immediately before a checkout is materialized. That gap is why the missing version comparison inworktree-settings-service.tsLines 235-258 is not caught.Add tests that:
- Accept a snapshot captured from the current settings.
- Reject a snapshot whose
versionno longer matches the persisted version.- Reject a repository root that overlaps the materialization root.
Do you want me to generate these 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/server-core/src/git/__tests__/worktree-settings.test.ts` at line 34, Add coverage for WorktreeSettingsService.validateForCreation in the existing suite: verify a snapshot from the current settings is accepted, a snapshot with a stale version is rejected, and an overlapping repository root versus materialization root is rejected. Use the existing test fixtures and assertion conventions without changing production behavior.packages/server-core/src/git/__tests__/worktree-registry.test.ts (2)
76-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the idempotence assertion.
The test compares
statSync(path).mtimeMsbefore and after the secondload(). Filesystem timestamp granularity can be one second or coarser, so a rewrite inside the same tick still produces an equal value. The invariant under test is that a valid V2 registry is not rewritten, which is a core claim of this change.Compare the inode and the bytes as well.
💚 Proposed fix
- const before = statSync(path).mtimeMs + const beforeStat = statSync(path) + const beforeBytes = readFileSync(path, 'utf8') registry.load() - expect(statSync(path).mtimeMs).toBe(before) + const afterStat = statSync(path) + expect(afterStat.ino).toBe(beforeStat.ino) + expect(afterStat.mtimeMs).toBe(beforeStat.mtimeMs) + expect(readFileSync(path, 'utf8')).toBe(beforeBytes) expect(registry.list()).toHaveLength(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/git/__tests__/worktree-registry.test.ts` around lines 76 - 79, Strengthen the second load() assertion in the worktree registry idempotence test: capture the file’s inode and contents before calling registry.load(), then assert both remain unchanged afterward in addition to mtimeMs. Keep the existing registry.list() assertion and use the same path and registry symbols.
45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
upsertIfUnchanged.The suite covers migration, corruption, recovery, racing writers, owner binding, and removal claims. It does not test
upsertIfUnchanged, which is the compare-and-swap primitive thatManagedWorktreeService.reconcilerelies on to avoid overwriting a concurrent owner bind or an in-flight removal.Add tests that assert:
upsertIfUnchangedreturnstrueand writes when the expected record matches.- It returns
falseand leaves the source bytes untouched when another writer changed the record first.- It returns
truewithout a write when the replacement equals the current record.Do you want me to generate these 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/server-core/src/git/__tests__/worktree-registry.test.ts` at line 45, Add focused tests in the WorktreeRegistry suite for upsertIfUnchanged: verify matching records are replaced and return true, stale expectations return false while preserving the original bytes, and identical replacements return true without writing. Reuse the suite’s existing registry setup and record fixtures to cover these compare-and-swap behaviors.packages/server-core/src/git/worktree-settings-service.ts (1)
204-222: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
getSnapshotperforms filesystem writes on a read path.
getSnapshotcallsensureRootUsable, which runsmkdirSync,openSyncwithwx,writeFileSync,fsyncSync,realpathSync, andrmSyncon every call. It also acquires the cross-process settings lock. Callers treatgetSnapshotas a cheap read:snapshot()delegates to it, and the RPC layer exposes it for settings display.Consider separating the concerns:
- Keep the writability probe in
updateandvalidateForCreation, where the root is about to be used.- Let
getSnapshotread and validate the stored settings without creating directories or probe files.If the directory creation on read is required so that a fresh install shows an existing default root, restrict the probe to the case where the root does not yet exist.
🤖 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-settings-service.ts` around lines 204 - 222, Update getSnapshot so it does not perform filesystem writes or acquire the write-oriented validation path on every read. Read and validate stored settings without calling ensureRootUsable; retain writability probing in update and validateForCreation, or limit getSnapshot’s probe to only a missing root when needed for fresh-install behavior.packages/server-core/src/git/worktree-registry.ts (1)
424-451: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winOne atomic-write helper is duplicated in two files, and neither fsyncs the parent directory.
writeBytesAtomicallyandwriteAtomicallyimplement the same sequence: create a0o600temporary file withwx, write, best-effortfsyncSyncthe file descriptor, rename, and remove the temporary file on failure. Neither fsyncs the containing directory, so the new directory entry is not durable after a power loss even though both callers verify the written hash. Both files are described as authoritative server-owned state rather than caches.
packages/server-core/src/git/worktree-registry.ts#L424-L451: extract this helper into a shared module, and fsync the parent directory afterrenameSync.packages/server-core/src/git/worktree-settings-service.ts#L116-L146: delete the localwriteAtomicallyand call the shared helper so the settings file gains the same durability guarantee.🤖 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-registry.ts` around lines 424 - 451, Extract writeBytesAtomically from packages/server-core/src/git/worktree-registry.ts#L424-L451 into a shared module, preserving its temporary-file, rename, cleanup, and optional beforeRename behavior, and fsync the containing directory after renameSync. Remove writeAtomically from packages/server-core/src/git/worktree-settings-service.ts#L116-L146 and update its callers to use the shared helper; no direct change is otherwise needed at the registry site beyond the extraction and directory fsync.packages/server-core/src/git/__tests__/mutation-lock.test.ts (1)
96-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCapture child output so a child failure is diagnosable.
The polling loop gives up after 100 attempts. If the child never writes the marker,
readFileSync(started, 'utf8')throwsENOENTand the child's stderr is discarded. The test at Lines 129-131 already collectsstdoutandstderrintochildOutput. Apply the same pattern here.🤖 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/__tests__/mutation-lock.test.ts` around lines 96 - 102, Update the test block polling for the started marker to collect the child process output in a childOutput variable, matching the existing pattern used later in the test. When the marker is absent and readFileSync throws, include the captured stdout and stderr in the failure diagnostics while preserving the current polling and lock assertions.packages/shared/src/protocol/__tests__/git-contracts.test.ts (1)
79-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unnecessary non-null assertion.
ManagedWorktreeRecordV2.managedWorktreeIdandSessionCheckoutV2.managedWorktreeIdare bothstring. The!operator adds no value here and hides a future type change.♻️ Proposed cleanup
const record: ManagedWorktreeRecordV2 = { - managedWorktreeId: checkout.managedWorktreeId!, + managedWorktreeId: checkout.managedWorktreeId,🤖 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/__tests__/git-contracts.test.ts` around lines 79 - 80, Remove the non-null assertion from the managedWorktreeId assignment in the record construction using ManagedWorktreeRecordV2 and SessionCheckoutV2, leaving the existing property access unchanged.packages/server-core/src/git/__tests__/managed-worktree-service.test.ts (3)
273-301: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the Unicode checkout leaf exists on disk.
The test asserts only the returned
checkoutPathstring. Filesystems normalize Unicode differently: APFS and HFS+ can store the NFD form, so a later path comparison orexistsSyncon the NFC string can fail. Add a filesystem assertion so the test proves the sanitized leaf is usable, not only well-formed.💚 Proposed addition
expect(record.checkoutPath).toMatch(/team-認証-refresh-[0-9a-f]{8}$/) + expect(existsSync(record.checkoutPath)).toBe(true) + expect(existsSync(join(record.checkoutPath, '.git'))).toBe(true)🤖 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/__tests__/managed-worktree-service.test.ts` around lines 273 - 301, Extend the test around createWorktree to assert that the returned record.checkoutPath exists on disk, using the filesystem assertion utility already used by nearby tests. Keep the existing expectedBranch, displayName, and path-format checks, and ensure the assertion validates the Unicode sanitized leaf rather than only the string shape.
61-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the feature-flag save/restore boilerplate into a helper.
Ten new tests repeat the same four-line save, two-line set, and four-line restore block for
KATA_FEATURE_GIT_WORKSPACE_V1andKATA_FEATURE_WORKTREE_V2. One missedfinallybranch leaks flag state into later tests in the file. A single helper removes the duplication and makes the leak impossible.♻️ Proposed helper
async function withFlags( flags: Record<string, string | undefined>, run: () => Promise<void>, ): Promise<void> { const previous = Object.fromEntries(Object.keys(flags).map((k) => [k, process.env[k]])) for (const [key, value] of Object.entries(flags)) { if (value === undefined) delete process.env[key] else process.env[key] = value } try { await run() } finally { for (const [key, value] of Object.entries(previous)) { if (value === undefined) delete process.env[key] else process.env[key] = value } } }- test('creates a named V2 worktree with the exact requested branch and a safe unique leaf', async () => { - const previousV1 = process.env.KATA_FEATURE_GIT_WORKSPACE_V1 - const previousV2 = process.env.KATA_FEATURE_WORKTREE_V2 - process.env.KATA_FEATURE_GIT_WORKSPACE_V1 = '1' - process.env.KATA_FEATURE_WORKTREE_V2 = '1' - try { + test('creates a named V2 worktree with the exact requested branch and a safe unique leaf', async () => { + await withFlags({ KATA_FEATURE_GIT_WORKSPACE_V1: '1', KATA_FEATURE_WORKTREE_V2: '1' }, async () => { const repo = tmp() // ... - } finally { - if (previousV1 === undefined) delete process.env.KATA_FEATURE_GIT_WORKSPACE_V1 - else process.env.KATA_FEATURE_GIT_WORKSPACE_V1 = previousV1 - if (previousV2 === undefined) delete process.env.KATA_FEATURE_WORKTREE_V2 - else process.env.KATA_FEATURE_WORKTREE_V2 = previousV2 - } + }) })🤖 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/__tests__/managed-worktree-service.test.ts` around lines 61 - 99, The test setup in managed-worktree-service.test.ts repeats feature-flag save/set/restore logic for KATA_FEATURE_GIT_WORKSPACE_V1 and KATA_FEATURE_WORKTREE_V2, and one path can leave process.env mutated for later tests. Extract that boilerplate into a shared helper (around the existing worktree tests) and wrap each flag-dependent test body with it so the previous env values are always restored, using the same KATA_FEATURE_GIT_WORKSPACE_V1 and KATA_FEATURE_WORKTREE_V2 symbols already present in the test file.
139-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the patched service methods after each injection test.
Both tests replace members through
(svc.repository as any).getContextand(svc.worktrees as any).getBranchOidand never restore them. The current tests pass becauseservicesFor()builds a new instance per test. If a later change moves service construction tobeforeAllor a module-level cache, the patched method leaks into other tests and produces confusing failures. Restore the original member in afinallyblock.Also applies to: 191-238
🤖 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/__tests__/managed-worktree-service.test.ts` around lines 139 - 189, Restore the original patched service members in each injection test by saving the bound methods from svc.repository.getContext and svc.worktrees.getBranchOid before overriding them, then reassigning them in a finally block after the createWorktree call. Keep the existing injection behavior and assertions unchanged, and make the cleanup local to the test cases that monkey-patch these methods so no state leaks past the test boundary.packages/server-core/src/handlers/rpc/git.test.ts (1)
420-435: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a capability-advertisement assertion for the disabled flag.
This test covers
GET_WORKTREE_SETTINGSandUPDATE_WORKTREE_SETTINGSwhile V2 is disabled. It does not assert whatGET_CAPABILITIESreturns in that state. The renderer depends on that exact answer:WorkspaceCheckoutBadgekeeps the V1 intent shape only when the response reportsworktreeV2: false.Assert
GET_CAPABILITIESresolves to{ serverId: 'mock-server', worktreeV2: false }in this test, so a regression that advertises V2 while the flag is off fails here.🤖 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 420 - 435, Extend the disabled-flag test to call the GET_CAPABILITIES handler and assert it resolves to { serverId: 'mock-server', worktreeV2: false }. Keep the existing capability-error assertions for GET_WORKTREE_SETTINGS and UPDATE_WORKTREE_SETTINGS unchanged.packages/server-core/src/git/managed-worktree-service.ts (1)
1257-1261: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle a concurrent directory creation instead of failing the whole creation.
The
existsSynccheck andmkdirSync(current)are separate operations. The mutation lock serializes by Git common directory, so two creations for different repositories in the same workspace race on the shared<root>/<workspaceId>component. The loser getsEEXISTfrommkdirSync, and the catch block at Line 1282 converts it intoWORKTREE_DESTINATION_UNSAFE. The reported cause is then wrong and the creation fails for a benign reason.Tolerate
EEXISTexplicitly and keep the subsequent no-follow checks as the actual safety gate.♻️ Proposed fix
let current = root for (const component of rel.split(/[\\/]+/).filter(Boolean)) { current = join(current, component) - if (!existsSync(current)) mkdirSync(current) + try { + mkdirSync(current) + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error + } if (this.isSymlink(current)) {🤖 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/managed-worktree-service.ts` around lines 1257 - 1261, Update the directory creation loop in the managed worktree creation flow to tolerate an EEXIST error from mkdirSync(current), which can occur after the existsSync check during concurrent creation. Preserve the subsequent no-follow symlink and safety checks as the authoritative validation, while continuing to propagate other mkdir errors so only benign concurrent directory creation is ignored.apps/electron/src/renderer/components/app-shell/input/__tests__/checkout-controls.test.ts (1)
61-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the documented difference between the two normalizers.
normalizeWorktreeNameInputis documented to keep a trailing separator while the user types, andnormalizeWorktreeNameis documented to trim it. Both current tests use inputs where the two functions return the same value, so a regression that merges the two behaviors would not fail.Add a case such as
normalizeWorktreeNameInput('auth ')versusnormalizeWorktreeName('auth '), and one case for an input that Git rejects as a ref name.🤖 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/input/__tests__/checkout-controls.test.ts` around lines 61 - 70, Extend the normalizeWorktreeName test suite with assertions that distinguish normalizeWorktreeNameInput from normalizeWorktreeName: for an input such as “auth ”, verify the input normalizer preserves the trailing separator while normalizeWorktreeName trims it, and add coverage for an input that Git rejects as a ref name using the documented expected behavior.apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx (1)
177-193: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winValidate the RPC snapshot before it drives the input.
value as WorktreeSettingsSnapshotaccepts whatever the server returns. IfmaterializationRootis absent,setRoot(undefined)switches the controlledSettingsInputto an uncontrolled input and React logs a warning, andsavedRootbecomesundefinedsoisDirtyturns true immediately. The same cast risk applies tohandleSaveat Line 209 and to the capability casts at Lines 99 and 130.Check the field types before you commit them to state, and surface a clear error otherwise.
♻️ Proposed fix
.then((value) => { if (cancelled) return const next = value as WorktreeSettingsSnapshot + if (typeof next?.materializationRoot !== 'string') { + setError(t('settings.worktrees.saveFailed')) + setSnapshot(null) + return + } setSnapshot(next) setRoot(next.materializationRoot) setSavedRoot(next.materializationRoot) })Add
tto the effect dependency list when you adopt this change.🤖 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/pages/settings/WorktreesSettingsPage.tsx` around lines 177 - 193, Validate RPC responses before updating state: in the settings-loading effect, handleSave, and the capability checks near the existing casts, verify each required field has the expected type before using it. On invalid data, surface a clear translated error through the existing error state instead of committing undefined values; preserve controlled input defaults and dirty-state behavior. Add t to the effect dependency list.
🤖 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/input/checkout-controls.ts`:
- Around line 36-43: Update normalizeWorktreeName to sanitize Git-forbidden ref
characters, including ~, ^, :, ?, *, [, backslash, control characters, and the
@{ and .. sequences, while preserving the existing case, whitespace, underscore,
segment trimming, and empty-segment behavior. Ensure inputs such as feat:auth
are converted to the same valid canonical name accepted by
assertNamedBranchAvailable before the Create flow emits its intent.
In
`@apps/electron/src/renderer/components/app-shell/input/WorkspaceCheckoutBadge.tsx`:
- Around line 565-573: Update the existing-worktree list rows near the
selectedWorktreeLabel logic to resolve each worktree’s displayName with
expectedBranch as fallback, matching the trigger label behavior. Use that
resolved label for the visible primary text and include it alongside the branch
identifier in each row’s search value, preserving branch-based searching.
In `@e2e/tests/git/worktree-v2.spec.ts`:
- Around line 118-120: Update the worktree assertions around the first and
second checkout flows to query each live checkout path with git branch
--show-current after creation and again after restart. Assert the command output
equals the exact kata-agent/<name> branch value, using the checkout paths rather
than persisted expectedBranch metadata; retain the existing accessibility and
materialization checks.
- Line 151: Replace the string-prefix check in the second checkout assertion
with a path-aware relative-path validation using relative(), isAbsolute(), and
sep. Reject empty relative paths, absolute results, and paths equal to or
beginning with .. followed by sep, while accepting only checkout paths located
beneath canonicalRoot.
In `@packages/server-core/src/git/__tests__/mutation-lock.test.ts`:
- Around line 52-58: Replace the hard-coded pid 999999 in both stale-owner
markers at packages/server-core/src/git/__tests__/mutation-lock.test.ts:52-58
and 70-74 with a shared helper that spawns a short-lived child, awaits its exit,
and returns its now-proven-dead pid; use that pid when writing each marker while
preserving the existing stale-lock test behavior.
- Around line 149-150: Replace the ineffective filesystem assertions in the
mutation lock test with assertions on the actual digest lock path under
lockRoot, verifying that the path exists while the lock is held and is removed
after release. Use the lock path produced by MutationLock or getLockPath, and
remove the checks for lockRoot/git and /repo/cross-process/.kata-lock.
In `@packages/server-core/src/git/index.ts`:
- Around line 58-74: Update createGitServices so an injected
config.worktreeSettings is bound to, or rejected unless it matches, the newly
created registry used by ManagedWorktreeService. Ensure root-update validation
consults the active registry, preventing overlaps with its protected storage and
existing worktrees; preserve the existing construction path for non-injected
settings.
In `@packages/server-core/src/git/mutation-lock.ts`:
- Around line 53-58: The synchronous WorktreeRegistry APIs currently block the
Node event loop while waiting for the lock. Add asynchronous registry variants
for load and mutation that use run(), update dependent read and mutation flows
such as list(), get(), getOwnerCount(), and resolveMutationContext() to await
them, and preserve the existing synchronous APIs only where required by their
callers.
- Around line 217-237: Update the lock publication path in claimSync and claim
so rename conflicts on Windows are treated the same as POSIX lock contention:
return false for EPERM and EACCES in addition to EEXIST and ENOTEMPTY. Then
adjust the retry handling in acquireInternalSync and acquireInternal to swallow
those contention codes and continue retrying instead of surfacing them as hard
failures.
In `@packages/server-core/src/git/worktree-registry.ts`:
- Around line 1183-1204: Update the startup reconciliation flow in the git RPC
handler to use ManagedWorktreeService.reconcile, preserving its
sessionCheckouts-aware policy and blocked-state handling instead of calling
WorktreeRegistry.reconcile directly. Ensure the reconcile call passes both
knownSessionIds and sessionCheckouts through the service-level method.
In `@packages/server-core/src/git/worktree-settings-service.ts`:
- Around line 235-258: Update validateForCreation to read the currently
persisted settings from this.settingsPath while holding the existing settings
lock, then compare its persisted version with snapshot.version and throw
WORKTREE_SETTINGS_CONFLICT on mismatch. Preserve the existing shape, root
usability, and repository-overlap validations, and ensure the snapshot is
accepted only when its version matches the stored settings.
In `@packages/server-core/src/handlers/rpc/headless-server-flow.test.ts`:
- Line 136: Remove the duplicate type assertion and its unmatched closing
parenthesis in the affected expression within the headless server flow test,
leaving a single `as { materializationRoot: string; version: number }` assertion
so the file parses correctly.
In `@packages/shared/src/i18n/locales/hu.json`:
- Line 1239: Update settings.worktrees.rootPlaceholder in
packages/shared/src/i18n/locales/hu.json at lines 1239-1239 and
packages/shared/src/i18n/locales/pl.json at lines 1249-1249 to use the
untranslated /path/to/worktrees filesystem example instead of localized path
text; no other translation changes are needed.
- Around line 590-593: Re-sort the keys in the affected locale JSON so they
follow the established ASCII alphabetical order used by the other files under
packages/shared/src/i18n/locales/*.json. Update the locale object containing
git.workspace.worktreeName and its related entries to match the surrounding key
order, and keep the worktree translations unchanged; if the current order is
intentional, ensure it is documented instead of leaving the file unsorted.
In `@packages/shared/src/protocol/routing.ts`:
- Around line 416-418: Update the handlers for
RPC_CHANNELS.git.GET_WORKTREE_SETTINGS and
RPC_CHANNELS.git.UPDATE_WORKTREE_SETTINGS to perform the existing permission
check for server-owned settings before returning the snapshot or calling
worktreeSettings.update(...). Reuse the established authorization mechanism and
reject unauthorized principals before either operation proceeds.
In `@packages/shared/src/protocol/types.ts`:
- Around line 93-94: Extend the ErrorCode union in types.ts with the remaining
worktree creation codes, then update the worktree error handling in the git RPC
handler to map WORKTREE_BRANCH_COLLISION, WORKTREE_NAME_INVALID,
WORKTREE_DESTINATION_UNSAFE, and WORKTREE_BRANCH_OWNERSHIP_UNKNOWN through throw
new CodedError(...), alongside the existing WorktreeSettingsError mapping.
Ensure these errors are transported with their registered wire codes instead of
being rethrown unmodified.
---
Outside diff comments:
In
`@apps/electron/src/renderer/components/app-shell/input/WorkspaceCheckoutBadge.tsx`:
- Around line 278-288: Update the handleSelectMode callback in
WorkspaceCheckoutBadge to include worktreeV2Enabled in its dependency list so it
always uses the latest capability state. This callback currently branches on
worktreeV2Enabled when handling the “new” choice, so keep the existing
refs.length/worktrees.length loading behavior intact while preventing the stale
closure from clearing the seeded worktree name after capability discovery.
---
Nitpick comments:
In
`@apps/electron/src/renderer/components/app-shell/input/__tests__/checkout-controls.test.ts`:
- Around line 61-70: Extend the normalizeWorktreeName test suite with assertions
that distinguish normalizeWorktreeNameInput from normalizeWorktreeName: for an
input such as “auth ”, verify the input normalizer preserves the trailing
separator while normalizeWorktreeName trims it, and add coverage for an input
that Git rejects as a ref name using the documented expected behavior.
In `@apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx`:
- Around line 177-193: Validate RPC responses before updating state: in the
settings-loading effect, handleSave, and the capability checks near the existing
casts, verify each required field has the expected type before using it. On
invalid data, surface a clear translated error through the existing error state
instead of committing undefined values; preserve controlled input defaults and
dirty-state behavior. Add t to the effect dependency list.
In `@packages/server-core/src/git/__tests__/managed-worktree-service.test.ts`:
- Around line 273-301: Extend the test around createWorktree to assert that the
returned record.checkoutPath exists on disk, using the filesystem assertion
utility already used by nearby tests. Keep the existing expectedBranch,
displayName, and path-format checks, and ensure the assertion validates the
Unicode sanitized leaf rather than only the string shape.
- Around line 61-99: The test setup in managed-worktree-service.test.ts repeats
feature-flag save/set/restore logic for KATA_FEATURE_GIT_WORKSPACE_V1 and
KATA_FEATURE_WORKTREE_V2, and one path can leave process.env mutated for later
tests. Extract that boilerplate into a shared helper (around the existing
worktree tests) and wrap each flag-dependent test body with it so the previous
env values are always restored, using the same KATA_FEATURE_GIT_WORKSPACE_V1 and
KATA_FEATURE_WORKTREE_V2 symbols already present in the test file.
- Around line 139-189: Restore the original patched service members in each
injection test by saving the bound methods from svc.repository.getContext and
svc.worktrees.getBranchOid before overriding them, then reassigning them in a
finally block after the createWorktree call. Keep the existing injection
behavior and assertions unchanged, and make the cleanup local to the test cases
that monkey-patch these methods so no state leaks past the test boundary.
In `@packages/server-core/src/git/__tests__/mutation-lock.test.ts`:
- Around line 96-102: Update the test block polling for the started marker to
collect the child process output in a childOutput variable, matching the
existing pattern used later in the test. When the marker is absent and
readFileSync throws, include the captured stdout and stderr in the failure
diagnostics while preserving the current polling and lock assertions.
In `@packages/server-core/src/git/__tests__/worktree-registry.test.ts`:
- Around line 76-79: Strengthen the second load() assertion in the worktree
registry idempotence test: capture the file’s inode and contents before calling
registry.load(), then assert both remain unchanged afterward in addition to
mtimeMs. Keep the existing registry.list() assertion and use the same path and
registry symbols.
- Line 45: Add focused tests in the WorktreeRegistry suite for
upsertIfUnchanged: verify matching records are replaced and return true, stale
expectations return false while preserving the original bytes, and identical
replacements return true without writing. Reuse the suite’s existing registry
setup and record fixtures to cover these compare-and-swap behaviors.
In `@packages/server-core/src/git/__tests__/worktree-settings.test.ts`:
- Around line 98-100: Update the three `settings.update` assertions in
`worktree-settings.test.ts` to capture the thrown `WorktreeSettingsError` and
assert its stable `code` field for the protected, repository, and checkout
cases. Remove the message-regex checks while preserving the existing path inputs
and error expectations.
- Around line 125-145: Rename the test at
packages/server-core/src/git/__tests__/worktree-settings.test.ts:125-145 to
describe version monotonicity across separate WorktreeSettingsService instances,
or replace its sequential calls with genuine cross-process contention modeled on
mutation-lock.test.ts. Rename the test at
packages/server-core/src/git/__tests__/worktree-registry.test.ts:243-266 to
describe owner-bind and removal-claim transitions across instances, or add a
genuinely contended case; ensure neither test name claims serialization without
concurrent execution.
- Line 34: Add coverage for WorktreeSettingsService.validateForCreation in the
existing suite: verify a snapshot from the current settings is accepted, a
snapshot with a stale version is rejected, and an overlapping repository root
versus materialization root is rejected. Use the existing test fixtures and
assertion conventions without changing production behavior.
In `@packages/server-core/src/git/managed-worktree-service.ts`:
- Around line 1257-1261: Update the directory creation loop in the managed
worktree creation flow to tolerate an EEXIST error from mkdirSync(current),
which can occur after the existsSync check during concurrent creation. Preserve
the subsequent no-follow symlink and safety checks as the authoritative
validation, while continuing to propagate other mkdir errors so only benign
concurrent directory creation is ignored.
In `@packages/server-core/src/git/mutation-lock.ts`:
- Around line 394-397: Update mkdirAsync to use a module-scope static import of
mkdir from node:fs/promises, removing the per-call dynamic import while
preserving the existing recursive mkdir behavior.
In `@packages/server-core/src/git/worktree-registry.ts`:
- Around line 424-451: Extract writeBytesAtomically from
packages/server-core/src/git/worktree-registry.ts#L424-L451 into a shared
module, preserving its temporary-file, rename, cleanup, and optional
beforeRename behavior, and fsync the containing directory after renameSync.
Remove writeAtomically from
packages/server-core/src/git/worktree-settings-service.ts#L116-L146 and update
its callers to use the shared helper; no direct change is otherwise needed at
the registry site beyond the extraction and directory fsync.
In `@packages/server-core/src/git/worktree-settings-service.ts`:
- Around line 204-222: Update getSnapshot so it does not perform filesystem
writes or acquire the write-oriented validation path on every read. Read and
validate stored settings without calling ensureRootUsable; retain writability
probing in update and validateForCreation, or limit getSnapshot’s probe to only
a missing root when needed for fresh-install behavior.
In `@packages/server-core/src/handlers/rpc/git.test.ts`:
- Around line 420-435: Extend the disabled-flag test to call the
GET_CAPABILITIES handler and assert it resolves to { serverId: 'mock-server',
worktreeV2: false }. Keep the existing capability-error assertions for
GET_WORKTREE_SETTINGS and UPDATE_WORKTREE_SETTINGS unchanged.
In `@packages/shared/src/protocol/__tests__/git-contracts.test.ts`:
- Around line 79-80: Remove the non-null assertion from the managedWorktreeId
assignment in the record construction using ManagedWorktreeRecordV2 and
SessionCheckoutV2, leaving the existing property access unchanged.
🪄 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: 9a8c5237-0a41-4edf-a3ae-ec071a4f8a01
⛔ Files ignored due to path filters (7)
apps/electron/resources/release-notes/next.mdis excluded by!**/*.mddocs/adrs/2026-07-29-server-owned-managed-worktrees.mdis excluded by!**/*.mddocs/adrs/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 (49)
apps/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/icons/SettingsIcons.tsxapps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsxapps/electron/src/renderer/pages/settings/settings-pages.tsapps/electron/src/shared/__tests__/ipc-channels.test.tsapps/electron/src/shared/menu-schema.tsapps/electron/src/shared/settings-registry.tsapps/electron/src/shared/types.tsapps/electron/src/transport/channel-map.tsapps/online-docs/core-concepts/git-worktrees.mdxe2e/src/config/tags.tse2e/src/flows/gitWorkspace.tse2e/tests/git/worktree-v2.spec.tspackages/server-core/src/git/__tests__/managed-worktree-service.test.tspackages/server-core/src/git/__tests__/mutation-lock.test.tspackages/server-core/src/git/__tests__/prepare-checkout.test.tspackages/server-core/src/git/__tests__/reconcile.test.tspackages/server-core/src/git/__tests__/remove-worktree-safety.test.tspackages/server-core/src/git/__tests__/worktree-registry.test.tspackages/server-core/src/git/__tests__/worktree-settings.test.tspackages/server-core/src/git/index.tspackages/server-core/src/git/managed-worktree-service.tspackages/server-core/src/git/mutation-lock.tspackages/server-core/src/git/worktree-registry.tspackages/server-core/src/git/worktree-settings-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/index.tspackages/server-core/src/handlers/session-manager-interface.tspackages/server-core/src/sessions/SessionManager.tspackages/shared/src/__tests__/feature-flags.test.tspackages/shared/src/feature-flags.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__/git-contracts.test.tspackages/shared/src/protocol/channels.tspackages/shared/src/protocol/dto.tspackages/shared/src/protocol/git.tspackages/shared/src/protocol/routing.tspackages/shared/src/protocol/types.tspackages/shared/src/sessions/types.ts
added 6 commits
August 5, 2026 08:15
The owning server's KATA_FEATURE_WORKTREE_V2 flag now drives the V2 name field, the Worktrees settings page, and its navigation entry, matching the documented server-side opt-in contract. A remote/headless V2-capable server no longer requires the desktop client to set the flag locally, and a V1-only server retains the V1 intent shape.\n\nRefs #40
Defer new-worktree preparation until the owning server's capability resolves so a V2-capable server never receives a persisted V1 intent, strip Git-forbidden ref characters in the name normalizer, and show the V2 display name in the existing-worktree list and search.\n\nRefs #40
Treat Windows rename-onto-existing-directory codes as lock contention without leaking the claim directory, prove stale-owner pids are dead in tests, assert the digest lock path positively, and reject injected worktree settings bound to a registry other than the active one.\n\nRefs #40
Register WORKTREE_NAME_INVALID, WORKTREE_BRANCH_COLLISION, WORKTREE_DESTINATION_UNSAFE, and WORKTREE_BRANCH_OWNERSHIP_UNKNOWN as wire ErrorCodes and map WorktreeCreationError through CodedError in the prepare-checkout handler.\n\nRefs #40
The materialization-root example stays /path/to/worktrees in every locale; translated path examples are not valid on any system.\n\nRefs #40
Query git branch --show-current for both checkouts after creation and after restart, and validate the custom-root checkout with a path-aware relative containment check instead of a string prefix.\n\nRefs #40
8 tasks
added 3 commits
August 5, 2026 08:59
The New-worktree menu callback captured the initial false capability forever, so opening the menu after capability resolution cleared the seeded default name and left Create disabled on every V2-capable server. Add worktreeV2Enabled to the callback dependencies.\n\nRefs #40
The repo .env enables Worktree V2, so every E2E server advertised V2 and the V1 @git specs hit the V2 name flow. Pin V2 off in the V1 specs (matching the existing V1 pinning pattern), and update the branch-badge spec for the app's empty-session auto-delete by keeping a draft in the first session.\n\nRefs #40
Assert the New-worktree Create button is enabled with the capability-seeded default before typing, guarding the stale-closure fix.\n\nRefs #40
gannonh
deleted the
feat/worktree-v2-phase-1-custom-identity-and-server-o
branch
August 5, 2026 16:20
This was referenced Aug 5, 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.
Summary
kata-agent/<name>branch identity and safe unique checkout leaves.Auth Refresh→auth-refresh).Verification
bun run lint:i18n:parity && bun run lint:i18n:sorted— passed.bun run ensure:electron && bun run electron:build— passed.bun run e2e --grep '@worktree-v2.*(name|root)'— 1 pass in real Electron, including name normalization and restart recovery.git diff --check— passed.The issue's Verify comment records a 13/13 acceptance matrix. Online-docs Mintlify validation was unavailable because this checkout does not have the
mintlifyexecutable installed.Closes #40
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Greptile Summary
This change adds named managed-worktree creation, server-owned materialization settings, registry recovery, and capability-aware desktop controls.
A confirmed removal of a managed worktree with uncommitted or unique work does not complete: the RPC handler drops the server-issued confirmation before calling the removal service. A real RPC reproduction inspected a dirty worktree, supplied the returned confirmation to removal, and observed the removal blocked while the worktree remained present. The removal contract must carry that confirmation across the RPC, transport, and client call path before this change can merge safely.
Confidence Score: 4/5
What T-Rex did
Comments Outside Diff (4)
packages/server-core/src/handlers/rpc/git.ts, line 356-359 (link)REMOVE_WORKTREEaccepts onlysessionIdandforce, then callsremoveManagedWorktree(sessionId, { force }). The removal service requires the server-issuedexpectedConfirmationsnapshot for forced deletion of a worktree with uncommitted or unique work, but this handler has no way to receive or forward it. Users can therefore inspect and confirm a risky removal in the UI, yet the server always rejects the forced removal because the confirmation fingerprint never reachesManagedWorktreeService. Accept and forward a typedWorktreeRemovalConfirmationpayload through this RPC and the session-manager interface.Artifacts
Targeted REMOVE_WORKTREE confirmation forwarding validation source
Direct SessionManager removal receives inspected confirmation
REMOVE_WORKTREE RPC drops inspected confirmation
Existing git RPC handler tests pass without confirmation forwarding coverage
Prompt To Fix With AI
General comment
packages/server-core/src/handlers/rpc/git.ts:356-359registers the handler with only(sessionId, force?)and callsremoveManagedWorktree(sessionId, { force }). A caller may supply an inspectedWorktreeRemovalConfirmation, but it is neither accepted by the handler nor passed onward. The targeted runtime harness supplied a valid confirmation as the next RPC argument and observed only{ force: true }atSessionManager.SessionManagerLikecontract atpackages/server-core/src/handlers/session-manager-interface.ts:126-129model removal options as only{ force?: boolean }, while the underlying removal service supportsexpectedConfirmation(packages/server-core/src/git/managed-worktree-service.ts:750-754) and rejects forced deletion without it (:824-832).expectedConfirmation: WorktreeRemovalConfirmation, forward it unchanged toremoveManagedWorktree, update the SessionManager interface, and add an RPC test that verifies the exact inspected confirmation reaches SessionManager.packages/server-core/src/handlers/rpc/git.ts, line 356-358 (link)A client that follows the required inspect → confirm → remove flow cannot remove a dirty or unique managed worktree. The removal service requires the confirmation snapshot returned by inspection, but this handler accepts only
forceand forwards{ force }, so the confirmation is lost and the service rejects the deletion as missing confirmation. Accept and forward the confirmation asexpectedConfirmation, and cover the complete RPC flow with a dirty worktree.Prompt To Fix With AI
General comment
INSPECT_WORKTREE_REMOVALas the third RPC argument, but removal was blocked as if no confirmation was supplied and the worktree remained on disk.packages/server-core/src/handlers/rpc/git.ts:356-358declares the handler as(sessionId, force?)and callsremoveManagedWorktree(sessionId, { force }); it never accepts or forwardsexpectedConfirmation. This conflicts with the real session-manager contract atpackages/server-core/src/sessions/SessionManager.ts:5702-5710and the managed-worktree service guard atpackages/server-core/src/git/managed-worktree-service.ts:824-844, which requiresexpectedConfirmationwhenever force is used.SessionManagerInterface, transport typing, and client call site to accept aWorktreeRemovalConfirmationand forward it as{ force, expectedConfirmation: confirmation }. Add an RPC-level test covering inspect → dirty/unique confirmation → remove and assertingremoved:trueand checkout absence.Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "test(e2e): pin seeded V2 name availabili..." | Re-trigger Greptile