Update default anthropic model to sonnet 4.5#1580
Conversation
…ne their own default model in their global config Signed-off-by: Christopher Petito <chrisjpetito@gmail.com>
667c164 to
f335c41
Compare
|
/review |
There was a problem hiding this comment.
Review Summary
Found 2 issues in the new code that should be addressed:
- Silent failure when parsing invalid
CAGENT_DEFAULT_MODEL- Users setting an invalid environment variable format won't receive any feedback - Potential invalid YAML output - Empty model configs could produce invalid shorthand format
The core functionality looks good, but these edge cases should be handled more gracefully.
|
|
||
| // Precedence for default model: environment variable > user config | ||
| if model := os.Getenv(envDefaultModel); model != "" { | ||
| runConfig.DefaultModel = parseModelShorthand(model) |
There was a problem hiding this comment.
Silent failure when parsing invalid environment variable
When CAGENT_DEFAULT_MODEL is set to an invalid format (e.g., "anthropic" without /model, or "/model" without provider), parseModelShorthand returns nil silently with no warning or error. This means:
- The user's explicit configuration is ignored without any feedback
- The system falls back to auto-detection or user config without the user knowing
- Users may waste time debugging why their environment variable isn't working
Recommendation: Add a warning log when the environment variable is set but parsing fails:
if model := os.Getenv(envDefaultModel); model != "" {
runConfig.DefaultModel = parseModelShorthand(model)
if runConfig.DefaultModel == nil {
slog.Warn("Invalid CAGENT_DEFAULT_MODEL format, expected 'provider/model'", "value", model)
}
} else if userCfg.DefaultModel != nil {
runConfig.DefaultModel = &userCfg.DefaultModel.ModelConfig
}This gives users immediate feedback when they've misconfigured the environment variable.
| if f.isShorthandOnly() { | ||
| return yaml.Marshal(f.Provider + "/" + f.Model) | ||
| } | ||
| return yaml.Marshal(f.ModelConfig) |
There was a problem hiding this comment.
MarshalYAML can output invalid shorthand format
isShorthandOnly() only checks if advanced fields are unset, but doesn't verify that Provider and Model are non-empty. If a FlexibleModelConfig is created with zero values (empty Provider and Model), isShorthandOnly() would return true, causing MarshalYAML to output "/" (empty + "/" + empty), which is invalid.
Recommendation: Add validation in isShorthandOnly() to ensure both fields are non-empty:
func (f *FlexibleModelConfig) isShorthandOnly() bool {
return f.Provider != "" &&
f.Model != "" &&
f.Temperature == nil &&
f.MaxTokens == nil &&
// ... rest of checks
}This ensures the shorthand format is only used when a valid provider/model combination exists.
And allow users to define their own default model in their global config
Both full model def and shorthand formats are supported
Examples of global config.yaml