fix temperature error.

reviewbot/llm_client.py

@@ -17,6 +17,37 @@ class _ToolsUnsupported(Exception):
     response body preview for logging."""
 
 
+class _TemperatureUnsupported(Exception):
+    """Raised when the upstream endpoint rejects ``temperature`` with a 400
+    (e.g. newer Claude models, which deprecated the parameter), so the
+    caller can retry once without it. Carries the response body preview for
+    logging."""
+
+
+def _is_temperature_rejection(body_preview: str) -> bool:
+    """True when a 400 body indicates the ``temperature`` parameter itself
+    is not accepted by the model — as opposed to a value-range complaint.
+    Provider-neutral: matches the OpenAI-compat error text emitted by
+    Anthropic's shim ("`temperature` is deprecated for this model.") and
+    similar unsupported-parameter wording from other endpoints."""
+    text = body_preview.lower()
+    if "temperature" not in text:
+        return False
+    return any(
+        signal in text
+        for signal in (
+            "deprecat",
+            "unsupported",
+            "not supported",
+            "unexpected",
+            "unknown",
+            "not permitted",
+            "not allowed",
+            "cannot be used",
+        )
+    )
+
+
 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
@@ -200,33 +231,33 @@ def complete(
         # the authoritative value once it lands.
         est_input_tokens = self._estimate_input_tokens(messages, tools)
         started = time.monotonic()
-        try:
-            return self._post_with_retries(
-                url,
-                payload,
-                attempts,
-                retryable,
-                started,
-                tools_in_use=bool(tools),
-                chunk_callback=chunk_callback,
-                est_input_tokens=est_input_tokens,
-            )
-        except _ToolsUnsupported:
-            # Endpoint rejected the tool-augmented payload. Strip the
-            # function-calling fields and try once more so the review can
-            # still complete (degraded: no browse tools).
-            for k in ("tools", "tool_choice"):
-                payload.pop(k, None)
-            return self._post_with_retries(
-                url,
-                payload,
-                attempts,
-                retryable,
-                started,
-                tools_in_use=False,
-                chunk_callback=chunk_callback,
-                est_input_tokens=est_input_tokens,
-            )
+        # The endpoint may reject specific payload fields with a 400: some
+        # models deprecate `temperature` (e.g. newer Claude), others don't
+        # support function-calling. Strip the offending field and retry,
+        # at most once per removable field, so the review still completes.
+        while True:
+            try:
+                return self._post_with_retries(
+                    url,
+                    payload,
+                    attempts,
+                    retryable,
+                    started,
+                    tools_in_use="tools" in payload,
+                    chunk_callback=chunk_callback,
+                    est_input_tokens=est_input_tokens,
+                )
+            except _TemperatureUnsupported:
+                if "temperature" not in payload:
+                    raise
+                payload.pop("temperature", None)
+            except _ToolsUnsupported:
+                # Strip the function-calling fields and try once more
+                # (degraded: no browse tools).
+                if "tools" not in payload:
+                    raise
+                payload.pop("tools", None)
+                payload.pop("tool_choice", None)
 
     # Cap any single retry wait, even if the server hands us a huge
     # Retry-After. 120s is plenty for dynamic-rate-limit recovery
@@ -343,12 +374,24 @@ def _post_with_retries(
                         url,
                         body_preview,
                     )
-                    if r.status_code == 400 and tools_in_use:
-                        log.warning(
-                            "Retrying once without tools (the endpoint may "
-                            "not support function-calling for this model)"
-                        )
-                        raise _ToolsUnsupported(body_preview)
+                    if r.status_code == 400:
+                        # Check the temperature rejection first: stripping
+                        # tools wouldn't fix it, and some models (e.g. newer
+                        # Claude) reject `temperature` outright.
+                        if "temperature" in payload and _is_temperature_rejection(
+                            body_preview
+                        ):
+                            log.warning(
+                                "Retrying without `temperature` (the model "
+                                "appears to reject the parameter)"
+                            )
+                            raise _TemperatureUnsupported(body_preview)
+                        if tools_in_use:
+                            log.warning(
+                                "Retrying once without tools (the endpoint may "
+                                "not support function-calling for this model)"
+                            )
+                            raise _ToolsUnsupported(body_preview)
                     raise LLMResponseError(
                         r.status_code, r.reason or "", url, body_preview
                     )

tests/test_llm_client.py

@@ -591,6 +591,78 @@ def test_complete_falls_back_when_endpoint_rejects_tools(self) -> None:
         self.assertIn("tools", first_payload)
         self.assertNotIn("tools", second_payload)
 
+    def test_complete_falls_back_when_endpoint_rejects_temperature(self) -> None:
+        # First call (with temperature) returns 400 complaining the
+        # parameter is deprecated; the retry drops temperature and succeeds.
+        rejected = Mock(
+            status_code=400,
+            ok=False,
+            reason="Bad Request",
+            text=(
+                '{"error":{"code":"invalid_request_error","message":'
+                '"`temperature` is deprecated for this model.",'
+                '"type":"invalid_request_error"}}'
+            ),
+            raise_for_status=Mock(side_effect=AssertionError("should not be reached")),
+        )
+        ok = Mock(
+            status_code=200,
+            ok=True,
+            json=Mock(
+                return_value={
+                    "choices": [{"message": {"content": "ok"}, "finish_reason": "stop"}]
+                }
+            ),
+            raise_for_status=Mock(),
+        )
+        with (
+            patch("reviewbot.llm_client.time.sleep"),
+            patch(
+                "reviewbot.llm_client.requests.post", side_effect=[rejected, ok]
+            ) as mock_post,
+        ):
+            client = ChatCompletionClient(
+                "https://example.com/v1", "token", "fixed-model"
+            )
+            result = client.complete(
+                [{"role": "user", "content": "hi"}],
+                tools=[{"type": "function", "function": {"name": "read_file"}}],
+            )
+
+        self.assertEqual(result.content, "ok")
+        # Two POSTs: first with temperature, second without.
+        self.assertEqual(mock_post.call_count, 2)
+        first_payload = json.loads(mock_post.call_args_list[0].kwargs["data"])
+        second_payload = json.loads(mock_post.call_args_list[1].kwargs["data"])
+        self.assertIn("temperature", first_payload)
+        self.assertNotIn("temperature", second_payload)
+        # Tools are unrelated to the rejection, so they're preserved.
+        self.assertIn("tools", second_payload)
+
+    def test_complete_does_not_strip_temperature_on_value_error(self) -> None:
+        # A value-range complaint mentions "temperature" but isn't a
+        # parameter rejection — it should bubble up, not silently retry.
+        rejected = Mock(
+            status_code=400,
+            ok=False,
+            reason="Bad Request",
+            text='{"error":"temperature must be between 0 and 2"}',
+            raise_for_status=Mock(side_effect=requests.HTTPError("400")),
+        )
+        with (
+            patch("reviewbot.llm_client.time.sleep"),
+            patch(
+                "reviewbot.llm_client.requests.post", return_value=rejected
+            ) as mock_post,
+        ):
+            client = ChatCompletionClient(
+                "https://example.com/v1", "token", "fixed-model"
+            )
+            with self.assertRaises(requests.HTTPError):
+                client.complete([{"role": "user", "content": "hi"}])
+
+        self.assertEqual(mock_post.call_count, 1)
+
     def test_complete_does_not_fall_back_on_400_without_tools(self) -> None:
         # If we never sent tools, a 400 should bubble up — not silently retry.
         rejected = Mock(