diff --git a/apps/backend/runners/github/orchestrator.py b/apps/backend/runners/github/orchestrator.py index ad67d575..7c1da592 100644 --- a/apps/backend/runners/github/orchestrator.py +++ b/apps/backend/runners/github/orchestrator.py @@ -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 diff --git a/tests/test_github_pr_review.py b/tests/test_github_pr_review.py index 741d69bc..50b86064 100644 --- a/tests/test_github_pr_review.py +++ b/tests/test_github_pr_review.py @@ -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."""