Skip to content

fix(worktree): skip rescue for noise-only worktrees #397

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dimakis merged 1 commit into from
Jun 25, 2026
Merged

fix(worktree): skip rescue for noise-only worktrees #397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 67 additions & 5 deletions server/__tests__/rescue-worktree.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ describe('rescueDirtyWorktree', () => {
mockExecFileSync.mockReturnValueOnce('git@github.com:dimakis/mgmt.git\n' as never);
// Step 1: git add -u
mockExecFileSync.mockReturnValueOnce('' as never);
// Step 1b: git diff --cached --name-only → meaningful file
mockExecFileSync.mockReturnValueOnce('src/feature.ts\n' as never);
// Step 2: git commit
mockExecFileSync.mockReturnValueOnce('' as never);
// Step 3: git push
Expand Down Expand Up @@ -186,6 +188,8 @@ describe('rescueDirtyWorktree', () => {
mockExecFileSync.mockReturnValueOnce('git@github.com:dimakis/mgmt.git\n' as never);
// Step 1: git add -u succeeds
mockExecFileSync.mockReturnValueOnce('' as never);
// Step 1b: git diff --cached --name-only → meaningful file
mockExecFileSync.mockReturnValueOnce('src/feature.ts\n' as never);
// Step 2: git commit fails
mockExecFileSync.mockImplementationOnce(() => {
throw new Error('nothing to commit');
Expand All @@ -202,6 +206,8 @@ describe('rescueDirtyWorktree', () => {
mockExecFileSync.mockReturnValueOnce('git@github.com:dimakis/mgmt.git\n' as never);
// Step 1: git add -u
mockExecFileSync.mockReturnValueOnce('' as never);
// Step 1b: git diff --cached --name-only → meaningful file
mockExecFileSync.mockReturnValueOnce('src/feature.ts\n' as never);
// Step 2: git commit
mockExecFileSync.mockReturnValueOnce('' as never);
// Step 3: git push fails
Expand All @@ -220,6 +226,8 @@ describe('rescueDirtyWorktree', () => {
mockExecFileSync.mockReturnValueOnce('git@github.com:dimakis/mgmt.git\n' as never);
// Step 1: git add
mockExecFileSync.mockReturnValueOnce('' as never);
// Step 1b: git diff --cached --name-only → meaningful file
mockExecFileSync.mockReturnValueOnce('src/feature.ts\n' as never);
// Step 2: git commit
mockExecFileSync.mockReturnValueOnce('' as never);
// Step 3: git push
Expand Down Expand Up @@ -252,14 +260,68 @@ describe('rescueDirtyWorktree', () => {
mockExecFileSync.mockReturnValueOnce('git@github.com:dimakis/mgmt.git\n' as never);
// Step 1: git add -u succeeds (but stages nothing — only untracked files)
mockExecFileSync.mockReturnValueOnce('' as never);
// Step 2: git commit fails — nothing staged
mockExecFileSync.mockImplementationOnce(() => {
throw new Error('nothing to commit, working tree clean');
});
// Step 1b: git diff --cached --name-only → empty (nothing staged)
mockExecFileSync.mockReturnValueOnce('' as never);
// Step 1b: git reset HEAD (unstage)
mockExecFileSync.mockReturnValueOnce('' as never);

const result = rescueDirtyWorktree('/tmp/worktree', 'session/test-123', 'test-123');

expect(result.success).toBe(false);
expect(result.error).toContain('nothing to commit');
expect(result.error).toContain('no meaningful changes');
});

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

@dimakis dimakis Jun 25, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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]

mockExecFileSync.mockReturnValueOnce('' as never);
// Step 1b: git diff --cached --name-only → only .mitzo-session
mockExecFileSync.mockReturnValueOnce('.mitzo-session\n' as never);
// Step 1b: git reset HEAD (unstage)
mockExecFileSync.mockReturnValueOnce('' as never);

const result = rescueDirtyWorktree('/tmp/worktree', 'session/test-123', 'test-123');

expect(result.success).toBe(false);
expect(result.error).toContain('no meaningful changes');
// Should NOT have called commit, push, or pr create
expect(mockExecFileSync).toHaveBeenCalledTimes(4); // remote + add + diff + reset
});

it('proceeds with rescue when meaningful files are staged alongside noise files', () => {
// Step 0: getRepoRemote
mockExecFileSync.mockReturnValueOnce('git@github.com:dimakis/mgmt.git\n' as never);
// Step 1: git add -A
mockExecFileSync.mockReturnValueOnce('' as never);
// Step 1b: git diff --cached --name-only → noise + real file
mockExecFileSync.mockReturnValueOnce('.mitzo-session\nsrc/feature.ts\n' as never);
// Step 2: git commit
mockExecFileSync.mockReturnValueOnce('' as never);
// Step 3: git push
mockExecFileSync.mockReturnValueOnce('' as never);
// Step 4: gh pr create
mockExecFileSync.mockReturnValueOnce('https://github.com/dimakis/mgmt/pull/99\n' as never);

const result = rescueDirtyWorktree('/tmp/worktree', 'session/test-123', 'test-123');

expect(result.success).toBe(true);
expect(result.prUrl).toBe('https://github.com/dimakis/mgmt/pull/99');
});

it('skips rescue when no files are staged', () => {
// Step 0: getRepoRemote
mockExecFileSync.mockReturnValueOnce('git@github.com:dimakis/mgmt.git\n' as never);
// Step 1: git add -A
mockExecFileSync.mockReturnValueOnce('' as never);
// Step 1b: git diff --cached --name-only → empty
mockExecFileSync.mockReturnValueOnce('' as never);
// Step 1b: git reset HEAD (unstage)
mockExecFileSync.mockReturnValueOnce('' as never);

const result = rescueDirtyWorktree('/tmp/worktree', 'session/test-123', 'test-123');

expect(result.success).toBe(false);
expect(result.error).toContain('no meaningful changes');
});
});
29 changes: 29 additions & 0 deletions server/worktree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,13 @@ import { createLogger } from './logger.js';

const log = createLogger('worktree');

/**
* Files that are not meaningful work — rescue should skip if these are the only changes.
* Matched via exact path from `git diff --cached --name-only` (root-relative).
* Only root-level files are supported; subdirectory paths would need `subdir/file` entries.
*/
const RESCUE_NOISE_FILES = new Set(['.mitzo-session']);

/**
* Detect the default branch of a repo (e.g. 'main' or 'master').
* Prefers origin/HEAD (remote truth) since session worktrees should branch
Expand Down Expand Up @@ -463,6 +470,28 @@ export function rescueDirtyWorktree(
return { success: false, error: err instanceof Error ? err.message : String(err) };
}

// 1b. Check if staged changes contain meaningful work (not just noise markers)

@dimakis dimakis Jun 25, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 dimakis Jun 25, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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]

try {
const staged = execFileSync('git', ['-C', worktreePath, 'diff', '--cached', '--name-only'], {
encoding: 'utf-8',
stdio: ['pipe', 'pipe', 'pipe'],
timeout: WORKTREE_GIT_TIMEOUT_MS,
}).trim();
const files = staged ? staged.split('\n') : [];
if (files.length === 0 || files.every((f) => RESCUE_NOISE_FILES.has(f))) {

@dimakis dimakis Jun 25, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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]

log.info('rescue skipped — only noise files changed', { wtId, files });
// Unstage so the worktree can be cleaned up without leaving staged changes
try {
execFileSync('git', ['-C', worktreePath, 'reset', 'HEAD'], gitOpts);
} catch {
// Non-fatal — worktree is about to be removed anyway
}
return { success: false, error: 'no meaningful changes to rescue' };

@dimakis dimakis Jun 25, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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]

}
} catch {
// If we can't inspect staged files, proceed with the rescue anyway — safer than skipping
}

try {
// 2. Commit
execFileSync(
Expand Down
Loading