From 8461979b0df217334e92c77498dc8dff86300632 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Wed, 10 Jun 2026 10:44:13 -0700 Subject: [PATCH] fix: honest publish_review reporting + structured thread-not-found errors (#1481) * fix: honest publish_review reporting + structured thread-not-found errors - Document skipped_empty_re_review and dry_run in the publish_review docstring and add a closing-summary contract to the reviewer prompt so the agent never claims a review was published when review_id is null. - Raise ReviewerThreadMissingError from replace_findings on SDK NotFoundError; add_finding/update_finding/publish_review return a structured do-not-retry result instead of raising, so the agent reports the blocker after one failure instead of retrying 10-30 times. * fix: translate thread 404s across all reviewer tool boundaries get_thread_metadata now raises ReviewerThreadMissingError instead of swallowing a missing thread as {} (which produced misleading 'No finding found' results), set_reviewer_thread_metadata translates the SDK 404 the same way, and every reviewer tool entrypoint (add/update/list findings, publish_review incl. eval dry-run, resolve/reply thread) returns the structured do-not-retry result. --------- Co-authored-by: open-swe[bot] --- agent/reviewer.py | 17 ++++++ agent/reviewer_findings.py | 53 ++++++++++++++++-- agent/tools/add_finding.py | 12 ++++- agent/tools/list_findings.py | 7 ++- agent/tools/publish_review.py | 36 ++++++++++--- agent/tools/reply_to_finding_thread.py | 23 ++++---- agent/tools/resolve_finding_thread.py | 25 +++++---- agent/tools/update_finding.py | 19 +++++-- tests/test_reviewer.py | 15 ++++++ tests/test_reviewer_findings.py | 75 ++++++++++++++++++++++++++ tests/test_reviewer_publish.py | 33 ++++++++++++ tests/test_reviewer_tools.py | 51 ++++++++++++++++++ 12 files changed, 331 insertions(+), 35 deletions(-) diff --git a/agent/reviewer.py b/agent/reviewer.py index eb61b1e9..e56dd589 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -254,6 +254,23 @@ severities — they're not findings. - Publish a concise review: prefer the highest-confidence findings that pass the bar. Use fewer when fewer issues are defensible; publish zero only after the workflow above found no concrete regression. + +# After publish_review — closing summary + +Inspect the returned `review_id`, `skipped_empty_re_review`, and `dry_run` +fields before composing your final message; `success: true` alone does NOT +mean a review was posted. + +- `review_id` is a number and neither flag is set → you may say the review + was published/posted and cite `surfaced_count`. +- `skipped_empty_re_review: true` or `review_id: null` → say "no new review + was posted" / "the re-review had nothing new to surface". Do NOT use + "published", "submitted", or "posted". +- `dry_run: true` → say "Simulated publish (eval mode) — review not posted + to GitHub", then list the findings inline. Do NOT claim publication. +- `error: "thread_not_found"` → findings storage is gone; do not retry the + tool. Report the blocker and include your intended findings inline in the + final message. """ diff --git a/agent/reviewer_findings.py b/agent/reviewer_findings.py index bea2a11c..d8cfa59b 100644 --- a/agent/reviewer_findings.py +++ b/agent/reviewer_findings.py @@ -19,9 +19,24 @@ from typing import Any, Literal, TypedDict, cast from langgraph.config import get_config from langgraph_sdk import get_client +from langgraph_sdk.errors import NotFoundError as LangGraphSDKNotFoundError logger = logging.getLogger(__name__) + +class ReviewerThreadMissingError(RuntimeError): + """The reviewer thread backing findings storage does not exist. + + Raised instead of the SDK's ``NotFoundError`` so tool wrappers can return a + structured do-not-retry result: the thread won't appear on retry (evicted, + eval-mode, or never created), and blind retries burn the whole run. + """ + + def __init__(self, thread_id: str, original: Exception) -> None: + super().__init__(f"Reviewer thread {thread_id!r} not found: {original}") + self.thread_id = thread_id + + REVIEWER_THREAD_KIND = "reviewer" # Suggestions are only useful when the reader can scan them at a glance and @@ -287,10 +302,18 @@ def get_thread_id_from_runtime() -> str: async def get_thread_metadata(thread_id: str) -> dict[str, Any]: - """Fetch the current metadata for a thread. Returns ``{}`` on miss.""" + """Fetch the current metadata for a thread. + + Raises :class:`ReviewerThreadMissingError` when the thread does not exist + (swallowing it as ``{}`` made tools report misleading results like "No + finding found" instead of the do-not-retry contract). Other transient + failures still degrade to ``{}``. + """ client = get_client() try: thread = await client.threads.get(thread_id) + except LangGraphSDKNotFoundError as exc: + raise ReviewerThreadMissingError(thread_id, exc) from exc except Exception: # noqa: BLE001 logger.exception("Failed to fetch thread metadata for %s", thread_id) return {} @@ -339,7 +362,28 @@ async def replace_findings(thread_id: str, findings: list[Finding]) -> None: lost-update window a blind overwrite here leaves open. """ client = get_client() - await client.threads.update(thread_id=thread_id, metadata={"findings": findings}) + try: + await client.threads.update(thread_id=thread_id, metadata={"findings": findings}) + except LangGraphSDKNotFoundError as exc: + raise ReviewerThreadMissingError(thread_id, exc) from exc + + +def thread_missing_tool_result(exc: ReviewerThreadMissingError) -> dict[str, Any]: + """Structured tool result for a missing reviewer thread. + + Returned (not raised) so the agent sees an explicit do-not-retry contract + instead of an empty error blob it retries against. + """ + return { + "success": False, + "error": "thread_not_found", + "thread_id": exc.thread_id, + "note": ( + "Reviewer findings storage is unavailable. Do not retry; report the " + "blocker and include intended findings inline in the final message." + ), + "detail": str(exc), + } async def mutate_findings( @@ -520,7 +564,10 @@ async def set_reviewer_thread_metadata( metadata["slack_thread"] = slack_thread if extra: metadata.update(extra) - await client.threads.update(thread_id=thread_id, metadata=metadata) + try: + await client.threads.update(thread_id=thread_id, metadata=metadata) + except LangGraphSDKNotFoundError as exc: + raise ReviewerThreadMissingError(thread_id, exc) from exc def get_thread_watch_flag(metadata: dict[str, Any]) -> bool: diff --git a/agent/tools/add_finding.py b/agent/tools/add_finding.py index 90a58d59..8a8fc63a 100644 --- a/agent/tools/add_finding.py +++ b/agent/tools/add_finding.py @@ -14,6 +14,7 @@ from ..reviewer_findings import ( Confidence, DiffSide, Finding, + ReviewerThreadMissingError, Severity, append_finding, clip_suggestion, @@ -21,6 +22,7 @@ from ..reviewer_findings import ( new_finding, normalize_finding_title, resolve_review_head_sha, + thread_missing_tool_result, ) @@ -128,7 +130,10 @@ def add_finding( clipped_suggestion, suggestion_dropped = clip_suggestion(suggestion) thread_id = get_thread_id_from_runtime() - head_sha = asyncio.run(resolve_review_head_sha(thread_id, configurable)) + try: + head_sha = asyncio.run(resolve_review_head_sha(thread_id, configurable)) + except ReviewerThreadMissingError as exc: + return thread_missing_tool_result(exc) finding: Finding = new_finding( severity=_cast_severity(severity), @@ -146,7 +151,10 @@ def add_finding( in_diff=in_diff, ) - asyncio.run(append_finding(thread_id, finding)) + try: + asyncio.run(append_finding(thread_id, finding)) + except ReviewerThreadMissingError as exc: + return thread_missing_tool_result(exc) result: dict[str, Any] = {"success": True, "finding_id": finding["id"]} if not in_diff: result["in_diff"] = False diff --git a/agent/tools/list_findings.py b/agent/tools/list_findings.py index eb15544a..4ce949f8 100644 --- a/agent/tools/list_findings.py +++ b/agent/tools/list_findings.py @@ -6,7 +6,9 @@ import asyncio from typing import Any from ..reviewer_findings import ( + ReviewerThreadMissingError, get_thread_id_from_runtime, + thread_missing_tool_result, ) from ..reviewer_findings import ( list_findings as list_findings_async, @@ -30,7 +32,10 @@ def list_findings(status_filter: str | None = None) -> dict[str, Any]: return {"findings": [], "count": 0, "error": f"Invalid status_filter: {status_filter}"} thread_id = get_thread_id_from_runtime() - findings = asyncio.run(list_findings_async(thread_id)) + try: + findings = asyncio.run(list_findings_async(thread_id)) + except ReviewerThreadMissingError as exc: + return thread_missing_tool_result(exc) if status_filter is not None: findings = [f for f in findings if f.get("status") == status_filter] return {"findings": findings, "count": len(findings)} diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index 0cc51c48..3db28f3d 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -11,6 +11,7 @@ from ..dashboard.team_settings import get_team_review_trace_links_enabled from ..reviewer_diff import compute_diff_line_set, fetch_pr_diff, is_range_in_diff from ..reviewer_findings import ( Finding, + ReviewerThreadMissingError, Severity, _coerce_surface, filter_findings_for_publish, @@ -21,6 +22,7 @@ from ..reviewer_findings import ( replace_findings, resolve_review_head_sha, set_reviewer_thread_metadata, + thread_missing_tool_result, ) from ..reviewer_findings import ( list_findings as list_findings_async, @@ -74,7 +76,22 @@ def publish_review( Returns: Dictionary with ``success``, ``review_id``, ``surfaced_count``, - ``hidden_count``, ``resolved_thread_count``. + ``hidden_count``, ``resolved_thread_count``, and sometimes + ``out_of_diff_count``, ``unresolvable_findings``, plus the flags below. + + ``success: true`` alone does NOT mean a GitHub Review was posted — + check the flags: + + - ``skipped_empty_re_review: true`` (with ``review_id: null``): an + empty re-review was deliberately skipped. No GitHub Review was + created; the call was a valid no-op. Do not describe the review as + published/posted/submitted. + - ``dry_run: true`` (with ``review_id: null``): eval/benchmark mode — + the publish was simulated and nothing was posted to GitHub. Do not + claim publication. + + Only a numeric ``review_id`` (with neither flag set) confirms a real + GitHub Review was created. """ if severity_threshold not in {"low", "medium", "high", "critical"}: return {"success": False, "error": f"Invalid severity_threshold: {severity_threshold}"} @@ -99,13 +116,16 @@ def publish_review( return {"success": False, "error": "Missing head_sha in run config"} if _is_reviewer_eval_mode(configurable): - return asyncio.run( - _publish_review_eval_dry_run_async( - head_sha=head_sha, - severity_threshold=_cast_severity(severity_threshold), - cap=cap, + try: + return asyncio.run( + _publish_review_eval_dry_run_async( + head_sha=head_sha, + severity_threshold=_cast_severity(severity_threshold), + cap=cap, + ) ) - ) + except ReviewerThreadMissingError as exc: + return thread_missing_tool_result(exc) token = get_github_token() if not token: @@ -126,6 +146,8 @@ def publish_review( trace_link_config_override=configurable.get("review_trace_link_enabled"), ) ) + except ReviewerThreadMissingError as exc: + return thread_missing_tool_result(exc) except GitHubAuthError as exc: thread_id = get_thread_id_from_runtime() if thread_id: diff --git a/agent/tools/reply_to_finding_thread.py b/agent/tools/reply_to_finding_thread.py index ba49d519..ebfe0fe7 100644 --- a/agent/tools/reply_to_finding_thread.py +++ b/agent/tools/reply_to_finding_thread.py @@ -7,9 +7,11 @@ from langgraph.config import get_config from ..reviewer_findings import ( FindingInteraction, + ReviewerThreadMissingError, append_finding_interaction, get_finding, get_thread_id_from_runtime, + thread_missing_tool_result, update_finding_fields, ) from ..reviewer_publish import reply_to_review_comment @@ -37,16 +39,19 @@ def reply_to_finding_thread(finding_id: str, body: str) -> dict[str, Any]: if not token: return {"success": False, "error": "No GitHub token available"} - return asyncio.run( - _reply_to_finding_thread_async( - finding_id=finding_id, - body=body, - owner=str(repo_config["owner"]), - repo=str(repo_config["name"]), - pr_number=pr_number, - token=token, + try: + return asyncio.run( + _reply_to_finding_thread_async( + finding_id=finding_id, + body=body, + owner=str(repo_config["owner"]), + repo=str(repo_config["name"]), + pr_number=pr_number, + token=token, + ) ) - ) + except ReviewerThreadMissingError as exc: + return thread_missing_tool_result(exc) async def _reply_to_finding_thread_async( diff --git a/agent/tools/resolve_finding_thread.py b/agent/tools/resolve_finding_thread.py index 4ad4215c..49bf597a 100644 --- a/agent/tools/resolve_finding_thread.py +++ b/agent/tools/resolve_finding_thread.py @@ -7,8 +7,10 @@ from langgraph.config import get_config from ..reviewer_findings import ( Finding, + ReviewerThreadMissingError, get_finding, get_thread_id_from_runtime, + thread_missing_tool_result, update_finding_fields, update_finding_surface, ) @@ -67,17 +69,20 @@ def resolve_finding_thread( if not token: return {"success": False, "error": "No GitHub token available"} - result = asyncio.run( - _resolve_finding_thread_async( - finding_id=finding_id, - status=status, - note=normalized_note, - owner=str(repo_config["owner"]), - repo=str(repo_config["name"]), - pr_number=pr_number, - token=token, + try: + result = asyncio.run( + _resolve_finding_thread_async( + finding_id=finding_id, + status=status, + note=normalized_note, + owner=str(repo_config["owner"]), + repo=str(repo_config["name"]), + pr_number=pr_number, + token=token, + ) ) - ) + except ReviewerThreadMissingError as exc: + return thread_missing_tool_result(exc) if result.get("success") and isinstance(result.get("finding"), dict): thread_id = configurable.get("thread_id") if isinstance(configurable, dict) else None emit_finding_status_outcome( diff --git a/agent/tools/update_finding.py b/agent/tools/update_finding.py index 1d66a75f..491eba35 100644 --- a/agent/tools/update_finding.py +++ b/agent/tools/update_finding.py @@ -11,11 +11,13 @@ from ..reviewer_findings import ( DEFAULT_FINDING_TITLE, MAX_SUGGESTION_LINES, Finding, + ReviewerThreadMissingError, clip_suggestion, get_thread_id_from_runtime, list_findings, normalize_finding_title, resolve_review_head_sha, + thread_missing_tool_result, update_finding_fields, ) from ..utils.reviewer_outcomes import emit_finding_status_outcome @@ -129,7 +131,12 @@ def update_finding( config = get_config() configurable = config.get("configurable", {}) if isinstance(config, dict) else {} if status == "open": - head_sha = asyncio.run(resolve_review_head_sha(get_thread_id_from_runtime(), configurable)) + try: + head_sha = asyncio.run( + resolve_review_head_sha(get_thread_id_from_runtime(), configurable) + ) + except ReviewerThreadMissingError as exc: + return thread_missing_tool_result(exc) if head_sha: updates["last_confirmed_sha"] = head_sha @@ -148,7 +155,10 @@ def update_finding( return {"success": False, "error": "No fields provided to update"} thread_id = get_thread_id_from_runtime() - findings = asyncio.run(list_findings(thread_id)) + try: + findings = asyncio.run(list_findings(thread_id)) + except ReviewerThreadMissingError as exc: + return thread_missing_tool_result(exc) finding = next((item for item in findings if item.get("id") == finding_id), None) if finding is None: return {"success": False, "error": f"No finding found with id {finding_id}"} @@ -195,7 +205,10 @@ def update_finding( ) return result - updated = asyncio.run(update_finding_fields(thread_id, finding_id, updates)) + try: + updated = asyncio.run(update_finding_fields(thread_id, finding_id, updates)) + except ReviewerThreadMissingError as exc: + return thread_missing_tool_result(exc) if updated is None: return {"success": False, "error": f"No finding found with id {finding_id}"} if status in {"resolved", "dismissed"} and not delegated_resolution: diff --git a/tests/test_reviewer.py b/tests/test_reviewer.py index e4a11082..c423c69f 100644 --- a/tests/test_reviewer.py +++ b/tests/test_reviewer.py @@ -1273,3 +1273,18 @@ async def test_reviewer_injects_pr_title_and_body_into_context() -> None: assert "PR title and description" in captured["system_prompt"] assert "Add retry logic for uploads" in captured["system_prompt"] assert "Retries flaky uploads up to 3 times." in captured["system_prompt"] + + +def test_reviewer_system_prompt_includes_closing_summary_contract() -> None: + """The prompt must tell the agent how to report publish_review outcomes: + dry_run / skipped_empty_re_review / thread_not_found are not publications.""" + prompt = reviewer._reviewer_system_prompt( + "/tmp/wd", + repo_owner="o", + repo_name="r", + pr_number=1, + ) + assert "skipped_empty_re_review" in prompt + assert "dry_run" in prompt + assert "Simulated publish (eval mode)" in prompt + assert "thread_not_found" in prompt diff --git a/tests/test_reviewer_findings.py b/tests/test_reviewer_findings.py index 773b39ec..6ca02908 100644 --- a/tests/test_reviewer_findings.py +++ b/tests/test_reviewer_findings.py @@ -257,3 +257,78 @@ async def test_resolve_review_head_sha_falls_back_without_thread_id() -> None: head = await resolve_review_head_sha("", {"head_sha": "confighead"}) assert head == "confighead" fake_client.threads.get.assert_not_called() + + +@pytest.mark.asyncio +async def test_replace_findings_raises_domain_error_when_thread_missing() -> None: + import httpx + from langgraph_sdk.errors import NotFoundError + + from agent.reviewer_findings import ReviewerThreadMissingError + + not_found = NotFoundError( + "thread tid not found", + response=httpx.Response(404, request=httpx.Request("PATCH", "http://x")), + body=None, + ) + fake_client = AsyncMock() + fake_client.threads.update.side_effect = not_found + + with patch("agent.reviewer_findings.get_client", return_value=fake_client): + with pytest.raises(ReviewerThreadMissingError) as excinfo: + await replace_findings("tid", [_f(id="f_a")]) + + assert excinfo.value.thread_id == "tid" + assert "not found" in str(excinfo.value) + + +def _not_found(method: str = "GET") -> Exception: + import httpx + from langgraph_sdk.errors import NotFoundError + + return NotFoundError( + "thread tid not found", + response=httpx.Response(404, request=httpx.Request(method, "http://x")), + body=None, + ) + + +@pytest.mark.asyncio +async def test_get_thread_metadata_raises_domain_error_when_thread_missing() -> None: + """A missing thread must surface as ReviewerThreadMissingError, not be + swallowed into ``{}`` — that produced misleading tool results like + "No finding found" instead of the do-not-retry contract.""" + from agent.reviewer_findings import ReviewerThreadMissingError, get_thread_metadata + + fake_client = AsyncMock() + fake_client.threads.get.side_effect = _not_found() + + with patch("agent.reviewer_findings.get_client", return_value=fake_client): + with pytest.raises(ReviewerThreadMissingError): + await get_thread_metadata("tid") + + +@pytest.mark.asyncio +async def test_get_thread_metadata_still_degrades_on_other_failures() -> None: + from agent.reviewer_findings import get_thread_metadata + + fake_client = AsyncMock() + fake_client.threads.get.side_effect = RuntimeError("transient") + + with patch("agent.reviewer_findings.get_client", return_value=fake_client): + assert await get_thread_metadata("tid") == {} + + +@pytest.mark.asyncio +async def test_set_reviewer_thread_metadata_raises_domain_error_when_thread_missing() -> None: + from agent.reviewer_findings import ( + ReviewerThreadMissingError, + set_reviewer_thread_metadata, + ) + + fake_client = AsyncMock() + fake_client.threads.update.side_effect = _not_found("PATCH") + + with patch("agent.reviewer_findings.get_client", return_value=fake_client): + with pytest.raises(ReviewerThreadMissingError): + await set_reviewer_thread_metadata("tid", last_reviewed_sha="sha") diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index 5a56d359..2a227ba1 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -2108,3 +2108,36 @@ async def test_publish_review_fetches_pr_diff_when_diff_line_set_missing() -> No assert {c["path"] for c in retry_inline} == {"in_diff.py"} assert result["success"] is True assert result["unresolvable_findings"] == ["f_bad"] + + +def test_publish_review_tool_returns_structured_error_when_thread_missing() -> None: + """A missing reviewer thread surfaces as a do-not-retry tool result instead + of an exception the middleware swallows into an empty tool message.""" + from agent.reviewer_findings import ReviewerThreadMissingError + from agent.tools.publish_review import publish_review + + publish_async = AsyncMock( + side_effect=ReviewerThreadMissingError("tid", RuntimeError("thread tid not found")) + ) + with ( + patch( + "agent.tools.publish_review.get_config", + return_value={ + "configurable": { + "thread_id": "tid", + "repo": {"owner": "o", "name": "r"}, + "pr_number": 7, + "head_sha": "sha", + }, + "metadata": {}, + }, + ), + patch("agent.tools.publish_review.get_github_token", return_value="token"), + patch("agent.tools.publish_review._publish_review_async", publish_async), + ): + result = publish_review() + + assert result["success"] is False + assert result["error"] == "thread_not_found" + assert result["thread_id"] == "tid" + assert "Do not retry" in result["note"] diff --git a/tests/test_reviewer_tools.py b/tests/test_reviewer_tools.py index 558f4e79..6f173bee 100644 --- a/tests/test_reviewer_tools.py +++ b/tests/test_reviewer_tools.py @@ -712,3 +712,54 @@ def test_list_findings_returns_all_when_filter_omitted() -> None: result = list_findings() assert result["count"] == 2 + + +def test_add_finding_returns_structured_error_when_thread_missing() -> None: + """A missing reviewer thread must come back as a do-not-retry tool result, + not a raised exception the agent retries against 10-30 times.""" + from agent.reviewer_findings import ReviewerThreadMissingError + + async def fake_append(thread_id: str, finding: Any) -> Any: + raise ReviewerThreadMissingError(thread_id, RuntimeError("thread X not found")) + + with ( + patch("agent.tools.add_finding.get_config", return_value=_config()), + patch("agent.tools.add_finding.get_thread_id_from_runtime", return_value="tid-1"), + patch("agent.tools.add_finding.append_finding", side_effect=fake_append), + ): + result = add_finding( + severity="medium", + confidence="high", + category="correctness", + file="foo.py", + title="Rename breaks reference", + description="rename", + start_line=11, + ) + + assert result["success"] is False + assert result["error"] == "thread_not_found" + assert result["thread_id"] == "tid-1" + assert "Do not retry" in result["note"] + + +def test_update_finding_returns_structured_error_when_thread_missing() -> None: + from agent.reviewer_findings import ReviewerThreadMissingError + + async def fake_update(thread_id: str, finding_id: str, updates: Any) -> Any: + raise ReviewerThreadMissingError(thread_id, RuntimeError("thread X not found")) + + with ( + patch("agent.tools.update_finding.get_config", return_value=_config()), + patch("agent.tools.update_finding.get_thread_id_from_runtime", return_value="tid-1"), + patch( + "agent.tools.update_finding.list_findings", + AsyncMock(return_value=[_existing_finding()]), + ), + patch("agent.tools.update_finding.update_finding_fields", side_effect=fake_update), + ): + result = update_finding(finding_id="f_a", status="resolved", note="fixed") + + assert result["success"] is False + assert result["error"] == "thread_not_found" + assert result["thread_id"] == "tid-1"