fix(worktree): prevent primary worktree deletion on cleanup#391
Conversation
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 6 issue(s) (2 warning).
packages/client/src/sse-connection.ts
Worktree dedup fix is solid defense-in-depth but relies on string equality for path comparison without normalization. The SSE reconnect refactor has a race condition: the fire-and-forget reconnect POST may not arrive before flushed pending sends.
- 🟡 bugs (L224): Race condition:
doPost('reconnect', ...)is fire-and-forget (not awaited), butflushPendingSends()on line 234 runs immediately after. If there are queued sends, they race the reconnect POST to the server. If a send arrives before the reconnect, the server hasn't reattached the session yet — events may be lost or misordered. The reconnect POST should be awaited before flushing pending sends.[fixable]
server/worktree.ts
Worktree dedup fix is solid defense-in-depth but relies on string equality for path comparison without normalization. The SSE reconnect refactor has a race condition: the fire-and-forget reconnect POST may not arrive before flushed pending sends.
- 🟡 unsafe_assumptions (L243): Path deduplication uses strict string equality (
repoPath === primaryRepo) with no normalization.loadRepoConfiginrepo-config.tsstores paths as raw strings from.mitzo.json. If the primary and secondary differ by trailing slash, symlink, or relative vs absolute form, the guard fails and both entries are added. Consider usingpath.resolve()orfs.realpathSync()before comparison.[fixable]
server/chat.ts
Worktree dedup fix is solid defense-in-depth but relies on string equality for path comparison without normalization. The SSE reconnect refactor has a race condition: the fire-and-forget reconnect POST may not arrive before flushed pending sends.
- 🔵 unsafe_assumptions (L1238): Same string-equality issue as
discoverSessionWorktrees:path === primaryPathcompares worktree paths constructed viajoin(repoPath, ...). If the repo paths weren't normalized at discovery time, this guard also fails. However, since both paths are constructed withjoin()from the same source, the cleanup guard will match whenever the discovery guard matches — so this is consistent, just not robust against symlinks or non-canonical paths.[fixable] - 🔵 style (L1233): The
cleanupSessionWorktreesguard is defense-in-depth against duplicates thatdiscoverSessionWorktreesshould have already filtered. The comment explains why, which is good. But this means there are two dedup mechanisms that must stay in sync. Consider extracting the path normalization into a shared helper so both call sites use identical logic.[fixable]
packages/client/src/__tests__/sse-connection.test.ts
Worktree dedup fix is solid defense-in-depth but relies on string equality for path comparison without normalization. The SSE reconnect refactor has a race condition: the fire-and-forget reconnect POST may not arrive before flushed pending sends.
- 🔵 missing_tests (L293): No test verifies the ordering contract: reconnect POST must complete before pending sends are flushed. A test where
mockFetchdelays the reconnect response and verifies sends aren't dispatched until after would catch the race condition identified above.[fixable] - 🔵 missing_tests (L320): No test covers what happens when the reconnect POST fails (fetch rejects or returns non-ok). The current code silently swallows the error, but the test suite should verify the client degrades gracefully (e.g., still flushes sends, doesn't throw).
[fixable]
| // watch, no reattach, no event replay. This POST ensures every | ||
| // reconnect (auto or explicit) triggers the full server-side | ||
| // reconnect flow: watch + reattach + event replay. | ||
| if (this._isReconnect && this.seqBySession.size > 0) { |
There was a problem hiding this comment.
🟡 bugs: Race condition: doPost('reconnect', ...) is fire-and-forget (not awaited), but flushPendingSends() on line 234 runs immediately after. If there are queued sends, they race the reconnect POST to the server. If a send arrives before the reconnect, the server hasn't reattached the session yet — events may be lost or misordered. The reconnect POST should be awaited before flushing pending sends. [fixable]
| // Skip secondary repos that resolve to the same path as the primary — | ||
| // otherwise both "primary" and "mgmt" map to the same worktree, and | ||
| // cleanupSessionWorktrees removes the primary thinking it's a secondary. | ||
| if (repoPath === primaryRepo) continue; |
There was a problem hiding this comment.
🟡 unsafe_assumptions: Path deduplication uses strict string equality (repoPath === primaryRepo) with no normalization. loadRepoConfig in repo-config.ts stores paths as raw strings from .mitzo.json. If the primary and secondary differ by trailing slash, symlink, or relative vs absolute form, the guard fails and both entries are added. Consider using path.resolve() or fs.realpathSync() before comparison. [fixable]
| // Guard: never remove a secondary whose path matches the primary worktree. | ||
| // This can happen when the base repo is also listed in .mitzo.json repos | ||
| // (e.g. "mgmt"), causing discoverSessionWorktrees to add both entries. | ||
| if (primaryPath && path === primaryPath) { |
There was a problem hiding this comment.
🔵 unsafe_assumptions: Same string-equality issue as discoverSessionWorktrees: path === primaryPath compares worktree paths constructed via join(repoPath, ...). If the repo paths weren't normalized at discovery time, this guard also fails. However, since both paths are constructed with join() from the same source, the cleanup guard will match whenever the discovery guard matches — so this is consistent, just not robust against symlinks or non-canonical paths. [fixable]
| const primaryPath = session.worktreePaths.get('primary')?.path; | ||
| for (const [repoName, { wtId, path }] of session.worktreePaths) { | ||
| if (repoName === 'primary') continue; | ||
| const repoPath = config.repos[repoName]; |
There was a problem hiding this comment.
🔵 style: The cleanupSessionWorktrees guard is defense-in-depth against duplicates that discoverSessionWorktrees should have already filtered. The comment explains why, which is good. But this means there are two dedup mechanisms that must stay in sync. Consider extracting the path normalization into a shared helper so both call sites use identical logic. [fixable]
|
|
||
| it('includes sessions in reconnect URL', () => { | ||
| const conn = new SseConnection(createConfig()); | ||
| it('sends reconnect POST on welcome when has tracked sessions', () => { |
There was a problem hiding this comment.
🔵 missing_tests: No test verifies the ordering contract: reconnect POST must complete before pending sends are flushed. A test where mockFetch delays the reconnect response and verifies sends aren't dispatched until after would catch the race condition identified above. [fixable]
| }), | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🔵 missing_tests: No test covers what happens when the reconnect POST fails (fetch rejects or returns non-ok). The current code silently swallows the error, but the test suite should verify the client degrades gracefully (e.g., still flushes sends, doesn't throw). [fixable]
…so 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]>
dimakis
force-pushed
the
fix/worktree-primary-dedup
branch
from
84e1e31 to
8b92974
Compare
Centaur ReviewFound 3 issue(s) (1 warning).
|
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 3 issue(s) (1 warning).
server/worktree.ts
Solid defense-in-depth fix at two layers (prevention + safety net), but the string-equality path comparison is fragile — should use path.resolve() to handle trailing slashes, symlinks, and relative segments.
- 🟡 unsafe_assumptions (L243): String equality (
repoPath === primaryRepo) is fragile — paths from different sources (.mitzo.json vs REPO_PATH env var) may differ by trailing slash, symlink, or relative component (e.g./repovs/repo/vs/home/user/../repo). Useresolve(repoPath) === resolve(primaryRepo)(frompath) for canonical comparison. The same applies topath === primaryPathin chat.ts:1271. Sincerepo-config.tsstores paths as-is without normalization, the mismatch risk is real.[fixable]
server/__tests__/worktree.test.ts
Solid defense-in-depth fix at two layers (prevention + safety net), but the string-equality path comparison is fragile — should use path.resolve() to handle trailing slashes, symlinks, and relative segments.
- 🔵 missing_tests (L602): The test only covers the exact-string-match dedup case. A test with a trailing-slash variant (e.g.
mgmt: primaryRepo + '/') would document the current limitation or validate aresolve()-based fix.[fixable]
server/chat.ts
Solid defense-in-depth fix at two layers (prevention + safety net), but the string-equality path comparison is fragile — should use path.resolve() to handle trailing slashes, symlinks, and relative segments.
- 🔵 style (L1268): The 3-line comment block explaining the guard is justified (non-obvious invariant), but the log message at line 1272 duplicates the same explanation. Consider shortening the comment to just the first line since the log message already serves as documentation for runtime debugging.
[fixable]
| // Skip secondary repos that resolve to the same path as the primary — | ||
| // otherwise both "primary" and "mgmt" map to the same worktree, and | ||
| // cleanupSessionWorktrees removes the primary thinking it's a secondary. | ||
| if (repoPath === primaryRepo) continue; |
There was a problem hiding this comment.
🟡 unsafe_assumptions: String equality (repoPath === primaryRepo) is fragile — paths from different sources (.mitzo.json vs REPO_PATH env var) may differ by trailing slash, symlink, or relative component (e.g. /repo vs /repo/ vs /home/user/../repo). Use resolve(repoPath) === resolve(primaryRepo) (from path) for canonical comparison. The same applies to path === primaryPath in chat.ts:1271. Since repo-config.ts stores paths as-is without normalization, the mismatch risk is real. [fixable]
| expect(result.has('secondary')).toBe(false); | ||
| }); | ||
|
|
||
| it('deduplicates secondary repos that match primaryRepo', () => { |
There was a problem hiding this comment.
🔵 missing_tests: The test only covers the exact-string-match dedup case. A test with a trailing-slash variant (e.g. mgmt: primaryRepo + '/') would document the current limitation or validate a resolve()-based fix. [fixable]
| if (repoName === 'primary') continue; | ||
| const repoPath = config.repos[repoName]; | ||
| if (!repoPath) continue; | ||
| // Guard: never remove a secondary whose path matches the primary worktree. |
There was a problem hiding this comment.
🔵 style: The 3-line comment block explaining the guard is justified (non-obvious invariant), but the log message at line 1272 duplicates the same explanation. Consider shortening the comment to just the first line since the log message already serves as documentation for runtime debugging. [fixable]
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]>
Centaur ReviewFound 3 issue(s) (1 warning).
|
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]>
Centaur ReviewFound 2 issue(s).
|
1 participant
Summary
.mitzo.jsonrepos,discoverSessionWorktreesadds both"primary"and"mgmt"entries pointing to the same worktree directory. On session cleanup,cleanupSessionWorktreesskips"primary"but removes"mgmt"— destroying the primary worktree underneath the active SDK session."Path does not exist"errors mid-session, especially on iOS where frequent WS reconnects trigger zombie-abort → resume cycles that hit the cleanup path.discoverSessionWorktrees: skip secondary repos whose path matchesprimaryRepocleanupSessionWorktrees: guard against removing any secondary whose worktree path matches the primary (defense in depth)Test plan
discoverSessionWorktreesdeduplicates secondary repos matching primaryRepocleanupSessionWorktreesskips secondary whose path matches primary worktree🤖 Generated with Claude Code