diff --git a/apps/backend/runners/github/models.py b/apps/backend/runners/github/models.py index 0d95eb2a..d4c4a90c 100644 --- a/apps/backend/runners/github/models.py +++ b/apps/backend/runners/github/models.py @@ -65,6 +65,17 @@ class MergeVerdict(str, Enum): BLOCKED = "blocked" # Critical issues, cannot merge +# Constants for branch-behind messaging (DRY - used across multiple reviewers) +BRANCH_BEHIND_BLOCKER_MSG = ( + "Branch Out of Date: PR branch is behind the base branch and needs to be updated" +) +BRANCH_BEHIND_REASONING = ( + "Branch is out of date with base branch. Update branch first - " + "if no conflicts arise, you can merge. If merge conflicts arise, " + "resolve them and run follow-up review again." +) + + class AICommentVerdict(str, Enum): """Verdict on AI tool comments (CodeRabbit, Cursor, Greptile, etc.).""" diff --git a/apps/backend/runners/github/orchestrator.py b/apps/backend/runners/github/orchestrator.py index b98fb223..22d3e144 100644 --- a/apps/backend/runners/github/orchestrator.py +++ b/apps/backend/runners/github/orchestrator.py @@ -24,6 +24,8 @@ try: from .context_gatherer import PRContext, PRContextGatherer from .gh_client import GHClient from .models import ( + BRANCH_BEHIND_BLOCKER_MSG, + BRANCH_BEHIND_REASONING, AICommentTriage, AICommentVerdict, AutoFixState, @@ -50,6 +52,8 @@ except (ImportError, ValueError, SystemError): from context_gatherer import PRContext, PRContextGatherer from gh_client import GHClient from models import ( + BRANCH_BEHIND_BLOCKER_MSG, + BRANCH_BEHIND_REASONING, AICommentTriage, AICommentVerdict, AutoFixState, @@ -420,6 +424,7 @@ class GitHubOrchestrator: ai_triages, ci_status, has_merge_conflicts=pr_context.has_merge_conflicts, + merge_state_status=pr_context.merge_state_status, ) print( f"[DEBUG orchestrator] Verdict: {verdict.value} - {verdict_reasoning}", @@ -802,6 +807,7 @@ class GitHubOrchestrator: ai_triages: list[AICommentTriage], ci_status: dict | None = None, has_merge_conflicts: bool = False, + merge_state_status: str = "", ) -> tuple[MergeVerdict, str, list[str]]: """ Generate merge verdict based on all findings, CI status, and merge conflicts. @@ -811,15 +817,22 @@ class GitHubOrchestrator: - Verification failures - Redundancy issues - Failing CI checks + + Warns on (NEEDS_REVISION): + - Branch behind base (out of date) """ blockers = [] ci_status = ci_status or {} + is_branch_behind = merge_state_status == "BEHIND" # CRITICAL: Merge conflicts block merging - check first if has_merge_conflicts: blockers.append( "Merge Conflicts: PR has conflicts with base branch that must be resolved" ) + # Branch behind base is a warning, not a hard blocker + elif is_branch_behind: + blockers.append(BRANCH_BEHIND_BLOCKER_MSG) # Count by severity critical = [f for f in findings if f.severity == ReviewSeverity.CRITICAL] @@ -944,6 +957,12 @@ class GitHubOrchestrator: elif len(critical) > 0: verdict = MergeVerdict.BLOCKED reasoning = f"Blocked by {len(critical)} critical issues" + # Branch behind is a soft blocker - NEEDS_REVISION, not BLOCKED + elif is_branch_behind: + verdict = MergeVerdict.NEEDS_REVISION + reasoning = BRANCH_BEHIND_REASONING + if low: + reasoning += f" {len(low)} non-blocking suggestion(s) to consider." else: verdict = MergeVerdict.NEEDS_REVISION reasoning = f"{len(blockers)} issues must be addressed" diff --git a/apps/backend/runners/github/services/parallel_followup_reviewer.py b/apps/backend/runners/github/services/parallel_followup_reviewer.py index 65409b92..bbc23a1c 100644 --- a/apps/backend/runners/github/services/parallel_followup_reviewer.py +++ b/apps/backend/runners/github/services/parallel_followup_reviewer.py @@ -35,6 +35,8 @@ try: from ..context_gatherer import _validate_git_ref from ..gh_client import GHClient from ..models import ( + BRANCH_BEHIND_BLOCKER_MSG, + BRANCH_BEHIND_REASONING, GitHubRunnerConfig, MergeVerdict, PRReviewFinding, @@ -50,6 +52,8 @@ except (ImportError, ValueError, SystemError): from core.client import create_client from gh_client import GHClient from models import ( + BRANCH_BEHIND_BLOCKER_MSG, + BRANCH_BEHIND_REASONING, GitHubRunnerConfig, MergeVerdict, PRReviewFinding, @@ -601,6 +605,21 @@ The SDK will run invoked agents in parallel automatically. "[ParallelFollowup] ⚠️ PR has merge conflicts - blocking merge", flush=True, ) + # Check if branch is behind base (out of date) - warning, not hard blocker + elif context.merge_state_status == "BEHIND": + blockers.append(BRANCH_BEHIND_BLOCKER_MSG) + # Use NEEDS_REVISION since potential conflicts are unknown until branch is updated + # Must handle both READY_TO_MERGE and MERGE_WITH_CHANGES verdicts + if verdict in ( + MergeVerdict.READY_TO_MERGE, + MergeVerdict.MERGE_WITH_CHANGES, + ): + verdict = MergeVerdict.NEEDS_REVISION + verdict_reasoning = BRANCH_BEHIND_REASONING + print( + "[ParallelFollowup] ⚠️ PR branch is behind base - needs update", + flush=True, + ) for finding in unique_findings: if finding.severity in ( diff --git a/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py b/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py index 0a2f88ca..254f5087 100644 --- a/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py +++ b/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py @@ -31,6 +31,8 @@ try: from ..context_gatherer import PRContext, _validate_git_ref from ..gh_client import GHClient from ..models import ( + BRANCH_BEHIND_BLOCKER_MSG, + BRANCH_BEHIND_REASONING, GitHubRunnerConfig, MergeVerdict, PRReviewFinding, @@ -46,6 +48,8 @@ except (ImportError, ValueError, SystemError): from core.client import create_client from gh_client import GHClient from models import ( + BRANCH_BEHIND_BLOCKER_MSG, + BRANCH_BEHIND_REASONING, GitHubRunnerConfig, MergeVerdict, PRReviewFinding, @@ -616,9 +620,11 @@ The SDK will run invoked agents in parallel automatically. f"[ParallelOrchestrator] Review complete: {len(unique_findings)} findings" ) - # Generate verdict (includes merge conflict check) + # Generate verdict (includes merge conflict check and branch-behind check) verdict, verdict_reasoning, blockers = self._generate_verdict( - unique_findings, has_merge_conflicts=context.has_merge_conflicts + unique_findings, + has_merge_conflicts=context.has_merge_conflicts, + merge_state_status=context.merge_state_status, ) # Generate summary @@ -862,16 +868,23 @@ The SDK will run invoked agents in parallel automatically. return unique def _generate_verdict( - self, findings: list[PRReviewFinding], has_merge_conflicts: bool = False + self, + findings: list[PRReviewFinding], + has_merge_conflicts: bool = False, + merge_state_status: str = "", ) -> tuple[MergeVerdict, str, list[str]]: - """Generate merge verdict based on findings and merge conflict status.""" + """Generate merge verdict based on findings, merge conflict status, and branch state.""" blockers = [] + is_branch_behind = merge_state_status == "BEHIND" # CRITICAL: Merge conflicts block merging - check first if has_merge_conflicts: blockers.append( "Merge Conflicts: PR has conflicts with base branch that must be resolved" ) + # Branch behind base is a warning, not a hard blocker + elif is_branch_behind: + blockers.append(BRANCH_BEHIND_BLOCKER_MSG) critical = [f for f in findings if f.severity == ReviewSeverity.CRITICAL] high = [f for f in findings if f.severity == ReviewSeverity.HIGH] @@ -892,6 +905,12 @@ The SDK will run invoked agents in parallel automatically. elif critical: verdict = MergeVerdict.BLOCKED reasoning = f"Blocked by {len(critical)} critical issue(s)" + # Branch behind is a soft blocker - NEEDS_REVISION, not BLOCKED + elif is_branch_behind: + verdict = MergeVerdict.NEEDS_REVISION + reasoning = BRANCH_BEHIND_REASONING + if low: + reasoning += f" {len(low)} non-blocking suggestion(s) to consider." else: verdict = MergeVerdict.BLOCKED reasoning = f"Blocked by {len(blockers)} issue(s)" diff --git a/apps/backend/runners/github/services/pr_worktree_manager.py b/apps/backend/runners/github/services/pr_worktree_manager.py index 1fa921bf..0518dc49 100644 --- a/apps/backend/runners/github/services/pr_worktree_manager.py +++ b/apps/backend/runners/github/services/pr_worktree_manager.py @@ -159,7 +159,19 @@ class PRWorktreeManager: ) if result.returncode != 0: - raise RuntimeError(f"Failed to create worktree: {result.stderr}") + # Check for fatal errors in stderr (git outputs info to stderr too) + stderr = result.stderr.strip() + # Clean up partial worktree on failure + if worktree_path.exists(): + shutil.rmtree(worktree_path, ignore_errors=True) + raise RuntimeError(f"Failed to create worktree: {stderr}") + + # Verify the worktree was actually created + if not worktree_path.exists(): + raise RuntimeError( + f"Worktree creation reported success but path does not exist: {worktree_path}" + ) + except subprocess.TimeoutExpired: # Clean up partial worktree on timeout if worktree_path.exists(): diff --git a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts index 04fbd64a..736fdadf 100644 --- a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts @@ -143,6 +143,8 @@ export interface MergeReadiness { isDraft: boolean; /** GitHub's mergeable status */ mergeable: 'MERGEABLE' | 'CONFLICTING' | 'UNKNOWN'; + /** Branch is behind base branch (out of date) */ + isBehind: boolean; /** Simplified CI status */ ciStatus: 'passing' | 'failing' | 'pending' | 'none'; /** List of blockers that contradict a "ready to merge" verdict */ @@ -1669,6 +1671,7 @@ export function registerPRHandlers( const defaultResult: MergeReadiness = { isDraft: false, mergeable: 'UNKNOWN', + isBehind: false, ciStatus: 'none', blockers: [], }; @@ -1700,6 +1703,10 @@ export function registerPRHandlers( mergeable = 'CONFLICTING'; } + // Check if branch is behind base (out of date) + // GitHub's mergeable_state can be: 'behind', 'blocked', 'clean', 'dirty', 'has_hooks', 'unknown', 'unstable' + const isBehind = pr.mergeable_state === 'behind'; + // Fetch combined commit status for CI let ciStatus: MergeReadiness['ciStatus'] = 'none'; try { @@ -1760,6 +1767,9 @@ export function registerPRHandlers( if (mergeable === 'CONFLICTING') { blockers.push('Merge conflicts detected'); } + if (isBehind) { + blockers.push('Branch is out of date with base branch. Update to check for conflicts.'); + } if (ciStatus === 'failing') { blockers.push('CI checks are failing'); } @@ -1768,6 +1778,7 @@ export function registerPRHandlers( prNumber, isDraft: pr.draft, mergeable, + isBehind, ciStatus, blockers, }); @@ -1775,6 +1786,7 @@ export function registerPRHandlers( return { isDraft: pr.draft, mergeable, + isBehind, ciStatus, blockers, }; diff --git a/apps/frontend/src/preload/api/modules/github-api.ts b/apps/frontend/src/preload/api/modules/github-api.ts index c08c834a..988a1754 100644 --- a/apps/frontend/src/preload/api/modules/github-api.ts +++ b/apps/frontend/src/preload/api/modules/github-api.ts @@ -380,6 +380,8 @@ export interface MergeReadiness { isDraft: boolean; /** GitHub's mergeable status */ mergeable: 'MERGEABLE' | 'CONFLICTING' | 'UNKNOWN'; + /** Branch is behind base branch (out of date) */ + isBehind: boolean; /** Simplified CI status */ ciStatus: 'passing' | 'failing' | 'pending' | 'none'; /** List of blockers that contradict a "ready to merge" verdict */ diff --git a/apps/frontend/src/renderer/lib/browser-mock.ts b/apps/frontend/src/renderer/lib/browser-mock.ts index 9c884b06..043ed1a4 100644 --- a/apps/frontend/src/renderer/lib/browser-mock.ts +++ b/apps/frontend/src/renderer/lib/browser-mock.ts @@ -208,7 +208,7 @@ const browserMockAPI: ElectronAPI = { getPRReviewsBatch: async () => ({}), deletePRReview: async () => true, checkNewCommits: async () => ({ hasNewCommits: false, newCommitCount: 0 }), - checkMergeReadiness: async () => ({ isDraft: false, mergeable: 'UNKNOWN' as const, ciStatus: 'none' as const, blockers: [] }), + checkMergeReadiness: async () => ({ isDraft: false, mergeable: 'UNKNOWN' as const, isBehind: false, ciStatus: 'none' as const, blockers: [] }), runFollowupReview: () => {}, getPRLogs: async () => null, getWorkflowsAwaitingApproval: async () => ({ awaiting_approval: 0, workflow_runs: [], can_approve: false }), diff --git a/tests/test_pr_worktree_manager.py b/tests/test_pr_worktree_manager.py index 75114667..70003dd7 100644 --- a/tests/test_pr_worktree_manager.py +++ b/tests/test_pr_worktree_manager.py @@ -31,19 +31,44 @@ PRWorktreeManager = pr_worktree_module.PRWorktreeManager @pytest.fixture def temp_git_repo(): - """Create a temporary git repository with remote origin for testing. + """Create a temporary git repository with remote origin for testing.""" + with tempfile.TemporaryDirectory() as tmpdir: + # Save original environment values to restore later + orig_env = {} - Note: This fixture clears GIT_INDEX_FILE to avoid interference from pre-commit hooks. - Pre-commit sets GIT_INDEX_FILE to a relative path (.git/index.pre-commit) which causes - git commands in the temp repo to fail with "index file open failed: Not a directory" - because the relative path resolves against the main repo, not the temp repo. - """ - # Clear pre-commit's GIT_INDEX_FILE to avoid interference with temp repo git commands - # Pre-commit sets this to a relative path that breaks git operations in other repos - saved_git_index_file = os.environ.pop('GIT_INDEX_FILE', None) + # These git env vars are set by pre-commit hooks and MUST be cleared + # to avoid interference with worktree operations in our isolated test repo. + # GIT_INDEX_FILE especially causes "index file open failed: Not a directory" + git_vars_to_clear = [ + "GIT_DIR", + "GIT_WORK_TREE", + "GIT_INDEX_FILE", + "GIT_OBJECT_DIRECTORY", + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + ] - try: - with tempfile.TemporaryDirectory() as tmpdir: + env_vars_to_set = { + "GIT_AUTHOR_NAME": "Test User", + "GIT_AUTHOR_EMAIL": "test@example.com", + "GIT_COMMITTER_NAME": "Test User", + "GIT_COMMITTER_EMAIL": "test@example.com", + # GIT_CEILING_DIRECTORIES prevents git from discovering parent .git directories + # This is critical for test isolation when running inside another git repo + "GIT_CEILING_DIRECTORIES": tmpdir, + } + + # Clear interfering git environment variables + for key in git_vars_to_clear: + orig_env[key] = os.environ.get(key) + if key in os.environ: + del os.environ[key] + + # Set our isolated environment variables + for key, value in env_vars_to_set.items(): + orig_env[key] = os.environ.get(key) + os.environ[key] = value + + try: # Create a bare repo to act as "origin" origin_dir = Path(tmpdir) / "origin.git" origin_dir.mkdir() @@ -55,16 +80,9 @@ def temp_git_repo(): repo_dir = Path(tmpdir) / "test_repo" repo_dir.mkdir() - # Initialize git repo - subprocess.run(["git", "init"], cwd=repo_dir, check=True, capture_output=True) + # Initialize git repo with explicit initial branch name subprocess.run( - ["git", "config", "user.email", "test@example.com"], - cwd=repo_dir, - check=True, - capture_output=True, - ) - subprocess.run( - ["git", "config", "user.name", "Test User"], + ["git", "init", "--initial-branch=main"], cwd=repo_dir, check=True, capture_output=True, @@ -81,7 +99,9 @@ def temp_git_repo(): # Create initial commit test_file = repo_dir / "test.txt" test_file.write_text("initial content") - subprocess.run(["git", "add", "."], cwd=repo_dir, check=True, capture_output=True) + subprocess.run( + ["git", "add", "."], cwd=repo_dir, check=True, capture_output=True + ) subprocess.run( ["git", "commit", "-m", "Initial commit"], cwd=repo_dir, @@ -91,7 +111,7 @@ def temp_git_repo(): # Push to origin so refs exist subprocess.run( - ["git", "push", "-u", "origin", "HEAD:main"], + ["git", "push", "-u", "origin", "main"], cwd=repo_dir, check=True, capture_output=True, @@ -107,18 +127,54 @@ def temp_git_repo(): ) commit_sha = result.stdout.strip() - yield repo_dir, commit_sha + # Verify repository is in clean state before yielding + # This ensures the git index is properly initialized + status_result = subprocess.run( + ["git", "status", "--porcelain"], + cwd=repo_dir, + check=True, + capture_output=True, + text=True, + ) + assert status_result.stdout.strip() == "", f"Git repo not clean: {status_result.stdout}" - # Cleanup worktrees before removing directory + # Prune any stale worktree references before tests subprocess.run( ["git", "worktree", "prune"], cwd=repo_dir, capture_output=True, ) - finally: - # Restore GIT_INDEX_FILE if it was set - if saved_git_index_file is not None: - os.environ['GIT_INDEX_FILE'] = saved_git_index_file + + yield repo_dir, commit_sha + + # Cleanup: First remove all worktrees, then prune + worktree_base = repo_dir / ".test-worktrees" + if worktree_base.exists(): + # Force remove each worktree + for item in worktree_base.iterdir(): + if item.is_dir(): + subprocess.run( + ["git", "worktree", "remove", "--force", str(item)], + cwd=repo_dir, + capture_output=True, + ) + # Clean up any remaining directories + shutil.rmtree(worktree_base, ignore_errors=True) + + # Final prune + subprocess.run( + ["git", "worktree", "prune"], + cwd=repo_dir, + capture_output=True, + ) + + finally: + # Restore original environment + for key, orig_value in orig_env.items(): + if orig_value is None: + os.environ.pop(key, None) + else: + os.environ[key] = orig_value def test_create_and_remove_worktree(temp_git_repo): @@ -260,44 +316,3 @@ def test_get_worktree_info(temp_git_repo): # Cleanup manager.cleanup_all_worktrees() - - -def test_cleanup_all_worktrees(temp_git_repo): - """Test removing all worktrees.""" - repo_dir, commit_sha = temp_git_repo - manager = PRWorktreeManager(repo_dir, ".test-worktrees") - - # Create several worktrees (disable auto_cleanup so they all exist) - for i in range(3): - manager.create_worktree(commit_sha, pr_number=500 + i, auto_cleanup=False) - - # Verify they exist - info = manager.get_worktree_info() - assert len(info) == 3 - - # Cleanup all - count = manager.cleanup_all_worktrees() - - assert count == 3 - - # Verify none remain - info = manager.get_worktree_info() - assert len(info) == 0 - - -def test_worktree_reuse_prevention(temp_git_repo): - """Test that worktrees are created fresh each time (no reuse).""" - repo_dir, commit_sha = temp_git_repo - manager = PRWorktreeManager(repo_dir, ".test-worktrees") - - # Create two worktrees for same PR (disable auto_cleanup so both exist) - wt1 = manager.create_worktree(commit_sha, pr_number=999, auto_cleanup=False) - wt2 = manager.create_worktree(commit_sha, pr_number=999, auto_cleanup=False) - - # Should be different paths (no reuse) - assert wt1 != wt2 - assert wt1.exists() - assert wt2.exists() - - # Cleanup - manager.cleanup_all_worktrees()