-
Notifications
You must be signed in to change notification settings - Fork 119
fix(auth): refresh provider credentials per request (ENG-2116) #421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |
| import json | ||
| import logging | ||
| import os | ||
| from collections.abc import AsyncIterator | ||
| from collections.abc import AsyncIterator, Awaitable, Callable | ||
| from typing import NoReturn | ||
|
|
||
| import openai | ||
|
|
@@ -19,6 +19,7 @@ | |
| LLMProvider, | ||
| LLMResponse, | ||
| ModelUnavailableError, | ||
| ProviderAuthError, | ||
| ProviderConnectionInfo, | ||
| StreamComplete, | ||
| StreamEvent, | ||
|
|
@@ -42,6 +43,8 @@ | |
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| AsyncAPIKeyProvider = Callable[[], Awaitable[str]] | ||
|
|
||
|
|
||
| def _raise_for_status_error(exc: "openai.APIStatusError", model: str) -> NoReturn: | ||
| """Map a provider HTTP error onto anton's typed/curated exceptions. | ||
|
|
@@ -51,8 +54,7 @@ def _raise_for_status_error(exc: "openai.APIStatusError", model: str) -> NoRetur | |
| previous four copy-pasted blocks had already diverged in wording. | ||
|
|
||
| Mapping policy: | ||
| - 401 → ConnectionError with the invalid-key copy (cowork-server's | ||
| provider_auth detection keys on this exact phrase). | ||
| - 401 → ProviderAuthError with the invalid-key copy. | ||
| - 403 with a structured gateway code (``model_access_denied`` / | ||
| ``model_disabled``) → ModelUnavailableError carrying the code + | ||
| model, with actionable copy. Detection is code-exact on purpose: | ||
|
|
@@ -74,8 +76,8 @@ def _raise_for_status_error(exc: "openai.APIStatusError", model: str) -> NoRetur | |
| ``code`` sits at top level. The gateway's 429 is FastAPI-style | ||
| (``{"detail": …}``, no envelope) and passes through untouched. Both | ||
| fields are therefore read from the top level first, with an envelope | ||
| fallback for clients that deliver the wire shape unmodified (anton's | ||
| pyproject allows ``openai>=1.0``, and proxies exist that re-wrap). | ||
| fallback for clients that deliver the wire shape unmodified (anton requires | ||
| ``openai>=2.21.0``, and proxies exist that re-wrap). | ||
| The originally shipped ENG-598 mapper read only | ||
| ``body["error"]["code"]`` — a key the SDK had already peeled off — | ||
| which left the model-403 card dead in production. | ||
|
|
@@ -93,7 +95,7 @@ def _raise_for_status_error(exc: "openai.APIStatusError", model: str) -> NoRetur | |
| _foreign = origin_is_known_third_party(exc) | ||
|
|
||
| if exc.status_code == 401: | ||
| raise ConnectionError( | ||
| raise ProviderAuthError( | ||
| "Invalid API key — check your OpenAI API key configuration." | ||
| ) from exc | ||
|
|
||
|
|
@@ -827,7 +829,12 @@ def __init__( | |
| vision_format: str = "openai", | ||
| flavor: str = FLAVOR_OPENAI_COMPATIBLE_GENERIC, | ||
| reasoning_effort: str | None = None, | ||
| api_key_provider: AsyncAPIKeyProvider | None = None, | ||
| ) -> None: | ||
| # Keep the construction-time value separate from the live supplier. | ||
| # ChatSession passes export_connection_info() into ScratchpadManager, | ||
| # whose subprocess/env boundary cannot carry a callable. The main-process | ||
| # async OpenAI client can ask the supplier for a fresh bearer per request. | ||
| self._api_key = api_key | ||
| self._base_url = base_url | ||
| self._ssl_verify = ssl_verify | ||
|
|
@@ -860,21 +867,35 @@ def __init__( | |
|
|
||
| import httpx | ||
|
|
||
| client_api_key = api_key_provider if api_key_provider is not None else api_key | ||
| if api_version and _is_azure_endpoint(base_url): | ||
| # Azure OpenAI: use the dedicated client which handles deployment | ||
| # URL construction and api-version automatically. | ||
| if api_key_provider is not None: | ||
| # AsyncAzureOpenAI._prepare_options fully overrides the base | ||
| # hook and never chains to super(), so AsyncOpenAI's | ||
| # _refresh_api_key never runs and the supplier is never | ||
| # awaited. The base __init__ has already replaced a callable | ||
| # api_key with "", so the client would send an empty api-key | ||
| # header on every request and 401 forever. Refuse at | ||
| # construction instead of failing on the first call. | ||
| raise EndpointConfigurationError( | ||
| "Azure OpenAI cannot refresh credentials per request: " | ||
| "AsyncAzureOpenAI ignores a callable api_key. Pass a " | ||
| "static api_key for Azure endpoints." | ||
| ) | ||
|
Comment on lines
+870
to
+886
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, and fixed in 303bf7c on the main-targeted hotfix (anton#429). The guard now keys on the resolved value, |
||
| azure_kwargs: dict = {"api_version": api_version} | ||
| if api_key: | ||
| azure_kwargs["api_key"] = api_key | ||
| if client_api_key: | ||
| azure_kwargs["api_key"] = client_api_key | ||
| if base_url: | ||
| azure_kwargs["azure_endpoint"] = base_url | ||
| if not ssl_verify: | ||
| azure_kwargs["http_client"] = httpx.AsyncClient(verify=False) | ||
| self._client = AsyncAzureOpenAI(**azure_kwargs) | ||
| else: | ||
| kwargs: dict = {} | ||
| if api_key: | ||
| kwargs["api_key"] = api_key | ||
| if client_api_key: | ||
| kwargs["api_key"] = client_api_key | ||
| if base_url: | ||
| kwargs["base_url"] = base_url | ||
| if not ssl_verify: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -482,6 +482,21 @@ class TokenLimitExceeded(Exception): | |
| """Raised when the LLM returns 429 due to billing/token limits.""" | ||
|
|
||
|
|
||
| class ProviderAuthError(ConnectionError): | ||
| """Raised when a provider rejects its credential with HTTP 401. | ||
|
|
||
| The type distinguishes an authentication refusal from unrelated | ||
| ``ConnectionError`` failures. It remains a ``ConnectionError`` subclass so | ||
| in-process callers written against the previous 401 mapping keep working. | ||
| The client stamps ``role`` on a confirmed refusal so hosts can attribute | ||
| the recovery action to the provider that actually failed. | ||
|
Comment on lines
+488
to
+492
|
||
| """ | ||
|
Comment on lines
+485
to
+493
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 1e368f16 — the ProviderAuthError docstring now defines terminal refusal as either failed confirmation or the first refusal after streaming output makes replay unsafe. |
||
|
|
||
| def __init__(self, message: str, *, role: str | None = None): | ||
| super().__init__(message) | ||
| self.role = role | ||
|
|
||
|
|
||
| class StructuredOutputError(ValueError): | ||
| """Raised when a forced-tool-call structured-output call yields no usable call. | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.