diff --git a/.gitignore b/.gitignore index e0acc0dc5b..64670c18d5 100644 --- a/.gitignore +++ b/.gitignore @@ -61,6 +61,7 @@ lerna-debug.log* .auto-claude-status .claude_settings.json .update-metadata.json +tmpclaude-* # =========================== # Python (apps/backend) diff --git a/apps/backend/core/worktree.py b/apps/backend/core/worktree.py index a94964ef7f..5fc1183e22 100644 --- a/apps/backend/core/worktree.py +++ b/apps/backend/core/worktree.py @@ -31,6 +31,11 @@ T = TypeVar("T") +# Base branches that should be filtered out when listing worktrees +# These indicate orphaned worktrees (e.g., after GitHub PR merge) +# NOTE: Keep in sync with BASE_BRANCHES in apps/frontend/src/shared/constants/git.ts +BASE_BRANCHES = frozenset({"main", "master", "develop", "head"}) + def _is_retryable_network_error(stderr: str) -> bool: """Check if an error is a retryable network/connection issue.""" @@ -647,29 +652,10 @@ def merge_worktree( result = self._run_git(merge_args) if result.returncode != 0: - # Check if it's "already up to date" - not an error - output = (result.stdout + result.stderr).lower() - if "already up to date" in output or "already up-to-date" in output: - print(f"Branch {info.branch} is already up to date.") - if no_commit: - print("No changes to stage.") - if delete_after: - self.remove_worktree(spec_name, delete_branch=True) - return True - # Check for actual conflicts - if "conflict" in output: - print("Merge conflict! Aborting merge...") - self._run_git(["merge", "--abort"]) - return False - # Other error - show details - stderr_msg = ( - result.stderr[:200] - if result.stderr - else result.stdout[:200] - if result.stdout - else "" + error_detail = ( + result.stderr.strip() or result.stdout.strip() or "Unknown error" ) - print(f"Merge failed: {stderr_msg}") + print(f"Merge failed: {error_detail}") self._run_git(["merge", "--abort"]) return False @@ -719,7 +705,8 @@ def list_all_worktrees(self) -> list[WorktreeInfo]: for item in self.worktrees_dir.iterdir(): if item.is_dir(): info = self.get_worktree_info(item.name) - if info: + # Skip worktrees on base branches (orphaned after GitHub merge) + if info and info.branch.lower() not in BASE_BRANCHES: worktrees.append(info) seen_specs.add(item.name) @@ -729,7 +716,8 @@ def list_all_worktrees(self) -> list[WorktreeInfo]: for item in legacy_dir.iterdir(): if item.is_dir() and item.name not in seen_specs: info = self.get_worktree_info(item.name) - if info: + # Skip worktrees on base branches (orphaned after GitHub merge) + if info and info.branch.lower() not in BASE_BRANCHES: worktrees.append(info) return worktrees diff --git a/apps/frontend/src/main/__tests__/insights-config.test.ts b/apps/frontend/src/main/__tests__/insights-config.test.ts index ef3df618dc..710b25112f 100644 --- a/apps/frontend/src/main/__tests__/insights-config.test.ts +++ b/apps/frontend/src/main/__tests__/insights-config.test.ts @@ -59,9 +59,8 @@ describe('InsightsConfig', () => { expect(env.CLAUDE_CODE_OAUTH_TOKEN).toBe('oauth-token'); expect(env.ANTHROPIC_BASE_URL).toBe('https://api.z.ai'); expect(env.ANTHROPIC_AUTH_TOKEN).toBe('key'); - expect(env.PYTHONPATH).toBe( - [path.resolve('/site-packages'), path.resolve('/backend')].join(path.delimiter) - ); + // path.resolve() normalizes paths to platform-specific format + expect(env.PYTHONPATH).toBe([path.resolve('/site-packages'), path.resolve('/backend')].join(path.delimiter)); }); it('should clear ANTHROPIC env vars in OAuth mode when no API profile is set', async () => { @@ -86,6 +85,7 @@ describe('InsightsConfig', () => { const env = await config.getProcessEnv(); + // path.resolve() normalizes paths to platform-specific format expect(env.PYTHONPATH).toBe(path.resolve('/backend')); }); @@ -96,6 +96,7 @@ describe('InsightsConfig', () => { const env = await config.getProcessEnv(); + // path.resolve() normalizes paths to platform-specific format expect(env.PYTHONPATH).toBe(path.resolve('/site-packages')); }); }); diff --git a/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts b/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts index 68fa2ec404..7d10eeced8 100644 --- a/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts @@ -1,8 +1,8 @@ import { ipcMain, BrowserWindow } from 'electron'; -import { IPC_CHANNELS, AUTO_BUILD_PATHS, getSpecsDir } from '../../../shared/constants'; -import type { IPCResult, TaskStartOptions, TaskStatus, ImageAttachment } from '../../../shared/types'; +import { IPC_CHANNELS, AUTO_BUILD_PATHS, getSpecsDir, BASE_BRANCHES } from '../../../shared/constants'; +import type { IPCResult, TaskStartOptions, TaskStatus, ImageAttachment, WorktreeCleanupResult } from '../../../shared/types'; import path from 'path'; -import { existsSync, readFileSync, writeFileSync, renameSync, unlinkSync, mkdirSync } from 'fs'; +import { existsSync, readFileSync, writeFileSync, renameSync, unlinkSync, mkdirSync, rmSync } from 'fs'; import { spawnSync, execFileSync } from 'child_process'; import { getToolPath } from '../../cli-tool-manager'; import { AgentManager } from '../../agent'; @@ -23,6 +23,234 @@ import { projectStore } from '../../project-store'; * Writes to a temporary file first, then atomically renames to target. * This ensures the target file is never in an inconsistent state. */ +/** + * Result of checking a worktree's branch state. + * Uses discriminated union to distinguish between valid branches, base branches, and errors. + */ +type GetWorktreeBranchResult = + | { valid: true; branch: string } + | { valid: false; reason: 'base_branch'; branch: string } + | { valid: false; reason: 'git_error'; error: Error }; + +/** + * Check if a worktree is in a valid git state with a feature branch. + * Returns a discriminated union to distinguish between: + * - valid: true with branch name (feature branch) + * - valid: false, reason: 'base_branch' (orphaned after GitHub merge) + * - valid: false, reason: 'git_error' (transient git failure - should NOT trigger cleanup) + * + * This handles the edge case where a PR was merged via GitHub web UI but the + * local worktree directory still exists. + */ +function getWorktreeBranch(worktreePath: string): GetWorktreeBranchResult { + try { + const branch = execFileSync(getToolPath('git'), ['rev-parse', '--abbrev-ref', 'HEAD'], { + cwd: worktreePath, + encoding: 'utf-8', + stdio: ['pipe', 'pipe', 'pipe'], + timeout: 10000 + }).trim(); + + // Skip if on a base branch (orphaned after GitHub merge) + // Allow any feature branch pattern (auto-claude/*, feature/*, etc.) + // Use case-insensitive comparison for robustness + if (BASE_BRANCHES.includes(branch.toLowerCase() as typeof BASE_BRANCHES[number])) { + return { valid: false, reason: 'base_branch', branch }; + } + return { valid: true, branch }; + } catch (error) { + // Git command failed - could be transient (lock file, disk I/O) - do NOT treat as orphaned + console.warn(`[getWorktreeBranch] Failed to get branch for worktree at ${worktreePath}:`, error); + return { valid: false, reason: 'git_error', error: error instanceof Error ? error : new Error(String(error)) }; + } +} + +/** + * Remove a worktree directory using git worktree remove with retry logic. + * Falls back to direct directory deletion if git command fails. + */ +async function removeWorktreeWithRetry( + projectPath: string, + worktreePath: string, + maxRetries = 3 +): Promise { + let removed = false; + let lastError: Error | null = null; + + for (let attempt = 1; attempt <= maxRetries && !removed; attempt++) { + try { + execFileSync(getToolPath('git'), ['worktree', 'remove', '--force', worktreePath], { + cwd: projectPath, + encoding: 'utf-8', + timeout: 30000 + }); + removed = true; + console.warn(`[removeWorktreeWithRetry] Worktree removed: ${worktreePath}`); + } catch (err) { + lastError = err instanceof Error ? err : new Error(String(err)); + if (attempt < maxRetries) { + console.warn(`[removeWorktreeWithRetry] Retry ${attempt}/${maxRetries} - waiting before retry...`); + await new Promise(resolve => setTimeout(resolve, 1000)); + } + } + } + + if (!removed && lastError) { + const errorMessage = lastError.message || String(lastError); + // Check for "not a working tree" error - directory exists but isn't registered with git + if (errorMessage.includes('not a working tree') || errorMessage.includes('is not a valid working tree')) { + console.warn(`[removeWorktreeWithRetry] Directory exists but isn't a git worktree - deleting directly`); + try { + rmSync(worktreePath, { recursive: true, force: true }); + removed = true; + console.warn(`[removeWorktreeWithRetry] Directory deleted: ${worktreePath}`); + pruneWorktrees(projectPath); + } catch (rmError) { + return { + success: false, + canSkipCleanup: true, + worktreePath, + error: `Cannot delete worktree directory: ${rmError instanceof Error ? rmError.message : String(rmError)}` + }; + } + } + if (!removed) { + return { + success: false, + canSkipCleanup: true, + worktreePath, + error: `Cannot delete worktree: ${errorMessage}` + }; + } + } + + return { success: true }; +} + +/** + * Prune stale worktree references (best-effort, logs errors for debugging). + */ +function pruneWorktrees(projectPath: string): void { + try { + execFileSync(getToolPath('git'), ['worktree', 'prune'], { + cwd: projectPath, + encoding: 'utf-8', + timeout: 30000 + }); + } catch (e) { + console.debug('[pruneWorktrees] Prune failed (non-blocking):', e); + } +} + +/** + * Delete a git branch (best-effort, logs warning on failure). + */ +function deleteBranch(projectPath: string, branch: string): void { + try { + execFileSync(getToolPath('git'), ['branch', '-D', branch], { + cwd: projectPath, + encoding: 'utf-8', + timeout: 30000 + }); + console.warn(`[deleteBranch] Branch deleted: ${branch}`); + } catch { + console.warn(`[deleteBranch] Could not delete branch ${branch} (may not exist or be checked out elsewhere)`); + } +} + +/** + * Clean up an orphaned worktree (no valid auto-claude/* branch). + * This handles the edge case where a PR was merged via GitHub web UI. + * Returns WorktreeCleanupResult to allow caller to handle failures appropriately. + */ +function cleanupOrphanedWorktree( + projectPath: string, + worktreePath: string, + expectedBranch: string +): WorktreeCleanupResult { + try { + // Try git worktree remove first, fall back to direct deletion + try { + execFileSync(getToolPath('git'), ['worktree', 'remove', '--force', worktreePath], { + cwd: projectPath, + encoding: 'utf-8', + timeout: 30000 + }); + console.warn(`[cleanupOrphanedWorktree] Orphaned worktree removed: ${worktreePath}`); + } catch (gitRemoveError) { + console.warn(`[cleanupOrphanedWorktree] 'git worktree remove' failed, falling back to directory deletion. Error:`, gitRemoveError); + try { + rmSync(worktreePath, { recursive: true, force: true }); + // Verify the directory was actually deleted + if (!existsSync(worktreePath)) { + console.warn(`[cleanupOrphanedWorktree] Orphaned worktree directory deleted: ${worktreePath}`); + } else { + console.error(`[cleanupOrphanedWorktree] Directory still exists after rmSync: ${worktreePath}`); + return { + success: false, + canSkipCleanup: true, + worktreePath, + error: `Failed to delete worktree directory: ${worktreePath}. Files may be in use.` + }; + } + } catch (rmError) { + console.error(`[cleanupOrphanedWorktree] rmSync failed:`, rmError); + return { + success: false, + canSkipCleanup: true, + worktreePath, + error: `Cannot delete worktree directory: ${rmError instanceof Error ? rmError.message : String(rmError)}` + }; + } + } + + // Directory was successfully removed (all failure paths return early above) + // Now safe to delete the branch and prune stale worktree references + deleteBranch(projectPath, expectedBranch); + pruneWorktrees(projectPath); + + console.warn(`[cleanupOrphanedWorktree] Orphaned worktree cleanup completed`); + return { success: true }; + } catch (cleanupError) { + console.error(`[cleanupOrphanedWorktree] Failed to cleanup orphaned worktree:`, cleanupError); + return { + success: false, + canSkipCleanup: true, + worktreePath, + error: `Cleanup failed: ${cleanupError instanceof Error ? cleanupError.message : String(cleanupError)}` + }; + } +} + +/** + * Clean up a valid worktree with user-confirmed force cleanup. + * Uses retry logic for Windows file locks. + */ +async function cleanupValidWorktree( + projectPath: string, + worktreePath: string, + branch: string +): Promise { + try { + const result = await removeWorktreeWithRetry(projectPath, worktreePath); + if (!result.success) { + return result; + } + + // Delete the branch + deleteBranch(projectPath, branch); + console.warn(`[cleanupValidWorktree] Worktree cleanup completed successfully`); + return { success: true }; + } catch (cleanupError) { + return { + success: false, + canSkipCleanup: true, + worktreePath, + error: `Failed to cleanup worktree: ${cleanupError instanceof Error ? cleanupError.message : String(cleanupError)}\n\nYou can skip cleanup and mark the task as done. The worktree will remain for manual deletion.` + }; + } +} + function atomicWriteFileSync(filePath: string, content: string): void { const tempPath = `${filePath}.${process.pid}.tmp`; try { @@ -533,6 +761,7 @@ export function registerTaskExecutionHandlers( * Update task status manually * Options: * - forceCleanup: When setting to 'done' with a worktree present, delete the worktree first + * - skipCleanup: When setting to 'done', skip worktree cleanup entirely (for when files are locked) */ ipcMain.handle( IPC_CHANNELS.TASK_UPDATE_STATUS, @@ -540,8 +769,8 @@ export function registerTaskExecutionHandlers( _, taskId: string, status: TaskStatus, - options?: { forceCleanup?: boolean } - ): Promise => { + options?: { forceCleanup?: boolean; skipCleanup?: boolean } + ): Promise => { // Find task and project first (needed for worktree check) const { task, project } = findTaskAndProject(taskId); @@ -552,68 +781,55 @@ export function registerTaskExecutionHandlers( // Validate status transition - 'done' can only be set through merge handler // UNLESS there's no worktree (limbo state - already merged/discarded or failed) // OR forceCleanup is requested (user confirmed they want to delete the worktree) + // OR skipCleanup is requested (user wants to mark done without cleanup) + // OR the worktree is in an invalid state (orphaned after GitHub PR merge) if (status === 'done') { + // Stop file watcher BEFORE any cleanup operations to release file handles + // This prevents file-locking issues on Windows where chokidar holds handles open + fileWatcher.unwatch(taskId); + + // If skipCleanup is requested, skip all worktree operations + if (options?.skipCleanup) { + console.warn(`[TASK_UPDATE_STATUS] Skipping worktree cleanup for task ${taskId} (user requested)`); + // Fall through to status update - don't check or cleanup worktree + } else { // Check if worktree exists (task.specId matches worktree folder name) const worktreePath = findTaskWorktree(project.path, task.specId); const hasWorktree = worktreePath !== null; if (hasWorktree) { - if (options?.forceCleanup) { - // User confirmed cleanup - delete worktree and branch - console.warn(`[TASK_UPDATE_STATUS] Cleaning up worktree for task ${taskId} (user confirmed)`); - try { - // Get the branch name before removing the worktree - let branch = ''; - let usingFallbackBranch = false; - try { - branch = execFileSync(getToolPath('git'), ['rev-parse', '--abbrev-ref', 'HEAD'], { - cwd: worktreePath, - encoding: 'utf-8', - timeout: 30000 - }).trim(); - } catch (branchError) { - // If we can't get branch name, use the default pattern - branch = `auto-claude/${task.specId}`; - usingFallbackBranch = true; - console.warn(`[TASK_UPDATE_STATUS] Could not get branch name, using fallback pattern: ${branch}`, branchError); - } - - // Remove the worktree - execFileSync(getToolPath('git'), ['worktree', 'remove', '--force', worktreePath], { - cwd: project.path, - encoding: 'utf-8', - timeout: 30000 - }); - console.warn(`[TASK_UPDATE_STATUS] Worktree removed: ${worktreePath}`); - - // Delete the branch (ignore errors if branch doesn't exist) - try { - execFileSync(getToolPath('git'), ['branch', '-D', branch], { - cwd: project.path, - encoding: 'utf-8', - timeout: 30000 - }); - console.warn(`[TASK_UPDATE_STATUS] Branch deleted: ${branch}`); - } catch (branchDeleteError) { - // Branch may not exist or may be the current branch - if (usingFallbackBranch) { - // More concerning - fallback pattern didn't match actual branch - console.warn(`[TASK_UPDATE_STATUS] Could not delete branch ${branch} using fallback pattern. Actual branch may still exist and need manual cleanup.`, branchDeleteError); - } else { - console.warn(`[TASK_UPDATE_STATUS] Could not delete branch ${branch} (may not exist or be checked out elsewhere)`); - } - } + // Check if the worktree is in a valid git state with an auto-claude/* branch + const branchResult = getWorktreeBranch(worktreePath); - console.warn(`[TASK_UPDATE_STATUS] Worktree cleanup completed successfully`); - } catch (cleanupError) { - console.error(`[TASK_UPDATE_STATUS] Failed to cleanup worktree:`, cleanupError); - return { - success: false, - error: `Failed to cleanup worktree: ${cleanupError instanceof Error ? cleanupError.message : String(cleanupError)}` - }; + if (!branchResult.valid && branchResult.reason === 'git_error') { + // Git error (transient failure like lock file, disk I/O) - do NOT auto-cleanup + // This prevents accidentally deleting a valid worktree with uncommitted changes + console.warn(`[TASK_UPDATE_STATUS] Git error checking worktree for task ${taskId}. Not auto-cleaning to avoid data loss.`); + return { + success: false, + error: `Cannot verify worktree state: ${branchResult.error.message}. Please check if git is working correctly and try again.` + }; + } else if (!branchResult.valid && branchResult.reason === 'base_branch') { + // Orphaned worktree (e.g., after GitHub PR merge) - clean up automatically + console.warn(`[TASK_UPDATE_STATUS] Orphaned worktree detected for task ${taskId} (on base branch ${branchResult.branch}). Auto-cleaning...`); + const expectedBranch = `auto-claude/${task.specId}`; + const cleanupResult = cleanupOrphanedWorktree(project.path, worktreePath, expectedBranch); + if (!cleanupResult.success) { + // Orphaned cleanup failed - return error so UI can show "Skip Cleanup" option + // This prevents silent accumulation of orphaned directories that can cause + // "already exists" errors on future task creations + console.warn(`[TASK_UPDATE_STATUS] Orphaned worktree cleanup failed: ${cleanupResult.error}`); + return cleanupResult; } - } else { - // Worktree exists but no forceCleanup - return special response for UI to show confirmation + } else if (branchResult.valid && options?.forceCleanup) { + // Valid worktree exists and user confirmed cleanup + console.warn(`[TASK_UPDATE_STATUS] Cleaning up worktree for task ${taskId} (user confirmed)`); + const cleanupResult = await cleanupValidWorktree(project.path, worktreePath, branchResult.branch); + if (!cleanupResult.success) { + return cleanupResult; + } + } else if (branchResult.valid) { + // Valid worktree exists but no forceCleanup - return special response for UI console.warn(`[TASK_UPDATE_STATUS] Worktree exists for task ${taskId}. Requesting user confirmation.`); return { success: false, @@ -626,6 +842,7 @@ export function registerTaskExecutionHandlers( // No worktree - allow marking as done (limbo state recovery) console.warn(`[TASK_UPDATE_STATUS] Allowing status 'done' for task ${taskId} (no worktree found - limbo state)`); } + } // end of else block for !skipCleanup } // Validate status transition - 'human_review' requires actual work to have been done @@ -680,6 +897,7 @@ export function registerTaskExecutionHandlers( if (status !== 'in_progress' && agentManager.isRunning(taskId)) { console.warn('[TASK_UPDATE_STATUS] Stopping task due to status change away from in_progress:', taskId); agentManager.killTask(taskId); + fileWatcher.unwatch(taskId); } // Auto-start task when status changes to 'in_progress' and no process is running diff --git a/apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts b/apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts index ce8a822c6d..a9b946421a 100644 --- a/apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts @@ -1,5 +1,5 @@ import { ipcMain, BrowserWindow, shell, app } from 'electron'; -import { IPC_CHANNELS, AUTO_BUILD_PATHS, DEFAULT_APP_SETTINGS, DEFAULT_FEATURE_MODELS, DEFAULT_FEATURE_THINKING, MODEL_ID_MAP, THINKING_BUDGET_MAP, getSpecsDir } from '../../../shared/constants'; +import { IPC_CHANNELS, AUTO_BUILD_PATHS, DEFAULT_APP_SETTINGS, DEFAULT_FEATURE_MODELS, DEFAULT_FEATURE_THINKING, MODEL_ID_MAP, THINKING_BUDGET_MAP, getSpecsDir, BASE_BRANCHES } from '../../../shared/constants'; import type { IPCResult, WorktreeStatus, WorktreeDiff, WorktreeDiffFile, WorktreeMergeResult, WorktreeDiscardResult, WorktreeListResult, WorktreeListItem, WorktreeCreatePROptions, WorktreeCreatePRResult, SupportedIDE, SupportedTerminal, AppSettings } from '../../../shared/types'; import path from 'path'; import { existsSync, readdirSync, statSync, readFileSync } from 'fs'; @@ -2678,6 +2678,15 @@ export function registerWorktreeHandlers( encoding: 'utf-8' }).trim(); + // Skip if HEAD is detached or pointing to a main/base branch + // (this can happen if worktree is orphaned after GitHub merge) + // But allow any feature branch pattern (auto-claude/*, feature/*, etc.) + // Use case-insensitive comparison for robustness + if (BASE_BRANCHES.includes(branch.toLowerCase() as typeof BASE_BRANCHES[number])) { + console.warn(`[TASK_LIST_WORKTREES] Skipping worktree ${entry} - on base branch: ${branch}`); + return; // Skip - likely orphaned or misconfigured + } + // Get base branch using proper fallback chain: // 1. Task metadata baseBranch, 2. Project settings mainBranch, 3. main/master detection // Note: We do NOT use current HEAD as that may be a feature branch diff --git a/apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts b/apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts index 8f44838aa4..b680001cff 100644 --- a/apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts +++ b/apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts @@ -4,7 +4,6 @@ import path from 'path'; import { describe, expect, it, vi, beforeEach } from 'vitest'; import type * as pty from '@lydell/node-pty'; import type { TerminalProcess } from '../types'; -import { buildCdCommand } from '../../../shared/utils/shell-escape'; /** Escape special regex characters in a string for safe use in RegExp constructor */ const escapeForRegex = (str: string): string => str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); @@ -102,7 +101,9 @@ describe('claude-integration-handler', () => { invokeClaude(terminal, '/tmp/project', undefined, () => null, vi.fn()); const written = vi.mocked(terminal.pty.write).mock.calls[0][0] as string; - expect(written).toContain(buildCdCommand('/tmp/project')); + // buildCdCommand uses double quotes on Windows, single quotes on Unix + const cdQuote = process.platform === 'win32' ? '"' : "'"; + expect(written).toContain(`cd ${cdQuote}/tmp/project${cdQuote} && `); expect(written).toContain("PATH='/opt/claude/bin:/usr/bin' "); expect(written).toContain("'/opt/claude bin/claude'\\''s'"); expect(mockReleaseSessionId).toHaveBeenCalledWith('term-1'); @@ -235,8 +236,10 @@ describe('claude-integration-handler', () => { const tokenPath = vi.mocked(writeFileSync).mock.calls[0]?.[0] as string; const tokenContents = vi.mocked(writeFileSync).mock.calls[0]?.[1] as string; - const tokenPrefix = path.join(tmpdir(), '.claude-token-1234-'); - expect(tokenPath).toMatch(new RegExp(`^${escapeForRegex(tokenPrefix)}[0-9a-f]{16}$`)); + // path.join normalizes the path to use platform-specific separators + const normalizedTmpDir = escapeForRegex(path.join(tmpdir(), '')); + const pathSep = escapeForRegex(path.sep); + expect(tokenPath).toMatch(new RegExp(`^${normalizedTmpDir}${pathSep}\\.claude-token-1234-[0-9a-f]{16}$`)); expect(tokenContents).toBe("export CLAUDE_CODE_OAUTH_TOKEN='token-value'\n"); const written = vi.mocked(terminal.pty.write).mock.calls[0][0] as string; expect(written).toContain("HISTFILE= HISTCONTROL=ignorespace "); @@ -278,8 +281,10 @@ describe('claude-integration-handler', () => { const tokenPath = vi.mocked(writeFileSync).mock.calls[0]?.[0] as string; const tokenContents = vi.mocked(writeFileSync).mock.calls[0]?.[1] as string; - const tokenPrefix = path.join(tmpdir(), '.claude-token-5678-'); - expect(tokenPath).toMatch(new RegExp(`^${escapeForRegex(tokenPrefix)}[0-9a-f]{16}$`)); + // path.join normalizes the path to use platform-specific separators + const normalizedTmpDir = escapeForRegex(path.join(tmpdir(), '')); + const pathSep = escapeForRegex(path.sep); + expect(tokenPath).toMatch(new RegExp(`^${normalizedTmpDir}${pathSep}\\.claude-token-5678-[0-9a-f]{16}$`)); expect(tokenContents).toBe("export CLAUDE_CODE_OAUTH_TOKEN='token-value'\n"); const written = vi.mocked(terminal.pty.write).mock.calls[0][0] as string; expect(written).toContain(`source '${tokenPath}'`); diff --git a/apps/frontend/src/preload/api/task-api.ts b/apps/frontend/src/preload/api/task-api.ts index 417b73f43f..2ee051d429 100644 --- a/apps/frontend/src/preload/api/task-api.ts +++ b/apps/frontend/src/preload/api/task-api.ts @@ -42,8 +42,8 @@ export interface TaskAPI { updateTaskStatus: ( taskId: string, status: TaskStatus, - options?: { forceCleanup?: boolean } - ) => Promise; + options?: { forceCleanup?: boolean; skipCleanup?: boolean } + ) => Promise; recoverStuckTask: ( taskId: string, options?: import('../../shared/types').TaskRecoveryOptions @@ -122,8 +122,8 @@ export const createTaskAPI = (): TaskAPI => ({ updateTaskStatus: ( taskId: string, status: TaskStatus, - options?: { forceCleanup?: boolean } - ): Promise => + options?: { forceCleanup?: boolean; skipCleanup?: boolean } + ): Promise => ipcRenderer.invoke(IPC_CHANNELS.TASK_UPDATE_STATUS, taskId, status, options), recoverStuckTask: ( diff --git a/apps/frontend/src/renderer/components/KanbanBoard.tsx b/apps/frontend/src/renderer/components/KanbanBoard.tsx index cf609f6db8..f69fb4e878 100644 --- a/apps/frontend/src/renderer/components/KanbanBoard.tsx +++ b/apps/frontend/src/renderer/components/KanbanBoard.tsx @@ -27,7 +27,7 @@ import { TaskCard } from './TaskCard'; import { SortableTaskCard } from './SortableTaskCard'; import { TASK_STATUS_COLUMNS, TASK_STATUS_LABELS } from '../../shared/constants'; import { cn } from '../lib/utils'; -import { persistTaskStatus, forceCompleteTask, archiveTasks } from '../stores/task-store'; +import { persistTaskStatus, forceCompleteTask, skipCleanupCompleteTask, archiveTasks } from '../stores/task-store'; import { useToast } from '../hooks/use-toast'; import { WorktreeCleanupDialog } from './WorktreeCleanupDialog'; import type { Task, TaskStatus } from '../../shared/types'; @@ -348,21 +348,27 @@ export function KanbanBoard({ tasks, onTaskClick, onNewTaskClick, onRefresh, isR const { showArchived, toggleShowArchived } = useViewState(); // Worktree cleanup dialog state - const [worktreeCleanupDialog, setWorktreeCleanupDialog] = useState<{ + type WorktreeCleanupDialogState = { open: boolean; taskId: string | null; taskTitle: string; worktreePath?: string; isProcessing: boolean; error?: string; - }>({ + canSkipCleanup?: boolean; + }; + + const INITIAL_WORKTREE_DIALOG_STATE: WorktreeCleanupDialogState = { open: false, taskId: null, taskTitle: '', worktreePath: undefined, isProcessing: false, - error: undefined - }); + error: undefined, + canSkipCleanup: false + }; + + const [worktreeCleanupDialog, setWorktreeCleanupDialog] = useState(INITIAL_WORKTREE_DIALOG_STATE); // Calculate archived count for Done column button const archivedCount = useMemo(() => @@ -485,7 +491,8 @@ export function KanbanBoard({ tasks, onTaskClick, onNewTaskClick, onRefresh, isR taskTitle: task?.title || t('tasks:untitled'), worktreePath: result.worktreePath, isProcessing: false, - error: undefined + error: undefined, + canSkipCleanup: false }); } else { // Show error toast for other failures @@ -504,21 +511,37 @@ export function KanbanBoard({ tasks, onTaskClick, onNewTaskClick, onRefresh, isR const handleWorktreeCleanupConfirm = async () => { if (!worktreeCleanupDialog.taskId) return; - setWorktreeCleanupDialog(prev => ({ ...prev, isProcessing: true, error: undefined })); + setWorktreeCleanupDialog(prev => ({ ...prev, isProcessing: true, error: undefined, canSkipCleanup: false })); const result = await forceCompleteTask(worktreeCleanupDialog.taskId); if (result.success) { - setWorktreeCleanupDialog({ - open: false, - taskId: null, - taskTitle: '', - worktreePath: undefined, - isProcessing: false, - error: undefined - }); + setWorktreeCleanupDialog(INITIAL_WORKTREE_DIALOG_STATE); } else { // Keep dialog open with error state for retry - show actual error if available + // Check if the error allows skipping cleanup (e.g., files in use) + setWorktreeCleanupDialog(prev => ({ + ...prev, + isProcessing: false, + error: result.error || t('dialogs:worktreeCleanup.errorDescription'), + canSkipCleanup: result.canSkipCleanup || false + })); + } + }; + + /** + * Handle skipping worktree cleanup (mark done without deleting worktree) + */ + const handleWorktreeSkipCleanup = async () => { + if (!worktreeCleanupDialog.taskId) return; + + setWorktreeCleanupDialog(prev => ({ ...prev, isProcessing: true, error: undefined, canSkipCleanup: false })); + + const result = await skipCleanupCompleteTask(worktreeCleanupDialog.taskId); + + if (result.success) { + setWorktreeCleanupDialog(INITIAL_WORKTREE_DIALOG_STATE); + } else { setWorktreeCleanupDialog(prev => ({ ...prev, isProcessing: false, @@ -624,12 +647,14 @@ export function KanbanBoard({ tasks, onTaskClick, onNewTaskClick, onRefresh, isR worktreePath={worktreeCleanupDialog.worktreePath} isProcessing={worktreeCleanupDialog.isProcessing} error={worktreeCleanupDialog.error} + canSkipCleanup={worktreeCleanupDialog.canSkipCleanup} onOpenChange={(open) => { if (!open && !worktreeCleanupDialog.isProcessing) { - setWorktreeCleanupDialog(prev => ({ ...prev, open: false, error: undefined })); + setWorktreeCleanupDialog(prev => ({ ...prev, open: false, error: undefined, canSkipCleanup: false })); } }} onConfirm={handleWorktreeCleanupConfirm} + onSkipCleanup={handleWorktreeSkipCleanup} /> ); diff --git a/apps/frontend/src/renderer/components/WorktreeCleanupDialog.tsx b/apps/frontend/src/renderer/components/WorktreeCleanupDialog.tsx index fccb027a8e..1249cd64f8 100644 --- a/apps/frontend/src/renderer/components/WorktreeCleanupDialog.tsx +++ b/apps/frontend/src/renderer/components/WorktreeCleanupDialog.tsx @@ -17,8 +17,10 @@ interface WorktreeCleanupDialogProps { worktreePath?: string; isProcessing: boolean; error?: string; + canSkipCleanup?: boolean; onOpenChange: (open: boolean) => void; onConfirm: () => void; + onSkipCleanup?: () => void; } /** @@ -30,8 +32,10 @@ export function WorktreeCleanupDialog({ worktreePath, isProcessing, error, + canSkipCleanup, onOpenChange, - onConfirm + onConfirm, + onSkipCleanup }: WorktreeCleanupDialogProps) { const { t } = useTranslation(['dialogs', 'common']); @@ -80,6 +84,19 @@ export function WorktreeCleanupDialog({ {t('common:buttons.cancel')} + {error && canSkipCleanup && onSkipCleanup && ( + { + e.preventDefault(); + onSkipCleanup(); + }} + disabled={isProcessing} + className="bg-muted text-muted-foreground hover:bg-muted/80" + > + + {t('dialogs:worktreeCleanup.skipCleanup', 'Mark Done (Keep Worktree)')} + + )} { e.preventDefault(); diff --git a/apps/frontend/src/renderer/stores/task-store.ts b/apps/frontend/src/renderer/stores/task-store.ts index 8d44ad639f..910995b29d 100644 --- a/apps/frontend/src/renderer/stores/task-store.ts +++ b/apps/frontend/src/renderer/stores/task-store.ts @@ -517,6 +517,7 @@ export interface PersistStatusResult { success: boolean; worktreeExists?: boolean; worktreePath?: string; + canSkipCleanup?: boolean; error?: string; } @@ -527,7 +528,7 @@ export interface PersistStatusResult { export async function persistTaskStatus( taskId: string, status: TaskStatus, - options?: { forceCleanup?: boolean } + options?: { forceCleanup?: boolean; skipCleanup?: boolean } ): Promise { const store = useTaskStore.getState(); @@ -546,6 +547,16 @@ export async function persistTaskStatus( error: result.error }; } + // Check if cleanup failed but can be skipped + if (result.canSkipCleanup) { + console.log('[persistTaskStatus] Cleanup failed, skip option available'); + return { + success: false, + canSkipCleanup: true, + worktreePath: result.worktreePath, + error: result.error + }; + } console.error('Failed to persist task status:', result.error); return { success: false, error: result.error }; } @@ -568,6 +579,15 @@ export async function forceCompleteTask(taskId: string): Promise { + return persistTaskStatus(taskId, 'done', { skipCleanup: true }); +} + /** * Update task title/description/metadata and persist to file */ diff --git a/apps/frontend/src/shared/constants/git.ts b/apps/frontend/src/shared/constants/git.ts new file mode 100644 index 0000000000..45f437eec8 --- /dev/null +++ b/apps/frontend/src/shared/constants/git.ts @@ -0,0 +1,15 @@ +/** + * Git-related constants + * + * NOTE: These constants are also defined in apps/backend/core/worktree.py + * If you modify BASE_BRANCHES here, ensure the Python version stays in sync. + */ + +/** + * Base/default branch names that should be filtered out when listing worktrees. + * Used to detect orphaned worktrees after GitHub merge operations. + * Comparison should be case-insensitive (use .toLowerCase()). + */ +export const BASE_BRANCHES = ['main', 'master', 'develop', 'head'] as const; + +export type BaseBranch = (typeof BASE_BRANCHES)[number]; diff --git a/apps/frontend/src/shared/constants/index.ts b/apps/frontend/src/shared/constants/index.ts index 5b3f49872f..d1346c00fa 100644 --- a/apps/frontend/src/shared/constants/index.ts +++ b/apps/frontend/src/shared/constants/index.ts @@ -35,3 +35,6 @@ export * from './api-profiles'; // Configuration and paths export * from './config'; + +// Git-related constants +export * from './git'; diff --git a/apps/frontend/src/shared/types/ipc.ts b/apps/frontend/src/shared/types/ipc.ts index a607fad067..e0fee91178 100644 --- a/apps/frontend/src/shared/types/ipc.ts +++ b/apps/frontend/src/shared/types/ipc.ts @@ -160,7 +160,7 @@ export interface ElectronAPI { startTask: (taskId: string, options?: TaskStartOptions) => void; stopTask: (taskId: string) => void; submitReview: (taskId: string, approved: boolean, feedback?: string, images?: ImageAttachment[]) => Promise; - updateTaskStatus: (taskId: string, status: TaskStatus, options?: { forceCleanup?: boolean }) => Promise; + updateTaskStatus: (taskId: string, status: TaskStatus, options?: { forceCleanup?: boolean; skipCleanup?: boolean }) => Promise; recoverStuckTask: (taskId: string, options?: TaskRecoveryOptions) => Promise>; checkTaskRunning: (taskId: string) => Promise>; diff --git a/apps/frontend/src/shared/types/task.ts b/apps/frontend/src/shared/types/task.ts index 47e3b78462..8d35c22568 100644 --- a/apps/frontend/src/shared/types/task.ts +++ b/apps/frontend/src/shared/types/task.ts @@ -437,6 +437,16 @@ export interface WorktreeCreatePRResult { alreadyExists?: boolean; } +/** + * Result of worktree cleanup operations (removing worktree directory and branch) + */ +export interface WorktreeCleanupResult { + success: boolean; + canSkipCleanup?: boolean; + worktreePath?: string; + error?: string; +} + /** * Information about a single spec worktree * Per-spec architecture: Each spec has its own worktree at .worktrees/{spec-name}/ diff --git a/tests/test_worktree.py b/tests/test_worktree.py index a6b725d20e..95b414f6ba 100644 --- a/tests/test_worktree.py +++ b/tests/test_worktree.py @@ -10,15 +10,17 @@ - Merge operations - Change tracking - Worktree cleanup and age detection +- Cross-language constant validation """ +import re import subprocess from datetime import datetime from pathlib import Path import pytest -from worktree import WorktreeManager +from worktree import WorktreeManager, BASE_BRANCHES class TestWorktreeManagerInitialization: @@ -709,3 +711,51 @@ def test_get_worktree_count_critical_warning(self, temp_git_repo: Path): warning = manager.get_worktree_count_warning(critical_threshold=20) assert warning is not None assert "CRITICAL" in warning + + +class TestCrossLanguageConstantValidation: + """Tests to ensure Python and TypeScript constants stay in sync.""" + + def test_base_branches_matches_typescript(self): + """Validate Python BASE_BRANCHES matches TypeScript BASE_BRANCHES constant. + + This test ensures the cross-language constant synchronization documented in + apps/backend/core/worktree.py and apps/frontend/src/shared/constants/git.ts + is maintained. + """ + # Navigate from tests/ to repo root, then to frontend constants + repo_root = Path(__file__).parent.parent + ts_file = repo_root / "apps" / "frontend" / "src" / "shared" / "constants" / "git.ts" + + assert ts_file.exists(), ( + f"TypeScript constants file not found: {ts_file}\n" + f"Expected path relative to repo root: apps/frontend/src/shared/constants/git.ts" + ) + + # Read and parse TypeScript file + ts_content = ts_file.read_text() + + # Extract BASE_BRANCHES array from TypeScript using regex + # Matches: export const BASE_BRANCHES = ['main', 'master', 'develop', 'head'] as const; + match = re.search( + r"export\s+const\s+BASE_BRANCHES\s*=\s*\[(.*?)\]\s*as\s+const", + ts_content, + re.DOTALL + ) + + assert match, "Could not find BASE_BRANCHES constant in TypeScript file" + + # Parse the array contents (extract quoted strings) + array_content = match.group(1) + ts_branches = set(re.findall(r"['\"](\w+)['\"]", array_content)) + + # Compare with Python constant (case-insensitive since both use .toLowerCase()/.lower()) + py_branches = set(BASE_BRANCHES) + + assert ts_branches == py_branches, ( + f"BASE_BRANCHES mismatch between Python and TypeScript!\n" + f"Python: {sorted(py_branches)}\n" + f"TypeScript: {sorted(ts_branches)}\n" + f"Missing in Python: {sorted(ts_branches - py_branches)}\n" + f"Missing in TypeScript: {sorted(py_branches - ts_branches)}" + )