Skip to content

fix: prevent DefaultConfig template values from leaking into user model_list entries #724

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yinwm merged 1 commit into from
Feb 24, 2026
Merged

fix: prevent DefaultConfig template values from leaking into user model_list entries #724

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,20 @@ func LoadConfig(path string) (*Config, error) {
return nil, err
}

// 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.
Comment on lines +502 to +507
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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.

Copilot uses AI. Check for mistakes.
var tmp Config
if err := json.Unmarshal(data, &tmp); err != nil {
return nil, err
}
if len(tmp.ModelList) > 0 {
Comment on lines +508 to +512
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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 {

Copilot uses AI. Check for mistakes.
cfg.ModelList = nil
}

if err := json.Unmarshal(data, cfg); err != nil {
Comment on lines +502 to 516
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
return nil, err
}
Expand Down
Loading