diff --git a/pr_agent/tools/pr_reviewer.py b/pr_agent/tools/pr_reviewer.py index 2ed1f8b413..bac288dc91 100644 --- a/pr_agent/tools/pr_reviewer.py +++ b/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 diff --git a/tests/unittest/test_pr_reviewer_core.py b/tests/unittest/test_pr_reviewer_core.py index af4aa5d77d..3222310966 100644 --- a/tests/unittest/test_pr_reviewer_core.py +++ b/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 diff --git a/tests/unittest/test_review_large_diff_chunking.py b/tests/unittest/test_review_large_diff_chunking.py index dbcf56e655..87b36d020a 100644 --- a/tests/unittest/test_review_large_diff_chunking.py +++ b/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"), ):