feat(cli): /model with no argument opens a cross-provider model picker - #9658
feat(cli): /model with no argument opens a cross-provider model picker#9658jlmalone wants to merge 24 commits into
/model with no argument opens a cross-provider model picker#9658Conversation
Bare /model now lists models from every configured provider (local LM Studio, NVIDIA, etc.) in one searchable menu and switches the live session's provider and model on selection. /model <name> keeps its existing within-provider behavior. pick_and_switch_model enumerates configured providers, fetches each provider's models (15s timeout, skip on error), aggregates into one list, reuses the configure fuzzy-search UI, then create()+update_provider. provider_is_configured gates the menu to providers whose required keys resolve via env/secret/default. interactive_model_search is now pub(crate) for reuse. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
![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: 2ccfac992e
ℹ️ 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".
|
|
||
| for (meta, _ptype) in &all { | ||
| let include = | ||
| meta.name.as_str() == current_provider_name || provider_is_configured(meta); |
There was a problem hiding this comment.
In a normal install with no AWS credentials, this predicate still treats Amazon Bedrock as configured because its required AWS_REGION key has a built-in default in crates/goose/src/providers/bedrock.rs, and Bedrock's fetch_supported_models returns a static list without validating credentials. That makes bare /model show Bedrock models and allows switching the live session to an unusable provider, with the failure deferred until the next prompt; the picker should require an actual provider configuration/credential signal rather than counting defaults alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3a28adf. A built-in default is now treated as configuration, not a credential: a provider that authenticates with a secret (API key / bearer token) must have at least one real secret value to be offered in /model — so Bedrock no longer appears merely because AWS_REGION has a default. Providers with no secret keys (LM Studio, Ollama, and other local no-auth backends) declare no secrets and stay always available. Added a regression test covering the Bedrock-like, local, and credentialed cases.
provider_is_configured() counted a required key satisfied by its built-in default as configured, so the /model picker offered cloud providers with no usable credentials -- e.g. Bedrock, whose required AWS_REGION has a default. Treat a built-in default as configuration, not a credential: a provider with secret keys must have at least one real secret value, while local no-auth providers (LM Studio, Ollama) declare no secrets and stay available. Adds a regression test. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a28adf74d
ℹ️ 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".
| let secret_keys: Vec<_> = meta.config_keys.iter().filter(|k| k.secret).collect(); | ||
| secret_keys.is_empty() || secret_keys.iter().any(|k| has_value(k)) |
There was a problem hiding this comment.
When OpenAI is configured only for a local OpenAI-compatible endpoint, e.g. OPENAI_BASE_URL or a non-default OPENAI_HOST with no OPENAI_API_KEY, OpenAiProvider::from_env can use AuthMethod::NoAuth, but this predicate rejects it because OpenAI declares optional secret keys and none have a value. If the current session is on another provider, bare /model will omit that working local OpenAI provider, which is one of the advertised switch targets; consider treating non-secret provider config as sufficient or using the provider's inventory_configured().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real gap — and it's in direct tension with the Bedrock fix above. provider_is_configured was tightened to require an actual secret value precisely so default-only cloud providers (Bedrock via its built-in AWS_REGION default) stop appearing as usable; that same strictness now also excludes a NoAuth local OpenAI endpoint (OPENAI_BASE_URL / non-default OPENAI_HOST, no key). Rather than loosen the predicate here and risk re-introducing the Bedrock case, the right fix is to gate on each provider's own configured signal (inventory_configured() / a NoAuth from_env). Tracked in #9822.
| match tokio::time::timeout( | ||
| Duration::from_secs(15), | ||
| temp_provider.fetch_supported_models(), | ||
| ) |
There was a problem hiding this comment.
When a configured Claude Code provider is installed but its model_list control request hangs or exceeds 15s, this timeout cancels ClaudeCodeProvider::fetch_supported_models while it is waiting; that function spawned a child from build_stream_json_command() and only calls child.kill().await after exchange_control returns, with no kill_on_drop, so the timed-out /model picker can leave a claude process running. Either make subprocess model listing cancellation-safe or avoid wrapping it in this timeout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. When the 15s list timeout cancels fetch_supported_models() for a Claude Code provider, the spawned claude child can outlive it because the command isn't kill_on_drop. The picker can't reap a child it doesn't own, so the durable fix belongs in ClaudeCodeProvider (kill_on_drop(true) on the command, or a cancellation-safe control request) rather than wrapping it differently from the picker side. Tracked in #9822.
…noise The /model picker now avoids landing on models that do not actually work: - Probes the chosen model with a 30s-bounded request before committing the switch. Providers can list catalog models the account cannot run (e.g. NVIDIA returns 404 'function not found'); the picker now reports that and keeps the current model instead of silently switching to a dead one. - Collapses the wall of per-provider 'skipping / could not list' warnings (uninstalled ACP commands, unreachable local servers) into a single 'Skipped N provider(s) not available here' line. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6be91f44ae
ℹ️ 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".
| let probe_msg = [Message::user().with_text("ok")]; | ||
| match tokio::time::timeout( | ||
| Duration::from_secs(30), | ||
| new_provider.complete(&probe_config, "model-check", "", &probe_msg, &[]), |
There was a problem hiding this comment.
When the selected provider is an ACP provider such as claude-acp or codex-acp, this probe runs a real complete on the provider instance that will later be installed. AcpProvider::stream marks its first prompt as the one-time handoff-context opportunity, so the synthetic "ok" prompt consumes that state before update_provider; the user's first real prompt after switching is then treated as a later ACP prompt and the existing conversation history is not sent. Skip this probe for providers that manage their own context, or recreate the provider after probing before installing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f7eaa6a. The probe and the to-be-installed provider were the same instance, so the synthetic complete("ok") consumed an ACP provider's one-time handoff-context opportunity before update_provider — the user's first real prompt then dropped the existing history. The picker now probes a throwaway instance, drops it, and creates a fresh provider for the actual switch (ModelConfig/ExtensionConfig are Clone, so the second create() reuses the same config). Verified with cargo check/clippy -p goose-cli.
Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
The probe-before-switch ran complete() on the same provider instance that then gets installed. An ACP provider (claude-acp/codex-acp) treats its first prompt as the one-time handoff-context opportunity, so the synthetic probe consumed it and the user's first real prompt after switching dropped the existing conversation history. Probe a throwaway instance, drop it, and install a fresh one. Addresses the Codex P2 on the probe path. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7eaa6abf4
ℹ️ 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".
| if model.is_none() { | ||
| output::goose_mode_message(&format!( | ||
| "Current session model: '{}' (provider '{}')", | ||
| current_model_name, current_provider_name | ||
| )); | ||
| return Ok(()); | ||
| return self | ||
| .pick_and_switch_model( | ||
| ¤t_provider_name, | ||
| ¤t_model_config, |
There was a problem hiding this comment.
When the current provider is Claude Code, Gemini CLI, or an ACP provider, this new no-argument path enters the picker before the existing provider.manages_own_context()/-acp checks below run. Those providers keep the authoritative conversation state in the subprocess, which is why /model <name> is rejected; allowing the picker to switch away from them can make the next provider continue from only Goose's partial transcript and lose hidden CLI/ACP context. Apply the same guard before opening the picker for these current providers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ab9e2ae. The -acp and manages_own_context() guards now run in handle_model before the model.is_none() dispatch, so bare /model is rejected for a context-managed current provider exactly like /model <name> already was. The picker can no longer switch away from Claude Code / Gemini CLI / an ACP provider and strand the conversation state they hold in their subprocess.
| match tokio::time::timeout( | ||
| Duration::from_secs(30), | ||
| probe_provider.complete(&probe_config, "model-check", "", &probe_msg, &[]), | ||
| ) |
There was a problem hiding this comment.
If the selected provider is an ACP provider and this probe prompt hangs, the 30s timeout cancels the complete future, but dropping the AcpProvider still synchronously joins its client loop while that loop is waiting on the in-flight ACP PromptRequest. In that scenario the command can remain stuck after reporting/attempting the timeout instead of returning to the session; skip probing providers that manage their own context or make the ACP prompt cancellation path shut down the subprocess before joining.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ab9e2ae by not probing context-managed targets at all. The probe exists only to catch cloud providers that list models the account can't actually run (e.g. NVIDIA 404s); ACP / Claude Code / Gemini CLI don't have that failure mode, and as you note a hung probe would wedge on the client-loop join at drop. The picker now skips the probe when the chosen provider is -acp or manages_own_context() and installs it directly. That also let me drop the throwaway-instance workaround from the previous commit — once we never probe these providers, the handoff-context concern is moot too, so it's a single instance again.
Two issues Codex flagged on the probe path, both about providers that keep authoritative conversation state in a subprocess (ACP, Claude Code, Gemini CLI): - Bare /model entered the picker before the existing manages_own_context()/-acp guard that already rejects /model <name> for these providers, so the picker could switch away from them and continue the next provider from only Goose's partial transcript. Hoist the guard ahead of the picker dispatch. - Probing a selected context-managed provider could consume its one-time handoff context or, if the probe hung, wedge the cancellation path while the ACP client loop is joined on drop. Skip the probe for those providers (they have no dead-catalog-model failure mode); probe only the cloud/API providers it was meant for. Also removes the throwaway-instance dance the previous commit added. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab9e2ae750
ℹ️ 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".
| let include = | ||
| meta.name.as_str() == current_provider_name || provider_is_configured(meta); |
There was a problem hiding this comment.
When a user has already signed in to an OAuth-backed provider such as ChatGPT Codex or xAI OAuth and the current session is on a different provider, the usable credential lives in the provider's token cache rather than in Config::global() under the *_TOKEN metadata key; for example, ChatGptCodexProvider::inventory_configured() checks TokenCache::new().load().is_some(). This metadata-only predicate therefore returns false and bare /model omits an actually configured provider from the cross-provider picker, so users cannot switch to it despite being authenticated. Use the registry entry's inventory_configured() or equivalent provider-specific status instead of only ProviderMetadata keys here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real gap, but it belongs in the per-provider follow-up (#9822) rather than this metadata predicate. The usable credential for OAuth-backed providers lives in their token cache, which only inventory_configured() (e.g. ChatGptCodexProvider's TokenCache::new().load().is_some()) can see. The shared check_provider_configured works from ProviderMetadata keys and structurally can't observe a provider's token cache. Gating the picker on each registry entry's inventory_configured() is the fix, and it closes this together with the other provider-specific cases.
|
Thanks for the careful back-and-forth on this @jlmalone — you've landed pretty much every codex point. Stepping back though: most of the recurring codex comments (default-only Bedrock, API-keyless OpenAI endpoints, and now the OAuth-token providers one from 2026-06-17) are all symptoms of the same thing — We already have that canonical logic: Rather than keep patching the CLI heuristic, could we share the real one? The CLI can't depend on goose-server, so the move would be to lift One smaller thing: Happy to help wire up the shared helper if useful. Snoozing a few days to give you a chance to weigh in. |
…ider-picker # Conflicts: # crates/goose-cli/src/commands/configure.rs
The CLI model picker carried its own provider_is_configured heuristic, a partial re-implementation of "is this provider usable?" that kept missing cases the rest of goose already handled (default-only Bedrock, API-keyless OpenAI endpoints, OAuth-token providers). Those were the recurring review comments and the deferred items in aaif-goose#9822. Move the canonical check_provider_configured (plus its Hugging Face OAuth helpers and tests) from goose-server routes::utils down into the goose core crate (providers::configured), and have both the goose-server config routes and the CLI picker call it. The picker already held the ProviderType (it was iterating over (meta, _ptype)), so it now passes that through. Removes the duplicated heuristic and the racy env-mutating test (test_provider_is_configured_default_is_not_a_credential); the OAuth, NoAuth, and Bedrock cases are now handled in one place. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 966cb547ce
ℹ️ 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".
|
|
||
| // If there are no non-default keys, this provider needs at least one key explicitly set | ||
| if required_non_default_keys.is_empty() { | ||
| return required_keys.iter().any(|key| { |
There was a problem hiding this comment.
When OpenAI is configured only via OPENAI_API_KEY in the environment/keychain and there is no providers.openai.configured entry, this branch returns false because OpenAI's required keys are only the defaulted OPENAI_HOST/OPENAI_BASE_PATH, while the usable optional secret is never considered. Since bare /model now calls this shared predicate before listing cross-provider choices, a working env-keyed OpenAI provider disappears from the picker whenever the current session is on another provider. Fresh evidence after the earlier Bedrock fix is that the final refactor replaced the picker-specific secret check with this required-key-only branch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 13b18cf. When every required key resolves from a default, check_provider_configured now also treats a real value in any secret key as the configuration signal, so an env/keychain OPENAI_API_KEY (with OPENAI_HOST / OPENAI_BASE_PATH defaulted) marks OpenAI configured and the cross-provider picker lists it. The same secret check now backs the single-required-key branch as well (see the Bedrock thread, 395a8ff).
check_provider_configured only inspected required keys, so a provider whose required keys all resolve via defaults (e.g. OpenAI, whose HOST and BASE_PATH are defaulted while the API key is an optional secret) was reported as unconfigured when set up via its API key alone. The bare /model picker calls this shared predicate, so such a provider vanished from the cross-provider list. When every required key resolves from a default, also treat a real value in any secret key as the configuration signal, keeping the server and CLI consistent. Fixes the OpenAI case Codex flagged on the refactor. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13b18cfa32
ℹ️ 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".
| let is_set_in_env = env::var(&key.name).is_ok(); | ||
| let is_set_in_config = config.get(&key.name, key.secret).is_ok(); | ||
|
|
||
| return is_set_in_env || is_set_in_config; |
There was a problem hiding this comment.
When Bedrock is configured with AWS_BEARER_TOKEN_BEDROCK and the region comes from the AWS SDK chain such as AWS_DEFAULT_REGION or an AWS profile, this early return ignores the real optional secret and reports the provider as unconfigured unless AWS_REGION itself is explicitly set. Since the bare /model picker filters providers through this predicate before it ever tries to instantiate/list them, a usable Bedrock setup is omitted from cross-provider switching.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 395a8ff. The single-required-key-with-default branch (Bedrock's only required key, AWS_REGION, is defaulted) short-circuited on whether that key was explicitly set and ignored the secret. It now also reports configured when any secret key holds a real value, so a Bedrock setup using AWS_BEARER_TOKEN_BEDROCK with the region resolved from an AWS profile or AWS_DEFAULT_REGION is offered in the picker. I factored the secret check into a shared any_secret_configured helper so this branch and the all-defaulted-required-keys branch can't drift again.
This doesn't reopen the original default-only Bedrock case: a fresh install has neither an explicit AWS_REGION nor a bearer token, so Bedrock stays excluded until a real credential is present.
|
Thanks Douwe, agreed on consolidating. Done across
I didn't re-add the configured-state test in env-mutating form, since |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fafb9688cf
ℹ️ 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".
| let temp_provider = | ||
| match goose::providers::create(&meta.name, model_config, Vec::new()).await { |
There was a problem hiding this comment.
When a configured ACP provider is present but its adapter hangs during initialize/newSession, the picker stalls here before the 15s timeout is installed: goose::providers::create calls the ACP constructor, and AcpProvider::start waits on init_rx and session_rx during construction. That means a single bad configured ACP target can block bare /model indefinitely instead of being counted as skipped; wrap create+fetch in the same timeout or avoid constructing ACP providers just to list models.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e5eef0e. The 15s timeout wrapped only fetch_supported_models(), so goose::providers::create() ran unbounded and a hung ACP initialize / newSession handshake could stall the picker before the timeout ever applied. create() now runs inside the same timeout as the listing call; a provider that errors, times out, or returns no models is skipped, exactly as before.
A construction cancelled mid-handshake can still leave the adapter's child process around, but that's the provider-side kill_on_drop cleanup tracked in #9822, not something the picker can reap from the outside.
| match tokio::time::timeout( | ||
| Duration::from_secs(30), | ||
| new_provider.complete(&probe_config, "model-check", "", &probe_msg, &[]), |
There was a problem hiding this comment.
When the selected target is subprocess-backed but not context-managed, e.g. codex or cursor_agent, a hung CLI can exceed this 30s timeout and cancel complete while those providers' execute_command implementations have spawned a child and only wait after stdout EOF; they do not set kill_on_drop, so the picker can report a timeout while leaving a codex exec/cursor-agent process running. Either make these providers cancellation-safe or skip the probe for them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and this is the same provider-side issue as the Claude Code listing leak, just at the probe call site: codex / cursor_agent spawn a child in execute_command without kill_on_drop, so a 30s probe timeout can cancel complete and leave it running. The picker can't reap a child it doesn't own, so the durable fix is kill_on_drop(true) (or a cancellation-safe wait) in those providers rather than a picker-side special case. Folded into #9822 alongside the Claude Code item. The common HTTP providers the probe actually targets (e.g. the NVIDIA dead-catalog 404 case) are unaffected.
…d key check_provider_configured short-circuited the single-required-key-with-default case (e.g. Bedrock, whose only required key AWS_REGION is defaulted) on whether that key was explicitly set, ignoring any secret. A Bedrock setup that authenticates with AWS_BEARER_TOKEN_BEDROCK while the region resolves from an AWS profile or AWS_DEFAULT_REGION was therefore reported unconfigured, so the bare /model picker omitted it. Treat a real value in any secret key as the configuration signal here too, matching the all-defaulted-required-keys branch, and factor the secret check into a shared helper so the two branches cannot drift. Fixes the Bedrock case Codex flagged on the refactor. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
…timeout The picker wrapped only fetch_supported_models() in the 15s timeout, leaving goose::providers::create() unbounded. An ACP provider's create() blocks on the adapter's initialize/newSession handshake, so a single hung adapter could stall bare /model indefinitely before the timeout could fire. Move create() inside the same timeout as the listing call; a provider that errors, times out, or returns no models is skipped from the menu as before. Fixes the unbounded ACP construction Codex flagged. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5eef0eb93
ℹ️ 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".
| // handoff context or, if it hangs, wedge the cancellation path while the | ||
| // client loop is joined on drop — and they don't have the dead-catalog | ||
| // failure mode the probe guards against. | ||
| if !chosen_provider.ends_with("-acp") && !new_provider.manages_own_context() { |
There was a problem hiding this comment.
When an existing session has history and the user picks a context-managed CLI target such as Gemini CLI or Claude Code from bare /model, this path only skips the probe and then installs the provider. Those providers start with no subprocess context and only forward the latest user message (GeminiCliProvider::build_prompt / ClaudeCodeProvider::last_user_content_blocks), so the prior Goose transcript is silently dropped on the next prompt; either block these targets unless there is no prior history or add an explicit handoff path like ACP uses.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0c9476567. Bare /model now refuses to switch into a provider that manages its own context (Claude Code, Gemini CLI) when the session already has history, mirroring the current-provider guard in handle_model. Those providers forward only the latest user message and have no ACP-style handoff, so the existing transcript would be silently dropped. ACP targets report manages_own_context() == false and carry their own first-prompt handoff (the reason the probe is skipped for them), so they stay switchable. A real handoff path for the raw CLI providers would be the richer fix but is out of scope for this PR.
|
@DOsinga thanks, that was the right call. The shared-helper refactor is in: With the logic in one place, the recurring config-detection comments collapse to a single helper, and the last Codex nits on it are addressed there:
Both now route through a single The one remaining item is provider-side: a subprocess-backed probe ( |
The bare /model picker (and the configure model search) opened a mandatory text-input box before showing any models, so a large catalog could not be browsed without first typing a filter, and there was no way to just arrow to a model. Use cliclack's native select instead: filter_mode() gives inline type-to-filter and max_rows() a scrolling viewport, so the list shows up front, is navigable with the arrow keys, and is selectable without typing. Removes the bespoke search loop. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
Esc / Ctrl-C in the picker surfaced as io::ErrorKind::Interrupted, which the
session loop treated as a fatal error and tore down the whole TUI ("operation
interrupted"). Catch the cancellation (the same signal the elicitation prompts
already handle), keep the current model, and return to the session prompt. The
cross-provider picker now routes through the shared searchable menu for any
provider count, since that menu filters and scrolls on its own.
Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02ddfe5072
ℹ️ 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".
| }; | ||
|
|
||
| let (_, chosen_provider, chosen_model) = | ||
| match entries.into_iter().find(|(l, _, _)| l == &chosen_label) { |
There was a problem hiding this comment.
When two rows render the same label, such as a custom/declarative provider using the same display name and model id as another provider, cliclack returns only that label string and this lookup always picks the first matching entry. In that scenario selecting the later row switches to the wrong provider/model; use a stable unique item value (or include the provider name in the value/label) so the selected row maps back to the intended entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0c9476567. cliclack returns only the selected label, so two entries with the same display name and model id mapped back ambiguously and the lookup took the first. The picker now disambiguates any colliding label with the internal provider name before the menu is shown, so each row resolves to exactly one (provider, model). Non-colliding labels are unchanged.
…-safe Two correctness issues Codex flagged on the cross-provider picker: - Duplicate entry labels resolved to the wrong provider. cliclack returns only the selected label, so two providers sharing a display name and model id produced identical rows and the lookup always took the first. Disambiguate colliding labels with the internal provider name so each maps to one entry. - Switching into a context-managed CLI provider (Claude Code, Gemini CLI) mid-session silently dropped the transcript: those forward only the latest user message and, unlike ACP, have no handoff. Refuse the switch when the session already has history, mirroring the existing current-provider guard. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c9476567c
ℹ️ 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".
| // there is history to lose, mirroring the current-provider guard in | ||
| // handle_model. ACP targets report manages_own_context() == false and | ||
| // carry their own handoff, so they stay switchable. | ||
| if new_provider.manages_own_context() && !self.messages.is_empty() { |
There was a problem hiding this comment.
When a non-empty session selects an ACP target such as claude-acp or codex-acp from the bare /model picker, this condition rejects it because AcpProvider::manages_own_context() returns true (crates/goose/src/acp/provider.rs:417), even though ACP has first-prompt handoff logic to carry the current transcript. The picker therefore never installs ACP providers mid-session and instead prints the drop-history error; exclude *-acp targets from this check or use a separate guard for the CLI providers that lack handoff.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in d975b3e67. AcpProvider::manages_own_context() does return true, so the history guard was rejecting ACP targets even though their first-prompt handoff relays the transcript. The guard now excludes *-acp targets and only blocks the no-handoff CLI providers (Claude Code, Gemini CLI), which is what it was meant to catch.
…lter_mode filter_mode shares its scroll offset (view.start) with the filtered list and does not reset it when the list shrinks, so after the list narrows the viewport could render empty while Enter still selected an off-screen row: typing a query showed no suggestions yet picked a model blindly. Replace it with a plain scrollable select (max_rows) plus an explicit Search item that opens a filter prompt. The list still shows immediately and is arrow-selectable without typing; filtering is now predictable and always shows what it matched. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d817fc235
ℹ️ 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".
| let key = &required_keys[0]; | ||
| let is_set_in_env = env::var(&key.name).is_ok(); | ||
| let is_set_in_config = config.get(&key.name, key.secret).is_ok(); | ||
| return is_set_in_env || is_set_in_config || any_secret_configured(metadata, config); |
There was a problem hiding this comment.
When a provider's only required key is a defaulted command, such as CODEX_COMMAND, CURSOR_AGENT_COMMAND, or CLAUDE_CODE_COMMAND, this returns false unless the user explicitly saved that command name. Bare /model filters through this predicate before it ever calls create()/fetch_supported_models(), so an installed and authenticated Codex/Cursor/Claude CLI available under the default executable name is omitted from the cross-provider picker instead of being probed and skipped only if the command is actually unavailable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same per-provider class as #9822. The metadata predicate can't tell whether a defaulted command (CODEX_COMMAND / CURSOR_AGENT_COMMAND / CLAUDE_CODE_COMMAND) actually resolves to an installed executable on PATH; only attempting the provider (or its own configured check) can. Keying the predicate on the presence of a defaulted command name would mark these configured on every machine. The right behavior, probe and skip when the command is unavailable, needs the per-provider signal in #9822, where I'm folding this in.
…-config API main decoupled the model config from the provider: create() no longer takes a ModelConfig, Provider::get_model_config() is gone, and Agent::update_provider() now takes the model config explicitly. Migrate pick_and_switch_model to match: - create(name, extensions) instead of create(name, model_config, extensions), dropping the now-unused seed/model_config in the listing loop. - probe with the locally built new_model_config rather than a provider getter. - update_provider(provider, new_model_config, session_id). handle_model already picked up main's model_config_for_session() via the merge. Workspace builds and clippy --all-targets -D warnings is clean. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0d37b53f4
ℹ️ 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".
| let key = &required_keys[0]; | ||
| let is_set_in_env = env::var(&key.name).is_ok(); | ||
| let is_set_in_config = config.get(&key.name, key.secret).is_ok(); | ||
| return is_set_in_env || is_set_in_config || any_secret_configured(metadata, config); |
There was a problem hiding this comment.
When Bedrock is usable through the AWS SDK chain, e.g. the user sets AWS_PROFILE and the selected profile supplies the region (or AWS_DEFAULT_REGION) without an explicit AWS_REGION, this branch still reports the provider as unconfigured because it only checks the defaulted required key and secret keys. BedrockProvider::from_env does load AWS_PROFILE and allows the SDK config/profile to resolve the region, but bare /model filters through this predicate before it ever creates the provider, so those working Bedrock setups are omitted from cross-provider switching.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, but it's the same blind spot as the other provider-specific cases and shouldn't be patched in this predicate. AWS_PROFILE is a non-required key defaulting to default, and it's commonly set in a developer's shell for unrelated AWS CLI use, so gating Bedrock on it would surface the provider for anyone who has AWS configured at all, reintroducing the fresh-install false positive the strict check was added to prevent. Only BedrockProvider::from_env (which resolves the region from the profile/SDK chain) can tell a Bedrock-capable setup from generic AWS config. Folding into the per-provider inventory_configured() work in #9822.
The context-managed-target guard rejected any provider whose manages_own_context() is true. AcpProvider returns true there, but ACP targets carry a first-prompt handoff that relays the existing transcript, so blocking them was wrong: only the no-handoff CLI providers (Claude Code, Gemini CLI) actually drop history. Exclude *-acp targets from the guard. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d975b3e67d
ℹ️ 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".
| { | ||
| label.push_str(" (current)"); | ||
| } | ||
| entries.push((label, meta.name.clone(), m)); |
There was a problem hiding this comment.
When models comes from an ACP target whose wrapper does not set a model config option, this adds every reported ACP model as a selectable row even though the selected model is never applied. For example, claude-acp sets model_config_option_id: None in crates/goose/src/providers/claude_acp.rs:85, and AcpProvider::apply_model_if_changed returns immediately without an option id in crates/goose/src/acp/provider.rs:334-337; selecting Claude Code ▸ sonnet from bare /model therefore persists sonnet in the Goose session while the ACP subprocess stays on its default/current model. Only expose a sentinel/current row for ACP providers without a model option, or wire the option id before offering per-model choices.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0bc763e6b. You're right that the per-model rows were misleading for an ACP target that can't apply a model choice: apply_model_if_changed returns early when model_config_option_id is None, so the pick was persisted goose-side while the agent stayed on its own current model.
The picker now asks each provider whether a model pick is meaningful via a new Provider::applies_selected_model() (default true; AcpProvider returns self.model_config_option_id.is_some()). When it's false (Claude Code, Amp, Codex, Pi) the provider gets a single … ▸ (agent's current model) row that switches via the ACP_CURRENT_MODEL sentinel, so nothing is pinned that the agent won't honor. Copilot, which does set model_config_option_id, keeps its full per-model list. Added a unit test for the predicate.
The cross-provider `/model` picker listed every model an ACP target reported, but ACP wrappers with no model-config option (Claude Code, Amp, Codex, Pi) ignore a per-model choice: apply_model_if_changed returns early when model_config_option_id is None, so picking "Claude Code > sonnet" persisted `sonnet` on the goose side while the agent silently stayed on its own current model. Add Provider::applies_selected_model() (default true; AcpProvider returns model_config_option_id.is_some()). When it is false the picker offers a single entry that switches to the provider using the ACP_CURRENT_MODEL sentinel instead of a misleading per-model menu. Copilot, which does set a model option, keeps its full per-model list. Surfaced by codex review on aaif-goose#9658. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bc763e6b2
ℹ️ 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".
| // fire. A provider that errors, times out, or lists nothing is skipped. | ||
| let provider_name = meta.name.clone(); | ||
| let listed = tokio::time::timeout(Duration::from_secs(15), async move { | ||
| let temp_provider = goose::providers::create(&provider_name, Vec::new()).await?; |
There was a problem hiding this comment.
When the candidate is an ACP provider with a model config option, this construction uses the process-global active model before the picker knows which target model the user wants: CopilotAcpProvider::from_env reads Config::get_goose_model() into its session_config_options (crates/goose/src/providers/copilot_acp.rs:68-73), and AcpProvider applies those options during newSession (crates/goose/src/acp/provider.rs:1185-1204). If the current/global session is on another provider, e.g. OpenAI with GOOSE_MODEL=gpt-4o, a configured Copilot ACP adapter can reject that foreign model before fetch_supported_models() runs, so the picker skips Copilot even though it has valid models to select. Create ACP candidates for listing with the current sentinel or defer applying a model option until after the user selects one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 78ab68c6b. Real gap: the picker builds each candidate by calling create() then fetch_supported_models(), and Copilot (the only ACP provider with a model config option) folds get_goose_model() into its session_config_options at construction, which the eager newSession then applies. With the session on another provider (e.g. GOOSE_MODEL=gpt-4o under OpenAI), Copilot rejects the foreign model, create() errors, and the provider is dropped even though it lists fine.
The fix takes your first suggestion (list on the current sentinel): a new acp::while_listing_models task-local scope wraps the listing construction, and copilot_acp consults it (model_to_pin) to start model-neutral while listing. Copilot's normal construction is untouched, and its real model is still applied at stream() time via apply_model_if_changed, so runtime behavior is identical. Added unit tests for both the scope and the model_to_pin decision.
The picker lists each provider's models by constructing it and calling fetch_supported_models(). Copilot, the only ACP provider with a model config option, pins the active session's model at construction, so when the session is on another provider (e.g. OpenAI gpt-4o) the Copilot adapter rejects the foreign model and the provider is dropped from the picker even though it has valid models to offer. Add an acp::while_listing_models scope that suppresses ACP model pinning during listing. Copilot then constructs on its current model and lists normally; its real model is still applied at stream() time via apply_model_if_changed, so runtime behavior is unchanged. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78ab68c6b6
ℹ️ 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".
| let extensions = self.agent.get_extension_configs().await; | ||
| let new_provider = goose::providers::create(&chosen_provider, extensions) | ||
| .await |
There was a problem hiding this comment.
When the user selects copilot-acp while the global GOOSE_MODEL still belongs to another provider, this switch-time create() runs outside the while_listing_models scope, so CopilotAcpProvider::from_env again pins Config::get_goose_model() as a session config option before the chosen model can be applied. The listing path was fixed to avoid this rejection, but the provider that is actually installed is still constructed with the foreign model and can fail the ACP session/setConfigOption handshake, making the visible Copilot choice impossible to switch to.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d9d24938a. Same failure as listing, one call later: the switch-time create() ran outside the suppression scope, so Copilot re-pinned the stale global GOOSE_MODEL and could reject it before the chosen model was ever applied. The scope is renamed without_model_pinning (it no longer only guards listing) and now wraps the install-time create() too; the chosen model rides in new_model_config and is applied by apply_model_if_changed on first use.
| let temp_provider = | ||
| goose::providers::create(&provider_name, Vec::new()).await?; | ||
| let applies_selected_model = temp_provider.applies_selected_model(); | ||
| let models = temp_provider.fetch_supported_models().await?; |
There was a problem hiding this comment.
This uses the raw provider catalog instead of the existing fetch_recommended_models(global_toolshim()) path used by configuration, so providers whose /models endpoint includes non-text or no-tool-call catalog entries can add unusable rows to the cross-provider picker. The later probe sends only a plain "ok" with no tools, so a text model that cannot do tool calls can pass the probe and then break normal Goose turns with extensions; use the recommended-model filter before adding rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d9d24938a. The picker now lists via fetch_recommended_models(global_toolshim()), matching the configure flow and the desktop picker (inventory/mod.rs), so non-text and no-tool-call catalog entries are filtered out the same way everywhere models are offered. Its built-in fallback (filtered-to-empty returns the full catalog) keeps ACP agents, whose model ids often do not map to the canonical registry, listing their full set.
main's provider refactor dropped the session_id argument from Provider::complete. Update the liveness probe to the current 4-argument signature so the picker still builds after merging the latest main. Signed-off-by: Joseph Malone <jlmalone@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6536ab79b5
ℹ️ 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".
| let temp_provider = | ||
| goose::providers::create(&provider_name, Vec::new()).await?; | ||
| let applies_selected_model = temp_provider.applies_selected_model(); | ||
| let models = temp_provider.fetch_supported_models().await?; |
There was a problem hiding this comment.
When a configured provider relies on ProviderMetadata.known_models instead of overriding fetch_supported_models(), this picker treats it as unavailable. For example, with the aws-providers feature, SageMakerTgiProvider lists SAGEMAKER_TGI_DEFAULT_MODEL in metadata but its Provider impl does not override fetch_supported_models(), so the trait default returns an empty vec; the new picker then skips SageMaker and can report no models even though the provider is configured and usable. Fall back to the registry metadata/known models for empty dynamic lists, or add provider-specific list implementations before filtering it out.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d9d24938a. When the filtered listing comes back empty the picker now falls back to the provider metadata known_models, so metadata-only providers like SageMaker TGI (no fetch_supported_models override; the trait default lists nothing) keep their rows instead of being skipped.
The switch-time create() ran outside the pinning-suppression scope, so an option-backed ACP target (Copilot) re-pinned the stale global model and could fail the switch before the chosen model was applied; the scope is renamed to without_model_pinning now that it guards install as well as listing. The listing now uses fetch_recommended_models, the same tool-usability filter as configure and the desktop picker, and falls back to metadata known_models for providers that do not override fetch_supported_models.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9d24938a3
ℹ️ 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".
| let listed = goose::acp::without_model_pinning(tokio::time::timeout( | ||
| Duration::from_secs(15), | ||
| async move { | ||
| let temp_provider = | ||
| goose::providers::create(&provider_name, Vec::new()).await?; |
There was a problem hiding this comment.
When a configured ACP adapter spawns but never answers initialize/newSession, this timeout cancels goose::providers::create after AcpProvider::start has already spawned the client loop (crates/goose/src/acp/provider.rs:251) but before an AcpProvider exists to run Drop. In that case /model reports the provider as skipped while the detached ACP thread/subprocess can keep running; the fresh evidence after the earlier construction-timeout fix is that the timeout now wraps the constructor itself, whose spawned loop is not owned by any cleanup guard until start returns.
Useful? React with 👍 / 👎.
| Ok(Ok(_)) | Ok(Err(_)) | Err(_) => { | ||
| skipped.push(meta.display_name.clone()); |
There was a problem hiding this comment.
When an option-less ACP adapter starts successfully but does not advertise a model config option, AcpProvider::fetch_supported_models returns an error because resolve_model_info requires model config_options, and this arm treats that as an unavailable provider. That omits configured ACP targets that can still run on the agent's current model and should use the sentinel row; fresh evidence after the earlier ACP-row fix is that the final code only adds the sentinel for Ok((false, models)) with a non-empty model list, while the no-model-option path lands here as Ok(Err(_)).
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale because it has not had recent activity for 23 days. What happens next?
Thank you for your contribution! 🚀 |
/builtin and /extension add extensions, but nothing showed which ones are actually loaded — a question that comes up right after a failed extension start, or after /new restarted them. /extensions prints a table of the loaded extensions with their type and description, following the shape /skills already uses. /provider was considered alongside this and deliberately left out: aaif-goose#9658 turns a bare /model into a cross-provider picker that switches provider and model together, which covers the same ground more directly.
|
Closing as superseded- not a reflection of the quality of the work here. Main has moved past this branch: #10585 rewrote handle_model and landed cross-provider switching (/model --provider [model] with tab completion), goose-server was removed in #10224 (taking the shared check_provider_configured anchor with it), and model enumeration now flows through the provider inventory. A rebase would effectively be a rewrite. The interactive picker on bare /model is still wanted (#9412), but it's now best done as a fresh, small PR over the inventory on top of the current handle_model. @jlmalone thanks for the careful iteration. if you'd like to build the successor, anchoring on #9412 first is the way to go. |
3 participants
What
A bare
/modelin agoose sessioncurrently switches models within the active provider only. This makes a no-argument/modelopen one searchable menu that aggregates models from every configured provider and switches the live session's provider and model on selection./model <name>keeps its existing within-provider behavior.Why
With several providers configured (e.g. a local OpenAI-compatible server plus a hosted provider), switching to a model on a different provider mid-session means leaving the session or editing config. This lets you jump to any model on any configured provider in one fuzzy-search step.
How
pick_and_switch_model()enumerates configured providers, fetches each provider's models, aggregates them, reuses the existing configure-flow fuzzy-search UI, thencreate()+update_provider()on the session.interactive_model_searchis nowpub(crate)for reuse.provider_is_configured()requires a provider that authenticates with a secret to have a real secret value, not just a built-in default — so default-only cloud providers (e.g. Bedrock, whoseAWS_REGIONhas a default) don't appear as usable. NoAuth local backends (LM Studio, Ollama) declare no secrets and stay available.Skipped N provider(s) not available hereline instead of a wall of per-provider warnings. Listing is bounded by a 15s per-provider timeout so one slow endpoint can't block the menu./modelis rejected up front — same guard/model <name>already applied — so the picker can't switch away and strand that context.Testing
cargo fmt,cargo check -p goose-cli, andcargo clippy -p goose-cli -- -D warningsclean against currentmain(branch merged up to date)./modellists models across configured providers and switches the session; unreachable endpoints are skipped rather than blocking the list; a model the account can't run is rejected with the current model kept;/model <name>unchanged.Two follow-ups from review (NoAuth-OpenAI gating and Claude Code subprocess cleanup, both provider-side) are tracked in #9822. Every commit is DCO signed-off.