diff --git a/agent/reviewer_publish.py b/agent/reviewer_publish.py index 9d419a77..b980d99b 100644 --- a/agent/reviewer_publish.py +++ b/agent/reviewer_publish.py @@ -144,7 +144,17 @@ async def post_pull_request_review( logger.exception("Failed to POST PR review for %s/%s#%s", owner, repo, pr_number) return {"_error": f"{type(e).__name__}: {e}"} data = response.json() - return data if isinstance(data, dict) else None + if isinstance(data, dict): + return data + body_excerpt = (response.text or "")[:500] + logger.error( + "POST PR review for %s/%s#%s returned non-dict body: %s", + owner, + repo, + pr_number, + body_excerpt, + ) + return {"_error": (f"HTTP {response.status_code}: non-dict response body: {body_excerpt}")} async def fetch_review_comments( diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index 5cc5979b..9ea87620 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -174,13 +174,18 @@ async def _publish_review_async( inline_comments=inline_comments, token=token, ) - if review_response is None: - return {"success": False, "error": "Failed to POST PR review"} if isinstance(review_response, dict) and "_error" in review_response: return { "success": False, "error": f"Failed to POST PR review: {review_response['_error']}", } + if review_response is None: + # Defensive guard: with the upstream change this should never happen, + # but keep a clear signal if it does so the agent doesn't retry blindly. + return { + "success": False, + "error": "Failed to POST PR review: no response from GitHub", + } review_id = review_response.get("id") if isinstance(review_response, dict) else None if review_id is not None and inline_comments: diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index 749073b9..360132bb 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -9,6 +9,7 @@ import pytest from agent.reviewer_findings import Finding, new_finding from agent.reviewer_publish import ( + post_pull_request_review, render_inline_comment_body, render_inline_comment_payload, render_review_body, @@ -102,6 +103,42 @@ async def test_resolve_review_thread_returns_true_on_success() -> None: assert ok is True +@pytest.mark.asyncio +async def test_post_pull_request_review_non_dict_body_surfaces_status_and_excerpt() -> None: + """A non-dict GitHub response body must surface status code + body excerpt + via ``_error`` rather than collapsing to a bare ``None`` (which the + user-facing tool would render as the unhelpful ``Failed to POST PR review``).""" + response = MagicMock() + response.status_code = 200 + response.json.return_value = ["unexpected", "list", "body"] + response.text = '["unexpected", "list", "body"]' + response.raise_for_status.return_value = None + + client_cm = AsyncMock() + client_cm.__aenter__.return_value = client_cm + client_cm.post = AsyncMock(return_value=response) + + with patch("agent.reviewer_publish.httpx.AsyncClient", return_value=client_cm): + result = await post_pull_request_review( + owner="o", + repo="r", + pr_number=1, + head_sha="sha", + body="b", + inline_comments=[], + token="t", + ) + + assert isinstance(result, dict) + assert "_error" in result + err = result["_error"] + assert "HTTP 200" in err + assert "non-dict" in err + assert "unexpected" in err + # The bare legacy string must not be the only signal anymore. + assert err != "Failed to POST PR review" + + @pytest.mark.asyncio async def test_resolve_review_thread_returns_false_on_graphql_errors() -> None: response = MagicMock()