From cbb1cb8154c06824d390bfb906c53fa8e7113cdd Mon Sep 17 00:00:00 2001 From: Andy <119136210+AndyMik90@users.noreply.github.com> Date: Thu, 8 Jan 2026 13:58:17 +0100 Subject: [PATCH] =?UTF-8?q?feat(github):=20enhance=20PR=20merge=20readines?= =?UTF-8?q?s=20checks=20with=20branch=20state=20val=E2=80=A6=20(#751)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(github): enhance PR merge readiness checks with branch state validation - Added support for checking if a PR branch is behind the base branch, introducing a new warning state for "Branch Out of Date." - Updated the verdict generation logic to classify this state as a soft blocker (NEEDS_REVISION) rather than a hard blocker. - Enhanced the merge readiness interface to include an `isBehind` property for better frontend integration. - Updated relevant services and handlers to accommodate the new branch state checks, ensuring accurate feedback during PR reviews. This improves the user experience by providing clearer guidance on necessary actions for PRs that are not up to date with the base branch. * fix: address PR feedback for branch-behind detection - Fix HIGH: Handle MERGE_WITH_CHANGES verdict when branch is behind - Fix MEDIUM: Extract duplicated reasoning strings to shared constants (BRANCH_BEHIND_BLOCKER_MSG, BRANCH_BEHIND_REASONING in models.py) - Fix LOW: Remove unreachable dead code for branch-behind checks in orchestrator.py and parallel_orchestrator_reviewer.py - Consolidate low-severity suggestions note into the active branch-behind path Co-authored-by: CodeRabbit 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 --- apps/backend/runners/github/models.py | 11 ++ apps/backend/runners/github/orchestrator.py | 19 +++ .../services/parallel_followup_reviewer.py | 19 +++ .../parallel_orchestrator_reviewer.py | 27 +++- .../github/services/pr_worktree_manager.py | 14 +- .../main/ipc-handlers/github/pr-handlers.ts | 12 ++ .../src/preload/api/modules/github-api.ts | 2 + .../frontend/src/renderer/lib/browser-mock.ts | 2 +- tests/test_pr_worktree_manager.py | 153 ++++++++++-------- 9 files changed, 184 insertions(+), 75 deletions(-) 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()