add error handling

Author: tarekziade

reviewbot/llm_client.py

@@ -15,6 +15,22 @@ class _ToolsUnsupported(Exception):
     response body preview for logging."""
 
 
+class LLMResponseError(requests.HTTPError):
+    """Non-OK HTTP response from the chat-completions endpoint that
+    exhausted retries (or wasn't retryable to begin with). Carries the
+    status code, upstream reason phrase, and a short preview of the
+    response body so the web UI / action log can show *why* the request
+    failed without re-fetching it.
+    """
+
+    def __init__(self, status_code: int, reason: str, url: str, body_preview: str):
+        self.status_code = status_code
+        self.reason = reason
+        self.url = url
+        self.body_preview = body_preview
+        super().__init__(f"{status_code} {reason} for {url}: {body_preview}")
+
+
 @dataclass
 class ToolCall:
     """One tool/function call emitted by the assistant. ``arguments`` is
@@ -196,6 +212,35 @@ def complete(
                 est_input_tokens=est_input_tokens,
             )
 
+    # Cap any single retry wait, even if the server hands us a huge
+    # Retry-After. 120s is plenty for dynamic-rate-limit recovery
+    # without letting a misbehaving upstream pin a worker forever.
+    _RETRY_WAIT_CEILING_SECONDS = 120.0
+
+    @staticmethod
+    def _retry_delay(attempt: int, response: "requests.Response") -> float:
+        """Compute how long to sleep before retrying. Honors ``Retry-After``
+        (seconds or HTTP-date form) when the server provides one; otherwise
+        falls back to exponential backoff (2,4,8,...)."""
+        ra = response.headers.get("Retry-After")
+        # Guard against non-string values (e.g. Mock objects in tests).
+        # Real ``requests.Response`` only ever returns ``str | None`` here.
+        if isinstance(ra, str) and ra:
+            try:
+                return min(float(ra), ChatCompletionClient._RETRY_WAIT_CEILING_SECONDS)
+            except ValueError:
+                # HTTP-date form (rare on LLM endpoints) — parse it.
+                try:
+                    from email.utils import parsedate_to_datetime
+
+                    target = parsedate_to_datetime(ra)
+                    secs = (target.timestamp() - time.time())
+                    if secs > 0:
+                        return min(secs, ChatCompletionClient._RETRY_WAIT_CEILING_SECONDS)
+                except Exception:  # noqa: BLE001
+                    pass
+        return float(2**attempt)
+
     @staticmethod
     def _estimate_input_tokens(
         messages: list[dict[str, Any]], tools: Optional[list[dict[str, Any]]]
@@ -246,14 +291,23 @@ def _post_with_retries(
                     timeout=300,
                     stream=self.stream,
                 )
-                if r.status_code >= 500 and attempt < attempts:
+                # 429 (rate limit) and 5xx are retryable. 429 in particular
+                # gets hit during agentic tool loops on providers with
+                # bursty-traffic dynamic limits (e.g. Together.ai via HF
+                # Router, where a few quick tool turns can exceed a 1 RPM
+                # cap). Honor Retry-After when the server provides it.
+                if (
+                    r.status_code == 429 or r.status_code >= 500
+                ) and attempt < attempts:
+                    delay = self._retry_delay(attempt, r)
                     log.warning(
-                        "LLM call attempt %d/%d returned %d; retrying",
+                        "LLM call attempt %d/%d returned %d; retrying in %.1fs",
                         attempt,
                         attempts,
                         r.status_code,
+                        delay,
                     )
-                    time.sleep(2**attempt)
+                    time.sleep(delay)
                     continue
                 if not r.ok:
                     # Surface the upstream error body so the action log
@@ -274,7 +328,9 @@ def _post_with_retries(
                             "not support function-calling for this model)"
                         )
                         raise _ToolsUnsupported(body_preview)
-                r.raise_for_status()
+                    raise LLMResponseError(
+                        r.status_code, r.reason or "", url, body_preview
+                    )
                 if self.stream:
                     (
                         content,

reviewbot/webapp.py

@@ -42,6 +42,7 @@
     installation_token,
 )
 from .github_client import GitHubClient
+from .llm_client import LLMResponseError
 from .reviewer import (
     DraftComment,
     ReviewDraft,
@@ -418,6 +419,34 @@ def _run_review_worker(job: Job) -> None:
         _push_event(job, "step", "error")
         _push_event(job, "error", job.error)
         _push_event(job, "done", "")
+    except LLMResponseError as exc:
+        # Upstream chat-completions endpoint returned a non-OK status
+        # (and either wasn't retryable, or retries didn't recover).
+        # Surface the status code + a body excerpt directly to the SSE
+        # client — without it the UI just shows "review crashed (see
+        # server log)" and the user has to SSH into the box to figure
+        # out whether it was a 429 (rate limit), a 400 (bad schema),
+        # auth, etc. The body comes from the LLM provider's own error
+        # response — no auth tokens of ours are echoed there.
+        log.warning(
+            "LLM endpoint returned %d for job %s: %s",
+            exc.status_code,
+            job.id,
+            exc.body_preview[:400],
+        )
+        job.status = "error"
+        excerpt = exc.body_preview.strip()
+        if len(excerpt) > 600:
+            excerpt = excerpt[:600] + "…"
+        reason_part = f" {exc.reason}" if exc.reason else ""
+        job.error = (
+            f"LLM endpoint returned {exc.status_code}{reason_part}: {excerpt}"
+            if excerpt
+            else f"LLM endpoint returned {exc.status_code}{reason_part}"
+        )
+        _push_event(job, "step", "error")
+        _push_event(job, "error", job.error)
+        _push_event(job, "done", "")
     except Exception as exc:  # noqa: BLE001
         log.exception("review worker crashed for job %s", job.id)
         job.status = "error"

tests/test_llm_client.py

@@ -4,7 +4,7 @@
 
 import requests
 
-from reviewbot.llm_client import ChatCompletionClient
+from reviewbot.llm_client import ChatCompletionClient, LLMResponseError
 
 
 def _interrupted_iter_lines(prefix_lines: list[str], exc: Exception):
@@ -158,6 +158,50 @@ def test_complete_retries_on_5xx_then_succeeds(self) -> None:
         self.assertEqual(result.content, "ok")
         self.assertEqual(mock_post.call_count, 2)
 
+    def test_complete_retries_on_429_then_succeeds(self) -> None:
+        rate_limited = Mock(
+            status_code=429,
+            ok=False,
+            reason="Too Many Requests",
+            text="rate limit",
+            headers={"Retry-After": "1"},
+        )
+        ok = Mock(
+            status_code=200,
+            json=Mock(return_value={"choices": [{"message": {"content": "ok"}}]}),
+            raise_for_status=Mock(),
+        )
+        with patch("reviewbot.llm_client.time.sleep") as mock_sleep, patch(
+            "reviewbot.llm_client.requests.post", side_effect=[rate_limited, ok]
+        ) as mock_post:
+            client = ChatCompletionClient("https://example.com/v1", "token", "fixed-model")
+            result = client.complete([{"role": "user", "content": "hi"}])
+
+        self.assertEqual(result.content, "ok")
+        self.assertEqual(mock_post.call_count, 2)
+        # Honored the Retry-After: 1 header, capped to the per-wait ceiling.
+        mock_sleep.assert_called_once_with(1.0)
+
+    def test_complete_raises_llmresponseerror_with_status_and_body_after_4xx(self) -> None:
+        bad = Mock(
+            status_code=400,
+            ok=False,
+            reason="Bad Request",
+            text='{"error":{"message":"response_format.type bad"}}',
+            headers={},
+        )
+        with patch("reviewbot.llm_client.requests.post", return_value=bad):
+            client = ChatCompletionClient("https://example.com/v1", "token", "fixed-model")
+            with self.assertRaises(LLMResponseError) as ctx:
+                client.complete([{"role": "user", "content": "hi"}])
+
+        exc = ctx.exception
+        self.assertEqual(exc.status_code, 400)
+        self.assertEqual(exc.reason, "Bad Request")
+        self.assertIn("response_format.type bad", exc.body_preview)
+        # Also a real requests.HTTPError so existing handlers still catch it.
+        self.assertIsInstance(exc, requests.HTTPError)
+
     def test_complete_consumes_sse_stream_when_streaming_enabled(self) -> None:
         sse_lines = [
             'data: {"choices":[{"delta":{"content":"he"}}]}',