electron-rare/Aperant
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat(github): enhance PR merge readiness checks with branch state val… (#751)

Browse Source 
* 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 <coderabbit@users.noreply.github.com>

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Andy
2026-01-08 13:58:17 +01:00
committed by GitHub
parent 32e8fee3b2
commit cbb1cb8154
9 changed files with 184 additions and 75 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/backend/runners/github/models.py
+11
View File
@@ -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.)."""
apps/backend/runners/github/orchestrator.py
+19
View File
@@ -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"
apps/backend/runners/github/services/parallel_followup_reviewer.py
+19
View File
@@ -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 (
apps/backend/runners/github/services/parallel_orchestrator_reviewer.py
+23 -4
View File
@@ -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)"
apps/backend/runners/github/services/pr_worktree_manager.py
+13 -1
View File
@@ -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():
apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts
+12
View File
@@ -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,
};
apps/frontend/src/preload/api/modules/github-api.ts
+2
View File
@@ -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 */
apps/frontend/src/renderer/lib/browser-mock.ts
+1 -1
View File
@@ -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 }),
tests/test_pr_worktree_manager.py
+84 -69
View File
@@ -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()