fix(model): cap derived max tokens - #1482
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR introduces a derived max-tokens cap and resolver, replacing usages of Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/main/presenter/providerDbModelConfig.test.ts (2)
184-190: Consider importingDERIVED_MODEL_MAX_TOKENS_CAPinstead of hardcoding32000.The cap value
32000is duplicated in two assertions and in the test description. If the constant is tuned in the future (e.g., raised to48000), these tests will silently drift from the production default and require manual updates. Importing the shared constant keeps the tests in lockstep with the source of truth while still asserting the intended behavior.♻️ Proposed refactor
import { ModelConfigHelper } from '../../../src/main/presenter/configPresenter/modelConfig' import { modelCapabilities } from '../../../src/main/presenter/configPresenter/modelCapabilities' import { ModelType } from '../../../src/shared/model' +import { DERIVED_MODEL_MAX_TOKENS_CAP } from '../../../src/shared/modelConfigDefaults' @@ - expect(cfg.maxTokens).toBe(32000) + expect(cfg.maxTokens).toBe(DERIVED_MODEL_MAX_TOKENS_CAP) @@ - expect(providerRead.maxTokens).toBe(32000) + expect(providerRead.maxTokens).toBe(DERIVED_MODEL_MAX_TOKENS_CAP)Adjust the import path to match the actual module that exports
DERIVED_MODEL_MAX_TOKENS_CAP.Also applies to: 235-282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/providerDbModelConfig.test.ts` around lines 184 - 190, Replace the hardcoded 32000 in the test with the shared constant DERIVED_MODEL_MAX_TOKENS_CAP: import DERIVED_MODEL_MAX_TOKENS_CAP from the module that exports it, then assert expect(cfg.maxTokens).toBe(DERIVED_MODEL_MAX_TOKENS_CAP) and update the test description to reference the constant (or remove the literal) so ModelConfigHelper-derived cap assertions track the single source of truth; apply the same change to the other occurrences noted around lines 235-282.
235-282: Good coverage of the read-cap vs. user-preservation boundary.This test nicely pins down the two distinct behaviors introduced by the PR: provider-sourced cached entries get capped on read, while explicit user overrides bypass the cap. One small suggestion: also assert
providerRead.isUserDefinedisfalse(or absent) on line 262 to lock in that provider-cache reads don't accidentally get promoted to user-defined by the normalization path.♻️ Optional assertion hardening
const providerRead = helper.getModelConfig('large-output', 'test-provider') expect(providerRead.maxTokens).toBe(32000) + expect(providerRead.isUserDefined).toBe(false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/providerDbModelConfig.test.ts` around lines 235 - 282, Add an assertion to ensure provider-cache reads are not marked as user-defined: after calling helper.getModelConfig('large-output', 'test-provider') and assigning providerRead, assert that providerRead.isUserDefined is false (or undefined) to confirm provider-sourced entries from ModelConfigHelper (via generateCacheKey/importConfigs/getModelConfig) are not accidentally promoted to user-defined by normalization; keep the rest of the test (including the later setModelConfig call and userRead assertions) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/configPresenter/index.ts`:
- Line 1093: The renderer normalization path in refreshStandardModels currently
normalizes cached storedModels using resolveModelMaxTokens and then merges with
overrides ({ ...model, ...override, providerId }), which lets uncapped token
values win; update the renderer path to apply resolveDerivedModelMaxTokens (the
same capping used for DB values) when computing maxTokens for normalized models
so merged overrides cannot reintroduce values above the derived cap—ensure you
call resolveDerivedModelMaxTokens wherever resolveModelMaxTokens was used during
renderer model normalization (including before/after the merge step with
override and providerId) so storedModels and overrides both respect the derived
cap.
In `@src/shared/modelConfigDefaults.ts`:
- Around line 22-23: resolveDerivedModelMaxTokens currently forwards malformed
values (0, negative, NaN) through resolveModelMaxTokens and then applies
DERIVED_MODEL_MAX_TOKENS_CAP, which can produce unusable maxTokens; update
resolveDerivedModelMaxTokens to first obtain the resolved value via
resolveModelMaxTokens(name) and then clamp it to a sane positive minimum (e.g.,
ensure Number.isFinite(value) && value > 0 ? value : 1) before applying Math.min
with DERIVED_MODEL_MAX_TOKENS_CAP so that resolveDerivedModelMaxTokens always
returns a finite positive token limit.
---
Nitpick comments:
In `@test/main/presenter/providerDbModelConfig.test.ts`:
- Around line 184-190: Replace the hardcoded 32000 in the test with the shared
constant DERIVED_MODEL_MAX_TOKENS_CAP: import DERIVED_MODEL_MAX_TOKENS_CAP from
the module that exports it, then assert
expect(cfg.maxTokens).toBe(DERIVED_MODEL_MAX_TOKENS_CAP) and update the test
description to reference the constant (or remove the literal) so
ModelConfigHelper-derived cap assertions track the single source of truth; apply
the same change to the other occurrences noted around lines 235-282.
- Around line 235-282: Add an assertion to ensure provider-cache reads are not
marked as user-defined: after calling helper.getModelConfig('large-output',
'test-provider') and assigning providerRead, assert that
providerRead.isUserDefined is false (or undefined) to confirm provider-sourced
entries from ModelConfigHelper (via
generateCacheKey/importConfigs/getModelConfig) are not accidentally promoted to
user-defined by normalization; keep the rest of the test (including the later
setModelConfig call and userRead assertions) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc1f1525-1e12-44d6-9420-7775a8f7d686
📒 Files selected for processing (9)
src/main/presenter/configPresenter/index.tssrc/main/presenter/configPresenter/modelConfig.tssrc/main/presenter/llmProviderPresenter/providers/aiSdkProvider.tssrc/renderer/src/utils/maxOutputTokens.tssrc/shared/modelConfigDefaults.tstest/main/presenter/modelConfig.test.tstest/main/presenter/providerDbModelConfig.test.tstest/main/shared/modelConfigDefaults.test.tstest/renderer/components/ChatStatusBar.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/main/shared/modelConfigDefaults.test.ts (1)
39-46: Misplaced assertion insideresolveModelMaxTokensdescribe block.The cap constant assertion isn't about
resolveModelMaxTokens. Consider moving it into its owndescribe('DERIVED_MODEL_MAX_TOKENS_CAP', …)(or promoting to a top-levelit) so the test grouping reflects what's being validated.♻️ Suggested reorganization
describe('resolveModelMaxTokens', () => { it('preserves user-sized values above the derived cap', () => { expect(resolveModelMaxTokens(64000)).toBe(64000) }) +}) - it('exports the shared derived cap constant', () => { - expect(DERIVED_MODEL_MAX_TOKENS_CAP).toBe(32000) - }) +describe('DERIVED_MODEL_MAX_TOKENS_CAP', () => { + it('exports the shared derived cap constant', () => { + expect(DERIVED_MODEL_MAX_TOKENS_CAP).toBe(32000) + }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/shared/modelConfigDefaults.test.ts` around lines 39 - 46, The test asserting DERIVED_MODEL_MAX_TOKENS_CAP is misplaced inside the describe('resolveModelMaxTokens') block; extract that expectation into its own test group by moving expect(DERIVED_MODEL_MAX_TOKENS_CAP).toBe(32000) out of the resolveModelMaxTokens describe and into a new describe('DERIVED_MODEL_MAX_TOKENS_CAP', ...) (or a top-level it) so the assertion targets the constant directly while keeping resolveModelMaxTokens tests focused on resolveModelMaxTokens behavior.src/renderer/src/stores/modelStore.ts (1)
94-131: DRY: collapsenormalizeRendererModelandnormalizeDerivedRendererModel.The two normalizers are byte-for-byte identical except for the
maxTokensresolver. Duplicating ~18 lines invites drift — a later change to one (e.g., a new capability field) will almost certainly miss the other. Parameterize the resolver instead.♻️ Proposed refactor
- const normalizeRendererModel = (model: MODEL_META, providerId: string): RENDERER_MODEL_META => ({ - id: model.id, - name: model.name || model.id, - contextLength: resolveModelContextLength(model.contextLength), - maxTokens: resolveModelMaxTokens(model.maxTokens), - group: model.group || 'default', - providerId, - enabled: (model as RENDERER_MODEL_META).enabled ?? false, - isCustom: model.isCustom ?? false, - vision: resolveModelVision(model.vision), - functionCall: resolveModelFunctionCall(model.functionCall), - reasoning: model.reasoning ?? false, - enableSearch: (model as RENDERER_MODEL_META).enableSearch ?? false, - type: (model.type ?? ModelType.Chat) as ModelType, - supportedEndpointTypes: model.supportedEndpointTypes, - endpointType: model.endpointType - }) - - const normalizeDerivedRendererModel = ( - model: MODEL_META, - providerId: string - ): RENDERER_MODEL_META => ({ - id: model.id, - name: model.name || model.id, - contextLength: resolveModelContextLength(model.contextLength), - maxTokens: resolveDerivedModelMaxTokens(model.maxTokens), - group: model.group || 'default', - providerId, - enabled: (model as RENDERER_MODEL_META).enabled ?? false, - isCustom: model.isCustom ?? false, - vision: resolveModelVision(model.vision), - functionCall: resolveModelFunctionCall(model.functionCall), - reasoning: model.reasoning ?? false, - enableSearch: (model as RENDERER_MODEL_META).enableSearch ?? false, - type: (model.type ?? ModelType.Chat) as ModelType, - supportedEndpointTypes: model.supportedEndpointTypes, - endpointType: model.endpointType - }) + const buildRendererModel = ( + model: MODEL_META, + providerId: string, + resolveMaxTokens: (value: number | undefined | null) => number + ): RENDERER_MODEL_META => ({ + id: model.id, + name: model.name || model.id, + contextLength: resolveModelContextLength(model.contextLength), + maxTokens: resolveMaxTokens(model.maxTokens), + group: model.group || 'default', + providerId, + enabled: (model as RENDERER_MODEL_META).enabled ?? false, + isCustom: model.isCustom ?? false, + vision: resolveModelVision(model.vision), + functionCall: resolveModelFunctionCall(model.functionCall), + reasoning: model.reasoning ?? false, + enableSearch: (model as RENDERER_MODEL_META).enableSearch ?? false, + type: (model.type ?? ModelType.Chat) as ModelType, + supportedEndpointTypes: model.supportedEndpointTypes, + endpointType: model.endpointType + }) + + const normalizeRendererModel = (model: MODEL_META, providerId: string) => + buildRendererModel(model, providerId, resolveModelMaxTokens) + + const normalizeDerivedRendererModel = (model: MODEL_META, providerId: string) => + buildRendererModel(model, providerId, resolveDerivedModelMaxTokens)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/stores/modelStore.ts` around lines 94 - 131, The two functions normalizeRendererModel and normalizeDerivedRendererModel are identical except for how maxTokens is computed; collapse them into a single factory/normalized function (e.g., normalizeRendererModel) that accepts an additional parameter (a maxTokensResolver: (v: any) => number) or a boolean flag to choose between resolveModelMaxTokens and resolveDerivedModelMaxTokens, then use that resolver for the maxTokens field and remove the duplicate function; update all call sites that used normalizeDerivedRendererModel to call the unified function with resolveDerivedModelMaxTokens and keep typing (MODEL_META -> RENDERER_MODEL_META) unchanged so other fields like enabled, isCustom, vision, functionCall, supportedEndpointTypes and endpointType remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/src/stores/modelStore.ts`:
- Around line 94-131: The two functions normalizeRendererModel and
normalizeDerivedRendererModel are identical except for how maxTokens is
computed; collapse them into a single factory/normalized function (e.g.,
normalizeRendererModel) that accepts an additional parameter (a
maxTokensResolver: (v: any) => number) or a boolean flag to choose between
resolveModelMaxTokens and resolveDerivedModelMaxTokens, then use that resolver
for the maxTokens field and remove the duplicate function; update all call sites
that used normalizeDerivedRendererModel to call the unified function with
resolveDerivedModelMaxTokens and keep typing (MODEL_META -> RENDERER_MODEL_META)
unchanged so other fields like enabled, isCustom, vision, functionCall,
supportedEndpointTypes and endpointType remain identical.
In `@test/main/shared/modelConfigDefaults.test.ts`:
- Around line 39-46: The test asserting DERIVED_MODEL_MAX_TOKENS_CAP is
misplaced inside the describe('resolveModelMaxTokens') block; extract that
expectation into its own test group by moving
expect(DERIVED_MODEL_MAX_TOKENS_CAP).toBe(32000) out of the
resolveModelMaxTokens describe and into a new
describe('DERIVED_MODEL_MAX_TOKENS_CAP', ...) (or a top-level it) so the
assertion targets the constant directly while keeping resolveModelMaxTokens
tests focused on resolveModelMaxTokens behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 933395be-7ade-40e0-9ece-7c2aaaa74d42
📒 Files selected for processing (4)
src/renderer/src/stores/modelStore.tssrc/shared/modelConfigDefaults.tstest/main/shared/modelConfigDefaults.test.tstest/renderer/stores/modelStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/shared/modelConfigDefaults.ts
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
1 participant
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.
Summary by CodeRabbit
New Features
Improvements
Tests