fix(worktree): skip rescue for noise-only worktrees#397
Conversation
Centaur ReviewFound 4 issue(s) (1 warning).
|
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 defensive PR addressing replay storms and noise worktrees. Two substantive issues: basename() in noise-file check is overly broad (should be strict equality), and silently capping replay depth without a client-side gap signal may cause subtle data loss on reconnect.
- 🟡 bugs (L470):
basename(f)matches.mitzo-sessionat any directory depth (e.g.subdir/.mitzo-session). Since.mitzo-sessionis only created at the worktree root, a simple equality checkf === '.mitzo-session'would be more precise and avoid false positives if a file with the same name ever appears in a subdirectory.[fixable]
packages/harness/src/connection-registry.ts
Solid defensive PR addressing replay storms and noise worktrees. Two substantive issues: basename() in noise-file check is overly broad (should be strict equality), and silently capping replay depth without a client-side gap signal may cause subtle data loss on reconnect.
- 🟡 unsafe_assumptions (L248): When the replay gap is capped, events between the old and new cursor are silently dropped with no client-side notification. The comment says clients can lazy-load via REST API, but there is no gap signal (e.g.
events_skippedmessage) sent to the client, so the client has no way to know it missed events unless it independently tracks seq continuity. Consider sending a gap notification so the client can trigger a full restore.[fixable]
server/chat.ts
Solid defensive PR addressing replay storms and noise worktrees. Two substantive issues: basename() in noise-file check is overly broad (should be strict equality), and silently capping replay depth without a client-side gap signal may cause subtle data loss on reconnect.
- 🔵 missing_tests (L1269): The new try/catch around
queryInstance.interrupt()has no test coverage. A test whereinterruptrejects (e.g.vi.fn().mockRejectedValue(new Error('not ready'))) should verify that the function still returns true and the prompt is still pushed toinputQueue.[fixable]
packages/harness/__tests__/connection-registry.test.ts
Solid defensive PR addressing replay storms and noise worktrees. Two substantive issues: basename() in noise-file check is overly broad (should be strict equality), and silently capping replay depth without a client-side gap signal may cause subtle data loss on reconnect.
- 🔵 missing_tests (L507): The 'caps replay depth' test doesn't verify the log.warn call or that the cursor is actually persisted (subsequent sync rounds should start from the new cursor, not re-cap). A follow-up assertion after a second timer advance would confirm the cursor update sticks.
[fixable]
server/index.ts
Solid defensive PR addressing replay storms and noise worktrees. Two substantive issues: basename() in noise-file check is overly broad (should be strict equality), and silently capping replay depth without a client-side gap signal may cause subtle data loss on reconnect.
- 🔵 style (L372): The comment update ('SSE Chat Transport (primary)' and 'Default transport') is documentation about the transport migration status — useful context but may go stale. Minor nit, no action needed.
| timeout: WORKTREE_GIT_TIMEOUT_MS, | ||
| }).trim(); | ||
| const files = staged ? staged.split('\n') : []; | ||
| if (files.length === 0 || files.every((f) => RESCUE_NOISE_FILES.has(basename(f)))) { |
There was a problem hiding this comment.
🟡 bugs: basename(f) matches .mitzo-session at any directory depth (e.g. subdir/.mitzo-session). Since .mitzo-session is only created at the worktree root, a simple equality check f === '.mitzo-session' would be more precise and avoid false positives if a file with the same name ever appears in a subdirectory. [fixable]
| connectionId, | ||
| sessionId, | ||
| oldCursor: cursor, | ||
| newCursor, |
There was a problem hiding this comment.
🟡 unsafe_assumptions: When the replay gap is capped, events between the old and new cursor are silently dropped with no client-side notification. The comment says clients can lazy-load via REST API, but there is no gap signal (e.g. events_skipped message) sent to the client, so the client has no way to know it missed events unless it independently tracks seq continuity. Consider sending a gap notification so the client can trigger a full restore. [fixable]
| await Promise.allSettled(stops); | ||
| } | ||
| await session.queryInstance.interrupt(); | ||
| try { |
There was a problem hiding this comment.
🔵 missing_tests: The new try/catch around queryInstance.interrupt() has no test coverage. A test where interrupt rejects (e.g. vi.fn().mockRejectedValue(new Error('not ready'))) should verify that the function still returns true and the prompt is still pushed to inputQueue. [fixable]
| const sessionIds = calls.map((c: unknown[]) => c[0]); | ||
| expect(sessionIds).toContain('sess-a'); | ||
| expect(sessionIds).toContain('sess-b'); | ||
|
|
There was a problem hiding this comment.
🔵 missing_tests: The 'caps replay depth' test doesn't verify the log.warn call or that the cursor is actually persisted (subsequent sync rounds should start from the new cursor, not re-cap). A follow-up assertion after a second timer advance would confirm the cursor update sticks. [fixable]
| }; | ||
|
|
||
| // ─── SSE Chat Transport ────────────────────────────────────────────────────── | ||
| // ─── SSE Chat Transport (primary) ──────────────────────────────────────────── |
There was a problem hiding this comment.
🔵 style: The comment update ('SSE Chat Transport (primary)' and 'Default transport') is documentation about the transport migration status — useful context but may go stale. Minor nit, no action needed.
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 5 issue(s) (1 warning).
packages/client/src/sse-connection.ts
Solid defensive fixes for replay storms and noise-only worktree rescue. Main concern: the SSE send POST re-queue has no retry limit or backoff, risking infinite retries for persistently failing messages.
- 🟡 bugs (L338): Re-queuing in
doPostbypasses theMAX_PENDING_SENDScap that guardssend(). If asendPOST fails persistently (e.g. 400 from malformed payload), it will be re-queued on every reconnect indefinitely with no retry limit, attempt counter, or backoff. The queue won't grow unbounded within a single flush (sinceflushPendingSendsswaps the array), but across reconnect cycles a poisoned message retries forever.[fixable]
packages/client/src/__tests__/sse-connection.test.ts
Solid defensive fixes for replay storms and noise-only worktree rescue. Main concern: the SSE send POST re-queue has no retry limit or backoff, risking infinite retries for persistently failing messages.
- 🔵 missing_tests (L878): No test covers the case where a re-queued
sendfails again on the next reconnect cycle. This is the scenario that would expose the unbounded retry issue — a message that always returns 400 should eventually be discarded or capped.[fixable]
server/chat.ts
Solid defensive fixes for replay storms and noise-only worktree rescue. Main concern: the SSE send POST re-queue has no retry limit or backoff, risking infinite retries for persistently failing messages.
- 🔵 missing_tests (L1269): The new try/catch around
queryInstance.interrupt()has no test covering the error path. The existing interrupt tests insend-to-chat.test.tsmockinterrupt()to succeed. A test whereinterrupt()rejects would verify the error is caught and logged without crashing the session.[fixable]
server/index.ts
Solid defensive fixes for replay storms and noise-only worktree rescue. Main concern: the SSE send POST re-queue has no retry limit or backoff, risking infinite retries for persistently failing messages.
- 🔵 style (L111): The
shouldSyncallowlist (ACTIVE || STARTING || CREATED) is correct but fragile — adding a new syncable state requires remembering to update this check. Consider a denylist (state !== 'ENDED' && state !== 'SUSPENDED' && ...) that matches the comment, or import the states as a const set, so new states default to synced rather than silently dropped.[fixable]
server/worktree.ts
Solid defensive fixes for replay storms and noise-only worktree rescue. Main concern: the SSE send POST re-queue has no retry limit or backoff, risking infinite retries for persistently failing messages.
- 🔵 unsafe_assumptions (L462): The
RESCUE_NOISE_FILESset only contains.mitzo-session, which is correct since that file is only created at the worktree root. But the set is declared at module scope — if future noise files are added at subdirectory paths,git diff --cached --name-onlywould return them assubdir/fileand exact-match would miss them. A comment noting the root-only assumption would help future maintainers.[fixable]
| }); | ||
| if (!res.ok && endpoint === 'send') { | ||
| // Message delivery failed (e.g. 404 from stale connectionId). | ||
| // Re-queue so it flushes on the next successful reconnect. |
There was a problem hiding this comment.
🟡 bugs: Re-queuing in doPost bypasses the MAX_PENDING_SENDS cap that guards send(). If a send POST fails persistently (e.g. 400 from malformed payload), it will be re-queued on every reconnect indefinitely with no retry limit, attempt counter, or backoff. The queue won't grow unbounded within a single flush (since flushPendingSends swaps the array), but across reconnect cycles a poisoned message retries forever. [fixable]
|
|
||
| // ─── POST retry on failure ─────────────────────────────────────────────── | ||
|
|
||
| it('re-queues send POST on HTTP error (e.g. 404 from stale connectionId)', async () => { |
There was a problem hiding this comment.
🔵 missing_tests: No test covers the case where a re-queued send fails again on the next reconnect cycle. This is the scenario that would expose the unbounded retry issue — a message that always returns 400 should eventually be discarded or capped. [fixable]
| await Promise.allSettled(stops); | ||
| } | ||
| await session.queryInstance.interrupt(); | ||
| try { |
There was a problem hiding this comment.
🔵 missing_tests: The new try/catch around queryInstance.interrupt() has no test covering the error path. The existing interrupt tests in send-to-chat.test.ts mock interrupt() to succeed. A test where interrupt() rejects would verify the error is caught and logged without crashing the session. [fixable]
| const state = eventStore.getSessionState(sessionId); | ||
| // Only sync sessions that are actively running or starting. | ||
| // ENDED/SUSPENDED/DETACHED/CLOSING don't need delivery retries. | ||
| return state === 'ACTIVE' || state === 'STARTING' || state === 'CREATED'; |
There was a problem hiding this comment.
🔵 style: The shouldSync allowlist (ACTIVE || STARTING || CREATED) is correct but fragile — adding a new syncable state requires remembering to update this check. Consider a denylist (state !== 'ENDED' && state !== 'SUSPENDED' && ...) that matches the comment, or import the states as a const set, so new states default to synced rather than silently dropped. [fixable]
| return { success: false, error: err instanceof Error ? err.message : String(err) }; | ||
| } | ||
|
|
||
| // 1b. Check if staged changes contain meaningful work (not just noise markers) |
There was a problem hiding this comment.
🔵 unsafe_assumptions: The RESCUE_NOISE_FILES set only contains .mitzo-session, which is correct since that file is only created at the worktree root. But the set is declared at module scope — if future noise files are added at subdirectory paths, git diff --cached --name-only would return them as subdir/file and exact-match would miss them. A comment noting the root-only assumption would help future maintainers. [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 4 issue(s) (2 warning).
server/worktree.ts
The core transport fixes (SSE retry, replay cap, interrupt guard) are solid and well-tested. The main issue is that noise-only worktrees become permanently stuck in cleanup — cleanupStaleWorktrees needs to handle the 'no meaningful changes' rescue result by removing the worktree instead of skipping it.
- 🟡 bugs (L470): Noise-only worktrees become permanently stuck.
cleanupStaleWorktreescallshasUncommittedWork(which reports dirty for.mitzo-session), thenrescueDirtyWorktreereturns{ success: false, error: 'no meaningful changes to rescue' }. The cleanup treats this identically to a real rescue failure — it skips the worktree and posts to inbox. On subsequent cycles,alreadyNotifiedis true so rescue is never retried, and the worktree is never removed. Pre-PR, these worktrees were rescued (commit+push+PR); post-PR, they accumulate on disk forever. Fix:cleanupStaleWorktreesshould detect the 'no meaningful changes' case and remove the worktree directly, since there's nothing worth saving.[fixable]
packages/harness/__tests__/connection-registry.test.ts
The core transport fixes (SSE retry, replay cap, interrupt guard) are solid and well-tested. The main issue is that noise-only worktrees become permanently stuck in cleanup — cleanupStaleWorktrees needs to handle the 'no meaningful changes' rescue result by removing the worktree instead of skipping it.
- 🟡 missing_tests (L535): The 'caps replay depth when cursor is far behind head' test (line 511) is missing
registry.stopPeriodicSync()at the end, unlike every other periodic sync test in this describe block. This leaks asetIntervaltimer that could cause flaky test behavior if timer state bleeds between tests.[fixable]
packages/protocol/src/event-store.ts
The core transport fixes (SSE retry, replay cap, interrupt guard) are solid and well-tested. The main issue is that noise-only worktrees become permanently stuck in cleanup — cleanupStaleWorktrees needs to handle the 'no meaningful changes' rescue result by removing the worktree instead of skipping it.
- 🔵 missing_tests (L618): The new
getHeadSeqmethod has no direct unit test inpackages/protocol/__tests__/event-store.test.ts. It's only indirectly tested via mockedgetHeadSeqin connection-registry tests. A bug in the SQL query (SELECT MAX(seq)) or null handling would not be caught. Add a test covering: session with events returns max seq, session with no events returns 0, unknown session returns 0.[fixable]
server/index.ts
The core transport fixes (SSE retry, replay cap, interrupt guard) are solid and well-tested. The main issue is that noise-only worktrees become permanently stuck in cleanup — cleanupStaleWorktrees needs to handle the 'no meaningful changes' rescue result by removing the worktree instead of skipping it.
- 🔵 style (L107): The
shouldSynccallback returnsfalsewhengetSessionStatereturnsnull(session not in DB). This is likely correct, but worth a brief comment since the implicit null→false behavior is non-obvious and could silently skip sync for sessions whose state record was lost (e.g. after DB corruption).[fixable]
| timeout: WORKTREE_GIT_TIMEOUT_MS, | ||
| }).trim(); | ||
| const files = staged ? staged.split('\n') : []; | ||
| if (files.length === 0 || files.every((f) => RESCUE_NOISE_FILES.has(f))) { |
There was a problem hiding this comment.
🟡 bugs: Noise-only worktrees become permanently stuck. cleanupStaleWorktrees calls hasUncommittedWork (which reports dirty for .mitzo-session), then rescueDirtyWorktree returns { success: false, error: 'no meaningful changes to rescue' }. The cleanup treats this identically to a real rescue failure — it skips the worktree and posts to inbox. On subsequent cycles, alreadyNotified is true so rescue is never retried, and the worktree is never removed. Pre-PR, these worktrees were rescued (commit+push+PR); post-PR, they accumulate on disk forever. Fix: cleanupStaleWorktrees should detect the 'no meaningful changes' case and remove the worktree directly, since there's nothing worth saving. [fixable]
| // Should have fetched from 9800 (head - 200), not from 0 | ||
| const calls = (store.getEventsAfter as ReturnType<typeof vi.fn>).mock.calls; | ||
| expect(calls[0][1]).toBe(9800); // afterSeq should be head - MAX_REPLAY_GAP | ||
| }); |
There was a problem hiding this comment.
🟡 missing_tests: The 'caps replay depth when cursor is far behind head' test (line 511) is missing registry.stopPeriodicSync() at the end, unlike every other periodic sync test in this describe block. This leaks a setInterval timer that could cause flaky test behavior if timer state bleeds between tests. [fixable]
| } | ||
|
|
||
| /** Return the highest seq number for a session, or 0 if no events exist. */ | ||
| getHeadSeq(sessionId: string): number { |
There was a problem hiding this comment.
🔵 missing_tests: The new getHeadSeq method has no direct unit test in packages/protocol/__tests__/event-store.test.ts. It's only indirectly tested via mocked getHeadSeq in connection-registry tests. A bug in the SQL query (SELECT MAX(seq)) or null handling would not be caught. Add a test covering: session with events returns max seq, session with no events returns 0, unknown session returns 0. [fixable]
| getEventsAfter: (sessionId, afterSeq, limit) => | ||
| eventStore.getEventsAfter(sessionId, afterSeq, limit), | ||
| isSessionActive: (sessionId) => eventStore.getSession(sessionId)?.isActive ?? false, | ||
| shouldSync: (sessionId) => { |
There was a problem hiding this comment.
🔵 style: The shouldSync callback returns false when getSessionState returns null (session not in DB). This is likely correct, but worth a brief comment since the implicit null→false behavior is non-obvious and could silently skip sync for sessions whose state record was lost (e.g. after DB corruption). [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 7 issue(s) (2 warning).
server/index.ts
Solid defensive PR addressing three real failure modes (replay storms, noise-file rescues, interrupt crashes). The re-queue logic in SseConnection lacks backoff/retry-count enforcement on the doPost path which could cause spin under persistent server errors, but the bounded queue prevents memory issues. No critical bugs found.
- 🟡 bugs (L111): shouldSync returns false when getSessionState returns null (unknown/missing session). This means a session that hasn't been registered in the EventStore yet (e.g. race between watch and session creation) will be silently skipped by periodic sync. The previous isSessionActive had the same behavior (defaulted to false via
?? false), so this isn't a regression — but worth noting as a latent issue. Consider treating null state as sync-eligible.[fixable]
packages/client/src/sse-connection.ts
Solid defensive PR addressing three real failure modes (replay storms, noise-file rescues, interrupt crashes). The re-queue logic in SseConnection lacks backoff/retry-count enforcement on the doPost path which could cause spin under persistent server errors, but the bounded queue prevents memory issues. No critical bugs found.
- 🟡 unsafe_assumptions (L337): doPost re-queues failed sends into pendingSends, but flushPendingSends does not await doPost calls (line 357). If the flush fires and all POSTs fail (e.g. server is up but returns 500), messages silently accumulate back into pendingSends without any backoff or retry-count cap. The MAX_PENDING_SENDS=100 limit in send() does not apply to re-queued items pushed from doPost — only to items pushed via the public send() method — so a persistent server-side error could cause the same messages to cycle indefinitely through flush→fail→requeue.
[fixable]
packages/client/src/__tests__/sse-connection.test.ts
Solid defensive PR addressing three real failure modes (replay storms, noise-file rescues, interrupt crashes). The re-queue logic in SseConnection lacks backoff/retry-count enforcement on the doPost path which could cause spin under persistent server errors, but the bounded queue prevents memory issues. No critical bugs found.
- 🔵 missing_tests: No test verifies behavior when a re-queued send fails again on the second flush. This would exercise the flush→fail→requeue→flush cycle and confirm no infinite loop or unbounded growth.
[fixable]
packages/protocol/src/event-store.ts
Solid defensive PR addressing three real failure modes (replay storms, noise-file rescues, interrupt crashes). The re-queue logic in SseConnection lacks backoff/retry-count enforcement on the doPost path which could cause spin under persistent server errors, but the bounded queue prevents memory issues. No critical bugs found.
- 🔵 missing_tests (L618): getHeadSeq is a new public method on EventStore but has no unit test. Should verify: returns 0 for unknown session, returns correct max seq for a session with events, and returns correct value after new events are appended.
[fixable]
packages/harness/src/connection-registry.ts
Solid defensive PR addressing three real failure modes (replay storms, noise-file rescues, interrupt crashes). The re-queue logic in SseConnection lacks backoff/retry-count enforcement on the doPost path which could cause spin under persistent server errors, but the bounded queue prevents memory issues. No critical bugs found.
- 🔵 bugs (L242): The replay gap check
head - cursor > MAX_REPLAY_GAPperforms integer subtraction without clamping. If getHeadSeq returns 0 (no events) and cursor is 0, the gap is 0 which is fine. But if a buggy adapter returns a negative head, the subtraction could produce unexpected results. Minor — the real EventStore.getHeadSeq returnsrow?.head ?? 0so this can't happen in practice.[fixable]
server/worktree.ts
Solid defensive PR addressing three real failure modes (replay storms, noise-file rescues, interrupt crashes). The re-queue logic in SseConnection lacks backoff/retry-count enforcement on the doPost path which could cause spin under persistent server errors, but the bounded queue prevents memory issues. No critical bugs found.
- 🔵 style (L466): The noise-file check step is labeled '1b' in test comments but has no step comment in the actual code (unlike steps 1, 2, 3, 4 which have numbered comments). Adding a consistent step label would improve readability.
[fixable]
server/__tests__/rescue-worktree.test.ts
Solid defensive PR addressing three real failure modes (replay storms, noise-file rescues, interrupt crashes). The re-queue logic in SseConnection lacks backoff/retry-count enforcement on the doPost path which could cause spin under persistent server errors, but the bounded queue prevents memory issues. No critical bugs found.
- 🔵 missing_tests: No test covers the case where the
git diff --cached --name-onlycommand itself fails (the outer catch block at worktree.ts line 484). The comment says 'proceed with rescue anyway' — a test confirming this fallthrough behavior would be valuable.[fixable]
| const state = eventStore.getSessionState(sessionId); | ||
| // Only sync sessions that are actively running or starting. | ||
| // ENDED/SUSPENDED/DETACHED/CLOSING don't need delivery retries. | ||
| return state === 'ACTIVE' || state === 'STARTING' || state === 'CREATED'; |
There was a problem hiding this comment.
🟡 bugs: shouldSync returns false when getSessionState returns null (unknown/missing session). This means a session that hasn't been registered in the EventStore yet (e.g. race between watch and session creation) will be silently skipped by periodic sync. The previous isSessionActive had the same behavior (defaulted to false via ?? false), so this isn't a regression — but worth noting as a latent issue. Consider treating null state as sync-eligible. [fixable]
| body: JSON.stringify(body), | ||
| }); | ||
| if (!res.ok && endpoint === 'send') { | ||
| // Message delivery failed (e.g. 404 from stale connectionId). |
There was a problem hiding this comment.
🟡 unsafe_assumptions: doPost re-queues failed sends into pendingSends, but flushPendingSends does not await doPost calls (line 357). If the flush fires and all POSTs fail (e.g. server is up but returns 500), messages silently accumulate back into pendingSends without any backoff or retry-count cap. The MAX_PENDING_SENDS=100 limit in send() does not apply to re-queued items pushed from doPost — only to items pushed via the public send() method — so a persistent server-side error could cause the same messages to cycle indefinitely through flush→fail→requeue. [fixable]
| } | ||
|
|
||
| /** Return the highest seq number for a session, or 0 if no events exist. */ | ||
| getHeadSeq(sessionId: string): number { |
There was a problem hiding this comment.
🔵 missing_tests: getHeadSeq is a new public method on EventStore but has no unit test. Should verify: returns 0 for unknown session, returns correct max seq for a session with events, and returns correct value after new events are appended. [fixable]
| // Prevents replay storms after server restart or long disconnects. | ||
| if (this.eventStore.getHeadSeq) { | ||
| const head = this.eventStore.getHeadSeq(sessionId); | ||
| if (head - cursor > MAX_REPLAY_GAP) { |
There was a problem hiding this comment.
🔵 bugs: The replay gap check head - cursor > MAX_REPLAY_GAP performs integer subtraction without clamping. If getHeadSeq returns 0 (no events) and cursor is 0, the gap is 0 which is fine. But if a buggy adapter returns a negative head, the subtraction could produce unexpected results. Minor — the real EventStore.getHeadSeq returns row?.head ?? 0 so this can't happen in practice. [fixable]
| return { success: false, error: err instanceof Error ? err.message : String(err) }; | ||
| } | ||
|
|
||
| // 1b. Check if staged changes contain meaningful work (not just noise markers) |
There was a problem hiding this comment.
🔵 style: The noise-file check step is labeled '1b' in test comments but has no step comment in the actual code (unlike steps 1, 2, 3, 4 which have numbered comments). Adding a consistent step label would improve readability. [fixable]
Worktree rescue was creating draft PRs for sessions with no meaningful work — just the .mitzo-session marker file. Added a noise-file check after staging: if all staged files are in RESCUE_NOISE_FILES, bail out early instead of committing/pushing/creating a PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dimakis
force-pushed
the
session/2026-06-25-90eaf3eb17db
branch
from
9d9d8c0 to
1858d72
Compare
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
The noise-file filter correctly prevents useless rescue PRs, but callers still treat the result as a rescue failure — noise-only worktrees will accumulate on disk because stale GC re-encounters them every cycle.
- 🟡 bugs (L489): When rescue returns
{ success: false, error: 'no meaningful changes to rescue' }, both callers (cleanupStaleWorktreesat line ~682 andcleanupSessionWorktreesin chat.ts at line ~1319) treat this identically to a real rescue failure: the worktree is preserved on disk, and in the stale-GC path an inbox notification is posted. SincehasUncommittedWorkstill detects.mitzo-sessionas dirty (it's not in.gitignore), stale GC will re-encounter the same worktree every cycle, re-attempt rescue, get the same 'no meaningful changes' result, and preserve it indefinitely. Noise-only worktrees will accumulate on disk. Consider either (a) returning a distinct signal (e.g.{ success: false, noMeaningfulChanges: true }) so callers can safely remove the worktree, or (b) filtering noise files inhasUncommittedWorkso the worktree is seen as clean.[fixable]
server/__tests__/rescue-worktree.test.ts
The noise-file filter correctly prevents useless rescue PRs, but callers still treat the result as a rescue failure — noise-only worktrees will accumulate on disk because stale GC re-encounters them every cycle.
- 🔵 style (L277): Three new test cases have comments saying 'Step 1: git add -A' (lines 277, 296, 317) but the implementation at worktree.ts:468 uses
git add -u. The distinction matters (tracked-only vs all files). Inaccurate test comments can mislead future maintainers about what the code actually does.[fixable] - 🔵 missing_tests: No test covers the outer
catchat worktree.ts:491 wheregit diff --cached --name-onlyitself throws. The code deliberately proceeds with rescue in that case ('safer than skipping'), but this fail-open behavior is untested.[fixable]
| } catch { | ||
| // Non-fatal — worktree is about to be removed anyway | ||
| } | ||
| return { success: false, error: 'no meaningful changes to rescue' }; |
There was a problem hiding this comment.
🟡 bugs: When rescue returns { success: false, error: 'no meaningful changes to rescue' }, both callers (cleanupStaleWorktrees at line ~682 and cleanupSessionWorktrees in chat.ts at line ~1319) treat this identically to a real rescue failure: the worktree is preserved on disk, and in the stale-GC path an inbox notification is posted. Since hasUncommittedWork still detects .mitzo-session as dirty (it's not in .gitignore), stale GC will re-encounter the same worktree every cycle, re-attempt rescue, get the same 'no meaningful changes' result, and preserve it indefinitely. Noise-only worktrees will accumulate on disk. Consider either (a) returning a distinct signal (e.g. { success: false, noMeaningfulChanges: true }) so callers can safely remove the worktree, or (b) filtering noise files in hasUncommittedWork so the worktree is seen as clean. [fixable]
| it('skips rescue when only noise files (.mitzo-session) are staged', () => { | ||
| // Step 0: getRepoRemote | ||
| mockExecFileSync.mockReturnValueOnce('git@github.com:dimakis/mgmt.git\n' as never); | ||
| // Step 1: git add -A |
There was a problem hiding this comment.
🔵 style: Three new test cases have comments saying 'Step 1: git add -A' (lines 277, 296, 317) but the implementation at worktree.ts:468 uses git add -u. The distinction matters (tracked-only vs all files). Inaccurate test comments can mislead future maintainers about what the code actually does. [fixable]
1 participant
Summary
.mitzo-sessionmarker file. This caused 15+ empty "Rescued" PRs to pile up in mgmt.RESCUE_NOISE_FILES(currently just.mitzo-session), the rescue bails out early — no commit, no push, no PR.Test plan
.mitzo-sessionis staged🤖 Generated with Claude Code