fix(provider): preserve user-defined model variants from config#12170
fix(provider): preserve user-defined model variants from config#12170monotykamary wants to merge 1 commit intoanomalyco:devfrom
Conversation
The previous code would unconditionally overwrite model.variants with
ProviderTransform.variants() result, which returns {} for kimi and other
models. This caused user-defined variants from config to be lost.
The fix merges ProviderTransform variants with existing model.variants
instead of overwriting, ensuring user config always takes precedence.
Subtle bug: In the rare case where both ProviderTransform and user config
define a variant with the same name, the old merge order could cause
unexpected behavior when configVariants lookup failed.
|
The following comment was made by an LLM, it may be inaccurate: No duplicate PRs found |
There was a problem hiding this comment.
Pull request overview
This pull request attempts to simplify the variant merging logic in the provider initialization code. The stated goal is to preserve user-defined model variants from config and fix a "subtle bug" where variants could be unintentionally overwritten. However, the implementation introduces a critical bug that breaks the handling of disabled variants.
Changes:
- Modified variant merging logic in
packages/opencode/src/provider/provider.ts(lines 925-932) - Removed direct access to
configProvider?.models?.[modelID]?.variants - Changed from a two-step process (overwrite then merge) to direct merging
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
can't u just change the conditional here to be if (configVariants) because model.variants should always be "defined" as in {} at worst right
There was a problem hiding this comment.
- if (configVariants && model.variants)
+ if (configVariants)Oh you mean this; it would probably be simpler. This would work if the configProvider lookup reliably returns the variants, but:
- it would require looking up the config twice
- if the config loading has any edge cases, variants could still be lost
const baseVariants = ProviderTransform.variants(model)
const mergedVariants = mergeDeep(baseVariants, model.variants ?? {})The subtle bug is overwriting then re-merging from config again, which this fix would hopefully future proof to:
- preserve whatever is already in model.variants
- merge ProviderTransform defaults underneath
- no double config lookup (i think)
- should be a bit more predictable
2 participants
Summary
Fixes a subtle bug where
model.variantscould be unintentionally overwritten whenProviderTransform.variants()returns{}for certain models (kimi, deepseek, minimax, etc.).The Problem
In
provider.ts, the previous code would:model.variantswithProviderTransform.variants(model)result (which is{}for kimi models)configVariantsWhile this currently works, it has a subtle issue: if
configVariantslookup fails or if variants were set in a previous processing step, they would be lost.The Fix
Instead of overwriting then merging, we now merge directly:
ProviderTransform.variants()provides the base defaultsmodel.variants(from user config) is merged on top, taking precedenceChanges
packages/opencode/src/provider/provider.ts(lines 925-932)Testing
Related
Closes #12169