Structured per-provider config block, non-destructive provider switching - #8977
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b392bd365c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42f48d1bc7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89ab45b81c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c89513f1ac
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef0b302ac9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ef0b302 to
e338767
Compare
Switching providers previously overwrote GOOSE_PROVIDER and GOOSE_MODEL, losing the previous provider's settings. This adds a `providers:` config block that preserves each provider's model and configured state across switches, with a migration from the old flat-key layout. Fixes aaif-goose#6692 & aaif-goose#8373
- Apply cargo fmt (rustfmt 1.92) formatting - Remove needless borrows flagged by clippy (needless_borrows_for_generic_args) - Regenerate openapi.json and types.gen.ts to match ProviderDetails schema - Fix test_lower_priority_values_not_persisted_on_write: migration in load() was saving the merged multi-layer config, leaking base-layer values into the user config file. Removed; load_write_config() already handles it. - Fix test_multi_path_config_loading: same root cause (migration in load() converted flat keys to structured block, then get_param couldn't find them) - Intercept GOOSE_PROVIDER/GOOSE_MODEL in /config/remove so deletions clear structured state, not just the (now-absent) flat key - Use get_goose_provider() in on_delete_custom_provider so the active-provider guard works with structured storage - Switch posthog.rs to get_goose_provider()/get_goose_model() so analytics reports the effective provider - Verified .deb builds and installs cleanly with all changes included.
- Fix clippy::bind_instead_of_map in set_config_provider (pre-existing issue) - Split migrations: load() runs only non-destructive extensions migration in-memory (so read paths see platform extensions); provider migration runs only in load_write_config() where it persists to disk - Preserve target provider's saved model on GOOSE_PROVIDER upsert instead of copying the current active model into the new entry - Verified .deb builds and installs cleanly with all changes included
…ders check_provider_configured no longer blindly trusts the structured `configured` flag for providers with required keys; only trusted for OAuth and zero-config providers where there's no key to validate. Note: set_active_provider() swallows write errors (Codex P2); deferring — changing the return type to Result touches 12+ call sites. Verified .deb builds and installs cleanly with all changes included.
set_active_provider and set_provider_entry now return Result<(), ConfigError> instead of swallowing errors with warn!. All call sites updated to propagate. Signed-off-by: Douwe Osinga <douwe@squareup.com>
e338767 to
0e38705
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e38705020
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Working on a solution. |
Rebase the structured per-provider config changes onto current upstream main, resolving conflicts from the sacp→agent_client_protocol rename and the preferences/defaults API refactor. Fixes beyond the mechanical resync: - config.rs: on_defaults_read uses get_goose_provider()/get_goose_model() instead of reading legacy flat keys directly - config.rs: on_defaults_save calls set_active_provider() instead of writing flat GOOSE_PROVIDER/GOOSE_MODEL keys - config.rs: remove unused optional_config_string function - onboarding.rs: apply_goose_config_candidate uses set_active_provider() and test asserts via get_goose_provider() - builder.rs: remove redundant get_active_provider/get_active_model calls already handled inside get_goose_provider/get_goose_model All 1363 tests pass, cargo fmt clean, clippy clean. Refs: aaif-goose#6692, aaif-goose#8373 Signed-off-by: Aaron Yourk <ayourk@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9acf243846
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…migration cleanup
Guard onboarding import against model-only configs: skip provider
activation when GOOSE_PROVIDER is absent instead of creating an
empty-string provider entry via set_active_provider("", model).
Backfill active_provider from legacy GOOSE_PROVIDER during migration
cleanup: when a providers block exists but active_provider is missing,
preserve the legacy value before deleting flat keys so
get_goose_provider() continues to resolve after migration.
Refs: Codex P2 "Require provider before writing imported defaults",
Codex P2 "Migrate legacy active provider before cleanup-only path"
Signed-off-by: Aaron Yourk <ayourk@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 141cb17fbe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Stop swallowing config.delete() errors in the /config/remove handler for GOOSE_PROVIDER, active_provider, and GOOSE_MODEL. delete() is already a no-op when the key is absent (shift_remove + save), so the only errors it returns are genuine I/O failures that callers need to see. Refs: Codex P2 "Propagate remove failures for virtual provider keys" Signed-off-by: Aaron Yourk <ayourk@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfd0776554
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e27fb8964f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
DOsinga
added
the
needs_human
When an imported config has GOOSE_PROVIDER but not GOOSE_MODEL, fall back to the target provider's already-saved model instead of defaulting to "" and overwriting it. Refs: Codex P2 "Preserve existing model when import omits GOOSE_MODEL" Signed-off-by: Aaron Yourk <ayourk@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f1ca43fea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
set_provider_entry read the providers map via get_param (read path, no provider migration) then wrote via set_param (write path, runs migration). On a legacy config this produced an empty map that overwrote the freshly migrated providers block with a single entry. Add Config::update_param for atomic read-modify-write through the write path, and use it in set_provider_entry so the closure receives the post-migration providers map. Refs: Codex P1 "Merge provider writes from migrated writable state" Signed-off-by: Aaron Yourk <ayourk@gmail.com>
Audit every set_active_provider call site for empty-model overwrites
and fix the remaining cases:
- config_management.rs: GOOSE_PROVIDER upsert on legacy configs falls
back to get_goose_model() when get_provider_entry() returns None,
preserving the legacy active model through migration
- config.rs: on_defaults_save with model_id=None reads the provider's
existing entry model instead of defaulting to ""
- base.rs: set_goose_provider preserves the target provider's saved
model instead of copying the current active model
- signup_nanogpt: set default model on signup instead of ""
Refs: Codex P1 "Preserve legacy active model during provider upsert",
Codex P2 "Keep saved model when defaults save omits model_id",
jamadeo review comment on signup_nanogpt/mod.rs:119
Signed-off-by: Aaron Yourk <ayourk@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbcb91cc0d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 930beb6d27
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29a2b50156
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…sponse After migration strips GOOSE_PROVIDER and GOOSE_MODEL from persisted YAML, the /config bulk-read endpoint (which calls all_values()) no longer returns those keys. The desktop UI reads typedConfig.GOOSE_PROVIDER from this response to determine the active provider, so it falls back to empty after the first migration write. Inject GOOSE_PROVIDER and GOOSE_MODEL into the all_values() map by calling the existing get_goose_provider() and get_goose_model() getters that synthesize these values from the structured providers block.
29a2b50 to
637856c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 637856c07c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9649752eb0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
When configure_provider_oauth runs against a legacy config with no providers block, get_provider_entry returns None and the fallback creates a ProviderEntry with an empty model. Because set_provider_entry triggers a write-path migration first, the user's existing model gets migrated into the structured block and then immediately overwritten with empty string. Read the active model via get_goose_model() before constructing the fallback entry so the migrated value is preserved.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35f7a3d54a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The previous fix (2aa870a) preserved the migrated model via get_goose_model(), but that copies the *active* provider's model into any provider being configured — wrong when authenticating provider B while provider A is active. Root cause is a read-write skew: get_provider_entry() reads through load() (no provider migration), while set_provider_entry() writes through load_write_config() (runs migration). The else branch fires because the read sees pre-migration state, but whatever model we construct gets written post-migration, overwriting migration's result. Neither String::new() nor get_goose_model() is universally correct: - String::new(): correct for cross-provider, loses model for same - get_goose_model(): correct for same-provider, copies wrong model cross-provider Fix: branch on whether the provider being configured matches the active provider. Same provider preserves the active model (matching what migration creates); different provider starts with empty model (user hasn't chosen one yet).
PR #8977 introduced structured per-provider config (`providers:` block + `active_provider` key). Wire these into GooseConfigSchema so they flow through the typed API endpoints and both JSON schemas. GOOSE_PROVIDER/GOOSE_MODEL reads now use the resolution-chain accessors (env → active_provider → legacy flat key), and writes route through set_active_provider to keep the providers block canonical. Signed-off-by: Will Pfleger <wpfleger@block.xyz>
* origin/main: (160 commits) Add Linux musl CLI builds (#9240) feat(acp): paginate session list (#9199) docs: reorganize (#9310) Structured per-provider config block, non-destructive provider switching (#8977) feat(cli): add `goose review` local code review command (#9114) feat(tui): diff viewer (#9260) fix(otel): emit trace_output as span attribute instead of event (#9255) docs: add guide for connecting goose Desktop to a remote goosed server (#9275) fix(config): check file fallback when keyring has no entry (#9279) fix(desktop): ScheduleModal error message styling (#9278) fix(ui): align sidebar hamburger in macOS fullscreen (#9257) Add documentation for new provider SaladCloud AI Gateway (#9253) fix: use current_exe() instead of PATH lookup when spawning goose (#9236) fix(extension_manager): set TCP_USER_TIMEOUT on streamable HTTP clients (#9207) fix: activate custom provider after adding via configure (#9213) Flush OTLP traces reliably on exit with configurable timeout (#9228) fix: reduce excessive MISSING_TRANSLATION warnings for fallback locales (#9294) feat(acp): pass session cwd param to acp providers (#9229) fix(desktop): eliminate cross-window deep link contamination (#9273) fix: improve Telegram gateway error reporting and connection reliability (#9223) ... Signed-off-by: Michael Neale <michael.neale@gmail.com> # Conflicts: # crates/goose/src/agents/agent.rs # crates/goose/tests/agent.rs
* main: (70 commits) Feat/summon subagent instructions (#9325) feat: open-plugins generalization + skills (#9112) feat(hooks): PreToolUse denial (#9304) Add support for optional api_key configuration for declarative openai-engine providers (#9202) fix(cli): use plain '> ' prompt instead of goose emoji (#9305) flag for login shell PATH (#9313) Remove popular chat topics from new chat screen (#9307) fix: stop killing goosed when a window closes (#9302) Remove vendored Windows binaries (#9318) Add Linux musl CLI builds (#9240) feat(acp): paginate session list (#9199) docs: reorganize (#9310) Structured per-provider config block, non-destructive provider switching (#8977) feat(cli): add `goose review` local code review command (#9114) feat(tui): diff viewer (#9260) fix(otel): emit trace_output as span attribute instead of event (#9255) docs: add guide for connecting goose Desktop to a remote goosed server (#9275) fix(config): check file fallback when keyring has no entry (#9279) fix(desktop): ScheduleModal error message styling (#9278) fix(ui): align sidebar hamburger in macOS fullscreen (#9257) ...
…ing (aaif-goose#8977) Signed-off-by: Douwe Osinga <douwe@squareup.com> Signed-off-by: Aaron Yourk <ayourk@gmail.com> Co-authored-by: Douwe Osinga <douwe@squareup.com>
PR aaif-goose#8977 introduced structured per-provider config (`providers:` block + `active_provider` key). Wire these into GooseConfigSchema so they flow through the typed API endpoints and both JSON schemas. GOOSE_PROVIDER/GOOSE_MODEL reads now use the resolution-chain accessors (env → active_provider → legacy flat key), and writes route through set_active_provider to keep the providers block canonical. Signed-off-by: Will Pfleger <wpfleger@block.xyz>
3 participants
Summary
Switching providers destructively overwrote
GOOSE_PROVIDER/GOOSE_MODEL,losing the previous provider's settings (#6692). Zero-config ACP providers
(claude-acp, codex-acp, etc.) never got their
_configuredmarker set,so they didn't appear in the model switcher (#8373).
This PR adds a structured
providers:config block (similar toextensions:)that preserves each provider's model and configured state independently.
An automatic migration converts the old flat-key layout on first load.
Old format (flat keys, destructive on switch):
New format (per-provider, non-destructive):
Key changes:
crate::config::providersmodule withProviderEntry, read/write helpers, and active-provider accessorsget_goose_provider()/get_goose_model()now resolve through the structured block first, falling back to legacy flat keys/config/readand/config/upsertinterceptGOOSE_PROVIDER/GOOSE_MODELkeys so existing frontend code works unchangedGOOSE_PROVIDER/GOOSE_MODELenv vars →active_provider/providers.{name}.modelin config → legacy flat keys in configGOOSE_PROVIDER,GOOSE_MODEL,*_configured) on loadProviderDetailsAPI response includessaved_modelso the model switcher can pre-populateTETRATE_PROVIDER_NAME, etc.) madepubto avoid literal string duplication across crates (if you'd prefer a different approach, happy to adjust)Questions for maintainers:
ui/desktop/src/api/types.gen.tsis committed (not gitignored) but auto-generated bypnpm run generate-api. This PR hand-edits it to addsaved_model?toProviderDetails. Should this be regenerated from the OpenAPI spec instead, and if so, how much of the non-generatedtypes.gen.tscontent is expected to be hand-maintained? In my experience, generated files typically don't get tracked.This PR only moves
model,enabled, andconfiguredinto the per-provider block — the minimum needed for non-destructive switching. Are there other per-provider settings (e.g. thinking params, context limits, request params) that should also move to this block?CUSTOM_DISTROS.mdhas config file examples using the old flat-key format (GOOSE_PROVIDER,GOOSE_MODEL). These still work through the fallback chain, but the examples could be updated to show the newproviders:/active_provider:format. I would be willing to update those references in this PR or a follow-up, whichever you prefer.Testing
providers.rs(read/write, fallback chains, multi-provider preservation)merge_config_valueswith providers blockRelated Issues
Fixes #6692
Fixes #8373