Skip to content

fix(thinking): clamp unsupported levels by model info#3869

Closed
sususu98 wants to merge 1 commit into
router-for-me:devfrom
sususu98:fix/thinking-level-clamp-upstream-dev
Closed

fix(thinking): clamp unsupported levels by model info#3869
sususu98 wants to merge 1 commit into
router-for-me:devfrom
sususu98:fix/thinking-level-clamp-upstream-dev

Conversation

@sususu98

Copy link
Copy Markdown
Collaborator

Summary

  • Clamp unsupported thinking levels against the target model's ThinkingSupport instead of rejecting same-family requests.
  • Keep budget strict validation unchanged for same-family body requests.
  • Update thinking E2E tests so same-family xhigh/max requests clamp to the nearest supported level.

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

  • go test ./internal/thinking/...
  • go test -run 'TestThinkingE2EMatrix_(Suffix|Body)' ./test
  • go build -o test-output ./cmd/server && rm test-output

@gemini-code-assist gemini-code-assist Bot 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.

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.

Comment thread internal/thinking/validate.go Outdated
Comment on lines +56 to +58
toCapability := detectModelCapability(modelInfo)
toHasLevelSupport := toCapability == CapabilityLevelOnly || toCapability == CapabilityHybrid
allowClampUnsupported := toHasLevelSupport && !isSameProviderFamily(fromFormat, toFormat)
_ = toHasLevelSupport

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.

medium

The variables toCapability and toHasLevelSupport are no longer used after removing allowClampUnsupported. Instead of using the blank identifier _ = toHasLevelSupport to silence the compiler, these redundant lines should be completely removed to keep the code clean and maintainable.

@sususu98 sususu98 force-pushed the fix/thinking-level-clamp-upstream-dev branch from 677cb1b to 3c38802 Compare June 16, 2026 14:31

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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".

Comment thread internal/thinking/validate.go Outdated
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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge 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 👍 / 👎.

@sususu98 sususu98 force-pushed the fix/thinking-level-clamp-upstream-dev branch 2 times, most recently from c644548 to bad794b Compare June 16, 2026 14:43
@sususu98 sususu98 force-pushed the fix/thinking-level-clamp-upstream-dev branch from bad794b to dc24e6a Compare June 16, 2026 15:12
@luispater luispater closed this Jun 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants