diff --git a/apps/backend/cli/workspace_commands.py b/apps/backend/cli/workspace_commands.py index 5be776a8..0fa510e0 100644 --- a/apps/backend/cli/workspace_commands.py +++ b/apps/backend/cli/workspace_commands.py @@ -536,6 +536,175 @@ def handle_cleanup_worktrees_command(project_dir: Path) -> None: cleanup_all_worktrees(project_dir, confirm=True) +def _detect_conflict_scenario( + project_dir: Path, + conflicting_files: list[str], + spec_branch: str, + base_branch: str, +) -> dict: + """ + Analyze conflicting files to determine the conflict scenario. + + This helps distinguish between: + - 'already_merged': Task changes already identical in target branch + - 'superseded': Target has newer version of same feature + - 'diverged': Standard diverged branches (AI can resolve) + - 'normal_conflict': Actual conflicting changes + + Returns dict with: + - scenario: 'already_merged' | 'superseded' | 'diverged' | 'normal_conflict' + - already_merged_files: files identical in task and target + - details: additional context + """ + if not conflicting_files: + return { + "scenario": "normal_conflict", + "already_merged_files": [], + "details": "No conflicting files to analyze", + } + + already_merged_files = [] + superseded_files = [] + diverged_files = [] + + try: + # Get the merge-base commit + merge_base_result = subprocess.run( + ["git", "merge-base", base_branch, spec_branch], + cwd=project_dir, + capture_output=True, + text=True, + ) + if merge_base_result.returncode != 0: + debug_warning( + MODULE, "Could not find merge base for conflict scenario detection" + ) + return { + "scenario": "normal_conflict", + "already_merged_files": [], + "details": "Could not determine merge base", + } + + merge_base = merge_base_result.stdout.strip() + + for file_path in conflicting_files: + try: + # Get content from spec branch (task's changes) + spec_content_result = subprocess.run( + ["git", "show", f"{spec_branch}:{file_path}"], + cwd=project_dir, + capture_output=True, + text=True, + ) + # Get content from base branch (target) + base_content_result = subprocess.run( + ["git", "show", f"{base_branch}:{file_path}"], + cwd=project_dir, + capture_output=True, + text=True, + ) + # Get content from merge-base (original state) + merge_base_content_result = subprocess.run( + ["git", "show", f"{merge_base}:{file_path}"], + cwd=project_dir, + capture_output=True, + text=True, + ) + + # Check file existence in each ref + spec_exists = spec_content_result.returncode == 0 + base_exists = base_content_result.returncode == 0 + merge_base_exists = merge_base_content_result.returncode == 0 + + if spec_exists and base_exists: + spec_content = spec_content_result.stdout + base_content = base_content_result.stdout + + # If contents are identical, the changes are already merged + if spec_content == base_content: + already_merged_files.append(file_path) + debug( + MODULE, + f"File {file_path}: already merged (identical content)", + ) + elif merge_base_exists: + merge_base_content = merge_base_content_result.stdout + # If base has changed from merge_base but spec matches merge_base, + # the task's changes are superseded by newer changes + if spec_content == merge_base_content: + superseded_files.append(file_path) + debug( + MODULE, + f"File {file_path}: superseded (base has newer changes)", + ) + else: + diverged_files.append(file_path) + debug( + MODULE, + f"File {file_path}: diverged (both branches modified)", + ) + else: + diverged_files.append(file_path) + else: + diverged_files.append(file_path) + + except Exception as e: + debug_warning( + MODULE, f"Error analyzing file {file_path} for scenario: {e}" + ) + diverged_files.append(file_path) + + # Determine overall scenario based on dominant pattern + total_files = len(conflicting_files) + + if len(already_merged_files) == total_files: + scenario = "already_merged" + details = "All conflicting files have identical content in both branches" + elif len(already_merged_files) > total_files / 2: + scenario = "already_merged" + details = f"{len(already_merged_files)} of {total_files} files already have the same content" + elif len(superseded_files) == total_files: + scenario = "superseded" + details = "All task changes have been superseded by newer changes in the target branch" + elif len(superseded_files) > total_files / 2: + scenario = "superseded" + details = ( + f"{len(superseded_files)} of {total_files} files have been superseded" + ) + elif diverged_files: + scenario = "diverged" + details = f"{len(diverged_files)} files have diverged and need AI merge" + else: + scenario = "normal_conflict" + details = "Standard merge conflicts detected" + + debug( + MODULE, + f"Conflict scenario: {scenario}", + already_merged=len(already_merged_files), + superseded=len(superseded_files), + diverged=len(diverged_files), + ) + + return { + "scenario": scenario, + "already_merged_files": already_merged_files, + "superseded_files": superseded_files, + "diverged_files": diverged_files, + "details": details, + } + + except Exception as e: + debug_error(MODULE, f"Error detecting conflict scenario: {e}") + return { + "scenario": "normal_conflict", + "already_merged_files": [], + "superseded_files": [], + "diverged_files": [], + "details": f"Error during analysis: {e}", + } + + def _check_git_merge_conflicts( project_dir: Path, spec_name: str, base_branch: str | None = None ) -> dict: @@ -879,6 +1048,24 @@ def handle_merge_preview_command( f for f in git_conflicts.get("conflicting_files", []) if not is_lock_file(f) ] + # Detect conflict scenario (already_merged, superseded, diverged, normal_conflict) + # This helps the UI show appropriate messaging and actions + conflict_scenario = None + if non_lock_conflicting_files: + conflict_scenario = _detect_conflict_scenario( + project_dir, + non_lock_conflicting_files, + git_conflicts["spec_branch"], + git_conflicts["base_branch"], + ) + debug( + MODULE, + f"Conflict scenario detected: {conflict_scenario.get('scenario')}", + already_merged_files=len( + conflict_scenario.get("already_merged_files", []) + ), + ) + # Use git diff file count as the authoritative totalFiles count # The semantic tracker may not track all files (e.g., test files, config files) # but we want to show the user all files that will be merged @@ -952,6 +1139,16 @@ def handle_merge_preview_command( # Path-mapped files that need AI merge due to renames "pathMappedAIMerges": path_mapped_ai_merges, "totalRenames": len(path_mappings), + # Conflict scenario detection for better UX messaging + "scenario": conflict_scenario.get("scenario") + if conflict_scenario + else None, + "alreadyMergedFiles": conflict_scenario.get("already_merged_files", []) + if conflict_scenario + else [], + "scenarioMessage": conflict_scenario.get("details") + if conflict_scenario + else None, }, "summary": { # Use git diff count, not semantic tracker count diff --git a/apps/backend/core/workspace.py b/apps/backend/core/workspace.py index 8e3552c0..3551b41b 100644 --- a/apps/backend/core/workspace.py +++ b/apps/backend/core/workspace.py @@ -121,6 +121,7 @@ from merge import ( FileTimelineTracker, MergeOrchestrator, ) +from merge.progress import MergeProgressCallback, MergeProgressStage, emit_progress MODULE = "workspace" @@ -145,6 +146,26 @@ MODULE = "workspace" # - _heuristic_merge +def _create_merge_progress_callback() -> MergeProgressCallback | None: + """ + Create a progress callback for merge operations when running as a subprocess. + + Returns emit_progress (writing JSON to stdout) only when stdout is piped + (i.e., running as a subprocess from the Electron frontend). Returns None + when running interactively in a terminal to avoid polluting CLI output. + + This function must be called at runtime (not at import time) to ensure + sys.stdout state is accurate. + """ + import sys + + # Only emit progress JSON when stdout is piped (subprocess mode). + # In interactive CLI mode (TTY), progress JSON would clutter the output. + if not sys.stdout.isatty(): + return emit_progress + return None + + def merge_existing_build( project_dir: Path, spec_name: str, @@ -252,10 +273,11 @@ def merge_existing_build( had_conflicts = stats.get("conflicts_resolved", 0) > 0 ai_assisted = stats.get("ai_assisted", 0) > 0 direct_copy = stats.get("direct_copy", False) + git_merge_used = stats.get("git_merge", False) - 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 + if had_conflicts or ai_assisted or direct_copy or git_merge_used: + # AI resolved conflicts, assisted with merges, git merge was used, or direct copy was used + # Changes are already written and staged - no need for additional git merge _print_merge_success( no_commit, stats, spec_name=spec_name, keep_worktree=True ) @@ -402,9 +424,20 @@ def _try_smart_merge_inner( no_commit=no_commit, ) + # Create progress callback for subprocess mode (Electron frontend). + # Only emits JSON to stdout when piped, not in interactive CLI. + progress_callback = _create_merge_progress_callback() + try: print(muted(" Analyzing changes with intent-aware merge...")) + if progress_callback is not None: + progress_callback( + MergeProgressStage.ANALYZING, + 0, + "Starting merge analysis", + ) + # Capture worktree state in FileTimelineTracker before merge try: timeline_tracker = FileTimelineTracker(project_dir) @@ -440,6 +473,13 @@ def _try_smart_merge_inner( ) # Check for git-level conflicts first (branch divergence) + if progress_callback is not None: + progress_callback( + MergeProgressStage.DETECTING_CONFLICTS, + 25, + "Checking for git-level conflicts", + ) + debug(MODULE, "Checking for git-level conflicts") git_conflicts = _check_git_conflicts(project_dir, spec_name) @@ -492,12 +532,11 @@ def _try_smart_merge_inner( # If rebase succeeded and now there are no conflicts, # the diverged_but_no_conflicts path will handle the merge else: - # Rebase failed - continue with conflict resolution as before - # The AI resolver will handle the conflicts - print( - warning( - " Rebase encountered issues, using AI conflict resolution..." - ) + # Rebase failed (likely due to worktree lock) - continue with merge + # Git merge or AI resolver will handle it depending on conflict state + debug( + MODULE, + "Rebase skipped or failed, continuing with merge flow", ) if git_conflicts.get("has_conflicts"): @@ -518,6 +557,18 @@ def _try_smart_merge_inner( num_conflicts=len(git_conflicts.get("conflicting_files", [])), ) + if progress_callback is not None: + progress_callback( + MergeProgressStage.RESOLVING, + 50, + f"Resolving {len(git_conflicts.get('conflicting_files', []))} conflicting files with AI", + { + "conflicts_found": len( + git_conflicts.get("conflicting_files", []) + ) + }, + ) + # Try to resolve git conflicts with AI resolution_result = _resolve_git_conflicts_with_ai( project_dir, @@ -535,6 +586,19 @@ def _try_smart_merge_inner( resolved_files=resolution_result.get("resolved_files", []), stats=resolution_result.get("stats", {}), ) + + if progress_callback is not None: + stats = resolution_result.get("stats", {}) + progress_callback( + MergeProgressStage.COMPLETE, + 100, + "Merge complete", + { + "conflicts_found": stats.get("conflicts_resolved", 0), + "conflicts_resolved": stats.get("conflicts_resolved", 0), + }, + ) + return resolution_result else: # AI couldn't resolve all conflicts @@ -547,6 +611,19 @@ def _try_smart_merge_inner( resolved_files=resolution_result.get("resolved_files", []), error=resolution_result.get("error"), ) + + if progress_callback is not None: + progress_callback( + MergeProgressStage.ERROR, + 0, + "Some conflicts could not be resolved", + { + "conflicts_found": len( + resolution_result.get("remaining_conflicts", []) + ), + }, + ) + return { "success": False, "conflicts": resolution_result.get("remaining_conflicts", []), @@ -555,148 +632,81 @@ def _try_smart_merge_inner( "error": resolution_result.get("error"), } - # Check if branches diverged but no actual conflicts (can do direct copy) + # Check if branches diverged but no actual conflicts (use git merge) if git_conflicts.get("diverged_but_no_conflicts"): - debug(MODULE, "Branches diverged but no conflicts - doing direct file copy") + debug(MODULE, "Branches diverged but no conflicts - using git merge") print(muted(" Branches diverged but no conflicts detected")) - print(muted(" Copying changed files directly from worktree...")) + print(muted(" Using git merge to combine changes...")) - # 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 = run_git( - ["merge-base", base_branch, spec_branch], + # Use git merge --no-commit to combine changes from both branches + # Since merge-tree confirmed no conflicts, this should succeed cleanly + merge_result = run_git( + ["merge", "--no-commit", "--no-ff", spec_branch], cwd=project_dir, ) - 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 + if merge_result.returncode == 0: + # Merge succeeded - get list of files that were merged + # Use git diff --cached to see what's staged + diff_result = run_git( + ["diff", "--cached", "--name-only"], + cwd=project_dir, + ) + merged_files = [ + f.strip() + for f in diff_result.stdout.splitlines() + if f.strip() and not _is_auto_claude_file(f.strip()) + ] + + debug_success( + MODULE, + "Git merge succeeded", + merged_files_count=len(merged_files), ) - 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 + for file_path in merged_files: + print(success(f" ✓ {file_path}")) - 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: - add_result = run_git( - ["add"] + files_to_stage, - cwd=project_dir, + if progress_callback is not None: + progress_callback( + MergeProgressStage.COMPLETE, + 100, + f"Git merge complete ({len(merged_files)} files)", ) - if add_result.returncode != 0: - debug_warning( - MODULE, - f"Failed to stage files for direct copy: {add_result.stderr}", - ) - # Return failure - files were written but not staged - return { - "success": False, - "error": f"Failed to stage files: {add_result.stderr}", - "resolved_files": [], - } - # Build result - check for skipped files to detect partial merges - result = { - "success": len(skipped_files) == 0, - "resolved_files": resolved_files, + return { + "success": True, + "resolved_files": merged_files, "stats": { - "files_merged": len(resolved_files), + "files_merged": len(merged_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), + "auto_merged": len(merged_files), + "git_merge": True, # Flag indicating git merge was used }, } - 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 + # Merge failed unexpectedly - abort and fall back to semantic analysis debug_warning( MODULE, - "Could not find merge-base between branches - falling back to semantic analysis", + "Git merge failed unexpectedly despite no conflicts detected", + stderr=merge_result.stderr[:500] if merge_result.stderr else "", + ) + # Abort the merge to restore clean state + abort_result = run_git(["merge", "--abort"], cwd=project_dir) + if abort_result.returncode != 0: + debug_error( + MODULE, + "Failed to abort merge - repo may be in inconsistent state", + stderr=abort_result.stderr, + ) + return None # Trigger fallback to avoid operating on inconsistent state + print( + warning( + " Git merge failed unexpectedly, falling back to semantic analysis..." + ) ) # No git conflicts - proceed with semantic analysis @@ -725,6 +735,14 @@ def _try_smart_merge_inner( # All conflicts can be auto-merged or no conflicts print(muted(" All changes compatible, proceeding with merge...")) + + if progress_callback is not None: + progress_callback( + MergeProgressStage.COMPLETE, + 100, + f"Analysis complete ({files_to_merge} files compatible)", + ) + return { "success": True, "stats": { @@ -737,6 +755,13 @@ def _try_smart_merge_inner( # If smart merge fails, fall back to git import traceback + if progress_callback is not None: + progress_callback( + MergeProgressStage.ERROR, + 0, + f"Smart merge error: {e}", + ) + print(muted(f" Smart merge error: {e}")) traceback.print_exc() return None @@ -748,14 +773,11 @@ def _rebase_spec_branch( base_branch: str, ) -> bool: """ - Rebase the spec branch onto the latest base branch. + Attempt to rebase the spec branch onto the latest base branch. - This performs an automatic rebase of the spec branch onto the current - base branch (main/develop) to bring it up to date before merging. - If conflicts occur during rebase, the function aborts and returns False - so that the caller can fall back to AI conflict resolution. - - The function preserves the current HEAD by restoring it after completion. + NOTE: This will fail if the spec branch is checked out in a worktree, + which is the normal case. The caller should handle failure gracefully + by falling back to git merge or AI conflict resolution. Args: project_dir: The project directory @@ -764,22 +786,36 @@ def _rebase_spec_branch( Returns: True if rebase succeeded cleanly or branch was already up-to-date, - False if rebase failed due to conflicts or other errors (aborted, no ref movement) + False if rebase failed (worktree lock, conflicts, or other errors) """ spec_branch = f"auto-claude/{spec_name}" debug( MODULE, - "Rebasing spec branch", + "Attempting to rebase spec branch", spec_branch=spec_branch, base_branch=base_branch, ) - # Save original branch to restore after rebase (HIGH: prevents leaving repo on spec branch) + # Check if spec branch is used by a worktree (common case) + # In this case, we can't checkout/rebase from the main repo + worktree_list_result = run_git(["worktree", "list", "--porcelain"], cwd=project_dir) + if worktree_list_result.returncode == 0: + # Check if spec_branch is in use by a worktree + output = worktree_list_result.stdout + if f"branch refs/heads/{spec_branch}" in output: + debug( + MODULE, + "Spec branch is checked out in a worktree - skipping rebase", + spec_branch=spec_branch, + ) + # This is expected - return False to let caller use git merge instead + return False + + # Save original branch to restore after rebase original_branch_result = run_git( ["rev-parse", "--abbrev-ref", "HEAD"], cwd=project_dir ) - # Check returncode and validate stdout before using original_branch if original_branch_result.returncode != 0: debug_error( MODULE, @@ -795,7 +831,6 @@ def _rebase_spec_branch( ) return False - # Save current state for recovery # Get the current commit of spec_branch before rebase before_commit_result = run_git(["rev-parse", spec_branch], cwd=project_dir) if before_commit_result.returncode != 0: @@ -804,8 +839,6 @@ def _rebase_spec_branch( "Could not get spec branch commit before rebase", stderr=before_commit_result.stderr, ) - # Restore original branch before returning - run_git(["checkout", original_branch], cwd=project_dir) return False before_commit = before_commit_result.stdout.strip() @@ -813,22 +846,18 @@ def _rebase_spec_branch( print(muted(f" Rebasing {spec_branch} onto {base_branch}...")) try: - # Perform the rebase using safe/standard invocation: - # 1. Checkout the spec branch first - # 2. Run standard rebase (no strategy options - let conflicts stop the rebase) - # If conflicts occur, we'll abort and let AI handle them during merge + # Try to checkout the spec branch checkout_result = run_git(["checkout", spec_branch], cwd=project_dir) if checkout_result.returncode != 0: - debug_error( + # Checkout failed - likely due to worktree lock + debug( MODULE, - "Could not checkout spec branch for rebase", - stderr=checkout_result.stderr, + "Could not checkout spec branch for rebase (likely worktree lock)", + stderr=checkout_result.stderr[:200] if checkout_result.stderr else "", ) return False - # Run standard rebase - will stop on conflicts so we can detect them - # Git syntax: git rebase [options] - # where is the branch to rebase onto + # Run standard rebase rebase_result = run_git( ["rebase", base_branch], cwd=project_dir, @@ -838,9 +867,6 @@ def _rebase_spec_branch( # Rebase failed - check if it was due to conflicts status_result = run_git(["status", "--porcelain"], cwd=project_dir) - # MEDIUM: Properly parse git status output for conflict markers - # Git status --porcelain uses two-character status codes: - # UU = both modified, AA = both added, DD = both deleted, etc. has_unmerged = any( line[:2] in ("UU", "AA", "DD", "AU", "UA", "DU", "UD") for line in status_result.stdout.splitlines() @@ -848,7 +874,6 @@ def _rebase_spec_branch( ) # Abort the rebase to return to clean state - # NEW-002: If abort fails, immediately return False (repo in bad state) abort_result = run_git(["rebase", "--abort"], cwd=project_dir) if abort_result.returncode != 0: debug_error( @@ -856,19 +881,16 @@ def _rebase_spec_branch( "Failed to abort rebase - repo may be in inconsistent state", stderr=abort_result.stderr, ) - return False # Abort failed - cannot safely continue - - if has_unmerged: - # Rebase failed due to conflicts - we aborted, so no ref movement happened - debug_warning( - MODULE, - "Rebase encountered conflicts - aborted, will use AI conflict resolution", - stderr=rebase_result.stderr[:200] if rebase_result.stderr else "", - ) - # Return False since we aborted - no rebase occurred, caller should use AI return False - # Other error (not conflict-related) + if has_unmerged: + debug_warning( + MODULE, + "Rebase encountered conflicts - aborted, will use alternative merge", + stderr=rebase_result.stderr[:200] if rebase_result.stderr else "", + ) + return False + debug_error( MODULE, "Rebase failed with unexpected error", @@ -882,9 +904,7 @@ def _rebase_spec_branch( if after_commit_result.returncode == 0: after_commit_hash = after_commit_result.stdout.strip() - # Verify the branch actually moved (commit changed) if before_commit == after_commit_hash: - # MEDIUM: Branch already up-to-date is a success condition, not failure debug( MODULE, "Branch already up-to-date, no rebase needed", @@ -904,8 +924,7 @@ def _rebase_spec_branch( debug_error(MODULE, "Could not verify spec branch commit after rebase") return False finally: - # HIGH: Always restore original branch, even on error/exception - # NEW-001: Log restoration failure (cannot modify return from finally block) + # Always restore original branch if original_branch: restore_result = run_git(["checkout", original_branch], cwd=project_dir) if restore_result.returncode != 0: @@ -914,8 +933,6 @@ def _rebase_spec_branch( f"Failed to restore original branch '{original_branch}'", stderr=restore_result.stderr, ) - # Note: Cannot modify return value from finally block, - # but restoration failure is rare and non-critical (user can manually switch back) def _check_git_conflicts(project_dir: Path, spec_name: str) -> dict: diff --git a/apps/backend/merge/conflict_resolver.py b/apps/backend/merge/conflict_resolver.py index fdbe739d..542e4bf2 100644 --- a/apps/backend/merge/conflict_resolver.py +++ b/apps/backend/merge/conflict_resolver.py @@ -17,6 +17,7 @@ import logging from .ai_resolver import AIResolver from .auto_merger import AutoMerger, MergeContext from .file_merger import apply_ai_merge, extract_location_content +from .progress import MergeProgressCallback, MergeProgressStage from .types import ( ConflictRegion, ConflictSeverity, @@ -60,6 +61,7 @@ class ConflictResolver: baseline_content: str, task_snapshots: list[TaskSnapshot], conflicts: list[ConflictRegion], + progress_callback: MergeProgressCallback | None = None, ) -> MergeResult: """ Resolve conflicts using AutoMerger and AIResolver. @@ -69,6 +71,8 @@ class ConflictResolver: baseline_content: Original file content task_snapshots: Snapshots from all tasks modifying this file conflicts: List of detected conflicts + progress_callback: Optional callback for emitting per-conflict + resolution progress with details about current file and conflict count Returns: MergeResult with resolution details @@ -78,8 +82,22 @@ class ConflictResolver: remaining: list[ConflictRegion] = [] ai_calls = 0 tokens_used = 0 + total_conflicts = len(conflicts) - for conflict in conflicts: + for idx, conflict in enumerate(conflicts): + if progress_callback: + # Emit per-conflict progress within the resolving stage (50-75%) + conflict_percent = 50 + int((idx / max(total_conflicts, 1)) * 25) + progress_callback( + stage=MergeProgressStage.RESOLVING, + percent=conflict_percent, + message=f"Resolving conflict {idx + 1}/{total_conflicts} in {file_path}", + details={ + "current_file": file_path, + "conflicts_found": total_conflicts, + "conflicts_resolved": len(resolved), + }, + ) # Try auto-merge first if conflict.can_auto_merge and conflict.merge_strategy: context = MergeContext( diff --git a/apps/backend/merge/merge_pipeline.py b/apps/backend/merge/merge_pipeline.py index 8a2cacaa..3ea2a49c 100644 --- a/apps/backend/merge/merge_pipeline.py +++ b/apps/backend/merge/merge_pipeline.py @@ -18,6 +18,7 @@ import logging from .conflict_detector import ConflictDetector from .conflict_resolver import ConflictResolver from .file_merger import apply_single_task_changes, combine_non_conflicting_changes +from .progress import MergeProgressCallback, MergeProgressStage from .types import ( ChangeType, FileAnalysis, @@ -57,6 +58,7 @@ class MergePipeline: file_path: str, baseline_content: str, task_snapshots: list[TaskSnapshot], + progress_callback: MergeProgressCallback | None = None, ) -> MergeResult: """ Merge changes from multiple tasks for a single file. @@ -65,6 +67,8 @@ class MergePipeline: file_path: Path to the file baseline_content: Original baseline content task_snapshots: Snapshots from tasks that modified this file + progress_callback: Optional callback for emitting per-file progress + within the 'resolving' stage (50-75% range) Returns: MergeResult with merged content or conflict info @@ -72,6 +76,14 @@ class MergePipeline: task_ids = [s.task_id for s in task_snapshots] logger.info(f"Merging {file_path} with {len(task_snapshots)} task(s)") + if progress_callback: + progress_callback( + stage=MergeProgressStage.RESOLVING, + percent=50, + message=f"Merging file: {file_path}", + details={"current_file": file_path}, + ) + # If only one task modified the file, no conflict possible if len(task_snapshots) == 1: snapshot = task_snapshots[0] @@ -119,6 +131,7 @@ class MergePipeline: baseline_content=baseline_content, task_snapshots=task_snapshots, conflicts=conflicts, + progress_callback=progress_callback, ) def _build_task_analyses( diff --git a/apps/backend/merge/orchestrator.py b/apps/backend/merge/orchestrator.py index bc481313..185e946b 100644 --- a/apps/backend/merge/orchestrator.py +++ b/apps/backend/merge/orchestrator.py @@ -33,6 +33,7 @@ from .merge_pipeline import MergePipeline # Re-export models for backwards compatibility from .models import MergeReport, MergeStats, TaskMergeRequest +from .progress import MergeProgressCallback, MergeProgressStage from .semantic_analyzer import SemanticAnalyzer from .types import ( ConflictRegion, @@ -260,6 +261,7 @@ class MergeOrchestrator: task_id: str, worktree_path: Path | None = None, target_branch: str = "main", + progress_callback: MergeProgressCallback | None = None, ) -> MergeReport: """ Merge a single task's changes into the target branch. @@ -268,6 +270,8 @@ class MergeOrchestrator: task_id: The task identifier worktree_path: Path to the task's worktree (auto-detected if not provided) target_branch: Branch to merge into + progress_callback: Optional callback for progress updates. + Called with (stage, percent, message, details) at key pipeline stages. Returns: MergeReport with results @@ -284,7 +288,20 @@ class MergeOrchestrator: report = MergeReport(started_at=datetime.now(), tasks_merged=[task_id]) start_time = datetime.now() + def _emit( + stage: MergeProgressStage, + percent: int, + message: str, + details: dict[str, Any] | None = None, + ) -> None: + """Emit progress if a callback is provided.""" + if progress_callback is not None: + progress_callback(stage, percent, message, details) + try: + # --- ANALYZING stage (0-25%) --- + _emit(MergeProgressStage.ANALYZING, 0, "Starting merge analysis") + # Find worktree if not provided if worktree_path is None: debug_detailed(MODULE, "Auto-detecting worktree path...") @@ -293,16 +310,23 @@ class MergeOrchestrator: debug_error(MODULE, f"Could not find worktree for task {task_id}") report.success = False report.error = f"Could not find worktree for task {task_id}" + _emit( + MergeProgressStage.ERROR, + 0, + f"Could not find worktree for task {task_id}", + ) return report debug_detailed(MODULE, f"Found worktree: {worktree_path}") # Ensure evolution data is up to date + _emit(MergeProgressStage.ANALYZING, 5, "Loading file evolution data") debug(MODULE, "Refreshing evolution data from git...") self.evolution_tracker.refresh_from_git( task_id, worktree_path, target_branch=target_branch ) # Get files modified by this task + _emit(MergeProgressStage.ANALYZING, 15, "Running semantic analysis") modifications = self.evolution_tracker.get_task_modifications(task_id) debug( MODULE, @@ -312,11 +336,38 @@ class MergeOrchestrator: if not modifications: debug_warning(MODULE, f"No modifications found for task {task_id}") logger.info(f"No modifications found for task {task_id}") + _emit( + MergeProgressStage.COMPLETE, + 100, + "No modifications found", + ) report.completed_at = datetime.now() return report - # Process each modified file - for file_path, snapshot in modifications: + _emit( + MergeProgressStage.ANALYZING, + 25, + f"Found {len(modifications)} modified files", + ) + + # --- DETECTING_CONFLICTS stage (25-50%) --- + _emit( + MergeProgressStage.DETECTING_CONFLICTS, + 25, + "Detecting conflicts", + ) + + # --- RESOLVING stage (50-75%) --- + total_files = len(modifications) + for idx, (file_path, snapshot) in enumerate(modifications): + file_percent = 50 + int((idx / max(total_files, 1)) * 25) + _emit( + MergeProgressStage.RESOLVING, + file_percent, + f"Merging file {idx + 1}/{total_files}", + {"current_file": file_path}, + ) + debug_detailed( MODULE, f"Processing file: {file_path}", @@ -349,13 +400,31 @@ class MergeOrchestrator: file=file_path, ) + # --- VALIDATING stage (75-100%) --- + _emit( + MergeProgressStage.VALIDATING, + 75, + "Validating merge results", + { + "conflicts_found": report.stats.conflicts_detected, + "conflicts_resolved": report.stats.conflicts_auto_resolved, + }, + ) + report.success = report.stats.files_failed == 0 + _emit( + MergeProgressStage.VALIDATING, + 90, + "Validation complete", + ) + except Exception as e: debug_error(MODULE, f"Merge failed for task {task_id}", error=str(e)) logger.exception(f"Merge failed for task {task_id}") report.success = False report.error = str(e) + _emit(MergeProgressStage.ERROR, 0, f"Merge failed: {e}") report.completed_at = datetime.now() report.stats.duration_seconds = ( @@ -366,6 +435,18 @@ class MergeOrchestrator: if not self.dry_run: self._save_report(report, task_id) + # --- COMPLETE stage (100%) --- + if report.success: + _emit( + MergeProgressStage.COMPLETE, + 100, + f"Merge complete for {task_id}", + { + "conflicts_found": report.stats.conflicts_detected, + "conflicts_resolved": report.stats.conflicts_auto_resolved, + }, + ) + debug_success( MODULE, f"Merge complete for {task_id}", @@ -382,6 +463,7 @@ class MergeOrchestrator: self, requests: list[TaskMergeRequest], target_branch: str = "main", + progress_callback: MergeProgressCallback | None = None, ) -> MergeReport: """ Merge multiple tasks' changes. @@ -392,6 +474,8 @@ class MergeOrchestrator: Args: requests: List of merge requests (one per task) target_branch: Branch to merge into + progress_callback: Optional callback for progress updates. + Called with (stage, percent, message, details) at key pipeline stages. Returns: MergeReport with combined results @@ -402,11 +486,33 @@ class MergeOrchestrator: ) start_time = datetime.now() + def _emit( + stage: MergeProgressStage, + percent: int, + message: str, + details: dict[str, Any] | None = None, + ) -> None: + """Emit progress if a callback is provided.""" + if progress_callback is not None: + progress_callback(stage, percent, message, details) + try: + # --- ANALYZING stage (0-25%) --- + _emit( + MergeProgressStage.ANALYZING, + 0, + f"Starting merge analysis for {len(requests)} tasks", + ) + # Sort by priority (higher first) requests = sorted(requests, key=lambda r: -r.priority) # Refresh evolution data for all tasks + _emit( + MergeProgressStage.ANALYZING, + 5, + "Loading file evolution data", + ) for request in requests: if request.worktree_path and request.worktree_path.exists(): self.evolution_tracker.refresh_from_git( @@ -416,11 +522,38 @@ class MergeOrchestrator: ) # Find all files modified by any task + _emit( + MergeProgressStage.ANALYZING, + 15, + "Running semantic analysis", + ) task_ids = [r.task_id for r in requests] file_tasks = self.evolution_tracker.get_files_modified_by_tasks(task_ids) - # Process each file - for file_path, modifying_tasks in file_tasks.items(): + _emit( + MergeProgressStage.ANALYZING, + 25, + f"Found {len(file_tasks)} files to merge", + ) + + # --- DETECTING_CONFLICTS stage (25-50%) --- + _emit( + MergeProgressStage.DETECTING_CONFLICTS, + 25, + "Detecting conflicts across tasks", + ) + + # --- RESOLVING stage (50-75%) --- + total_files = len(file_tasks) + for idx, (file_path, modifying_tasks) in enumerate(file_tasks.items()): + file_percent = 50 + int((idx / max(total_files, 1)) * 25) + _emit( + MergeProgressStage.RESOLVING, + file_percent, + f"Merging file {idx + 1}/{total_files}", + {"current_file": file_path}, + ) + # Get snapshots from all tasks that modified this file evolution = self.evolution_tracker.get_file_evolution(file_path) if not evolution: @@ -466,8 +599,25 @@ class MergeOrchestrator: report.file_results[file_path] = result self._update_stats(report.stats, result) + # --- VALIDATING stage (75-100%) --- + _emit( + MergeProgressStage.VALIDATING, + 75, + "Validating merge results", + { + "conflicts_found": report.stats.conflicts_detected, + "conflicts_resolved": report.stats.conflicts_auto_resolved, + }, + ) + report.success = report.stats.files_failed == 0 + _emit( + MergeProgressStage.VALIDATING, + 90, + "Validation complete", + ) + except Exception as e: debug_error( MODULE, @@ -478,6 +628,7 @@ class MergeOrchestrator: logger.exception("Merge failed") report.success = False report.error = str(e) + _emit(MergeProgressStage.ERROR, 0, f"Merge failed: {e}") report.completed_at = datetime.now() report.stats.duration_seconds = ( @@ -489,6 +640,18 @@ class MergeOrchestrator: timestamp = datetime.now().strftime("%Y%m%d_%H%M%S") self._save_report(report, f"multi_{timestamp}") + # --- COMPLETE stage (100%) --- + if report.success: + _emit( + MergeProgressStage.COMPLETE, + 100, + f"Merge complete for {len(requests)} tasks", + { + "conflicts_found": report.stats.conflicts_detected, + "conflicts_resolved": report.stats.conflicts_auto_resolved, + }, + ) + return report def _merge_file( diff --git a/apps/backend/merge/progress.py b/apps/backend/merge/progress.py new file mode 100644 index 00000000..fd114337 --- /dev/null +++ b/apps/backend/merge/progress.py @@ -0,0 +1,105 @@ +""" +Merge Progress Emission +======================= + +Structured progress event emission for the merge pipeline. + +This module provides the progress reporting infrastructure used by the +merge orchestrator to communicate real-time status updates to the +Electron frontend via stdout JSON lines. + +Progress events are emitted as JSON lines to stdout with type='progress', +allowing the frontend to parse them separately from the final merge result. + +Components: +- MergeProgressStage: Enum of pipeline stages +- MergeProgressCallback: Protocol for type-safe callback threading +- emit_progress: Function to emit structured progress events to stdout +""" + +from __future__ import annotations + +import json +from enum import Enum +from typing import Any, Protocol + + +class MergeProgressStage(Enum): + """ + Stages of the merge pipeline. + + Each stage corresponds to a phase of the merge process and maps + to a percentage range for progress reporting: + - ANALYZING: 0-25% — Loading file evolution, running semantic analysis + - DETECTING_CONFLICTS: 25-50% — Conflict detection and compatibility checks + - RESOLVING: 50-75% — Auto-merge and AI resolution of conflicts + - VALIDATING: 75-100% — Final validation of merged results + - COMPLETE: 100% — Merge finished successfully + - ERROR: N/A — Merge failed with an error + """ + + ANALYZING = "analyzing" + DETECTING_CONFLICTS = "detecting_conflicts" + RESOLVING = "resolving" + VALIDATING = "validating" + COMPLETE = "complete" + ERROR = "error" + + +class MergeProgressCallback(Protocol): + """ + Protocol for type-safe progress callback threading. + + Implementations receive structured progress updates from the merge + pipeline stages and can forward them to any output channel. + + Args: + stage: Current pipeline stage + percent: Progress percentage (0-100) + message: Human-readable status message + details: Optional additional context (conflicts_found, conflicts_resolved, current_file) + """ + + def __call__( + self, + stage: MergeProgressStage, + percent: int, + message: str, + details: dict[str, Any] | None = None, + ) -> None: ... + + +def emit_progress( + stage: MergeProgressStage, + percent: int, + message: str, + details: dict[str, Any] | None = None, +) -> None: + """ + Emit a progress event as a JSON line to stdout. + + The Electron main process parses these JSON lines from the merge + subprocess stdout and forwards them to the renderer via IPC. + + Args: + stage: Current pipeline stage + percent: Progress percentage (0-100), clamped to valid range + message: Human-readable status message + details: Optional dict with additional context. Supported keys: + - conflicts_found (int): Number of conflicts detected + - conflicts_resolved (int): Number of conflicts resolved so far + - current_file (str): File currently being processed + """ + percent = max(0, min(100, percent)) + + event: dict[str, Any] = { + "type": "progress", + "stage": stage.value, + "percent": percent, + "message": message, + } + + if details: + event["details"] = details + + print(json.dumps(event), flush=True) diff --git a/apps/backend/prompts/github/pr_parallel_orchestrator.md b/apps/backend/prompts/github/pr_parallel_orchestrator.md index 5efee4e6..88c8948f 100644 --- a/apps/backend/prompts/github/pr_parallel_orchestrator.md +++ b/apps/backend/prompts/github/pr_parallel_orchestrator.md @@ -2,6 +2,17 @@ You are an expert PR reviewer orchestrating a comprehensive, parallel code review. Your role is to analyze the PR, delegate to specialized review agents, and synthesize their findings into a final verdict. +## CRITICAL: Tool Execution Strategy + +**IMPORTANT: Execute tool calls ONE AT A TIME, waiting for each result before making the next call.** + +When you need to use multiple tools (Read, Grep, Glob, Task): +- ✅ Make ONE tool call, wait for the result +- ✅ Process the result, then make the NEXT tool call +- ❌ Do NOT make multiple tool calls in a single response + +**Why this matters:** Parallel tool execution can cause API errors when some tools fail while others succeed. Sequential execution ensures reliable operation and proper error handling. + ## Core Principle **YOU decide which agents to invoke based on YOUR analysis of the PR.** There are no programmatic rules - you evaluate the PR's content, complexity, and risk areas, then delegate to the appropriate specialists. diff --git a/apps/backend/requirements.txt b/apps/backend/requirements.txt index ff969cf8..c8f4659f 100644 --- a/apps/backend/requirements.txt +++ b/apps/backend/requirements.txt @@ -1,7 +1,7 @@ # Auto-Build Framework Dependencies -# SDK 0.1.22+ required for custom subagent support (bundles CLI 2.1.19+) -# Earlier versions bundled CLI 2.1.9 which didn't properly register --agents flag -claude-agent-sdk>=0.1.22 +# SDK 0.1.25+ required for improved tool use concurrency handling +# Earlier versions had 400 errors when tool_use blocks had partial failures +claude-agent-sdk>=0.1.25 python-dotenv>=1.0.0 # TOML parsing fallback for Python < 3.11 diff --git a/apps/backend/runners/github/bot_detection.py b/apps/backend/runners/github/bot_detection.py index 841769cb..9e8d52c5 100644 --- a/apps/backend/runners/github/bot_detection.py +++ b/apps/backend/runners/github/bot_detection.py @@ -10,6 +10,8 @@ Key Features: - Skips re-reviewing bot commits - Implements "cooling off" period to prevent rapid re-reviews - Tracks reviewed commits to avoid duplicate reviews +- In-progress tracking to prevent concurrent reviews +- Stale review detection with automatic cleanup Usage: detector = BotDetector(bot_token="ghp_...") @@ -20,8 +22,16 @@ Usage: print(f"Skipping PR: {reason}") return + # Mark review as started (prevents concurrent reviews) + detector.mark_review_started(pr_number) + + # Perform review... + # After successful review, mark as reviewed detector.mark_reviewed(pr_number, head_sha) + + # Or if review failed: + detector.mark_review_finished(pr_number, success=False) """ from __future__ import annotations @@ -55,11 +65,15 @@ class BotDetectionState: # PR number -> last review timestamp (ISO format) last_review_times: dict[int, str] = field(default_factory=dict) + # PR number -> in-progress review start time (ISO format) + in_progress_reviews: dict[int, str] = field(default_factory=dict) + def to_dict(self) -> dict: """Convert to dictionary for JSON serialization.""" return { "reviewed_commits": self.reviewed_commits, "last_review_times": self.last_review_times, + "in_progress_reviews": self.in_progress_reviews, } @classmethod @@ -68,6 +82,7 @@ class BotDetectionState: return cls( reviewed_commits=data.get("reviewed_commits", {}), last_review_times=data.get("last_review_times", {}), + in_progress_reviews=data.get("in_progress_reviews", {}), ) def save(self, state_dir: Path) -> None: @@ -104,11 +119,16 @@ class BotDetector: - 1-minute cooling off period between reviews of same PR (for testing) - Tracks reviewed commit SHAs to avoid duplicate reviews - Identifies bot user from token to skip bot-authored content + - In-progress tracking to prevent concurrent reviews + - Stale review detection (30-minute timeout) """ # Cooling off period in minutes (reduced to 1 for testing large PRs) COOLING_OFF_MINUTES = 1 + # Timeout for in-progress reviews in minutes (after this, review is considered stale/crashed) + IN_PROGRESS_TIMEOUT_MINUTES = 30 + def __init__( self, state_dir: Path, @@ -301,6 +321,104 @@ class BotDetector: reviewed = self.state.reviewed_commits.get(str(pr_number), []) return commit_sha in reviewed + def is_review_in_progress(self, pr_number: int) -> tuple[bool, str]: + """ + Check if a review is currently in progress for this PR. + + Also detects stale reviews (started > IN_PROGRESS_TIMEOUT_MINUTES ago). + + Args: + pr_number: The PR number + + Returns: + Tuple of (is_in_progress, reason_message) + """ + pr_key = str(pr_number) + start_time_str = self.state.in_progress_reviews.get(pr_key) + + if not start_time_str: + return False, "" + + try: + start_time = datetime.fromisoformat(start_time_str) + time_elapsed = datetime.now() - start_time + + # Check if review is stale (timeout exceeded) + if time_elapsed > timedelta(minutes=self.IN_PROGRESS_TIMEOUT_MINUTES): + # Mark as stale and clear the in-progress state + print( + f"[BotDetector] Review for PR #{pr_number} is stale " + f"(started {int(time_elapsed.total_seconds() / 60)}m ago, " + f"timeout: {self.IN_PROGRESS_TIMEOUT_MINUTES}m) - clearing in-progress state", + file=sys.stderr, + ) + self.mark_review_finished(pr_number, success=False) + return False, "" + + # Review is actively in progress + minutes_elapsed = int(time_elapsed.total_seconds() / 60) + reason = f"Review already in progress (started {minutes_elapsed}m ago)" + print(f"[BotDetector] PR #{pr_number}: {reason}", file=sys.stderr) + return True, reason + + except (ValueError, TypeError) as e: + print( + f"[BotDetector] Error parsing in-progress start time: {e}", + file=sys.stderr, + ) + # Clear invalid state + self.mark_review_finished(pr_number, success=False) + return False, "" + + def mark_review_started(self, pr_number: int) -> None: + """ + Mark a review as started for this PR. + + This should be called when beginning a review to prevent concurrent reviews. + + Args: + pr_number: The PR number + """ + pr_key = str(pr_number) + + # Record start time + self.state.in_progress_reviews[pr_key] = datetime.now().isoformat() + + # Save state + self.state.save(self.state_dir) + + logger.info(f"[BotDetector] Marked PR #{pr_number} review as started") + print(f"[BotDetector] Started review for PR #{pr_number}", file=sys.stderr) + + def mark_review_finished(self, pr_number: int, success: bool = True) -> None: + """ + Mark a review as finished for this PR. + + This clears the in-progress state. Should be called when review completes + (successfully or with error) or when detected as stale. + + Args: + pr_number: The PR number + success: Whether the review completed successfully + """ + pr_key = str(pr_number) + + # Clear in-progress state + if pr_key in self.state.in_progress_reviews: + del self.state.in_progress_reviews[pr_key] + + # Save state + self.state.save(self.state_dir) + + status = "successfully" if success else "with error/timeout" + logger.info( + f"[BotDetector] Marked PR #{pr_number} review as finished ({status})" + ) + print( + f"[BotDetector] Finished review for PR #{pr_number} ({status})", + file=sys.stderr, + ) + def should_skip_pr_review( self, pr_number: int, @@ -335,13 +453,19 @@ class BotDetector: print(f"[BotDetector] SKIP PR #{pr_number}: {reason}") return True, reason - # Check 3: Are we in the cooling off period? + # Check 3: Is a review already in progress? + is_in_progress, reason = self.is_review_in_progress(pr_number) + if is_in_progress: + print(f"[BotDetector] SKIP PR #{pr_number}: {reason}") + return True, reason + + # Check 4: Are we in the cooling off period? is_cooling, reason = self.is_within_cooling_off(pr_number) if is_cooling: print(f"[BotDetector] SKIP PR #{pr_number}: {reason}") return True, reason - # Check 4: Have we already reviewed this exact commit? + # Check 5: Have we already reviewed this exact commit? head_sha = self.get_last_commit_sha(commits) if commits else None if head_sha and self.has_reviewed_commit(pr_number, head_sha): reason = f"Already reviewed commit {head_sha[:8]}" @@ -357,6 +481,7 @@ class BotDetector: Mark a PR as reviewed at a specific commit. This should be called after successfully posting a review. + Also clears the in-progress state. Args: pr_number: The PR number @@ -374,6 +499,10 @@ class BotDetector: # Update last review time self.state.last_review_times[pr_key] = datetime.now().isoformat() + # Clear in-progress state + if pr_key in self.state.in_progress_reviews: + del self.state.in_progress_reviews[pr_key] + # Save state self.state.save(self.state_dir) @@ -397,6 +526,9 @@ class BotDetector: if pr_key in self.state.last_review_times: del self.state.last_review_times[pr_key] + if pr_key in self.state.in_progress_reviews: + del self.state.in_progress_reviews[pr_key] + self.state.save(self.state_dir) print(f"[BotDetector] Cleared state for PR #{pr_number}") @@ -412,13 +544,16 @@ class BotDetector: total_reviews = sum( len(commits) for commits in self.state.reviewed_commits.values() ) + in_progress_count = len(self.state.in_progress_reviews) return { "bot_username": self.bot_username, "review_own_prs": self.review_own_prs, "total_prs_tracked": total_prs, "total_reviews_performed": total_reviews, + "in_progress_reviews": in_progress_count, "cooling_off_minutes": self.COOLING_OFF_MINUTES, + "in_progress_timeout_minutes": self.IN_PROGRESS_TIMEOUT_MINUTES, } def cleanup_stale_prs(self, max_age_days: int = 30) -> int: @@ -428,6 +563,9 @@ class BotDetector: This prevents unbounded growth of the state file by cleaning up entries for PRs that are likely closed/merged. + Also cleans up stale in-progress reviews (reviews that have been + in progress for longer than IN_PROGRESS_TIMEOUT_MINUTES). + Args: max_age_days: Remove PRs not reviewed in this many days (default: 30) @@ -435,8 +573,13 @@ class BotDetector: Number of PRs cleaned up """ cutoff = datetime.now() - timedelta(days=max_age_days) + in_progress_cutoff = datetime.now() - timedelta( + minutes=self.IN_PROGRESS_TIMEOUT_MINUTES + ) prs_to_remove: list[str] = [] + stale_in_progress: list[str] = [] + # Find stale reviewed PRs for pr_key, last_review_str in self.state.last_review_times.items(): try: last_review = datetime.fromisoformat(last_review_str) @@ -446,18 +589,43 @@ class BotDetector: # Invalid timestamp - mark for removal prs_to_remove.append(pr_key) + # Find stale in-progress reviews + for pr_key, start_time_str in self.state.in_progress_reviews.items(): + try: + start_time = datetime.fromisoformat(start_time_str) + if start_time < in_progress_cutoff: + stale_in_progress.append(pr_key) + except (ValueError, TypeError): + # Invalid timestamp - mark for removal + stale_in_progress.append(pr_key) + # Remove stale PRs for pr_key in prs_to_remove: if pr_key in self.state.reviewed_commits: del self.state.reviewed_commits[pr_key] if pr_key in self.state.last_review_times: del self.state.last_review_times[pr_key] + if pr_key in self.state.in_progress_reviews: + del self.state.in_progress_reviews[pr_key] - if prs_to_remove: + # Remove stale in-progress reviews + for pr_key in stale_in_progress: + if pr_key in self.state.in_progress_reviews: + del self.state.in_progress_reviews[pr_key] + + total_cleaned = len(prs_to_remove) + len(stale_in_progress) + + if total_cleaned > 0: self.state.save(self.state_dir) - print( - f"[BotDetector] Cleaned up {len(prs_to_remove)} stale PRs " - f"(older than {max_age_days} days)" - ) + if prs_to_remove: + print( + f"[BotDetector] Cleaned up {len(prs_to_remove)} stale PRs " + f"(older than {max_age_days} days)" + ) + if stale_in_progress: + print( + f"[BotDetector] Cleaned up {len(stale_in_progress)} stale in-progress reviews " + f"(older than {self.IN_PROGRESS_TIMEOUT_MINUTES} minutes)" + ) - return len(prs_to_remove) + return total_cleaned diff --git a/apps/backend/runners/github/context_gatherer.py b/apps/backend/runners/github/context_gatherer.py index 70544ecf..e745193f 100644 --- a/apps/backend/runners/github/context_gatherer.py +++ b/apps/backend/runners/github/context_gatherer.py @@ -19,7 +19,6 @@ from __future__ import annotations import ast import asyncio import json -import os import re from dataclasses import dataclass, field from pathlib import Path @@ -825,41 +824,11 @@ class PRContextGatherer: """ Find files related to the changes. - This includes: - - Test files for changed source files - - Imported modules and dependencies - - Configuration files in the same directory - - Related type definition files - - Reverse dependencies (files that import changed files) + DEPRECATED: LLM agents now discover related files themselves using Read, Grep, and Glob tools. + This method returns an empty list - agents have domain expertise to find what's relevant. """ - related = set() - - for changed_file in changed_files: - path = Path(changed_file.path) - - # Find test files - related.update(self._find_test_files(path)) - - # Find imported files (for supported languages) - if path.suffix in [".ts", ".tsx", ".js", ".jsx", ".py"]: - related.update(self._find_imports(changed_file.content, path)) - - # Find config files in same directory - related.update(self._find_config_files(path.parent)) - - # Find type definition files - if path.suffix in [".ts", ".tsx"]: - related.update(self._find_type_definitions(path)) - - # Find reverse dependencies (files that import this file) - related.update(self._find_dependents(changed_file.path)) - - # Remove files that are already in changed_files - changed_paths = {cf.path for cf in changed_files} - related = {r for r in related if r not in changed_paths} - - # Use smart prioritization with increased limit (50 instead of 20) - return self._prioritize_related_files(related, limit=50) + # Return empty list - LLM agents will discover files via their tools + return [] def _find_test_files(self, source_path: Path) -> set[str]: """Find test files related to a source file.""" @@ -1071,170 +1040,35 @@ class PRContextGatherer: """ Find files that import the given file (reverse dependencies). - Uses pure Python to search for import statements referencing this file. - Cross-platform compatible (Windows, macOS, Linux). - Limited to prevent performance issues on large codebases. + DEPRECATED: LLM agents now discover reverse dependencies themselves using Grep and Read tools. + Returns empty set - agents can search the codebase with their domain expertise. Args: file_path: Path of the file to find dependents for max_results: Maximum number of dependents to return Returns: - Set of file paths that import this file. + Empty set - LLM agents will discover dependents via Grep tool. """ - dependents: set[str] = set() - path_obj = Path(file_path) - stem = path_obj.stem # e.g., 'helpers' from 'utils/helpers.ts' - - # NOTE: We no longer skip generic filenames like "utils", "types", "index". - # The LLM-driven exploration system decides what's relevant based on the PR context. - # Widely-used utilities are often the MOST important files to track dependents for. - - # Build regex patterns and file extensions based on file type - pattern = None - file_extensions = [] - - if path_obj.suffix in [".ts", ".tsx", ".js", ".jsx"]: - # Match various import styles for JS/TS - # from './helpers', from '../utils/helpers', from '@/utils/helpers' - # Escape stem for regex safety - escaped_stem = re.escape(stem) - pattern = re.compile(rf"['\"].*{escaped_stem}['\"]") - file_extensions = [".ts", ".tsx", ".js", ".jsx"] - elif path_obj.suffix == ".py": - # Match Python imports: from .helpers import, import helpers - escaped_stem = re.escape(stem) - pattern = re.compile(rf"(from.*{escaped_stem}|import.*{escaped_stem})") - file_extensions = [".py"] - else: - return dependents - - # Directories to exclude - exclude_dirs = { - "node_modules", - ".git", - "dist", - "build", - "__pycache__", - ".venv", - "venv", - } - - # Walk the project directory - project_path = Path(self.project_dir) - files_checked = 0 - max_files_to_check = 2000 # Prevent infinite scanning on large codebases - - try: - for root, dirs, files in os.walk(project_path): - # Modify dirs in-place to exclude certain directories - dirs[:] = [d for d in dirs if d not in exclude_dirs] - - for filename in files: - # Check if we've hit the file limit - if files_checked >= max_files_to_check: - safe_print( - f"[Context] File scan limit ({max_files_to_check}) reached for {file_path}. " - f"Found {len(dependents)} dependents. " - f"LLM agents can explore additional callers if needed via Read/Grep tools." - ) - return dependents - - # Check if file has the right extension - if not any(filename.endswith(ext) for ext in file_extensions): - continue - - file_full_path = Path(root) / filename - files_checked += 1 - - # Get relative path from project root - try: - relative_path = file_full_path.relative_to(project_path) - relative_path_str = str(relative_path).replace("\\", "/") - - # Don't include the file itself - if relative_path_str == file_path: - continue - - # Search for the pattern in the file - try: - with open( - file_full_path, encoding="utf-8", errors="ignore" - ) as f: - content = f.read() - if pattern.search(content): - dependents.add(relative_path_str) - if len(dependents) >= max_results: - return dependents - except (OSError, UnicodeDecodeError): - # Skip files that can't be read - continue - - except ValueError: - # File is not relative to project_path, skip it - continue - - except Exception as e: - safe_print(f"[Context] Error finding dependents: {e}") - - return dependents + # Return empty set - LLM agents will use Grep to find importers when needed + return set() def _prioritize_related_files(self, files: set[str], limit: int = 50) -> list[str]: """ Prioritize related files by relevance. - Priority order: - 1. Test files (most important for review context) - 2. Type definition files (.d.ts) - 3. Configuration files - 4. Direct imports/dependents - 5. Other files + DEPRECATED: LLM agents now prioritize exploration based on their domain expertise. + Returns empty list since _find_related_files no longer populates files. Args: files: Set of file paths to prioritize limit: Maximum number of files to return Returns: - List of files sorted by priority, limited to `limit`. + Empty list - LLM agents handle prioritization via their tools. """ - test_files = [] - type_files = [] - config_files = [] - other_files = [] - - for f in files: - path = Path(f) - name_lower = path.name.lower() - - # Test files - if ( - ".test." in name_lower - or ".spec." in name_lower - or name_lower.startswith("test_") - or name_lower.endswith("_test.py") - or "__tests__" in f - ): - test_files.append(f) - # Type definition files - elif name_lower.endswith(".d.ts") or "types" in name_lower: - type_files.append(f) - # Config files - elif name_lower in [ - n.lower() for n in CONFIG_FILE_NAMES - ] or name_lower.endswith((".config.js", ".config.ts", "rc", "rc.json")): - config_files.append(f) - else: - other_files.append(f) - - # Sort within each category alphabetically for consistency, then combine - prioritized = ( - sorted(test_files) - + sorted(type_files) - + sorted(config_files) - + sorted(other_files) - ) - - return prioritized[:limit] + # Return empty list - LLM agents will prioritize exploration themselves + return [] def _load_json_safe(self, filename: str) -> dict | None: """ @@ -1462,59 +1296,18 @@ class PRContextGatherer: """ Find files related to the changes using a specific project root. - This static method allows finding related files AFTER a worktree - has been created, ensuring files exist in the worktree filesystem. + DEPRECATED: LLM agents now discover related files themselves using Read, Grep, and Glob tools. + This method returns an empty list - agents have domain expertise to find what's relevant. Args: changed_files: List of changed files from the PR project_root: Path to search for related files (e.g., worktree path) Returns: - List of related file paths (relative to project root) + Empty list - LLM agents will discover files via their tools. """ - related: set[str] = set() - - for changed_file in changed_files: - path = Path(changed_file.path) - - # Find test files - test_patterns = [ - # Jest/Vitest patterns - path.parent / f"{path.stem}.test{path.suffix}", - path.parent / f"{path.stem}.spec{path.suffix}", - path.parent / "__tests__" / f"{path.name}", - # Python patterns - path.parent / f"test_{path.stem}.py", - path.parent / f"{path.stem}_test.py", - # Go patterns - path.parent / f"{path.stem}_test.go", - ] - - for test_path in test_patterns: - full_path = project_root / test_path - if full_path.exists() and full_path.is_file(): - related.add(str(test_path)) - - # Find config files in same directory - for name in CONFIG_FILE_NAMES: - config_path = path.parent / name - full_path = project_root / config_path - if full_path.exists() and full_path.is_file(): - related.add(str(config_path)) - - # Find type definition files - if path.suffix in [".ts", ".tsx"]: - type_def = path.parent / f"{path.stem}.d.ts" - full_path = project_root / type_def - if full_path.exists() and full_path.is_file(): - related.add(str(type_def)) - - # Remove files that are already in changed_files - changed_paths = {cf.path for cf in changed_files} - related = {r for r in related if r not in changed_paths} - - # Limit to 50 most relevant files (increased from 20) - return sorted(related)[:50] + # Return empty list - LLM agents will discover files via their tools + return [] class FollowupContextGatherer: diff --git a/apps/backend/runners/github/orchestrator.py b/apps/backend/runners/github/orchestrator.py index 7c1da592..9ac4be35 100644 --- a/apps/backend/runners/github/orchestrator.py +++ b/apps/backend/runners/github/orchestrator.py @@ -396,9 +396,15 @@ class GitHubOrchestrator: # No existing review found, create skip result return await self._create_skip_result(pr_number, skip_reason) else: - # For other skip reasons (bot-authored, cooling off), create a skip result + # For other skip reasons (bot-authored, cooling off, in-progress), create a skip result return await self._create_skip_result(pr_number, skip_reason) + # Mark review as started (prevents concurrent reviews) + self.bot_detector.mark_review_started(pr_number) + safe_print( + f"[BOT DETECTION] Marked PR #{pr_number} review as started", flush=True + ) + self._report_progress( "analyzing", 30, "Running multi-pass review...", pr_number=pr_number ) @@ -572,6 +578,13 @@ class GitHubOrchestrator: except Exception as e: import traceback + # Mark review as finished with error + self.bot_detector.mark_review_finished(pr_number, success=False) + safe_print( + f"[BOT DETECTION] Marked PR #{pr_number} review as finished (error)", + flush=True, + ) + # Log full exception details for debugging error_details = f"{type(e).__name__}: {e}" full_traceback = traceback.format_exc() @@ -634,6 +647,13 @@ class GitHubOrchestrator: pr_number=pr_number, ) + # Mark review as started (prevents concurrent reviews) + self.bot_detector.mark_review_started(pr_number) + safe_print( + f"[BOT DETECTION] Marked PR #{pr_number} follow-up review as started", + flush=True, + ) + try: # Import here to avoid circular imports at module level try: @@ -956,6 +976,13 @@ class GitHubOrchestrator: return result except Exception as e: + # Mark review as finished with error + self.bot_detector.mark_review_finished(pr_number, success=False) + safe_print( + f"[BOT DETECTION] Marked PR #{pr_number} follow-up review as finished (error)", + flush=True, + ) + result = PRReviewResult( pr_number=pr_number, repo=self.config.repo, diff --git a/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py b/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py index 31652b1e..79205b18 100644 --- a/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py +++ b/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py @@ -29,7 +29,7 @@ from claude_agent_sdk import AgentDefinition try: from ...core.client import create_client from ...phase_config import get_thinking_budget, resolve_model_id - from ..context_gatherer import PRContext, PRContextGatherer, _validate_git_ref + from ..context_gatherer import PRContext, _validate_git_ref from ..gh_client import GHClient from ..models import ( BRANCH_BEHIND_BLOCKER_MSG, @@ -51,7 +51,7 @@ try: ) from .sdk_utils import process_sdk_stream except (ImportError, ValueError, SystemError): - from context_gatherer import PRContext, PRContextGatherer, _validate_git_ref + from context_gatherer import PRContext, _validate_git_ref from core.client import create_client from gh_client import GHClient from models import ( @@ -255,9 +255,8 @@ class ParallelOrchestratorReviewer: "Security specialist. Use for OWASP Top 10, authentication, " "injection, cryptographic issues, and sensitive data exposure. " "Invoke when PR touches auth, API endpoints, user input, database queries, " - "or file operations. IMPORTANT: Also check related files listed in the " - "PR context - callers may be affected by security changes, and tests " - "should verify security behavior." + "or file operations. Use Read, Grep, and Glob tools to explore related files, " + "callers, and tests as needed." ), prompt=with_working_dir( security_prompt, "You are a security expert. Find vulnerabilities." @@ -269,9 +268,8 @@ class ParallelOrchestratorReviewer: description=( "Code quality expert. Use for complexity, duplication, error handling, " "maintainability, and pattern adherence. Invoke when PR has complex logic, " - "large functions, or significant business logic changes. IMPORTANT: Check " - "related files for pattern consistency - if a pattern is changed, similar " - "code elsewhere should be updated too." + "large functions, or significant business logic changes. Use Grep to search " + "for similar patterns across the codebase for consistency checks." ), prompt=with_working_dir( quality_prompt, @@ -285,8 +283,7 @@ class ParallelOrchestratorReviewer: "Logic and correctness specialist. Use for algorithm verification, " "edge cases, state management, and race conditions. Invoke when PR has " "algorithmic changes, data transformations, concurrent operations, or bug fixes. " - "IMPORTANT: Check callers and dependents in related files - logic changes " - "may break assumptions made by code that uses this file." + "Use Grep to find callers and dependents that may be affected by logic changes." ), prompt=with_working_dir( logic_prompt, "You are a logic expert. Find correctness issues." @@ -299,8 +296,7 @@ class ParallelOrchestratorReviewer: "Codebase consistency expert. Use for naming conventions, ecosystem fit, " "architectural alignment, and avoiding reinvention. Invoke when PR introduces " "new patterns, large additions, or code that might duplicate existing functionality. " - "IMPORTANT: Use related files to understand existing patterns - new code " - "should match established conventions in the codebase." + "Use Grep and Glob to explore existing patterns and conventions in the codebase." ), prompt=with_working_dir( codebase_fit_prompt, @@ -329,7 +325,7 @@ class ParallelOrchestratorReviewer: "Reads the ACTUAL CODE at the finding location with fresh eyes. " "CRITICAL: Invoke for ALL findings after specialist agents complete. " "Can confirm findings as valid OR dismiss them as false positives. " - "Check related files for mitigations the original agent missed." + "Use Read, Grep, and Glob to check for mitigations the original agent missed." ), prompt=with_working_dir( validator_prompt, "You validate whether findings are real issues." @@ -397,80 +393,10 @@ Found {len(context.ai_bot_comments)} comments from AI tools. {chr(10).join(commits_list)} """ - # Build related files section (CONTEXT-02) + # Removed: Related files and import graph sections + # LLM agents now discover relevant files themselves via Read, Grep, Glob tools related_files_section = "" - if context.related_files: - # Categorize by type - tests = [ - f - for f in context.related_files - if ".test." in f - or "_test." in f - or f.startswith("test") - or "/tests/" in f - or "\\tests\\" in f - ] - deps = [f for f in context.related_files if f not in tests] - - # Limit to avoid context overflow - tests = tests[:15] - deps = deps[:15] - - tests_str = ", ".join(f"`{t}`" for t in tests) if tests else "None found" - deps_str = ", ".join(f"`{d}`" for d in deps) if deps else "None found" - - related_files_section = f""" -### Related Files to Investigate -These files are related to the changes (imports, tests, dependents). **Pass relevant files to specialists when delegating.** - -**Tests** ({len(tests)} files): {tests_str} -**Dependencies/Callers** ({len(deps)} files): {deps_str} - -**When delegating to specialists, include relevant files:** -- **security-reviewer**: Mention files that handle the same data flow -- **logic-reviewer**: Mention callers that depend on changed function signatures -- **quality-reviewer**: Mention files with similar patterns for consistency check -- **codebase-fit-reviewer**: Mention existing implementations of similar features - -Example delegation: "Review the auth changes in login.ts. Also check auth_middleware.ts and auth.test.ts which use this module." -""" - - # Build import graph summary (CONTEXT-03) import_graph_section = "" - import_entries = [] - changed_paths = {f.path for f in context.changed_files} - - for file in context.changed_files[:10]: # Limit to 10 files - # Find what this file imports (look for related files it references) - imports_this = [ - r - for r in context.related_files - if r in (file.content or "") and r not in changed_paths - ][:5] - # Find what imports this file (reverse deps in related_files) - # Match by filename stem to catch imports without extension - file_stem = file.path.split("/")[-1].split(".")[0] - imported_by = [ - r - for r in context.related_files - if file_stem in r and r not in changed_paths - ][:5] - - if imports_this or imported_by: - entry = f"**{file.path}**" - if imports_this: - entry += f"\n - Imports: {', '.join(imports_this)}" - if imported_by: - entry += f"\n - Imported by: {', '.join(imported_by)}" - import_entries.append(entry) - - if import_entries: - import_graph_section = f""" -### Import Relationships -How the changed files connect to the codebase: - -{chr(10).join(import_entries[:20])} -""" pr_context = f""" --- @@ -790,25 +716,9 @@ The SDK will run invoked agents in parallel automatically. else self.project_dir ) - # Rescan for related files using the worktree/project root - # This fixes the issue where related files were 0 because context gathering - # happened BEFORE the worktree was created (PR files didn't exist locally) - if context.changed_files: - new_related_files = PRContextGatherer.find_related_files_for_root( - context.changed_files, - project_root, - ) - # Always log rescan result (not gated by DEBUG_MODE) - if new_related_files: - context.related_files = new_related_files - safe_print( - f"[PRReview] Rescanned in worktree: found {len(new_related_files)} related files" - ) - else: - safe_print( - f"[PRReview] Rescanned in worktree: found 0 related files " - f"(initial scan found {len(context.related_files)})" - ) + # Removed: Related files rescanning + # LLM agents now discover relevant files themselves via Read, Grep, Glob tools + # No need to pre-scan the codebase programmatically # Build orchestrator prompt AFTER worktree creation and related files rescan prompt = self._build_orchestrator_prompt(context) @@ -839,36 +749,110 @@ The SDK will run invoked agents in parallel automatically. ) # Run orchestrator session using shared SDK stream processor - async with client: - await client.query(prompt) + # Retry logic for tool use concurrency errors + MAX_RETRIES = 3 + RETRY_DELAY = 2.0 # seconds between retries - safe_print( - f"[ParallelOrchestrator] Running orchestrator ({model})...", - flush=True, - ) + result_text = "" + structured_output = None + agents_invoked = [] + msg_count = 0 + last_error = None - # Process SDK stream with shared utility - stream_result = await process_sdk_stream( - client=client, - context_name="ParallelOrchestrator", - model=model, - system_prompt=prompt, - agent_definitions=agent_defs, - ) - - # Check for stream processing errors - if stream_result.get("error"): - logger.error( - f"[ParallelOrchestrator] SDK stream failed: {stream_result['error']}" + for attempt in range(MAX_RETRIES): + if attempt > 0: + logger.info( + f"[ParallelOrchestrator] Retry attempt {attempt}/{MAX_RETRIES - 1} " + f"after tool concurrency error" ) - raise RuntimeError( - f"SDK stream processing failed: {stream_result['error']}" + safe_print( + f"[ParallelOrchestrator] Retry {attempt}/{MAX_RETRIES - 1} " + f"(tool concurrency error detected)" + ) + # Small delay before retry + import asyncio + + await asyncio.sleep(RETRY_DELAY) + + # Recreate client for retry (fresh session) + client = self._create_sdk_client( + project_root, model, thinking_budget ) - result_text = stream_result["result_text"] - structured_output = stream_result["structured_output"] - agents_invoked = stream_result["agents_invoked"] - msg_count = stream_result["msg_count"] + try: + async with client: + await client.query(prompt) + + safe_print( + f"[ParallelOrchestrator] Running orchestrator ({model})...", + flush=True, + ) + + # Process SDK stream with shared utility + stream_result = await process_sdk_stream( + client=client, + context_name="ParallelOrchestrator", + model=model, + system_prompt=prompt, + agent_definitions=agent_defs, + ) + + error = stream_result.get("error") + + # Check for tool concurrency error specifically + if ( + error == "tool_use_concurrency_error" + and attempt < MAX_RETRIES - 1 + ): + logger.warning( + f"[ParallelOrchestrator] Tool concurrency error on attempt {attempt + 1}, " + f"will retry..." + ) + last_error = error + continue # Retry + + # Check for other stream processing errors + if error: + logger.error( + f"[ParallelOrchestrator] SDK stream failed: {error}" + ) + raise RuntimeError(f"SDK stream processing failed: {error}") + + # Success - extract results and break retry loop + result_text = stream_result["result_text"] + structured_output = stream_result["structured_output"] + agents_invoked = stream_result["agents_invoked"] + msg_count = stream_result["msg_count"] + break # Success, exit retry loop + + except Exception as e: + # Check if this is a retryable error + error_str = str(e).lower() + is_retryable = ( + "400" in error_str + or "concurrency" in error_str + or "tool_use" in error_str + ) + + if is_retryable and attempt < MAX_RETRIES - 1: + logger.warning( + f"[ParallelOrchestrator] Retryable error on attempt {attempt + 1}: {e}" + ) + last_error = str(e) + continue # Retry + + # Not retryable or out of retries - re-raise + raise + else: + # All retries exhausted + logger.error( + f"[ParallelOrchestrator] Failed after {MAX_RETRIES} attempts. " + f"Last error: {last_error}" + ) + raise RuntimeError( + f"Orchestrator failed after {MAX_RETRIES} retry attempts. " + f"Last error: {last_error}" + ) self._report_progress( "finalizing", @@ -1427,6 +1411,10 @@ The SDK will run invoked agents in parallel automatically. if not findings: return [] + # Retry configuration for API errors + MAX_VALIDATION_RETRIES = 2 + VALIDATOR_MAX_MESSAGES = 200 # Lower limit for validator (simpler task) + # Build validation prompt with all findings findings_json = [] for f in findings: @@ -1466,47 +1454,101 @@ For EACH finding above: model_shorthand = self.config.model or "sonnet" model = resolve_model_id(model_shorthand) - # Create validator client (inherits worktree filesystem access) - try: - validator_client = create_client( - project_dir=worktree_path, - spec_dir=self.github_dir, - model=model, - agent_type="pr_finding_validator", - max_thinking_tokens=get_thinking_budget("medium"), - output_format={ - "type": "json_schema", - "schema": FindingValidationResponse.model_json_schema(), - }, - ) - except Exception as e: - logger.error(f"[PRReview] Failed to create validator client: {e}") - # Fail-safe: return original findings - return findings - - # Run validation - try: - async with validator_client: - await validator_client.query(prompt) - - stream_result = await process_sdk_stream( - client=validator_client, - context_name="FindingValidator", - model=model, - system_prompt=prompt, + # Retry loop for transient API errors + last_error = None + for attempt in range(MAX_VALIDATION_RETRIES + 1): + if attempt > 0: + logger.info( + f"[PRReview] Validation retry {attempt}/{MAX_VALIDATION_RETRIES}" + ) + safe_print( + f"[FindingValidator] Retry attempt {attempt}/{MAX_VALIDATION_RETRIES}" ) - if stream_result.get("error"): - logger.error( - f"[PRReview] Validation failed: {stream_result['error']}" + # Create validator client (inherits worktree filesystem access) + try: + validator_client = create_client( + project_dir=worktree_path, + spec_dir=self.github_dir, + model=model, + agent_type="pr_finding_validator", + max_thinking_tokens=get_thinking_budget("medium"), + output_format={ + "type": "json_schema", + "schema": FindingValidationResponse.model_json_schema(), + }, + ) + except Exception as e: + logger.error(f"[PRReview] Failed to create validator client: {e}") + last_error = e + continue # Try again + + # Run validation + try: + async with validator_client: + await validator_client.query(prompt) + + stream_result = await process_sdk_stream( + client=validator_client, + context_name="FindingValidator", + model=model, + system_prompt=prompt, + max_messages=VALIDATOR_MAX_MESSAGES, ) - # Fail-safe: return original findings - return findings - structured_output = stream_result.get("structured_output") + error = stream_result.get("error") + if error: + # Check for specific error types that warrant retry + error_str = str(error).lower() + is_retryable = ( + "400" in error_str + or "concurrency" in error_str + or "circuit breaker" in error_str + or "tool_use" in error_str + ) - except Exception as e: - logger.error(f"[PRReview] Validation stream error: {e}") + if is_retryable and attempt < MAX_VALIDATION_RETRIES: + logger.warning( + f"[PRReview] Retryable validation error: {error}" + ) + last_error = Exception(error) + continue # Retry + + logger.error(f"[PRReview] Validation failed: {error}") + # Fail-safe: return original findings + return findings + + structured_output = stream_result.get("structured_output") + + # Success - break out of retry loop + if structured_output: + break + + except Exception as e: + error_str = str(e).lower() + is_retryable = ( + "400" in error_str + or "concurrency" in error_str + or "rate" in error_str + ) + + if is_retryable and attempt < MAX_VALIDATION_RETRIES: + logger.warning(f"[PRReview] Retryable stream error: {e}") + last_error = e + continue # Retry + + logger.error(f"[PRReview] Validation stream error: {e}") + # Fail-safe: return original findings + return findings + else: + # All retries exhausted + logger.error( + f"[PRReview] Validation failed after {MAX_VALIDATION_RETRIES} retries. " + f"Last error: {last_error}" + ) + safe_print( + f"[FindingValidator] ERROR: Validation failed after {MAX_VALIDATION_RETRIES} retries" + ) # Fail-safe: return original findings return findings diff --git a/apps/backend/runners/github/services/pr_review_engine.py b/apps/backend/runners/github/services/pr_review_engine.py index 867a8f19..15263d4c 100644 --- a/apps/backend/runners/github/services/pr_review_engine.py +++ b/apps/backend/runners/github/services/pr_review_engine.py @@ -139,18 +139,9 @@ class PRReviewEngine: files_list.append(f"- ... and {len(context.changed_files) - 20} more files") files_str = "\n".join(files_list) - # NEW: Format related files (imports, tests, etc.) + # Removed: Related files section + # LLM agents now discover relevant files themselves via Read, Grep, Glob tools related_files_str = "" - if context.related_files: - related_files_list = [f"- `{f}`" for f in context.related_files[:10]] - if len(context.related_files) > 10: - related_files_list.append( - f"- ... and {len(context.related_files) - 10} more" - ) - related_files_str = f""" -### Related Files (imports, tests, configs) -{chr(10).join(related_files_list)} -""" # NEW: Format commits for context commits_str = "" diff --git a/apps/backend/runners/github/services/sdk_utils.py b/apps/backend/runners/github/services/sdk_utils.py index 88e0e01d..69f50736 100644 --- a/apps/backend/runners/github/services/sdk_utils.py +++ b/apps/backend/runners/github/services/sdk_utils.py @@ -25,243 +25,6 @@ logger = logging.getLogger(__name__) # Check if debug mode is enabled DEBUG_MODE = os.environ.get("DEBUG", "").lower() in ("true", "1", "yes") -# ── TEMPORARY: Per-PR full agent communication logger (v2) ──────────── -# Writes every message to .auto-claude/github/pr/debug_logs/_.log -# Remove after measurement phase is complete. -import datetime as _dt -import json as _json -from pathlib import Path as _Path - -# Derive project root dynamically from this file's location -# sdk_utils.py is at: apps/backend/runners/github/services/sdk_utils.py -# So project root is 5 levels up -_PROJECT_ROOT = _Path(__file__).resolve().parent.parent.parent.parent.parent -_PR_LOG_DIR = _PROJECT_ROOT / ".auto-claude" / "github" / "pr" / "debug_logs" - - -class _PRDebugLogger: - """Writes full agent communication to a log file for review. - - Improvements (v2): - - System prompt and agent definitions logged at session start - - No truncation on thinking, text, tool input, or tool results - - No duplicate logging (single structured dump per message) - - Empty/whitespace content shown via repr() - - Agent attribution via subagent_tool_ids mapping - """ - - def __init__(self, context_name: str, model: str | None = None): - self._f = None - self._subagent_tool_ids: dict[str, str] = {} # tool_id -> agent_name - - try: - _PR_LOG_DIR.mkdir(parents=True, exist_ok=True) - ts = _dt.datetime.now().strftime("%Y%m%d_%H%M%S") - self.path = _PR_LOG_DIR / f"{context_name}_{ts}.log" - self._f = open(self.path, "w", encoding="utf-8") - self._write( - f"=== {context_name} Session Started at {_dt.datetime.now().isoformat()} ===" - ) - if model: - self._write(f"Model: {model}") - self._write("") - except OSError as e: - # Failed to create directory or open file - logging disabled - logger.warning(f"PR debug logger disabled: {e}") - self.path = None - - def _write(self, text: str): - # Skip logging if file handle was not created successfully - if self._f is None: - return - try: - self._f.write(text + "\n") - self._f.flush() - except (OSError, ValueError) as e: - # File write failed (file closed, disk full, etc.) - disable logging - logger.warning(f"PR debug logger write failed: {e}") - self._f = None - - # ── Session preamble loggers ────────────────────────────────────── - - def log_system_prompt(self, prompt: str): - """Log the full system prompt (no truncation).""" - self._write(f"\n{'#' * 80}") - self._write("# SYSTEM PROMPT (full orchestrator instructions + PR context)") - self._write(f"# Length: {len(prompt)} chars") - self._write(f"{'#' * 80}") - self._write(prompt) - self._write(f"{'#' * 80}\n") - - def log_agent_definitions(self, agents: dict): - """Log all specialist agent definitions (prompts, tools, descriptions).""" - self._write(f"\n{'#' * 80}") - self._write(f"# AGENT DEFINITIONS ({len(agents)} specialists)") - self._write(f"{'#' * 80}") - for name, defn in agents.items(): - self._write(f"\n--- Agent: {name} ---") - self._write(f" description: {getattr(defn, 'description', 'N/A')}") - self._write(f" model: {getattr(defn, 'model', 'N/A')}") - self._write(f" tools: {getattr(defn, 'tools', 'N/A')}") - prompt = getattr(defn, "prompt", "") - self._write(f" prompt ({len(prompt)} chars):") - self._write(prompt) - self._write(f"{'#' * 80}\n") - - # ── Agent attribution ───────────────────────────────────────────── - - def set_subagent_mapping(self, mapping: dict[str, str]): - """Update the tool_id -> agent_name mapping for attribution.""" - self._subagent_tool_ids = mapping - - def _get_agent_label(self, tool_id: str) -> str: - """Return agent label if this tool_id belongs to a known subagent.""" - agent = self._subagent_tool_ids.get(tool_id) - return f" [Agent:{agent}]" if agent else "" - - # ── Per-message logger (single structured dump) ─────────────────── - - def log_message(self, msg_count: int, msg_type: str, msg: object): - self._write(f"\n{'=' * 80}") - self._write(f"--- Message #{msg_count} [{msg_type}] ---") - self._write(f"{'=' * 80}") - self._dump_raw(msg) - - def _dump_raw(self, msg: object, indent: int = 0): - """Dump full raw message content recursively — NO truncation.""" - prefix = " " * indent - # Content blocks - if hasattr(msg, "content"): - content = msg.content - if isinstance(content, list): - self._write(f"{prefix}[content] ({len(content)} blocks):") - for i, block in enumerate(content): - block_type = type(block).__name__ - self._write(f"{prefix} [{i}] {block_type}:") - self._dump_block(block, indent + 2) - elif isinstance(content, str): - if not content or content.isspace(): - self._write( - f"{prefix}[content] (string, {len(content)} chars): {repr(content)}" - ) - else: - self._write(f"{prefix}[content] (string, {len(content)} chars):") - self._write(content) - else: - self._write(f"{prefix}[content] ({type(content).__name__}):") - self._write(f"{prefix} {str(content)}") - - # Role / type - if hasattr(msg, "role"): - self._write(f"{prefix}[role] {msg.role}") - if hasattr(msg, "type") and not hasattr(msg, "content"): - self._write(f"{prefix}[type] {msg.type}") - - # Structured output - if hasattr(msg, "structured_output") and msg.structured_output: - self._write(f"{prefix}[structured_output]:") - try: - self._write(_json.dumps(msg.structured_output, indent=2, default=str)) - except Exception: - self._write(f"{prefix} {str(msg.structured_output)}") - - # Result message fields - if hasattr(msg, "subtype"): - self._write(f"{prefix}[subtype] {msg.subtype}") - if hasattr(msg, "is_error"): - self._write(f"{prefix}[is_error] {msg.is_error}") - if hasattr(msg, "duration_ms"): - self._write(f"{prefix}[duration_ms] {msg.duration_ms}") - if hasattr(msg, "session_id"): - self._write(f"{prefix}[session_id] {msg.session_id}") - - # Catch-all for messages without content blocks - for attr in ("text", "thinking", "name", "id", "input", "tool_use_id"): - if hasattr(msg, attr) and not hasattr(msg, "content"): - val = getattr(msg, attr) - if val is not None: - self._write(f"{prefix}[{attr}] {str(val)}") - - def _dump_block(self, block: object, indent: int = 0): - """Dump a single content block — NO truncation.""" - prefix = " " * indent - block_type = getattr(block, "type", type(block).__name__) - - if block_type in ("text", "TextBlock") and hasattr(block, "text"): - text = block.text - if not text or text.isspace(): - self._write(f"{prefix}[text] ({len(text)} chars): {repr(text)}") - else: - self._write(f"{prefix}[text] ({len(text)} chars):") - self._write(text) - - elif block_type in ("thinking", "ThinkingBlock") and hasattr(block, "thinking"): - text = block.thinking or getattr(block, "text", "") - self._write(f"{prefix}[thinking] ({len(text)} chars):") - self._write(text) - - elif block_type in ("tool_use", "ToolUseBlock"): - tool_name = getattr(block, "name", "unknown") - tool_id = getattr(block, "id", "unknown") - tool_input = getattr(block, "input", {}) - agent_label = self._get_agent_label(tool_id) - self._write(f"{prefix}[tool_use] {tool_name} (id={tool_id}){agent_label}") - try: - self._write(_json.dumps(tool_input, indent=2, default=str)) - except Exception: - self._write(str(tool_input)) - - elif block_type in ("tool_result", "ToolResultBlock"): - tool_id = getattr(block, "tool_use_id", "unknown") - is_error = getattr(block, "is_error", False) - result = getattr(block, "content", "") - if isinstance(result, list): - result = " ".join(str(getattr(c, "text", c)) for c in result) - status = "ERROR" if is_error else "OK" - agent_label = self._get_agent_label(tool_id) - self._write( - f"{prefix}[tool_result] (tool_id={tool_id}) {status}{agent_label}" - ) - self._write(str(result)) - - else: - # Unknown block type — dump everything we can - self._write(f"{prefix}[{block_type}] (raw dump):") - for attr in dir(block): - if not attr.startswith("_"): - try: - val = getattr(block, attr) - if not callable(val): - self._write(f"{prefix} {attr}: {str(val)}") - except Exception: - pass - - # ── Structured output (standalone, for final result) ────────────── - - def log_structured_output(self, output: dict): - self._write("[STRUCTURED_OUTPUT]") - try: - self._write(_json.dumps(output, indent=2, default=str)) - except Exception: - self._write(str(output)) - - # ── Session close ───────────────────────────────────────────────── - - def close(self, summary: dict): - self._write("\n=== Session Ended ===") - self._write(f"Messages: {summary.get('msg_count', '?')}") - self._write(f"Agents invoked: {summary.get('agents_invoked', [])}") - self._write(f"Error: {summary.get('error')}") - self._write(f"Log file: {self.path}") - if self._f is not None: - try: - self._f.close() - except OSError as e: - logger.warning(f"PR debug logger close failed: {e}") - - -# ── END TEMPORARY ────────────────────────────────────────────────────── - def _short_model_name(model: str | None) -> str: """Convert full model name to a short display name for logs. @@ -361,6 +124,49 @@ def _get_tool_detail(tool_name: str, tool_input: dict[str, Any]) -> str: return f"Using tool: {tool_name}" +# Circuit breaker threshold - abort if message count exceeds this +# Prevents runaway retry loops from consuming unbounded resources +MAX_MESSAGE_COUNT = 500 + + +def _is_tool_concurrency_error(text: str) -> bool: + """ + Detect the specific tool use concurrency error pattern. + + This error occurs when Claude makes multiple parallel tool_use blocks + and some fail, corrupting the tool_use/tool_result message pairing. + + Args: + text: Text to check for error pattern + + Returns: + True if this is the tool concurrency error, False otherwise + """ + text_lower = text.lower() + # Check for the specific error message pattern + # Pattern 1: Explicit concurrency or tool_use errors with 400 + has_400 = "400" in text_lower + has_tool = "tool" in text_lower + + if has_400 and has_tool: + # Look for specific keywords indicating tool concurrency issues + error_keywords = [ + "concurrency", + "tool_use", + "tool use", + "tool_result", + "tool result", + ] + if any(keyword in text_lower for keyword in error_keywords): + return True + + # Pattern 2: API error with 400 and tool mention + if "api error" in text_lower and has_400 and has_tool: + return True + + return False + + async def process_sdk_stream( client: Any, on_thinking: Callable[[str], None] | None = None, @@ -370,8 +176,10 @@ async def process_sdk_stream( on_structured_output: Callable[[dict[str, Any]], None] | None = None, context_name: str = "SDK", model: str | None = None, - system_prompt: str | None = None, - agent_definitions: dict | None = None, + max_messages: int | None = None, + # Deprecated parameters (kept for backwards compatibility, no longer used) + system_prompt: str | None = None, # noqa: ARG001 + agent_definitions: dict | None = None, # noqa: ARG001 ) -> dict[str, Any]: """ Process SDK response stream with customizable callbacks. @@ -392,8 +200,7 @@ async def process_sdk_stream( on_structured_output: Callback for structured output - receives dict context_name: Name for logging (e.g., "ParallelOrchestrator", "ParallelFollowup") model: Model name for logging (e.g., "claude-sonnet-4-5-20250929") - system_prompt: Full system prompt sent to the agent (logged at session start) - agent_definitions: Dict of agent name -> AgentDefinition (logged at session start) + max_messages: Optional override for max message count circuit breaker (default: MAX_MESSAGE_COUNT) Returns: Dictionary with: @@ -412,15 +219,11 @@ async def process_sdk_stream( # Track subagent tool IDs to log their results subagent_tool_ids: dict[str, str] = {} # tool_id -> agent_name completed_agent_tool_ids: set[str] = set() # tool_ids of completed agents + # Track tool concurrency errors for retry logic + detected_concurrency_error = False - # TEMPORARY: per-PR debug file logger - _dbg = _PRDebugLogger(context_name, model=model) - - # Log session preamble: system prompt and agent definitions - if system_prompt: - _dbg.log_system_prompt(system_prompt) - if agent_definitions: - _dbg.log_agent_definitions(agent_definitions) + # Circuit breaker: max messages before aborting + message_limit = max_messages if max_messages is not None else MAX_MESSAGE_COUNT safe_print(f"[{context_name}] Processing SDK stream...") if DEBUG_MODE: @@ -435,7 +238,17 @@ async def process_sdk_stream( try: msg_type = type(msg).__name__ msg_count += 1 - _dbg.log_message(msg_count, msg_type, msg) + + # CIRCUIT BREAKER: Abort if message count exceeds threshold + # This prevents runaway retry loops (e.g., 400 errors causing infinite retries) + if msg_count > message_limit: + stream_error = ( + f"Circuit breaker triggered: message count ({msg_count}) " + f"exceeded limit ({message_limit}). Possible retry loop detected." + ) + logger.error(f"[{context_name}] {stream_error}") + safe_print(f"[{context_name}] ERROR: {stream_error}") + break # Log progress periodically so user knows AI is working if msg_count - last_progress_log >= PROGRESS_LOG_INTERVAL: @@ -504,7 +317,6 @@ async def process_sdk_stream( agents_invoked.append(agent_name) # Track this tool ID to log its result later subagent_tool_ids[tool_id] = agent_name - _dbg.set_subagent_mapping(subagent_tool_ids) # Log with model info if available model_info = f" [{_short_model_name(model)}]" if model else "" safe_print( @@ -611,6 +423,15 @@ async def process_sdk_stream( block_type = type(block).__name__ if block_type == "TextBlock" and hasattr(block, "text"): result_text += block.text + # Check for tool concurrency error pattern in text output + if _is_tool_concurrency_error(block.text): + detected_concurrency_error = True + logger.warning( + f"[{context_name}] Detected tool use concurrency error in response" + ) + safe_print( + f"[{context_name}] WARNING: Tool concurrency error detected" + ) # Always print text content preview (not just in DEBUG_MODE) text_preview = block.text[:500].replace("\n", " ").strip() if text_preview: @@ -655,7 +476,6 @@ async def process_sdk_stream( # Only capture if we don't already have it (avoid duplicates) if structured_output is None: structured_output = msg.structured_output - _dbg.log_structured_output(msg.structured_output) safe_print(f"[{context_name}] Received structured output") if on_structured_output: on_structured_output(msg.structured_output) @@ -728,7 +548,14 @@ async def process_sdk_stream( safe_print(f"[{context_name}] Session ended. Total messages: {msg_count}") - result = { + # Set error flag if tool concurrency error was detected + if detected_concurrency_error and not stream_error: + stream_error = "tool_use_concurrency_error" + logger.warning( + f"[{context_name}] Tool use concurrency error detected - caller should retry" + ) + + return { "result_text": result_text, "structured_output": structured_output, "agents_invoked": agents_invoked, @@ -736,6 +563,3 @@ async def process_sdk_stream( "subagent_tool_ids": subagent_tool_ids, "error": stream_error, } - _dbg.close(result) - safe_print(f"[{context_name}] Full debug log: {_dbg.path}") - return result diff --git a/apps/backend/runners/github/test_bot_detection.py b/apps/backend/runners/github/test_bot_detection.py index 2160fe88..e8811502 100644 --- a/apps/backend/runners/github/test_bot_detection.py +++ b/apps/backend/runners/github/test_bot_detection.py @@ -532,5 +532,176 @@ class TestGhExecutableDetection: mock_run.assert_not_called() +class TestInProgressTracking: + """Test in-progress review tracking.""" + + def test_mark_review_started(self, mock_bot_detector, temp_state_dir): + """Test marking review as started.""" + mock_bot_detector.mark_review_started(123) + + # Check state + assert "123" in mock_bot_detector.state.in_progress_reviews + start_time_str = mock_bot_detector.state.in_progress_reviews["123"] + start_time = datetime.fromisoformat(start_time_str) + + # Should be very recent (within last 5 seconds) + time_diff = datetime.now() - start_time + assert time_diff.total_seconds() < 5 + + # Check persistence + loaded = BotDetectionState.load(temp_state_dir) + assert "123" in loaded.in_progress_reviews + + def test_mark_review_finished_success(self, mock_bot_detector, temp_state_dir): + """Test marking review as finished successfully.""" + mock_bot_detector.mark_review_started(123) + assert "123" in mock_bot_detector.state.in_progress_reviews + + mock_bot_detector.mark_review_finished(123, success=True) + + # In-progress state should be cleared + assert "123" not in mock_bot_detector.state.in_progress_reviews + + # Check persistence + loaded = BotDetectionState.load(temp_state_dir) + assert "123" not in loaded.in_progress_reviews + + def test_mark_review_finished_error(self, mock_bot_detector): + """Test marking review as finished with error.""" + mock_bot_detector.mark_review_started(123) + mock_bot_detector.mark_review_finished(123, success=False) + + # In-progress state should be cleared + assert "123" not in mock_bot_detector.state.in_progress_reviews + + def test_is_review_in_progress_active(self, mock_bot_detector): + """Test detecting active in-progress review.""" + mock_bot_detector.mark_review_started(123) + + is_in_progress, reason = mock_bot_detector.is_review_in_progress(123) + + assert is_in_progress is True + assert "already in progress" in reason.lower() + + def test_is_review_in_progress_not_started(self, mock_bot_detector): + """Test checking in-progress when review not started.""" + is_in_progress, reason = mock_bot_detector.is_review_in_progress(999) + + assert is_in_progress is False + assert reason == "" + + def test_is_review_in_progress_stale(self, mock_bot_detector): + """Test detecting stale in-progress review.""" + # Set review start time to 31 minutes ago (past timeout) + stale_time = datetime.now() - timedelta(minutes=31) + mock_bot_detector.state.in_progress_reviews["123"] = stale_time.isoformat() + + is_in_progress, reason = mock_bot_detector.is_review_in_progress(123) + + # Should detect as stale and clear it + assert is_in_progress is False + assert reason == "" + # Should be removed from state + assert "123" not in mock_bot_detector.state.in_progress_reviews + + def test_is_review_in_progress_invalid_timestamp(self, mock_bot_detector): + """Test handling invalid timestamp in in-progress state.""" + mock_bot_detector.state.in_progress_reviews["123"] = "invalid-timestamp" + + is_in_progress, reason = mock_bot_detector.is_review_in_progress(123) + + # Should clear invalid state + assert is_in_progress is False + assert reason == "" + assert "123" not in mock_bot_detector.state.in_progress_reviews + + def test_should_skip_review_in_progress(self, mock_bot_detector): + """Test skipping PR when review is in progress.""" + mock_bot_detector.mark_review_started(123) + + pr_data = {"author": {"login": "alice"}} + commits = [{"author": {"login": "alice"}, "oid": "abc123"}] + + should_skip, reason = mock_bot_detector.should_skip_pr_review( + pr_number=123, + pr_data=pr_data, + commits=commits, + ) + + assert should_skip is True + assert "already in progress" in reason.lower() + + def test_mark_reviewed_clears_in_progress(self, mock_bot_detector): + """Test that mark_reviewed also clears in-progress state.""" + mock_bot_detector.mark_review_started(123) + assert "123" in mock_bot_detector.state.in_progress_reviews + + mock_bot_detector.mark_reviewed(123, "abc123") + + # In-progress should be cleared + assert "123" not in mock_bot_detector.state.in_progress_reviews + # Reviewed state should be set + assert "123" in mock_bot_detector.state.reviewed_commits + assert "abc123" in mock_bot_detector.state.reviewed_commits["123"] + + def test_clear_pr_state_clears_in_progress(self, mock_bot_detector): + """Test that clear_pr_state also clears in-progress state.""" + mock_bot_detector.mark_review_started(123) + mock_bot_detector.mark_reviewed(123, "abc123") + + assert ( + "123" in mock_bot_detector.state.in_progress_reviews or True + ) # May be cleared by mark_reviewed + assert "123" in mock_bot_detector.state.reviewed_commits + + # Start another review + mock_bot_detector.mark_review_started(123) + assert "123" in mock_bot_detector.state.in_progress_reviews + + mock_bot_detector.clear_pr_state(123) + + # Everything should be cleared + assert "123" not in mock_bot_detector.state.in_progress_reviews + assert "123" not in mock_bot_detector.state.reviewed_commits + assert "123" not in mock_bot_detector.state.last_review_times + + def test_get_stats_includes_in_progress(self, mock_bot_detector): + """Test that get_stats includes in-progress count.""" + mock_bot_detector.mark_review_started(123) + mock_bot_detector.mark_review_started(456) + mock_bot_detector.mark_reviewed(789, "abc123") + + stats = mock_bot_detector.get_stats() + + assert stats["in_progress_reviews"] == 2 + assert stats["total_prs_tracked"] == 1 # Only 789 is tracked as reviewed + assert stats["in_progress_timeout_minutes"] == 30 + + def test_cleanup_stale_prs_removes_stale_in_progress(self, mock_bot_detector): + """Test that cleanup_stale_prs removes stale in-progress reviews.""" + # Add a stale in-progress review (32 minutes ago) + stale_time = datetime.now() - timedelta(minutes=32) + mock_bot_detector.state.in_progress_reviews["123"] = stale_time.isoformat() + + # Add an active in-progress review (5 minutes ago) + active_time = datetime.now() - timedelta(minutes=5) + mock_bot_detector.state.in_progress_reviews["456"] = active_time.isoformat() + + # Add a stale reviewed PR (40 days ago) + stale_review_time = datetime.now() - timedelta(days=40) + mock_bot_detector.state.reviewed_commits["789"] = ["abc123"] + mock_bot_detector.state.last_review_times["789"] = stale_review_time.isoformat() + + cleaned = mock_bot_detector.cleanup_stale_prs(max_age_days=30) + + # Should remove stale in-progress and stale reviewed PR + assert cleaned == 2 # 1 stale in-progress + 1 stale reviewed + assert "123" not in mock_bot_detector.state.in_progress_reviews + assert ( + "456" in mock_bot_detector.state.in_progress_reviews + ) # Active one remains + assert "789" not in mock_bot_detector.state.reviewed_commits + + if __name__ == "__main__": pytest.main([__file__, "-v"]) diff --git a/apps/backend/security/hooks.py b/apps/backend/security/hooks.py index 4bc7328d..0c344442 100644 --- a/apps/backend/security/hooks.py +++ b/apps/backend/security/hooks.py @@ -38,7 +38,7 @@ async def bash_security_hook( context: Optional context Returns: - Empty dict to allow, or {"decision": "block", "reason": "..."} to block + Empty dict to allow, or hookSpecificOutput with permissionDecision "deny" to block """ if input_data.get("tool_name") != "Bash": return {} @@ -49,15 +49,21 @@ async def bash_security_hook( # Check if tool_input is None (malformed tool call) if tool_input is None: return { - "decision": "block", - "reason": "Bash tool_input is None - malformed tool call from SDK", + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": "Bash tool_input is None - malformed tool call from SDK", + } } # Check if tool_input is a dict if not isinstance(tool_input, dict): return { - "decision": "block", - "reason": f"Bash tool_input must be dict, got {type(tool_input).__name__}", + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": f"Bash tool_input must be dict, got {type(tool_input).__name__}", + } } # Now safe to access command @@ -97,8 +103,11 @@ async def bash_security_hook( if not commands: # Could not parse - fail safe by blocking return { - "decision": "block", - "reason": f"Could not parse command for security validation: {command}", + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": f"Could not parse command for security validation: {command}", + } } # Split into segments for per-command validation @@ -114,8 +123,11 @@ async def bash_security_hook( if not is_allowed: return { - "decision": "block", - "reason": reason, + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": reason, + } } # Additional validation for sensitive commands @@ -127,7 +139,13 @@ async def bash_security_hook( validator = VALIDATORS[cmd] allowed, reason = validator(cmd_segment) if not allowed: - return {"decision": "block", "reason": reason} + return { + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": reason, + } + } return {} 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 5e490e2f..211d33f8 100644 --- a/apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts @@ -3,7 +3,7 @@ import { IPC_CHANNELS, AUTO_BUILD_PATHS, DEFAULT_APP_SETTINGS, DEFAULT_FEATURE_M import type { IPCResult, WorktreeStatus, WorktreeDiff, WorktreeDiffFile, WorktreeMergeResult, WorktreeDiscardResult, WorktreeListResult, WorktreeListItem, WorktreeCreatePROptions, WorktreeCreatePRResult, SupportedIDE, SupportedTerminal, AppSettings } from '../../../shared/types'; import path from 'path'; import { minimatch } from 'minimatch'; -import { existsSync, readdirSync, statSync, readFileSync } from 'fs'; +import { existsSync, readdirSync, statSync, readFileSync, promises as fsPromises } from 'fs'; import { execSync, execFileSync, spawn, spawnSync, exec, execFile } from 'child_process'; import { homedir } from 'os'; import { projectStore } from '../../project-store'; @@ -2009,6 +2009,18 @@ export function registerWorktreeHandlers( debug('TIMEOUT: Merge process exceeded', MERGE_TIMEOUT_MS, 'ms, killing...'); resolved = true; + // Send timeout error progress event to the renderer + const mainWindow = getMainWindow(); + if (mainWindow) { + mainWindow.webContents.send(IPC_CHANNELS.TASK_MERGE_PROGRESS, taskId, { + type: 'progress', + stage: 'error', + percent: 0, + message: 'Merge process timed out after 10 minutes', + details: {} + }); + } + // Platform-specific process termination with fallback killProcessGracefully(mergeProcess, { debugPrefix: '[MERGE]', @@ -2042,10 +2054,40 @@ export function registerWorktreeHandlers( } }, MERGE_TIMEOUT_MS); + let lineBuffer = ''; // Buffer for partial JSON lines spanning data chunks + mergeProcess.stdout.on('data', (data: Buffer) => { const chunk = data.toString('utf-8'); - stdout += chunk; debug('STDOUT:', chunk); + + // Prepend any buffered partial line from previous chunk + const combined = lineBuffer + chunk; + const lines = combined.split('\n'); + + // Last element may be a partial line - buffer it for next chunk + lineBuffer = lines.pop() || ''; + + for (const line of lines) { + const trimmed = line.trim(); + if (!trimmed) continue; + + try { + const parsed = JSON.parse(trimmed); + if (parsed && parsed.type === 'progress') { + const mainWindow = getMainWindow(); + if (mainWindow) { + mainWindow.webContents.send(IPC_CHANNELS.TASK_MERGE_PROGRESS, taskId, parsed); + } + // Don't accumulate progress lines in stdout - they are not part of the final result + continue; + } + } catch { + // Not valid JSON - treat as regular output + } + + // Accumulate non-progress lines for final result parsing + stdout += line + '\n'; + } }); mergeProcess.stderr.on('data', (data: Buffer) => { @@ -2060,6 +2102,24 @@ export function registerWorktreeHandlers( resolved = true; if (timeoutId) clearTimeout(timeoutId); + // Flush any remaining buffered line + if (lineBuffer.trim()) { + try { + const parsed = JSON.parse(lineBuffer.trim()); + if (parsed && parsed.type === 'progress') { + const mainWindow = getMainWindow(); + if (mainWindow) { + mainWindow.webContents.send(IPC_CHANNELS.TASK_MERGE_PROGRESS, taskId, parsed); + } + } else { + stdout += lineBuffer; + } + } catch { + stdout += lineBuffer; + } + lineBuffer = ''; + } + debug('Process exited with code:', code, 'signal:', signal); debug('Full stdout:', stdout); debug('Full stderr:', stderr); @@ -2353,6 +2413,19 @@ export function registerWorktreeHandlers( resolved = true; if (timeoutId) clearTimeout(timeoutId); console.error('[MERGE] Process spawn error:', err); + + // Send error progress event to the renderer + const mainWindow = getMainWindow(); + if (mainWindow) { + mainWindow.webContents.send(IPC_CHANNELS.TASK_MERGE_PROGRESS, taskId, { + type: 'progress', + stage: 'error', + percent: 0, + message: `Merge process crashed: ${err.message}`, + details: {} + }); + } + resolve({ success: false, error: `Failed to run merge: ${err.message}` @@ -2657,154 +2730,115 @@ export function registerWorktreeHandlers( } ); + // Promisified execFile for async git operations + const execFileAsync = promisify(execFile); + /** * List all spec worktrees for a project * Per-spec architecture: Each spec has its own worktree at .auto-claude/worktrees/tasks/{spec-name}/ - * - * Options: - * - includeStats: When true, fetches commit count, files changed, additions, deletions per worktree. - * When false (default), only fetches branch and base branch info for faster listing. */ ipcMain.handle( IPC_CHANNELS.TASK_LIST_WORKTREES, - async (_, projectId: string, options?: { includeStats?: boolean }): Promise> => { + async (_, projectId: string): Promise> => { try { const project = projectStore.getProject(projectId); if (!project) { return { success: false, error: 'Project not found' }; } - const includeStats = options?.includeStats ?? false; const worktreesDir = getTaskWorktreeDir(project.path); - const gitPath = getToolPath('git'); - // Detect the project's default branch once (main/master) instead of per-worktree - let projectDefaultBranch: string | undefined; - if (!project.settings?.mainBranch || !GIT_BRANCH_REGEX.test(project.settings.mainBranch)) { - for (const branch of ['main', 'master']) { - try { - await execFileAsync(gitPath, ['rev-parse', '--verify', branch], { - cwd: project.path, - encoding: 'utf-8', - }); - projectDefaultBranch = branch; - break; - } catch { - // Branch doesn't exist, try next - } - } - if (!projectDefaultBranch) { - projectDefaultBranch = 'main'; // fallback - } - } - - // Helper to get effective base branch using cached project default - const getBaseBranchForEntry = (entry: string): string => { - // 1. Try task metadata baseBranch - const specDir = path.join(project.path, '.auto-claude', 'specs', entry); - const taskBaseBranch = getTaskBaseBranch(specDir); - if (taskBaseBranch) return taskBaseBranch; - - // 2. Try project settings mainBranch - if (project.settings?.mainBranch && GIT_BRANCH_REGEX.test(project.settings.mainBranch)) { - return project.settings.mainBranch; - } - - // 3. Use pre-detected project default branch - return projectDefaultBranch || 'main'; - }; - - // Async helper to process a single worktree entry - const processWorktreeEntryAsync = async (entry: string, entryPath: string): Promise => { + // Helper to process a single worktree entry (async) + const processWorktreeEntry = async (entry: string, entryPath: string): Promise => { try { - // Get branch info (always needed) - const { stdout: branchOutput } = await execFileAsync(gitPath, ['rev-parse', '--abbrev-ref', 'HEAD'], { + // Get branch info (async) + const branchResult = await execFileAsync(getToolPath('git'), ['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: entryPath, encoding: 'utf-8' }); - const branch = branchOutput.trim(); + const branch = (branchResult.stdout as string).trim(); - const baseBranch = getBaseBranchForEntry(entry); + // 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, entry, project.settings?.mainBranch); - const item: WorktreeListItem = { + // Get commit count (async, cross-platform - no shell syntax) + let commitCount = 0; + try { + const countResult = await execFileAsync(getToolPath('git'), ['rev-list', '--count', `${baseBranch}..HEAD`], { + cwd: entryPath, + encoding: 'utf-8' + }); + commitCount = parseInt((countResult.stdout as string).trim(), 10) || 0; + } catch { + commitCount = 0; + } + + // Get diff stats (async, cross-platform - no shell syntax) + let filesChanged = 0; + let additions = 0; + let deletions = 0; + + try { + const diffResult = await execFileAsync(getToolPath('git'), ['diff', '--shortstat', `${baseBranch}...HEAD`], { + cwd: entryPath, + encoding: 'utf-8' + }); + const diffStat = (diffResult.stdout as string).trim(); + + const filesMatch = diffStat.match(/(\d+) files? changed/); + const addMatch = diffStat.match(/(\d+) insertions?/); + const delMatch = diffStat.match(/(\d+) deletions?/); + + if (filesMatch) filesChanged = parseInt(filesMatch[1], 10) || 0; + if (addMatch) additions = parseInt(addMatch[1], 10) || 0; + if (delMatch) deletions = parseInt(delMatch[1], 10) || 0; + } catch { + // Ignore diff errors + } + + return { specName: entry, path: entryPath, branch, baseBranch, + commitCount, + filesChanged, + additions, + deletions }; - - // Only fetch stats when requested - if (includeStats) { - // Run commit count and diff stats in parallel - const [countResult, diffResult] = await Promise.allSettled([ - execFileAsync(gitPath, ['rev-list', '--count', `${baseBranch}..HEAD`], { - cwd: entryPath, - encoding: 'utf-8', - }), - execFileAsync(gitPath, ['diff', '--shortstat', `${baseBranch}...HEAD`], { - cwd: entryPath, - encoding: 'utf-8', - }), - ]); - - if (countResult.status === 'fulfilled') { - item.commitCount = parseInt(countResult.value.stdout.trim(), 10) || 0; - } else { - item.commitCount = 0; - } - - if (diffResult.status === 'fulfilled') { - const diffStat = diffResult.value.stdout.trim(); - const filesMatch = diffStat.match(/(\d+) files? changed/); - const addMatch = diffStat.match(/(\d+) insertions?/); - const delMatch = diffStat.match(/(\d+) deletions?/); - - item.filesChanged = filesMatch ? parseInt(filesMatch[1], 10) || 0 : 0; - item.additions = addMatch ? parseInt(addMatch[1], 10) || 0 : 0; - item.deletions = delMatch ? parseInt(delMatch[1], 10) || 0 : 0; - } else { - item.filesChanged = 0; - item.additions = 0; - item.deletions = 0; - } - } - - return item; } catch (gitError) { console.error(`Error getting info for worktree ${entry}:`, gitError); + // Skip this worktree if we can't get git info return null; } }; - // Scan worktrees directory and process all entries in parallel - let worktrees: WorktreeListItem[] = []; - if (existsSync(worktreesDir)) { - const entries = readdirSync(worktreesDir); - const dirEntries: Array<{ entry: string; entryPath: string }> = []; - - for (const entry of entries) { - const entryPath = path.join(worktreesDir, entry); - try { - const stat = statSync(entryPath); - if (stat.isDirectory()) { - dirEntries.push({ entry, entryPath }); - } - } catch { - // Skip entries that can't be stat'd - } - } - - // Process all worktrees in parallel - const results = await Promise.allSettled( - dirEntries.map(({ entry, entryPath }) => processWorktreeEntryAsync(entry, entryPath)) - ); - - worktrees = results - .filter((r): r is PromiseFulfilledResult => r.status === 'fulfilled') - .map(r => r.value) - .filter((item): item is WorktreeListItem => item !== null); + // Scan worktrees directory (async) + if (!existsSync(worktreesDir)) { + return { success: true, data: { worktrees: [] } }; } + const entries = await fsPromises.readdir(worktreesDir); + + // Process all worktrees in parallel for better performance + const worktreePromises = entries.map(async (entry) => { + const entryPath = path.join(worktreesDir, entry); + try { + const stat = await fsPromises.stat(entryPath); + if (stat.isDirectory()) { + return processWorktreeEntry(entry, entryPath); + } + } catch { + // Skip entries that can't be stat'd + } + return null; + }); + + const results = await Promise.all(worktreePromises); + const worktrees = results.filter((w): w is WorktreeListItem => w !== null); + return { success: true, data: { worktrees } }; } catch (error) { console.error('Failed to list worktrees:', error); diff --git a/apps/frontend/src/preload/api/task-api.ts b/apps/frontend/src/preload/api/task-api.ts index 290fa787..7be2ded7 100644 --- a/apps/frontend/src/preload/api/task-api.ts +++ b/apps/frontend/src/preload/api/task-api.ts @@ -10,6 +10,7 @@ import type { TaskMetadata, TaskLogs, TaskLogStreamChunk, + MergeProgress, SupportedIDE, SupportedTerminal, WorktreeCreatePROptions, @@ -84,6 +85,9 @@ export interface TaskAPI { unwatchTaskLogs: (specId: string) => Promise; onTaskLogsChanged: (callback: (specId: string, logs: TaskLogs) => void) => () => void; onTaskLogsStream: (callback: (specId: string, chunk: TaskLogStreamChunk) => void) => () => void; + + // Merge Progress Events + onMergeProgress: (callback: (taskId: string, progress: MergeProgress) => void) => () => void; } export const createTaskAPI = (): TaskAPI => ({ @@ -308,5 +312,22 @@ export const createTaskAPI = (): TaskAPI => ({ return () => { ipcRenderer.removeListener(IPC_CHANNELS.TASK_LOGS_STREAM, handler); }; + }, + + // Merge Progress Events + onMergeProgress: ( + callback: (taskId: string, progress: MergeProgress) => void + ): (() => void) => { + const handler = ( + _event: Electron.IpcRendererEvent, + taskId: string, + progress: MergeProgress + ): void => { + callback(taskId, progress); + }; + ipcRenderer.on(IPC_CHANNELS.TASK_MERGE_PROGRESS, handler); + return () => { + ipcRenderer.removeListener(IPC_CHANNELS.TASK_MERGE_PROGRESS, handler); + }; } }); diff --git a/apps/frontend/src/renderer/components/ClaudeCodeStatusBadge.tsx b/apps/frontend/src/renderer/components/ClaudeCodeStatusBadge.tsx index c438df6c..6789c854 100644 --- a/apps/frontend/src/renderer/components/ClaudeCodeStatusBadge.tsx +++ b/apps/frontend/src/renderer/components/ClaudeCodeStatusBadge.tsx @@ -6,10 +6,33 @@ import { AlertTriangle, X, Loader2, + Download, + RefreshCw, + ExternalLink, + FolderOpen, } from "lucide-react"; +import { Button } from "./ui/button"; +import { Popover, PopoverContent, PopoverTrigger } from "./ui/popover"; import { Tooltip, TooltipContent, TooltipTrigger } from "./ui/tooltip"; +import { + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, +} from "./ui/alert-dialog"; +import { + Select, + SelectContent, + SelectItem, + SelectTrigger, + SelectValue, +} from "./ui/select"; import { cn } from "../lib/utils"; -import type { ClaudeCodeVersionInfo } from "../../shared/types/cli"; +import type { ClaudeCodeVersionInfo, ClaudeInstallationInfo } from "../../shared/types/cli"; interface ClaudeCodeStatusBadgeProps { className?: string; @@ -19,15 +42,36 @@ type StatusType = "loading" | "installed" | "outdated" | "not-found" | "error"; // Check every 24 hours const CHECK_INTERVAL_MS = 24 * 60 * 60 * 1000; +// Delay before re-checking version after install/update +const VERSION_RECHECK_DELAY_MS = 5000; /** * Claude Code CLI status badge for the sidebar. - * Shows installation status with a tooltip on hover. + * Shows installation status and provides quick access to install/update. */ export function ClaudeCodeStatusBadge({ className }: ClaudeCodeStatusBadgeProps) { const { t } = useTranslation(["common", "navigation"]); const [status, setStatus] = useState("loading"); const [versionInfo, setVersionInfo] = useState(null); + const [isInstalling, setIsInstalling] = useState(false); + const [lastChecked, setLastChecked] = useState(null); + const [isOpen, setIsOpen] = useState(false); + const [showUpdateWarning, setShowUpdateWarning] = useState(false); + + // Version rollback state + const [availableVersions, setAvailableVersions] = useState([]); + const [isLoadingVersions, setIsLoadingVersions] = useState(false); + const [versionsError, setVersionsError] = useState(null); + const [selectedVersion, setSelectedVersion] = useState(null); + const [showRollbackWarning, setShowRollbackWarning] = useState(false); + const [installError, setInstallError] = useState(null); + + // CLI path selection state + const [installations, setInstallations] = useState([]); + const [isLoadingInstallations, setIsLoadingInstallations] = useState(false); + const [installationsError, setInstallationsError] = useState(null); + const [selectedInstallation, setSelectedInstallation] = useState(null); + const [showPathChangeWarning, setShowPathChangeWarning] = useState(false); // Check Claude Code version const checkVersion = useCallback(async () => { @@ -41,6 +85,7 @@ export function ClaudeCodeStatusBadge({ className }: ClaudeCodeStatusBadgeProps) if (result.success && result.data) { setVersionInfo(result.data); + setLastChecked(new Date()); if (!result.data.installed) { setStatus("not-found"); @@ -52,11 +97,60 @@ export function ClaudeCodeStatusBadge({ className }: ClaudeCodeStatusBadgeProps) } else { setStatus("error"); } - } catch { + } catch (err) { + console.error("Failed to check Claude Code version:", err); setStatus("error"); } }, []); + // Fetch available versions + const fetchVersions = useCallback(async () => { + if (!window.electronAPI?.getClaudeCodeVersions) { + return; + } + + setIsLoadingVersions(true); + setVersionsError(null); + + try { + const result = await window.electronAPI.getClaudeCodeVersions(); + if (result.success && result.data) { + setAvailableVersions(result.data.versions); + } else { + setVersionsError(result.error || "Failed to load versions"); + } + } catch (err) { + console.error("Failed to fetch versions:", err); + setVersionsError("Failed to load versions"); + } finally { + setIsLoadingVersions(false); + } + }, []); + + // Fetch CLI installations + const fetchInstallations = useCallback(async () => { + if (!window.electronAPI?.getClaudeCodeInstallations) { + return; + } + + setIsLoadingInstallations(true); + setInstallationsError(null); + + try { + const result = await window.electronAPI.getClaudeCodeInstallations(); + if (result.success && result.data) { + setInstallations(result.data.installations); + } else { + setInstallationsError(result.error || "Failed to load installations"); + } + } catch (err) { + console.error("Failed to fetch installations:", err); + setInstallationsError("Failed to load installations"); + } finally { + setIsLoadingInstallations(false); + } + }, []); + // Initial check and periodic re-check useEffect(() => { checkVersion(); @@ -69,6 +163,156 @@ export function ClaudeCodeStatusBadge({ className }: ClaudeCodeStatusBadgeProps) return () => clearInterval(interval); }, [checkVersion]); + // Fetch versions when popover opens and Claude is installed + useEffect(() => { + if (isOpen && versionInfo?.installed && availableVersions.length === 0) { + fetchVersions(); + } + }, [isOpen, versionInfo?.installed, availableVersions.length, fetchVersions]); + + // Fetch installations when popover opens + useEffect(() => { + if (isOpen && installations.length === 0) { + fetchInstallations(); + } + }, [isOpen, installations.length, fetchInstallations]); + + // Perform the actual install/update + const performInstall = async () => { + setIsInstalling(true); + setShowUpdateWarning(false); + setInstallError(null); + try { + if (!window.electronAPI?.installClaudeCode) { + setInstallError("Installation not available"); + setIsInstalling(false); + return; + } + + const result = await window.electronAPI.installClaudeCode(); + + if (result.success) { + // Re-check after a delay + setTimeout(() => { + checkVersion(); + }, VERSION_RECHECK_DELAY_MS); + } else { + setInstallError(result.error || "Installation failed"); + } + } catch (err) { + console.error("Failed to install Claude Code:", err); + setInstallError(err instanceof Error ? err.message : "Installation failed"); + } finally { + setIsInstalling(false); + } + }; + + // Perform version rollback/switch + const performVersionSwitch = async () => { + if (!selectedVersion) return; + + setIsInstalling(true); + setShowRollbackWarning(false); + setInstallError(null); + + try { + if (!window.electronAPI?.installClaudeCodeVersion) { + setInstallError("Version switching not available"); + return; + } + + const result = await window.electronAPI.installClaudeCodeVersion(selectedVersion); + + if (result.success) { + // Re-check after a delay + setTimeout(() => { + checkVersion(); + }, VERSION_RECHECK_DELAY_MS); + } else { + setInstallError(result.error || "Failed to switch version"); + } + } catch (err) { + console.error("Failed to switch Claude Code version:", err); + setInstallError(err instanceof Error ? err.message : "Failed to switch version"); + } finally { + setIsInstalling(false); + setSelectedVersion(null); + } + }; + + // Perform CLI path switch + const performPathSwitch = async () => { + if (!selectedInstallation) return; + + setIsInstalling(true); + setShowPathChangeWarning(false); + setInstallError(null); + + try { + if (!window.electronAPI?.setClaudeCodeActivePath) { + setInstallError("Path switching not available"); + return; + } + + const result = await window.electronAPI.setClaudeCodeActivePath(selectedInstallation); + + if (result.success) { + // Re-check version and refresh installations + setTimeout(() => { + checkVersion(); + fetchInstallations(); + }, VERSION_RECHECK_DELAY_MS); + } else { + setInstallError(result.error || "Failed to switch CLI path"); + } + } catch (err) { + console.error("Failed to switch Claude CLI path:", err); + setInstallError(err instanceof Error ? err.message : "Failed to switch CLI path"); + } finally { + setIsInstalling(false); + setSelectedInstallation(null); + } + }; + + // Handle install/update button click + const handleInstall = () => { + if (status === "outdated") { + // Show warning for updates since it will close running Claude sessions + setShowUpdateWarning(true); + } else { + // Fresh install - no warning needed + performInstall(); + } + }; + + // Handle installation selection + const handleInstallationSelect = (cliPath: string) => { + // Don't do anything if it's the currently active installation + const installation = installations.find(i => i.path === cliPath); + if (installation?.isActive) { + return; + } + setInstallError(null); + setSelectedInstallation(cliPath); + setShowPathChangeWarning(true); + }; + + // Normalize version string by removing 'v' prefix for comparison + const normalizeVersion = (v: string) => v.replace(/^v/, ''); + + // Handle version selection + const handleVersionSelect = (version: string) => { + // Don't do anything if it's the currently installed version (normalize both for comparison) + const normalizedSelected = normalizeVersion(version); + const normalizedInstalled = versionInfo?.installed ? normalizeVersion(versionInfo.installed) : ''; + if (normalizedSelected === normalizedInstalled) { + return; + } + setInstallError(null); + setSelectedVersion(version); + setShowRollbackWarning(true); + }; + // Get status indicator color const getStatusColor = () => { switch (status) { @@ -84,27 +328,7 @@ export function ClaudeCodeStatusBadge({ className }: ClaudeCodeStatusBadgeProps) } }; - // Get tooltip text - const getTooltipText = () => { - switch (status) { - case "loading": - return t("navigation:claudeCode.checking", "Checking Claude Code..."); - case "installed": - return versionInfo?.installed - ? t("navigation:claudeCode.upToDateWithVersion", "Claude Code {{version}} installed", { - version: versionInfo.installed, - }) - : t("navigation:claudeCode.upToDate", "Claude Code is up to date"); - case "outdated": - return t("navigation:claudeCode.updateAvailable", "Claude Code update available"); - case "not-found": - return t("navigation:claudeCode.notInstalled", "Claude Code not installed"); - case "error": - return t("navigation:claudeCode.error", "Error checking Claude Code"); - } - }; - - // Get status icon for the badge + // Get status icon const getStatusIcon = () => { switch (status) { case "loading": @@ -114,52 +338,373 @@ export function ClaudeCodeStatusBadge({ className }: ClaudeCodeStatusBadgeProps) case "outdated": return ; case "not-found": - case "error": return ; + case "error": + return ; + } + }; + + // Get tooltip text + const getTooltipText = () => { + switch (status) { + case "loading": + return t("navigation:claudeCode.checking", "Checking Claude Code..."); + case "installed": + return t("navigation:claudeCode.upToDate", "Claude Code is up to date"); + case "outdated": + return t("navigation:claudeCode.updateAvailable", "Claude Code update available"); + case "not-found": + return t("navigation:claudeCode.notInstalled", "Claude Code not installed"); + case "error": + return t("navigation:claudeCode.error", "Error checking Claude Code"); } }; return ( - - -
-
- - + + + + + + + {getTooltipText()} + + + +
+ {/* Header */} +
+
+ +
+
+

Claude Code CLI

+

+ {getStatusIcon()} + {status === "installed" && t("navigation:claudeCode.installed", "Installed")} + {status === "outdated" && t("navigation:claudeCode.outdated", "Update available")} + {status === "not-found" && t("navigation:claudeCode.missing", "Not installed")} + {status === "loading" && t("navigation:claudeCode.checking", "Checking...")} + {status === "error" && t("navigation:claudeCode.error", "Error")} +

+
- Claude Code - {status === "outdated" && ( - - {getStatusIcon()} - {t("common:update", "Update")} - + + {/* Version info */} + {versionInfo && status !== "loading" && ( +
+ {versionInfo.installed && ( +
+ + {t("navigation:claudeCode.current", "Current")}: + + {versionInfo.installed} +
+ )} + {versionInfo.latest && versionInfo.latest !== "unknown" && ( +
+ + {t("navigation:claudeCode.latest", "Latest")}: + + {versionInfo.latest} +
+ )} + {versionInfo.path && ( +
+ + + {t("navigation:claudeCode.path", "Path")}: + + + {versionInfo.path} + +
+ )} + {lastChecked && ( +
+ {t("navigation:claudeCode.lastChecked", "Last checked")}: + {lastChecked.toLocaleTimeString()} +
+ )} +
)} - {status === "not-found" && ( - - {getStatusIcon()} - {t("common:install", "Install")} - + + {/* Actions */} +
+ {(status === "not-found" || status === "outdated") && ( + + )} + +
+ + {/* Install/Update error display */} + {installError && ( +
+ + {installError} +
)} - {status === "installed" && ( - - {getStatusIcon()} - + + {/* Version selector - only show when Claude is installed */} + {versionInfo?.installed && ( +
+ + +
)} + + {/* CLI Installation selector - show when multiple installations are found */} + {installations.length > 1 && ( +
+ + +
+ )} + + {/* Changelog link */} +
- - {getTooltipText()} - + + + {/* Update warning dialog */} + + + + + {t("navigation:claudeCode.updateWarningTitle", "Update Claude Code?")} + + + {t( + "navigation:claudeCode.updateWarningDescription", + "Updating will close all running Claude Code sessions. Any unsaved work in those sessions may be lost. Make sure to save your work before proceeding." + )} + + {t( + "navigation:claudeCode.updateWarningTerminalNote", + "A terminal window will open to run the installation command. Please wait for the installation to complete before continuing." + )} + + + + + {t("common:cancel", "Cancel")} + + {t("navigation:claudeCode.updateAnyway", "Open Terminal & Update")} + + + + + + {/* Version rollback warning dialog */} + + + + + {t("navigation:claudeCode.rollbackWarningTitle", "Switch to version {{version}}?", { + version: selectedVersion, + })} + + + {t( + "navigation:claudeCode.rollbackWarningDescription", + "Switching versions will close all running Claude Code sessions. Any unsaved work in those sessions may be lost. Make sure to save your work before proceeding." + )} + + {t( + "navigation:claudeCode.rollbackWarningTerminalNote", + "A terminal window will open to run the installation command. Please wait for the installation to complete before continuing." + )} + + + + + setSelectedVersion(null)}> + {t("common:cancel", "Cancel")} + + + {t("navigation:claudeCode.switchAnyway", "Open Terminal & Switch")} + + + + + + {/* Path change warning dialog */} + + + + + {t("navigation:claudeCode.pathChangeWarningTitle", "Switch CLI installation?")} + + + {t( + "navigation:claudeCode.pathChangeWarningDescription", + "Switching CLI installations will use a different Claude Code binary. Any running sessions will continue using the previous installation until restarted." + )} + + {selectedInstallation} + + + + + setSelectedInstallation(null)}> + {t("common:cancel", "Cancel")} + + + {t("navigation:claudeCode.switchInstallationConfirm", "Switch")} + + + + + ); } diff --git a/apps/frontend/src/renderer/components/task-detail/task-review/MergeProgressOverlay.tsx b/apps/frontend/src/renderer/components/task-detail/task-review/MergeProgressOverlay.tsx new file mode 100644 index 00000000..0d01fabc --- /dev/null +++ b/apps/frontend/src/renderer/components/task-detail/task-review/MergeProgressOverlay.tsx @@ -0,0 +1,226 @@ +import { useState, useRef, useEffect, useCallback } from 'react'; +import { useTranslation } from 'react-i18next'; +import { ChevronDown, ChevronRight, FileCode, AlertTriangle, Loader2, CheckCircle2, XCircle, Clock } from 'lucide-react'; +import { Progress } from '../../ui/progress'; +import { cn } from '../../../lib/utils'; +import type { MergeProgress, MergeLogEntry, MergeLogEntryType } from '../../../../shared/types'; + +interface MergeProgressOverlayProps { + mergeProgress: MergeProgress | null; + logEntries: MergeLogEntry[]; +} + +/** Time in ms without a progress update before showing stalled indicator */ +const STALL_THRESHOLD_MS = 30000; + +const STAGE_TO_I18N_KEY: Record = { + analyzing: 'stages.analyzing', + detecting_conflicts: 'stages.detectingConflicts', + resolving: 'stages.resolving', + validating: 'stages.validating', + complete: 'stages.complete', + error: 'stages.error', + stalled: 'stages.stalled', +}; + +const LOG_TYPE_COLORS: Record = { + info: 'text-info', + success: 'text-success', + warning: 'text-warning', + error: 'text-destructive', +}; + +/** + * Overlay component displaying real-time merge progress with a progress bar, + * stage label, conflict counter, current file indicator, and expandable log viewer. + * + * Detects stalled merges when no progress update is received for 30+ seconds. + */ +export function MergeProgressOverlay({ mergeProgress, logEntries }: MergeProgressOverlayProps) { + const { t } = useTranslation(['taskReview']); + const [logsExpanded, setLogsExpanded] = useState(false); + const [isStalled, setIsStalled] = useState(false); + const logContainerRef = useRef(null); + const stallTimerRef = useRef | null>(null); + + // Reset stall timer whenever we receive a new progress update + const resetStallTimer = useCallback(() => { + setIsStalled(false); + if (stallTimerRef.current) { + clearTimeout(stallTimerRef.current); + } + stallTimerRef.current = setTimeout(() => { + setIsStalled(true); + }, STALL_THRESHOLD_MS); + }, []); + + // Start/reset stall detection when progress updates arrive + useEffect(() => { + if (mergeProgress && mergeProgress.stage !== 'complete' && mergeProgress.stage !== 'error') { + resetStallTimer(); + } else { + // Clear timer on terminal states + setIsStalled(false); + if (stallTimerRef.current) { + clearTimeout(stallTimerRef.current); + stallTimerRef.current = null; + } + } + }, [mergeProgress, resetStallTimer]); + + // Cleanup stall timer on unmount + useEffect(() => { + return () => { + if (stallTimerRef.current) { + clearTimeout(stallTimerRef.current); + } + }; + }, []); + + // Auto-scroll log viewer to bottom when new entries arrive + useEffect(() => { + if (logsExpanded && logContainerRef.current) { + logContainerRef.current.scrollTop = logContainerRef.current.scrollHeight; + } + }, [logEntries, logsExpanded]); + + if (!mergeProgress) { + return null; + } + + const { stage, percent, message, details } = mergeProgress; + const isError = stage === 'error'; + const isComplete = stage === 'complete'; + + // Use stalled stage label when stalled, otherwise use the current stage + const effectiveStage = isStalled && !isError && !isComplete ? 'stalled' : stage; + const stageLabel = STAGE_TO_I18N_KEY[effectiveStage] + ? t(`taskReview:mergeProgress.${STAGE_TO_I18N_KEY[effectiveStage]}`) + : message; + + const conflictsFound = details?.conflicts_found ?? 0; + const conflictsResolved = details?.conflicts_resolved ?? 0; + const currentFile = details?.current_file; + + return ( +
+ {/* Stage label and percentage */} +
+
+ {isError ? ( + + ) : isComplete ? ( + + ) : isStalled ? ( + + ) : ( + + )} + + {stageLabel} + +
+ {percent}% +
+ + {/* Progress bar */} + div]:bg-destructive', + isComplete && '[&>div]:bg-success', + isStalled && !isError && !isComplete && '[&>div]:bg-warning', + !isError && !isComplete && !isStalled && '[&>div]:bg-info' + )} + animated={!isError && !isComplete} + /> + + {/* Conflict counter */} + {conflictsFound > 0 && ( +
+ + + {t('taskReview:mergeProgress.conflictCounter', { + found: conflictsFound, + resolved: conflictsResolved, + })} + +
+ )} + + {/* Current file indicator */} + {currentFile && !isComplete && ( +
+ + + {t('taskReview:mergeProgress.currentFile')}: {currentFile} + +
+ )} + + {/* Completion / error messages */} + {isComplete && ( +

{t('taskReview:mergeProgress.completionMessage')}

+ )} + {isError && ( +

{t('taskReview:mergeProgress.errorMessage')}

+ )} + + {/* Expandable log viewer */} + {logEntries.length > 0 && ( +
+ + + {logsExpanded && ( +
+
+ {logEntries.map((entry, idx) => ( +
+ + {new Date(entry.timestamp).toLocaleTimeString()} + + + {entry.message} + +
+ ))} +
+
+ )} +
+ )} +
+ ); +} 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 5948cd68..cf1429b4 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 @@ -1,3 +1,4 @@ +import { useState, useEffect, useRef } from 'react'; import { GitBranch, FileCode, @@ -13,14 +14,17 @@ import { CheckCircle, GitCommit, Code, - Terminal + Terminal, + Info, + CheckCheck } from 'lucide-react'; import { useTranslation } from 'react-i18next'; import { Button } from '../../ui/button'; import { Checkbox } from '../../ui/checkbox'; import { Tooltip, TooltipContent, TooltipTrigger } from '../../ui/tooltip'; import { cn } from '../../../lib/utils'; -import type { WorktreeStatus, MergeConflict, MergeStats, GitConflictInfo, SupportedIDE, SupportedTerminal } from '../../../../shared/types'; +import { MergeProgressOverlay } from './MergeProgressOverlay'; +import type { WorktreeStatus, MergeConflict, MergeStats, GitConflictInfo, SupportedIDE, SupportedTerminal, MergeProgress, MergeLogEntry, MergeLogEntryType } from '../../../../shared/types'; import { useSettingsStore } from '../../../stores/settings-store'; interface WorkspaceStatusProps { @@ -106,6 +110,97 @@ export function WorkspaceStatus({ const preferredIDE = settings.preferredIDE || 'vscode'; const preferredTerminal = settings.preferredTerminal || 'system'; + // Merge progress state + const [mergeProgress, setMergeProgress] = useState(null); + const [logEntries, setLogEntries] = useState([]); + const [showOverlay, setShowOverlay] = useState(false); + const prevIsMergingRef = useRef(isMerging); + const mergeStartTimeRef = useRef(null); + const minDisplayTimerRef = useRef | null>(null); + const ipcCleanupRef = useRef<(() => void) | null>(null); + + // Reset state when isMerging transitions from false → true + useEffect(() => { + if (isMerging && !prevIsMergingRef.current) { + setMergeProgress(null); + setLogEntries([]); + setShowOverlay(true); + mergeStartTimeRef.current = Date.now(); + } + prevIsMergingRef.current = isMerging; + }, [isMerging]); + + // Minimum display time: keep overlay visible for at least 500ms after merge ends + useEffect(() => { + if (!isMerging && showOverlay && mergeStartTimeRef.current !== null) { + const elapsed = Date.now() - mergeStartTimeRef.current; + const MIN_DISPLAY_MS = 500; + const remaining = Math.max(0, MIN_DISPLAY_MS - elapsed); + + if (remaining > 0) { + minDisplayTimerRef.current = setTimeout(() => { + setShowOverlay(false); + mergeStartTimeRef.current = null; + }, remaining); + } else { + setShowOverlay(false); + mergeStartTimeRef.current = null; + } + } + + return () => { + if (minDisplayTimerRef.current) { + clearTimeout(minDisplayTimerRef.current); + minDisplayTimerRef.current = null; + } + }; + }, [isMerging, showOverlay]); + + // Subscribe to merge progress IPC events + useEffect(() => { + if (!isMerging) return; + + const stageToLogType = (stage: string): MergeLogEntryType => { + switch (stage) { + case 'complete': return 'success'; + case 'error': return 'error'; + case 'resolving': return 'warning'; + default: return 'info'; + } + }; + + const cleanup = window.electronAPI.onMergeProgress((_taskId: string, progress: MergeProgress) => { + setMergeProgress(progress); + setLogEntries(prev => [ + ...prev, + { + timestamp: new Date().toISOString(), + type: stageToLogType(progress.stage), + message: progress.message, + details: progress.details?.current_file, + } + ]); + }); + + // Store cleanup ref so we can call it on unmount even if isMerging changes + ipcCleanupRef.current = cleanup; + + return cleanup; + }, [isMerging]); + + // Ensure IPC listener cleanup on unmount during active merge + useEffect(() => { + return () => { + if (ipcCleanupRef.current) { + ipcCleanupRef.current(); + ipcCleanupRef.current = null; + } + if (minDisplayTimerRef.current) { + clearTimeout(minDisplayTimerRef.current); + } + }; + }, []); + const handleOpenInIDE = async () => { if (!worktreeStatus.worktreePath) return; try { @@ -137,6 +232,12 @@ export function WorkspaceStatus({ const uncommittedCount = mergePreview?.uncommittedChanges?.count || 0; const hasAIConflicts = mergePreview && mergePreview.conflicts.length > 0; + // Conflict scenario detection for better UX messaging + const conflictScenario = mergePreview?.gitConflicts?.scenario; + const alreadyMergedFiles = mergePreview?.gitConflicts?.alreadyMergedFiles || []; + const isAlreadyMerged = conflictScenario === 'already_merged'; + const isSuperseded = conflictScenario === 'superseded'; + // Check if branch needs rebase (main has advanced since spec was created) // This requires AI merge even if no explicit file conflicts are detected const needsRebase = mergePreview?.gitConflicts?.needsRebase; @@ -177,7 +278,7 @@ export function WorkspaceStatus({
- {worktreeStatus.filesChanged || 0} files + {worktreeStatus.filesChanged || 0} {t('taskReview:merge.status.files')} @@ -267,8 +368,48 @@ export function WorkspaceStatus({
)} + {/* Already Merged Scenario - Show friendly message when task changes exist in target */} + {mergePreview && isAlreadyMerged && ( +
+ +
+

+ {t('taskReview:merge.alreadyMergedTitle')} +

+

+ {t('taskReview:merge.alreadyMergedDescription')} +

+ {alreadyMergedFiles.length > 0 && alreadyMergedFiles.length <= 5 && ( +
+ {t('taskReview:merge.matchingFiles')}: +
    + {alreadyMergedFiles.map(file => ( +
  • {file}
    • + ))} +
+
+ )} +
+
+ )} + + {/* Superseded Scenario - Target has newer version of changes */} + {mergePreview && isSuperseded && ( +
+ +
+

+ {t('taskReview:merge.supersededTitle')} +

+

+ {t('taskReview:merge.supersededDescription')} +

+
+
+ )} + {/* Merge Status */} - {mergePreview && ( + {mergePreview && !isAlreadyMerged && !isSuperseded && (
- Branch Diverged - AI will resolve + {t('taskReview:merge.status.branchDiverged')} + {t('taskReview:merge.status.aiWillResolve')}
) : isBranchBehind || hasPathMappedMerges ? ( @@ -291,26 +432,26 @@ export function WorkspaceStatus({
- {hasPathMappedMerges ? 'Files Renamed' : 'Branch Behind'} + {hasPathMappedMerges ? t('taskReview:merge.status.filesRenamed') : t('taskReview:merge.status.branchBehind')} - AI will resolve ({hasPathMappedMerges ? `${pathMappedAIMergeCount} files` : `${commitsBehind} commits`}) + {t('taskReview:merge.status.aiWillResolve')} ({hasPathMappedMerges ? `${pathMappedAIMergeCount} ${t('taskReview:merge.status.files')}` : `${commitsBehind} commits`})
) : !hasAIConflicts ? ( <> - Ready to merge + {t('taskReview:merge.status.readyToMerge')} - {mergePreview.summary.totalFiles} files + {mergePreview.summary.totalFiles} {t('taskReview:merge.status.files')} ) : ( <> - {mergePreview.conflicts.length} conflict{mergePreview.conflicts.length !== 1 ? 's' : ''} + {mergePreview.conflicts.length} {mergePreview.conflicts.length !== 1 ? t('taskReview:merge.status.conflicts') : t('taskReview:merge.status.conflict')} )} @@ -323,7 +464,7 @@ export function WorkspaceStatus({ onClick={() => onShowConflictDialog(true)} className="h-7 text-xs" > - Details + {t('taskReview:merge.status.details')} )}
)} - {/* Git Conflicts Details */} - {hasGitConflicts && mergePreview?.gitConflicts && ( + {/* Git Conflicts Details - hide for already_merged/superseded scenarios */} + {hasGitConflicts && mergePreview?.gitConflicts && !isAlreadyMerged && !isSuperseded && (
{t('taskReview:merge.branchHasNewCommits', { branch: mergePreview.gitConflicts.baseBranch, count: mergePreview.gitConflicts.commitsBehind })} {mergePreview.gitConflicts.conflictingFiles.length > 0 && ( @@ -357,7 +498,7 @@ export function WorkspaceStatus({ )} {/* Branch Behind Details (no explicit conflicts but needs AI merge due to path mappings) */} - {!hasGitConflicts && isBranchBehind && mergePreview?.gitConflicts && ( + {!hasGitConflicts && isBranchBehind && mergePreview?.gitConflicts && !isAlreadyMerged && !isSuperseded && (
{t('taskReview:merge.branchHasNewCommitsSinceBuild', { branch: mergePreview.gitConflicts.baseBranch, count: commitsBehind })} {hasPathMappedMerges ? ( @@ -373,10 +514,15 @@ export function WorkspaceStatus({ )}
+ {/* Merge Progress Overlay — shown during merge and for minimum display time after */} + {(isMerging || showOverlay) && ( + + )} + {/* Actions Footer */}
- {/* Stage Only Option - only show after conflicts have been checked */} - {mergePreview && ( + {/* Stage Only Option - only show after conflicts have been checked (not for already_merged/superseded) */} + {mergePreview && !isAlreadyMerged && !isSuperseded && ( )} @@ -417,8 +563,81 @@ export function WorkspaceStatus({ )} - {/* State 3: Merge preview loaded - show appropriate merge/stage button */} - {mergePreview && !isLoadingPreview && ( + {/* State 3a: Already Merged - show "Mark as Done" as primary action */} + {mergePreview && !isLoadingPreview && isAlreadyMerged && ( + + + + + +

+ {t('taskReview:merge.alreadyMergedTooltip')} +

+ + + )} + + {/* State 3b: Superseded - show both "View Comparison" and "Discard" */} + {mergePreview && !isLoadingPreview && isSuperseded && ( + <> + + + + + +

+ {t('taskReview:merge.supersededCompareTooltip')} +

+ + + + + + + +

+ {t('taskReview:merge.supersededDiscardTooltip')} +

+ + + + )} + + {/* State 3c: Normal merge - show appropriate merge/stage button */} + {mergePreview && !isLoadingPreview && !isAlreadyMerged && !isSuperseded && ( )} - + {/* Discard button - hide for superseded (shown as primary action there) */} + {!isSuperseded && ( + + )}
diff --git a/apps/frontend/src/renderer/lib/mocks/task-mock.ts b/apps/frontend/src/renderer/lib/mocks/task-mock.ts index 8432e0f0..ddcb12e1 100644 --- a/apps/frontend/src/renderer/lib/mocks/task-mock.ts +++ b/apps/frontend/src/renderer/lib/mocks/task-mock.ts @@ -97,5 +97,6 @@ export const taskMock = { onTaskStatusChange: () => () => {}, onTaskExecutionProgress: () => () => {}, onTaskLogsChanged: () => () => {}, - onTaskLogsStream: () => () => {} + onTaskLogsStream: () => () => {}, + onMergeProgress: () => () => {} }; diff --git a/apps/frontend/src/shared/constants/ipc.ts b/apps/frontend/src/shared/constants/ipc.ts index 6195383a..97de59be 100644 --- a/apps/frontend/src/shared/constants/ipc.ts +++ b/apps/frontend/src/shared/constants/ipc.ts @@ -62,6 +62,7 @@ export const IPC_CHANNELS = { TASK_LOGS_UNWATCH: 'task:logsUnwatch', // Stop watching for log changes TASK_LOGS_CHANGED: 'task:logsChanged', // Event: logs changed (main -> renderer) TASK_LOGS_STREAM: 'task:logsStream', // Event: streaming log chunk (main -> renderer) + TASK_MERGE_PROGRESS: 'task:mergeProgress', // Event: merge progress update (main -> renderer) // Terminal operations TERMINAL_CREATE: 'terminal:create', diff --git a/apps/frontend/src/shared/i18n/locales/en/navigation.json b/apps/frontend/src/shared/i18n/locales/en/navigation.json index 9e9db1cc..50483700 100644 --- a/apps/frontend/src/shared/i18n/locales/en/navigation.json +++ b/apps/frontend/src/shared/i18n/locales/en/navigation.json @@ -44,7 +44,6 @@ "claudeCode": { "checking": "Checking Claude Code...", "upToDate": "Claude Code is up to date", - "upToDateWithVersion": "Claude Code {{version}} installed", "updateAvailable": "Claude Code update available", "notInstalled": "Claude Code not installed", "error": "Error checking Claude Code", diff --git a/apps/frontend/src/shared/i18n/locales/en/taskReview.json b/apps/frontend/src/shared/i18n/locales/en/taskReview.json index a884d6d6..11f8550e 100644 --- a/apps/frontend/src/shared/i18n/locales/en/taskReview.json +++ b/apps/frontend/src/shared/i18n/locales/en/taskReview.json @@ -22,6 +22,29 @@ "fileRenamesDetected": "{{count}} file rename detected - AI will handle the merge.", "fileRenamesDetected_other": "{{count}} file renames detected - AI will handle the merge.", "filesRenamedOrMoved": "Files may have been renamed or moved - AI will handle the merge.", + "alreadyMergedTitle": "Changes already in your branch", + "alreadyMergedDescription": "These changes appear to already exist in your current branch. You can safely mark this task as done.", + "alreadyMergedTooltip": "The task's changes are already present in your branch. Marking as done will clean up the worktree without merging.", + "matchingFiles": "Matching files", + "supersededTitle": "Changes superseded", + "supersededDescription": "Your current branch has a newer version of these changes. Consider discarding this task or viewing the comparison.", + "supersededCompareTooltip": "View a detailed comparison to see how the current branch differs from this task's changes.", + "supersededDiscardTooltip": "Remove this task's worktree since the changes are no longer needed.", + "status": { + "branchDiverged": "Branch Diverged", + "aiWillResolve": "AI will resolve", + "filesRenamed": "Files Renamed", + "branchBehind": "Branch Behind", + "readyToMerge": "Ready to merge", + "files": "files", + "file": "file", + "conflict": "conflict", + "conflicts": "conflicts", + "details": "Details", + "refresh": "Refresh", + "stageOnly": "Stage only (review in IDE before committing)", + "discardBuild": "Discard build" + }, "buttons": { "stageWithAIMerge": "Stage with AI Merge", "mergeWithAI": "Merge with AI", @@ -29,7 +52,13 @@ "mergeTo": "Merge to {{branch}}", "resolving": "Resolving...", "staging": "Staging...", - "merging": "Merging..." + "merging": "Merging...", + "completing": "Completing..." + }, + "actions": { + "markAsDone": "Mark as Done", + "discardTask": "Discard Task", + "viewComparison": "View Comparison" } }, "pr": { @@ -63,6 +92,30 @@ "prTitle": "Leave empty to use the task title" } }, + "mergeProgress": { + "stages": { + "analyzing": "Analyzing changes", + "detectingConflicts": "Detecting conflicts", + "resolving": "Resolving conflicts", + "validating": "Validating merge", + "complete": "Merge complete", + "error": "Merge failed", + "stalled": "Merge stalled" + }, + "conflictCounter": "{{found}} found, {{resolved}} resolved", + "currentFile": "Current file", + "viewLogs": "View logs", + "hideLogs": "Hide logs", + "logTypes": { + "info": "Info", + "warning": "Warning", + "error": "Error", + "conflict": "Conflict", + "resolution": "Resolution" + }, + "completionMessage": "All changes have been merged successfully.", + "errorMessage": "An error occurred during the merge process." + }, "bulkPR": { "title": "Create Pull Requests", "description": "Create pull requests for {{count}} selected tasks", diff --git a/apps/frontend/src/shared/i18n/locales/fr/navigation.json b/apps/frontend/src/shared/i18n/locales/fr/navigation.json index 55e1a450..db779e10 100644 --- a/apps/frontend/src/shared/i18n/locales/fr/navigation.json +++ b/apps/frontend/src/shared/i18n/locales/fr/navigation.json @@ -44,7 +44,6 @@ "claudeCode": { "checking": "Vérification de Claude Code...", "upToDate": "Claude Code est à jour", - "upToDateWithVersion": "Claude Code {{version}} installé", "updateAvailable": "Mise à jour Claude Code disponible", "notInstalled": "Claude Code non installé", "error": "Erreur de vérification de Claude Code", diff --git a/apps/frontend/src/shared/i18n/locales/fr/taskReview.json b/apps/frontend/src/shared/i18n/locales/fr/taskReview.json index 46d7f84f..ada68345 100644 --- a/apps/frontend/src/shared/i18n/locales/fr/taskReview.json +++ b/apps/frontend/src/shared/i18n/locales/fr/taskReview.json @@ -22,6 +22,29 @@ "fileRenamesDetected": "{{count}} renommage de fichier détecté - l'IA gérera la fusion.", "fileRenamesDetected_other": "{{count}} renommages de fichiers détectés - l'IA gérera la fusion.", "filesRenamedOrMoved": "Des fichiers ont peut-être été renommés ou déplacés - l'IA gérera la fusion.", + "alreadyMergedTitle": "Modifications déjà dans votre branche", + "alreadyMergedDescription": "Ces modifications semblent déjà exister dans votre branche actuelle. Vous pouvez marquer cette tâche comme terminée en toute sécurité.", + "alreadyMergedTooltip": "Les modifications de la tâche sont déjà présentes dans votre branche. Marquer comme terminé nettoiera le worktree sans fusionner.", + "matchingFiles": "Fichiers correspondants", + "supersededTitle": "Modifications remplacées", + "supersededDescription": "Votre branche actuelle a une version plus récente de ces modifications. Envisagez de supprimer cette tâche ou de voir la comparaison.", + "supersededCompareTooltip": "Voir une comparaison détaillée pour voir comment la branche actuelle diffère des modifications de cette tâche.", + "supersededDiscardTooltip": "Supprimer le worktree de cette tâche puisque les modifications ne sont plus nécessaires.", + "status": { + "branchDiverged": "Branche divergée", + "aiWillResolve": "L'IA résoudra", + "filesRenamed": "Fichiers renommés", + "branchBehind": "Branche en retard", + "readyToMerge": "Prêt à fusionner", + "files": "fichiers", + "file": "fichier", + "conflict": "conflit", + "conflicts": "conflits", + "details": "Détails", + "refresh": "Actualiser", + "stageOnly": "Préparer seulement (réviser dans l'IDE avant de commiter)", + "discardBuild": "Supprimer le build" + }, "buttons": { "stageWithAIMerge": "Préparer avec fusion IA", "mergeWithAI": "Fusionner avec IA", @@ -29,7 +52,13 @@ "mergeTo": "Fusionner vers {{branch}}", "resolving": "Résolution...", "staging": "Préparation...", - "merging": "Fusion..." + "merging": "Fusion...", + "completing": "Finalisation..." + }, + "actions": { + "markAsDone": "Marquer comme terminé", + "discardTask": "Supprimer la tâche", + "viewComparison": "Voir la comparaison" } }, "pr": { @@ -63,6 +92,30 @@ "prTitle": "Laissez vide pour utiliser le titre de la tâche" } }, + "mergeProgress": { + "stages": { + "analyzing": "Analyse des modifications", + "detectingConflicts": "Détection des conflits", + "resolving": "Résolution des conflits", + "validating": "Validation de la fusion", + "complete": "Fusion terminée", + "error": "Échec de la fusion", + "stalled": "Fusion bloquée" + }, + "conflictCounter": "{{found}} trouvés, {{resolved}} résolus", + "currentFile": "Fichier actuel", + "viewLogs": "Voir les logs", + "hideLogs": "Masquer les logs", + "logTypes": { + "info": "Info", + "warning": "Avertissement", + "error": "Erreur", + "conflict": "Conflit", + "resolution": "Résolution" + }, + "completionMessage": "Toutes les modifications ont été fusionnées avec succès.", + "errorMessage": "Une erreur s'est produite pendant le processus de fusion." + }, "bulkPR": { "title": "Créer des Pull Requests", "description": "Créer des pull requests pour {{count}} tâches sélectionnées", diff --git a/apps/frontend/src/shared/types/ipc.ts b/apps/frontend/src/shared/types/ipc.ts index cc242bf7..972ed1db 100644 --- a/apps/frontend/src/shared/types/ipc.ts +++ b/apps/frontend/src/shared/types/ipc.ts @@ -3,6 +3,7 @@ */ import type { IPCResult } from './common'; +import type { KanbanPreferences } from './kanban'; import type { SupportedIDE, SupportedTerminal } from './settings'; import type { Project, @@ -44,7 +45,8 @@ import type { TaskMetadata, TaskLogs, TaskLogStreamChunk, - ImageAttachment + ImageAttachment, + MergeProgress } from './task'; import type { TerminalCreateOptions, @@ -136,7 +138,6 @@ import type { GitLabNewCommitsCheck } from './integrations'; import type { APIProfile, ProfilesFile, TestConnectionResult, DiscoverModelsResult } from './profile'; -import type { KanbanPreferences } from './kanban'; // Electron API exposed via contextBridge // Tab state interface (persisted in main process) @@ -159,7 +160,7 @@ export interface ElectronAPI { getTabState: () => Promise>; saveTabState: (tabState: TabState) => Promise; - // Kanban Preferences (persisted in main process per project) + // Kanban preferences (per-project column collapse state) getKanbanPreferences: (projectId: string) => Promise>; saveKanbanPreferences: (projectId: string, preferences: KanbanPreferences) => Promise; @@ -771,6 +772,9 @@ export interface ElectronAPI { onTaskLogsStream: ( callback: (specId: string, chunk: TaskLogStreamChunk) => void ) => () => void; + onMergeProgress: ( + callback: (taskId: string, progress: MergeProgress) => void + ) => () => void; // File explorer operations listDirectory: (dirPath: string) => Promise>; diff --git a/apps/frontend/src/shared/types/task.ts b/apps/frontend/src/shared/types/task.ts index c2095489..1104f503 100644 --- a/apps/frontend/src/shared/types/task.ts +++ b/apps/frontend/src/shared/types/task.ts @@ -352,6 +352,13 @@ export interface PathMappedAIMerge { reason: string; } +// Conflict scenario types for better UX messaging +// - 'already_merged': Task changes already identical in target branch +// - 'superseded': Target has newer version of same feature +// - 'diverged': Standard diverged branches (AI can resolve) +// - 'normal_conflict': Actual conflicting changes +export type ConflictScenario = 'already_merged' | 'superseded' | 'diverged' | 'normal_conflict'; + // Git-level conflict information (branch divergence) export interface GitConflictInfo { hasConflicts: boolean; @@ -364,6 +371,12 @@ export interface GitConflictInfo { pathMappedAIMerges?: PathMappedAIMerge[]; // Total number of file renames detected totalRenames?: number; + // Conflict scenario for better UX messaging + scenario?: ConflictScenario; + // Files that are already merged (identical in both branches) + alreadyMergedFiles?: string[]; + // Human-readable message about the scenario + scenarioMessage?: string; } // Summary statistics from merge preview/execution @@ -379,6 +392,30 @@ export interface MergeStats { pathMappedAIMergeCount?: number; } +// Merge progress tracking (for progress bar during merge operations) +export type MergeStage = 'analyzing' | 'detecting_conflicts' | 'resolving' | 'validating' | 'complete' | 'error'; + +export interface MergeProgress { + stage: MergeStage; + percent: number; + message: string; + details?: { + conflicts_found?: number; + conflicts_resolved?: number; + current_file?: string; + }; +} + +// Merge log entry (for conflict resolution logging) +export type MergeLogEntryType = 'info' | 'success' | 'warning' | 'error'; + +export interface MergeLogEntry { + timestamp: string; + type: MergeLogEntryType; + message: string; + details?: string; +} + export interface WorktreeMergeResult { success: boolean; message: string; @@ -442,10 +479,10 @@ export interface WorktreeListItem { path: string; branch: string; baseBranch: string; - commitCount?: number; - filesChanged?: number; - additions?: number; - deletions?: number; + commitCount: number; + filesChanged: number; + additions: number; + deletions: number; } /** diff --git a/tests/test_integration_phase4.py b/tests/test_integration_phase4.py index d06560a1..e725c1e7 100644 --- a/tests/test_integration_phase4.py +++ b/tests/test_integration_phase4.py @@ -27,20 +27,18 @@ import importlib.util # Load file_lock first (models.py depends on it) file_lock_spec = importlib.util.spec_from_file_location( - "file_lock", - backend_path / "runners" / "github" / "file_lock.py" + "file_lock", backend_path / "runners" / "github" / "file_lock.py" ) file_lock_module = importlib.util.module_from_spec(file_lock_spec) -sys.modules['file_lock'] = file_lock_module +sys.modules["file_lock"] = file_lock_module file_lock_spec.loader.exec_module(file_lock_module) # Load models next models_spec = importlib.util.spec_from_file_location( - "models", - backend_path / "runners" / "github" / "models.py" + "models", backend_path / "runners" / "github" / "models.py" ) models_module = importlib.util.module_from_spec(models_spec) -sys.modules['models'] = models_module +sys.modules["models"] = models_module models_spec.loader.exec_module(models_module) PRReviewFinding = models_module.PRReviewFinding PRReviewResult = models_module.PRReviewResult @@ -50,56 +48,58 @@ ReviewCategory = models_module.ReviewCategory # Load services module dependencies for parallel_orchestrator_reviewer category_utils_spec = importlib.util.spec_from_file_location( "category_utils", - backend_path / "runners" / "github" / "services" / "category_utils.py" + backend_path / "runners" / "github" / "services" / "category_utils.py", ) category_utils_module = importlib.util.module_from_spec(category_utils_spec) -sys.modules['services.category_utils'] = category_utils_module +sys.modules["services.category_utils"] = category_utils_module category_utils_spec.loader.exec_module(category_utils_module) # Load io_utils io_utils_spec = importlib.util.spec_from_file_location( - "io_utils", - backend_path / "runners" / "github" / "services" / "io_utils.py" + "io_utils", backend_path / "runners" / "github" / "services" / "io_utils.py" ) io_utils_module = importlib.util.module_from_spec(io_utils_spec) -sys.modules['services.io_utils'] = io_utils_module +sys.modules["services.io_utils"] = io_utils_module io_utils_spec.loader.exec_module(io_utils_module) # Load pydantic_models pydantic_models_spec = importlib.util.spec_from_file_location( "pydantic_models", - backend_path / "runners" / "github" / "services" / "pydantic_models.py" + backend_path / "runners" / "github" / "services" / "pydantic_models.py", ) pydantic_models_module = importlib.util.module_from_spec(pydantic_models_spec) -sys.modules['services.pydantic_models'] = pydantic_models_module +sys.modules["services.pydantic_models"] = pydantic_models_module pydantic_models_spec.loader.exec_module(pydantic_models_module) AgentAgreement = pydantic_models_module.AgentAgreement # Load agent_utils (shared utility for working directory injection) agent_utils_spec = importlib.util.spec_from_file_location( - "agent_utils", - backend_path / "runners" / "github" / "services" / "agent_utils.py" + "agent_utils", backend_path / "runners" / "github" / "services" / "agent_utils.py" ) agent_utils_module = importlib.util.module_from_spec(agent_utils_spec) -sys.modules['services.agent_utils'] = agent_utils_module +sys.modules["services.agent_utils"] = agent_utils_module agent_utils_spec.loader.exec_module(agent_utils_module) # Load parallel_orchestrator_reviewer (contains _is_finding_in_scope and _cross_validate_findings) orchestrator_spec = importlib.util.spec_from_file_location( "parallel_orchestrator_reviewer", - backend_path / "runners" / "github" / "services" / "parallel_orchestrator_reviewer.py" + backend_path + / "runners" + / "github" + / "services" + / "parallel_orchestrator_reviewer.py", ) orchestrator_module = importlib.util.module_from_spec(orchestrator_spec) # Mock dependencies that aren't needed for unit testing # IMPORTANT: Save and restore ALL mocked modules to avoid polluting sys.modules for other tests _modules_to_mock = [ - 'context_gatherer', - 'core.client', - 'gh_client', - 'phase_config', - 'services.pr_worktree_manager', - 'services.sdk_utils', - 'claude_agent_sdk', + "context_gatherer", + "core.client", + "gh_client", + "phase_config", + "services.pr_worktree_manager", + "services.sdk_utils", + "claude_agent_sdk", ] _original_modules = {name: sys.modules.get(name) for name in _modules_to_mock} for name in _modules_to_mock: @@ -120,6 +120,7 @@ _is_finding_in_scope = orchestrator_module._is_finding_in_scope # Phase 5+ Tests: Scope Filtering (Updated) # ============================================================================= + class TestScopeFiltering: """Test scope filtering logic (updated for Phase 5 - uses is_impact_finding schema field).""" @@ -132,11 +133,12 @@ class TestScopeFiltering: ParallelOrchestratorFinding Pydantic model. The actual code uses getattr(finding, 'is_impact_finding', False) to access it. """ + def _make_finding( file: str = "src/test.py", line: int = 10, is_impact_finding: bool = False, - **kwargs + **kwargs, ): defaults = { "id": "TEST001", @@ -152,6 +154,7 @@ class TestScopeFiltering: # Set is_impact_finding as attribute (accessed via getattr in _is_finding_in_scope) finding.is_impact_finding = is_impact_finding return finding + return _make_finding def test_finding_in_changed_files_passes(self, make_finding): @@ -166,9 +169,7 @@ class TestScopeFiltering: """Finding for a file NOT in changed_files should be filtered.""" changed_files = ["src/auth.py", "src/utils.py"] finding = make_finding( - file="src/database.py", - line=10, - description="This code has a bug" + file="src/database.py", line=10, description="This code has a bug" ) is_valid, reason = _is_finding_in_scope(finding, changed_files) @@ -199,7 +200,7 @@ class TestScopeFiltering: file="src/utils.py", line=10, is_impact_finding=True, # Schema field replaces keyword detection - description="This change breaks the helper function in utils.py" + description="This change breaks the helper function in utils.py", ) is_valid, _ = _is_finding_in_scope(finding, changed_files) assert is_valid @@ -213,7 +214,7 @@ class TestScopeFiltering: file="src/database.py", line=20, is_impact_finding=False, - description="database.py depends on modified auth module" + description="database.py depends on modified auth module", ) is_valid, reason = _is_finding_in_scope(finding, changed_files) assert not is_valid @@ -248,15 +249,14 @@ github_dir = backend_path / "runners" / "github" # Load context_gatherer module directly using spec loader # This avoids the complex package import chain _cg_spec = importlib.util.spec_from_file_location( - "context_gatherer_isolated", - github_dir / "context_gatherer.py" + "context_gatherer_isolated", github_dir / "context_gatherer.py" ) _cg_module = importlib.util.module_from_spec(_cg_spec) # Set up minimal module environment -sys.modules['context_gatherer_isolated'] = _cg_module +sys.modules["context_gatherer_isolated"] = _cg_module # Mock only the gh_client dependency _mock_gh = MagicMock() -sys.modules['gh_client'] = _mock_gh +sys.modules["gh_client"] = _mock_gh _cg_spec.loader.exec_module(_cg_module) PRContextGathererIsolated = _cg_module.PRContextGatherer @@ -278,7 +278,9 @@ class TestImportDetection: (src_dir / "config.ts").write_text("export const config = { debug: true };") # Create index.ts that re-exports - (src_dir / "index.ts").write_text("export * from './utils';\nexport { config } from './config';") + (src_dir / "index.ts").write_text( + "export * from './utils';\nexport { config } from './config';" + ) # Create shared directory shared_dir = src_dir / "shared" @@ -286,7 +288,9 @@ class TestImportDetection: (shared_dir / "types.ts").write_text("export type User = { id: string };") # Create Python module - (src_dir / "python_module.py").write_text("from .helpers import util_func\nimport os") + (src_dir / "python_module.py").write_text( + "from .helpers import util_func\nimport os" + ) (src_dir / "helpers.py").write_text("def util_func(): pass") (src_dir / "__init__.py").write_text("") @@ -295,19 +299,19 @@ class TestImportDetection: def test_path_alias_detection(self, temp_project): """Path alias imports (@/utils) should be detected and resolved.""" import json + # Create tsconfig.json with path aliases tsconfig = { "compilerOptions": { - "paths": { - "@/*": ["src/*"], - "@shared/*": ["src/shared/*"] - } + "paths": {"@/*": ["src/*"], "@shared/*": ["src/shared/*"]} } } (temp_project / "tsconfig.json").write_text(json.dumps(tsconfig)) # Create the target file that the alias points to - (temp_project / "src" / "utils.ts").write_text("export const helper = () => {};") + (temp_project / "src" / "utils.ts").write_text( + "export const helper = () => {};" + ) # Test file with alias import test_content = "import { helper } from '@/utils';" @@ -322,7 +326,9 @@ class TestImportDetection: assert isinstance(imports, set) # Normalize paths for cross-platform comparison (Windows uses backslashes) normalized_imports = {p.replace("\\", "/") for p in imports} - assert "src/utils.ts" in normalized_imports, f"Expected 'src/utils.ts' in imports, got: {imports}" + assert "src/utils.ts" in normalized_imports, ( + f"Expected 'src/utils.ts' in imports, got: {imports}" + ) def test_commonjs_require_detection(self, temp_project): """CommonJS require('./utils') should be detected.""" @@ -384,7 +390,18 @@ class TestImportDetection: class TestReverseDepDetection: - """Test reverse dependency detection (Phase 2).""" + """Test reverse dependency detection (Phase 2). + + ARCHITECTURE NOTE (2025-01): These tests document that programmatic file scanning + has been intentionally removed. The _find_dependents() method now returns an empty + set because LLM agents handle file discovery via their tools (Glob, Grep, Read). + + This design change: + - Removes the legacy 2000 file scan limit + - Lets LLM agents use their judgment to find relevant files + - Avoids pre-loading context that may not be needed + - Scales better for large codebases + """ @pytest.fixture def temp_project_with_deps(self, tmp_path): @@ -392,7 +409,7 @@ class TestReverseDepDetection: src_dir = tmp_path / "src" src_dir.mkdir() - # Create a utility file with non-generic name (helpers is in skip list) + # Create a utility file with non-generic name (src_dir / "formatter.ts").write_text( "export function format(s: string) { return s; }" ) @@ -405,63 +422,49 @@ class TestReverseDepDetection: "import { format } from './formatter';\nexport const fetch = () => {};" ) - # Create a file that does NOT import formatter - (src_dir / "standalone.ts").write_text( - "export const standalone = () => {};" - ) - return tmp_path - def test_finds_files_importing_changed_file(self, temp_project_with_deps): - """Verify grep-based detection finds files that import a given file.""" + def test_find_dependents_returns_empty_set(self, temp_project_with_deps): + """_find_dependents() returns empty - LLM agents discover files via tools. + + This is intentional: programmatic file scanning was removed in favor of + letting LLM agents use Glob/Grep/Read tools to discover relevant files + based on the PR context they receive. + """ gatherer = PRContextGathererIsolated(temp_project_with_deps, pr_number=1) - # Use non-generic name (helpers is in the skip list) dependents = gatherer._find_dependents("src/formatter.ts", max_results=10) - # Should find auth.ts and api.ts as dependents - assert any("auth.ts" in d for d in dependents) - assert any("api.ts" in d for d in dependents) - # Should NOT include standalone.ts - assert not any("standalone.ts" in d for d in dependents) + # Method now intentionally returns empty set + assert dependents == set() - def test_generic_names_not_skipped(self, tmp_path): - """Generic names (index, main, utils) are no longer skipped - LLM decides relevance.""" + def test_find_dependents_empty_for_any_file(self, tmp_path): + """Verify _find_dependents() returns empty for any input. + + The LLM-driven architecture means agents decide what's relevant, + not programmatic scanning. + """ src_dir = tmp_path / "src" src_dir.mkdir() - # Create files with generic names (src_dir / "index.ts").write_text("export * from './utils';") (src_dir / "main.ts").write_text("import { x } from './index';") gatherer = PRContextGathererIsolated(tmp_path, pr_number=1) + dependents = gatherer._find_dependents("src/index.ts") - # Generic names should NOT be skipped anymore (behavior changed in Phase 4) - # The LLM-driven system decides what's relevant based on PR context - dependents_index = gatherer._find_dependents("src/index.ts") + # Returns empty - LLM agents handle file discovery + assert dependents == set() - # main.ts imports index, so it should be found as a dependent - assert "src/main.ts" in dependents_index - - def test_respects_file_limit(self, tmp_path): - """Large repo search should stop after reaching file limit.""" + def test_find_dependents_returns_set_type(self, tmp_path): + """Verify _find_dependents() returns correct type (set).""" src_dir = tmp_path / "src" src_dir.mkdir() - (src_dir / "unique_name.ts").write_text("export const x = 1;") + (src_dir / "file.ts").write_text("export const x = 1;") gatherer = PRContextGathererIsolated(tmp_path, pr_number=1) + dependents = gatherer._find_dependents("src/file.ts") - # Mock os.walk to generate more files than the limit (2000) - # This simulates a large codebase without creating actual files - def mock_walk(path): - # Yield a directory with 3000 TypeScript files - yield (str(path), [], [f"file{i}.ts" for i in range(3000)]) - - with patch.object(_cg_module.os, "walk", mock_walk): - # The function should stop after max_files_to_check (2000) files - # and return gracefully without hanging - dependents = gatherer._find_dependents("src/unique_name.ts") - - # Should return a set (may be empty since mock files don't contain imports) + # Should return a set (empty, but correct type) assert isinstance(dependents, set) @@ -479,6 +482,7 @@ class TestCrossValidation: @pytest.fixture def make_finding(self): """Factory fixture to create PRReviewFinding instances.""" + def _make_finding( id: str = "TEST001", file: str = "src/test.py", @@ -487,7 +491,7 @@ class TestCrossValidation: severity: ReviewSeverity = ReviewSeverity.HIGH, confidence: float = 0.7, source_agents: list = None, - **kwargs + **kwargs, ): return PRReviewFinding( id=id, @@ -499,8 +503,11 @@ class TestCrossValidation: line=line, confidence=confidence, source_agents=source_agents or [], - **{k: v for k, v in kwargs.items() if k not in ["title", "description"]} + **{ + k: v for k, v in kwargs.items() if k not in ["title", "description"] + }, ) + return _make_finding @pytest.fixture @@ -508,18 +515,13 @@ class TestCrossValidation: """Create a mock ParallelOrchestratorReviewer instance.""" from models import GitHubRunnerConfig - config = GitHubRunnerConfig( - token="test-token", - repo="test/repo" - ) + config = GitHubRunnerConfig(token="test-token", repo="test/repo") # Create minimal directory structure github_dir = tmp_path / ".auto-claude" / "github" github_dir.mkdir(parents=True) reviewer = ParallelOrchestratorReviewer( - project_dir=tmp_path, - github_dir=github_dir, - config=config + project_dir=tmp_path, github_dir=github_dir, config=config ) return reviewer @@ -533,7 +535,7 @@ class TestCrossValidation: category=ReviewCategory.SECURITY, confidence=0.7, source_agents=["security-reviewer"], - description="SQL injection risk" + description="SQL injection risk", ) finding2 = make_finding( id="F2", @@ -542,10 +544,12 @@ class TestCrossValidation: category=ReviewCategory.SECURITY, confidence=0.6, source_agents=["quality-reviewer"], - description="Input not sanitized" + description="Input not sanitized", ) - validated, agreement = mock_reviewer._cross_validate_findings([finding1, finding2]) + validated, agreement = mock_reviewer._cross_validate_findings( + [finding1, finding2] + ) # Should merge into one finding assert len(validated) == 1 @@ -582,8 +586,12 @@ class TestCrossValidation: def test_merged_finding_has_cross_validated_true(self, make_finding, mock_reviewer): """Merged multi-agent findings should have cross_validated=True.""" - finding1 = make_finding(id="F1", file="src/test.py", line=5, source_agents=["agent1"]) - finding2 = make_finding(id="F2", file="src/test.py", line=5, source_agents=["agent2"]) + finding1 = make_finding( + id="F1", file="src/test.py", line=5, source_agents=["agent1"] + ) + finding2 = make_finding( + id="F2", file="src/test.py", line=5, source_agents=["agent2"] + ) validated, _ = mock_reviewer._cross_validate_findings([finding1, finding2])