fix(openai): bridge gpt-5.6+ tools to /v1/responses without reasoning_effort - #34043
Conversation
…_effort Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
|
Greptile SummaryThis PR fixes a 400 error that occurs when calling
Confidence Score: 4/5The bridge logic change is narrow, well-tested, and correctly scoped to gpt-5.6+; existing gpt-5.4/5.5 and reasoning-effort paths are untouched. The core fix is correct and the regression test suite is comprehensive. The main concern is that litellm/llms/openai/chat/gpt_5_transformation.py — the new
|
| Filename | Overview |
|---|---|
| litellm/llms/openai/chat/gpt_5_transformation.py | Refactors is_model_gpt_5_4_plus_model into a shared _gpt_5_minor_version_at_least helper and adds is_model_gpt_5_6_plus_model; logic is correct but the version boundary is hardcoded rather than driven by model_prices JSON per repo convention. |
| litellm/main.py | Adds the gpt-5.6+ tools-only bridge branch to responses_api_bridge_check; the condition correctly sits outside the reasoning_effort is not None guard, and the existing gpt-5.4/5.5 paths are unchanged. |
| tests/test_litellm/llms/openai/test_is_model_gpt_5_model.py | Adds TestOpenAIGPT5ConfigIsModelGpt56PlusModel with positive and negative parametrized cases; coverage is adequate for the new classifier, no real network calls. |
| tests/test_litellm/test_main.py | Adds three new bridge-check tests (gpt-5.6 tools-without-reasoning bridges, gpt-5.6 tools+effort bridges, gpt-5.4/5.5 tools-only stays on chat); tests are mocked, comprehensive, and correctly assert the expected mode. |
Reviews (1): Last reviewed commit: "fix(openai): bridge gpt-5.6+ tools to /v..." | Re-trigger Greptile
| major = version_str.split(".")[0] | ||
| return int(major) >= 4 | ||
| return int(major) >= minimum |
There was a problem hiding this comment.
The local variable
major stores the minor part of the gpt-5.X version (the X after the dot), not the major version. This naming is inverted and can mislead future readers who try to understand the parse logic.
| major = version_str.split(".")[0] | |
| return int(major) >= 4 | |
| return int(major) >= minimum | |
| minor = version_str.split(".")[0] | |
| return int(minor) >= minimum |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def is_model_gpt_5_6_plus_model(cls, model: str) -> bool: | ||
| """Check if the model is gpt-5.6 or newer (5.6, 5.7, etc., including named/pro variants).""" | ||
| return cls._gpt_5_minor_version_at_least(model, 6) |
There was a problem hiding this comment.
Hardcoded version boundary violates repo rule
The is_model_gpt_5_6_plus_model helper (and the unchanged is_model_gpt_5_4_plus_model) hardcode version thresholds in Python rather than using a flag in model_prices_and_context_window.json read via get_model_info. The repo rule exists so that new model families can be supported without a code change — for example, if a future gpt-5.8 family needs different bridging behaviour the >= 6 predicate would silently mis-classify it. A field like "tools_require_responses_bridge": true on each gpt-5.6 entry in the JSON (already present for azure/gpt-5.6, azure/gpt-5.6-sol, azure/gpt-5.6-terra, azure/gpt-5.6-luna) read through get_model_info would make the boundary data-driven and auditable.
Rule Used: What: Do not hardcode model-specific flags in the ... (source)
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Hi @krrish-berri-2 @greptile-apps! When are you merging this PR? Thanks! |
|
I'm a code review bot — I don't have any say in merge timelines. That decision is up to the maintainers (@krrish-berri-2 or whoever owns this repo). If the PR is ready for maintainer review, make sure the Greptile confidence score is at least 4/5 (per the pre-submission checklist) and then ping a maintainer directly on the PR. Tip: You can customize Greptile's behavior for this repo with |
|
when this can be merged into stable release? |
|
#34029's merged bridge covers gpt-5.4+ function tools even without explicit reasoning_effort, so #34043 is no longer needed. It's in v1.97.0-rc.1 and any version thereafter. Closing this |
|
Agreed, closing is right — #34029 is a strict superset of this PR and I'd rather have it. Verified against current #34029 also handles three things this PR did not: custom/grammar tools staying on chat ( Unrelated note for anyone landing here from the batch: #34044 (duration-only |
Relevant issues
Fixes #33221
Linear ticket
Pre-Submission checklist
Type
🐛 Bug Fix
Changes
Calling
/chat/completionswith function tools on a gpt-5.6 family model (gpt-5.6, gpt-5.6-sol/luna/terra) fails with a 400 even when the caller does not setreasoning_effort, because OpenAI applies a defaultreasoning_effortserver-side for that family and then rejects tools on Chat Completions ("use /v1/responses or set reasoning_effort to 'none'").responses_api_bridge_checkinmain.pyonly bridged tools-carrying requests to/v1/responseswhenreasoning_effort is not None, so these requests stayed on Chat Completions and 400'd.The narrow-but-wrong fix would be to bridge all gpt-5.4+ tool calls unconditionally, but staging already pins the opposite behavior for gpt-5.4/5.5:
test_responses_api_bridge_check_gpt_5_4_tools_without_reasoning_stays_chat(and its azure variant) assert that gpt-5.4 tools-only stays on Chat Completions. So this scopes the new tools-only bridge to gpt-5.6+ only.Bridge condition, before -> after:
Adds
OpenAIGPT5Config.is_model_gpt_5_6_plus_model, factoring the shared minor-version parse out ofis_model_gpt_5_4_plus_modelinto_gpt_5_minor_version_at_least(model, minimum)so both helpers stay in sync.Net behavior: gpt-5.6+ with tools bridges even without
reasoning_effort; gpt-5.4/5.5 tools-only keeps staying on Chat Completions; all existing explicit reasoning_effort / reasoning_summary paths are unchanged.Screenshots / Proof of Fix
These are internal/preview model names without a public endpoint I can bill against, so proof is at the bridge-decision layer that produces the 400. Regression tests in
tests/test_litellm/test_main.pyassertmode == "responses"for every gpt-5.6 variant (openai and azure) with tools andreasoning_effort=None, and assert gpt-5.4/5.5 tools-only stay on chat;tests/test_litellm/llms/openai/test_is_model_gpt_5_model.pycovers the new classifier. Happy to run a live/chat/completions-> bridged/v1/responsescurl if a callable gpt-5.6 deployment is available.Final Attestation
Link to Devin session: https://app.devin.ai/sessions/07cee113560043bb99d091e5ad1f1286
Requested by: @krrish-berri-2