diff --git a/eval/harbor/clawcodex_agent.py b/eval/harbor/clawcodex_agent.py index b0fad998..8b606ff1 100644 --- a/eval/harbor/clawcodex_agent.py +++ b/eval/harbor/clawcodex_agent.py @@ -63,15 +63,25 @@ (see ``source`` below) before an effort number means anything. ``xhigh`` is model-dependent (opus-5/opus-4-8 yes; sonnet-4-6/opus-4-6 no) and degrades to ``high`` where rejected. - - **OpenAI-compatible wire** (openrouter, openai, deepseek, zai, …) — - sent as the top-level ``reasoning_effort`` body field, with no model - allowlist and no ``xhigh`` clamp; the level passes through verbatim. - ONE exception: the ChatGPT-subscription path (``subscription=true`` - with ``--model openai/…``) clamps ``xhigh`` and ``max`` down to - ``high`` before sending, because that backend advertises only - low/medium/high and rejects higher tiers. So ``--ak effort=max`` plus - subscription auth really runs at ``high`` — an API-key or OpenRouter - run of the same model does not. + - **OpenAI-compatible wire** (openrouter, deepseek, zai, …) — sent as + the top-level ``reasoning_effort`` body field, with no model allowlist + and no ``xhigh`` clamp; the level passes through verbatim. This is the + path ``openrouter/openai/gpt-5.6-luna`` takes, and OpenRouter accepts + ``max`` for that model. + - **First-party ``--provider openai``** — reasoning models go over the + Responses API, which tops out at ``xhigh``: ``max`` is degraded to + ``xhigh`` rather than sent (the API rejects ``max`` outright with + "Supported values are: 'none', 'low', 'medium', 'high', and 'xhigh'"). + So ``--ak effort=max`` against ``openai/gpt-5.6-luna`` really runs at + ``xhigh``, while the same nominal setting on OpenRouter sends ``max``. + Non-reasoning models (gpt-4o, …) stay on Chat Completions and have the + effort field STRIPPED, since they reject it as an unknown argument. + - **ChatGPT subscription** (``subscription=true`` with + ``--model openai/…``) clamps ``xhigh`` and ``max`` down to ``high``, + because that backend advertises only low/medium/high. So the same + ``effort=max`` runs at three different levels depending on route: + ``max`` on OpenRouter, ``xhigh`` on an OpenAI key, ``high`` on a + ChatGPT plan. REQUIRES a clawcodex build from 2026-07-31 or later. Before that, effort was emitted ONLY on the Anthropic branch, so ``--effort`` was a silent diff --git a/src/providers/openai_provider.py b/src/providers/openai_provider.py index 2845039e..cd8eb432 100644 --- a/src/providers/openai_provider.py +++ b/src/providers/openai_provider.py @@ -1,16 +1,37 @@ """OpenAI provider implementation. -Two request paths: - -- **API key** (default): OpenAI SDK against ``/v1/chat/completions`` via - :class:`OpenAICompatibleProvider` — unchanged behaviour. -- **ChatGPT subscription**: when no API key is configured but the user has - connected a ChatGPT Plus/Pro plan (``clawcodex login`` → - ``src/auth/openai_subscription.py``), requests go to the ChatGPT Codex - backend (``https://chatgpt.com/backend-api/codex/responses``) speaking the - Responses API — the mechanism OpenCode's ``openai`` plugin uses +PROTOCOL and ROUTE are separate axes here. The protocol follows the MODEL, +the route follows the AUTH — conflating them is what this module got wrong +for a while, and the split is the thing to preserve when editing it. + +Protocol (``_use_responses``): + +- **Responses API** for reasoning models (gpt-5.x, o-series, codex). + ``/v1/chat/completions`` supports them inconsistently — it rejects tools + outright for some, tools-plus-effort for others — and an agentic run + always sends tools. Responses serves them all uniformly. +- **Chat Completions** via the OpenAI SDK and + :class:`OpenAICompatibleProvider` for everything else (gpt-4o, ``-chat`` + variants, …), which is the older and more heavily exercised path. + +Route (``_subscription_stream_request``), for the Responses protocol only: + +- **API key**: ``{base_url}/responses`` with a bearer key. Metered, so its + usage is billed and its effort ceiling is ``xhigh``. +- **ChatGPT subscription**: no API key configured but a connected Plus/Pro + plan (``clawcodex login`` → ``src/auth/openai_subscription.py``) sends to + the Codex backend (``https://chatgpt.com/backend-api/codex/responses``) + with OAuth — the mechanism OpenCode's ``openai`` plugin uses (reference_projects/opencode/packages/opencode/src/plugin/openai/codex.ts). - Wire-format conversion lives in ``src/providers/openai_responses.py``. + Flat-rate, so its usage is not billed, its effort tops out at ``high``, + and it rejects sampler params the public API accepts. + +Anything keyed on "is this a subscription" must therefore be about the +ROUTE, never the protocol: billing, the effort ceiling, ``max_output_tokens`` +and the 401-refresh dance all differ, and the two share every other byte. +A non-``api.openai.com`` base URL — from config OR ``$OPENAI_BASE_URL`` — +means a proxy, which disables both the Responses switch and the OAuth +fallback. Wire-format conversion lives in ``src/providers/openai_responses.py``. """ from __future__ import annotations @@ -38,6 +59,8 @@ RESPONSES_ITEM_BLOCK_TYPE, INCLUDE_ENCRYPTED_REASONING, SUBSCRIPTION_MODELS, + normalize_openai_effort, + supports_reasoning, build_usage_dict, convert_messages_to_responses_input, convert_tools_to_responses_format, @@ -96,9 +119,35 @@ def __init__(self, response: Any) -> None: self.response = response +class ResponsesHTTPError(RuntimeError): + """A non-200 from the Responses endpoint, carrying its status. + + The retry layer classifies purely by attribute: ``query.py`` reads + ``e.status_code`` to decide whether an error is retryable, and + ``_retry_after_seconds`` reads ``e.response.headers`` to honour + ``Retry-After``. A bare ``RuntimeError`` has neither, so a 429 or a 503 + here looked like a permanent failure — the request was abandoned instead + of backed off, and the server's own pacing hint was discarded. + + That was survivable while this path served only ChatGPT subscriptions, + which rate-limit by plan. It is not survivable for API keys, where 429 + is routine under the concurrency an eval run generates. + + Subclasses ``RuntimeError`` so existing ``except RuntimeError`` handlers + keep catching it. + """ + + def __init__(self, message: str, *, status_code: int, response: Any = None): + super().__init__(message) + self.status_code = status_code + self.response = response + + class OpenAIProvider(OpenAICompatibleProvider): - """OpenAI provider using OpenAI SDK (API key) or the ChatGPT Codex - backend (subscription OAuth).""" + """OpenAI provider: Responses API for reasoning models, Chat Completions + via the OpenAI SDK for the rest, over either an API key or ChatGPT + subscription OAuth. See the module docstring for the protocol/route + split.""" def __init__( self, api_key: str, base_url: Optional[str] = None, model: Optional[str] = None @@ -123,7 +172,15 @@ def __init__( # the first-party endpoint (custom base URLs mean a proxy/gateway # that expects the configured key semantics). Same policy as the # Anthropic provider's Claude-subscription fallback. - oauth_eligible = not base_url or urlparse(base_url).hostname == "api.openai.com" + # + # Shares ``_is_first_party_base_url`` rather than re-deriving the + # rule, so the ``$OPENAI_BASE_URL`` channel cannot be honoured in one + # place and ignored in the other. It reached this branch first: an + # OAuth session cannot be proxied, so with a proxy in the env, no key + # and a stale ChatGPT login, prompts went to chatgpt.com with no + # error — the exact outcome this guard exists to prevent. + # ``super().__init__`` above has already populated ``self.base_url``. + oauth_eligible = self._is_first_party_base_url() if not api_key and oauth_eligible: # Presence check only — deliberately NOT ``get_valid_credentials``: # that can perform a blocking token refresh (30 s urllib timeout), @@ -177,18 +234,134 @@ def get_available_models(self) -> list[str]: return list(PROVIDER_INFO["openai"]["available_models"]) # ------------------------------------------------------------------ - # ChatGPT-subscription path (Responses API against the Codex backend) + # Responses-API path (API key against /v1/responses, or the ChatGPT + # subscription against the Codex backend — same protocol, two routes) # ------------------------------------------------------------------ + def _use_responses(self, **kwargs: Any) -> bool: + """Whether this request goes over the Responses protocol. + + Keyed on the MODEL, never on the auth mode — the shape OpenCode's + provider facade takes (``model: responses``, with ``chat`` an + explicit opt-in). + + The reason is that Chat Completions supports reasoning models + INCONSISTENTLY, per model. Probed live against the real API + 2026-08-01, all with tools attached: + + model tools, no effort tools + effort + gpt-5 200 200 + gpt-5.4 200 400 + gpt-5.6-luna 400 400 + + with the 400 reading "Function tools with reasoning_effort are not + supported for in /v1/chat/completions. To use function tools, + use /v1/responses or set reasoning_effort to 'none'." — it fires for + luna even with NO effort in the body, because that model's default + reasoning level is not 'none'. + + So the endpoint's tool support is a per-model minefield that shifts + with each release, and an agentic run always sends tools. Responses + serves all of them uniformly and is the endpoint the error itself + points at, so routing every reasoning model there replaces the + minefield with one rule. + + Note this DOES move models that work today (gpt-5, and gpt-5.4 when + no effort is set) onto the Responses path. That is the intended + trade: one uniformly-supported protocol over a per-model matrix. + + Non-reasoning models (gpt-4o, gpt-3.5-turbo) stay on Chat + Completions. Responses serves them too, but they have no reasoning + block to negotiate and therefore no defect to fix, and that path + carries the older, more heavily exercised streaming code. + """ + if self._subscription_active: + return True + if not self.api_key: + return False + if not self._is_first_party_base_url(): + return False + return supports_reasoning(self._get_model(**kwargs)) + + def _is_first_party_base_url(self) -> bool: + """Whether requests go to OpenAI itself rather than a gateway. + + ``providers.openai.base_url`` is user-configurable (config.py), so + this provider is also how people reach LiteLLM/vLLM/Azure-style + proxies. Those speak Chat Completions universally but implement + ``/responses`` only sometimes, so switching protocol underneath one + would turn a working setup into a 404. + + The 400s that motivate the Responses route are api.openai.com's own + behaviour, and cannot be assumed to apply to a proxy that normalises + requests. So the switch is scoped to the host whose behaviour was + actually measured; everything else keeps the protocol it has today. + """ + base = self._configured_base_url() + if not base: + return True + return (urlparse(base).hostname or "").lower() == "api.openai.com" + + def _configured_base_url(self) -> str: + """The base URL actually in force, from EITHER channel. + + ``self.base_url`` is commonly None — ``set_api_key`` only writes the + key when one is passed (config.py), and the server forwards + ``provider_cfg.get("base_url")`` — in which case the OpenAI SDK falls + back to ``$OPENAI_BASE_URL`` (openai/_client.py). Reading only the + attribute would therefore see "first-party" for a session that the + Chat Completions path sends to a proxy, and the two protocols would + diverge to different hosts: an egress proxy configured by env var + would be bypassed for the default model, carrying the key and the + conversation straight to OpenAI with no error. ``$OPENAI_BASE_URL`` + is a documented knob here (eval/README.md). + """ + return (self.base_url or os.environ.get("OPENAI_BASE_URL") or "").strip() + + def _without_unsupported_reasoning(self, kwargs: dict[str, Any]) -> dict[str, Any]: + """Drop ``reasoning_effort`` for models that have no reasoning. + + The wire boundary injects ``extra_body.reasoning_effort`` for every + OpenAI-compatible provider, which is right for the reasoning models + but a hard 400 on the rest:: + + gpt-4o + reasoning_effort=high + -> 400 "Unrecognized request argument supplied: reasoning_effort" + + so ``--provider openai --model gpt-4o --effort high`` could not make a + single call. Gating on the model mirrors how OpenCode attaches + reasoning options only to models that declare the capability. + + Scoped to the first-party provider on purpose: the capability check + keys on OpenAI's own naming, and other OpenAI-compatible providers + (OpenRouter, DeepSeek) namespace their ids differently and accept the + field on models this predicate would not recognise. Applying it + globally would silently drop effort for them — including the + ``openrouter/openai/gpt-5.6-luna`` configuration the evals run on. + """ + extra = kwargs.get("extra_body") or {} + if "reasoning_effort" not in extra: + return kwargs + if supports_reasoning(self._get_model(**kwargs)): + return kwargs + pruned = {k: v for k, v in extra.items() if k != "reasoning_effort"} + out = dict(kwargs) + # Preserve an explicitly-empty extra_body rather than dropping the key, + # so callers that inspect it see the same shape they passed. + out["extra_body"] = pruned + return out + def chat( self, messages: list[MessageInput], tools: Optional[list[dict[str, Any]]] = None, **kwargs, ) -> ChatResponse: - if self._subscription_active: + if self._use_responses(**kwargs): return self._subscription_stream_request(messages, tools, **kwargs) - return super().chat(messages, tools, **kwargs) + return super().chat( + messages, tools, **self._without_unsupported_reasoning(kwargs) + ) def chat_stream( self, @@ -196,8 +369,10 @@ def chat_stream( tools: Optional[list[dict[str, Any]]] = None, **kwargs, ) -> Generator[str, None, None]: - if not self._subscription_active: - yield from super().chat_stream(messages, tools, **kwargs) + if not self._use_responses(**kwargs): + yield from super().chat_stream( + messages, tools, **self._without_unsupported_reasoning(kwargs) + ) return # Callback → generator adaptation: run the request on a worker and # relay text deltas through a bounded queue. @@ -226,7 +401,7 @@ def _run() -> None: raise item yield item - def chat_stream_response( + def _stream_attempt( self, messages: list[MessageInput], tools: Optional[list[dict[str, Any]]] = None, @@ -235,22 +410,33 @@ def chat_stream_response( on_thinking_chunk: TextChunkCallback | None = None, **kwargs, ) -> ChatResponse: - if self._subscription_active: + """One streaming attempt, over whichever protocol this model uses. + + Overriding the ATTEMPT rather than ``chat_stream_response`` is what + keeps the Responses path inside the base class's transport-drop retry + loop. That retry is load-bearing rather than theoretical: per + ``OpenAICompatibleProvider.chat_stream_response``, an un-retried + ``peer closed connection`` ended 8 of 89 terminal-bench 2.1 trials. + Returning early from ``chat_stream_response`` — as this class did + while the protocol was keyed on auth — silently opts every Responses + request out of it. + """ + if self._use_responses(**kwargs): return self._subscription_stream_request( messages, tools, on_text_chunk=on_text_chunk, - on_thinking_chunk=on_thinking_chunk, abort_signal=abort_signal, + on_thinking_chunk=on_thinking_chunk, **kwargs, ) - return super().chat_stream_response( + return super()._stream_attempt( messages, tools, on_text_chunk=on_text_chunk, abort_signal=abort_signal, on_thinking_chunk=on_thinking_chunk, - **kwargs, + **self._without_unsupported_reasoning(kwargs), ) def _subscription_request_body( @@ -285,16 +471,29 @@ def _subscription_request_body( "store": False, "stream": True, "include": list(INCLUDE_ENCRYPTED_REASONING), - "reasoning": { - # /effort arrives as extra_body.reasoning_effort, injected - # at the wire boundary by query.py::_call_model_sync. - "effort": _subscription_reasoning_effort( - (kwargs.get("extra_body") or {}).get("reasoning_effort") - ), - "summary": "auto", - }, "prompt_cache_key": self._subscription_session_id, } + # ``reasoning`` is gated on the MODEL, not the auth mode. Sending it + # to a non-reasoning model is a hard 400 ("Unsupported parameter: + # 'reasoning.effort' is not supported with this model" — gpt-4o, + # verified 2026-08-01), and the identical request without the block + # succeeds. That gate is what lets this protocol serve every OpenAI + # model rather than only the reasoning ones. + if supports_reasoning(model): + if self._subscription_active: + # The ChatGPT backend advertises only low/medium/high and + # rejects higher tiers, so it keeps its own clamp. + effort = _subscription_reasoning_effort( + (kwargs.get("extra_body") or {}).get("reasoning_effort") + ) + else: + # The public API accepts xhigh; only ``max`` is unsupported, + # and it degrades rather than failing the request. + effort = normalize_openai_effort( + (kwargs.get("extra_body") or {}).get("reasoning_effort") + ) + if effort: + body["reasoning"] = {"effort": effort, "summary": "auto"} if instructions: body["instructions"] = instructions if tools: @@ -305,13 +504,39 @@ def _subscription_request_body( # OpenCode sends verbosity=low for gpt-5.x non-codex non-chat # (transform.ts:1189); matches the backend's own default. body["text"] = {"verbosity": "low"} - # NOTE: remaining kwargs (max_tokens, temperature, …) are - # intentionally NOT forwarded. The Codex backend rejects sampler - # params on reasoning models, and OpenCode explicitly forces - # maxOutputTokens off for this provider ("Match codex cli", + # Remaining sampler kwargs (temperature, top_p, …) are intentionally + # NOT forwarded: the Codex backend rejects them on reasoning models, + # and OpenCode forces maxOutputTokens off for it ("Match codex cli", # plugin/openai/codex.ts:637-641). + # + # That rationale is about the CODEX BACKEND, though, so it stops + # applying once an API key uses this same protocol. The public API + # accepts ``max_output_tokens``, and callers rely on it as a bound + # rather than a preference: compaction summaries + # (COMPACT_MAX_OUTPUT_TOKENS), the permission classifier (512), the + # /goal judge, and the advisor all pass one. Dropping it silently + # unbounds them. Worse, query.py's ``max_output_tokens_escalate`` + # lane re-issues with ESCALATED_MAX_TOKENS after a truncated reply — + # if the value never reaches the wire, that retry is byte-identical + # to the request that just truncated, so it burns a full-context + # turn to reproduce the same failure. + if not self._subscription_active: + max_tokens = kwargs.get("max_tokens") + if max_tokens: + body["max_output_tokens"] = int(max_tokens) return body + def _responses_endpoint(self) -> str: + """The Responses URL for the API-key route. + + Derived from ``base_url`` so a proxy/gateway configuration keeps + working; falls back to the first-party endpoint. Kept separate from + the subscription's ``CODEX_API_ENDPOINT`` because they are two ROUTES + to the same protocol, not two protocols. + """ + base = (self._configured_base_url() or "https://api.openai.com/v1").rstrip("/") + return f"{base}/responses" + def _subscription_headers(self, access_token: str) -> dict[str, str]: from src.auth.openai_subscription import ORIGINATOR @@ -357,14 +582,31 @@ def _subscription_stream_request( guard = StreamAbortGuard(abort_signal) guard.raise_if_pre_aborted() - credentials = get_valid_credentials() - if credentials is None: - raise RuntimeError( - "ChatGPT subscription login was removed; run `clawcodex login`" + # ROUTE = endpoint + headers. The wire FORMAT below is identical for + # both; only where it is sent and how it authenticates differ. That + # split is the point of this method: protocol is a property of the + # provider, auth is a separate axis (OpenCode models it the same way — + # its Codex plugin rewrites the URL and swaps the bearer token, and + # the endpoint it rewrites TO is still ``/responses``). + credentials = None + if self._subscription_active: + credentials = get_valid_credentials() + if credentials is None: + raise RuntimeError( + "ChatGPT subscription login was removed; run `clawcodex login`" + ) + self._subscription_account_id = ( + credentials.account_id or self._subscription_account_id ) - self._subscription_account_id = ( - credentials.account_id or self._subscription_account_id - ) + endpoint = CODEX_API_ENDPOINT + headers = self._subscription_headers(credentials.access_token) + else: + endpoint = self._responses_endpoint() + headers = { + "Authorization": f"Bearer {self.api_key}", + "Content-Type": "application/json", + "Accept": "text/event-stream", + } body = self._subscription_request_body(messages, tools, **kwargs) @@ -380,16 +622,18 @@ def _subscription_stream_request( try: response = client.send( client.build_request( - "POST", - CODEX_API_ENDPOINT, - headers=self._subscription_headers(credentials.access_token), - json=body, + "POST", endpoint, headers=headers, json=body, ), stream=True, ) - if response.status_code == 401: + if response.status_code == 401 and self._subscription_active: # Server-side invalidation ahead of local expiry — refresh - # once and retry. + # once and retry. SUBSCRIPTION ONLY: on the API-key route a + # 401 means the key is rejected, and there is no OAuth token + # to refresh — running the refresh dance there would raise a + # "login expired" error for what is really a bad key, hiding + # the real cause. It falls through to the status check below, + # which surfaces the provider's own message. response.close() refreshed = force_refresh() if refreshed is None: @@ -410,8 +654,11 @@ def _subscription_stream_request( ) if response.status_code != 200: detail = response.read().decode("utf-8", "replace") - raise RuntimeError( - f"ChatGPT backend error ({response.status_code}): {detail[:600]}" + _who = "ChatGPT backend" if self._subscription_active else "OpenAI API" + raise ResponsesHTTPError( + f"{_who} error ({response.status_code}): {detail[:600]}", + status_code=response.status_code, + response=response, ) return self._consume_subscription_stream( response, guard, on_text_chunk, on_thinking_chunk, @@ -440,7 +687,13 @@ def _consume_subscription_stream( reasoning_parts: list[str] = [] items: list[dict[str, Any]] = [] tool_uses: list[dict[str, Any]] = [] - usage: dict[str, Any] = {"billing_mode": "subscription"} + # An API key is metered per token, a ChatGPT plan is flat-rate, and + # this stream now serves both — so the flag that zeroes cost in + # cost_tracker must follow the auth mode rather than the protocol. + subscription = self._subscription_active + usage: dict[str, Any] = ( + {"billing_mode": "subscription"} if subscription else {} + ) response_model = request_model finish_reason = "stop" failure: str | None = None @@ -510,14 +763,18 @@ def _drain() -> None: }) elif etype == "response.completed": payload = event.get("response") or {} - usage = build_usage_dict(payload.get("usage")) + usage = build_usage_dict( + payload.get("usage"), subscription=subscription + ) response_model = str(payload.get("model") or response_model) elif etype == "response.incomplete": payload = event.get("response") or {} details = payload.get("incomplete_details") or {} if "max_output_tokens" in str(details.get("reason", "")): finish_reason = "max_tokens" - usage = build_usage_dict(payload.get("usage")) + usage = build_usage_dict( + payload.get("usage"), subscription=subscription + ) elif etype in ("response.failed", "error"): if etype == "error": failure = str(event.get("message") or event) diff --git a/src/providers/openai_responses.py b/src/providers/openai_responses.py index b5556448..cf203607 100644 --- a/src/providers/openai_responses.py +++ b/src/providers/openai_responses.py @@ -98,6 +98,69 @@ def strip_responses_item_blocks( return result +# Effort levels the OpenAI Responses API accepts. Mirrors OpenCode, which +# excludes "max" from the OpenAI effort type at compile time +# (packages/llm/src/protocols/utils/openai-options.ts:5-7: +# ``Exclude``) while keeping it in the cross-provider +# union. Confirmed live 2026-08-01 against gpt-5.6-luna: +# +# reasoning_effort='max' -> 400 invalid_request_error +# "does not support 'max' with this model. +# Supported values are: 'none', 'low', 'medium', 'high', and 'xhigh'." +# +# This is why "unknown params are ignored" does not apply here: an +# unsupported VALUE of a KNOWN parameter is a hard rejection, not a no-op. +# OpenRouter tolerates ``max`` for the same model, so the two providers +# genuinely differ and the clamp has to be provider-scoped. +OPENAI_REASONING_EFFORTS = ("none", "low", "medium", "high", "xhigh") + + +def supports_reasoning(model: str) -> bool: + """Whether ``model`` accepts a ``reasoning`` block on the Responses API. + + Sending one to a non-reasoning model is a hard 400 — verified live + 2026-08-01: ``gpt-4o`` and ``gpt-4o-mini`` return "Unsupported parameter: + 'reasoning.effort' is not supported with this model", while the same + request WITHOUT the block succeeds. So the Responses endpoint itself is + universal; only the reasoning block is gated. + + That distinction is what makes Responses safe as the default protocol for + every OpenAI model, which is the routing shape this mirrors (OpenCode's + provider facade returns ``model: responses``). + """ + m = (model or "").lower() + if "-chat" in m: + # gpt-5-chat-latest and friends are the NON-reasoning variants of a + # reasoning family, so the prefix alone would misclassify them. + # ``supports_verbosity`` below already carves out the same variants + # for the same reason (OpenCode transform.ts:1189-1196). Excluding + # them costs nothing if wrong — they fall to Chat Completions, the + # older and more exercised path — whereas including them wrongly is a + # hard 400 on every request. + return False + if m.startswith(("gpt-5", "o1", "o3", "o4")): + return True + return "codex" in m + + +def normalize_openai_effort(effort: str | None) -> str | None: + """Coerce a cross-provider effort level to one the OpenAI API accepts. + + ``max`` degrades to ``xhigh`` (the highest OpenAI level) rather than + erroring, matching how ``resolve_thinking_effort`` degrades an + unsupported ``xhigh`` to ``high`` on the Anthropic wire — a level the + provider cannot take should cost a notch of depth, not the whole run. + Unknown values return ``None`` so the caller omits the block and lets the + API apply its own default. + """ + value = (effort or "").strip().lower() + if not value: + return None + if value == "max": + return "xhigh" + return value if value in OPENAI_REASONING_EFFORTS else None + + def supports_verbosity(model: str) -> bool: """gpt-5.x general models accept ``text.verbosity``; codex/chat variants don't (OpenCode transform.ts:1189-1196).""" @@ -442,12 +505,21 @@ def convert_messages_to_responses_input( # --- responses -------------------------------------------------------------- -def build_usage_dict(usage: dict[str, Any] | None) -> dict[str, Any]: +def build_usage_dict( + usage: dict[str, Any] | None, *, subscription: bool = True +) -> dict[str, Any]: """Responses usage JSON → the ChatResponse usage shape. ``billing_mode: subscription`` zeroes the cost in ``record_api_usage`` (cost_tracker.py — the #697 mechanism); token counts still feed the context-left display. + + That zeroing is correct for a flat-rate ChatGPT plan and WRONG for an API + key, which is metered per token. This protocol now carries both, so the + flag has to follow the auth mode — otherwise every API-key request + reports $0.00 and ``/cost`` silently under-reports the whole session. + The default stays ``True`` because the subscription path was this + function's only caller when it was written. """ usage = usage or {} input_details = usage.get("input_tokens_details") or {} @@ -455,8 +527,9 @@ def build_usage_dict(usage: dict[str, Any] | None) -> dict[str, Any]: "input_tokens": int(usage.get("input_tokens", 0) or 0), "output_tokens": int(usage.get("output_tokens", 0) or 0), "total_tokens": int(usage.get("total_tokens", 0) or 0), - "billing_mode": "subscription", } + if subscription: + result["billing_mode"] = "subscription" cached = input_details.get("cached_tokens") if cached: result["cache_read_input_tokens"] = int(cached) diff --git a/tests/test_openai_provider_routing.py b/tests/test_openai_provider_routing.py new file mode 100644 index 00000000..cd72e4ee --- /dev/null +++ b/tests/test_openai_provider_routing.py @@ -0,0 +1,534 @@ +"""Protocol/route selection for the first-party OpenAI provider. + +The provider used to pick its protocol from the AUTH MODE: subscription meant +Responses, an API key meant Chat Completions. That conflated two independent +axes and made the API-key route unusable for agentic work, because + + /v1/chat/completions + tools + gpt-5.6-luna -> 400 + "Function tools with reasoning_effort are not supported ... use + /v1/responses or set reasoning_effort to 'none'." + +fires even with no effort set. These tests pin the corrected split: the +PROTOCOL follows the model's capability, while auth only decides the ROUTE +(endpoint + headers) and the effort ceiling. +""" + +from __future__ import annotations + +import time +from unittest.mock import patch + +from src.auth import openai_subscription as auth +from src.auth.openai_subscription import CODEX_API_ENDPOINT +from src.providers.openai_provider import OpenAIProvider +from src.providers.openai_responses import ( + normalize_openai_effort, + supports_reasoning, +) + +REASONING = ["gpt-5.6-luna", "gpt-5.5", "gpt-5", "o1-preview", "o3-mini", "o4-mini", + "gpt-5-codex", "codex-mini-latest"] +PLAIN = ["gpt-4o", "gpt-4o-mini", "gpt-4.1", "gpt-3.5-turbo", "chatgpt-4o-latest", + # the non-reasoning variants of a reasoning family — the prefix + # alone would misclassify these + "gpt-5-chat-latest", "gpt-5-chat"] + + +def _credentials() -> auth.SubscriptionCredentials: + return auth.SubscriptionCredentials( + "access", "refresh", time.time() + 3600, "acct-123", "idtok" + ) + + +# --- capability predicate ----------------------------------------------- + + +def test_supports_reasoning_splits_the_model_families() -> None: + for model in REASONING: + assert supports_reasoning(model) is True, model + for model in PLAIN: + assert supports_reasoning(model) is False, model + + +def test_supports_reasoning_tolerates_missing_and_odd_case() -> None: + assert supports_reasoning("") is False + assert supports_reasoning(None) is False # type: ignore[arg-type] + assert supports_reasoning("GPT-5.6-Luna") is True + + +# --- effort normalisation ----------------------------------------------- + + +def test_max_is_clamped_to_xhigh_for_openai() -> None: + """OpenAI's ladder tops out at xhigh; `max` is an Anthropic-only rung. + + Clamping rather than rejecting keeps `--effort max` portable across + providers, which is how the evals drive it. + """ + assert normalize_openai_effort("max") == "xhigh" + assert normalize_openai_effort("MAX") == "xhigh" + + +def test_known_efforts_pass_through_and_junk_is_dropped() -> None: + for effort in ("none", "low", "medium", "high", "xhigh"): + assert normalize_openai_effort(effort) == effort + for junk in ("bogus", "", " ", None): + assert normalize_openai_effort(junk) is None + + +# --- protocol selection -------------------------------------------------- + + +def test_api_key_route_uses_responses_for_reasoning_models() -> None: + for model in REASONING: + provider = OpenAIProvider(api_key="sk-test", model=model) + assert provider._use_responses() is True, model + + +def test_api_key_route_keeps_chat_completions_for_plain_models() -> None: + for model in PLAIN: + provider = OpenAIProvider(api_key="sk-test", model=model) + assert provider._use_responses() is False, model + + +def test_subscription_always_uses_responses() -> None: + """Auth still forces the protocol one way: Codex only speaks Responses.""" + provider = OpenAIProvider(api_key=None, model="gpt-4o") + with patch.object(provider, "_subscription_active", True): + assert provider._use_responses() is True + + +def test_per_call_model_override_beats_the_constructor() -> None: + provider = OpenAIProvider(api_key="sk-test", model="gpt-4o") + assert provider._use_responses() is False + assert provider._use_responses(model="gpt-5.6-luna") is True + + +# --- route selection (endpoint + headers) -------------------------------- + + +def _captured_request(provider: OpenAIProvider, **kwargs): + """Run one streaming request against a stubbed transport; return it.""" + seen = {} + + class _Resp: + status_code = 200 + + def iter_lines(self): + return iter(()) + + def close(self): + pass + + def read(self): + return b"" + + class _Client: + def build_request(self, method, url, headers=None, json=None): + seen.update(url=url, headers=headers or {}, body=json or {}) + return object() + + def send(self, request, stream=False): + return _Resp() + + def close(self): + pass + + with patch("httpx.Client", return_value=_Client()): + provider._subscription_stream_request( + [{"role": "user", "content": "hi"}], None, **kwargs + ) + return seen + + +def test_api_key_route_targets_the_public_responses_endpoint() -> None: + provider = OpenAIProvider(api_key="sk-test", model="gpt-5.6-luna") + seen = _captured_request(provider) + assert seen["url"] == "https://api.openai.com/v1/responses" + assert seen["headers"]["Authorization"] == "Bearer sk-test" + + +def test_endpoint_is_derived_from_an_explicit_base_url() -> None: + """Derived rather than hardcoded, so a trailing slash still resolves.""" + provider = OpenAIProvider( + api_key="sk-test", model="gpt-5.6-luna", base_url="https://api.openai.com/v1/" + ) + assert _captured_request(provider)["url"] == "https://api.openai.com/v1/responses" + + +def test_a_gateway_base_url_keeps_chat_completions() -> None: + """A proxy may not implement /responses; switching would 404 it. + + `providers.openai.base_url` is user-configurable, so this provider is + also the route to LiteLLM/vLLM/Azure-style proxies. Those speak Chat + Completions universally, and the 400s motivating the Responses route are + api.openai.com's own behaviour, not necessarily theirs. + """ + for base in ( + "https://gw.example/v1", + "http://localhost:4000/v1", + "https://x.openai.azure.com/openai/deployments/d1", + ): + provider = OpenAIProvider( + api_key="sk-test", model="gpt-5.6-luna", base_url=base + ) + assert provider._use_responses() is False, base + + +def test_the_subscription_route_ignores_a_gateway_base_url() -> None: + """Codex speaks only Responses, and always at its own endpoint.""" + provider = OpenAIProvider(api_key=None, model="gpt-5.6-luna", + base_url="https://gw.example/v1") + with patch.object(provider, "_subscription_active", True): + assert provider._use_responses() is True + + +def test_subscription_route_targets_the_codex_backend() -> None: + provider = OpenAIProvider(api_key=None, model="gpt-5.6-luna") + with patch.object(provider, "_subscription_active", True), patch( + "src.auth.openai_subscription.get_valid_credentials", + return_value=_credentials(), + ): + seen = _captured_request(provider) + assert seen["url"] == CODEX_API_ENDPOINT + assert seen["headers"]["Authorization"] == "Bearer access" + + +# --- reasoning payload gating -------------------------------------------- + + +def test_reasoning_is_sent_only_to_models_that_have_it() -> None: + """`reasoning.effort` on a plain model is a hard 400, so it must be absent.""" + provider = OpenAIProvider(api_key="sk-test", model="gpt-5.6-luna") + body = provider._subscription_request_body( + [{"role": "user", "content": "hi"}], None, + extra_body={"reasoning_effort": "xhigh"}, + ) + assert body["reasoning"]["effort"] == "xhigh" + + plain = OpenAIProvider(api_key="sk-test", model="gpt-4o") + body = plain._subscription_request_body( + [{"role": "user", "content": "hi"}], None, + extra_body={"reasoning_effort": "xhigh"}, + ) + assert "reasoning" not in body + + +def test_max_reaches_the_wire_as_xhigh_on_the_api_key_route() -> None: + provider = OpenAIProvider(api_key="sk-test", model="gpt-5.6-luna") + body = provider._subscription_request_body( + [{"role": "user", "content": "hi"}], None, + extra_body={"reasoning_effort": "max"}, + ) + assert body["reasoning"]["effort"] == "xhigh" + + +# --- Chat Completions fallback sanitising -------------------------------- + + +def test_effort_is_stripped_before_a_plain_model_hits_chat_completions() -> None: + """`--provider openai --model gpt-4o --effort high` used to 400 on call 1.""" + provider = OpenAIProvider(api_key="sk-test", model="gpt-4o") + cleaned = provider._without_unsupported_reasoning( + {"extra_body": {"reasoning_effort": "high"}} + ) + assert "reasoning_effort" not in cleaned["extra_body"] + + +def test_stripping_preserves_other_extra_body_keys_and_the_original() -> None: + provider = OpenAIProvider(api_key="sk-test", model="gpt-4o") + original = {"extra_body": {"reasoning_effort": "high", "user": "u1"}} + cleaned = provider._without_unsupported_reasoning(original) + assert cleaned["extra_body"] == {"user": "u1"} + # the caller's dict is not mutated out from under it + assert original["extra_body"]["reasoning_effort"] == "high" + + +def test_reasoning_models_keep_their_effort_untouched() -> None: + provider = OpenAIProvider(api_key="sk-test", model="gpt-5.6-luna") + kwargs = {"extra_body": {"reasoning_effort": "xhigh"}} + assert provider._without_unsupported_reasoning(kwargs)["extra_body"] == { + "reasoning_effort": "xhigh" + } + + +def test_kwargs_without_effort_are_passed_through_unchanged() -> None: + provider = OpenAIProvider(api_key="sk-test", model="gpt-4o") + kwargs = {"temperature": 0.5} + assert provider._without_unsupported_reasoning(kwargs) is kwargs + + +# --- 401 handling -------------------------------------------------------- + + +def test_api_key_401_reports_the_provider_error_not_a_login_prompt() -> None: + """A rejected key is not an expired OAuth token; refreshing would mislead.""" + + class _Resp: + status_code = 401 + + def close(self): + pass + + def read(self): + return b'{"error":{"message":"Incorrect API key provided"}}' + + class _Client: + def build_request(self, *a, **k): + return object() + + def send(self, request, stream=False): + return _Resp() + + def close(self): + pass + + provider = OpenAIProvider(api_key="sk-bad", model="gpt-5.6-luna") + with patch("httpx.Client", return_value=_Client()), patch( + "src.auth.openai_subscription.force_refresh" + ) as refresh: + try: + provider._subscription_stream_request( + [{"role": "user", "content": "hi"}], None + ) + except RuntimeError as exc: + message = str(exc) + else: # pragma: no cover - the stub always fails + raise AssertionError("expected the 401 to surface") + + refresh.assert_not_called() + assert "Incorrect API key provided" in message + assert "login expired" not in message + + +# --- transport-drop retry ------------------------------------------------ + + +def test_responses_path_retries_a_dropped_stream() -> None: + """The Responses path must sit INSIDE the base class's retry loop. + + An un-retried "peer closed connection" ended 8 of 89 terminal-bench 2.1 + trials (see `OpenAICompatibleProvider.chat_stream_response`). Dispatching + from `chat_stream_response` instead of `_stream_attempt` would opt every + Responses request out of that loop without failing any other test. + """ + import httpx + + provider = OpenAIProvider(api_key="sk-test", model="gpt-5.6-luna") + calls = [] + + def _attempt(*args, **kwargs): + calls.append(1) + if len(calls) == 1: + raise httpx.RemoteProtocolError( + "peer closed connection without sending complete message body" + ) + return "recovered" + + with patch.object(provider, "_subscription_stream_request", _attempt): + result = provider.chat_stream_response([{"role": "user", "content": "hi"}]) + + assert result == "recovered" + assert len(calls) == 2, "the dropped stream was not re-issued" + + +def test_a_server_verdict_is_not_retried_on_the_responses_path() -> None: + """A 4xx is a decision, not a drop — re-issuing would double-bill it.""" + provider = OpenAIProvider(api_key="sk-test", model="gpt-5.6-luna") + calls = [] + + def _attempt(*args, **kwargs): + calls.append(1) + raise RuntimeError("OpenAI API error (400): bad request") + + with patch.object(provider, "_subscription_stream_request", _attempt): + try: + provider.chat_stream_response([{"role": "user", "content": "hi"}]) + except RuntimeError: + pass + + assert len(calls) == 1, "a 400 must not be re-issued" + + +# --- cost accounting ----------------------------------------------------- + + +def test_api_key_usage_is_billed_and_subscription_usage_is_not() -> None: + """`billing_mode` follows the AUTH mode, not the protocol. + + `cost_tracker.record_api_usage` zeroes the cost of any usage marked + `billing_mode: subscription`. That is right for a flat-rate ChatGPT plan + and wrong for a metered API key — and this protocol now carries both, so + routing API-key traffic here without splitting the flag would report + every OpenAI request as $0.00. + """ + from src.providers.openai_responses import build_usage_dict + + raw = {"input_tokens": 100, "output_tokens": 50, "total_tokens": 150} + assert build_usage_dict(raw)["billing_mode"] == "subscription" + assert "billing_mode" not in build_usage_dict(raw, subscription=False) + + +def test_billed_usage_produces_a_non_zero_cost() -> None: + """End of the chain: the flag actually changes what `/cost` records.""" + from src.cost_tracker import compute_cost + from src.providers.openai_responses import build_usage_dict + + billed = build_usage_dict( + {"input_tokens": 100_000, "output_tokens": 50_000}, subscription=False + ) + assert billed.get("billing_mode") != "subscription" + # a metered key must be able to produce real cost; a zero here would mean + # the usage shape never reaches the pricing table + assert compute_cost("gpt-5.6-luna", billed) > 0 + + +def test_token_counts_survive_either_billing_mode() -> None: + """Cost is separate from the context-left display, which needs the counts.""" + raw = {"input_tokens": 7, "output_tokens": 3, "total_tokens": 10} + for kwargs in ({}, {"subscription": False}): + from src.providers.openai_responses import build_usage_dict + + usage = build_usage_dict(raw, **kwargs) + assert usage["input_tokens"] == 7 + assert usage["output_tokens"] == 3 + + +# --- $OPENAI_BASE_URL ---------------------------------------------------- + + +def test_env_base_url_keeps_reasoning_models_on_chat_completions(monkeypatch) -> None: + """`self.base_url` is not the only channel — the SDK reads the env var. + + `set_api_key` writes the config key only when one is passed, so + `base_url=None` with `$OPENAI_BASE_URL` exported is an ordinary state. + Reading only the attribute would send Chat Completions traffic to the + proxy while the default model went straight to api.openai.com, carrying + the key and the conversation around a configured egress path. + """ + monkeypatch.setenv("OPENAI_BASE_URL", "https://llm.corp.internal/v1") + provider = OpenAIProvider(api_key="sk-test", model="gpt-5.4") + assert provider._is_first_party_base_url() is False + assert provider._use_responses() is False + + +def test_env_base_url_pointing_at_openai_still_uses_responses(monkeypatch) -> None: + monkeypatch.setenv("OPENAI_BASE_URL", "https://api.openai.com/v1") + provider = OpenAIProvider(api_key="sk-test", model="gpt-5.4") + assert provider._use_responses() is True + + +def test_an_explicit_base_url_beats_the_env_var(monkeypatch) -> None: + monkeypatch.setenv("OPENAI_BASE_URL", "https://llm.corp.internal/v1") + provider = OpenAIProvider( + api_key="sk-test", model="gpt-5.4", base_url="https://api.openai.com/v1" + ) + assert provider._use_responses() is True + + +# --- max_output_tokens --------------------------------------------------- + + +def test_max_tokens_reaches_the_wire_on_the_api_key_route() -> None: + """Callers pass `max_tokens` as a BOUND, not a preference. + + Compaction summaries, the permission classifier, the /goal judge and the + advisor all set one. Dropping it silently unbounds them — and query.py's + `max_output_tokens_escalate` lane re-issues with a larger value after a + truncated reply, which without this is byte-identical to the request that + just truncated. + """ + provider = OpenAIProvider(api_key="sk-test", model="gpt-5.6-luna") + body = provider._subscription_request_body( + [{"role": "user", "content": "hi"}], None, max_tokens=64_000 + ) + assert body["max_output_tokens"] == 64_000 + + +def test_the_subscription_route_still_omits_max_tokens() -> None: + """The Codex backend rejects it — that carve-out is route-specific.""" + provider = OpenAIProvider(api_key=None, model="gpt-5.6-luna") + with patch.object(provider, "_subscription_active", True): + body = provider._subscription_request_body( + [{"role": "user", "content": "hi"}], None, max_tokens=64_000 + ) + assert "max_output_tokens" not in body + + +# --- error classification ------------------------------------------------ + + +def test_a_429_is_classified_as_retryable_not_as_a_generic_failure() -> None: + """The retry layer classifies by ATTRIBUTE, not by message text. + + A bare RuntimeError carries no `status_code`, so a 429 read as a + permanent failure: the request was abandoned rather than backed off, + and the server's `Retry-After` was discarded. + """ + from src.providers.openai_provider import ResponsesHTTPError + from src.services.api.errors import is_overloaded_error, is_rate_limit_error + + assert is_rate_limit_error( + ResponsesHTTPError("OpenAI API error (429): slow down", status_code=429) + ) + assert is_overloaded_error( + ResponsesHTTPError("OpenAI API error (529): busy", status_code=529) + ) + # the shape it replaced classified as neither + assert not is_rate_limit_error(RuntimeError("OpenAI API error (429): slow down")) + + +def test_the_http_error_stays_catchable_as_a_runtime_error() -> None: + """Existing `except RuntimeError` handlers must keep working.""" + from src.providers.openai_provider import ResponsesHTTPError + + assert isinstance( + ResponsesHTTPError("x", status_code=500), RuntimeError + ) + + +def test_retry_after_is_reachable_from_the_error() -> None: + """`_retry_after_seconds` reads `e.response.headers`.""" + from src.query.query import _retry_after_seconds + from src.providers.openai_provider import ResponsesHTTPError + + class _Resp: + headers = {"retry-after": "7"} + + err = ResponsesHTTPError("429", status_code=429, response=_Resp()) + assert _retry_after_seconds(err, default=1.0) == 7.0 + + +# --- OAuth eligibility --------------------------------------------------- + + +def test_a_proxy_in_the_env_disables_the_oauth_fallback(monkeypatch) -> None: + """An OAuth session cannot be proxied, so it must not silently activate. + + `oauth_eligible` derived the first-party rule a second time from the + constructor PARAMETER, so it never learned about `$OPENAI_BASE_URL`. + With a proxy in the env, no API key and a stored ChatGPT login, prompts + went to chatgpt.com with no error — the outcome the guard exists to + prevent. It now shares `_is_first_party_base_url`. + """ + monkeypatch.setenv("OPENAI_BASE_URL", "https://llm.corp.internal/v1") + creds = _credentials() + with patch.object(auth, "load_credentials", return_value=creds): + provider = OpenAIProvider(api_key=None, model="gpt-5.4") + assert provider._subscription_active is False + + +def test_the_oauth_fallback_still_activates_without_a_proxy(monkeypatch) -> None: + monkeypatch.delenv("OPENAI_BASE_URL", raising=False) + creds = _credentials() + with patch.object(auth, "load_credentials", return_value=creds): + provider = OpenAIProvider(api_key=None, model="gpt-5.4") + assert provider._subscription_active is True + + +def test_an_api_key_still_beats_a_stored_login(monkeypatch) -> None: + monkeypatch.delenv("OPENAI_BASE_URL", raising=False) + creds = _credentials() + with patch.object(auth, "load_credentials", return_value=creds): + provider = OpenAIProvider(api_key="sk-test", model="gpt-5.4") + assert provider._subscription_active is False diff --git a/tests/test_providers.py b/tests/test_providers.py index 0a0d9f8a..00776e47 100644 --- a/tests/test_providers.py +++ b/tests/test_providers.py @@ -408,7 +408,11 @@ def test_chat(self, mock_openai): mock_openai.return_value = mock_client # Test - provider = OpenAIProvider(api_key="test_key") + # gpt-4o pins this to Chat Completions: the mocks below are Chat + # Completions chunk shapes, and reasoning models now route to the + # Responses protocol instead. The path under test is still live for + # non-reasoning models. + provider = OpenAIProvider(api_key="test_key", model="gpt-4o") messages = [ChatMessage(role="user", content="Hi")] response = provider.chat(messages) @@ -432,7 +436,11 @@ def test_chat_accepts_dict_messages(self, mock_openai): mock_client.with_options.return_value = mock_client # see _apply_client_timeout mock_openai.return_value = mock_client - provider = OpenAIProvider(api_key="test_key") + # gpt-4o pins this to Chat Completions: the mocks below are Chat + # Completions chunk shapes, and reasoning models now route to the + # Responses protocol instead. The path under test is still live for + # non-reasoning models. + provider = OpenAIProvider(api_key="test_key", model="gpt-4o") messages = [{"role": "user", "content": "Hi"}] response = provider.chat(messages) @@ -476,7 +484,11 @@ def test_chat_stream_response_rebuilds_tool_calls(self, mock_openai): mock_client.with_options.return_value = mock_client # see _apply_client_timeout mock_openai.return_value = mock_client - provider = OpenAIProvider(api_key="test_key") + # gpt-4o pins this to Chat Completions: the mocks below are Chat + # Completions chunk shapes, and reasoning models now route to the + # Responses protocol instead. The path under test is still live for + # non-reasoning models. + provider = OpenAIProvider(api_key="test_key", model="gpt-4o") chunks: list[str] = [] response = provider.chat_stream_response( [ChatMessage(role="user", content="Hi")], diff --git a/tests/test_tool_arg_recovery.py b/tests/test_tool_arg_recovery.py index 02e716d2..5699194f 100644 --- a/tests/test_tool_arg_recovery.py +++ b/tests/test_tool_arg_recovery.py @@ -109,7 +109,10 @@ def test_stream_response_recovers_truncated_tool_args(mock_openai): mock_client.with_options.return_value = mock_client # see _apply_client_timeout mock_openai.return_value = mock_client - provider = OpenAIProvider(api_key="test_key") + # gpt-4o pins this to Chat Completions: the truncated-tool-args recovery + # under test is that protocol's delta reconstruction, and reasoning + # models now route to the Responses protocol instead. + provider = OpenAIProvider(api_key="test_key", model="gpt-4o") resp = provider.chat_stream_response( [ChatMessage(role="user", content="Hi")], tools=[{"name": "Read", "description": "", "input_schema": {"type": "object"}}],