fix(thinking): clamp unsupported levels by model info#3869
Conversation
There was a problem hiding this comment.
Code Review
This pull request simplifies the thinking configuration validation by clamping unsupported levels against the target model's advertised levels, regardless of whether the translation crosses provider families. This replaces the previous strict validation that returned errors for same-family conversions with unsupported levels. The test suite has been updated to reflect this change, updating cases that previously expected errors to now expect clamped values. Feedback on the changes suggests removing the now-redundant variables toCapability and toHasLevelSupport entirely, rather than using a blank identifier to silence compiler warnings.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| toCapability := detectModelCapability(modelInfo) | ||
| toHasLevelSupport := toCapability == CapabilityLevelOnly || toCapability == CapabilityHybrid | ||
| allowClampUnsupported := toHasLevelSupport && !isSameProviderFamily(fromFormat, toFormat) | ||
| _ = toHasLevelSupport |
There was a problem hiding this comment.
677cb1b to
3c38802
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 677cb1b230
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| config.Level = clampLevel(config.Level, modelInfo, toFormat) | ||
| } | ||
| // Clamp against the target model's advertised levels, regardless of translator family. | ||
| config.Level = clampLevel(config.Level, modelInfo, toFormat) |
There was a problem hiding this comment.
Map xhigh to max for max-capable Claude models
When fromFormat == toFormat == "claude" and a max-capable adaptive model receives output_config.effort:"xhigh", this new unconditional clamp calls clampLevel, whose tie-breaker prefers the lower neighbor when both high and max are supported. That silently normalizes the request to high instead of max, so the updated C24 scenario in test/thinking_conversion_test.go will not match production behavior and users requesting extra-high effort on Opus get downgraded; this path needs the Claude effort mapping or a clamp rule that chooses max for this case.
Useful? React with 👍 / 👎.
c644548 to
bad794b
Compare
bad794b to
dc24e6a
Compare
Summary
Why
Requests like reasoning_effort=xhigh were rejected with invalid_request_error when the target model only supports low/medium/high, even though the clamp path already exists for cross-family conversions. Level validation should follow modelInfo, not translator family.
Verification