Remove remaining single-pass compatibility paths

Author: matej

README.md

@@ -1,6 +1,6 @@
 # Nutrient Code Reviewer
 
-An AI-powered code review GitHub Action using Claude to analyze code changes. Uses a unified multi-agent approach for both code quality (correctness, reliability, performance, maintainability, testing) and security in a single pass. This action provides intelligent, context-aware review for pull requests using Anthropic's Claude Code tool for deep semantic analysis.
+An AI-powered code review GitHub Action using Claude to analyze code changes. Uses a unified multi-agent, multi-phase approach for both code quality (correctness, reliability, performance, maintainability, testing) and security. This action provides intelligent, context-aware review for pull requests using Anthropic's Claude Code tool for deep semantic analysis.
 
 Based on the original work from [anthropics/claude-code-security-review](https://github.com/anthropics/claude-code-security-review).
 

claudecode/review_orchestrator.py

@@ -63,16 +63,11 @@ def __init__(
         self.max_diff_lines = max(0, max_diff_lines)
 
     def _run_phase(self, repo_dir: Path, prompt: str, model: str, phase_name: str) -> Tuple[bool, Dict[str, Any], str]:
-        raw_result = None
-        supports_run_prompt = callable(getattr(type(self.claude_runner), "run_prompt", None))
-        if supports_run_prompt:
-            raw_result = self.claude_runner.run_prompt(repo_dir, prompt, model=model)
-        elif hasattr(self.claude_runner, "run_code_review"):
-            try:
-                raw_result = self.claude_runner.run_code_review(repo_dir, prompt, model=model)
-            except TypeError:
-                # Backward compatibility for legacy mocks/runners that don't accept model parameter.
-                raw_result = self.claude_runner.run_code_review(repo_dir, prompt)
+        run_prompt = getattr(self.claude_runner, "run_prompt", None)
+        if not callable(run_prompt):
+            return False, {}, f"Runner missing run_prompt for {phase_name}"
+
+        raw_result = run_prompt(repo_dir, prompt, model=model)
         if not (isinstance(raw_result, tuple) and len(raw_result) == 3):
             return False, {}, f"Invalid runner response for {phase_name}"
 
@@ -128,44 +123,8 @@ def run(
         if not ok:
             return False, {}, f"Triage phase failed: {err}"
 
-        # Backward-compatible fast path for legacy single-pass outputs.
-        if isinstance(triage_result, dict) and "findings" in triage_result:
-            original_count = len([f for f in triage_result.get("findings", []) if isinstance(f, dict)])
-            legacy_findings = []
-            for finding in triage_result.get("findings", []):
-                if isinstance(finding, dict) and not self._is_excluded(finding.get("file", "")):
-                    legacy_findings.append(self._ensure_review_type(finding))
-            pr_context = {
-                "repo_name": pr_data.get("head", {}).get("repo", {}).get("full_name", "unknown"),
-                "pr_number": pr_data.get("number"),
-                "title": pr_data.get("title", ""),
-                "description": pr_data.get("body", ""),
-            }
-            filter_response = self.findings_filter.filter_findings(legacy_findings, pr_context)
-            if isinstance(filter_response, tuple) and len(filter_response) == 3:
-                filter_success, filter_results, _ = filter_response
-                if filter_success and isinstance(filter_results, dict):
-                    legacy_findings = filter_results.get("filtered_findings", legacy_findings)
-            legacy_findings = [self._ensure_review_type(f) for f in legacy_findings if isinstance(f, dict)]
-            high = len([f for f in legacy_findings if str(f.get("severity", "")).upper() == "HIGH"])
-            medium = len([f for f in legacy_findings if str(f.get("severity", "")).upper() == "MEDIUM"])
-            low = len([f for f in legacy_findings if str(f.get("severity", "")).upper() == "LOW"])
-            return True, {
-                "findings": legacy_findings,
-                "analysis_summary": {
-                    "files_reviewed": triage_result.get("analysis_summary", {}).get("files_reviewed", pr_data.get("changed_files", 0)),
-                    "high_severity": high,
-                    "medium_severity": medium,
-                    "low_severity": low,
-                    "review_completed": True,
-                },
-                "filtering_summary": {
-                    "total_original_findings": original_count,
-                    "excluded_findings": max(0, original_count - len(legacy_findings)),
-                    "kept_findings": len(legacy_findings),
-                },
-                "triage": {"skip_review": False, "reason": "legacy_single_pass", "risk_level": "medium"},
-            }, ""
+        if not isinstance(triage_result, dict) or "skip_review" not in triage_result:
+            return False, {}, "Triage phase returned invalid schema"
 
         if triage_result.get("skip_review") is True:
             logger.info("Skipping review based on triage decision: %s", triage_result.get("reason", ""))

claudecode/test_main_function.py

@@ -12,6 +12,28 @@
 from claudecode.github_action_audit import main
 
 
+def _mock_multiphase_success(mock_runner, findings=None):
+    """Configure runner mock for strict multi-phase orchestration."""
+    findings = findings or []
+    mock_runner.run_prompt.side_effect = [
+        (True, "", {"skip_review": False, "reason": "", "risk_level": "medium"}),  # triage
+        (True, "", {"claude_md_files": [], "change_summary": "", "hotspots": [], "priority_files": []}),  # context
+        (True, "", {"findings": findings}),  # compliance
+        (True, "", {"findings": []}),  # quality
+        (True, "", {"findings": []}),  # security
+        (
+            True,
+            "",
+            {
+                "validated_findings": [
+                    {"finding_index": idx, "keep": True, "confidence": 0.95, "reason": "valid"}
+                    for idx in range(len(findings))
+                ]
+            },
+        ),  # validation
+    ]
+
+
 class TestMainFunction:
     """Test main function execution flow."""
 
@@ -232,20 +254,7 @@ def test_main_successful_audit_no_findings(self, mock_client_class, mock_runner_
 
         mock_runner = Mock()
         mock_runner.validate_claude_available.return_value = (True, "")
-        # Unified review - single call
-        mock_runner.run_code_review.return_value = (
-            True,
-            "",
-            {
-                'findings': [],
-                'analysis_summary': {
-                    'files_reviewed': 5,
-                    'high_severity': 0,
-                    'medium_severity': 0,
-                    'low_severity': 0
-                }
-            }
-        )
+        _mock_multiphase_success(mock_runner, findings=[])
         mock_runner_class.return_value = mock_runner
 
         mock_filter = Mock()
@@ -299,6 +308,7 @@ def test_main_successful_audit_with_findings(self, mock_client_class, mock_runne
         }
         mock_client.get_pr_diff.return_value = "diff content"
         mock_client._is_excluded.return_value = False  # Don't exclude any files in tests
+        mock_client.is_excluded.return_value = False
         mock_client_class.return_value = mock_client
 
         findings = [
@@ -320,20 +330,7 @@ def test_main_successful_audit_with_findings(self, mock_client_class, mock_runne
 
         mock_runner = Mock()
         mock_runner.validate_claude_available.return_value = (True, "")
-        # Unified review - single call with all findings
-        mock_runner.run_code_review.return_value = (
-            True,
-            "",
-            {
-                'findings': findings,
-                'analysis_summary': {
-                    'files_reviewed': 2,
-                    'high_severity': 1,
-                    'medium_severity': 1,
-                    'low_severity': 0
-                }
-            }
-        )
+        _mock_multiphase_success(mock_runner, findings=findings)
         mock_runner_class.return_value = mock_runner
 
         # Filter keeps only high severity
@@ -391,13 +388,14 @@ def test_main_with_full_filter(self, mock_client_class, mock_runner_class,
         }
         mock_client.get_pr_diff.return_value = "diff content"
         mock_client._is_excluded.return_value = False  # Don't exclude any files in tests
+        mock_client.is_excluded.return_value = False
         mock_client_class.return_value = mock_client
 
         findings = [{'file': 'test.py', 'line': 10, 'severity': 'HIGH', 'description': 'Issue'}]
 
         mock_runner = Mock()
         mock_runner.validate_claude_available.return_value = (True, "")
-        mock_runner.run_code_review.return_value = (True, "", {'findings': findings})
+        _mock_multiphase_success(mock_runner, findings=findings)
         mock_runner_class.return_value = mock_runner
 
         mock_prompt_func.return_value = "prompt"
@@ -450,6 +448,7 @@ def test_main_filter_failure_keeps_all_findings(self, mock_client_class, mock_ru
         mock_client.get_pr_data.return_value = {'number': 123, 'title': 'Test', 'body': ''}
         mock_client.get_pr_diff.return_value = "diff"
         mock_client._is_excluded.return_value = False  # Don't exclude any files in tests
+        mock_client.is_excluded.return_value = False
         mock_client_class.return_value = mock_client
 
         findings = [
@@ -459,7 +458,7 @@ def test_main_filter_failure_keeps_all_findings(self, mock_client_class, mock_ru
 
         mock_runner = Mock()
         mock_runner.validate_claude_available.return_value = (True, "")
-        mock_runner.run_code_review.return_value = (True, "", {'findings': findings})
+        _mock_multiphase_success(mock_runner, findings=findings)
         mock_runner_class.return_value = mock_runner
 
         mock_prompt_func.return_value = "prompt"
@@ -539,11 +538,7 @@ def test_audit_failure(self, mock_client_class, mock_runner_class,
 
         mock_runner = Mock()
         mock_runner.validate_claude_available.return_value = (True, "")
-        mock_runner.run_code_review.return_value = (
-            False,
-            "Claude execution failed",
-            {}
-        )
+        mock_runner.run_prompt.return_value = (False, "Claude execution failed", {})
         mock_runner_class.return_value = mock_runner
 
         mock_filter_class.return_value = Mock()
@@ -582,6 +577,7 @@ def test_findings_have_correct_review_type(self, mock_client_class, mock_runner_
         mock_client.get_pr_data.return_value = {'number': 123, 'title': 'Test', 'body': ''}
         mock_client.get_pr_diff.return_value = "diff"
         mock_client._is_excluded.return_value = False
+        mock_client.is_excluded.return_value = False
         mock_client_class.return_value = mock_client
 
         # Mixed findings - security and non-security
@@ -592,7 +588,7 @@ def test_findings_have_correct_review_type(self, mock_client_class, mock_runner_
 
         mock_runner = Mock()
         mock_runner.validate_claude_available.return_value = (True, "")
-        mock_runner.run_code_review.return_value = (True, "", {'findings': findings})
+        _mock_multiphase_success(mock_runner, findings=findings)
         mock_runner_class.return_value = mock_runner
 
         # Filter passes through all findings
@@ -644,6 +640,7 @@ def test_review_type_not_overwritten_if_already_set(self, mock_client_class, moc
         mock_client.get_pr_data.return_value = {'number': 123, 'title': 'Test', 'body': ''}
         mock_client.get_pr_diff.return_value = "diff"
         mock_client._is_excluded.return_value = False
+        mock_client.is_excluded.return_value = False
         mock_client_class.return_value = mock_client
 
         # Finding already has review_type set (e.g., from a custom analyzer)
@@ -654,7 +651,7 @@ def test_review_type_not_overwritten_if_already_set(self, mock_client_class, moc
 
         mock_runner = Mock()
         mock_runner.validate_claude_available.return_value = (True, "")
-        mock_runner.run_code_review.return_value = (True, "", {'findings': findings})
+        _mock_multiphase_success(mock_runner, findings=findings)
         mock_runner_class.return_value = mock_runner
 
         mock_filter = Mock()

claudecode/test_review_orchestrator.py

@@ -20,26 +20,16 @@ def test_review_model_config_from_env_with_fallbacks():
     assert cfg.validation == "global-model"
 
 
-def test_orchestrator_legacy_single_pass_path():
+def test_orchestrator_invalid_triage_schema_fails():
     runner = Mock()
-    runner.run_code_review.return_value = (
+    runner.run_prompt.return_value = (
         True,
         "",
-        {
-            "findings": [
-                {"file": "a.py", "line": 3, "severity": "HIGH", "category": "security", "title": "Issue"}
-            ],
-            "analysis_summary": {"files_reviewed": 1},
-        },
+        {"findings": [{"file": "a.py", "line": 3, "severity": "HIGH", "category": "security", "title": "Issue"}]},
     )
     findings_filter = Mock()
-    findings_filter.filter_findings.return_value = (
-        True,
-        {"filtered_findings": runner.run_code_review.return_value[2]["findings"]},
-        Mock(),
-    )
     github_client = Mock()
-    github_client._is_excluded.return_value = False
+    github_client.is_excluded.return_value = False
     cfg = ReviewModelConfig("t", "c", "q", "s", "v")
 
     orchestrator = ReviewOrchestrator(runner, findings_filter, github_client, cfg, 100)
@@ -49,7 +39,6 @@ def test_orchestrator_legacy_single_pass_path():
         pr_diff="diff --git a/a.py b/a.py",
     )
 
-    assert ok is True
-    assert err == ""
-    assert len(result["findings"]) == 1
-    assert result["findings"][0]["review_type"] == "security"
+    assert ok is False
+    assert result == {}
+    assert "invalid schema" in err.lower()