From 74f5df5df80a081b64d760ba460c018267663499 Mon Sep 17 00:00:00 2001
From: "langsmith-forge[bot]"
 <270983758+langsmith-forge[bot]@users.noreply.github.com>
Date: Tue, 12 May 2026 13:55:39 -0700
Subject: [PATCH] fix: publish_review tool returns generic "Failed to POST PR
 review" without GitHub API status/body, agent retries with no signal (#1299)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix(reviewer): surface HTTP status and body excerpt for non-dict GitHub PR review responses

When post_pull_request_review received a non-dict body, it returned
None and publish_review surfaced a generic 'Failed to POST PR review'
string with no signal for the agent to adapt — leading to blind
retries with permuted cap/severity_threshold args.

Now the non-dict-body path mirrors the existing HTTPStatusError /
HTTPError paths: it returns {'_error': 'HTTP <status>: non-dict
response body: <excerpt>'} so the user-facing tool can include the
underlying detail. The bare-None branch in publish_review.py is kept
as a defensive guard with a clearer message.

* ci: apply ruff format to reviewer_publish.py

Collapse the multi-line return dict into a single line so it matches the
output of `ruff format`, unblocking the Agent lint / format-check CI jobs.

Co-authored-by: Johannes du Plessis <51395795+johannes117@users.noreply.github.com>

---------

Co-authored-by: LangSmith Issues Agent <issues-agent@langsmith.dev>
Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
Co-authored-by: Johannes du Plessis <51395795+johannes117@users.noreply.github.com>
---
 agent/reviewer_publish.py      | 12 ++++++++++-
 agent/tools/publish_review.py  |  9 +++++++--
 tests/test_reviewer_publish.py | 37 ++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 3 deletions(-)

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()