Skip to content

Conversation

@EurFelux
Copy link
Collaborator

@EurFelux EurFelux commented Dec 3, 2025

What this PR does

Before this PR:

  • Used a single ambiguous function isNotSupportTemperatureAndTopP to check if models support temperature/top_p parameters
  • The function name was unclear about whether it meant "both parameters unsupported" vs "parameters cannot be used together"
  • No explicit handling for Claude 4.5's mutual exclusivity constraint between temperature and top_p

After this PR:

  • Split into two clear functions: isSupportTemperatureModel and isSupportTopPModel
  • Added isTemperatureTopPMutuallyExclusiveModel to explicitly handle Claude 4.5 reasoning models' constraint
  • Improved code readability with positive logic (isSupportX instead of isNotSupportX)
  • Added comprehensive test coverage for all new functions
  • Deprecated old function with backward compatibility

Why we need it and why it was done in this way

Motivation:

  1. The original function name isNotSupportTemperatureAndTopP was semantically ambiguous
  2. Claude 4.5 reasoning models have a unique constraint where temperature and top_p are mutually exclusive (not just unsupported)
  3. Better separation of concerns: support checking vs mutual exclusivity checking

Tradeoffs made:

  • Added slightly more code but improved maintainability and clarity

Alternatives considered:

  • Renaming the original function: Would cause breaking changes
  • Using a single combined function: Would not clearly express the mutual exclusivity constraint

Breaking changes

None.

Special notes for your reviewer

Key changes:

  1. src/renderer/src/config/models/utils.ts:

    • Added isSupportTemperatureModel() - checks if model supports temperature parameter
    • Added isSupportTopPModel() - checks if model supports top_p parameter
    • Added isTemperatureTopPMutuallyExclusiveModel() - checks if temperature and top_p are mutually exclusive (Claude 4.5)
    • Deprecated isNotSupportTemperatureAndTopP() with backward compatibility
  2. src/renderer/src/aiCore/legacy/clients/BaseApiClient.ts:

    • Updated getTemperature() and getTopP() to use new functions
  3. src/renderer/src/aiCore/prepareParams/modelParameters.ts:

    • Updated parameter handling logic to use new functions
  4. Test coverage:

    • Added comprehensive tests for isSupportTemperatureModel
    • Added comprehensive tests for isSupportTopPModel
    • Added comprehensive tests for isTemperatureTopPMutuallyExclusiveModel
    • Kept tests for deprecated function to ensure backward compatibility

Testing:

  • ✅ All 72 tests passing
  • ✅ Biome format/lint checks passing
  • ✅ No functional changes, pure refactoring

Checklist

  • PR: The PR description is expressive enough and will help future contributors
  • Code: Write code that humans can understand and Keep it simple
  • Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
  • Upgrade: Impact of this change on upgrade flows was considered and addressed if required
  • Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature.

Release note

NONE

…to separate functions

Replace deprecated isNotSupportTemperatureAndTopP with isSupportTemperatureModel and isSupportTopPModel
Add comprehensive tests for new model parameter support functions
…ndling

- Add fallback to DEFAULT_ASSISTANT_SETTINGS when enableTemperature/enableTopP is undefined
- Simplify conditional logic in parameter validation
- Update documentation to better explain parameter selection rules
…nction

The function was marked as deprecated and its usage has been replaced by isSupportTemperatureModel and isSupportTopPModel. Also removed corresponding test cases.
Add new utility function to enforce mutual exclusivity between temperature and top_p parameters for Claude 4.5 reasoning models. Update model parameter preparation logic to use this new check and add corresponding tests.
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.

3 participants