diff --git a/packages/cli/src/commands/isolation.test.ts b/packages/cli/src/commands/isolation.test.ts index 81ca60651e..0a399fb12a 100644 --- a/packages/cli/src/commands/isolation.test.ts +++ b/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', () => { diff --git a/packages/cli/src/commands/isolation.ts b/packages/cli/src/commands/isolation.ts index 6e44a0fb67..a24855486a 100644 --- a/packages/cli/src/commands/isolation.ts +++ b/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'); diff --git a/packages/core/src/services/cleanup-service.test.ts b/packages/core/src/services/cleanup-service.test.ts index 3d1b204d35..8b17e700c7 100644 --- a/packages/core/src/services/cleanup-service.test.ts +++ b/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'; diff --git a/packages/core/src/services/cleanup-service.ts b/packages/core/src/services/cleanup-service.ts index 50d9da0d2a..2ee21a1f06 100644 --- a/packages/core/src/services/cleanup-service.ts +++ b/packages/core/src/services/cleanup-service.ts @@ -128,22 +128,42 @@ export interface RemoveEnvironmentOptions { deleteRemoteBranch?: boolean; } +/** + * Result from removeEnvironment indicating what actually happened + */ +export interface RemoveEnvironmentResult { + /** Whether the worktree was removed from disk */ + worktreeRemoved: boolean; + /** Whether the branch was deleted (null if branch cleanup was not attempted) */ + branchDeleted: boolean | null; + /** If the operation was a no-op, why it was skipped */ + skippedReason?: string; + /** Warnings from partial cleanup (e.g., branch couldn't be deleted) */ + warnings: string[]; +} + /** * Remove a specific environment */ export async function removeEnvironment( envId: string, options?: RemoveEnvironmentOptions -): Promise { +): Promise { + const noopResult: RemoveEnvironmentResult = { + worktreeRemoved: false, + branchDeleted: false, + warnings: [], + }; + const env = await isolationEnvDb.getById(envId); if (!env) { getLog().debug({ envId }, 'env_not_found'); - return; + return { ...noopResult, skippedReason: 'environment not found' }; } if (env.status === 'destroyed') { getLog().debug({ envId }, 'env_already_destroyed'); - return; + return { ...noopResult, skippedReason: 'already destroyed' }; } // Get canonical repo path from codebase for branch cleanup @@ -164,7 +184,7 @@ export async function removeEnvironment( const hasChanges = await hasUncommittedChanges(toWorktreePath(env.working_path)); if (hasChanges) { getLog().warn({ envId, workingPath: env.working_path }, 'env_has_uncommitted_changes'); - return; + return { ...noopResult, skippedReason: 'has uncommitted changes' }; } } @@ -186,6 +206,12 @@ export async function removeEnvironment( await isolationEnvDb.updateStatus(envId, 'destroyed'); getLog().info({ envId, workingPath: env.working_path }, 'env_removed'); + + return { + worktreeRemoved: destroyResult.worktreeRemoved, + branchDeleted: destroyResult.branchDeleted, + warnings: destroyResult.warnings, + }; } catch (error) { const err = error as Error & { code?: string; stderr?: string }; const errorText = `${err.message} ${err.stderr ?? ''}`; @@ -202,7 +228,7 @@ export async function removeEnvironment( if (isPathNotFoundError) { await isolationEnvDb.updateStatus(envId, 'destroyed'); getLog().info({ envId }, 'env_removed_externally'); - return; + return { worktreeRemoved: true, branchDeleted: false, warnings: [] }; } getLog().error({ err, envId }, 'env_remove_failed'); @@ -271,8 +297,12 @@ export async function runScheduledCleanup(): Promise { const pathExists = await worktreeExists(toWorktreePath(env.working_path)); if (!pathExists) { // Path doesn't exist - call removeEnvironment to clean up branch and mark as destroyed - await removeEnvironment(env.id, { force: false }); - report.removed.push(`${env.id} (path missing)`); + const removeResult = await removeEnvironment(env.id, { force: false }); + if (removeResult.skippedReason) { + report.skipped.push({ id: env.id, reason: removeResult.skippedReason }); + } else { + report.removed.push(`${env.id} (path missing)`); + } continue; } @@ -301,8 +331,15 @@ export async function runScheduledCleanup(): Promise { } // Safe to remove merged branch (also delete remote branch) - await removeEnvironment(env.id, { force: false, deleteRemoteBranch: true }); - report.removed.push(`${env.id} (merged)`); + const mergedResult = await removeEnvironment(env.id, { + force: false, + deleteRemoteBranch: true, + }); + if (mergedResult.skippedReason) { + report.skipped.push({ id: env.id, reason: mergedResult.skippedReason }); + } else { + report.removed.push(`${env.id} (merged)`); + } continue; } @@ -328,8 +365,12 @@ export async function runScheduledCleanup(): Promise { continue; } - await removeEnvironment(env.id, { force: false }); - report.removed.push(`${env.id} (stale)`); + const staleResult = await removeEnvironment(env.id, { force: false }); + if (staleResult.skippedReason) { + report.skipped.push({ id: env.id, reason: staleResult.skippedReason }); + } else { + report.removed.push(`${env.id} (stale)`); + } } } catch (error) { const err = error as Error; @@ -490,8 +531,12 @@ export async function cleanupStaleWorktrees( // Safe to remove try { - await removeEnvironment(env.id); - result.removed.push(env.branch_name); + const removeResult = await removeEnvironment(env.id); + if (removeResult.skippedReason) { + result.skipped.push({ branchName: env.branch_name, reason: removeResult.skippedReason }); + } else { + result.removed.push(env.branch_name); + } } catch (error) { const err = error as Error; result.skipped.push({ branchName: env.branch_name, reason: err.message }); @@ -591,8 +636,12 @@ export async function cleanupMergedWorktrees( // Safe to remove (also delete remote branch since it's merged) try { - await removeEnvironment(env.id, { deleteRemoteBranch: true }); - result.removed.push(env.branch_name); + const removeResult = await removeEnvironment(env.id, { deleteRemoteBranch: true }); + if (removeResult.skippedReason) { + result.skipped.push({ branchName: env.branch_name, reason: removeResult.skippedReason }); + } else { + result.removed.push(env.branch_name); + } } catch (error) { const err = error as Error; result.skipped.push({ branchName: env.branch_name, reason: err.message }); diff --git a/packages/isolation/src/providers/worktree.ts b/packages/isolation/src/providers/worktree.ts index 326cafc9c8..528a63774f 100644 --- a/packages/isolation/src/providers/worktree.ts +++ b/packages/isolation/src/providers/worktree.ts @@ -180,6 +180,26 @@ export class WorktreeProvider implements IIsolationProvider { } } + // Prune stale worktree references — runs even when path is already gone, + // because git may still have a stale ref for a manually-deleted worktree + try { + await execFileAsync('git', ['-C', repoPath, 'worktree', 'prune'], { timeout: 15000 }); + } catch (_error) { + // Best-effort — pruning failure is not critical + getLog().debug({ repoPath }, 'worktree_prune_failed'); + } + + // Post-removal verification: confirm worktree is actually gone from git + if (result.worktreeRemoved) { + const stillRegistered = await this.isWorktreeRegistered(repoPath, worktreePath); + if (stillRegistered) { + result.worktreeRemoved = false; + const warning = `Worktree at ${worktreePath} was reported removed but is still registered in git`; + getLog().warn({ worktreePath, repoPath }, 'worktree_removal_verification_failed'); + result.warnings.push(warning); + } + } + // Delete associated branch if provided (best-effort cleanup) if (options?.branchName) { result.branchDeleted = await this.deleteBranchTracked(repoPath, options.branchName, result); @@ -211,6 +231,30 @@ export class WorktreeProvider implements IIsolationProvider { ); } + /** + * Check if a worktree path is still registered in `git worktree list`. + * Used for post-removal verification. + */ + private async isWorktreeRegistered(repoPath: string, worktreePath: string): Promise { + try { + const { stdout } = await execFileAsync( + 'git', + ['-C', repoPath, 'worktree', 'list', '--porcelain'], + { timeout: 15000 } + ); + // Porcelain output has "worktree " lines with resolved absolute paths + const normalizedTarget = resolve(worktreePath); + return stdout.split('\n').some(line => { + if (!line.startsWith('worktree ')) return false; + const listed = line.slice('worktree '.length).trim(); + return resolve(listed) === normalizedTarget; + }); + } catch (_error) { + // If we can't verify, assume it's gone (don't block on verification failure) + return false; + } + } + /** * Delete a branch and track the result. Never throws - branch deletion is best-effort. * Returns true if branch was deleted or already gone, false if deletion failed.