From 155ba11c4a5d200b948b78d7012ec9ed1fea1631 Mon Sep 17 00:00:00 2001 From: alex-clawd Date: Sun, 26 Jul 2026 03:28:42 -0700 Subject: [PATCH] refactor(openai): drop the hardcoded model list entirely `_TOOLS_REASONING_EFFORT_INCOMPATIBLE` was kept as a "fast path" to skip a doomed request for gpt-5.4/5.5/5.6. That was the wrong call: it still meant a model table in this file, and I got that table wrong twice already -- first missing gpt-5.4 and gpt-5.5 entirely, then missing the -mini and -nano variants. Nothing is assumed up front now. The first call for a model goes out exactly as the caller asked; if it comes back 400, `_call_completions` retries with reasoning_effort="none" and records the model, so subsequent calls skip straight to "none". Same shape as the responses-only handling. Cost is one wasted round trip per model per process instead of a release every time OpenAI extends the restriction. `_supports_reasoning_effort_with_tools` is now purely a cache lookup, so both recovery paths in this file are driven by what the API actually returns rather than by what was measured on a particular night. Verified live with no list present: learned before -> set() 1st call gpt-5.6-sol -> tool call returned (400 caught and recovered) learned after -> {('https://api.openai.com/v1', 'gpt-5.6-sol')} 2nd call -> tool call returned, no 400 real Agent + tools -> 391 Tests: the fast-path cases are rewritten around learned state -- an unknown model sends what was asked, a known one is rewritten to "none", learning doesn't leak to other models, and the vacuous-pass guard is kept. tests/llms/openai -> 159 passed, 1 skipped. Ruff + mypy clean. --- .../llms/providers/openai/completion.py | 50 ++++------- .../test_gpt56_tools_reasoning_effort.py | 90 ++++++++----------- 2 files changed, 50 insertions(+), 90 deletions(-) diff --git a/lib/crewai/src/crewai/llms/providers/openai/completion.py b/lib/crewai/src/crewai/llms/providers/openai/completion.py index cb4658823..e2d4c3e40 100644 --- a/lib/crewai/src/crewai/llms/providers/openai/completion.py +++ b/lib/crewai/src/crewai/llms/providers/openai/completion.py @@ -67,25 +67,10 @@ if TYPE_CHECKING: # 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 -- -# the authoritative check is `_is_tools_reasoning_effort_error`, which catches -# families we haven't measured yet. -# -# Measured against /v1/chat/completions: gpt-5.4, gpt-5.5 and gpt-5.6 (including -# -mini, -nano, dated snapshots and the 5.6 sol/terra/luna variants) all reject -# the combination. gpt-5.2 and earlier, o1, o3 and o4-mini accept it. -# Matched as substrings against the lowercased model name so provider prefixes -# ("openai/gpt-5.5") and suffixed variants ("gpt-5.6-sol") are covered too. -_TOOLS_REASONING_EFFORT_INCOMPATIBLE: tuple[str, ...] = ( - "gpt-5.4", - "gpt-5.5", - "gpt-5.6", -) - -# 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. +# Models learned at runtime to reject `reasoning_effort` alongside function tools, +# keyed by (endpoint, model). Populated from the 400 by +# `_remember_reasoning_effort_conflict` so the wasted round trip is paid once per +# model per process rather than on every call. _LEARNED_TOOLS_REASONING_EFFORT_CONFLICTS: set[tuple[str, str]] = set() @@ -1749,27 +1734,23 @@ class OpenAICompletion(BaseLLM): return f"Model {self.model} not found: {error}" def _supports_reasoning_effort_with_tools(self, model: str) -> bool: - """Whether a model accepts `reasoning_effort` together with function tools. + """Whether this model is already known to reject the combination. - On /v1/chat/completions, newer reasoning models reject the combination: + Nothing is assumed up front -- no model list. A model is only known once a + 400 has been seen for it, which `_call_completions` recovers from: "Function tools with reasoning_effort are not supported for gpt-5.5 in /v1/chat/completions. To use function tools, use /v1/responses or set reasoning_effort to 'none'." - Known families are listed in `_TOOLS_REASONING_EFFORT_INCOMPATIBLE` purely - to skip a doomed request. Models discovered through a 400 at runtime are - remembered in `_LEARNED_TOOLS_REASONING_EFFORT_CONFLICTS`, so OpenAI can - extend the restriction to new families without a release here. + So OpenAI can extend the restriction to new families without a release here. The Responses API has no such restriction (it takes `reasoning: {"effort": ...}`), so this only constrains the completions path. """ - if self._model_cache_key(model) in _LEARNED_TOOLS_REASONING_EFFORT_CONFLICTS: - return False - model_lower = model.lower() - return not any( - family in model_lower for family in _TOOLS_REASONING_EFFORT_INCOMPATIBLE + return ( + self._model_cache_key(model) + not in _LEARNED_TOOLS_REASONING_EFFORT_CONFLICTS ) @staticmethod @@ -1942,11 +1923,10 @@ class OpenAICompletion(BaseLLM): params["tools"] = self._convert_tools_for_interference(tools) params["tool_choice"] = "auto" - # gpt-5.4+ reject `reasoning_effort` on the completions endpoint as soon - # as function tools are present. Anything other than "none" is a hard 400, - # so drop it up front for models we already know about rather than burn a - # round trip. Families we haven't measured are caught by the 400 handler - # in `_call_completions` instead. + # Some models reject `reasoning_effort` on the completions endpoint as + # soon as function tools are present. Once a 400 has taught us that about + # this model, skip straight to "none" instead of repeating the doomed + # request; the first occurrence is recovered in `_call_completions`. # Checked after the tools block on purpose: `reasoning_effort` can also # arrive through `additional_params`, which bypasses the typed field. if ( diff --git a/lib/crewai/tests/llms/openai/test_gpt56_tools_reasoning_effort.py b/lib/crewai/tests/llms/openai/test_gpt56_tools_reasoning_effort.py index bbb0e05cd..0e25beaa9 100644 --- a/lib/crewai/tests/llms/openai/test_gpt56_tools_reasoning_effort.py +++ b/lib/crewai/tests/llms/openai/test_gpt56_tools_reasoning_effort.py @@ -77,37 +77,41 @@ def _clear_learned_conflicts(): completion_module._LEARNED_TOOLS_REASONING_EFFORT_CONFLICTS.clear() -class TestKnownFamiliesFastPath: - """Models we've measured are stripped before the request goes out.""" +class TestLearnedFastPath: + """Nothing is assumed up front; a 400 is what teaches us about a model.""" - @pytest.mark.parametrize( - "model", - [ - "gpt-5.6", - "openai/gpt-5.6", - "gpt-5.6-sol", - "GPT-5.6-Terra", - "gpt-5.6-luna", - "gpt-5.5", - "gpt-5.5-2026-04-23", - "gpt-5.4", - "gpt-5.4-mini", - "gpt-5.4-nano", - ], - ) - def test_drops_reasoning_effort_for_incompatible_models(self, model: str): - llm = build(model, additional_params={"reasoning_effort": "high"}) + def test_sends_reasoning_effort_for_an_unknown_model(self): + """No model list, so the first request goes out as the caller asked. + + The 400 it may come back with is recovered in `_call_completions`. + """ + llm = build("gpt-5.6", additional_params={"reasoning_effort": "high"}) params = llm._prepare_completion_params(MESSAGES, tools=TOOLS) + assert params["reasoning_effort"] == "high" + + def test_forces_none_once_the_model_is_known(self): + """After a 400, skip the doomed request and go straight to "none". + + Explicit "none" rather than removing the key: gpt-5.6-* apply a + server-side default, so omitting it is rejected the same way. + """ + llm = build("gpt-5.6", additional_params={"reasoning_effort": "high"}) + llm._remember_reasoning_effort_conflict() + + 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" # Tools must survive — they are the reason the call exists. assert params["tools"] assert params["tool_choice"] == "auto" - def test_drops_reasoning_effort_supplied_via_additional_params(self): + def test_learning_applies_to_the_additional_params_leak(self): """additional_params bypasses the typed field, so it must be checked too.""" llm = build("gpt-5.5", additional_params={"reasoning_effort": "medium"}) + llm._remember_reasoning_effort_conflict() params = llm._prepare_completion_params(MESSAGES, tools=TOOLS) @@ -116,51 +120,27 @@ class TestKnownFamiliesFastPath: def test_keeps_reasoning_effort_without_tools(self): """Without tools the combination is legal, so nothing should change.""" llm = build("gpt-5.5", additional_params={"reasoning_effort": "high"}) + llm._remember_reasoning_effort_conflict() params = llm._prepare_completion_params(MESSAGES, tools=None) assert params["reasoning_effort"] == "high" - def test_forces_none_rather_than_removing_the_parameter(self): - """Omitting `reasoning_effort` is not sufficient. + def test_learning_does_not_touch_other_models(self): + """A conflict is remembered per model, not globally.""" + build("gpt-5.5")._remember_reasoning_effort_conflict() + other = build("gpt-5.2", additional_params={"reasoning_effort": "high"}) - 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"}) - - params = llm._prepare_completion_params(MESSAGES, tools=TOOLS) - - assert params["reasoning_effort"] == "none" - - @pytest.mark.parametrize("model", ["o1", "o3-mini", "gpt-5.2", "gpt-5"]) - def test_unaffected_models_keep_reasoning_effort_with_tools(self, model: str): - """Measured as accepting the combination — must not be stripped.""" - llm = build(model, additional_params={"reasoning_effort": "high"}) - - params = llm._prepare_completion_params(MESSAGES, tools=TOOLS) + params = other._prepare_completion_params(MESSAGES, tools=TOOLS) assert params["reasoning_effort"] == "high" def test_fast_path_actually_has_something_to_drop(self): - """Guard against the drop tests passing vacuously. + """Guard against the assertions above passing vacuously. The typed `reasoning_effort` field is gated behind `is_o1_model`, so it never reaches the payload for gpt-5.x. If a test set it that way there - would be nothing to strip and the assertion would pass for the wrong + would be nothing to rewrite and the assertion would pass for the wrong reason — `additional_params` is the path that actually leaks. """ leaked = build( @@ -168,9 +148,9 @@ class TestKnownFamiliesFastPath: )._prepare_completion_params(MESSAGES, tools=None) assert leaked["reasoning_effort"] == "high" - typed = build( - "gpt-5.5", reasoning_effort="high" - )._prepare_completion_params(MESSAGES, tools=None) + typed = build("gpt-5.5", reasoning_effort="high")._prepare_completion_params( + MESSAGES, tools=None + ) assert "reasoning_effort" not in typed