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] <open-swe@users.noreply.github.com>

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.
 """
 
 

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:

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

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

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:

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(

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(

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:

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

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

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"]

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"