diff --git a/src/conclave/adapters/anthropic.py b/src/conclave/adapters/anthropic.py index 74df4e7..94ce2ee 100644 --- a/src/conclave/adapters/anthropic.py +++ b/src/conclave/adapters/anthropic.py @@ -18,7 +18,7 @@ from ..models import TokenUsage from ..registry import PROVIDER_ENV_VARS -from .base import ProviderError +from .base import ProviderError, status_error ANTHROPIC_URL = "https://api.anthropic.com/v1/messages" ANTHROPIC_VERSION = "2023-06-01" @@ -47,13 +47,15 @@ def build_request( self, model_id: str, messages: list[dict[str, str]], - temperature: float, + temperature: float | None, timeout: float, api_key: str, ) -> tuple[str, dict[str, str], dict]: """Build the Messages POST, hoisting system out of the message array. - See :meth:`ProviderAdapter.build_request`. + ``temperature`` is included only when not ``None``; passing ``None`` + omits it so the provider applies its own default (some models reject an + explicit ``temperature``). See :meth:`ProviderAdapter.build_request`. """ headers = { "x-api-key": api_key, @@ -78,8 +80,9 @@ def build_request( "model": self._bare_model(model_id), "max_tokens": self.max_tokens, "messages": turns, - "temperature": temperature, } + if temperature is not None: + body["temperature"] = temperature if system_parts: body["system"] = "\n\n".join(system_parts) return self.completions_url, headers, body @@ -90,7 +93,9 @@ def parse_response(self, status: int, payload: object) -> tuple[str, TokenUsage See :meth:`ProviderAdapter.parse_response`. """ if status < 200 or status >= 300: - raise ProviderError(_status_error(status, payload)) + raise ProviderError( + status_error("anthropic", status, payload, secondary_keys=("type",)) + ) if not isinstance(payload, dict): raise ProviderError(f"anthropic: non-JSON response body (status {status})") @@ -120,18 +125,3 @@ def _parse_usage(raw: object) -> TokenUsage | None: completion_tokens=completion, total_tokens=prompt + completion, ) - - -def _status_error(status: int, payload: object) -> str: - """Build a concise, redact-safe error message for a non-2xx status.""" - detail = "" - if isinstance(payload, dict): - err = payload.get("error") - if isinstance(err, dict): - detail = str(err.get("message") or err.get("type") or "") - elif isinstance(err, str): - detail = err - elif isinstance(payload, str): - detail = payload[:200] - suffix = f": {detail}" if detail else "" - return f"anthropic: HTTP {status}{suffix}" diff --git a/src/conclave/adapters/base.py b/src/conclave/adapters/base.py index 2bfc7a2..f439ed1 100644 --- a/src/conclave/adapters/base.py +++ b/src/conclave/adapters/base.py @@ -38,6 +38,65 @@ _REDACTED = "[REDACTED]" +# Upper bound on the error-detail substring extracted from a provider body. A +# provider can return an arbitrarily large error payload (a multi-KB +# ``error.message`` has been observed); without a cap that whole blob lands in +# ``ModelAnswer.error``, logs, and ``--json`` output, and amplifies any leak the +# redactor then has to scrub. Bounded here so error strings stay readable. +_DETAIL_CAP = 500 + + +def status_error( + prefix: str, + status: int, + payload: object, + *, + secondary_keys: tuple[str, ...] = (), +) -> str: + """Build a concise, redact-safe ``"{prefix}: HTTP {status}[: detail]"`` string. + + Shared by every concrete adapter so the non-2xx error format -- and the + detail length cap -- live in exactly one place. The extracted ``detail`` is + always truncated to :data:`_DETAIL_CAP` characters regardless of whether it + came from a dict ``error.message``, a secondary key, a string ``error``, a + top-level ``message``, or a raw string body. The returned message is NOT + redacted here; callers wrap it in :class:`ProviderError`, which redacts on + construction. + + Args: + prefix: Provider label that opens the message (e.g. ``"anthropic"``). + status: HTTP status code returned by the transport. + payload: Decoded JSON object (dict), a raw string body, or anything else. + secondary_keys: Fallback keys read from a dict ``error`` object when + ``error.message`` is absent -- e.g. ``("type",)`` for Anthropic/OpenAI + or ``("status",)`` for Gemini. Tried in order. + + Returns: + A bounded, single-line error string safe to pass to ``ProviderError``. + """ + detail = "" + if isinstance(payload, dict): + err = payload.get("error") + if isinstance(err, dict): + detail = str(err.get("message") or "") + if not detail: + for key in secondary_keys: + value = err.get(key) + if value: + detail = str(value) + break + elif isinstance(err, str): + detail = err + # Some OpenAI-compatible providers put the message at the top level. + if not detail and "message" in payload: + detail = str(payload["message"]) + elif isinstance(payload, str): + detail = payload + + detail = detail[:_DETAIL_CAP] + suffix = f": {detail}" if detail else "" + return f"{prefix}: HTTP {status}{suffix}" + def _custom_endpoint_env_vars() -> list[str]: """Return the env-var NAMES declared by custom endpoints, or [] on any error. @@ -125,7 +184,7 @@ def build_request( self, model_id: str, messages: list[dict[str, str]], - temperature: float, + temperature: float | None, timeout: float, api_key: str, ) -> tuple[str, dict[str, str], dict]: @@ -134,7 +193,9 @@ def build_request( Args: model_id: Friendly-resolved model id (e.g. ``"xai/grok-4.3"``). messages: OpenAI-style message list (roles system/user/assistant). - temperature: Sampling temperature. + temperature: Sampling temperature, or ``None`` to omit the parameter + entirely so the provider applies its own default (some models + reject an explicit ``temperature``). timeout: Per-call timeout in seconds (informational for body params). api_key: The resolved key VALUE, read at call time and never stored. diff --git a/src/conclave/adapters/gemini.py b/src/conclave/adapters/gemini.py index 050f58b..d16b71e 100644 --- a/src/conclave/adapters/gemini.py +++ b/src/conclave/adapters/gemini.py @@ -20,7 +20,7 @@ from ..models import TokenUsage from ..registry import PROVIDER_ENV_VARS -from .base import ProviderError +from .base import ProviderError, status_error GEMINI_BASE = "https://generativelanguage.googleapis.com/v1beta/models" DEFAULT_MAX_OUTPUT_TOKENS = 4096 @@ -53,11 +53,16 @@ def build_request( self, model_id: str, messages: list[dict[str, str]], - temperature: float, + temperature: float | None, timeout: float, api_key: str, ) -> tuple[str, dict[str, str], dict]: - """Build the generateContent POST. See :meth:`ProviderAdapter.build_request`.""" + """Build the generateContent POST. + + ``temperature`` is added to ``generationConfig`` only when not ``None``; + passing ``None`` omits it so the model applies its own default. See + :meth:`ProviderAdapter.build_request`. + """ model = self._bare_model(model_id) url = f"{GEMINI_BASE}/{model}:generateContent" headers = { @@ -77,12 +82,12 @@ def build_request( gemini_role = _ROLE_MAP.get(role, "user") contents.append({"role": gemini_role, "parts": [{"text": content}]}) + generation_config: dict = {"maxOutputTokens": self.max_output_tokens} + if temperature is not None: + generation_config["temperature"] = temperature body: dict = { "contents": contents, - "generationConfig": { - "temperature": temperature, - "maxOutputTokens": self.max_output_tokens, - }, + "generationConfig": generation_config, } if system_parts: body["systemInstruction"] = {"parts": [{"text": "\n\n".join(system_parts)}]} @@ -91,7 +96,7 @@ def build_request( def parse_response(self, status: int, payload: object) -> tuple[str, TokenUsage | None]: """Concatenate the first candidate's text parts. See base protocol.""" if status < 200 or status >= 300: - raise ProviderError(_status_error(status, payload)) + raise ProviderError(status_error("gemini", status, payload, secondary_keys=("status",))) if not isinstance(payload, dict): raise ProviderError(f"gemini: non-JSON response body (status {status})") @@ -123,18 +128,3 @@ def _parse_usage(raw: object) -> TokenUsage | None: completion_tokens=int(raw.get("candidatesTokenCount", 0) or 0), total_tokens=int(raw.get("totalTokenCount", 0) or 0), ) - - -def _status_error(status: int, payload: object) -> str: - """Build a concise, redact-safe error message for a non-2xx status.""" - detail = "" - if isinstance(payload, dict): - err = payload.get("error") - if isinstance(err, dict): - detail = str(err.get("message") or err.get("status") or "") - elif isinstance(err, str): - detail = err - elif isinstance(payload, str): - detail = payload[:200] - suffix = f": {detail}" if detail else "" - return f"gemini: HTTP {status}{suffix}" diff --git a/src/conclave/adapters/openai_compat.py b/src/conclave/adapters/openai_compat.py index e1bdaca..157dfe3 100644 --- a/src/conclave/adapters/openai_compat.py +++ b/src/conclave/adapters/openai_compat.py @@ -15,7 +15,7 @@ from __future__ import annotations from ..models import TokenUsage -from .base import ProviderError +from .base import ProviderError, status_error # Verified per-provider full completions URLs. Note Perplexity has NO ``/v1`` # segment; xAI and OpenAI do. These are the authoritative endpoints. @@ -62,11 +62,17 @@ def build_request( self, model_id: str, messages: list[dict[str, str]], - temperature: float, + temperature: float | None, timeout: float, api_key: str, ) -> tuple[str, dict[str, str], dict]: - """Build the OpenAI-style POST. See :meth:`ProviderAdapter.build_request`.""" + """Build the OpenAI-style POST. + + ``temperature`` is included only when not ``None``; passing ``None`` + omits it so the provider applies its own default (some reasoning models + reject an explicit ``temperature`` with a 400). See + :meth:`ProviderAdapter.build_request`. + """ headers = { "Authorization": f"Bearer {api_key}", "Content-Type": "application/json", @@ -74,8 +80,9 @@ def build_request( body: dict = { "model": self._bare_model(model_id), "messages": messages, - "temperature": temperature, } + if temperature is not None: + body["temperature"] = temperature if self.max_tokens is not None: body["max_tokens"] = self.max_tokens return self.completions_url, headers, body @@ -86,7 +93,9 @@ def parse_response(self, status: int, payload: object) -> tuple[str, TokenUsage See :meth:`ProviderAdapter.parse_response`. """ if status < 200 or status >= 300: - raise ProviderError(_status_error(self.prefix, status, payload)) + raise ProviderError( + status_error(self.prefix, status, payload, secondary_keys=("type",)) + ) if not isinstance(payload, dict): raise ProviderError(f"{self.prefix}: non-JSON response body (status {status})") @@ -115,20 +124,3 @@ def _parse_usage(raw: object) -> TokenUsage | None: completion_tokens=int(raw.get("completion_tokens", 0) or 0), total_tokens=int(raw.get("total_tokens", 0) or 0), ) - - -def _status_error(prefix: str, status: int, payload: object) -> str: - """Build a concise, redact-safe error message for a non-2xx status.""" - detail = "" - if isinstance(payload, dict): - err = payload.get("error") - if isinstance(err, dict): - detail = str(err.get("message") or err.get("type") or "") - elif isinstance(err, str): - detail = err - if not detail and "message" in payload: - detail = str(payload["message"]) - elif isinstance(payload, str): - detail = payload[:200] - suffix = f": {detail}" if detail else "" - return f"{prefix}: HTTP {status}{suffix}" diff --git a/tests/test_adapters.py b/tests/test_adapters.py index e056b60..d393e38 100644 --- a/tests/test_adapters.py +++ b/tests/test_adapters.py @@ -19,6 +19,7 @@ from conclave.adapters import ProviderError from conclave.adapters.anthropic import AnthropicAdapter +from conclave.adapters.base import _DETAIL_CAP, status_error from conclave.adapters.gemini import GeminiAdapter from conclave.adapters.openai_compat import OpenAICompatAdapter @@ -274,3 +275,117 @@ def test_gemini_parse_error_status_raises(): payload = {"error": {"status": "PERMISSION_DENIED", "message": "no access"}} with pytest.raises(ProviderError, match="HTTP 403"): adapter.parse_response(403, payload) + + +# --------------------------------------------------------------------------- # +# Shared status_error helper (issue #16) — consolidation + detail cap (D-3/D-4) +# --------------------------------------------------------------------------- # + + +def test_status_error_dict_message(): + msg = status_error("openai", 401, {"error": {"message": "bad key", "type": "auth"}}) + assert msg == "openai: HTTP 401: bad key" + + +def test_status_error_secondary_key_fallback(): + # No message -> fall back to the requested secondary key. + anthropic_msg = status_error( + "anthropic", 529, {"error": {"type": "overloaded_error"}}, secondary_keys=("type",) + ) + assert anthropic_msg == "anthropic: HTTP 529: overloaded_error" + gemini_msg = status_error( + "gemini", 403, {"error": {"status": "PERMISSION_DENIED"}}, secondary_keys=("status",) + ) + assert gemini_msg == "gemini: HTTP 403: PERMISSION_DENIED" + + +def test_status_error_string_error_and_top_level_message(): + assert status_error("xai", 500, {"error": "boom"}) == "xai: HTTP 500: boom" + # OpenAI-compatible providers sometimes put the message at the top level. + assert status_error("openai", 400, {"message": "top-level"}) == "openai: HTTP 400: top-level" + + +def test_status_error_raw_string_body(): + assert status_error("openai", 502, "gateway error") == "openai: HTTP 502: gateway error" + + +def test_status_error_no_detail(): + assert status_error("openai", 503, {}) == "openai: HTTP 503" + assert status_error("openai", 503, None) == "openai: HTTP 503" + + +def test_status_error_caps_oversized_dict_message(): + # D-3: a huge dict error.message must be bounded just like the string path. + huge = "x" * 5000 + msg = status_error("openai", 400, {"error": {"message": huge}}) + detail = msg.split(": ", 2)[2] + assert len(detail) == _DETAIL_CAP + # Whole message stays well under the pre-fix ~5018-char blowup. + assert len(msg) <= _DETAIL_CAP + 64 + + +def test_status_error_caps_oversized_raw_string_body(): + msg = status_error("openai", 400, "y" * 5000) + assert len(msg.split(": ", 2)[2]) == _DETAIL_CAP + + +def test_status_error_caps_secondary_key(): + msg = status_error("gemini", 400, {"error": {"status": "z" * 5000}}, secondary_keys=("status",)) + assert len(msg.split(": ", 2)[2]) == _DETAIL_CAP + + +# --------------------------------------------------------------------------- # +# Conditional temperature (issue #22) — None omits the param (D-11) +# --------------------------------------------------------------------------- # + + +def test_openai_compat_temperature_included_when_set(): + adapter = _openai_adapter() + _url, _headers, body = adapter.build_request( + "openai/gpt-4.1", [{"role": "user", "content": "hi"}], 0.7, 120.0, "k" + ) + assert body["temperature"] == 0.7 + + +def test_openai_compat_temperature_omitted_when_none(): + adapter = _openai_adapter() + _url, _headers, body = adapter.build_request( + "openai/o1", [{"role": "user", "content": "hi"}], None, 120.0, "k" + ) + assert "temperature" not in body + + +def test_anthropic_temperature_included_when_set(): + adapter = AnthropicAdapter() + _url, _headers, body = adapter.build_request( + "anthropic/claude-sonnet-4-6", [{"role": "user", "content": "hi"}], 0.3, 120.0, "k" + ) + assert body["temperature"] == 0.3 + + +def test_anthropic_temperature_omitted_when_none(): + adapter = AnthropicAdapter() + _url, _headers, body = adapter.build_request( + "anthropic/claude-sonnet-4-6", [{"role": "user", "content": "hi"}], None, 120.0, "k" + ) + assert "temperature" not in body + # Required params are still present. + assert body["max_tokens"] == 4096 + + +def test_gemini_temperature_included_when_set(): + adapter = GeminiAdapter() + _url, _headers, body = adapter.build_request( + "gemini/gemini-2.5-pro", [{"role": "user", "content": "hi"}], 0.4, 120.0, "k" + ) + assert body["generationConfig"]["temperature"] == 0.4 + + +def test_gemini_temperature_omitted_when_none(): + adapter = GeminiAdapter() + _url, _headers, body = adapter.build_request( + "gemini/gemini-2.5-pro", [{"role": "user", "content": "hi"}], None, 120.0, "k" + ) + assert "temperature" not in body["generationConfig"] + # maxOutputTokens still set. + assert body["generationConfig"]["maxOutputTokens"] == 4096