fix(review): avoid publishing empty review output

pr_agent/tools/pr_reviewer.py

@@ -233,6 +233,9 @@ async def run(self) -> None:
             pr_review = self._prepare_pr_review()
             get_logger().debug("PR output", artifact=pr_review)
 
+            if not pr_review:
+                raise ValueError("Failed to prepare review output")
+
             state_result = getattr(self, "_review_state_result", None)
             state_changed = bool(state_result and state_result.changed)
             state_blocked = getattr(self, "_review_state_blocked", False)
@@ -699,7 +702,7 @@ async def _prepare_chunked_prediction(self, model: str) -> bool:
             if isinstance(prediction, BaseException):
                 raise prediction
             data = self._load_review_yaml(prediction)
-            if not isinstance(data, dict) or not isinstance(data.get("review"), dict):
+            if not isinstance(data, dict) or not isinstance(data.get("review"), dict) or not data["review"]:
                 get_logger().warning(f"Failed to parse the review of chunk {chunk_index + 1}",
                                      artifact={"data": data})
                 continue
@@ -764,7 +767,7 @@ def _prepare_pr_review(self) -> str:
         data = self.prediction_data if self.prediction_data is not None else self._load_review_yaml(self.prediction)
         github_action_output(data, 'review')
 
-        if not isinstance(data.get('review'), dict):
+        if not isinstance(data, dict) or not isinstance(data.get('review'), dict) or not data['review']:
             if self._review_finding_state_enabled():
                 self._review_state_blocked = True
                 self._review_state_block_reason = _STATE_BLOCK_REVIEW_DATA

tests/unittest/test_pr_reviewer_core.py

@@ -83,14 +83,14 @@ async def test_prepare_prediction_keeps_incremental_review_compatible_with_tuple
 
 
 def _render_review(reviewer, remaining_files, supports_gfm_markdown=False):
-    reviewer.prediction = "review: {}"
+    reviewer.prediction = "review:\n  summary: test"
     reviewer.remaining_files_list = remaining_files
     reviewer.git_provider.get_diff_files.return_value = []
     reviewer.git_provider.is_supported.return_value = supports_gfm_markdown
     reviewer.set_review_labels = MagicMock()
 
     with (
-        patch("pr_agent.tools.pr_reviewer.load_yaml", return_value={"review": {}}),
+        patch("pr_agent.tools.pr_reviewer.load_yaml", return_value={"review": {"summary": "test"}}),
         patch("pr_agent.tools.pr_reviewer.github_action_output"),
         patch("pr_agent.tools.pr_reviewer.convert_to_markdown_v2", return_value="original review"),
     ):
@@ -617,6 +617,82 @@ async def test_run_removes_its_progress_comment_when_review_generation_fails(
     git_provider.remove_initial_comment.assert_not_called()
 
 
+@pytest.mark.asyncio
+@pytest.mark.parametrize(
+    ("prediction", "expected_action_data"),
+    [
+        ("::: not : valid : yaml :::\n\t- [", {}),
+        ("review: {}", {"review": {}}),
+        ("review: [invalid]", {"review": ["invalid"]}),
+    ],
+    ids=["unparseable", "empty-review", "invalid-review-shape"],
+)
+@pytest.mark.parametrize("persistent_comment", [False, True])
+@pytest.mark.parametrize("propagate_tool_errors", [False, True])
+async def test_run_does_not_publish_an_empty_review(
+    monkeypatch,
+    prediction,
+    expected_action_data,
+    persistent_comment,
+    propagate_tool_errors,
+):
+    from pr_agent.tools import pr_reviewer as pr_reviewer_module
+
+    progress_comment = MagicMock()
+    git_provider = MagicMock()
+    git_provider.get_files.return_value = ["app.py"]
+    git_provider.publish_comment.return_value = progress_comment
+    reviewer = _make_reviewer(git_provider)
+    reviewer.incremental = SimpleNamespace(is_incremental=False)
+    reviewer.vars = {}
+    reviewer.prediction = None
+    reviewer.prediction_data = None
+
+    async def fake_retry(prepare_fn, model_type=None):
+        reviewer.prediction = prediction
+
+    monkeypatch.setattr(pr_reviewer_module, "extract_and_cache_pr_tickets", AsyncMock())
+    monkeypatch.setattr(pr_reviewer_module, "retry_with_fallback_models", fake_retry)
+    action_output = MagicMock()
+    push_output = MagicMock()
+    monkeypatch.setattr(pr_reviewer_module, "github_action_output", action_output)
+    monkeypatch.setattr(pr_reviewer_module, "push_outputs", push_output)
+
+    settings = get_settings()
+    original = {
+        "publish_output": settings.config.publish_output,
+        "is_auto_command": settings.config.get("is_auto_command", False),
+        "propagate_tool_errors": settings.config.get("propagate_tool_errors", False),
+        "persistent_comment": settings.pr_reviewer.persistent_comment,
+    }
+    try:
+        settings.config.publish_output = True
+        settings.config.is_auto_command = False
+        settings.config.propagate_tool_errors = propagate_tool_errors
+        settings.pr_reviewer.persistent_comment = persistent_comment
+
+        if propagate_tool_errors:
+            with pytest.raises(ValueError, match="Failed to prepare review output"):
+                await reviewer.run()
+        else:
+            await reviewer.run()
+    finally:
+        settings.config.publish_output = original["publish_output"]
+        settings.config.is_auto_command = original["is_auto_command"]
+        settings.config.propagate_tool_errors = original["propagate_tool_errors"]
+        settings.pr_reviewer.persistent_comment = original["persistent_comment"]
+
+    assert git_provider.publish_comment.call_args_list == [
+        (("Preparing review...",), {"is_temporary": True}),
+        (("Failed to review PR",), {}),
+    ]
+    git_provider.publish_persistent_comment.assert_not_called()
+    git_provider.publish_structured_review.assert_not_called()
+    action_output.assert_called_once_with(expected_action_data, "review")
+    push_output.assert_not_called()
+    git_provider.remove_comment.assert_called_once_with(progress_comment)
+
+
 @pytest.mark.asyncio
 async def test_run_publishes_failure_result_when_progress_comment_has_no_handle(monkeypatch):
     from pr_agent.tools import pr_reviewer as pr_reviewer_module

tests/unittest/test_review_large_diff_chunking.py

@@ -174,6 +174,24 @@ async def test_a_chunk_that_fails_does_not_lose_the_chunks_that_succeeded(chunki
     assert reviewer.review_failed_chunk_count == 1
 
 
+@pytest.mark.asyncio
+async def test_an_empty_chunk_does_not_lose_a_valid_sibling_or_trigger_fallback(chunking_enabled):
+    reviewer = _make_reviewer()
+    reviewer._get_prediction = AsyncMock(side_effect=["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"], [])),
+    ):
+        await reviewer._prepare_prediction("model")
+
+    assert reviewer._get_prediction.await_count == 2
+    assert reviewer.prediction_data["review"]["score"] == "40"
+    assert reviewer.review_chunk_count == 2
+    assert reviewer.review_failed_chunk_count == 1
+
+
 @pytest.mark.asyncio
 async def test_a_review_where_every_chunk_failed_raises_so_a_fallback_model_is_tried(chunking_enabled):
     reviewer = _make_reviewer()
@@ -190,9 +208,14 @@ async def test_a_review_where_every_chunk_failed_raises_so_a_fallback_model_is_t
 
 
 @pytest.mark.asyncio
-async def test_chunks_that_answer_nothing_parsable_fall_back_to_a_single_call_review(chunking_enabled):
+@pytest.mark.parametrize("chunk_predictions", [
+    ["not yaml at all", "nor is this"],
+    ["review: {}", "review: {}"],
+])
+async def test_chunks_without_nonempty_reviews_fall_back_to_a_single_call_review(chunking_enabled,
+                                                                                 chunk_predictions):
     reviewer = _make_reviewer()
-    reviewer._get_prediction = AsyncMock(side_effect=["not yaml at all", "nor is this", CHUNK_A])
+    reviewer._get_prediction = AsyncMock(side_effect=[*chunk_predictions, CHUNK_A])
 
     with (
         patch("pr_agent.tools.pr_reviewer.get_pr_diff", return_value=("diff", ["b.py"])),
@@ -207,13 +230,13 @@ async def test_chunks_that_answer_nothing_parsable_fall_back_to_a_single_call_re
 
 
 def _render_review(reviewer):
-    reviewer.prediction = "review: {}"
+    reviewer.prediction = "review:\n  summary: test"
     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": {}}),
+        patch("pr_agent.tools.pr_reviewer.load_yaml", return_value={"review": {"summary": "test"}}),
         patch("pr_agent.tools.pr_reviewer.github_action_output"),
         patch("pr_agent.tools.pr_reviewer.convert_to_markdown_v2", return_value="original review"),
     ):