Skip to content

fix(gpt5): clamp reasoning_effort 'max' where xhigh is unsupported - #3273

Open
auroraxo wants to merge 1 commit into
The-PR-Agent:mainfrom
auroraxo:fix/gpt5-reasoning-effort-clamp
Open

auroraxo wants to merge 1 commit into
The-PR-Agent:mainfrom
auroraxo:fix/gpt5-reasoning-effort-clamp

Conversation

@auroraxo

Copy link
Copy Markdown
Contributor

Closes #3271. Stacks on #3259 — the first two commits here are that PR's; the third is this change. Reviewing the last commit alone (78eb004) gives the whole of this fix, and this becomes a one-commit diff once #3259 lands.

#3259 translates this project's max alias to xhigh on the direct GPT-5 path. As #3271 notes, that is right for GPT-5.2 and later and wrong for the two older models, which do not accept xhigh either: it swaps one rejected value for another. This clamps to high where litellm says xhigh is unsupported.

Registry reading, verified locally on litellm 1.100.0, identical under the fetched and the bundled map:

model supports_xhigh_reasoning_effort
gpt-5 False
gpt-5-mini False
gpt-5.1 False
gpt-5.1-codex False
gpt-5.2 True
gpt-5.5 True
gpt-5.1-codex-max True

So the set is wider than the issue names — gpt-5-mini and gpt-5.1-codex are affected too, which is the argument for reading the metadata rather than listing model names.

What it does. On reasoning_effort='max' down the GPT-5 branch, it looks the model up with litellm.get_model_info (stripping a _thinking suffix, which the surrounding code already treats as a decoration rather than part of the model id) and sends high only when the registry answers False outright. True, a missing key, and a lookup that raises all keep xhigh: an unknown model is not evidence that xhigh is unsupported, and silently downgrading a model litellm has not heard of yet would be the same class of bug in the other direction.

Test plan. test_gpt5_reasoning_effort_max_uses_xhigh_metadata is parametrised over the four cases — supported, unsupported, key absent, lookup raises — and asserts both the effort that goes out and the exact string the lookup was made with, so the _thinking stripping and the provider-prefix stripping are pinned rather than incidental.

  • PYTHONPATH=. uv run pytest tests/unittest/test_litellm_reasoning_effort.py -q — 245 passed
  • full unit suite — 4458 passed
  • uv run ruff check on both touched files — clean

@Ricky-7-Yan Ricky-7-Yan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the follow-up commit independently. I also checked the model registry shipped with this repository's pinned LiteLLM 1.98.0 (rather than relying on the newer 1.100.0 result): it reports supports_xhigh_reasoning_effort=false for gpt-5, gpt-5-mini, gpt-5.1, and gpt-5.1-codex, and true for gpt-5.2, gpt-5.5, and gpt-5.1-codex-max. The metadata-driven clamp therefore matches the runtime dependency, while the missing-key/lookup-error fallback avoids downgrading future unknown models. The parameterized test covers both decisions plus prefix and _thinking normalization. Approving the logic. The branch currently conflicts after #3259's squash merge, so it still needs a rebase/drop of the two stacked commits before it can merge.

@auroraxo
auroraxo force-pushed the fix/gpt5-reasoning-effort-clamp branch from 78eb004 to 054489e Compare September 10, 2026 17:32
@qodo-code-review

qodo-code-review Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

Grey Divider

Sorry, something went wrong

We weren't able to complete the code review on our side. Please try again manually by commenting /agentic_review on this PR.

Grey Divider

Qodo Logo

@auroraxo

Copy link
Copy Markdown
Contributor Author

Rebased onto main at f51d8215, now 054489ec.

@Ricky-7-Yan was right that the branch had gone stale: #3259 landed the max to xhigh mapping as a squash merge, so the two stacked commits this branch carried were already in main under a different sha. Dropping them leaves a single commit, and the diff against main is now only the clamp itself — the supports_xhigh_reasoning_effort lookup, the high fallback for models that report False, and the keep-xhigh fallback when the model is unknown or the lookup raises.

Nothing in the clamp changed in the rebase; the conflict was entirely in the lines #3259 had already taken.

test_litellm_reasoning_effort.py: 245 passed. Full suite: 4556 passed, 1 skipped, 1 xfailed. ruff clean on both touched files. All 5 checks green on 054489ec, and the PR is mergeable again.

@IsmaelMartinez IsmaelMartinez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for turning the rebase round so fast after Ricky's note, and for reading the registry rather than listing model names.

One thing to redo, and it is not yours: #2999 landed on main about ninety minutes after you rebased and moved this block onto helper methods, so model_base no longer exists in that scope. It text-merges cleanly and the green checks predate it, so nothing on this page shows it. Merged onto today's main the suite is 4 failed with name 'model_base' is not defined.

Suggestion inline. With it the suite is green, and reverting the handler still takes all four new cases red.

"using 'xhigh' for reasoning_effort='max'"
)
effort = ReasoningEffort.XHIGH.value
lookup_model = model_base.removesuffix('_thinking')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After #2999 the model reaching this block carries the routed provider prefix, so the lookup name has to be stripped here. Applies and runs green on this head too.

Suggested change
lookup_model = model_base.removesuffix('_thinking')
lookup_model = model
while lookup_model.startswith(("openai/", "azure/")):
lookup_model = lookup_model.removeprefix("openai/").removeprefix("azure/")
lookup_model = lookup_model.removesuffix("_thinking")

@IsmaelMartinez

Copy link
Copy Markdown
Collaborator

@auroraxo, the inline suggestion from 11 Sep is the only thing between this and a merge: model_base moved with #2999, so the lookup needs the routed prefix stripped. Apply it and I merge. If it is still quiet on Thursday I will apply it myself and merge, with your commit intact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

reasoning_effort = "max" still fails on gpt-5 and gpt-5.1

3 participants