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

fix(pr-review): allow re-review when previous review failed (#1268)

Browse Source 
* fix(pr-review): allow re-review when previous review failed

Previously, when a PR review failed (e.g., SDK validation error), the
bot detector would mark the commit as 'already reviewed' and refuse to
retry. This caused instant failures on subsequent review attempts.

Now, the orchestrator checks if the existing review was successful before
returning it. Failed reviews are no longer treated as blocking - instead,
the system allows a fresh review attempt.

Fixes: PR reviews failing instantly with 'Review failed: None'

* fix(pr-review): address PR review feedback

- Rename test_failed_review_allows_re_review to test_failed_review_model_persistence
  with updated docstring to accurately reflect what it tests (model persistence,
  not orchestrator re-review behavior)
- Extract duplicate skip result creation into _create_skip_result helper method
  to reduce code duplication in orchestrator.py

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-17 23:17:19 +01:00
committed by GitHub
parent d7ed770ed8
commit 4cc8f4dbfd
2 changed files with 97 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/backend/runners/github/orchestrator.py
+42 -13
View File
@@ -279,6 +279,33 @@ class GitHubOrchestrator:
flush=True,
)
# =========================================================================
# Helper Methods
# =========================================================================
async def _create_skip_result(
self, pr_number: int, skip_reason: str
) -> PRReviewResult:
"""Create and save a skip result for a PR that should not be reviewed.
Args:
pr_number: The PR number
skip_reason: Reason why the review was skipped
Returns:
PRReviewResult with success=True and skip reason in summary
"""
result = PRReviewResult(
pr_number=pr_number,
repo=self.config.repo,
success=True,
findings=[],
summary=f"Skipped review: {skip_reason}",
overall_status="comment",
)
await result.save(self.github_dir)
return result
# =========================================================================
# PR REVIEW WORKFLOW
# =========================================================================
@@ -349,7 +376,9 @@ class GitHubOrchestrator:
# instead of creating a new empty "skipped" result
if "Already reviewed" in skip_reason:
existing_review = PRReviewResult.load(self.github_dir, pr_number)
if existing_review:
# Only return existing review if it was successful
# A failed review should not block re-review attempts
if existing_review and existing_review.success:
safe_print(
"[BOT DETECTION] Returning existing review (no new commits)",
flush=True,
@@ -357,18 +386,18 @@ class GitHubOrchestrator:
# Don't overwrite - return the existing review as-is
# The frontend will see "no new commits" via the newCommitsCheck
return existing_review
# For other skip reasons (bot-authored, cooling off), create a skip result
result = PRReviewResult(
pr_number=pr_number,
repo=self.config.repo,
success=True,
findings=[],
summary=f"Skipped review: {skip_reason}",
overall_status="comment",
)
await result.save(self.github_dir)
return result
elif existing_review and not existing_review.success:
safe_print(
"[BOT DETECTION] Previous review failed, allowing re-review",
flush=True,
)
# Fall through to perform a new review (don't return here)
else:
# 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
return await self._create_skip_result(pr_number, skip_reason)
self._report_progress(
"analyzing", 30, "Running multi-pass review...", pr_number=pr_number
tests/test_github_pr_review.py
+55 -6
View File
@@ -37,6 +37,7 @@ from bot_detection import BotDetector, BotDetectionState
# Fixtures
# ============================================================================
@pytest.fixture
def temp_github_dir(tmp_path):
"""Create temporary GitHub directory structure."""
@@ -98,6 +99,7 @@ def mock_bot_detector(tmp_path):
# PRReviewResult Tests
# ============================================================================
class TestPRReviewResult:
"""Test PRReviewResult model."""
@@ -108,7 +110,9 @@ class TestPRReviewResult:
await sample_review_result.save(temp_github_dir)
# Verify file exists
review_file = temp_github_dir / "pr" / f"review_{sample_review_result.pr_number}.json"
review_file = (
temp_github_dir / "pr" / f"review_{sample_review_result.pr_number}.json"
)
assert review_file.exists()
# Load
@@ -192,6 +196,7 @@ class TestPRReviewFinding:
# Follow-up Review Context Tests
# ============================================================================
class TestFollowupReviewContext:
"""Test FollowupReviewContext model."""
@@ -225,7 +230,9 @@ class TestFollowupReviewContext:
assert context.error is not None
assert "Failed to compare commits" in context.error
def test_context_rebase_detected_files_changed_no_commits(self, sample_review_result):
def test_context_rebase_detected_files_changed_no_commits(
self, sample_review_result
):
"""Test follow-up context when PR was rebased (files changed but no trackable commits).
After a rebase/force-push, commit SHAs are rewritten so we can't identify "new" commits.
@@ -238,7 +245,10 @@ class TestFollowupReviewContext:
previous_commit_sha="abc123", # This SHA no longer exists in PR after rebase
current_commit_sha="xyz789",
commits_since_review=[], # Empty after rebase - can't determine "new" commits
files_changed_since_review=["src/db.py", "src/api.py"], # But blob comparison found changes
files_changed_since_review=[
"src/db.py",
"src/api.py",
], # But blob comparison found changes
diff_since_review="--- a/src/db.py\n+++ b/src/db.py\n@@ -1,3 +1,3 @@\n-old\n+new",
)
@@ -253,7 +263,9 @@ class TestFollowupReviewContext:
has_changes = bool(context.commits_since_review) or bool(
context.files_changed_since_review
)
assert has_changes is True, "Rebase with file changes should be treated as having changes"
assert has_changes is True, (
"Rebase with file changes should be treated as having changes"
)
def test_context_truly_no_changes(self, sample_review_result):
"""Test follow-up context when there are truly no changes (same SHA, no files)."""
@@ -278,6 +290,7 @@ class TestFollowupReviewContext:
# Bot Detection Integration Tests
# ============================================================================
class TestBotDetectionIntegration:
"""Test bot detection integration with review flow."""
@@ -332,11 +345,14 @@ class TestBotDetectionIntegration:
# Orchestrator Skip Logic Tests
# ============================================================================
class TestOrchestratorSkipLogic:
"""Test orchestrator behavior when bot detection skips."""
@pytest.mark.asyncio
async def test_skip_returns_existing_review(self, temp_github_dir, sample_review_result):
async def test_skip_returns_existing_review(
self, temp_github_dir, sample_review_result
):
"""Test that skipping 'Already reviewed' returns existing review."""
# Save existing review
await sample_review_result.save(temp_github_dir)
@@ -371,11 +387,39 @@ class TestOrchestratorSkipLogic:
assert len(result.findings) == 0
assert "bot user" in result.summary
@pytest.mark.asyncio
async def test_failed_review_model_persistence(self, temp_github_dir):
"""Test that a failed PRReviewResult can be saved and loaded with success=False.
This verifies that the model correctly persists failure state, which is
a prerequisite for the orchestrator's re-review logic (tested separately
in TestOrchestratorReReviewLogic).
"""
failed_review = PRReviewResult(
pr_number=789,
repo="test/repo",
success=False,
findings=[],
summary="Review failed: SDK validation error",
overall_status="comment",
error="SDK stream processing failed",
reviewed_commit_sha="abc123def456",
)
await failed_review.save(temp_github_dir)
# Verify the failed review can be loaded and maintains its failure state
loaded_review = PRReviewResult.load(temp_github_dir, 789)
assert loaded_review is not None
assert loaded_review.success is False
assert loaded_review.error == "SDK stream processing failed"
assert loaded_review.reviewed_commit_sha == "abc123def456"
# ============================================================================
# Follow-up Review Logic Tests
# ============================================================================
class TestFollowupReviewLogic:
"""Test follow-up review resolution logic."""
@@ -445,6 +489,7 @@ class TestFollowupReviewLogic:
# Posted Findings Tracking Tests
# ============================================================================
class TestPostedFindingsTracking:
"""Test posted findings tracking for follow-up eligibility."""
@@ -462,7 +507,9 @@ class TestPostedFindingsTracking:
assert len(sample_review_result.posted_finding_ids) == 1
@pytest.mark.asyncio
async def test_posted_findings_serialization(self, temp_github_dir, sample_review_result):
async def test_posted_findings_serialization(
self, temp_github_dir, sample_review_result
):
"""Test that posted findings are serialized correctly."""
# Set posted findings
sample_review_result.has_posted_findings = True
@@ -484,6 +531,7 @@ class TestPostedFindingsTracking:
# Error Handling Tests
# ============================================================================
class TestErrorHandling:
"""Test error handling in review flow."""
@@ -539,6 +587,7 @@ class TestErrorHandling:
# Blocker Generation Tests
# ============================================================================
class TestBlockerGeneration:
"""Test blocker generation from findings."""