Address remaining PR #21 review feedback

Author: matej

claudecode/github_action_audit.py

@@ -234,6 +234,10 @@ def _is_excluded(self, filepath: str) -> bool:
                 return True
 
         return False
+
+    def is_excluded(self, filepath: str) -> bool:
+        """Public wrapper for exclusion checks."""
+        return self._is_excluded(filepath)
 
     def _filter_generated_files(self, diff_text: str) -> str:
         """Filter out generated files and excluded directories from diff content."""
@@ -329,7 +333,7 @@ def run_prompt(self, repo_dir: Path, prompt: str, model: Optional[str] = None) -
                     if (isinstance(parsed_result, dict) and 
                         parsed_result.get('type') == 'result' and 
                         parsed_result.get('subtype') == 'error_during_execution' and
-                        attempt == 0):
+                        attempt < NUM_RETRIES - 1):
                         continue
 
                     if isinstance(parsed_result, dict) and 'result' in parsed_result and isinstance(parsed_result['result'], str):

claudecode/prompts.py

@@ -19,6 +19,11 @@ def _build_hybrid_diff_section(pr_diff: str, max_lines: int) -> str:
     """Build a bounded inline diff section while requiring tool-based context reads."""
     if not pr_diff:
         return "\nNo inline diff available. Use repository tools to inspect changed files.\n"
+    if max_lines == 0:
+        return (
+            "\nInline diff intentionally omitted (max-diff-lines=0). "
+            "Use repository tools to inspect changed files and context.\n"
+        )
 
     lines = pr_diff.splitlines()
     if max_lines > 0 and len(lines) > max_lines:

claudecode/review_orchestrator.py

@@ -64,9 +64,10 @@ def __init__(
 
     def _run_phase(self, repo_dir: Path, prompt: str, model: str, phase_name: str) -> Tuple[bool, Dict[str, Any], str]:
         raw_result = None
-        if hasattr(self.claude_runner, "run_prompt"):
+        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)
-        if not (isinstance(raw_result, tuple) and len(raw_result) == 3) and hasattr(self.claude_runner, "run_code_review"):
+        elif hasattr(self.claude_runner, "run_code_review"):
             try:
                 raw_result = self.claude_runner.run_code_review(repo_dir, prompt, model=model)
             except TypeError:
@@ -87,6 +88,13 @@ def _run_phase(self, repo_dir: Path, prompt: str, model: str, phase_name: str) -
             return False, {}, f"Failed to parse {phase_name} output"
         return True, parsed, ""
 
+    def _is_excluded(self, filepath: str) -> bool:
+        checker = getattr(self.github_client, "is_excluded", None)
+        if callable(checker):
+            return bool(checker(filepath))
+        # Fallback for current client/tests that still expose private method.
+        return bool(self.github_client._is_excluded(filepath))
+
     def _collect_phase_findings(self, phase_result: Dict[str, Any], source_agent: str) -> List[Dict[str, Any]]:
         findings: List[Dict[str, Any]] = []
         for finding in phase_result.get("findings", []):
@@ -125,7 +133,7 @@ def run(
             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.github_client._is_excluded(finding.get("file", "")):
+                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"),
@@ -255,7 +263,7 @@ def run(
             "description": pr_data.get("body", ""),
         }
         kept_findings = validated
-        original_count = len(validated)
+        original_count = len(all_candidates)
         filter_response = self.findings_filter.filter_findings(validated, pr_context)
         if isinstance(filter_response, tuple) and len(filter_response) == 3:
             filter_success, filter_results, _ = filter_response
@@ -266,7 +274,7 @@ def run(
         for finding in kept_findings:
             if not isinstance(finding, dict):
                 continue
-            if self.github_client._is_excluded(finding.get("file", "")):
+            if self._is_excluded(finding.get("file", "")):
                 continue
             final_findings.append(self._ensure_review_type(finding))
 

claudecode/test_findings_merge.py

@@ -0,0 +1,32 @@
+"""Unit tests for findings_merge utilities."""
+
+from claudecode.findings_merge import merge_findings
+
+
+def test_merge_findings_empty_input():
+    assert merge_findings([]) == []
+
+
+def test_merge_findings_keeps_higher_severity_then_confidence():
+    findings = [
+        {"file": "a.py", "line": 10, "category": "security", "title": "Issue", "severity": "MEDIUM", "confidence": 0.9},
+        {"file": "a.py", "line": 10, "category": "security", "title": "Issue", "severity": "HIGH", "confidence": 0.8},
+        {"file": "a.py", "line": 10, "category": "security", "title": "Issue", "severity": "HIGH", "confidence": 0.95},
+    ]
+
+    merged = merge_findings(findings)
+    assert len(merged) == 1
+    assert merged[0]["severity"] == "HIGH"
+    assert merged[0]["confidence"] == 0.95
+
+
+def test_merge_findings_separate_keys_are_preserved():
+    findings = [
+        {"file": "a.py", "line": 1, "category": "correctness", "title": "One", "severity": "LOW"},
+        {"file": "a.py", "line": 2, "category": "correctness", "title": "Two", "severity": "LOW"},
+        {"file": "b.py", "line": 1, "category": "security", "title": "One", "severity": "MEDIUM"},
+    ]
+
+    merged = merge_findings(findings)
+    assert len(merged) == 3
+

claudecode/test_prompts.py

@@ -192,4 +192,12 @@ def test_get_unified_review_prompt_includes_summary_guidelines(self):
 
         assert "PR SUMMARY GUIDELINES:" in prompt
         assert "2-4 sentences" in prompt
-        assert "~10 words" in prompt
+        assert "~10 words" in prompt
+
+    def test_build_hybrid_diff_section_max_lines_zero_omits_inline_diff(self):
+        from claudecode.prompts import _build_hybrid_diff_section
+
+        section = _build_hybrid_diff_section("diff --git a/a.py b/a.py\n+print('x')", 0)
+
+        assert "intentionally omitted" in section
+        assert "```diff" not in section

claudecode/test_review_orchestrator.py

@@ -0,0 +1,55 @@
+"""Unit tests for review_orchestrator."""
+
+from pathlib import Path
+from unittest.mock import Mock
+
+from claudecode.review_orchestrator import ReviewModelConfig, ReviewOrchestrator
+
+
+def test_review_model_config_from_env_with_fallbacks():
+    env = {
+        "CLAUDE_MODEL": "global-model",
+        "MODEL_TRIAGE": "triage-model",
+        "MODEL_SECURITY": "security-model",
+    }
+    cfg = ReviewModelConfig.from_env(env, "default-model")
+    assert cfg.triage == "triage-model"
+    assert cfg.compliance == "global-model"
+    assert cfg.quality == "global-model"
+    assert cfg.security == "security-model"
+    assert cfg.validation == "global-model"
+
+
+def test_orchestrator_legacy_single_pass_path():
+    runner = Mock()
+    runner.run_code_review.return_value = (
+        True,
+        "",
+        {
+            "findings": [
+                {"file": "a.py", "line": 3, "severity": "HIGH", "category": "security", "title": "Issue"}
+            ],
+            "analysis_summary": {"files_reviewed": 1},
+        },
+    )
+    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
+    cfg = ReviewModelConfig("t", "c", "q", "s", "v")
+
+    orchestrator = ReviewOrchestrator(runner, findings_filter, github_client, cfg, 100)
+    ok, result, err = orchestrator.run(
+        repo_dir=Path("."),
+        pr_data={"number": 1, "changed_files": 1, "head": {"repo": {"full_name": "x/y"}}},
+        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"