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

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

Three changes to prevent ghost worktrees:

1. isolationCompleteCommand now checks result.worktreeRemoved — if the
   worktree was not actually removed (partial failure), it reports
   'Partial' with warnings and counts as failed, not completed.
   Previously only skippedReason was checked; a destroy that returned
   successfully but with worktreeRemoved=false would still print
   'Completed'.

2. WorktreeProvider.destroy() now runs 'git worktree prune' after
   removal to clean up stale worktree references that git may keep
   even after the directory is removed.

3. WorktreeProvider.destroy() adds post-removal verification: after
   git worktree remove, it checks 'git worktree list --porcelain' to
   confirm the worktree is actually unregistered. If still registered,
   worktreeRemoved is set back to false with a descriptive warning.

* fix: address CodeRabbit review — ghost worktree prune, partial cleanup callers, accurate messages

* test: add regression test for Partial branch in isolation complete

Exercises the !result.worktreeRemoved path (without skippedReason)
that was flagged as uncovered by CodeRabbit review.

Author: kagura-agent

packages/cli/src/commands/isolation.test.ts

@@ -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', () => ({
@@ -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 });
 
@@ -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' });
@@ -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 });
 
@@ -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'], {
@@ -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');
+  });
 });
 
 describe('isolationCleanupMergedCommand', () => {

packages/cli/src/commands/isolation.ts

@@ -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,
@@ -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(', ')})`
+        );
+        for (const warning of result.warnings) {
+          console.error(`    ⚠ ${warning}`);
+        }
+        failed++;
+      } else {
+        console.log(`  Completed: ${branch}`);
+        completed++;
+      }
     } catch (error) {
       const err = error as Error;
       getLog().warn({ err, branch, envId: env.id }, 'isolation.complete_failed');

packages/core/src/services/cleanup-service.test.ts

@@ -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', {
@@ -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 () => {
@@ -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';