fix: validate review model responses

pr_agent/algo/utils.py

@@ -482,6 +482,11 @@ def convert_to_markdown_v2(output_data: dict,
                     try:
                         if not issue or not isinstance(issue, dict):
                             continue
+                        if any(
+                            field in issue and not isinstance(issue[field], str)
+                            for field in ('relevant_file', 'issue_header', 'issue_content')
+                        ):
+                            continue
                         relevant_file = issue.get('relevant_file', '').strip()
                         issue_header = issue.get('issue_header', '').strip()
                         if issue_header.lower() == 'possible bug':

pr_agent/tools/pr_reviewer.py

@@ -6,6 +6,7 @@
 from typing import List, Optional, Tuple
 
 from jinja2 import Environment, StrictUndefined
+from pydantic import ValidationError
 
 from pr_agent.algo.ai_handlers.base_ai_handler import BaseAiHandler
 from pr_agent.algo.ai_handlers.litellm_ai_handler import LiteLLMAIHandler
@@ -17,6 +18,7 @@
     key_issue_fingerprint,
     key_issue_location_fingerprint,
 )
+from pr_agent.algo.output_models import PRReview
 from pr_agent.algo.pr_processing import (
     PreparedPRDiff,
     add_ai_metadata_to_diff_files,
@@ -839,6 +841,7 @@ async def _prepare_chunked_prediction(self, model: str,
             if isinstance(prediction, BaseException):
                 raise prediction
             data = self._load_valid_review_yaml(prediction, source=f"review chunk {chunk_index + 1}")
+            self._validate_review_schema(data)
             raw_predictions.append(prediction)
             chunk_outputs.append(data)
 
@@ -887,7 +890,50 @@ def _load_review_yaml(prediction: str) -> dict:
                          keys_fix_yaml=["ticket_compliance_check", "estimated_effort_to_review_[1-5]:", "risk_level:",
                                         "merge_recommendation:", "security_concerns:", "key_issues_to_review:",
                                         "relevant_file:", "relevant_line:", "suggestion:"],
-                         first_key='review', last_key='security_concerns')
+                        first_key='review', last_key='security_concerns')
+
+    def _validate_review_schema(self, data: object) -> bool:
+        try:
+            PRReview.model_validate(data)
+        except ValidationError as error:
+            first_error = error.errors()[0]
+            field_path = ".".join(str(part) for part in first_error.get("loc", ())) or "$"
+            value = None if first_error.get("type") == "missing" else first_error.get("input")
+            get_logger().warning(
+                "Review output failed schema validation",
+                artifact={
+                    "field": field_path,
+                    "value": value,
+                },
+            )
+            return False
+
+        if isinstance(data, dict) and isinstance(data.get("review"), dict):
+            review = data["review"]
+            required_fields = (
+                ("ticket_compliance_check", "related_tickets"),
+                ("estimated_effort_to_review_[1-5]", "require_estimate_effort_to_review"),
+                ("risk_level", "require_risk_assessment"),
+                ("merge_recommendation", "require_merge_recommendation"),
+                ("review_priority_files", "require_priority_files"),
+                ("contribution_time_cost_estimate", "require_estimate_contribution_time_cost"),
+                ("score", "require_score"),
+                ("relevant_tests", "require_tests"),
+                ("insights_from_user_answers", "question_str"),
+                ("security_concerns", "require_security_review"),
+                ("todo_sections", "require_todo_scan"),
+                ("can_be_split", "require_can_be_split_review"),
+            )
+            vars_ = getattr(self, "vars", {})
+            for field_name, setting_name in required_fields:
+                if not vars_.get(setting_name) or field_name in review and review[field_name] is not None:
+                    continue
+                get_logger().warning(
+                    "Review output failed schema validation",
+                    artifact={"field": f"review.{field_name}", "value": None},
+                )
+                return False
+        return True
 
     @classmethod
     def _load_valid_review_yaml(cls, prediction: str, *, source: str = "model response") -> dict:
@@ -903,6 +949,8 @@ def _prepare_pr_review(self) -> str:
         the feedback.
         """
         data = self.prediction_data if self.prediction_data is not None else self._load_review_yaml(self.prediction)
+        if self.prediction_data is None:
+            self._validate_review_schema(data)
         github_action_output(data, 'review')
 
         if not isinstance(data, dict) or not isinstance(data.get('review'), dict) or not data['review']:

tests/unittest/test_convert_to_markdown.py

@@ -1,6 +1,6 @@
 # Generated by CodiumAI
 import textwrap
-from unittest.mock import Mock
+from unittest.mock import Mock, patch
 
 from pr_agent.algo.utils import PRReviewHeader, _expand_minute_suffix, convert_to_markdown_v2
 from pr_agent.tools.pr_description import insert_br_after_x_chars
@@ -123,6 +123,34 @@ def test_key_issues_to_review(self):
         assert convert_to_markdown_v2(input_data, git_provider=mock_git_provider).strip() == expected_output.strip()
         mock_git_provider.get_line_link.assert_called_with('src/utils.py', 30, 50)
 
+    def test_key_issue_with_non_string_text_is_skipped_without_render_error(self):
+        input_data = {'review': {'key_issues_to_review': [{
+            'relevant_file': 'src/utils.py',
+            'issue_header': 'Code Smell',
+            'issue_content': 123,
+            'start_line': 30,
+            'end_line': 50,
+        }]}}
+
+        with patch('pr_agent.algo.utils.get_logger') as get_logger:
+            output = convert_to_markdown_v2(input_data)
+
+        assert 'Recommended focus areas for review' in output
+        assert 'Code Smell' not in output
+        get_logger.return_value.exception.assert_not_called()
+
+    def test_key_issue_with_omitted_text_keeps_empty_fallback(self):
+        input_data = {'review': {'key_issues_to_review': [{
+            'relevant_file': 'src/utils.py',
+            'issue_content': 'The function needs review.',
+            'start_line': 30,
+            'end_line': 50,
+        }]}}
+
+        output = convert_to_markdown_v2(input_data)
+
+        assert '<strong></strong><br>The function needs review.' in output
+
     def test_ticket_compliance(self):
         input_data = {'review': {
             'ticket_compliance_check': [

tests/unittest/test_pr_reviewer_core.py

@@ -213,6 +213,71 @@ def test_prepare_pr_review_leaves_original_content_unchanged_without_remaining_f
     assert "Review coverage" not in review
 
 
+def test_prepare_pr_review_warns_on_invalid_model_output_without_changing_markdown():
+    reviewer = _make_prediction_reviewer()
+    reviewer.prediction = "review:\n  key_issues_to_review: wrong"
+    reviewer.git_provider.get_diff_files.return_value = []
+    reviewer.git_provider.is_supported.return_value = False
+    reviewer.set_review_labels = MagicMock()
+
+    with (
+        patch("pr_agent.tools.pr_reviewer.load_yaml", return_value={"review": {"key_issues_to_review": "wrong"}}),
+        patch("pr_agent.tools.pr_reviewer.github_action_output"),
+        patch("pr_agent.tools.pr_reviewer.convert_to_markdown_v2", return_value="original review"),
+        patch("pr_agent.tools.pr_reviewer.get_logger") as get_logger,
+    ):
+        review = reviewer._prepare_pr_review()
+
+    assert review == "original review"
+    get_logger.return_value.warning.assert_called_once()
+    warning = get_logger.return_value.warning.call_args.kwargs
+    assert warning["artifact"] == {"field": "review.key_issues_to_review", "value": "wrong"}
+
+
+def test_prepare_pr_review_does_not_warn_for_valid_model_output():
+    reviewer = _make_prediction_reviewer()
+    reviewer.prediction = "review:\n  key_issues_to_review: []"
+    reviewer.git_provider.get_diff_files.return_value = []
+    reviewer.git_provider.is_supported.return_value = False
+    reviewer.set_review_labels = MagicMock()
+
+    with (
+        patch("pr_agent.tools.pr_reviewer.load_yaml", return_value={"review": {"key_issues_to_review": []}}),
+        patch("pr_agent.tools.pr_reviewer.github_action_output"),
+        patch("pr_agent.tools.pr_reviewer.convert_to_markdown_v2", return_value="original review"),
+        patch("pr_agent.tools.pr_reviewer.get_logger") as get_logger,
+    ):
+        review = reviewer._prepare_pr_review()
+
+    assert review == "original review"
+    get_logger.return_value.warning.assert_not_called()
+
+
+def test_review_schema_requires_enabled_prompt_fields_only():
+    reviewer = _make_prediction_reviewer()
+    reviewer.vars = {"require_tests": True}
+    with patch("pr_agent.tools.pr_reviewer.get_logger") as get_logger:
+        assert reviewer._validate_review_schema({"review": {"key_issues_to_review": []}}) is False
+
+    warning = get_logger.return_value.warning.call_args.kwargs
+    assert warning["artifact"] == {"field": "review.relevant_tests", "value": None}
+
+    reviewer.vars = {"require_tests": False}
+    get_logger.return_value.warning.reset_mock()
+    assert reviewer._validate_review_schema({"review": {"key_issues_to_review": []}}) is True
+    get_logger.return_value.warning.assert_not_called()
+
+
+def test_review_schema_reports_none_for_missing_fields():
+    reviewer = _make_prediction_reviewer()
+    with patch("pr_agent.tools.pr_reviewer.get_logger") as get_logger:
+        assert reviewer._validate_review_schema({"review": {}}) is False
+
+    warning = get_logger.return_value.warning.call_args.kwargs
+    assert warning["artifact"]["field"] == "review.key_issues_to_review"
+    assert warning["artifact"]["value"] is None
+
+
 def test_prepare_pr_review_limits_coverage_footer_to_50_files():
     reviewer = _make_prediction_reviewer()
     remaining_files = [f"file_{index}.py" for index in range(51)]

tests/unittest/test_review_large_diff_chunking.py

@@ -19,7 +19,7 @@
 _TRACKED_KEYS = ("pr_reviewer.enable_large_pr_chunking", "pr_reviewer.max_number_of_calls")
 
 CHUNK_A = """review:
-  score: "90"
+  score: 90
   key_issues_to_review:
     - relevant_file: |
         a.py
@@ -34,7 +34,7 @@
 """
 
 CHUNK_B = """review:
-  score: "40"
+  score: 40
   key_issues_to_review: []
   security_concerns: |
     SQL injection: the query is built by string concatenation
@@ -116,7 +116,7 @@ async def test_a_truncated_diff_is_reviewed_chunk_by_chunk_and_merged(chunking_e
     assert [call.args[1] for call in reviewer._get_prediction.await_args_list] == ["chunk-a", "chunk-b"]
 
     review = reviewer.prediction_data["review"]
-    assert review["score"] == "40"  # the worst chunk sets the score
+    assert review["score"] == 40  # the worst chunk sets the score
     assert [issue["relevant_file"].strip() for issue in review["key_issues_to_review"]] == ["a.py"]
     assert review["security_concerns"].startswith("SQL injection:")
     assert reviewer.review_chunk_count == 2
@@ -194,7 +194,7 @@ async def test_a_chunk_that_fails_does_not_lose_the_chunks_that_succeeded(chunki
     ):
         await reviewer._prepare_prediction("model")
 
-    assert reviewer.prediction_data["review"]["score"] == "40"
+    assert reviewer.prediction_data["review"]["score"] == 40
     assert reviewer.review_chunk_count == 2
     assert reviewer.review_failed_chunk_count == 1
 
@@ -288,6 +288,29 @@ async def test_chunks_without_nonempty_reviews_fail_the_model_attempt(chunking_e
     assert reviewer.prediction_data is None
 
 
+@pytest.mark.asyncio
+async def test_invalid_chunk_emits_one_schema_warning_before_rendering(chunking_enabled):
+    reviewer = _make_reviewer()
+    reviewer._get_prediction = AsyncMock(side_effect=[
+        "review:\n  score: 101\n  key_issues_to_review: []",
+        CHUNK_B,
+    ])
+
+    with (
+        patch("pr_agent.tools.pr_reviewer.get_pr_diff", return_value=("diff", ["b.py"])),
+        patch("pr_agent.tools.pr_reviewer.get_pr_multi_diffs",
+              return_value=(["chunk-a", "chunk-b"], [])),
+        patch("pr_agent.tools.pr_reviewer.get_logger") as get_logger,
+    ):
+        await reviewer._prepare_prediction("model")
+        reviewer._prepare_pr_review()
+
+    warnings = get_logger.return_value.warning.call_args_list
+    schema_warnings = [call for call in warnings if call.args == ("Review output failed schema validation",)]
+    assert len(schema_warnings) == 1
+    assert schema_warnings[0].kwargs["artifact"] == {"field": "review.score", "value": 101}
+
+
 def _render_review(reviewer):
     reviewer.prediction = "review:\n  summary: test"
     reviewer.git_provider.get_diff_files.return_value = []