diff --git a/.husky/pre-commit b/.husky/pre-commit index 5e0e6a21..90f4edcc 100755 --- a/.husky/pre-commit +++ b/.husky/pre-commit @@ -1,5 +1,14 @@ #!/bin/sh +# Preserve git worktree context - prevent HEAD corruption in worktrees +if [ -f ".git" ]; then + WORKTREE_GIT_DIR=$(cat .git | sed 's/gitdir: //') + if [ -n "$WORKTREE_GIT_DIR" ]; then + export GIT_DIR="$WORKTREE_GIT_DIR" + export GIT_WORK_TREE="$(pwd)" + fi +fi + echo "Running pre-commit checks..." # ============================================================================= @@ -126,28 +135,30 @@ if git diff --cached --name-only | grep -q "^apps/backend/.*\.py$"; then fi # Run pytest (skip slow/integration tests and Windows-incompatible tests for pre-commit speed) + # Use subshell to isolate directory changes and prevent worktree corruption echo "Running Python tests..." - cd apps/backend - # Tests to skip: graphiti (external deps), merge_file_tracker/service_orchestrator/worktree/workspace (Windows path/git issues) - IGNORE_TESTS="--ignore=../../tests/test_graphiti.py --ignore=../../tests/test_merge_file_tracker.py --ignore=../../tests/test_service_orchestrator.py --ignore=../../tests/test_worktree.py --ignore=../../tests/test_workspace.py" - if [ -d ".venv" ]; then - # Use venv if it exists - if [ -f ".venv/bin/pytest" ]; then - PYTHONPATH=. .venv/bin/pytest ../../tests/ -v --tb=short -x -m "not slow and not integration" $IGNORE_TESTS - elif [ -f ".venv/Scripts/pytest.exe" ]; then - # Windows - PYTHONPATH=. .venv/Scripts/pytest.exe ../../tests/ -v --tb=short -x -m "not slow and not integration" $IGNORE_TESTS + ( + cd apps/backend + # Tests to skip: graphiti (external deps), merge_file_tracker/service_orchestrator/worktree/workspace (Windows path/git issues) + IGNORE_TESTS="--ignore=../../tests/test_graphiti.py --ignore=../../tests/test_merge_file_tracker.py --ignore=../../tests/test_service_orchestrator.py --ignore=../../tests/test_worktree.py --ignore=../../tests/test_workspace.py" + if [ -d ".venv" ]; then + # Use venv if it exists + if [ -f ".venv/bin/pytest" ]; then + PYTHONPATH=. .venv/bin/pytest ../../tests/ -v --tb=short -x -m "not slow and not integration" $IGNORE_TESTS + elif [ -f ".venv/Scripts/pytest.exe" ]; then + # Windows + PYTHONPATH=. .venv/Scripts/pytest.exe ../../tests/ -v --tb=short -x -m "not slow and not integration" $IGNORE_TESTS + else + PYTHONPATH=. python -m pytest ../../tests/ -v --tb=short -x -m "not slow and not integration" $IGNORE_TESTS + fi else PYTHONPATH=. python -m pytest ../../tests/ -v --tb=short -x -m "not slow and not integration" $IGNORE_TESTS fi - else - PYTHONPATH=. python -m pytest ../../tests/ -v --tb=short -x -m "not slow and not integration" $IGNORE_TESTS - fi + ) if [ $? -ne 0 ]; then echo "Python tests failed. Please fix failing tests before committing." exit 1 fi - cd ../.. echo "Backend checks passed!" fi @@ -159,40 +170,44 @@ fi # Check if there are staged files in apps/frontend if git diff --cached --name-only | grep -q "^apps/frontend/"; then echo "Frontend changes detected, running frontend checks..." - cd apps/frontend + # Use subshell to isolate directory changes and prevent worktree corruption + ( + cd apps/frontend - # Run lint-staged (handles staged .ts/.tsx files) - npm exec lint-staged + # Run lint-staged (handles staged .ts/.tsx files) + npm exec lint-staged + if [ $? -ne 0 ]; then + echo "lint-staged failed. Please fix linting errors before committing." + exit 1 + fi + + # Run TypeScript type check + echo "Running type check..." + npm run typecheck + if [ $? -ne 0 ]; then + echo "Type check failed. Please fix TypeScript errors before committing." + exit 1 + fi + + # Run linting + echo "Running lint..." + npm run lint + if [ $? -ne 0 ]; then + echo "Lint failed. Run 'npm run lint:fix' to auto-fix issues." + exit 1 + fi + + # Check for vulnerabilities (only high severity) + echo "Checking for vulnerabilities..." + npm audit --audit-level=high + if [ $? -ne 0 ]; then + echo "High severity vulnerabilities found. Run 'npm audit fix' to resolve." + exit 1 + fi + ) if [ $? -ne 0 ]; then - echo "lint-staged failed. Please fix linting errors before committing." exit 1 fi - - # Run TypeScript type check - echo "Running type check..." - npm run typecheck - if [ $? -ne 0 ]; then - echo "Type check failed. Please fix TypeScript errors before committing." - exit 1 - fi - - # Run linting - echo "Running lint..." - npm run lint - if [ $? -ne 0 ]; then - echo "Lint failed. Run 'npm run lint:fix' to auto-fix issues." - exit 1 - fi - - # Check for vulnerabilities (only high severity) - echo "Checking for vulnerabilities..." - npm audit --audit-level=high - if [ $? -ne 0 ]; then - echo "High severity vulnerabilities found. Run 'npm audit fix' to resolve." - exit 1 - fi - - cd ../.. echo "Frontend checks passed!" fi diff --git a/apps/backend/cli/build_commands.py b/apps/backend/cli/build_commands.py index ad5766ac..99fdb96f 100644 --- a/apps/backend/cli/build_commands.py +++ b/apps/backend/cli/build_commands.py @@ -87,6 +87,7 @@ def handle_build_command( debug_success, ) from phase_config import get_phase_model + from prompts_pkg.prompts import get_base_branch_from_metadata from qa_loop import run_qa_validation_loop, should_run_qa from .utils import print_banner, validate_environment @@ -194,6 +195,14 @@ def handle_build_command( auto_continue=auto_continue, ) + # If base_branch not provided via CLI, try to read from task_metadata.json + # This ensures the backend uses the branch configured in the frontend + if base_branch is None: + metadata_branch = get_base_branch_from_metadata(spec_dir) + if metadata_branch: + base_branch = metadata_branch + debug("run.py", f"Using base branch from task metadata: {base_branch}") + if workspace_mode == WorkspaceMode.ISOLATED: # Keep reference to original spec directory for syncing progress back source_spec_dir = spec_dir diff --git a/apps/backend/core/workspace.py b/apps/backend/core/workspace.py index 6ae292ab..2190d55a 100644 --- a/apps/backend/core/workspace.py +++ b/apps/backend/core/workspace.py @@ -252,9 +252,10 @@ def merge_existing_build( stats = smart_result.get("stats", {}) had_conflicts = stats.get("conflicts_resolved", 0) > 0 ai_assisted = stats.get("ai_assisted", 0) > 0 + direct_copy = stats.get("direct_copy", False) - if had_conflicts or ai_assisted: - # AI actually resolved conflicts or assisted with merges + if had_conflicts or ai_assisted or direct_copy: + # AI resolved conflicts, assisted with merges, or direct copy was used # Changes are already written and staged - no need for git merge _print_merge_success( no_commit, stats, spec_name=spec_name, keep_worktree=True @@ -499,6 +500,155 @@ def _try_smart_merge_inner( "error": resolution_result.get("error"), } + # Check if branches diverged but no actual conflicts (can do direct copy) + if git_conflicts.get("diverged_but_no_conflicts"): + debug(MODULE, "Branches diverged but no conflicts - doing direct file copy") + print(muted(" Branches diverged but no conflicts detected")) + print(muted(" Copying changed files directly from worktree...")) + + # Get changed files from spec branch + spec_branch = f"auto-claude/{spec_name}" + base_branch = git_conflicts.get("base_branch", "main") + + # Get merge-base for diff + merge_base_result = subprocess.run( + ["git", "merge-base", base_branch, spec_branch], + cwd=project_dir, + capture_output=True, + text=True, + ) + merge_base = ( + merge_base_result.stdout.strip() + if merge_base_result.returncode == 0 + else None + ) + + if merge_base: + # Get list of changed files in spec branch + changed_files = _get_changed_files_from_branch( + project_dir, merge_base, spec_branch + ) + + resolved_files = [] + skipped_files = [] # Track files that failed to copy + files_to_stage = [] + for file_path, status in changed_files: + if _is_auto_claude_file(file_path): + continue + + try: + target_path = project_dir / file_path + + if status == "D": + # Deleted in worktree + if target_path.exists(): + target_path.unlink() + files_to_stage.append(file_path) + resolved_files.append(file_path) + print(success(f" ✓ {file_path} (deleted)")) + else: + # New or modified - copy from spec branch + target_path.parent.mkdir(parents=True, exist_ok=True) + + if _is_binary_file(file_path): + binary_content = _get_binary_file_content_from_ref( + project_dir, spec_branch, file_path + ) + if binary_content is not None: + target_path.write_bytes(binary_content) + files_to_stage.append(file_path) + resolved_files.append(file_path) + status_label = ( + "new file" if status == "A" else "updated" + ) + print( + success(f" ✓ {file_path} ({status_label})") + ) + else: + skipped_files.append(file_path) + debug_warning( + MODULE, + f"Could not retrieve binary content for {file_path}", + ) + else: + content = _get_file_content_from_ref( + project_dir, spec_branch, file_path + ) + if content is not None: + target_path.write_text(content, encoding="utf-8") + files_to_stage.append(file_path) + resolved_files.append(file_path) + status_label = ( + "new file" if status == "A" else "updated" + ) + print( + success(f" ✓ {file_path} ({status_label})") + ) + else: + skipped_files.append(file_path) + debug_warning( + MODULE, + f"Could not retrieve content for {file_path}", + ) + + except Exception as e: + skipped_files.append(file_path) + debug_warning(MODULE, f"Could not copy {file_path}: {e}") + + # Stage all files in a single git add call for efficiency + if files_to_stage: + try: + subprocess.run( + ["git", "add"] + files_to_stage, + cwd=project_dir, + capture_output=True, + text=True, + check=True, + ) + except subprocess.CalledProcessError as e: + debug_warning( + MODULE, f"Failed to stage files for direct copy: {e.stderr}" + ) + # Return failure - files were written but not staged + return { + "success": False, + "error": f"Failed to stage files: {e.stderr}", + "resolved_files": [], + } + + # Build result - check for skipped files to detect partial merges + result = { + "success": len(skipped_files) == 0, + "resolved_files": resolved_files, + "stats": { + "files_merged": len(resolved_files), + "conflicts_resolved": 0, + "ai_assisted": 0, + "auto_merged": len(resolved_files), + "direct_copy": True, # Flag indicating direct copy was used + "skipped_count": len(skipped_files), + }, + } + if skipped_files: + result["skipped_files"] = skipped_files + result["partial_success"] = len(resolved_files) > 0 + print() + print( + warning( + f" ⚠ {len(skipped_files)} file(s) could not be retrieved:" + ) + ) + for skipped_file in skipped_files: + print(muted(f" - {skipped_file}")) + print(muted(" These files may need manual review.")) + return result + else: + # merge-base failed - branches may not share history + debug_warning( + MODULE, + "Could not find merge-base between branches - falling back to semantic analysis", + ) + # No git conflicts - proceed with semantic analysis debug(MODULE, "No git conflicts, proceeding with semantic analysis") preview = orchestrator.preview_merge([spec_name]) @@ -623,11 +773,10 @@ def _check_git_conflicts(project_dir: Path, spec_name: str) -> dict: text=True, ) - # merge-tree returns exit code 1 if there are conflicts + # merge-tree returns exit code 1 if there are actual text conflicts + # Exit code 0 means clean merge possible if merge_tree_result.returncode != 0: - result["has_conflicts"] = True - - # Parse the output for conflicting files + # Parse the output for ACTUAL conflicting files (look for CONFLICT markers) output = merge_tree_result.stdout + merge_tree_result.stderr for line in output.split("\n"): if "CONFLICT" in line: @@ -645,38 +794,27 @@ def _check_git_conflicts(project_dir: Path, spec_name: str) -> dict: ): result["conflicting_files"].append(file_path) - # Fallback: if we didn't parse conflicts, use diff to find files changed in both branches - if not result["conflicting_files"]: - main_files_result = subprocess.run( - ["git", "diff", "--name-only", merge_base, main_commit], - cwd=project_dir, - capture_output=True, - text=True, + # Only set has_conflicts if we found ACTUAL CONFLICT markers + # A non-zero exit code without CONFLICT markers just means branches diverged + # but git can auto-merge them - we handle this with direct file copy + if result["conflicting_files"]: + result["has_conflicts"] = True + debug( + MODULE, + f"Found {len(result['conflicting_files'])} actual git conflicts", + files=result["conflicting_files"], ) - main_files = ( - set(main_files_result.stdout.strip().split("\n")) - if main_files_result.stdout.strip() - else set() + else: + # No CONFLICT markers = no actual conflicts + # Branches diverged but changes don't overlap - git can auto-merge + # We'll handle this by copying files directly from spec branch + debug( + MODULE, + "No CONFLICT markers - branches diverged but can be auto-merged", + merge_tree_returncode=merge_tree_result.returncode, ) - - spec_files_result = subprocess.run( - ["git", "diff", "--name-only", merge_base, spec_commit], - cwd=project_dir, - capture_output=True, - text=True, - ) - spec_files = ( - set(spec_files_result.stdout.strip().split("\n")) - if spec_files_result.stdout.strip() - else set() - ) - - # Files modified in both = potential conflicts - # Filter out .auto-claude files - they should never be merged - conflicting = main_files & spec_files - result["conflicting_files"] = [ - f for f in conflicting if not _is_auto_claude_file(f) - ] + result["has_conflicts"] = False + result["diverged_but_no_conflicts"] = True # Flag for direct copy except Exception as e: print(muted(f" Error checking git conflicts: {e}")) @@ -829,6 +967,7 @@ def _resolve_git_conflicts_with_ai( tuple[str, str | None] ] = [] # (file_path, merged_content or None for delete) lock_files_excluded: list[str] = [] # Lock files excluded from merge + auto_merged_simple: set[str] = set() # Files that were auto-merged via simple 3-way debug(MODULE, "Categorizing conflicting files for parallel processing") @@ -887,27 +1026,49 @@ def _resolve_git_conflicts_with_ai( f" {target_file_path}: lock file (excluded - will use main version)", ) else: - # Regular file - needs AI merge - # Store the TARGET path for writing, but track original for content retrieval - files_needing_ai_merge.append( - ParallelMergeTask( - file_path=target_file_path, # Use target path for writing - main_content=main_content, - worktree_content=worktree_content, - base_content=base_content, - spec_name=spec_name, - project_dir=project_dir, + # File exists in both - try simple 3-way merge FIRST (no AI needed) + # This handles cases where: + # - Only one side changed from base (ours==base or theirs==base) + # - Both sides made identical changes (ours==theirs) + simple_success, simple_merged = _try_simple_3way_merge( + base_content, main_content, worktree_content + ) + + if simple_success and simple_merged is not None: + # Simple 3-way merge succeeded - no AI needed! + simple_merges.append((target_file_path, simple_merged)) + auto_merged_simple.add(target_file_path) # Track for stats + debug( + MODULE, + f" {file_path}: auto-merged (simple 3-way, no AI needed)" + + ( + f" (will write to {target_file_path})" + if target_file_path != file_path + else "" + ), + ) + else: + # Simple merge failed - needs AI merge + # Store the TARGET path for writing, but track original for content retrieval + files_needing_ai_merge.append( + ParallelMergeTask( + file_path=target_file_path, # Use target path for writing + main_content=main_content, + worktree_content=worktree_content, + base_content=base_content, + spec_name=spec_name, + project_dir=project_dir, + ) + ) + debug( + MODULE, + f" {file_path}: needs AI merge (both sides changed differently)" + + ( + f" (will write to {target_file_path})" + if target_file_path != file_path + else "" + ), ) - ) - debug( - MODULE, - f" {file_path}: needs AI merge" - + ( - f" (will write to {target_file_path})" - if target_file_path != file_path - else "" - ), - ) except Exception as e: print(error(f" ✗ Failed to categorize {file_path}: {e}")) @@ -932,7 +1093,18 @@ def _resolve_git_conflicts_with_ai( ["git", "add", file_path], cwd=project_dir, capture_output=True ) resolved_files.append(file_path) - print(success(f" ✓ {file_path} (new file)")) + # Show appropriate message based on merge type + if file_path in auto_merged_simple: + print(success(f" ✓ {file_path} (auto-merged)")) + auto_merged_count += 1 # Count for stats + elif file_path in lock_files_excluded: + print( + success( + f" ✓ {file_path} (lock file - kept main version)" + ) + ) + else: + print(success(f" ✓ {file_path} (new file)")) else: # Delete the file target_path = project_dir / file_path @@ -1198,6 +1370,9 @@ def _resolve_git_conflicts_with_ai( "conflicts_resolved": len(conflicting_files) - len(remaining_conflicts), "ai_assisted": ai_merged_count, "auto_merged": auto_merged_count, + "simple_3way_merged": len( + auto_merged_simple + ), # Files auto-merged without AI "parallel_ai_merges": len(files_needing_ai_merge), "lock_files_excluded": len(lock_files_excluded), }, diff --git a/apps/backend/prompts_pkg/prompts.py b/apps/backend/prompts_pkg/prompts.py index 83a87269..fd533b31 100644 --- a/apps/backend/prompts_pkg/prompts.py +++ b/apps/backend/prompts_pkg/prompts.py @@ -58,7 +58,7 @@ def _validate_branch_name(branch: str | None) -> str | None: return branch -def _get_base_branch_from_metadata(spec_dir: Path) -> str | None: +def get_base_branch_from_metadata(spec_dir: Path) -> str | None: """ Read baseBranch from task_metadata.json if it exists. @@ -81,6 +81,10 @@ def _get_base_branch_from_metadata(spec_dir: Path) -> str | None: return None +# Alias for backwards compatibility (internal use) +_get_base_branch_from_metadata = get_base_branch_from_metadata + + def _detect_base_branch(spec_dir: Path, project_dir: Path) -> str: """ Detect the base branch for a project/task. diff --git a/apps/backend/qa/loop.py b/apps/backend/qa/loop.py index fcbc1c7f..b28ad62e 100644 --- a/apps/backend/qa/loop.py +++ b/apps/backend/qa/loop.py @@ -317,17 +317,18 @@ async def run_qa_validation_loop( issues=current_issues[:3] if current_issues else [], # Show first 3 ) - # Record rejected iteration - record_iteration( - spec_dir, qa_iteration, "rejected", current_issues, iteration_duration - ) - - # Check for recurring issues + # Check for recurring issues BEFORE recording current iteration + # This prevents the current issues from matching themselves in history history = get_iteration_history(spec_dir) has_recurring, recurring_issues = has_recurring_issues( current_issues, history ) + # Record rejected iteration AFTER checking for recurring issues + record_iteration( + spec_dir, qa_iteration, "rejected", current_issues, iteration_duration + ) + if has_recurring: from .report import RECURRING_ISSUE_THRESHOLD 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 1626190f..4f68e10e 100644 --- a/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts @@ -3,7 +3,8 @@ import { IPC_CHANNELS, AUTO_BUILD_PATHS, getSpecsDir } from '../../../shared/con import type { IPCResult, TaskStartOptions, TaskStatus } from '../../../shared/types'; import path from 'path'; import { existsSync, readFileSync, writeFileSync, renameSync, unlinkSync } from 'fs'; -import { spawnSync } from 'child_process'; +import { spawnSync, execFileSync } from 'child_process'; +import { getToolPath } from '../../cli-tool-manager'; import { AgentManager } from '../../agent'; import { fileWatcher } from '../../file-watcher'; import { findTaskAndProject } from './shared'; @@ -438,14 +439,17 @@ export function registerTaskExecutionHandlers( /** * Update task status manually + * Options: + * - forceCleanup: When setting to 'done' with a worktree present, delete the worktree first */ ipcMain.handle( IPC_CHANNELS.TASK_UPDATE_STATUS, async ( _, taskId: string, - status: TaskStatus - ): Promise => { + status: TaskStatus, + options?: { forceCleanup?: boolean } + ): Promise => { // Find task and project first (needed for worktree check) const { task, project } = findTaskAndProject(taskId); @@ -455,21 +459,80 @@ 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) if (status === 'done') { // Check if worktree exists (task.specId matches worktree folder name) const worktreePath = findTaskWorktree(project.path, task.specId); const hasWorktree = worktreePath !== null; if (hasWorktree) { - // Worktree exists - must use merge workflow - console.warn(`[TASK_UPDATE_STATUS] Blocked attempt to set status 'done' directly for task ${taskId}. Use merge workflow instead.`); - return { - success: false, - error: "Cannot set status to 'done' directly. Complete the human review and merge the worktree changes instead." - }; + 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)`); + } + } + + 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)}` + }; + } + } else { + // Worktree exists but no forceCleanup - return special response for UI to show confirmation + console.warn(`[TASK_UPDATE_STATUS] Worktree exists for task ${taskId}. Requesting user confirmation.`); + return { + success: false, + worktreeExists: true, + worktreePath: worktreePath, + error: "A worktree still exists for this task. Would you like to delete it and mark the task as complete?" + }; + } } else { // No worktree - allow marking as done (limbo state recovery) - console.log(`[TASK_UPDATE_STATUS] Allowing status 'done' for task ${taskId} (no worktree found - limbo state)`); + console.warn(`[TASK_UPDATE_STATUS] Allowing status 'done' for task ${taskId} (no worktree found - limbo state)`); } } 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 6a10e345..ce8a822c 100644 --- a/apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts @@ -1656,9 +1656,19 @@ export function registerWorktreeHandlers( // 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 const baseBranch = getEffectiveBaseBranch(project.path, task.specId, project.settings?.mainBranch); + // Get user's current branch in main project (this is where changes will merge INTO) + let currentProjectBranch: string | undefined; + try { + currentProjectBranch = execFileSync(getToolPath('git'), ['rev-parse', '--abbrev-ref', 'HEAD'], { + cwd: project.path, + encoding: 'utf-8' + }).trim(); + } catch { + // Ignore - might be in detached HEAD or git error + } + // Get commit count (cross-platform - no shell syntax) let commitCount = 0; try { @@ -1703,6 +1713,7 @@ export function registerWorktreeHandlers( worktreePath, branch, baseBranch, + currentProjectBranch, commitCount, filesChanged, additions, @@ -2133,39 +2144,16 @@ export function registerWorktreeHandlers( if (isStageOnly && !hasActualStagedChanges && mergeAlreadyCommitted) { // Stage-only was requested but merge was already committed previously - // Mark as done since changes are already in the branch - newStatus = 'done'; - planStatus = 'completed'; - message = 'Changes were already merged and committed. Task marked as done.'; + // Keep in human_review and let user explicitly mark as done (which will trigger cleanup confirmation) + // This ensures user is in control of when the worktree is deleted + newStatus = 'human_review'; + planStatus = 'review'; + message = 'Changes were already merged and committed. You can mark this task as complete when ready.'; staged = false; - debug('Stage-only requested but merge already committed. Marking as done.'); - - // Clean up worktree since merge is complete (fixes #243) - // This is the same cleanup as the full merge path, needed because - // stageOnly defaults to true for human_review tasks - try { - if (worktreePath && existsSync(worktreePath)) { - execFileSync(getToolPath('git'), ['worktree', 'remove', '--force', worktreePath], { - cwd: project.path, - encoding: 'utf-8' - }); - debug('Worktree cleaned up (already merged):', worktreePath); - - // Also delete the task branch - const taskBranch = `auto-claude/${task.specId}`; - try { - execFileSync(getToolPath('git'), ['branch', '-D', taskBranch], { - cwd: project.path, - encoding: 'utf-8' - }); - debug('Task branch deleted:', taskBranch); - } catch { - // Branch might not exist or already deleted - } - } - } catch (cleanupErr) { - debug('Worktree cleanup failed (non-fatal):', cleanupErr); - } + debug('Stage-only requested but merge already committed. Keeping in human_review for user to confirm completion.'); + // NOTE: We intentionally do NOT auto-clean the worktree here. + // User can drag the task to "Done" column which will show a confirmation dialog + // asking if they want to delete the worktree and mark complete. } else if (isStageOnly && !hasActualStagedChanges) { // Stage-only was requested but no changes to stage (and not committed) // This could mean nothing to merge or an error - keep in human_review for investigation diff --git a/apps/frontend/src/main/worktree-paths.ts b/apps/frontend/src/main/worktree-paths.ts index b7e3d02e..3716e070 100644 --- a/apps/frontend/src/main/worktree-paths.ts +++ b/apps/frontend/src/main/worktree-paths.ts @@ -32,18 +32,47 @@ export function getTaskWorktreePath(projectPath: string, specId: string): string return path.join(projectPath, TASK_WORKTREE_DIR, specId); } +/** + * Validate that a resolved path is within the expected base directory + * Protects against path traversal attacks (e.g., specId containing "..") + */ +function isPathWithinBase(resolvedPath: string, basePath: string): boolean { + const normalizedPath = path.resolve(resolvedPath); + const normalizedBase = path.resolve(basePath); + return normalizedPath.startsWith(normalizedBase + path.sep) || normalizedPath === normalizedBase; +} + /** * Find a task worktree path, checking new location first then legacy * Returns the path if found, null otherwise + * Includes path traversal protection to ensure paths stay within project */ export function findTaskWorktree(projectPath: string, specId: string): string | null { + const normalizedProject = path.resolve(projectPath); + // Check new path first const newPath = path.join(projectPath, TASK_WORKTREE_DIR, specId); - if (existsSync(newPath)) return newPath; + const resolvedNewPath = path.resolve(newPath); + + // Validate path stays within project (defense against path traversal) + if (!isPathWithinBase(resolvedNewPath, normalizedProject)) { + console.error(`[worktree-paths] Path traversal detected: specId "${specId}" resolves outside project`); + return null; + } + + if (existsSync(resolvedNewPath)) return resolvedNewPath; // Legacy fallback const legacyPath = path.join(projectPath, LEGACY_WORKTREE_DIR, specId); - if (existsSync(legacyPath)) return legacyPath; + const resolvedLegacyPath = path.resolve(legacyPath); + + // Validate legacy path as well + if (!isPathWithinBase(resolvedLegacyPath, normalizedProject)) { + console.error(`[worktree-paths] Path traversal detected: specId "${specId}" resolves outside project (legacy)`); + return null; + } + + if (existsSync(resolvedLegacyPath)) return resolvedLegacyPath; return null; } @@ -65,15 +94,34 @@ export function getTerminalWorktreePath(projectPath: string, name: string): stri /** * Find a terminal worktree path, checking new location first then legacy * Returns the path if found, null otherwise + * Includes path traversal protection to ensure paths stay within project */ export function findTerminalWorktree(projectPath: string, name: string): string | null { + const normalizedProject = path.resolve(projectPath); + // Check new path first const newPath = path.join(projectPath, TERMINAL_WORKTREE_DIR, name); - if (existsSync(newPath)) return newPath; + const resolvedNewPath = path.resolve(newPath); + + // Validate path stays within project (defense against path traversal) + if (!isPathWithinBase(resolvedNewPath, normalizedProject)) { + console.error(`[worktree-paths] Path traversal detected: name "${name}" resolves outside project`); + return null; + } + + if (existsSync(resolvedNewPath)) return resolvedNewPath; // Legacy fallback (terminal worktrees used terminal-{name} prefix) const legacyPath = path.join(projectPath, LEGACY_WORKTREE_DIR, `terminal-${name}`); - if (existsSync(legacyPath)) return legacyPath; + const resolvedLegacyPath = path.resolve(legacyPath); + + // Validate legacy path as well + if (!isPathWithinBase(resolvedLegacyPath, normalizedProject)) { + console.error(`[worktree-paths] Path traversal detected: name "${name}" resolves outside project (legacy)`); + return null; + } + + if (existsSync(resolvedLegacyPath)) return resolvedLegacyPath; return null; } diff --git a/apps/frontend/src/preload/api/task-api.ts b/apps/frontend/src/preload/api/task-api.ts index 167e39f0..0fbe408d 100644 --- a/apps/frontend/src/preload/api/task-api.ts +++ b/apps/frontend/src/preload/api/task-api.ts @@ -39,8 +39,9 @@ export interface TaskAPI { ) => Promise; updateTaskStatus: ( taskId: string, - status: TaskStatus - ) => Promise; + status: TaskStatus, + options?: { forceCleanup?: boolean } + ) => Promise; recoverStuckTask: ( taskId: string, options?: import('../../shared/types').TaskRecoveryOptions @@ -117,9 +118,10 @@ export const createTaskAPI = (): TaskAPI => ({ updateTaskStatus: ( taskId: string, - status: TaskStatus - ): Promise => - ipcRenderer.invoke(IPC_CHANNELS.TASK_UPDATE_STATUS, taskId, status), + status: TaskStatus, + options?: { forceCleanup?: boolean } + ): Promise => + ipcRenderer.invoke(IPC_CHANNELS.TASK_UPDATE_STATUS, taskId, status, options), recoverStuckTask: ( taskId: string, diff --git a/apps/frontend/src/renderer/components/KanbanBoard.tsx b/apps/frontend/src/renderer/components/KanbanBoard.tsx index 56f797b8..4119be4b 100644 --- a/apps/frontend/src/renderer/components/KanbanBoard.tsx +++ b/apps/frontend/src/renderer/components/KanbanBoard.tsx @@ -19,23 +19,17 @@ import { sortableKeyboardCoordinates, verticalListSortingStrategy } from '@dnd-kit/sortable'; -import { Plus, Inbox, Loader2, Eye, CheckCircle2, Archive, RefreshCw, Trash2, FolderCheck } from 'lucide-react'; +import { Plus, Inbox, Loader2, Eye, CheckCircle2, Archive, RefreshCw } from 'lucide-react'; import { ScrollArea } from './ui/scroll-area'; import { Button } from './ui/button'; import { Tooltip, TooltipContent, TooltipTrigger } from './ui/tooltip'; -import { - AlertDialog, - AlertDialogContent, - AlertDialogDescription, - AlertDialogFooter, - AlertDialogHeader, - AlertDialogTitle, -} from './ui/alert-dialog'; 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, archiveTasks } from '../stores/task-store'; +import { persistTaskStatus, forceCompleteTask, archiveTasks } from '../stores/task-store'; +import { useToast } from '../hooks/use-toast'; +import { WorktreeCleanupDialog } from './WorktreeCleanupDialog'; import type { Task, TaskStatus } from '../../shared/types'; // Type guard for valid drop column targets - preserves literal type from TASK_STATUS_COLUMNS @@ -339,15 +333,28 @@ const DroppableColumn = memo(function DroppableColumn({ status, tasks, onTaskCli }, droppableColumnPropsAreEqual); export function KanbanBoard({ tasks, onTaskClick, onNewTaskClick, onRefresh, isRefreshing }: KanbanBoardProps) { - const { t } = useTranslation('tasks'); + const { t } = useTranslation(['tasks', 'dialogs', 'common']); + const { toast } = useToast(); const [activeTask, setActiveTask] = useState(null); const [overColumnId, setOverColumnId] = useState(null); const { showArchived, toggleShowArchived } = useViewState(); // Worktree cleanup dialog state - const [worktreeDialogOpen, setWorktreeDialogOpen] = useState(false); - const [pendingDoneTask, setPendingDoneTask] = useState(null); - const [isCleaningUp, setIsCleaningUp] = useState(false); + const [worktreeCleanupDialog, setWorktreeCleanupDialog] = useState<{ + open: boolean; + taskId: string | null; + taskTitle: string; + worktreePath?: string; + isProcessing: boolean; + error?: string; + }>({ + open: false, + taskId: null, + taskTitle: '', + worktreePath: undefined, + isProcessing: false, + error: undefined + }); // Calculate archived count for Done column button const archivedCount = useMemo(() => @@ -452,39 +459,66 @@ export function KanbanBoard({ tasks, onTaskClick, onNewTaskClick, onRefresh, isR } }; - // Check if a task has a worktree (async check) - const checkTaskHasWorktree = async (taskId: string): Promise => { - try { - const result = await window.electronAPI.getWorktreeStatus(taskId); - return result.success && result.data?.exists === true; - } catch { - return false; - } - }; + /** + * Handle status change with worktree cleanup dialog support + * Consolidated handler that accepts an optional task object for the dialog title + */ + const handleStatusChange = async (taskId: string, newStatus: TaskStatus, providedTask?: Task) => { + const task = providedTask || tasks.find(t => t.id === taskId); + const result = await persistTaskStatus(taskId, newStatus); - // Handle moving task to done with worktree cleanup option - const handleMoveToDone = async (task: Task, deleteWorktree: boolean) => { - setIsCleaningUp(true); - try { - if (deleteWorktree) { - // Delete worktree first, skip automatic status change to backlog - // since we're about to set status to 'done' - const result = await window.electronAPI.discardWorktree(task.id, true); - if (!result.success) { - console.error('Failed to delete worktree:', result.error); - // Continue anyway - user can clean up manually - } + if (!result.success) { + if (result.worktreeExists) { + // Show the worktree cleanup dialog + setWorktreeCleanupDialog({ + open: true, + taskId: taskId, + taskTitle: task?.title || t('tasks:untitled'), + worktreePath: result.worktreePath, + isProcessing: false, + error: undefined + }); + } else { + // Show error toast for other failures + toast({ + title: t('common:errors.operationFailed'), + description: result.error || t('common:errors.unknownError'), + variant: 'destructive' + }); } - // Mark as done - await persistTaskStatus(task.id, 'done'); - } finally { - setIsCleaningUp(false); - setWorktreeDialogOpen(false); - setPendingDoneTask(null); } }; - const handleDragEnd = async (event: DragEndEvent) => { + /** + * Handle worktree cleanup confirmation + */ + const handleWorktreeCleanupConfirm = async () => { + if (!worktreeCleanupDialog.taskId) return; + + setWorktreeCleanupDialog(prev => ({ ...prev, isProcessing: true, error: undefined })); + + const result = await forceCompleteTask(worktreeCleanupDialog.taskId); + + if (result.success) { + setWorktreeCleanupDialog({ + open: false, + taskId: null, + taskTitle: '', + worktreePath: undefined, + isProcessing: false, + error: undefined + }); + } else { + // Keep dialog open with error state for retry - show actual error if available + setWorktreeCleanupDialog(prev => ({ + ...prev, + isProcessing: false, + error: result.error || t('dialogs:worktreeCleanup.errorDescription') + })); + } + }; + + const handleDragEnd = (event: DragEndEvent) => { const { active, over } = event; setActiveTask(null); setOverColumnId(null); @@ -494,38 +528,31 @@ export function KanbanBoard({ tasks, onTaskClick, onNewTaskClick, onRefresh, isR const activeTaskId = active.id as string; const overId = over.id as string; - // Determine target status - let targetStatus: TaskStatus | null = null; - + // Check if dropped on a column if (isValidDropColumn(overId)) { - targetStatus = overId; - } else { - // Dropped on another task - get its column - const overTask = tasks.find((t) => t.id === overId); - if (overTask) { - targetStatus = overTask.status; + const newStatus = overId; + const task = tasks.find((t) => t.id === activeTaskId); + + if (task && task.status !== newStatus) { + // Persist status change to file and update local state + handleStatusChange(activeTaskId, newStatus, task).catch((err) => + console.error('[KanbanBoard] Status change failed:', err) + ); } + return; } - if (!targetStatus) return; - - const task = tasks.find((t) => t.id === activeTaskId); - if (!task || task.status === targetStatus) return; - - // Special handling for moving to "done" - check for worktree - if (targetStatus === 'done') { - const hasWorktree = await checkTaskHasWorktree(task.id); - - if (hasWorktree) { - // Show dialog asking about worktree cleanup - setPendingDoneTask(task); - setWorktreeDialogOpen(true); - return; + // Check if dropped on another task - move to that task's column + const overTask = tasks.find((t) => t.id === overId); + if (overTask) { + const task = tasks.find((t) => t.id === activeTaskId); + if (task && task.status !== overTask.status) { + // Persist status change to file and update local state + handleStatusChange(activeTaskId, overTask.status, task).catch((err) => + console.error('[KanbanBoard] Status change failed:', err) + ); } } - - // Normal status change - persistTaskStatus(activeTaskId, targetStatus); }; return ( @@ -560,7 +587,7 @@ export function KanbanBoard({ tasks, onTaskClick, onNewTaskClick, onRefresh, isR status={status} tasks={tasksByStatus[status]} onTaskClick={onTaskClick} - onStatusChange={persistTaskStatus} + onStatusChange={handleStatusChange} isOver={overColumnId === status} onAddClick={status === 'backlog' ? onNewTaskClick : undefined} onArchiveAll={status === 'done' ? handleArchiveAll : undefined} @@ -582,71 +609,19 @@ export function KanbanBoard({ tasks, onTaskClick, onNewTaskClick, onRefresh, isR {/* Worktree cleanup confirmation dialog */} - - - - - - {t('kanban.worktreeCleanupTitle', 'Worktree Cleanup')} - - -
- {pendingDoneTask?.stagedInMainProject ? ( -

- {t('kanban.worktreeCleanupStaged', 'This task has been staged and has a worktree. Would you like to clean up the worktree?')} -

- ) : ( -

- {t('kanban.worktreeCleanupNotStaged', 'This task has a worktree with changes that have not been merged. Delete the worktree to mark as done, or cancel to review the changes first.')} -

- )} - {pendingDoneTask && ( -

- {pendingDoneTask.title} -

- )} -
- - - - - {/* Only show "Keep Worktree" option if task is staged */} - {pendingDoneTask?.stagedInMainProject && ( - - )} - - - - + { + if (!open && !worktreeCleanupDialog.isProcessing) { + setWorktreeCleanupDialog(prev => ({ ...prev, open: false, error: undefined })); + } + }} + onConfirm={handleWorktreeCleanupConfirm} + /> ); } diff --git a/apps/frontend/src/renderer/components/TaskCreationWizard.tsx b/apps/frontend/src/renderer/components/TaskCreationWizard.tsx index aa0fd3c4..cbfa73e5 100644 --- a/apps/frontend/src/renderer/components/TaskCreationWizard.tsx +++ b/apps/frontend/src/renderer/components/TaskCreationWizard.tsx @@ -348,7 +348,14 @@ export function TaskCreationWizard({ if (images.length > 0) metadata.attachedImages = images; if (allReferencedFiles.length > 0) metadata.referencedFiles = allReferencedFiles; if (requireReviewBeforeCoding) metadata.requireReviewBeforeCoding = true; - if (baseBranch && baseBranch !== PROJECT_DEFAULT_BRANCH) metadata.baseBranch = baseBranch; + // Always include baseBranch - resolve PROJECT_DEFAULT_BRANCH to actual branch name + // This ensures the backend always knows which branch to use for worktree creation + if (baseBranch === PROJECT_DEFAULT_BRANCH) { + // Use the resolved project default branch + if (projectDefaultBranch) metadata.baseBranch = projectDefaultBranch; + } else if (baseBranch) { + metadata.baseBranch = baseBranch; + } // Pass worktree preference - false means use --direct mode if (!useWorktree) metadata.useWorktree = false; diff --git a/apps/frontend/src/renderer/components/WorktreeCleanupDialog.tsx b/apps/frontend/src/renderer/components/WorktreeCleanupDialog.tsx new file mode 100644 index 00000000..fccb027a --- /dev/null +++ b/apps/frontend/src/renderer/components/WorktreeCleanupDialog.tsx @@ -0,0 +1,112 @@ +import { useTranslation, Trans } from 'react-i18next'; +import { AlertCircle, CheckCircle2, FolderX, Loader2, RefreshCw } from 'lucide-react'; +import { + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, +} from './ui/alert-dialog'; + +interface WorktreeCleanupDialogProps { + open: boolean; + taskTitle: string; + worktreePath?: string; + isProcessing: boolean; + error?: string; + onOpenChange: (open: boolean) => void; + onConfirm: () => void; +} + +/** + * Confirmation dialog for cleaning up worktree when marking task as done + */ +export function WorktreeCleanupDialog({ + open, + taskTitle, + worktreePath, + isProcessing, + error, + onOpenChange, + onConfirm +}: WorktreeCleanupDialogProps) { + const { t } = useTranslation(['dialogs', 'common']); + + return ( + + + + + {error ? ( + + ) : ( + + )} + {error ? t('dialogs:worktreeCleanup.errorTitle') : t('dialogs:worktreeCleanup.title')} + + +
+ {error ? ( +

{error}

+ ) : ( + <> +

+ }} + /> +

+

+ {t('dialogs:worktreeCleanup.willDelete')} +

+ + )} + {worktreePath && ( +
+ {worktreePath} +
+ )} + {!error && ( +

+ {t('dialogs:worktreeCleanup.warning')} +

+ )} +
+ + + + {t('common:buttons.cancel')} + { + e.preventDefault(); + onConfirm(); + }} + disabled={isProcessing} + className={error ? "bg-primary text-primary-foreground hover:bg-primary/90" : "bg-success text-success-foreground hover:bg-success/90"} + > + {isProcessing ? ( + <> + + {t('dialogs:worktreeCleanup.completing')} + + ) : error ? ( + <> + + {t('dialogs:worktreeCleanup.retry')} + + ) : ( + <> + + {t('dialogs:worktreeCleanup.confirm')} + + )} + + + + + ); +} diff --git a/apps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsx b/apps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsx index a68de379..c869e6a7 100644 --- a/apps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsx +++ b/apps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsx @@ -192,12 +192,12 @@ export function WorkspaceStatus({ - {/* Branch info */} + {/* Branch info: spec branch → user's current branch (merge target) */} {worktreeStatus.branch && (
{worktreeStatus.branch} → - {worktreeStatus.baseBranch || 'main'} + {worktreeStatus.currentProjectBranch || worktreeStatus.baseBranch || 'main'}
)} diff --git a/apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts b/apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts index 39e0917c..ff151dbe 100644 --- a/apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts +++ b/apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts @@ -6,7 +6,7 @@ * Unit tests for useXterm keyboard handlers * Tests terminal copy/paste keyboard shortcuts and platform detection */ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach, beforeAll, afterAll } from 'vitest'; import type { Mock } from 'vitest'; import { renderHook, act, render } from '@testing-library/react'; import React from 'react'; @@ -61,8 +61,9 @@ vi.mock('../../../../lib/terminal-buffer-manager', () => ({ // Mock navigator.platform for platform detection const originalNavigatorPlatform = navigator.platform; -// Mock requestAnimationFrame for jsdom environment (not provided by default) -global.requestAnimationFrame = vi.fn((cb: FrameRequestCallback) => setTimeout(cb, 0) as unknown as number); +// Store original requestAnimationFrame for restoration after tests +const originalRequestAnimationFrame = global.requestAnimationFrame; +const originalCancelAnimationFrame = global.cancelAnimationFrame; /** * Helper function to set up XTerm mocks and render the hook @@ -152,7 +153,22 @@ describe('useXterm keyboard handlers', () => { readText: ReturnType; }; + // Mock requestAnimationFrame for jsdom environment (not provided by default) + // Isolated to this test file to prevent affecting other tests + beforeAll(() => { + global.requestAnimationFrame = vi.fn((cb: FrameRequestCallback) => setTimeout(cb, 0) as unknown as number); + global.cancelAnimationFrame = vi.fn((id: number) => clearTimeout(id)); + }); + + afterAll(() => { + global.requestAnimationFrame = originalRequestAnimationFrame; + global.cancelAnimationFrame = originalCancelAnimationFrame; + }); + beforeEach(() => { + // Use fake timers to control async behavior and prevent timer leaks + vi.useFakeTimers(); + // Clear all mocks before each test vi.clearAllMocks(); @@ -183,6 +199,11 @@ describe('useXterm keyboard handlers', () => { }); afterEach(() => { + // Clear all pending timers before restoring mocks to prevent + // "requestAnimationFrame is not defined" errors from delayed callbacks + vi.clearAllTimers(); + vi.useRealTimers(); + vi.restoreAllMocks(); // Reset navigator.platform to original value Object.defineProperty(navigator, 'platform', { @@ -211,7 +232,7 @@ describe('useXterm keyboard handlers', () => { }); keyEventHandler(event); - await new Promise(resolve => setTimeout(resolve, 0)); + await vi.advanceTimersByTimeAsync(0); }); // Windows should enable CTRL+V paste @@ -238,7 +259,7 @@ describe('useXterm keyboard handlers', () => { }); keyEventHandler(event); - await new Promise(resolve => setTimeout(resolve, 0)); + await vi.advanceTimersByTimeAsync(0); }); expect(mockPaste).toHaveBeenCalledTimes(1); @@ -252,7 +273,7 @@ describe('useXterm keyboard handlers', () => { }); keyEventHandler(event); - await new Promise(resolve => setTimeout(resolve, 0)); + await vi.advanceTimersByTimeAsync(0); }); expect(mockPaste).toHaveBeenCalledTimes(2); @@ -282,7 +303,7 @@ describe('useXterm keyboard handlers', () => { }); keyEventHandler(event); - await new Promise(resolve => setTimeout(resolve, 0)); + await vi.advanceTimersByTimeAsync(0); }); expect(mockClipboard.writeText).toHaveBeenCalledTimes(1); @@ -296,7 +317,7 @@ describe('useXterm keyboard handlers', () => { }); keyEventHandler(event); - await new Promise(resolve => setTimeout(resolve, 0)); + await vi.advanceTimersByTimeAsync(0); }); expect(mockClipboard.writeText).toHaveBeenCalledTimes(2); @@ -321,7 +342,7 @@ describe('useXterm keyboard handlers', () => { }); keyEventHandler(event); - await new Promise(resolve => setTimeout(resolve, 0)); + await vi.advanceTimersByTimeAsync(0); }); // macOS should NOT use custom CTRL+V handler (uses system Cmd+V instead) @@ -352,7 +373,7 @@ describe('useXterm keyboard handlers', () => { expect(handled).toBe(false); // Should prevent xterm handling // Wait for clipboard write - await new Promise(resolve => setTimeout(resolve, 0)); + await vi.advanceTimersByTimeAsync(0); }); // Verify the xterm instance methods were called @@ -408,7 +429,7 @@ describe('useXterm keyboard handlers', () => { if (keyEventHandler) { keyEventHandler!(event); // Wait for clipboard write - await new Promise(resolve => setTimeout(resolve, 0)); + await vi.advanceTimersByTimeAsync(0); } }); @@ -423,7 +444,7 @@ describe('useXterm keyboard handlers', () => { if (keyEventHandler) { keyEventHandler!(event); // Wait for clipboard write - await new Promise(resolve => setTimeout(resolve, 0)); + await vi.advanceTimersByTimeAsync(0); } }); @@ -455,7 +476,7 @@ describe('useXterm keyboard handlers', () => { expect(handled).toBe(false); // Should prevent literal ^V // Wait for clipboard read and paste - await new Promise(resolve => setTimeout(resolve, 0)); + await vi.advanceTimersByTimeAsync(0); } }); @@ -484,7 +505,7 @@ describe('useXterm keyboard handlers', () => { const handled = keyEventHandler(event); expect(handled).toBe(false); - await new Promise(resolve => setTimeout(resolve, 0)); + await vi.advanceTimersByTimeAsync(0); }); expect(mockClipboard.readText).toHaveBeenCalled(); @@ -546,7 +567,7 @@ describe('useXterm keyboard handlers', () => { const handled = keyEventHandler(event); expect(handled).toBe(false); - await new Promise(resolve => setTimeout(resolve, 0)); + await vi.advanceTimersByTimeAsync(0); }); expect(mockClipboard.writeText).toHaveBeenCalledWith('selected text'); @@ -603,7 +624,7 @@ describe('useXterm keyboard handlers', () => { const handled = keyEventHandler(event); expect(handled).toBe(false); - await new Promise(resolve => setTimeout(resolve, 0)); + await vi.advanceTimersByTimeAsync(0); }); expect(mockClipboard.readText).toHaveBeenCalled(); @@ -632,7 +653,7 @@ describe('useXterm keyboard handlers', () => { }); keyEventHandler(event); - await new Promise(resolve => setTimeout(resolve, 0)); + await vi.advanceTimersByTimeAsync(0); }); // Should log error but not throw @@ -666,7 +687,7 @@ describe('useXterm keyboard handlers', () => { }); keyEventHandler(event); - await new Promise(resolve => setTimeout(resolve, 0)); + await vi.advanceTimersByTimeAsync(0); }); // Should log error but not throw diff --git a/apps/frontend/src/renderer/lib/mocks/task-mock.ts b/apps/frontend/src/renderer/lib/mocks/task-mock.ts index 3c5175d9..ff4d07d8 100644 --- a/apps/frontend/src/renderer/lib/mocks/task-mock.ts +++ b/apps/frontend/src/renderer/lib/mocks/task-mock.ts @@ -60,7 +60,7 @@ export const taskMock = { unarchiveTasks: async () => ({ success: true, data: true }), // Task status operations - updateTaskStatus: async () => ({ success: true }), + updateTaskStatus: async (_taskId: string, _status: string, _options?: { forceCleanup?: boolean }) => ({ success: true }), recoverStuckTask: async (taskId: string, options?: TaskRecoveryOptions) => ({ success: true, diff --git a/apps/frontend/src/renderer/stores/task-store.ts b/apps/frontend/src/renderer/stores/task-store.ts index 144b7a93..f3086d98 100644 --- a/apps/frontend/src/renderer/stores/task-store.ts +++ b/apps/frontend/src/renderer/stores/task-store.ts @@ -451,32 +451,64 @@ export async function submitReview( } } +/** + * Result type for persistTaskStatus with worktree info + */ +export interface PersistStatusResult { + success: boolean; + worktreeExists?: boolean; + worktreePath?: string; + error?: string; +} + /** * Update task status and persist to file + * Returns additional info if a worktree exists and needs cleanup confirmation */ export async function persistTaskStatus( taskId: string, - status: TaskStatus -): Promise { + status: TaskStatus, + options?: { forceCleanup?: boolean } +): Promise { const store = useTaskStore.getState(); try { - // Update local state first for immediate feedback - store.updateTaskStatus(taskId, status); + // Persist to file first (don't optimistically update for 'done' status) + const result = await window.electronAPI.updateTaskStatus(taskId, status, options); - // Persist to file - const result = await window.electronAPI.updateTaskStatus(taskId, status); if (!result.success) { + // Check if this is a worktree exists case + if (result.worktreeExists) { + console.log('[persistTaskStatus] Worktree exists, confirmation needed'); + return { + success: false, + worktreeExists: true, + worktreePath: result.worktreePath, + error: result.error + }; + } console.error('Failed to persist task status:', result.error); - return false; + return { success: false, error: result.error }; } - return true; + + // Only update local state after backend confirms success + store.updateTaskStatus(taskId, status); + return { success: true }; } catch (error) { console.error('Error persisting task status:', error); - return false; + return { success: false, error: String(error) }; } } +/** + * Force complete a task by cleaning up its worktree + * Used when user confirms they want to delete the worktree and mark as done + * Returns full result including error details for better UX + */ +export async function forceCompleteTask(taskId: string): Promise { + return persistTaskStatus(taskId, 'done', { forceCleanup: true }); +} + /** * Update task title/description/metadata and persist to file */ diff --git a/apps/frontend/src/shared/i18n/locales/en/common.json b/apps/frontend/src/shared/i18n/locales/en/common.json index cb4d363b..377c7e93 100644 --- a/apps/frontend/src/shared/i18n/locales/en/common.json +++ b/apps/frontend/src/shared/i18n/locales/en/common.json @@ -101,6 +101,7 @@ "errors": { "generic": "An error occurred", "unknownError": "An unknown error occurred", + "operationFailed": "Operation failed", "networkError": "Network error", "notFound": "Not found", "unauthorized": "Unauthorized" diff --git a/apps/frontend/src/shared/i18n/locales/en/dialogs.json b/apps/frontend/src/shared/i18n/locales/en/dialogs.json index cd51b4fb..7792ded2 100644 --- a/apps/frontend/src/shared/i18n/locales/en/dialogs.json +++ b/apps/frontend/src/shared/i18n/locales/en/dialogs.json @@ -56,6 +56,17 @@ "delete": "Delete Worktree?", "deleteDescription": "This will permanently delete the worktree and all uncommitted changes. This action cannot be undone." }, + "worktreeCleanup": { + "title": "Complete Task", + "hasWorktree": "The task \"{{taskTitle}}\" still has an isolated workspace (worktree).", + "willDelete": "To mark this task as complete, the worktree and its associated branch will be deleted.", + "warning": "Make sure you have merged or saved any changes you want to keep before proceeding.", + "confirm": "Delete Worktree & Complete", + "completing": "Completing...", + "retry": "Try Again", + "errorTitle": "Cleanup Failed", + "errorDescription": "Failed to cleanup worktree. Please try again." + }, "update": { "title": "Auto Claude", "projectInitialized": "Project is initialized." diff --git a/apps/frontend/src/shared/i18n/locales/fr/common.json b/apps/frontend/src/shared/i18n/locales/fr/common.json index 752e861f..623b260e 100644 --- a/apps/frontend/src/shared/i18n/locales/fr/common.json +++ b/apps/frontend/src/shared/i18n/locales/fr/common.json @@ -101,6 +101,7 @@ "errors": { "generic": "Une erreur s'est produite", "unknownError": "Une erreur inconnue s'est produite", + "operationFailed": "Opération échouée", "networkError": "Erreur réseau", "notFound": "Non trouvé", "unauthorized": "Non autorisé" diff --git a/apps/frontend/src/shared/i18n/locales/fr/dialogs.json b/apps/frontend/src/shared/i18n/locales/fr/dialogs.json index 08d8c56f..b98c7104 100644 --- a/apps/frontend/src/shared/i18n/locales/fr/dialogs.json +++ b/apps/frontend/src/shared/i18n/locales/fr/dialogs.json @@ -56,6 +56,17 @@ "delete": "Supprimer le worktree ?", "deleteDescription": "Ceci supprimera définitivement le worktree et toutes les modifications non committées. Cette action est irréversible." }, + "worktreeCleanup": { + "title": "Terminer la tâche", + "hasWorktree": "La tâche \"{{taskTitle}}\" a encore un espace de travail isolé (worktree).", + "willDelete": "Pour marquer cette tâche comme terminée, le worktree et sa branche associée seront supprimés.", + "warning": "Assurez-vous d'avoir fusionné ou sauvegardé les modifications que vous souhaitez conserver avant de continuer.", + "confirm": "Supprimer le worktree et terminer", + "completing": "Finalisation...", + "retry": "Réessayer", + "errorTitle": "Échec du nettoyage", + "errorDescription": "Échec du nettoyage du worktree. Veuillez réessayer." + }, "update": { "title": "Auto Claude", "projectInitialized": "Le projet est initialisé." diff --git a/apps/frontend/src/shared/types/ipc.ts b/apps/frontend/src/shared/types/ipc.ts index 48789df9..f8bd3d9f 100644 --- a/apps/frontend/src/shared/types/ipc.ts +++ b/apps/frontend/src/shared/types/ipc.ts @@ -159,7 +159,7 @@ export interface ElectronAPI { startTask: (taskId: string, options?: TaskStartOptions) => void; stopTask: (taskId: string) => void; submitReview: (taskId: string, approved: boolean, feedback?: string) => Promise; - updateTaskStatus: (taskId: string, status: TaskStatus) => Promise; + updateTaskStatus: (taskId: string, status: TaskStatus, options?: { forceCleanup?: 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 38b50696..c272402a 100644 --- a/apps/frontend/src/shared/types/task.ts +++ b/apps/frontend/src/shared/types/task.ts @@ -301,6 +301,7 @@ export interface WorktreeStatus { worktreePath?: string; branch?: string; baseBranch?: string; + currentProjectBranch?: string; // User's current checked-out branch in main project (merge target) commitCount?: number; filesChanged?: number; additions?: number;