Skip to content

fix(aiCore): fix temperature/topP incorrectly disabled when reasoning_effort is default#13505

Merged
EurFelux merged 6 commits intomainfrom
fix/13503
Mar 24, 2026
Merged

fix(aiCore): fix temperature/topP incorrectly disabled when reasoning_effort is default#13505
EurFelux merged 6 commits intomainfrom
fix/13503

Conversation

@EurFelux
Copy link
Copy Markdown
Collaborator

@EurFelux EurFelux commented Mar 16, 2026

What this PR does

Before this PR:

  • When reasoning_effort was set to "default", temperature and topP parameters were incorrectly suppressed because "default" is a truthy string value.
  • Claude 3.7/4.0/4.5 models had no dedicated thinking model type, falling back to "default" which gave them generic reasoning effort options. Users could not select "none" to disable thinking even though the Anthropic API supports it.
  • TopP was fully disabled for Claude reasoning models with reasoning effort set, instead of being clamped to a valid range.
  • Parameter adjustments (e.g., temperature clamping, topP disabling) happened silently with no user-visible feedback.

After this PR:

  • Explicitly checks reasoning_effort !== 'default' and reasoning_effort !== 'none' so that temperature and topP are only disabled for actual reasoning effort values.
  • Adds a 'claude' thinking model type for Claude 3.7/4.0/4.5 that supports 'none' but not 'xhigh' (options: ['default', 'none', 'low', 'medium', 'high']).
  • TopP is clamped to [0.95, 1] for Claude reasoning models with thinking enabled, instead of being fully disabled.
  • Refactors getTemperature/getTopP by inlining helper functions and moving enable-checks to the top as early returns.
  • Adds logger.info messages 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:

  • Inlined getTemperatureValue/getTopPValue helper functions — they added indirection without value, and the enable-check was buried at the end instead of being a clear early return.
  • Added logger.info for user-visible feedback on parameter adjustments — info level is visible in user logs, helping users understand why their settings were modified.
  • Clamped topP to [0.95, 1] per Anthropic extended thinking docs rather than fully disabling it.
  • Added a separate 'claude' thinking type instead of reusing 'claude46', since Claude 3.7/4.0/4.5 do not support xhigh reasoning effort.

The following alternatives were considered:

  • Keeping the helper functions but fixing only the reasoning_effort check — rejected because the refactor makes the logic significantly easier to follow.
  • Using a single 'claude' type for all Claude models — rejected because 4.6 supports xhigh which older models don't.

Breaking changes

None

Special notes for your reviewer

  • The enableTemperature/enableTopP checks are moved to the top of each function as early returns, making the control flow clearer.
  • Temperature now falls back to DEFAULT_ASSISTANT_SETTINGS.temperature and the isMaxTemperatureOneModel clamp always runs (previously skipped when temperature was 0).
  • The Claude branch is placed first in _getThinkModelType for early matching.

Checklist

Release note

Fix temperature and topP parameters being incorrectly disabled when reasoning effort is set to "default" or "none" for Claude reasoning models. Add "none" option to disable thinking for Claude 3.7/4.0/4.5 models. TopP is now clamped to [0.95, 1] when thinking is enabled instead of being fully disabled.

…_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]>
@EurFelux EurFelux requested a review from DeJeune March 16, 2026 08:19
@EurFelux EurFelux marked this pull request as draft March 16, 2026 08:28
EurFelux and others added 2 commits March 16, 2026 16:41
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]>
@EurFelux EurFelux marked this pull request as ready for review March 16, 2026 09:00
@EurFelux EurFelux requested a review from kangfenmao March 16, 2026 09:00
@EurFelux EurFelux requested a review from GeorgeDong32 March 23, 2026 15:54
Copy link
Copy Markdown
Collaborator Author

@EurFelux EurFelux left a comment

Choose a reason for hiding this comment

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

Note

This review was translated by Claude.

The overall code quality is good, and the logic refactoring is clear. There is just a small typo.


Original Content

整体代码质量很好,逻辑重构清晰。只有一个小 typo。

}

if (isTemperatureTopPMutuallyExclusiveModel(model) && assistant.settings?.enableTemperature) {
logger.info(`Model ${model.id} only accepts one of temperature and top, disabling topP.`)
Copy link
Copy Markdown
Collaborator Author

@EurFelux EurFelux Mar 23, 2026

Choose a reason for hiding this comment

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

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 行的措辞保持一致。

Copy link
Copy Markdown
Collaborator

@GeorgeDong32 GeorgeDong32 left a comment

Choose a reason for hiding this comment

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

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

  1. Condition Check Fix: Correctly excludes default and none reasoning_effort values
  2. New claude thinking type: Provides correct options for Claude 3.7/4.0/4.5
  3. 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,变更清晰且测试覆盖充分。

核心修复

  1. 条件检查修复:正确排除 defaultnone 的 reasoning_effort 值
  2. 新增 claude thinking type:为 Claude 3.7/4.0/4.5 提供正确选项
  3. TopP clamping:按官方文档将 topP 限制到 [0.95, 1]

代码质量

  • ✅ 早期返回模式清晰
  • ✅ 日志反馈用户可见
  • ✅ 测试覆盖充分
  • ✅ 遵循项目规范

建议:批准合并

EurFelux and others added 3 commits March 24, 2026 13:25
- 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]>
@EurFelux EurFelux merged commit f0774b5 into main Mar 24, 2026
5 checks passed
@EurFelux EurFelux deleted the fix/13503 branch March 24, 2026 07:29
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.

[Bug]: Cherry Studio is not sending model parameters to the LLM

3 participants