fix(worktree): prevent cleanup from wiping worktrees with uncommitted work#395
Conversation
… work Two bugs caused worktree data loss: 1. parseWorktreeAge regex expected 6 hex chars but generateWtId produces 12, so the name-based age safety check was dead code — all worktrees fell back to mtime-only staleness detection. 2. cleanupSessionWorktrees (called by stopChat) removed secondary worktrees immediately without checking for uncommitted work. The stale GC had auto-rescue but this path bypassed it entirely. Fixes: regex accepts 6–12 hex chars; cleanupSessionWorktrees now checks hasUncommittedWork and attempts rescueDirtyWorktree before removing. If rescue fails, the worktree is preserved on disk. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Centaur ReviewFound 5 issue(s) (4 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/__tests__/chat.test.ts
The core implementation is sound — dirty worktrees are correctly preserved on disk when rescue fails. One test has a misleading comment with an assertion that doesn't verify what it claims (checks primary instead of mitzo), and the new worktree functions lack direct unit tests.
- 🟡 bugs (L282): Test comment says 'The mitzo entry should still be in the map (not cleared)' but the assertion checks
has('primary'), nothas('mitzo'). The implementation'sworktreePaths.clear()at line 1337 of chat.ts removes the mitzo entry — sohas('mitzo')would befalse, contradicting the comment. Either the comment is wrong (the map reference doesn't matter since the worktree stays on disk) and should be updated, or the implementation should preserve dirty entries in the map and the assertion should verifyhas('mitzo')istrue.[fixable] - 🔵 style (L252): Both new tests ('skips dirty secondary when rescue fails' and 'removes dirty secondary after successful rescue') duplicate the boilerplate for importing, configuring mocks, and overriding Date.now. The existing tests in this describe block also do this. Consider extracting shared setup into the existing beforeEach/afterEach hooks to reduce per-test boilerplate.
[fixable]
server/__tests__/worktree.test.ts
The core implementation is sound — dirty worktrees are correctly preserved on disk when rescue fails. One test has a misleading comment with an assertion that doesn't verify what it claims (checks primary instead of mitzo), and the new worktree functions lack direct unit tests.
- 🔵 missing_tests:
hasUncommittedWorkandrescueDirtyWorktreeare new exported functions with non-trivial logic (execFileSync calls, error handling, gh CLI interaction) but have no direct unit tests in worktree.test.ts. They are only tested indirectly via mocks in chat.test.ts. Given the project's TDD mandate, consider adding direct tests — at minimum forhasUncommittedWorkreturning null on clean repos and a string on dirty ones, and forrescueDirtyWorktree's error paths.[fixable]
| // Dirty + rescue failed → worktree must NOT be removed | ||
| expect(removeWorktreeMock).not.toHaveBeenCalled(); | ||
| // The mitzo entry should still be in the map (not cleared) | ||
| expect(session.worktreePaths.has('primary')).toBe(true); |
There was a problem hiding this comment.
🟡 bugs: Test comment says 'The mitzo entry should still be in the map (not cleared)' but the assertion checks has('primary'), not has('mitzo'). The implementation's worktreePaths.clear() at line 1337 of chat.ts removes the mitzo entry — so has('mitzo') would be false, contradicting the comment. Either the comment is wrong (the map reference doesn't matter since the worktree stays on disk) and should be updated, or the implementation should preserve dirty entries in the map and the assertion should verify has('mitzo') is true. [fixable]
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it('skips dirty secondary when rescue fails', async () => { |
There was a problem hiding this comment.
🔵 style: Both new tests ('skips dirty secondary when rescue fails' and 'removes dirty secondary after successful rescue') duplicate the boilerplate for importing, configuring mocks, and overriding Date.now. The existing tests in this describe block also do this. Consider extracting shared setup into the existing beforeEach/afterEach hooks to reduce per-test boilerplate. [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 4 issue(s) (1 warning).
server/__tests__/chat.test.ts
Solid safety-first change — the dirty-worktree guard and rescue flow are well-structured with correct control flow. Findings are limited to test comment accuracy and coverage gaps for the underlying rescue functions.
- 🔵 style (L281): Comment says "The mitzo entry should still be in the map (not cleared)" but this is inaccurate —
cleanupSessionWorktreesalways clears the map and re-adds only 'primary' (chat.ts:1336-1338). The worktree directory is preserved (removeWorktree not called), but the map entry is removed. Comment should say something like "Worktree directory preserved on disk (removeWorktree not called)".[fixable] - 🔵 missing_tests (L318): The 'removes dirty secondary after successful rescue' test verifies
removeWorktreewas called with the right args but does not assert thatrescueDirtyWorktreewas called with the expected(path, branch, wtId)arguments. Addingexpect(rescueDirtyWorktreeMock).toHaveBeenCalledWith('/tools/mitzo/.claude/worktrees/abc', 'session/abc', 'abc')would catch regressions in the argument-wiring logic.[fixable] - 🔵 missing_tests (L282): The 'skips dirty secondary when rescue fails' test checks
has('primary')but does not assert map size. The analogous existing test (line 246) assertssession.worktreePaths.size === 1. Adding the same assertion here would ensure no stale entries leak through.[fixable]
server/__tests__/worktree.test.ts
Solid safety-first change — the dirty-worktree guard and rescue flow are well-structured with correct control flow. Findings are limited to test comment accuracy and coverage gaps for the underlying rescue functions.
- 🟡 missing_tests:
hasUncommittedWorkandrescueDirtyWorktreein worktree.ts have no dedicated unit tests — they are only exercised via mocks in chat.test.ts. Both shell out togit/ghwith non-trivial error handling (e.g.,hasUncommittedWorktreats git failures as dirty). They would benefit from their own test suite in worktree.test.ts, similar to the existingcreateWorktree/cleanupStaleWorktreestests that use temporary git repos.
|
|
||
| // Dirty + rescue failed → worktree must NOT be removed | ||
| expect(removeWorktreeMock).not.toHaveBeenCalled(); | ||
| // The mitzo entry should still be in the map (not cleared) |
There was a problem hiding this comment.
🔵 style: Comment says "The mitzo entry should still be in the map (not cleared)" but this is inaccurate — cleanupSessionWorktrees always clears the map and re-adds only 'primary' (chat.ts:1336-1338). The worktree directory is preserved (removeWorktree not called), but the map entry is removed. Comment should say something like "Worktree directory preserved on disk (removeWorktree not called)". [fixable]
| cleanupSessionWorktrees(session); | ||
|
|
||
| // Dirty but rescue succeeded → worktree should be removed | ||
| expect(removeWorktreeMock).toHaveBeenCalledWith('abc', '/tools/mitzo'); |
There was a problem hiding this comment.
🔵 missing_tests: The 'removes dirty secondary after successful rescue' test verifies removeWorktree was called with the right args but does not assert that rescueDirtyWorktree was called with the expected (path, branch, wtId) arguments. Adding expect(rescueDirtyWorktreeMock).toHaveBeenCalledWith('/tools/mitzo/.claude/worktrees/abc', 'session/abc', 'abc') would catch regressions in the argument-wiring logic. [fixable]
| // Dirty + rescue failed → worktree must NOT be removed | ||
| expect(removeWorktreeMock).not.toHaveBeenCalled(); | ||
| // The mitzo entry should still be in the map (not cleared) | ||
| expect(session.worktreePaths.has('primary')).toBe(true); |
There was a problem hiding this comment.
🔵 missing_tests: The 'skips dirty secondary when rescue fails' test checks has('primary') but does not assert map size. The analogous existing test (line 246) asserts session.worktreePaths.size === 1. Adding the same assertion here would ensure no stale entries leak through. [fixable]
- Fix misleading test comment about map entry vs disk preservation - Change git add -A to git add -u in rescueDirtyWorktree to avoid staging untracked files that may contain secrets (.env, credentials) - Rename sessionId parameter to wtId in rescueDirtyWorktree to match call sites and avoid confusion with SDK sessionId - Defer dirty worktree rescue to setTimeout(0) so cleanupSessionWorktrees doesn't block the event loop during stopChat Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Centaur ReviewFound 3 issue(s) (1 warning).
|
Centaur ReviewFound 5 issue(s) (2 warning).
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Centaur ReviewFound 4 issue(s) (1 warning).
|
…TTL, sentinel test - Fix stale `git add -A` comments in rescue-worktree tests (now -u) - Reword setTimeout comment to clarify it defers the cleanup loop, not the event loop - Add vi.advanceTimersByTime(10_000) to bust getRepoConfig TTL cache in fake-timer tests - Add test for git-failure sentinel path (hasUncommittedWork returns error string) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 4 issue(s) (2 warning).
server/chat.ts
Solid safety improvement — the rescue-before-delete logic and regex fix are correct. Main concern is a detection/rescue mismatch when dirty status comes solely from untracked files (safe but produces futile rescue attempts and confusing logs).
- 🟡 bugs (L1310): Detection/rescue mismatch for untracked-only dirty worktrees.
hasUncommittedWorkusesgit status --porcelainwhich reports untracked files (e.g. a new.envcreated during the session), butrescueDirtyWorktreeusesgit add -uwhich only stages tracked changes. If a worktree is dirty only from untracked files,hasUncommittedWorkreturns non-null, rescue is attempted,git add -ustages nothing,git commitfails → rescue returns{ success: false }→ worktree stays on disk. This is safe (no data loss), but the rescue attempt is futile and produces a confusing warning log. Consider either: (a) passing thedirtyoutput torescueDirtyWorktreeso it can detect nothing-to-commit upfront and return early with a clearer message, or (b) filteringgit status --porcelainto exclude??lines before deciding to rescue.[fixable] - 🟡 unsafe_assumptions (L1318):
setTimeout(0)defersrescueDirtyWorktreewhich callsexecFileSyncup to 4 times (each with a 30s timeout). If multiple secondary repos are dirty, each gets its ownsetTimeout(0)callback — these fire sequentially on the event loop, potentially blocking it for N × 120s. With 3 dirty secondaries that's up to 6 minutes of event-loop blocking. Consider using the asyncexecFileequivalent orworker_threads, or at minimum adding a log at the start of the callback so operators can correlate event-loop stalls.[fixable] - 🔵 style (L1314): The
captured*variables (capturedRepoName,capturedPath,capturedWtId,capturedRepoPath) are unnecessary —for...ofwithconstdestructuring creates block-scoped bindings per iteration, so the closure already captures the correct values. Removing them would reduce 4 lines of noise. That said, this is a readability-vs-conciseness tradeoff and the explicit captures do communicate intent clearly.[fixable]
server/__tests__/chat.test.ts
Solid safety improvement — the rescue-before-delete logic and regex fix are correct. Main concern is a detection/rescue mismatch when dirty status comes solely from untracked files (safe but produces futile rescue attempts and confusing logs).
- 🔵 missing_tests: No test covers the untracked-only dirty scenario:
hasUncommittedWorkreturns non-null (e.g.?? .env), butrescueDirtyWorktreefails becausegit add -u+git commitfinds nothing to commit. This is the most likely real-world dirty state for secondary worktrees (agent creates a temp file). A test would document the expected behavior and prevent regressions if thegit add -ustrategy changes.[fixable]
| continue; | ||
| } | ||
| try { | ||
| const dirty = hasUncommittedWork(path); |
There was a problem hiding this comment.
🟡 bugs: Detection/rescue mismatch for untracked-only dirty worktrees. hasUncommittedWork uses git status --porcelain which reports untracked files (e.g. a new .env created during the session), but rescueDirtyWorktree uses git add -u which only stages tracked changes. If a worktree is dirty only from untracked files, hasUncommittedWork returns non-null, rescue is attempted, git add -u stages nothing, git commit fails → rescue returns { success: false } → worktree stays on disk. This is safe (no data loss), but the rescue attempt is futile and produces a confusing warning log. Consider either: (a) passing the dirty output to rescueDirtyWorktree so it can detect nothing-to-commit upfront and return early with a clearer message, or (b) filtering git status --porcelain to exclude ?? lines before deciding to rescue. [fixable]
| const capturedPath = path; | ||
| const capturedWtId = wtId; | ||
| const capturedRepoPath = repoPath; | ||
| setTimeout(() => { |
There was a problem hiding this comment.
🟡 unsafe_assumptions: setTimeout(0) defers rescueDirtyWorktree which calls execFileSync up to 4 times (each with a 30s timeout). If multiple secondary repos are dirty, each gets its own setTimeout(0) callback — these fire sequentially on the event loop, potentially blocking it for N × 120s. With 3 dirty secondaries that's up to 6 minutes of event-loop blocking. Consider using the async execFile equivalent or worker_threads, or at minimum adding a log at the start of the callback so operators can correlate event-loop stalls. [fixable]
| if (dirty) { | ||
| // Defer rescue to avoid blocking the event loop — rescueDirtyWorktree | ||
| // makes up to 4 sync git/gh calls (~30s timeout each). | ||
| const capturedRepoName = repoName; |
There was a problem hiding this comment.
🔵 style: The captured* variables (capturedRepoName, capturedPath, capturedWtId, capturedRepoPath) are unnecessary — for...of with const destructuring creates block-scoped bindings per iteration, so the closure already captures the correct values. Removing them would reduce 4 lines of noise. That said, this is a readability-vs-conciseness tradeoff and the explicit captures do communicate intent clearly. [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 4 issue(s) (1 warning).
server/chat.ts
Solid safety improvement — the dirty-check + rescue flow is correct, git add -u is a good security hardening, and the three new test cases cover the key branches. Only minor suggestions around edge-case documentation and test completeness.
- 🟡 unsafe_assumptions (L1316): The deferred
setTimeout(0)rescue callback has no completion tracking. IfcleanupSessionWorktreesis called during process shutdown (e.g. SIGTERM handler), the callback may never execute. The worktree directory survives on disk so stale GC will eventually find it, but a rescue opportunity is silently lost. Consider logging at thesetTimeoutscheduling site (not just inside the callback) so operators can correlate cleanup-start with rescue-start.[fixable] - 🔵 style (L1312): The explicit
capturedRepoName,capturedPath,capturedWtId,capturedRepoPathvariables are unnecessary —for...ofwithconstdestructuring already creates fresh bindings per iteration that closures capture correctly. The captures add 4 lines of noise. Either remove them or add a one-line comment explaining the defensive intent.[fixable]
server/worktree.ts
Solid safety improvement — the dirty-check + rescue flow is correct, git add -u is a good security hardening, and the three new test cases cover the key branches. Only minor suggestions around edge-case documentation and test completeness.
- 🔵 bugs (L453):
hasUncommittedWorkdetects untracked files (?? filein porcelain output), butgit add -uwill not stage them — sogit commitwill fail with 'nothing to commit', rescue returns failure, and the worktree is preserved. The outcome is safe (no data loss), but a worktree that is 'dirty' only because of untracked files will always fail rescue and never be cleaned up until stale GC. If this is intentional, the comment onhasUncommittedWork("modified, staged, or untracked files") should note that untracked-only dirtiness intentionally skips rescue.[fixable]
server/__tests__/chat.test.ts
Solid safety improvement — the dirty-check + rescue flow is correct, git add -u is a good security hardening, and the three new test cases cover the key branches. Only minor suggestions around edge-case documentation and test completeness.
- 🔵 missing_tests (L277): The 'skips dirty secondary when rescue fails' test asserts
removeWorktreeMockwas not called and primary survives, but doesn't assert the Map size. Addingexpect(session.worktreePaths.size).toBe(1)would explicitly verify the secondary entry was removed from the tracking Map (separate from the on-disk worktree being preserved).[fixable]
| // makes up to 4 sync git/gh calls (~30s timeout each). | ||
| const capturedRepoName = repoName; | ||
| const capturedPath = path; | ||
| const capturedWtId = wtId; |
There was a problem hiding this comment.
🟡 unsafe_assumptions: The deferred setTimeout(0) rescue callback has no completion tracking. If cleanupSessionWorktrees is called during process shutdown (e.g. SIGTERM handler), the callback may never execute. The worktree directory survives on disk so stale GC will eventually find it, but a rescue opportunity is silently lost. Consider logging at the setTimeout scheduling site (not just inside the callback) so operators can correlate cleanup-start with rescue-start. [fixable]
| try { | ||
| const dirty = hasUncommittedWork(path); | ||
| if (dirty) { | ||
| // Defer rescue so the cleanup loop can finish first — rescueDirtyWorktree |
There was a problem hiding this comment.
🔵 style: The explicit capturedRepoName, capturedPath, capturedWtId, capturedRepoPath variables are unnecessary — for...of with const destructuring already creates fresh bindings per iteration that closures capture correctly. The captures add 4 lines of noise. Either remove them or add a one-line comment explaining the defensive intent. [fixable]
| @@ -450,8 +451,9 @@ export function rescueDirtyWorktree( | |||
| } | |||
|
|
|||
| try { | |||
There was a problem hiding this comment.
🔵 bugs: hasUncommittedWork detects untracked files (?? file in porcelain output), but git add -u will not stage them — so git commit will fail with 'nothing to commit', rescue returns failure, and the worktree is preserved. The outcome is safe (no data loss), but a worktree that is 'dirty' only because of untracked files will always fail rescue and never be cleaned up until stale GC. If this is intentional, the comment on hasUncommittedWork ("modified, staged, or untracked files") should note that untracked-only dirtiness intentionally skips rescue. [fixable]
| ['primary', { path: '/repo/.claude/worktrees/abc', wtId: 'abc' }], | ||
| ['mitzo', { path: '/tools/mitzo/.claude/worktrees/abc', wtId: 'abc' }], | ||
| ]), | ||
| } as unknown as ManagedSession; |
There was a problem hiding this comment.
🔵 missing_tests: The 'skips dirty secondary when rescue fails' test asserts removeWorktreeMock was not called and primary survives, but doesn't assert the Map size. Adding expect(session.worktreePaths.size).toBe(1) would explicitly verify the secondary entry was removed from the tracking Map (separate from the on-disk worktree being preserved). [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 5 issue(s) (2 warning).
server/worktree.ts
Solid data-safety improvement with good test coverage. The main concern is a semantic mismatch: hasUncommittedWork detects untracked files but git add -u can't rescue them — safe (worktree preserved) but confusing. The deferred-rescue approach works but creates orphaned worktrees on failure.
- 🟡 bugs (L456): Semantic mismatch between detection and rescue:
hasUncommittedWorkdetects untracked files (??in porcelain output), butgit add -uonly stages tracked files. If a worktree is dirty only due to untracked files, rescue will attemptgit commitwith nothing staged — the commit fails, rescue reports failure, and the worktree is preserved. This is safe (no data loss), but the rescue error message will be confusing ("nothing to commit"). Consider either: (a) passing--no-untracked-filestogit status --porcelaininhasUncommittedWorkso detection matches what rescue can handle, or (b) documenting this as intentional in the rescue comment.[fixable]
server/__tests__/chat.test.ts
Solid data-safety improvement with good test coverage. The main concern is a semantic mismatch: hasUncommittedWork detects untracked files but git add -u can't rescue them — safe (worktree preserved) but confusing. The deferred-rescue approach works but creates orphaned worktrees on failure.
- 🔵 missing_tests: No test for the case where
hasUncommittedWorkthrows an exception (not a git-status-failure sentinel, but an actual JS exception — e.g.ENOENTif the worktree path was deleted between iteration and check). Thetry/catchat line 1309 of chat.ts wraps the entire block including thehasUncommittedWorkcall, so it would be caught, but the behavior (falling through to the outer catch, logging, then continuing) isn't explicitly tested.[fixable]
server/chat.ts
Solid data-safety improvement with good test coverage. The main concern is a semantic mismatch: hasUncommittedWork detects untracked files but git add -u can't rescue them — safe (worktree preserved) but confusing. The deferred-rescue approach works but creates orphaned worktrees on failure.
- 🟡 unsafe_assumptions (L1318): The deferred rescue via
setTimeout(0)means rescue runs aftersession.worktreePaths.clear()(line 1354). If rescue fails and the worktree is preserved on disk, there is no record of it in the session'sworktreePathsmap — the worktree becomes orphaned from the session's perspective. It will only be cleaned up by the stale GC (96h). This is probably acceptable given the alternative (blocking the cleanup loop for ~2min of sync git calls), but the orphan scenario should be documented or the stale GC should be mentioned in the rescue-failure log message.[fixable] - 🔵 style (L1314): The four
const captured*variables exist because the loop vars would be stale inside thesetTimeoutcallback. Since this is afor...ofover a Map withconstdestructuring, the bindings are per-iteration and already stable —repoName,path,wtIdare safe to close over directly. OnlyrepoPath(derived fromconfig.repos[repoName]) needs capture, and it's also block-scoped. The captures are harmless but unnecessary.[fixable]
server/__tests__/rescue-worktree.test.ts
Solid data-safety improvement with good test coverage. The main concern is a semantic mismatch: hasUncommittedWork detects untracked files but git add -u can't rescue them — safe (worktree preserved) but confusing. The deferred-rescue approach works but creates orphaned worktrees on failure.
- 🔵 missing_tests: No test verifies rescue behavior when
git add -usucceeds but stages nothing (untracked-only dirt). The commit step would then fail with "nothing to commit". This is the edge case created by changing-Ato-u.[fixable]
| execFileSync('git', ['-C', worktreePath, 'add', '-A'], gitOpts); | ||
| // 1. Stage tracked file changes only (-u avoids staging untracked files | ||
| // that may contain secrets like .env or credentials) | ||
| execFileSync('git', ['-C', worktreePath, 'add', '-u'], gitOpts); |
There was a problem hiding this comment.
🟡 bugs: Semantic mismatch between detection and rescue: hasUncommittedWork detects untracked files (?? in porcelain output), but git add -u only stages tracked files. If a worktree is dirty only due to untracked files, rescue will attempt git commit with nothing staged — the commit fails, rescue reports failure, and the worktree is preserved. This is safe (no data loss), but the rescue error message will be confusing ("nothing to commit"). Consider either: (a) passing --no-untracked-files to git status --porcelain in hasUncommittedWork so detection matches what rescue can handle, or (b) documenting this as intentional in the rescue comment. [fixable]
| const capturedPath = path; | ||
| const capturedWtId = wtId; | ||
| const capturedRepoPath = repoPath; | ||
| setTimeout(() => { |
There was a problem hiding this comment.
🟡 unsafe_assumptions: The deferred rescue via setTimeout(0) means rescue runs after session.worktreePaths.clear() (line 1354). If rescue fails and the worktree is preserved on disk, there is no record of it in the session's worktreePaths map — the worktree becomes orphaned from the session's perspective. It will only be cleaned up by the stale GC (96h). This is probably acceptable given the alternative (blocking the cleanup loop for ~2min of sync git calls), but the orphan scenario should be documented or the stale GC should be mentioned in the rescue-failure log message. [fixable]
| if (dirty) { | ||
| // Defer rescue so the cleanup loop can finish first — rescueDirtyWorktree | ||
| // makes up to 4 sync git/gh calls (~30s timeout each). | ||
| const capturedRepoName = repoName; |
There was a problem hiding this comment.
🔵 style: The four const captured* variables exist because the loop vars would be stale inside the setTimeout callback. Since this is a for...of over a Map with const destructuring, the bindings are per-iteration and already stable — repoName, path, wtId are safe to close over directly. Only repoPath (derived from config.repos[repoName]) needs capture, and it's also block-scoped. The captures are harmless but unnecessary. [fixable]
…edge tests - Document detection/rescue mismatch in rescueDirtyWorktree JSDoc (hasUncommittedWork detects untracked, git add -u only stages tracked) - Remove unnecessary captured* variables — for...of const bindings are per-iteration and safe to close over directly - Improve rescue-failure log to mention stale GC will handle orphans - Add test: hasUncommittedWork throws exception (ENOENT) — caught gracefully - Add test: untracked-only dirt causes commit failure (intentional safe path) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 2 issue(s).
server/__tests__/chat.test.ts
Well-structured data-loss prevention: git add -A → -u plugs a secret-staging hole, dirty-worktree rescue is correctly deferred via setTimeout(0) with safe closures, and test coverage is thorough. Two minor suggestions around test robustness.
- 🔵 style: New dirty-worktree tests manually reset mock state at the end of each test body (
hasUncommittedWorkMock.mockReturnValue(null); vi.restoreAllMocks()). If a test fails mid-body, cleanup doesn't run and stale mock state leaks into the next test. Moving these resets into anafterEachinside the describe block would be more robust — the existingbeforeEachalready follows this pattern forremoveWorktreeMock.[fixable] - 🔵 missing_tests: No test covers the case where multiple secondary worktrees are dirty simultaneously. The
setTimeout(0)design means N callbacks fire after the loop completes andworktreePaths.clear()runs. The closures are correct (per-iterationconstbindings), but a test with 2+ dirty secondaries (one rescue succeeds, one fails) would document this concurrent-callback behavior explicitly.[fixable]
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
LGTM — no issues found.
…ch, add multi-secondary test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
[a-f0-9]{6}butgenerateWtIdproduces 12 hex chars — name-based age safety check was dead code, all worktrees fell back to mtime-only stalenessstopChat()removed secondary worktrees immediately without checking for uncommitted work. Now callshasUncommittedWork()+rescueDirtyWorktree()before removal — if rescue fails, worktree is preserved on diskRoot cause
Two bugs working together caused data loss:
cleanupSessionWorktrees) bypassed the auto-rescue that the stale GC hadTest plan
parseWorktreeAgeaccepts 6–12 hex char suffixesparseWorktreeAgerejects <6 and >12 hex charscleanupSessionWorktreesskips dirty secondary when rescue failscleanupSessionWorktreesremoves dirty secondary after successful rescueFollow-up
Telos
e28ee6e9: persist full SessionRegistry to disk so startup cleanup never runs with empty registry (Bug 3).🤖 Generated with Claude Code