Skip to content

fix(isolation): complete reports false success when worktree remains on disk (fixes #964) #1034

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
Wirasm merged 3 commits into from
Apr 14, 2026
Merged

fix(isolation): complete reports false success when worktree remains on disk (fixes #964) #1034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2c241dc
fix(isolation): complete reports false success when worktree remains …
kagura-agent Apr 10, 2026
3e81262
fix: address CodeRabbit review — ghost worktree prune, partial cleanu…
kagura-agent Apr 11, 2026
548870f
test: add regression test for Partial branch in isolation complete
kagura-agent Apr 11, 2026
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
77 changes: 72 additions & 5 deletions packages/cli/src/commands/isolation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ mock.module('@archon/core/db/workflows', () => ({
getActiveWorkflowRunByPath: mockGetActiveWorkflowRunByPath,
}));

const mockRemoveEnvironment = mock(() => Promise.resolve());
const mockRemoveEnvironment = mock(() =>
Promise.resolve({ worktreeRemoved: true, branchDeleted: true, warnings: [] })
);
const mockCleanupMergedWorktrees = mock(() => Promise.resolve({ removed: [], skipped: [] }));

mock.module('@archon/core/services/cleanup-service', () => ({
Expand Down Expand Up @@ -136,7 +138,11 @@ describe('isolationCompleteCommand', () => {

it('completes a branch when env is found and all checks pass', async () => {
mockFindActiveByBranchName.mockResolvedValueOnce(mockEnv);
mockRemoveEnvironment.mockResolvedValueOnce(undefined);
mockRemoveEnvironment.mockResolvedValueOnce({
worktreeRemoved: true,
branchDeleted: true,
warnings: [],
});

await isolationCompleteCommand(['feature-branch'], { force: false, deleteRemote: true });

Expand Down Expand Up @@ -309,7 +315,11 @@ describe('isolationCompleteCommand', () => {

it('skips PR check with warning when gh CLI is not available', async () => {
mockFindActiveByBranchName.mockResolvedValueOnce(mockEnv);
mockRemoveEnvironment.mockResolvedValueOnce(undefined);
mockRemoveEnvironment.mockResolvedValueOnce({
worktreeRemoved: true,
branchDeleted: true,
warnings: [],
});
mockExecFileAsync.mockImplementation((cmd: string) => {
if (cmd === 'gh') {
const err = Object.assign(new Error('spawn gh ENOENT'), { code: 'ENOENT' });
Expand All @@ -335,7 +345,11 @@ describe('isolationCompleteCommand', () => {
id: 'run-abc',
workflow_name: 'implement',
});
mockRemoveEnvironment.mockResolvedValueOnce(undefined);
mockRemoveEnvironment.mockResolvedValueOnce({
worktreeRemoved: true,
branchDeleted: true,
warnings: [],
});

await isolationCompleteCommand(['dirty-branch'], { force: true, deleteRemote: true });

Expand Down Expand Up @@ -368,7 +382,7 @@ describe('isolationCompleteCommand', () => {
.mockResolvedValueOnce(null) // not found: branch-2
.mockResolvedValueOnce(mockEnv); // found: branch-3 (will fail)
mockRemoveEnvironment
.mockResolvedValueOnce(undefined) // branch-1 succeeds
.mockResolvedValueOnce({ worktreeRemoved: true, branchDeleted: true, warnings: [] }) // branch-1 succeeds
.mockRejectedValueOnce(new Error('some error')); // branch-3 fails

await isolationCompleteCommand(['branch-1', 'branch-2', 'branch-3'], {
Expand All @@ -378,6 +392,59 @@ describe('isolationCompleteCommand', () => {

expect(consoleLogSpy).toHaveBeenCalledWith('\nComplete: 1 completed, 1 failed, 1 not found');
});
it('counts as failed when removeEnvironment returns skippedReason (ghost worktree)', async () => {
mockFindActiveByBranchName.mockResolvedValueOnce(mockEnv);
mockRemoveEnvironment.mockResolvedValueOnce({
worktreeRemoved: false,
branchDeleted: false,
skippedReason: 'has uncommitted changes',
warnings: [],
});

await isolationCompleteCommand(['ghost-branch'], { force: true, deleteRemote: true });

expect(consoleErrorSpy).toHaveBeenCalledWith(
' Blocked: ghost-branch — has uncommitted changes'
);
expect(consoleErrorSpy).toHaveBeenCalledWith(' Use --force to override.');
expect(consoleLogSpy).toHaveBeenCalledWith('\nComplete: 0 completed, 1 failed, 0 not found');
});

it('counts as failed when removeEnvironment returns partial (worktree not removed, branch deleted)', async () => {
mockFindActiveByBranchName.mockResolvedValueOnce(mockEnv);
mockRemoveEnvironment.mockResolvedValueOnce({
worktreeRemoved: false,
branchDeleted: true,
warnings: ['Some warning'],
skippedReason: undefined,
});

await isolationCompleteCommand(['partial-branch'], { force: true, deleteRemote: true });

expect(consoleErrorSpy).toHaveBeenCalledWith(
' Partial: partial-branch — worktree was not removed from disk (branch deleted, DB updated)'
);
expect(consoleErrorSpy).toHaveBeenCalledWith(' ⚠ Some warning');
expect(consoleLogSpy).toHaveBeenCalledWith('\nComplete: 0 completed, 1 failed, 0 not found');
});

it('surfaces warnings from removeEnvironment result', async () => {
mockFindActiveByBranchName.mockResolvedValueOnce(mockEnv);
mockRemoveEnvironment.mockResolvedValueOnce({
worktreeRemoved: true,
branchDeleted: false,
warnings: ["Cannot delete branch 'feature-branch': checked out elsewhere"],
});

await isolationCompleteCommand(['feature-branch'], { force: true, deleteRemote: true });

expect(consoleWarnSpy).toHaveBeenCalledWith(
" Warning: Cannot delete branch 'feature-branch': checked out elsewhere"
);
// Should still count as completed since worktree was removed
expect(consoleLogSpy).toHaveBeenCalledWith(' Completed: feature-branch');
expect(consoleLogSpy).toHaveBeenCalledWith('\nComplete: 1 completed, 0 failed, 0 not found');
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});

describe('isolationCleanupMergedCommand', () => {
Expand Down
36 changes: 32 additions & 4 deletions packages/cli/src/commands/isolation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ import {
getDefaultBranch,
} from '@archon/git';
import { getIsolationProvider } from '@archon/isolation';
import { removeEnvironment } from '@archon/core/services/cleanup-service';
import {
removeEnvironment,
type RemoveEnvironmentResult,
} from '@archon/core/services/cleanup-service';
import {
listEnvironments,
cleanupMergedEnvironments,
Expand Down Expand Up @@ -298,12 +301,37 @@ export async function isolationCompleteCommand(
}

try {
await removeEnvironment(env.id, {
const result: RemoveEnvironmentResult = await removeEnvironment(env.id, {
force: options.force,
deleteRemoteBranch: options.deleteRemote ?? true,
});
console.log(` Completed: ${branch}`);
completed++;

// Surface warnings from partial cleanup
for (const warning of result.warnings) {
console.warn(` Warning: ${warning}`);
}

if (result.skippedReason) {
console.error(` Blocked: ${branch} — ${result.skippedReason}`);
if (result.skippedReason === 'has uncommitted changes') {
console.error(' Use --force to override.');
}
failed++;
} else if (!result.worktreeRemoved) {
const parts: string[] = [];
if (result.branchDeleted) parts.push('branch deleted');
parts.push('DB updated');
console.error(
` Partial: ${branch} — worktree was not removed from disk (${parts.join(', ')})`
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
for (const warning of result.warnings) {
console.error(` ⚠ ${warning}`);
}
Comment on lines +309 to +329
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Duplicate warning output in partial cleanup path.

When worktreeRemoved is false, warnings are printed twice:

  1. Lines 310-312: console.warn( Warning: ${warning}) for all warnings
  2. Lines 327-329: console.error( ⚠ ${warning}) for all warnings

This results in duplicate output for the same warnings with different formatting.

🔧 Suggested fix: Move the warning loop inside the else branches 
     try {
       const result: RemoveEnvironmentResult = await removeEnvironment(env.id, {
         force: options.force,
         deleteRemoteBranch: options.deleteRemote ?? true,
       });

-      // Surface warnings from partial cleanup
-      for (const warning of result.warnings) {
-        console.warn(`  Warning: ${warning}`);
-      }
-
       if (result.skippedReason) {
         console.error(`  Blocked: ${branch} — ${result.skippedReason}`);
         if (result.skippedReason === 'has uncommitted changes') {
           console.error('    Use --force to override.');
         }
         failed++;
       } else if (!result.worktreeRemoved) {
         const parts: string[] = [];
         if (result.branchDeleted) parts.push('branch deleted');
         parts.push('DB updated');
         console.error(
           `  Partial: ${branch} — worktree was not removed from disk (${parts.join(', ')})`
         );
         for (const warning of result.warnings) {
           console.error(`    ⚠ ${warning}`);
         }
         failed++;
       } else {
         console.log(`  Completed: ${branch}`);
+        // Surface warnings from partial cleanup (e.g., branch couldn't be deleted)
+        for (const warning of result.warnings) {
+          console.warn(`  Warning: ${warning}`);
+        }
         completed++;
       }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/commands/isolation.ts` around lines 309 - 329, The duplicate
warning output comes from printing result.warnings both unconditionally at the
top and again inside the partial cleanup branch; remove the initial
unconditional loop over result.warnings and instead print warnings only inside
the relevant branches (when result.skippedReason is set and when
!result.worktreeRemoved). Concretely, delete the top-level for (const warning of
result.warnings) console.warn block and add a single warning loop inside the
skippedReason branch (alongside the "Blocked" message) and reuse the existing
loop inside the !result.worktreeRemoved branch so each warning is emitted once
and in the appropriate format; keep references to result.warnings,
result.skippedReason, result.worktreeRemoved, branchDeleted and branch to
preserve current messages and formatting.

failed++;
} else {
console.log(` Completed: ${branch}`);
completed++;
}
} catch (error) {
const err = error as Error;
getLog().warn({ err, branch, envId: env.id }, 'isolation.complete_failed');
Expand Down
85 changes: 84 additions & 1 deletion packages/core/src/services/cleanup-service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ describe('cleanup-service', () => {

// worktreeExists returns false (default)

await removeEnvironment(envId);
const result = await removeEnvironment(envId);

// Should call destroy with branchName and canonicalRepoPath for cleanup
expect(mockDestroy).toHaveBeenCalledWith('/path/that/does/not/exist', {
Expand All @@ -163,6 +163,9 @@ describe('cleanup-service', () => {
});
// Should mark as destroyed
expect(mockUpdateStatus).toHaveBeenCalledWith(envId, 'destroyed');
// Should return success result
expect(result.worktreeRemoved).toBe(true);
expect(result.skippedReason).toBeUndefined();
});

test('handles git worktree remove failure for missing path', async () => {
Expand Down Expand Up @@ -316,6 +319,86 @@ describe('cleanup-service', () => {
});
});

test('returns skippedReason when worktree has uncommitted changes without force', async () => {
const envId = 'env-uncommitted';

mockGetById.mockResolvedValueOnce({
id: envId,
codebase_id: 'codebase-123',
workflow_type: 'issue',
workflow_id: '42',
provider: 'worktree',
working_path: '/workspace/worktrees/issue-42',
branch_name: 'issue-42',
status: 'active',
created_at: new Date(),
created_by_platform: 'github',
metadata: {},
});

mockGetCodebase.mockResolvedValueOnce({
id: 'codebase-123',
name: 'test-repo',
default_cwd: '/workspace/repo',
});

// worktreeExists returns true (path exists)
mockWorktreeExists.mockResolvedValueOnce(true);
// hasUncommittedChanges returns true
mockHasUncommittedChanges.mockResolvedValueOnce(true);

const result = await removeEnvironment(envId);

// Should NOT call destroy or mark as destroyed
expect(mockDestroy).not.toHaveBeenCalled();
expect(mockUpdateStatus).not.toHaveBeenCalled();
// Should return skipped result
expect(result.worktreeRemoved).toBe(false);
expect(result.branchDeleted).toBe(false);
expect(result.skippedReason).toBe('has uncommitted changes');
});

test('returns warnings from partial destroy', async () => {
const envId = 'env-partial';

mockGetById.mockResolvedValueOnce({
id: envId,
codebase_id: 'codebase-123',
workflow_type: 'issue',
workflow_id: '42',
provider: 'worktree',
working_path: '/workspace/worktrees/issue-42',
branch_name: 'issue-42',
status: 'active',
created_at: new Date(),
created_by_platform: 'github',
metadata: {},
});

mockGetCodebase.mockResolvedValueOnce({
id: 'codebase-123',
name: 'test-repo',
default_cwd: '/workspace/repo',
});

// worktreeExists returns false (default)

mockDestroy.mockResolvedValueOnce({
worktreeRemoved: true,
branchDeleted: false,
remoteBranchDeleted: null,
directoryClean: true,
warnings: ["Cannot delete branch 'issue-42': checked out elsewhere"],
});

const result = await removeEnvironment(envId);

expect(result.worktreeRemoved).toBe(true);
expect(result.branchDeleted).toBe(false);
expect(result.warnings).toEqual(["Cannot delete branch 'issue-42': checked out elsewhere"]);
expect(result.skippedReason).toBeUndefined();
});

test('re-throws non-directory errors from provider.destroy', async () => {
const envId = 'env-real-error';

Expand Down
Loading