andre/crewAI
1
0
Fork 0
mirror of https://github.com/crewAIInc/crewAI.git synced 2026-07-27 17:49:22 +00:00
Code Issues Actions 6 Packages Projects Releases Wiki Activity

fix(openai): address CodeRabbit review on the reasoning_effort retry

Browse Source 
Three defects, two raised in review and one found while verifying them.

1. Learned-behaviour caches ignored the endpoint (CodeRabbit).
   Both _LEARNED_TOOLS_REASONING_EFFORT_CONFLICTS and, from #6656,
   _LEARNED_RESPONSES_ONLY_MODELS were keyed on the model name alone. Two
   instances using the same model string against api.openai.com and an
   OpenAI-compatible proxy shared the cache, so one endpoint's 400 silently
   degraded the other. Both are now keyed by (endpoint, model) via
   _model_cache_key().

2. A recovered call still reported a failure (CodeRabbit).
   _handle_completion / _ahandle_completion log "OpenAI API call failed" and emit
   LLMCallFailedEvent for anything they catch, including the errors the retry
   wrappers exist to recover from. That printed a user-visible "LLM Error" panel
   for a call that then succeeded. Both handlers now re-raise early for errors
   _call_completions will retry, via _is_recoverable_completion_error() so the
   suppressing and retrying sides can't drift apart.

3. Dropping reasoning_effort didn't actually fix the request.
   Measured against the live endpoint while testing the above:

       gpt-5.6-sol  tools, no reasoning_effort   -> 400 (same error)
       gpt-5.6-sol  tools, reasoning_effort=none -> OK
       gpt-5.5      tools, no reasoning_effort   -> OK
       gpt-5.4      tools, no reasoning_effort   -> OK

   gpt-5.6-* apply a server-side default, so omitting the parameter fails exactly
   like sending "high". The retry and the fast path now send an explicit "none"
   instead of removing the key. Without this the whole retry was a no-op for the
   family that prompted the fix.

Also made error parsing tolerant of both body shapes the SDK produces: the live
BadRequestError carries "message"/"param" at the top level of .body, while a
synthetic one nests them under "error". The original code only read the nested
form, so detection returned False against real API errors and the retry never
fired. Shared _error_param_and_message() now handles both.

Verified live -- real Agent + Task + Crew().kickoff() on gpt-5.6-sol with the fast
path disabled so the runtime retry is exercised:

    AGENT RESULT: 391
    tool calls  : [(17, 23)]
    spurious LLMCallFailedEvent: 0
    spurious failure ERROR logs: 0

Tests: 153 passed, 1 skipped in tests/llms/openai. New cases for endpoint-scoped
learning on both caches, recoverable-error classification, and the forced-"none"
behaviour with the measured API results recorded. Ruff + mypy clean.
This commit is contained in:
alex-clawd
2026-07-26 02:21:58 -07:00
parent 4f455a3511
commit 52ab798410
3 changed files with 169 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files

108
lib/crewai/src/crewai/llms/providers/openai/completion.py
View File

@@ -62,7 +62,10 @@ if TYPE_CHECKING:
# the model string as configured. Populated from the 404 by
# `_remember_responses_only_model` so the wasted round trip is paid once per model
# per process rather than on every call.
_LEARNED_RESPONSES_ONLY_MODELS: set[str] = set()
# Keyed by (endpoint, model) rather than model alone: the same model name can
# behave differently on api.openai.com and on an OpenAI-compatible proxy, so a
# conflict learned against one endpoint must not leak to another.
_LEARNED_RESPONSES_ONLY_MODELS: set[tuple[str, str]] = set()
# Known model families that reject `reasoning_effort` alongside function tools on
# /v1/chat/completions. This is only a fast path to avoid a wasted round trip --
@@ -83,7 +86,7 @@ _TOOLS_REASONING_EFFORT_INCOMPATIBLE: tuple[str, ...] = (
# Models learned at runtime from a 400, keyed by the model string as configured.
# Populated by `_remember_reasoning_effort_conflict` so the retry is paid once
# per model per process rather than on every call.
_LEARNED_TOOLS_REASONING_EFFORT_CONFLICTS: set[str] = set()
_LEARNED_TOOLS_REASONING_EFFORT_CONFLICTS: set[tuple[str, str]] = set()
class WebSearchResult(TypedDict, total=False):
@@ -1687,7 +1690,7 @@ class OpenAICompletion(BaseLLM):
def _remember_responses_only_model(self) -> None:
"""Record that this model must use the Responses API."""
_LEARNED_RESPONSES_ONLY_MODELS.add(self.model)
_LEARNED_RESPONSES_ONLY_MODELS.add(self._model_cache_key())
def _model_not_found_message(self, error: Exception) -> str:
"""Build a 404 message that says what to do about it.
@@ -1720,7 +1723,7 @@ class OpenAICompletion(BaseLLM):
The Responses API has no such restriction (it takes
`reasoning: {"effort": ...}`), so this only constrains the completions path.
"""
if model in _LEARNED_TOOLS_REASONING_EFFORT_CONFLICTS:
if self._model_cache_key(model) in _LEARNED_TOOLS_REASONING_EFFORT_CONFLICTS:
return False
model_lower = model.lower()
return not any(
@@ -1740,33 +1743,71 @@ class OpenAICompletion(BaseLLM):
"""
if not isinstance(error, BadRequestError):
return False
param = None
body = getattr(error, "body", None)
if isinstance(body, dict):
inner = body.get("error")
if isinstance(inner, dict):
param = inner.get("param")
param, body_message = OpenAICompletion._error_param_and_message(error)
if param != "reasoning_effort":
return False
message = str(getattr(error, "message", "") or error).lower()
return "function tools" in message and "reasoning_effort" in message
return "function tools" in body_message and "reasoning_effort" in body_message
@staticmethod
def _error_param_and_message(error: BaseException) -> tuple[str | None, str]:
"""Pull ``param`` and the API message out of an OpenAI error body.
The SDK populates ``body`` either flat (``{"message": ..., "param": ...}``)
or nested under an ``"error"`` key depending on how the response was
parsed, so both are handled. Falls back to the exception text.
"""
body = getattr(error, "body", None)
source: dict[str, Any] | None = None
if isinstance(body, dict):
inner = body.get("error")
source = inner if isinstance(inner, dict) else body
param = source.get("param") if source else None
message = str((source or {}).get("message") or "") or str(
getattr(error, "message", "") or error
)
return param, message.lower()
def _model_cache_key(self, model: str | None = None) -> tuple[str, str]:
"""Identity for the learned-behaviour caches: the endpoint plus the model.
The same model string can be served by api.openai.com and by an
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
def _is_recoverable_completion_error(self, error: BaseException) -> bool:
"""Whether ``_call_completions`` will retry instead of surfacing this error.
Kept next to the retry logic so the handlers that suppress failure
reporting and the handler that performs the retry can't drift apart.
"""
cause = error.__cause__ or error
if self._is_tools_reasoning_effort_error(cause):
return True
return self._is_responses_only_error(cause) and not self.custom_openai
def _remember_reasoning_effort_conflict(self) -> None:
"""Record that this model can't combine `reasoning_effort` with tools."""
_LEARNED_TOOLS_REASONING_EFFORT_CONFLICTS.add(self.model)
_LEARNED_TOOLS_REASONING_EFFORT_CONFLICTS.add(self._model_cache_key())
def _retry_params_without_reasoning_effort(
self, params: dict[str, Any]
) -> dict[str, Any] | None:
"""Strip `reasoning_effort` for a retry, or None if there's nothing to strip."""
if params.get("reasoning_effort") is None:
"""Force `reasoning_effort="none"` for a retry, or None if already there.
Omitting the parameter is not enough. Some models (gpt-5.6-*) apply a
server-side default, so a request with no `reasoning_effort` at all still
fails the same way -- only an explicit "none" is accepted alongside tools.
"""
if params.get("reasoning_effort") == "none":
return None
retry_params = {k: v for k, v in params.items() if k != "reasoning_effort"}
retry_params = {**params, "reasoning_effort": "none"}
logging.warning(
"Dropping reasoning_effort=%r and retrying: %r rejects it alongside "
'function tools on /v1/chat/completions. Use api="responses" to keep '
"reasoning effort with tools.",
params["reasoning_effort"],
'Retrying with reasoning_effort="none": %r rejects reasoning effort '
'alongside function tools on /v1/chat/completions. Use api="responses" '
"to keep reasoning effort with tools.",
self.model,
)
return retry_params
@@ -1785,7 +1826,7 @@ class OpenAICompletion(BaseLLM):
if (
self.api == "completions"
and not self.custom_openai
and self.model in _LEARNED_RESPONSES_ONLY_MODELS
and self._model_cache_key() in _LEARNED_RESPONSES_ONLY_MODELS
):
return "responses"
return self.api
@@ -1853,12 +1894,15 @@ class OpenAICompletion(BaseLLM):
and params.get("reasoning_effort") not in (None, "none")
and not self._supports_reasoning_effort_with_tools(self.model)
):
dropped = params.pop("reasoning_effort")
requested = params["reasoning_effort"]
# Explicit "none" rather than removing the key: gpt-5.6-* apply a
# server-side default, so omitting it fails the same way.
params["reasoning_effort"] = "none"
logging.debug(
"Dropping reasoning_effort=%r for model %r: function tools and "
"reasoning_effort cannot be combined on /v1/chat/completions. "
'Use api="responses" to keep reasoning effort with tools.',
dropped,
'Forcing reasoning_effort="none" (requested %r) for model %r: '
"function tools and reasoning effort cannot be combined on "
'/v1/chat/completions. Use api="responses" to keep both.',
requested,
self.model,
)
@@ -2058,6 +2102,12 @@ class OpenAICompletion(BaseLLM):
logging.error(f"Context window exceeded: {e}")
raise LLMContextLengthExceededError(str(e)) from e
# Recoverable by the caller: `_call_completions` retries these on the
# Responses API or without `reasoning_effort`. Reporting a failed call
# here would surface an error the user never actually experiences.
if self._is_recoverable_completion_error(e):
raise
error_msg = f"OpenAI API call failed: {e!s}"
logging.error(error_msg)
self._emit_call_failed_event(
@@ -2481,6 +2531,12 @@ class OpenAICompletion(BaseLLM):
logging.error(f"Context window exceeded: {e}")
raise LLMContextLengthExceededError(str(e)) from e
# Recoverable by the caller: `_call_completions` retries these on the
# Responses API or without `reasoning_effort`. Reporting a failed call
# here would surface an error the user never actually experiences.
if self._is_recoverable_completion_error(e):
raise
error_msg = f"OpenAI API call failed: {e!s}"
logging.error(error_msg)
self._emit_call_failed_event(

83
lib/crewai/tests/llms/openai/test_gpt56_tools_reasoning_effort.py
View File

@@ -100,7 +100,7 @@ class TestKnownFamiliesFastPath:
params = llm._prepare_completion_params(MESSAGES, tools=TOOLS)
assert "reasoning_effort" not in params
assert params["reasoning_effort"] == "none"
# Tools must survive — they are the reason the call exists.
assert params["tools"]
assert params["tool_choice"] == "auto"
@@ -111,7 +111,7 @@ class TestKnownFamiliesFastPath:
params = llm._prepare_completion_params(MESSAGES, tools=TOOLS)
assert "reasoning_effort" not in params
assert params["reasoning_effort"] == "none"
def test_keeps_reasoning_effort_without_tools(self):
"""Without tools the combination is legal, so nothing should change."""
@@ -121,6 +121,23 @@ class TestKnownFamiliesFastPath:
assert params["reasoning_effort"] == "high"
def test_forces_none_rather_than_removing_the_parameter(self):
"""Omitting `reasoning_effort` is not sufficient.
Measured against the live endpoint: gpt-5.6-* apply a server-side default,
so a request carrying no `reasoning_effort` at all is rejected exactly like
one carrying "high". Only an explicit "none" is accepted with tools.
gpt-5.6-sol tools, no reasoning_effort -> 400
gpt-5.6-sol tools, reasoning_effort=none -> OK
"""
llm = build("gpt-5.6-sol", additional_params={"reasoning_effort": "high"})
params = llm._prepare_completion_params(MESSAGES, tools=TOOLS)
assert "reasoning_effort" in params, "the key must be sent, not dropped"
assert params["reasoning_effort"] == "none"
def test_keeps_explicit_none_effort_with_tools(self):
"""OpenAI explicitly allows reasoning_effort='none' with tools here."""
llm = build("gpt-5.6", additional_params={"reasoning_effort": "none"})
@@ -193,7 +210,7 @@ class TestRuntimeRetry:
def fake_handle(params, **kwargs):
seen.append(params)
if "reasoning_effort" in params:
if params.get("reasoning_effort") not in (None, "none"):
raise make_bad_request(model="gpt-5.9")
return "ok"
@@ -204,7 +221,7 @@ class TestRuntimeRetry:
assert result == "ok"
assert len(seen) == 2, "expected one failed call and one retry"
assert seen[0]["reasoning_effort"] == "high"
assert "reasoning_effort" not in seen[1]
assert seen[1]["reasoning_effort"] == "none"
# Tools have to survive the retry.
assert seen[1]["tools"]
@@ -212,7 +229,7 @@ class TestRuntimeRetry:
llm = build("gpt-5.9", additional_params={"reasoning_effort": "high"})
def fake_handle(params, **kwargs):
if "reasoning_effort" in params:
if params.get("reasoning_effort") not in (None, "none"):
raise make_bad_request(model="gpt-5.9")
return "ok"
@@ -221,7 +238,7 @@ class TestRuntimeRetry:
# Second call must strip up front instead of paying the 400 again.
params = llm._prepare_completion_params(MESSAGES, tools=TOOLS)
assert "reasoning_effort" not in params
assert params["reasoning_effort"] == "none"
assert not llm._supports_reasoning_effort_with_tools("gpt-5.9")
def test_does_not_retry_unrelated_bad_request(self, monkeypatch):
@@ -239,9 +256,53 @@ class TestRuntimeRetry:
assert len(calls) == 1, "must not retry errors it doesn't understand"
def test_does_not_retry_when_nothing_to_strip(self, monkeypatch):
"""No reasoning_effort to remove means the retry can't help."""
def test_learning_is_scoped_to_the_endpoint(self, monkeypatch):
"""A conflict learned against one endpoint must not leak to another.
An OpenAI-compatible proxy may accept the combination that api.openai.com
rejects, so stripping the parameter there would silently degrade it.
"""
real = build("gpt-5.9", additional_params={"reasoning_effort": "high"})
proxy = build(
"gpt-5.9",
base_url="https://proxy.internal/v1",
additional_params={"reasoning_effort": "high"},
)
real._remember_reasoning_effort_conflict()
assert (
real._prepare_completion_params(MESSAGES, tools=TOOLS)["reasoning_effort"]
== "none"
)
assert (
proxy._prepare_completion_params(MESSAGES, tools=TOOLS)[
"reasoning_effort"
]
== "high"
)
def test_recoverable_error_is_not_reported_as_a_failure(self):
"""The retry must not surface a failure the user never experiences.
`_handle_completion` logs "OpenAI API call failed" and emits
LLMCallFailedEvent for anything it catches. For an error the caller is about
to recover from, that produces a user-visible error panel for a call that
ultimately succeeds.
"""
llm = build("gpt-5.9")
assert llm._is_recoverable_completion_error(
ValueError("wrapped").with_traceback(None)
) is False
recoverable = ValueError("wrapped")
recoverable.__cause__ = make_bad_request()
assert llm._is_recoverable_completion_error(recoverable)
def test_does_not_retry_twice_when_already_none(self, monkeypatch):
"""Once reasoning_effort is "none" there is nothing left to try."""
llm = build("gpt-5.9", additional_params={"reasoning_effort": "none"})
calls: list[dict] = []
def fake_handle(params, **kwargs):
@@ -253,7 +314,7 @@ class TestRuntimeRetry:
with pytest.raises(BadRequestError):
llm._call_completions(MESSAGES, tools=TOOLS)
assert len(calls) == 1
assert len(calls) == 1, "must not retry when already at 'none'"
@pytest.mark.asyncio
async def test_async_path_retries_too(self, monkeypatch):
@@ -262,7 +323,7 @@ class TestRuntimeRetry:
async def fake_handle(params, **kwargs):
seen.append(params)
if "reasoning_effort" in params:
if params.get("reasoning_effort") not in (None, "none"):
raise make_bad_request(model="gpt-5.9")
return "ok"
@@ -272,7 +333,7 @@ class TestRuntimeRetry:
assert result == "ok"
assert len(seen) == 2
assert "reasoning_effort" not in seen[1]
assert seen[1]["reasoning_effort"] == "none"
class TestResponsesApiUntouched:

16
lib/crewai/tests/llms/openai/test_responses_only_models.py
View File

@@ -187,10 +187,24 @@ class TestEffectiveApi:
llm = build(
"gpt-5-pro", custom_openai=True, base_url="https://my-vllm.internal/v1"
)
completion_module._LEARNED_RESPONSES_ONLY_MODELS.add("gpt-5-pro")
completion_module._LEARNED_RESPONSES_ONLY_MODELS.add(llm._model_cache_key())
assert llm._effective_api() == "completions"
def test_learning_is_scoped_to_the_endpoint(self):
"""A conflict learned against one endpoint must not leak to another.
The same model name can be served by api.openai.com and by an
OpenAI-compatible proxy with different capabilities.
"""
real = build("gpt-5-pro")
proxy = build("gpt-5-pro", base_url="https://proxy.internal/v1")
real._remember_responses_only_model()
assert real._effective_api() == "responses"
assert proxy._effective_api() == "completions"
class TestNotFoundMessage:
@pytest.mark.parametrize("message", RESPONSES_ONLY_MESSAGES)