feat(config): support viper multi-format config loading and CLI confi…#484
feat(config): support viper multi-format config loading and CLI confi…#484sky5454 wants to merge 1 commit intosipeed:mainfrom
Conversation
…g override - replace json-only load/save with viper-based multi-format support (json/yaml/toml/ini) - add config path resolution for ~/.picoclaw with extension fallback and explicit --config/-c override - keep agent model backward compatibility (string/object forms) via decoder hooks - add regression tests for yaml/json loading, provider api_key decode, and config path resolution - add config examples and update docs across README translations
|
This is the way the command line specifies configuration files for the time being, and it is better for the owner to support better uniform parameter parsing in the future. |
nikolasdehor
left a comment
There was a problem hiding this comment.
Interesting approach using viper for multi-format config support. A few observations:
Positives:
- The
ResolveConfigPathfunction with extension fallback is well-designed — it tries json/yaml/toml/ini in order. - The
--config/-cCLI flag is a good addition for explicit path override. - Backward compatibility is maintained via the
AgentModelConfigdecoder hook that handles both string and object forms. - Good test coverage:
loadconfig_agentmodel_test.go,path_resolution_test.go,viper_config_test.go. - The
build.batfor Windows is a nice bonus.
Concerns:
-
New dependency weight: Adding
spf13/viper+go-viper/mapstructure/v2is a significant dependency addition for a project that aims to be ultra-lightweight (<10MB). Viper pulls in a large transitive dependency tree (pflag, fsnotify, cast, etc.). Has the binary size impact been measured? -
MarshalJSONremoval: TheAgentModelConfig.MarshalJSONmethod was deleted, butSaveConfigstill needs to produce valid output. With viper handling the write path, does saving a config that was loaded from YAML produce correct JSON output if the user has aconfig.json? The format detection inSaveConfigbased onfilepath.Ext(path)looks correct, but this is worth testing explicitly. -
INI format limitations: INI does not natively support nested structures or arrays. How does viper handle
allow_fromlists and nestedagents.defaultsin INI format? The example file uses flat keys likeallow_from=YOUR_USER_IDbut the real config expects[]string. This might silently produce incorrect config. -
mapstructure:"model_fallbacks,omitempty": Theomitemptytag on mapstructure fields is not standard — mapstructure does not supportomitempty. This will be silently ignored but is misleading. -
Deleted
config_test.gotests: 24 lines of tests were deleted. Are the replacement tests in the new test files covering the same cases?
Would like to see the INI array handling tested explicitly before merging.
…g override
replace json-only load/save with viper-based multi-format support (json/yaml/toml/ini)
add config path resolution for ~/.picoclaw with extension fallback and explicit --config/-c override
keep agent model backward compatibility (string/object forms) via decoder hooks
add regression tests for yaml/json loading, provider api_key decode, and config path resolution
add config examples and update docs across README translations
📝 Description
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
📚 Technical Context (Skip for Docs)
🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist