fix(aiCore): fix temperature/topP incorrectly disabled when reasoning_effort is default#13505
Conversation
…_effort is default When reasoning_effort was set to 'default', temperature and topP were being suppressed because 'default' is truthy. Now explicitly check for the 'default' value to preserve normal parameter behavior. Also refactors getTemperature/getTopP by inlining helper functions, moving enableTemperature/enableTopP checks to the top, and adding logger warnings for mutually exclusive parameter conflicts. Fixes #13503 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: icarus <[email protected]>
models - Exclude 'none' from reasoning effort check alongside 'default' - Clamp topP to [0.95, 1] for Claude reasoning models with thinking enabled - Remove redundant reasoning effort check that fully disabled topP - Add logger.info for parameter decisions to aid debugging - Update JSDoc to reflect new behavior Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: icarus <[email protected]>
…and update tests Add a separate 'claude' thinking model type for non-4.6 Claude reasoning models that supports 'none' but not 'xhigh'. Move Claude branch to the top of _getThinkModelType for early matching. Add tests for reasoning_effort 'default'/'none' behavior, topP clamping, and the new claude thinking model type. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: icarus <[email protected]>
kangfenmao
approved these changes
Mar 19, 2026
EurFelux
commented
Mar 23, 2026
| } | ||
|
|
||
| if (isTemperatureTopPMutuallyExclusiveModel(model) && assistant.settings?.enableTemperature) { | ||
| logger.info(`Model ${model.id} only accepts one of temperature and top, disabling topP.`) |
Collaborator
Author
There was a problem hiding this comment.
Note
This issue/comment/review was translated by Claude.
[C5] Typo: "only accepts one of temperature and top, disabling topP." → Should be "temperature and topP", to keep the wording consistent with lines 68 and 89.
Original Content
[C5] Typo: "only accepts one of temperature and top, disabling topP." → 应为 "temperature and topP",与第 68 行和第 89 行的措辞保持一致。
GeorgeDong32
approved these changes
Mar 23, 2026
Collaborator
There was a problem hiding this comment.
Note
This review was translated by Claude.
Review Complete
This PR fixes the parameter handling bug for Claude reasoning models, with clear changes and comprehensive test coverage.
Core Fixes
- Condition Check Fix: Correctly excludes
defaultandnonereasoning_effort values - New
claudethinking type: Provides correct options for Claude 3.7/4.0/4.5 - TopP clamping: Limits topP to [0.95, 1] per official documentation
Code Quality
- ✅ Early return pattern is clear
- ✅ Log feedback is user-visible
- ✅ Test coverage is comprehensive
- ✅ Follows project conventions
Recommendation: Approve merge
Original Content
审查完成
此 PR 修复了 Claude 推理模型的参数处理 bug,变更清晰且测试覆盖充分。
核心修复
- 条件检查修复:正确排除
default和none的 reasoning_effort 值 - 新增
claudethinking type:为 Claude 3.7/4.0/4.5 提供正确选项 - TopP clamping:按官方文档将 topP 限制到 [0.95, 1]
代码质量
- ✅ 早期返回模式清晰
- ✅ 日志反馈用户可见
- ✅ 测试覆盖充分
- ✅ 遵循项目规范
建议:批准合并
- Fix typo: "temperature and top" → "temperature and topP" - Only log temperature/topP clamping when values actually change - Clarify mutually exclusive log to describe own behavior without assuming other function's actions Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: icarus <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: icarus <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Before this PR:
reasoning_effortwas set to"default", temperature and topP parameters were incorrectly suppressed because"default"is a truthy string value."default"which gave them generic reasoning effort options. Users could not select"none"to disable thinking even though the Anthropic API supports it.After this PR:
reasoning_effort !== 'default'andreasoning_effort !== 'none'so that temperature and topP are only disabled for actual reasoning effort values.'claude'thinking model type for Claude 3.7/4.0/4.5 that supports'none'but not'xhigh'(options:['default', 'none', 'low', 'medium', 'high']).[0.95, 1]for Claude reasoning models with thinking enabled, instead of being fully disabled.getTemperature/getTopPby inlining helper functions and moving enable-checks to the top as early returns.logger.infomessages so users can see when parameters are adjusted or disabled in the log (e.g., temperature clamped, topP dropped due to mutual exclusivity).Fixes #13503
Why we need it and why it was done in this way
The following tradeoffs were made:
getTemperatureValue/getTopPValuehelper functions — they added indirection without value, and the enable-check was buried at the end instead of being a clear early return.logger.infofor user-visible feedback on parameter adjustments — info level is visible in user logs, helping users understand why their settings were modified.[0.95, 1]per Anthropic extended thinking docs rather than fully disabling it.'claude'thinking type instead of reusing'claude46', since Claude 3.7/4.0/4.5 do not supportxhighreasoning effort.The following alternatives were considered:
reasoning_effortcheck — rejected because the refactor makes the logic significantly easier to follow.'claude'type for all Claude models — rejected because 4.6 supportsxhighwhich older models don't.Breaking changes
None
Special notes for your reviewer
enableTemperature/enableTopPchecks are moved to the top of each function as early returns, making the control flow clearer.DEFAULT_ASSISTANT_SETTINGS.temperatureand theisMaxTemperatureOneModelclamp always runs (previously skipped when temperature was0)._getThinkModelTypefor early matching.Checklist
/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note