feat(model): support none and xhigh effort - #1474
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughCentralized ReasoningEffort/Verbosity enums, guards, and helpers were added/expanded (including 'none' and 'xhigh'); agent registry entries were version-bumped; reasoning-related logic was propagated across presenters, provider option mapping, DB mappers, renderer components, types, i18n, and tests. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,230,241,0.5)
participant UI as Renderer (ModelConfig / Chat)
end
rect rgba(200,230,200,0.5)
participant Presenter as ConfigPresenter / AgentRuntime
end
rect rgba(241,220,220,0.5)
participant Cap as ModelCapabilities
end
rect rgba(255,245,200,0.5)
participant DB as Provider DB / Registry
end
rect rgba(230,230,255,0.5)
participant Provider as LLM Provider Mapper
end
UI->>Presenter: request model portrait / update generation settings
Presenter->>Cap: getReasoningPortrait(modelId)
Cap->>DB: read merged portrait (registry, legacy, extra)
DB-->>Cap: portrait (may include effort, effortOptions)
Cap-->>Presenter: reasoningPortrait
Presenter->>Presenter: normalizeReasoningEffort & compute reasoningEnabled
Presenter->>Provider: buildProviderOptions(reasoningEnabled, reasoningEffort, thinkingBudget)
Provider-->Provider: gate vendor flags (sendReasoning/thinking/effort)
Provider-->>UI: provider options applied / session started
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (1)
test/renderer/components/ChatStatusBar.test.ts (1)
878-901: Use a selectable model id in this test setupLine 881-883 and Line 895 use
gpt-5.2, but the local mocked model list doesn’t include it. This can make the test pass for the wrong reason and become brittle if selection validation tightens.💡 Proposed test tightening
- preferredModel: { providerId: 'openai', modelId: 'gpt-5.2' }, - defaultModel: { providerId: 'openai', modelId: 'gpt-5.2' }, + preferredModel: { providerId: 'openai', modelId: 'gpt-4' }, + defaultModel: { providerId: 'openai', modelId: 'gpt-4' }, @@ - await (wrapper.vm as any).openModelSettings('openai', 'gpt-5.2') + await (wrapper.vm as any).openModelSettings('openai', 'gpt-4')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/components/ChatStatusBar.test.ts` around lines 878 - 901, The test uses a hard-coded model id 'gpt-5.2' that isn't present in the mocked model list, making selection brittle; update the test setup to use a selectable model id from the mock data (or add 'gpt-5.2' to the mock models) so openModelSettings('openai','gpt-5.2') actually selects a real entry—adjust the setup call or the mock model list referenced by setup(...) so the localSettings.reasoningEffort assertion and wrapper.text() checks exercise the real model selection path.
🤖 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/modelCapabilities.ts`:
- Around line 83-91: The helper uses a local DEFAULT_REASONING_EFFORT_OPTIONS
list which is out of sync with the shared reasoning-effort domain; update
usesExtendedEffortDefaultWithoutOptions to reference the canonical/shared
default-effort options (the exported constant or accessor from the
reasoning-effort domain) instead of DEFAULT_REASONING_EFFORT_OPTIONS, and adjust
the Boolean check so newly supported defaults (e.g., 'none', 'xhigh') are
treated correctly; apply the same replacement to the other occurrence that
touches resolvedPortrait.effortOptions (the logic that may delete
resolvedPortrait.effortOptions) so both places use the shared domain
constant/function and remain consistent with main-process portrait
normalization.
---
Nitpick comments:
In `@test/renderer/components/ChatStatusBar.test.ts`:
- Around line 878-901: The test uses a hard-coded model id 'gpt-5.2' that isn't
present in the mocked model list, making selection brittle; update the test
setup to use a selectable model id from the mock data (or add 'gpt-5.2' to the
mock models) so openModelSettings('openai','gpt-5.2') actually selects a real
entry—adjust the setup call or the mock model list referenced by setup(...) so
the localSettings.reasoningEffort assertion and wrapper.text() checks exercise
the real model selection path.
🪄 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: 5fd2a9a2-44b7-4d34-a37f-cb5ebb42ca61
📒 Files selected for processing (39)
resources/acp-registry/registry.jsonresources/model-db/providers.jsonscripts/fetch-provider-db.mjssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/configPresenter/modelCapabilities.tssrc/main/presenter/configPresenter/modelConfig.tssrc/main/presenter/sessionPresenter/types.tssrc/main/presenter/sqlitePresenter/tables/conversations.tssrc/main/presenter/sqlitePresenter/tables/deepchatSessions.tssrc/renderer/src/components/ChatConfig.vuesrc/renderer/src/components/chat/ChatStatusBar.vuesrc/renderer/src/components/settings/ModelConfigDialog.vuesrc/renderer/src/composables/useChatConfigFields.tssrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/shared/types/agent-interface.d.tssrc/shared/types/model-db.tssrc/shared/types/presenters/legacy.presenters.d.tssrc/shared/types/presenters/session.presenter.d.tssrc/shared/types/presenters/thread.presenter.d.tstest/main/presenter/agentSessionPresenter/integration.test.tstest/main/presenter/configPresenter/modelCapabilities.test.tstest/main/presenter/llmProviderPresenter/aiSdkProviderOptionsMapper.test.tstest/main/presenter/providerDbModelConfig.test.tstest/main/shared/modelDb.test.tstest/renderer/components/ChatStatusBar.test.tstest/renderer/components/ModelConfigDialog.test.tstest/renderer/components/NewThreadPage.test.ts
Comment on lines
+83
to
+91
| const usesExtendedEffortDefaultWithoutOptions = ( | ||
| portrait: ReasoningPortrait | undefined | ||
| ): boolean => { | ||
| if (!portrait || portrait.effortOptions !== undefined || portrait.mode === 'budget') { | ||
| return false | ||
| } | ||
|
|
||
| return Boolean(portrait.effort && !DEFAULT_REASONING_EFFORT_OPTIONS.includes(portrait.effort)) | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the shared reasoning-effort domain here.
This helper still keys off the local four-value DEFAULT_REASONING_EFFORT_OPTIONS, so newly supported defaults like none/xhigh are classified as “extended” and can hit the delete resolvedPortrait.effortOptions path below. That leaves the main-process portrait normalization out of sync with the PR’s expanded effort support.
♻️ Suggested fix
-import {
- ProviderAggregate,
- ProviderModel,
- ReasoningPortrait,
- type ReasoningEffort,
- type Verbosity
-} from '@shared/types/model-db'
+import {
+ DEFAULT_REASONING_EFFORT_OPTIONS,
+ ProviderAggregate,
+ ProviderModel,
+ ReasoningPortrait,
+ type ReasoningEffort,
+ type Verbosity
+} from '@shared/types/model-db'
@@
-const DEFAULT_REASONING_EFFORT_OPTIONS: ReasoningEffort[] = ['minimal', 'low', 'medium', 'high']
const BINARY_REASONING_EFFORT_OPTIONS: ReasoningEffort[] = ['low', 'high']Also applies to: 478-480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/configPresenter/modelCapabilities.ts` around lines 83 -
91, The helper uses a local DEFAULT_REASONING_EFFORT_OPTIONS list which is out
of sync with the shared reasoning-effort domain; update
usesExtendedEffortDefaultWithoutOptions to reference the canonical/shared
default-effort options (the exported constant or accessor from the
reasoning-effort domain) instead of DEFAULT_REASONING_EFFORT_OPTIONS, and adjust
the Boolean check so newly supported defaults (e.g., 'none', 'xhigh') are
treated correctly; apply the same replacement to the other occurrence that
touches resolvedPortrait.effortOptions (the logic that may delete
resolvedPortrait.effortOptions) so both places use the shared domain
constant/function and remain consistent with main-process portrait
normalization.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/configPresenter/index.ts (1)
1046-1067:⚠️ Potential issue | 🟠 MajorResolve DB-backed reasoning support through the capability helper.
Line 1066 bypasses
resolveCapabilityProviderId(...)/supportsReasoningCapability(...)and queriesmodelCapabilitieswith the rawproviderId. That will misreport reasoning support for aliased providers andnew-apimodels, so the renderer can hide the new reasoning controls for supported models.Suggested fix
- reasoning: modelCapabilities.supportsReasoning(providerId, m.id), + reasoning: this.supportsReasoningCapability(providerId, m.id),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/index.ts` around lines 1046 - 1067, In getDbProviderModels, the reasoning support check uses modelCapabilities.supportsReasoning(providerId, m.id) with the raw providerId which misreports for aliased/new-api providers; replace that call to use the capability-resolved provider id by calling resolveCapabilityProviderId(providerId) (falling back to providerId) and then invoke modelCapabilities.supportsReasoningCapability(resolvedCapabilityProviderId, m.id) (or the corresponding helper) so reasoning is determined via the capability helper for aliased/new-api providers.
🤖 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/llmProviderPresenter/aiSdk/providerOptionsMapper.ts`:
- Around line 123-130: The code calls modelCapabilities.getReasoningPortrait
directly with params.providerId and params.modelId which can bypass provider
aliasing; instead first resolve the effective provider id using
resolveCapabilityProviderId(...) (the same mapper used by the runtime) and then
call modelCapabilities.getReasoningPortrait with that resolved provider id and
params.modelId before passing the portrait into getReasoningEffectiveEnabled;
update the block referencing modelCapabilities.getReasoningPortrait,
resolveCapabilityProviderId, getReasoningEffectiveEnabled, and
params.providerId/params.modelId so reasoningEnabled is derived from the same
resolved provider the runtime uses.
---
Outside diff comments:
In `@src/main/presenter/configPresenter/index.ts`:
- Around line 1046-1067: In getDbProviderModels, the reasoning support check
uses modelCapabilities.supportsReasoning(providerId, m.id) with the raw
providerId which misreports for aliased/new-api providers; replace that call to
use the capability-resolved provider id by calling
resolveCapabilityProviderId(providerId) (falling back to providerId) and then
invoke
modelCapabilities.supportsReasoningCapability(resolvedCapabilityProviderId,
m.id) (or the corresponding helper) so reasoning is determined via the
capability helper for aliased/new-api providers.
🪄 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: b1f66a6c-8e84-42aa-9e1b-054fe2b11993
📒 Files selected for processing (21)
src/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/llmProviderPresenter/aiSdk/providerOptionsMapper.tssrc/renderer/src/components/settings/ModelConfigDialog.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/stores/modelStore.tssrc/shared/types/model-db.tstest/main/presenter/llmProviderPresenter/aiSdkProviderOptionsMapper.test.tstest/main/shared/modelDb.test.tstest/renderer/components/ModelConfigDialog.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/renderer/src/i18n/zh-TW/settings.json
- src/renderer/src/i18n/ru-RU/settings.json
🚧 Files skipped from review as they are similar to previous changes (10)
- src/renderer/src/i18n/pt-BR/settings.json
- src/renderer/src/i18n/en-US/settings.json
- test/main/shared/modelDb.test.ts
- src/renderer/src/i18n/fa-IR/settings.json
- src/renderer/src/i18n/fr-FR/settings.json
- test/main/presenter/llmProviderPresenter/aiSdkProviderOptionsMapper.test.ts
- test/renderer/components/ModelConfigDialog.test.ts
- src/renderer/src/i18n/ko-KR/settings.json
- src/renderer/src/i18n/da-DK/settings.json
- src/renderer/src/i18n/ja-JP/settings.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/llmProviderPresenter/aiSdk/providerOptionsMapper.ts (1)
233-245:⚠️ Potential issue | 🟠 MajorGuard
effortbehindreasoningEnabledand explicitly map extended enum values instead of collapsing them to'medium'.The current condition sends
config.effortwheneverreasoningEffortis set, but this doesn't account for the reasoning disabled state. More critically, it collapses all non-'low'/'high'values to'medium', which is incorrect:
reasoningEffort: 'none'emitsconfig.effort = 'medium'(should omit effort entirely)reasoningEffort: 'xhigh'emitsconfig.effort = 'medium'(should map to'high')Add the
reasoningEnabledcheck to match thesendReasoningguard (line 31), and explicitly map all enum values ('none','minimal','low','medium','high','xhigh') to their Anthropic equivalents instead of defaulting unknown values to'medium'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/llmProviderPresenter/aiSdk/providerOptionsMapper.ts` around lines 233 - 245, Guard setting config.effort by the existing reasoningEnabled flag and explicitly map every allowed params.modelConfig.reasoningEffort enum to the correct Anthropic value instead of collapsing unknowns to "medium": when officialAnthropicProvider && reasoningEnabled && params.modelConfig.reasoningEffort is set, translate values as follows — 'none' = do not set config.effort (omit it), 'minimal' -> 'low', 'low' -> 'low', 'medium' -> 'medium', 'high' -> 'high', 'xhigh' -> 'high' — and assign the mapped value to config.effort; use the symbols officialAnthropicProvider, reasoningEnabled, params.modelConfig.reasoningEffort and config.effort in providerOptionsMapper.ts to locate and implement the change.
🧹 Nitpick comments (2)
test/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.ts (1)
1991-2012: Add anoneregression beside thisxhighcase.This only proves stale values collapse to a fixed default. The other half of the feature is
reasoningEffort: 'none'disabling reasoning, so a sibling test would better protect the new request-mapping path from regressing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.ts` around lines 1991 - 2012, Add a sibling test to the existing "normalizes stale reasoning effort values to a fixed portrait default" spec that verifies stale reasoning values map to 'none' to disable reasoning: mock configPresenter.getReasoningPortrait to return mode: 'effort' with effort: 'none', call agent.initSession('s1', ...) and then agent.updateGenerationSettings('s1', { reasoningEffort: 'low' }) and assert the returned updated.reasoningEffort is 'none' and that sqlitePresenter.deepchatSessionsTable.updateGenerationSettings was called with an objectContaining { reasoningEffort: 'none' }; mirror the structure and assertions used in the existing test that checks for 'xhigh' to ensure parity.test/main/presenter/configPresenter/providerModelCapabilityMapping.test.ts (1)
15-22: Redundantvi.restoreAllMocks()call inafterEach.
vi.restoreAllMocks()is already called inbeforeEach(line 17), which runs before each test. TheafterEachcall at line 21 is redundant sincebeforeEachwill reset mocks before the next test anyway.🧹 Suggested cleanup
- afterEach(() => { - vi.restoreAllMocks() - })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/configPresenter/providerModelCapabilityMapping.test.ts` around lines 15 - 22, The afterEach block contains a redundant vi.restoreAllMocks() because beforeEach already calls vi.resetModules() and vi.restoreAllMocks(); remove the vi.restoreAllMocks() call from the afterEach so mocks are only restored in beforeEach and avoid duplicate restores—update the afterEach (or remove it entirely if it no longer contains any statements) in providerModelCapabilityMapping.test.ts to eliminate the redundant vi.restoreAllMocks() invocation.
🤖 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/shared/types/model-db.ts`:
- Around line 177-205: The function normalizeReasoningEffortValue currently
falls through to returning the original value even when portrait.mode is
'budget', 'level', or 'mixed', allowing stale reasoningEffort to be preserved;
update normalizeReasoningEffortValue to return undefined for portraits that do
not support effort controls (i.e., when
canResolveReasoningEffortFromPortrait(portrait) is false or portrait.mode is
'budget'|'level'|'mixed'), keeping the existing logic that validates against
portrait.effort and portrait.effortOptions (use isReasoningEffort and
options.includes checks as-is) but replace the final return value with undefined
for non-effort portraits so callers (e.g.,
ModelConfigHelper.buildConfigFromProviderModel) cannot emit an unsupported
reasoningEffort.
---
Outside diff comments:
In `@src/main/presenter/llmProviderPresenter/aiSdk/providerOptionsMapper.ts`:
- Around line 233-245: Guard setting config.effort by the existing
reasoningEnabled flag and explicitly map every allowed
params.modelConfig.reasoningEffort enum to the correct Anthropic value instead
of collapsing unknowns to "medium": when officialAnthropicProvider &&
reasoningEnabled && params.modelConfig.reasoningEffort is set, translate values
as follows — 'none' = do not set config.effort (omit it), 'minimal' -> 'low',
'low' -> 'low', 'medium' -> 'medium', 'high' -> 'high', 'xhigh' -> 'high' — and
assign the mapped value to config.effort; use the symbols
officialAnthropicProvider, reasoningEnabled, params.modelConfig.reasoningEffort
and config.effort in providerOptionsMapper.ts to locate and implement the
change.
---
Nitpick comments:
In `@test/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.ts`:
- Around line 1991-2012: Add a sibling test to the existing "normalizes stale
reasoning effort values to a fixed portrait default" spec that verifies stale
reasoning values map to 'none' to disable reasoning: mock
configPresenter.getReasoningPortrait to return mode: 'effort' with effort:
'none', call agent.initSession('s1', ...) and then
agent.updateGenerationSettings('s1', { reasoningEffort: 'low' }) and assert the
returned updated.reasoningEffort is 'none' and that
sqlitePresenter.deepchatSessionsTable.updateGenerationSettings was called with
an objectContaining { reasoningEffort: 'none' }; mirror the structure and
assertions used in the existing test that checks for 'xhigh' to ensure parity.
In `@test/main/presenter/configPresenter/providerModelCapabilityMapping.test.ts`:
- Around line 15-22: The afterEach block contains a redundant
vi.restoreAllMocks() because beforeEach already calls vi.resetModules() and
vi.restoreAllMocks(); remove the vi.restoreAllMocks() call from the afterEach so
mocks are only restored in beforeEach and avoid duplicate restores—update the
afterEach (or remove it entirely if it no longer contains any statements) in
providerModelCapabilityMapping.test.ts to eliminate the redundant
vi.restoreAllMocks() invocation.
🪄 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: 9b27e1ed-cfab-4ef5-985c-34626f403021
📒 Files selected for processing (12)
src/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/configPresenter/modelConfig.tssrc/main/presenter/llmProviderPresenter/aiSdk/providerOptionsMapper.tssrc/renderer/src/components/settings/ModelConfigDialog.vuesrc/renderer/src/stores/modelStore.tssrc/shared/types/model-db.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/configPresenter/providerModelCapabilityMapping.test.tstest/main/presenter/llmProviderPresenter/aiSdkProviderOptionsMapper.test.tstest/main/shared/modelDb.test.tstest/renderer/stores/modelStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/main/presenter/llmProviderPresenter/aiSdkProviderOptionsMapper.test.ts
Comment on lines
+177
to
+205
| const canResolveReasoningEffortFromPortrait = ( | ||
| portrait: ReasoningPortrait | null | undefined | ||
| ): boolean => | ||
| portrait?.mode !== 'budget' && portrait?.mode !== 'level' && portrait?.mode !== 'mixed' | ||
|
|
||
| export const normalizeReasoningEffortValue = ( | ||
| portrait: ReasoningPortrait | null | undefined, | ||
| value: unknown | ||
| ): ReasoningEffort | undefined => { | ||
| if (!isReasoningEffort(value)) { | ||
| return undefined | ||
| } | ||
|
|
||
| const options = portrait?.effortOptions?.filter(isReasoningEffort) | ||
| if (options && options.length > 0) { | ||
| if (options.includes(value)) { | ||
| return value | ||
| } | ||
|
|
||
| return isReasoningEffort(portrait?.effort) && options.includes(portrait.effort) | ||
| ? portrait.effort | ||
| : undefined | ||
| } | ||
|
|
||
| if (canResolveReasoningEffortFromPortrait(portrait) && isReasoningEffort(portrait?.effort)) { | ||
| return value === portrait.effort ? value : portrait.effort | ||
| } | ||
|
|
||
| return value |
Contributor
There was a problem hiding this comment.
Don't preserve reasoningEffort for non-effort portraits.
This helper falls through to return value when portrait.mode is budget, level,
or mixed, so callers like ModelConfigHelper.buildConfigFromProviderModel() can still
emit a stale reasoningEffort for models that do not support effort controls.
💡 Proposed fix
export const normalizeReasoningEffortValue = (
portrait: ReasoningPortrait | null | undefined,
value: unknown
): ReasoningEffort | undefined => {
if (!isReasoningEffort(value)) {
return undefined
}
+
+ if (portrait && !canResolveReasoningEffortFromPortrait(portrait)) {
+ return undefined
+ }
const options = portrait?.effortOptions?.filter(isReasoningEffort)
if (options && options.length > 0) {
if (options.includes(value)) {
return value📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const canResolveReasoningEffortFromPortrait = ( | |
| portrait: ReasoningPortrait | null | undefined | |
| ): boolean => | |
| portrait?.mode !== 'budget' && portrait?.mode !== 'level' && portrait?.mode !== 'mixed' | |
| export const normalizeReasoningEffortValue = ( | |
| portrait: ReasoningPortrait | null | undefined, | |
| value: unknown | |
| ): ReasoningEffort | undefined => { | |
| if (!isReasoningEffort(value)) { | |
| return undefined | |
| } | |
| const options = portrait?.effortOptions?.filter(isReasoningEffort) | |
| if (options && options.length > 0) { | |
| if (options.includes(value)) { | |
| return value | |
| } | |
| return isReasoningEffort(portrait?.effort) && options.includes(portrait.effort) | |
| ? portrait.effort | |
| : undefined | |
| } | |
| if (canResolveReasoningEffortFromPortrait(portrait) && isReasoningEffort(portrait?.effort)) { | |
| return value === portrait.effort ? value : portrait.effort | |
| } | |
| return value | |
| const canResolveReasoningEffortFromPortrait = ( | |
| portrait: ReasoningPortrait | null | undefined | |
| ): boolean => | |
| portrait?.mode !== 'budget' && portrait?.mode !== 'level' && portrait?.mode !== 'mixed' | |
| export const normalizeReasoningEffortValue = ( | |
| portrait: ReasoningPortrait | null | undefined, | |
| value: unknown | |
| ): ReasoningEffort | undefined => { | |
| if (!isReasoningEffort(value)) { | |
| return undefined | |
| } | |
| if (portrait && !canResolveReasoningEffortFromPortrait(portrait)) { | |
| return undefined | |
| } | |
| const options = portrait?.effortOptions?.filter(isReasoningEffort) | |
| if (options && options.length > 0) { | |
| if (options.includes(value)) { | |
| return value | |
| } | |
| return isReasoningEffort(portrait?.effort) && options.includes(portrait.effort) | |
| ? portrait.effort | |
| : undefined | |
| } | |
| if (canResolveReasoningEffortFromPortrait(portrait) && isReasoningEffort(portrait?.effort)) { | |
| return value === portrait.effort ? value : portrait.effort | |
| } | |
| return value |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/types/model-db.ts` around lines 177 - 205, The function
normalizeReasoningEffortValue currently falls through to returning the original
value even when portrait.mode is 'budget', 'level', or 'mixed', allowing stale
reasoningEffort to be preserved; update normalizeReasoningEffortValue to return
undefined for portraits that do not support effort controls (i.e., when
canResolveReasoningEffortFromPortrait(portrait) is false or portrait.mode is
'budget'|'level'|'mixed'), keeping the existing logic that validates against
portrait.effort and portrait.effortOptions (use isReasoningEffort and
options.includes checks as-is) but replace the final return value with undefined
for non-effort portraits so callers (e.g.,
ModelConfigHelper.buildConfigFromProviderModel) cannot emit an unsupported
reasoningEffort.
This was referenced Apr 18, 2026
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
2 participants
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.
support none and xhigh effort
Summary by CodeRabbit
New Features
Chores
Tests