fix: prevent DefaultConfig template values from leaking into user model_list entries#724
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a config-loading bug where DefaultConfig()’s pre-populated ModelList entries could leak template field values (e.g., api_base) into user-provided model_list entries due to encoding/json reusing slice backing arrays during unmarshaling.
Changes:
- Pre-scan the config JSON into a temporary
Configto detect whether the user provided anymodel_listentries. - If the user provided entries, reset
cfg.ModelListtonilbefore the real unmarshal to force zero-value initialization ofModelConfigelements.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var tmp Config | ||
| if err := json.Unmarshal(data, &tmp); err != nil { | ||
| return nil, err | ||
| } | ||
| if len(tmp.ModelList) > 0 { |
There was a problem hiding this comment.
This pre-scan unmarshals the entire Config twice just to detect whether model_list has entries. That means running all nested decoding/custom unmarshaling twice (e.g., AgentModelConfig/FlexibleStringSlice) and doing extra allocations. Consider unmarshaling only the presence/length of model_list using a small struct with json.RawMessage/[]json.RawMessage (or *[]... to detect presence) so the main unmarshal runs only once.
| var tmp Config | |
| if err := json.Unmarshal(data, &tmp); err != nil { | |
| return nil, err | |
| } | |
| if len(tmp.ModelList) > 0 { | |
| type modelListEnvelope struct { | |
| ModelList *[]json.RawMessage `json:"model_list"` | |
| } | |
| var pre modelListEnvelope | |
| if err := json.Unmarshal(data, &pre); err != nil { | |
| return nil, err | |
| } | |
| if pre.ModelList != nil && len(*pre.ModelList) > 0 { |
| // Pre-scan the JSON to check how many model_list entries the user provided. | ||
| // Go's JSON decoder reuses existing slice backing-array elements rather than | ||
| // zero-initializing them, so fields absent from the user's JSON (e.g. api_base) | ||
| // would silently inherit values from the DefaultConfig template at the same | ||
| // index position. We only reset cfg.ModelList when the user actually provides | ||
| // entries; when count is 0 we keep DefaultConfig's built-in list as fallback. |
There was a problem hiding this comment.
The logic len(tmp.ModelList) > 0 can’t distinguish between (a) model_list missing and (b) model_list present but empty ([]) or null—all yield length 0 in tmp. If the intended behavior is “missing/empty keeps DefaultConfig model list as fallback”, the subsequent json.Unmarshal(data, cfg) will still clear the default slice when model_list: [] is explicitly present. Either adjust the comment/PR semantics to state the fallback only applies when the field is omitted, or change the pre-scan to detect field presence (e.g., *[]json.RawMessage or map[string]json.RawMessage) and handle empty-vs-missing explicitly.
| // Pre-scan the JSON to check how many model_list entries the user provided. | |
| // Go's JSON decoder reuses existing slice backing-array elements rather than | |
| // zero-initializing them, so fields absent from the user's JSON (e.g. api_base) | |
| // would silently inherit values from the DefaultConfig template at the same | |
| // index position. We only reset cfg.ModelList when the user actually provides | |
| // entries; when count is 0 we keep DefaultConfig's built-in list as fallback. | |
| // Pre-scan the JSON to check whether the user provided any model_list entries. | |
| // Go's JSON decoder reuses existing slice backing-array elements rather than | |
| // zero-initializing them, so fields absent from the user's JSON (e.g. api_base) | |
| // would silently inherit values from the DefaultConfig template at the same | |
| // index position. We only reset cfg.ModelList when the user actually provides | |
| // one or more entries. If model_list is omitted entirely, DefaultConfig's | |
| // built-in list is kept as a fallback; if model_list is present (even if empty | |
| // or null), the subsequent unmarshal will override and disable that fallback. |
| // Pre-scan the JSON to check how many model_list entries the user provided. | ||
| // Go's JSON decoder reuses existing slice backing-array elements rather than | ||
| // zero-initializing them, so fields absent from the user's JSON (e.g. api_base) | ||
| // would silently inherit values from the DefaultConfig template at the same | ||
| // index position. We only reset cfg.ModelList when the user actually provides | ||
| // entries; when count is 0 we keep DefaultConfig's built-in list as fallback. | ||
| var tmp Config | ||
| if err := json.Unmarshal(data, &tmp); err != nil { | ||
| return nil, err | ||
| } | ||
| if len(tmp.ModelList) > 0 { | ||
| cfg.ModelList = nil | ||
| } | ||
|
|
||
| if err := json.Unmarshal(data, cfg); err != nil { |
There was a problem hiding this comment.
This change fixes a subtle encoding/json slice-reuse bug but currently has no direct regression test. Please add a LoadConfig unit test that writes a config with model_list containing a single entry that omits api_base, calls LoadConfig, and asserts the resulting cfg.ModelList[0].APIBase is empty (and does not inherit the DefaultConfig template value). A second assertion for the “model_list omitted” case preserving DefaultConfig can help lock in the intended fallback behavior.
| func (c *Config) GetModelConfig(modelName string) (*ModelConfig, error) { | ||
| matches := c.findMatches(modelName) | ||
| if len(matches) == 0 { | ||
| return nil, fmt.Errorf("model %q not found in model_list or providers", modelName) | ||
| } |
There was a problem hiding this comment.
GetModelConfig reports "not found in model_list or providers", but findMatches only searches ModelList and does not consult Providers (nor does it trigger conversion). This makes the error message misleading; either update it to only mention model_list, or extend the lookup to include a providers->model_list fallback if that behavior is desired.
|
LGTM ✅ |
yinwm
left a comment
There was a problem hiding this comment.
LGTM - high quality bug fix. Accurately identifies a tricky Go encoding/json behavior and fixes it correctly with clear fallback semantics.
|
thanks for the pr |
|
@mqyang56 That DefaultConfig leak was a sneaky one. Slice reuse during JSON decode silently inheriting template values at the same index is the kind of bug that's easy to miss. Clean fix with the pre-scan approach. We're building a PicoClaw Dev Group on Discord for contributors to connect and collaborate. If you're interested, drop an email to |
…lue-leak fix: prevent DefaultConfig template values from leaking into user model_list entries
Summary
Fixes #721
When
DefaultConfig()pre-populatesModelListwith ~18 template entries, Go's JSON decoder reuses the existing slice backing-array elements rather than zero-initializing them. This means fields absent from the user's JSON (e.g.api_base) silently inherit the template value at the same index position — for example,ModelList[0]is the Zhipuglm-4.7entry withAPIBase: "https://open.bigmodel.cn/api/paas/v4", which leaks into any user-defined model at index 0 that omitsapi_base.Fix: pre-scan the JSON into a zero-value
Configto count user-providedmodel_listentries. Only resetcfg.ModelList = nilwhen the user provides >0 entries, ensuring clean zero-value initialization during the real unmarshal. When the user provides 0 entries, theDefaultConfigbuilt-in list is preserved as a fallback.Behavior change
model_listhas 0 entriesmodel_listhas ≥1 entries, some fields omittedTest plan
pkg/configtests passgo build ./...)model_listentry with noapi_base→ confirm the model entry has emptyapi_baseinstead of the Zhipu template URL🤖 Generated with Claude Code