feat(config): support shared top-level providers#387
feat(config): support shared top-level providers#387huangdijia wants to merge 1 commit intochenhg5:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared, top-level [[providers]] configuration model so provider definitions can be reused across projects, with legacy per-project provider blocks still readable as a fallback.
Changes:
- Adds
Config.Providersand “effective providers” resolution (top-level first, legacy per-project fallback), plus validation for duplicate shared provider names and invalid active-provider references. - Updates provider persistence and startup/hot-reload wiring to use the effective-provider resolver and to write to top-level providers (with promotion from legacy on first write).
- Refreshes docs/examples and expands tests to cover top-level precedence and legacy fallback.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/usage.md | Updates examples/docs to prefer top-level [[providers]] and [[providers.models]]. |
| docs/usage.zh-CN.md | Same as above for Chinese docs, with an added explanation of shared providers + legacy compatibility. |
| config/config.go | Adds Config.Providers, effective-provider helpers, new validation, and updates provider CRUD/persistence to target top-level providers. |
| config/config_test.go | Adds/updates tests for top-level provider validation, precedence, fallback, and promotion-on-write behavior. |
| config.example.toml | Updates commented examples to show top-level [[providers]] as the recommended format. |
| cmd/cc-connect/provider.go | Updates CLI help text/output to reflect shared providers and “effective providers” listing. |
| cmd/cc-connect/main.go | Refactors provider wiring (startup + reload) through a shared configureAgentProviders helper using effective providers. |
| cmd/cc-connect/main_test.go | Adds tests ensuring wiring and reload use top-level providers (with legacy fallback). |
Comments suppressed due to low confidence (1)
config/config.go:595
- RemoveProviderFromConfig can remove a shared provider without updating any project’s agent.options.provider that references it. With the new validation that active provider must exist in the effective provider list, this can leave the config in an unloadable state on next restart/reload. Consider either refusing removal when any project has this provider active, or clearing/updating those projects’ provider selection as part of the removal.
if err := ensureTopLevelProvidersForProject(cfg, projectName); err != nil {
return err
}
found := false
for i := range cfg.Providers {
if cfg.Providers[i].Name == providerName {
cfg.Providers = append(cfg.Providers[:i], cfg.Providers[i+1:]...)
found = true
break
}
}
if !found {
return fmt.Errorf("provider %q not found in project %q", providerName, projectName)
}
return saveConfig(cfg)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(cfg.Providers) > 0 { | ||
| return nil | ||
| } | ||
| cfg.Providers = cloneProviderConfigs(cfg.effectiveProvidersForProject(*proj)) |
There was a problem hiding this comment.
ensureTopLevelProvidersForProject promotes legacy per-project providers into cfg.Providers, but neither this function nor saveConfig validates the resulting top-level provider list. If a legacy config contains duplicate provider names (or other now-invalid state), provider write operations can successfully persist a config that later fails config.Load() due to the new duplicate-name validation. Consider validating (e.g., check firstDuplicateProviderName after promotion, or run cfg.validate()) before returning nil so provider commands don’t brick the config file.
| cfg.Providers = cloneProviderConfigs(cfg.effectiveProvidersForProject(*proj)) | |
| promoted := cloneProviderConfigs(cfg.effectiveProvidersForProject(*proj)) | |
| if dup := firstDuplicateProviderName(promoted); dup != "" { | |
| return fmt.Errorf("invalid provider configuration: duplicate provider name %q after promotion", dup) | |
| } | |
| cfg.Providers = promoted |
| // AddProviderToConfig adds a provider to a project's agent config and saves. | ||
| func AddProviderToConfig(projectName string, provider ProviderConfig) error { | ||
| configMu.Lock() | ||
| defer configMu.Unlock() | ||
| if ConfigPath == "" { | ||
| return fmt.Errorf("config path not set") | ||
| } | ||
| data, err := os.ReadFile(ConfigPath) | ||
| if err != nil { | ||
| return fmt.Errorf("read config: %w", err) | ||
| } | ||
| cfg := &Config{} | ||
| if err := toml.Unmarshal(data, cfg); err != nil { | ||
| return fmt.Errorf("parse config: %w", err) | ||
| } | ||
|
|
||
| found := false | ||
| for i := range cfg.Projects { | ||
| if cfg.Projects[i].Name == projectName { | ||
| for _, existing := range cfg.Projects[i].Agent.Providers { | ||
| if existing.Name == provider.Name { | ||
| return fmt.Errorf("provider %q already exists in project %q", provider.Name, projectName) | ||
| } | ||
| } | ||
| cfg.Projects[i].Agent.Providers = append(cfg.Projects[i].Agent.Providers, provider) | ||
| found = true | ||
| break | ||
| } | ||
| if err := ensureTopLevelProvidersForProject(cfg, projectName); err != nil { | ||
| return err | ||
| } | ||
| if !found { | ||
| return fmt.Errorf("project %q not found in config", projectName) | ||
| for _, existing := range cfg.Providers { | ||
| if existing.Name == provider.Name { | ||
| return fmt.Errorf("provider %q already exists in project %q", provider.Name, projectName) | ||
| } | ||
| } | ||
| cfg.Providers = append(cfg.Providers, provider) | ||
| return saveConfig(cfg) |
There was a problem hiding this comment.
AddProviderToConfig/RemoveProviderFromConfig/SaveProviderModel still return errors like "provider ... not found in project ..." and "already exists in project ...", but these operations now act on the shared top-level provider list. Updating these error messages to reference the shared/top-level provider set would make failures less misleading for users.
chenhg5
left a comment
chenhg5
left a comment
There was a problem hiding this comment.
LGTM. Well-designed shared providers feature.
Review summary:
- ✅ Top-level
[[providers]]reduces config duplication - ✅ Legacy per-project providers still supported for backward compatibility
- ✅ Proper validation (duplicate names, invalid references)
- ✅ Tests and docs updated
- ✅ CI passes
Great improvement for multi-project setups.
Summary
[[providers]]support for shared provider definitions across projectsBackground
agent.options.provider, so the config shape could be simplified around shared provider definitionsChanges
Config.Providersand effective-provider resolution helpers inconfigconfig.example.toml,docs/usage.md, anddocs/usage.zh-CN.mdto prefer top-level[[providers]]Test Plan
go test ./config -run 'Test(ConfigValidate|ProviderConfig|SaveAgentModel|Load)'go test ./cmd/cc-connect -run 'Test(ConfigureAgentProviders|ReloadConfigUsesTopLevelProviders|ProjectStatePath|ApplyProjectStateOverride)'go test ./...currently still has existing unrelated failures inagent/acp,agent/codex,core, andplatform/wecomin this environmentRisks
[[providers]]exists, legacy[[projects.agent.providers]]is ignored by design