Skip to content

Commit d5587a2

Browse files
dimakisclaude
andauthored
fix(worktree): prevent primary worktree deletion on cleanup (#391)
* fix(worktree): prevent primary worktree deletion when base repo is also in repos config When the base repo (mgmt) is listed in .mitzo.json repos as a secondary, discoverSessionWorktrees adds both "primary" and "mgmt" entries pointing to the same worktree. On session cleanup, cleanupSessionWorktrees skips "primary" but removes "mgmt" — destroying the primary worktree and breaking resume with "Path does not exist" errors. Fix at two layers: - discoverSessionWorktrees: skip secondary repos matching primaryRepo - cleanupSessionWorktrees: guard against removing secondaries whose path matches the primary worktree (defense in depth) Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix(worktree): normalize paths with resolve() before dedup comparison Address Centaur review: string equality could miss trailing slashes, symlinks, or relative vs absolute paths. Use path.resolve() in both discoverSessionWorktrees and cleanupSessionWorktrees guards. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix(worktree): add trailing-slash dedup test, trim comment Address second Centaur review: add test validating resolve()-based dedup handles trailing slash differences, shorten redundant comment. Co-Authored-By: Claude Opus 4.6 <[email protected]> --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
1 parent 978c453 commit d5587a2

4 files changed

Lines changed: 78 additions & 2 deletions

File tree

server/__tests__/chat.test.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,41 @@ describe('cleanupSessionWorktrees', () => {
211211

212212
expect(session.worktreePaths.size).toBe(0);
213213
});
214+
215+
it('skips secondary whose path matches primary worktree', async () => {
216+
const { loadRepoConfig } = await import('../repo-config.js');
217+
(loadRepoConfig as ReturnType<typeof vi.fn>).mockReturnValue({
218+
repos: { mgmt: '/repo', mitzo: '/tools/mitzo' },
219+
isolation: true,
220+
});
221+
222+
const realNow = Date.now;
223+
Date.now = () => realNow() + 10_000;
224+
225+
const { cleanupSessionWorktrees } = await import('../chat.js');
226+
227+
// "mgmt" secondary points to the same path as "primary" —
228+
// simulates discoverSessionWorktrees adding both entries
229+
const session = {
230+
worktreePaths: new Map([
231+
['primary', { path: '/repo/.claude/worktrees/abc', wtId: 'abc' }],
232+
['mgmt', { path: '/repo/.claude/worktrees/abc', wtId: 'abc' }],
233+
['mitzo', { path: '/tools/mitzo/.claude/worktrees/abc', wtId: 'abc' }],
234+
]),
235+
} as unknown as ManagedSession;
236+
237+
cleanupSessionWorktrees(session);
238+
239+
// mgmt should NOT be removed (same path as primary), mitzo should be removed
240+
expect(removeWorktreeMock).toHaveBeenCalledWith('abc', '/tools/mitzo');
241+
expect(removeWorktreeMock).toHaveBeenCalledTimes(1);
242+
243+
expect(session.worktreePaths.has('primary')).toBe(true);
244+
expect(session.worktreePaths.size).toBe(1);
245+
246+
Date.now = realNow;
247+
vi.restoreAllMocks();
248+
});
214249
});
215250

216251
describe('createSessionWorktrees — lazy secondary creation', () => {

server/__tests__/worktree.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -598,6 +598,34 @@ describe('discoverSessionWorktrees', () => {
598598
expect(result.has('primary')).toBe(true);
599599
expect(result.has('secondary')).toBe(false);
600600
});
601+
602+
it('deduplicates secondary repos that match primaryRepo', () => {
603+
const wtId = '2026-04-20-abc123def456';
604+
mkdirSync(join(primaryRepo, '.claude', 'worktrees', wtId), { recursive: true });
605+
606+
// Pass primaryRepo as a secondary too (simulates mgmt in .mitzo.json repos)
607+
const result = discoverSessionWorktrees(wtId, primaryRepo, {
608+
mgmt: primaryRepo,
609+
});
610+
611+
// Should only have "primary", not "mgmt" — dedup prevents double-mapping
612+
expect(result.size).toBe(1);
613+
expect(result.has('primary')).toBe(true);
614+
expect(result.has('mgmt')).toBe(false);
615+
});
616+
617+
it('deduplicates even with trailing slash differences', () => {
618+
const wtId = '2026-04-20-abc123def456';
619+
mkdirSync(join(primaryRepo, '.claude', 'worktrees', wtId), { recursive: true });
620+
621+
const result = discoverSessionWorktrees(wtId, primaryRepo, {
622+
mgmt: primaryRepo + '/',
623+
});
624+
625+
expect(result.size).toBe(1);
626+
expect(result.has('primary')).toBe(true);
627+
expect(result.has('mgmt')).toBe(false);
628+
});
601629
});
602630

603631
describe('cleanupStaleWorktrees', () => {

server/chat.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1260,10 +1260,19 @@ export function cleanupSessionWorktrees(
12601260
session: import('./session-registry.js').ManagedSession,
12611261
): void {
12621262
const config = getRepoConfig();
1263-
for (const [repoName, { wtId }] of session.worktreePaths) {
1263+
const primaryPath = session.worktreePaths.get('primary')?.path;
1264+
for (const [repoName, { wtId, path }] of session.worktreePaths) {
12641265
if (repoName === 'primary') continue;
12651266
const repoPath = config.repos[repoName];
12661267
if (!repoPath) continue;
1268+
// Guard: never remove a secondary whose path matches the primary worktree.
1269+
if (primaryPath && resolve(path) === resolve(primaryPath)) {
1270+
log.info('skipping secondary cleanup — path matches primary worktree', {
1271+
repoName,
1272+
path,
1273+
});
1274+
continue;
1275+
}
12671276
try {
12681277
removeWorktree(wtId, repoPath);
12691278
} catch (err: unknown) {

server/worktree.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import {
1414
unlinkSync,
1515
} from 'fs';
1616
import { mkdir } from 'fs/promises';
17-
import { join, basename } from 'path';
17+
import { join, basename, resolve } from 'path';
1818

1919
const execFileAsync = promisify(execFileCb);
2020
import {
@@ -237,6 +237,10 @@ export function discoverSessionWorktrees(
237237

238238
const allRepos: Array<[string, string]> = [['primary', primaryRepo]];
239239
for (const [name, repoPath] of Object.entries(secondaryRepos)) {
240+
// Skip secondary repos that resolve to the same path as the primary —
241+
// otherwise both "primary" and "mgmt" map to the same worktree, and
242+
// cleanupSessionWorktrees removes the primary thinking it's a secondary.
243+
if (resolve(repoPath) === resolve(primaryRepo)) continue;
240244
allRepos.push([name, repoPath]);
241245
}
242246

0 commit comments

Comments
 (0)