mirror of
https://github.com/crewAIInc/crewAI.git
synced 2026-07-27 17:49:22 +00:00
fix(openai): resolve the cache endpoint from env, and defer the 404 to the retry
Two more from CodeRabbit review, both verified against the code before fixing. 1. The cache key ignored OPENAI_BASE_URL / OPENAI_API_BASE. `_get_client_params` resolves the endpoint as base_url or api_base or OPENAI_BASE_URL or OPENAI_API_BASE, but `_model_cache_key` stopped at the two fields. An instance pointed at a proxy purely through the environment was therefore keyed as https://api.openai.com/v1 -- reintroducing exactly the cross-endpoint leakage the tuple key was added to prevent. Both now derive from a single `_resolved_base_url()`, so the client config and the cache keys can't disagree about what "endpoint" means. OPENAI_BASE_URL=https://proxy.internal/v1 -> ('https://proxy.internal/v1', 'gpt-5-pro') (was api.openai.com) base_url= explicit -> wins over env neither set -> api.openai.com 2. The responses-only branch of `_is_recoverable_completion_error` was dead. NotFoundError is caught by an earlier handler that logs, emits _emit_call_failed_event and re-raises as ValueError, so the 404 half never reached the generic guard. That both reported a failure the user never experiences and wrapped away the NotFoundError the retry needs to recognize. The same guard now sits in both NotFoundError handlers. Verified live -- a pro model still recovers, and the spurious event is gone: pro model -> 'PONG' learned -> {('https://api.openai.com/v1', 'gpt-5-pro')} spurious LLMCallFailedEvent -> 0 (previously fired) spurious failure logs -> 0 Tests: 6 new cases for endpoint resolution (env proxy, explicit override, default, and that a proxy doesn't inherit OpenAI's learning) and for the NotFoundError handler deferring to the retry without emitting a failure event. 1049 passed across tests/llms, tests/agents and test_agent_utils (1 pre-existing unrelated failure from a local OLLAMA_API_KEY env leak). Ruff + mypy clean.
This commit is contained in:
@@ -431,11 +431,7 @@ class OpenAICompletion(BaseLLM):
|
||||
"api_key": self.api_key,
|
||||
"organization": self.organization,
|
||||
"project": self.project,
|
||||
"base_url": self.base_url
|
||||
or self.api_base
|
||||
or os.getenv("OPENAI_BASE_URL")
|
||||
or os.getenv("OPENAI_API_BASE")
|
||||
or None,
|
||||
"base_url": self._resolved_base_url(),
|
||||
"timeout": self.timeout,
|
||||
"max_retries": self.max_retries,
|
||||
"default_headers": self.default_headers,
|
||||
@@ -1813,6 +1809,22 @@ class OpenAICompletion(BaseLLM):
|
||||
)
|
||||
return param, message.lower()
|
||||
|
||||
def _resolved_base_url(self) -> str | None:
|
||||
"""The endpoint this instance actually talks to, or None for OpenAI's own.
|
||||
|
||||
Environment variables count: an instance can be pointed at a proxy without
|
||||
either field being set. Single source of truth for both the client config
|
||||
and the learned-behaviour cache keys, which must agree on what "endpoint"
|
||||
means or a proxy ends up sharing api.openai.com's cache.
|
||||
"""
|
||||
return (
|
||||
self.base_url
|
||||
or self.api_base
|
||||
or os.getenv("OPENAI_BASE_URL")
|
||||
or os.getenv("OPENAI_API_BASE")
|
||||
or None
|
||||
)
|
||||
|
||||
def _model_cache_key(self, model: str | None = None) -> tuple[str, str]:
|
||||
"""Identity for the learned-behaviour caches: the endpoint plus the model.
|
||||
|
||||
@@ -1820,8 +1832,10 @@ class OpenAICompletion(BaseLLM):
|
||||
OpenAI-compatible proxy with different capabilities, so a conflict learned
|
||||
against one endpoint must not silently apply to the other.
|
||||
"""
|
||||
endpoint = self.base_url or self.api_base or "https://api.openai.com/v1"
|
||||
return endpoint, model if model is not None else self.model
|
||||
return (
|
||||
self._resolved_base_url() or "https://api.openai.com/v1",
|
||||
model if model is not None else self.model,
|
||||
)
|
||||
|
||||
def _is_recoverable_completion_error(self, error: BaseException) -> bool:
|
||||
"""Whether ``_call_completions`` will retry instead of surfacing this error.
|
||||
@@ -2130,6 +2144,12 @@ class OpenAICompletion(BaseLLM):
|
||||
params["messages"], content, from_agent
|
||||
)
|
||||
except NotFoundError as e:
|
||||
# `_call_completions` retries a responses-only model on the Responses
|
||||
# API. Reporting a failed call here would surface an error the user
|
||||
# never experiences, and the ValueError wrapper would also hide the
|
||||
# NotFoundError that the retry needs in order to recognize it.
|
||||
if self._is_recoverable_completion_error(e):
|
||||
raise
|
||||
error_msg = self._model_not_found_message(e)
|
||||
logging.error(error_msg)
|
||||
self._emit_call_failed_event(
|
||||
@@ -2559,6 +2579,12 @@ class OpenAICompletion(BaseLLM):
|
||||
if usage.get("total_tokens", 0) > 0:
|
||||
logging.info(f"OpenAI API usage: {usage}")
|
||||
except NotFoundError as e:
|
||||
# `_call_completions` retries a responses-only model on the Responses
|
||||
# API. Reporting a failed call here would surface an error the user
|
||||
# never experiences, and the ValueError wrapper would also hide the
|
||||
# NotFoundError that the retry needs in order to recognize it.
|
||||
if self._is_recoverable_completion_error(e):
|
||||
raise
|
||||
error_msg = self._model_not_found_message(e)
|
||||
logging.error(error_msg)
|
||||
self._emit_call_failed_event(
|
||||
|
||||
@@ -14,6 +14,8 @@ retried on the Responses API, then the model is remembered so the wasted round t
|
||||
is paid once per process.
|
||||
"""
|
||||
|
||||
from types import SimpleNamespace
|
||||
|
||||
import httpx
|
||||
import pytest
|
||||
from openai import NotFoundError
|
||||
@@ -206,6 +208,85 @@ class TestEffectiveApi:
|
||||
assert proxy._effective_api() == "completions"
|
||||
|
||||
|
||||
class TestEndpointResolution:
|
||||
"""The cache key must agree with the endpoint the client actually uses."""
|
||||
|
||||
def test_env_configured_proxy_gets_its_own_key(self, monkeypatch):
|
||||
"""An instance can be pointed at a proxy purely through the environment.
|
||||
|
||||
`_get_client_params` honours OPENAI_BASE_URL, so a key that stopped at the
|
||||
base_url/api_base fields would file a proxy under api.openai.com -- the
|
||||
exact leakage the tuple key exists to prevent.
|
||||
"""
|
||||
monkeypatch.setenv("OPENAI_BASE_URL", "https://proxy.internal/v1")
|
||||
llm = build("gpt-5-pro")
|
||||
|
||||
assert llm._model_cache_key() == ("https://proxy.internal/v1", "gpt-5-pro")
|
||||
assert llm._get_client_params()["base_url"] == "https://proxy.internal/v1"
|
||||
|
||||
def test_explicit_base_url_wins_over_env(self, monkeypatch):
|
||||
monkeypatch.setenv("OPENAI_BASE_URL", "https://proxy.internal/v1")
|
||||
llm = build("gpt-5-pro", base_url="https://explicit.internal/v1")
|
||||
|
||||
assert llm._model_cache_key()[0] == "https://explicit.internal/v1"
|
||||
|
||||
def test_defaults_to_openai(self, monkeypatch):
|
||||
monkeypatch.delenv("OPENAI_BASE_URL", raising=False)
|
||||
monkeypatch.delenv("OPENAI_API_BASE", raising=False)
|
||||
|
||||
assert build("gpt-5-pro")._model_cache_key() == (
|
||||
"https://api.openai.com/v1",
|
||||
"gpt-5-pro",
|
||||
)
|
||||
|
||||
def test_env_proxy_does_not_inherit_openai_learning(self, monkeypatch):
|
||||
monkeypatch.delenv("OPENAI_BASE_URL", raising=False)
|
||||
monkeypatch.delenv("OPENAI_API_BASE", raising=False)
|
||||
real = build("gpt-5-pro")
|
||||
real._remember_responses_only_model()
|
||||
assert real._effective_api() == "responses"
|
||||
|
||||
# Same model string, different endpoint -> must not inherit the learning.
|
||||
monkeypatch.setenv("OPENAI_BASE_URL", "https://proxy.internal/v1")
|
||||
assert build("gpt-5-pro")._effective_api() == "completions"
|
||||
|
||||
|
||||
class TestFailureReportingSuppressed:
|
||||
"""A recovered 404 must not surface as a failed call."""
|
||||
|
||||
def test_not_found_handler_defers_to_the_retry(self, monkeypatch):
|
||||
"""NotFoundError is caught before the generic handler.
|
||||
|
||||
Without a guard there it logs, emits LLMCallFailedEvent, and re-raises as
|
||||
ValueError -- which both reports a failure the user never sees and hides
|
||||
the NotFoundError the retry needs to recognize.
|
||||
"""
|
||||
llm = build("gpt-5-pro")
|
||||
emitted: list[str] = []
|
||||
monkeypatch.setattr(
|
||||
llm,
|
||||
"_emit_call_failed_event",
|
||||
lambda **kwargs: emitted.append(kwargs.get("error", "")),
|
||||
)
|
||||
error = make_not_found(RESPONSES_ONLY_MESSAGES[0])
|
||||
|
||||
class FakeCompletions:
|
||||
def create(self, **kwargs):
|
||||
raise error
|
||||
|
||||
monkeypatch.setattr(
|
||||
llm,
|
||||
"_get_sync_client",
|
||||
lambda: SimpleNamespace(chat=SimpleNamespace(completions=FakeCompletions())),
|
||||
)
|
||||
|
||||
# The original NotFoundError must survive, not a ValueError wrapper.
|
||||
with pytest.raises(NotFoundError):
|
||||
llm._handle_completion({"model": "gpt-5-pro", "messages": MESSAGES})
|
||||
|
||||
assert emitted == []
|
||||
|
||||
|
||||
class TestNotFoundMessage:
|
||||
@pytest.mark.parametrize("message", RESPONSES_ONLY_MESSAGES)
|
||||
def test_points_at_responses_api(self, message: str):
|
||||
|
||||
Reference in New Issue
Block a user