diff --git a/crates/tui/src/commands/groups/core/core.rs b/crates/tui/src/commands/groups/core/core.rs index 712907f0f..e5fdfbba5 100644 --- a/crates/tui/src/commands/groups/core/core.rs +++ b/crates/tui/src/commands/groups/core/core.rs @@ -5,7 +5,7 @@ use std::path::PathBuf; use crate::config::{ ApiProvider, COMMON_DEEPSEEK_MODELS, normalize_custom_model_id, - normalize_model_name_for_provider, provider_passes_model_through, + normalize_model_name_for_provider, }; use crate::localization::{MessageId, tr}; use crate::tui::app::{App, AppAction, AppMode, ReasoningEffort}; @@ -160,20 +160,8 @@ pub fn model(app: &mut App, model_name: Option<&str>) -> CommandResult { model_id } else { let Some(model_id) = normalize_model_name_for_provider(app.api_provider, name) else { - if let Some((provider, model_id)) = saved_provider_model_match(app, name) { - return CommandResult::with_message_and_action( - format!( - "Switching provider to {} for model {model_id}.", - provider.as_str() - ), - AppAction::SwitchProvider { - provider, - model: Some(model_id), - }, - ); - } return CommandResult::error(format!( - "Invalid model '{name}'. Expected auto, a model for the active provider, or a saved provider model. Common DeepSeek models: {}", + "Invalid model '{name}'. Expected auto or a model for the active provider. Common DeepSeek models: {}", COMMON_DEEPSEEK_MODELS.join(", ") )); }; @@ -225,43 +213,6 @@ fn provider_model_selection_persist_warning(provider: ApiProvider, model: &str) .map(|err| format!(" (not persisted: {err})")) } -fn saved_provider_model_match(app: &App, name: &str) -> Option<(ApiProvider, String)> { - let requested = normalize_custom_model_id(name)?; - let mut saved = app - .provider_models - .iter() - .filter_map(|(provider_name, model)| { - let provider = ApiProvider::parse(provider_name)?; - (provider != app.api_provider).then_some((provider, model.as_str())) - }) - .collect::>(); - saved.sort_by_key(|(provider, _)| provider.as_str()); - - for (provider, saved_model) in saved { - let Some(saved_model) = normalize_model_for_provider_selection(provider, saved_model) - else { - continue; - }; - let requested_model = normalize_model_for_provider_selection(provider, &requested) - .unwrap_or_else(|| requested.clone()); - if saved_model.eq_ignore_ascii_case(&requested_model) - || saved_model.eq_ignore_ascii_case(&requested) - { - return Some((provider, saved_model)); - } - } - - None -} - -fn normalize_model_for_provider_selection(provider: ApiProvider, model: &str) -> Option { - if provider_passes_model_through(provider) { - normalize_custom_model_id(model) - } else { - normalize_model_name_for_provider(provider, model) - } -} - /// Fetch and list available models from the configured API endpoint. pub fn models(_app: &mut App) -> CommandResult { CommandResult::action(AppAction::FetchModels) @@ -1255,7 +1206,7 @@ mod tests { } #[test] - fn model_command_switches_to_saved_provider_model() { + fn model_command_rejects_saved_model_from_other_provider() { let mut app = create_test_app(); app.api_provider = crate::config::ApiProvider::Deepseek; app.provider_models @@ -1263,13 +1214,10 @@ mod tests { let result = model(&mut app, Some("kimi-k2.6")); - match result.action { - Some(AppAction::SwitchProvider { provider, model }) => { - assert_eq!(provider, crate::config::ApiProvider::Moonshot); - assert_eq!(model.as_deref(), Some("kimi-k2.6")); - } - other => panic!("expected SwitchProvider action, got {other:?}"), - } + let message = result.message.expect("invalid model message"); + assert!(message.contains("Invalid model")); + assert!(message.contains("active provider")); + assert!(result.action.is_none()); assert_eq!(app.api_provider, crate::config::ApiProvider::Deepseek); assert_eq!(app.model, "deepseek-v4-pro"); } diff --git a/crates/tui/src/tui/model_picker.rs b/crates/tui/src/tui/model_picker.rs index 26e532a71..923ed5b08 100644 --- a/crates/tui/src/tui/model_picker.rs +++ b/crates/tui/src/tui/model_picker.rs @@ -380,38 +380,40 @@ fn picker_model_ids_for_provider(provider: ApiProvider) -> Vec<&'static str> { models } +pub(crate) fn provider_scoped_model_completion_ids(app: &App) -> Vec { + // Slash completions inline the current custom model so `/model ` + // stays visible even when it is outside the provider catalog. + provider_scoped_model_ids_for_app(app, true) +} + fn picker_model_rows_for_app(app: &App) -> Vec { + // The picker renders the current custom model as a dedicated row, so the + // provider catalog list does not need to duplicate it. + let model_ids = provider_scoped_model_ids_for_app(app, false); let mut rows = Vec::new(); - push_model_row( - &mut rows, - "auto".to_string(), - None, - picker_model_hint("auto"), - ); - - for id in model_completion_names_for_provider(app.api_provider) { - if id != "auto" { + for id in model_ids { + if id == "auto" { + push_model_row(&mut rows, id, None, picker_model_hint("auto")); + } else { push_model_row( &mut rows, - id.to_string(), + id.clone(), Some(app.api_provider), - picker_model_hint(id), + picker_model_hint(&id), ); } } - if !app.model_ids_passthrough { - for id in model_registry::seeded_model_ids() { - if let Some(metadata) = model_registry::lookup(id) { - let provider = model_registry::serving_provider(metadata.provider); - push_model_row( - &mut rows, - id.to_string(), - Some(provider), - picker_model_hint(id), - ); - } - } + rows +} + +fn provider_scoped_model_ids_for_app(app: &App, include_current_model: bool) -> Vec { + // `include_current_model` is for completion surfaces that do not have a + // separate custom/current-model row. + let mut models = Vec::new(); + push_model_id(&mut models, "auto"); + for id in model_completion_names_for_provider(app.api_provider) { + push_model_id(&mut models, id); } if let Some(model) = app @@ -420,46 +422,27 @@ fn picker_model_rows_for_app(app: &App) -> Vec { .map(|model| model.trim()) .filter(|model| !model.is_empty()) { - push_model_row( - &mut rows, - model.to_string(), - Some(app.api_provider), - format!("{} saved", app.api_provider.display_name()), - ); + push_model_id(&mut models, model); } - // Surface models saved under *other* providers in config (#2596). The - // active provider's list comes first; cross-provider saved models follow as - // a clearly labelled tail so a custom model that has never been selected on - // the current provider is still reachable. Selecting one switches provider - // on apply via `resolved_provider` / `build_event`. Rows are sorted by - // provider key so ordering stays deterministic regardless of map iteration. - // Parse each provider key once: drop unknown keys (cannot be applied) and - // the active provider (already listed above) in a single pass. `key` is - // kept only to keep ordering deterministic via the sort below. - let mut other_provider_models: Vec<(&String, ApiProvider, &String)> = app - .provider_models - .iter() - .filter_map(|(key, model)| { - let provider = ApiProvider::parse(key)?; - (provider != app.api_provider).then_some((key, provider, model)) - }) - .collect(); - other_provider_models.sort_by_key(|(a, ..)| *a); - for (_key, provider, model) in other_provider_models { - let model = model.trim(); - if model.is_empty() { - continue; - } - push_model_row( - &mut rows, - model.to_string(), - Some(provider), - format!("{} saved", provider.display_name()), - ); + if include_current_model && !app.auto_model { + push_model_id(&mut models, app.model.trim()); } - rows + models +} + +fn push_model_id(models: &mut Vec, model: &str) { + let model = model.trim(); + if model.is_empty() { + return; + } + if !models + .iter() + .any(|existing| existing.eq_ignore_ascii_case(model)) + { + models.push(model.to_string()); + } } fn push_model_row( @@ -824,29 +807,29 @@ mod tests { } #[test] - fn picker_lists_cross_provider_catalog_without_saved_entries() { - let (app, _lock) = create_test_app(); - let mut view = ModelPickerView::new(&app); + fn picker_main_rows_are_scoped_to_active_provider() { + let (mut app, _lock) = create_test_app(); + app.api_provider = crate::config::ApiProvider::Together; + app.model = crate::config::DEFAULT_TOGETHER_MODEL.to_string(); + app.provider_models.insert( + "openrouter".to_string(), + crate::config::DEFAULT_OPENROUTER_MODEL.to_string(), + ); - let kimi_idx = view - .visible_model_rows() - .iter() - .position(|row| { - row.id == "kimi-k2.7-code" - && row.provider == Some(crate::config::ApiProvider::Moonshot) - }) - .expect("Moonshot catalog model should be discoverable without saved config"); + let view = ModelPickerView::new(&app); - view.selected_model_idx = kimi_idx; - match view.build_event() { - ViewEvent::ModelPickerApplied { - model, provider, .. - } => { - assert_eq!(model, "kimi-k2.7-code"); - assert_eq!(provider, Some(crate::config::ApiProvider::Moonshot)); - } - other => panic!("expected model picker event, got {other:?}"), - } + assert!( + view.visible_model_rows() + .iter() + .all(|row| row.provider.is_none() + || row.provider == Some(crate::config::ApiProvider::Together)) + ); + assert!( + !view + .visible_model_ids() + .contains(&crate::config::DEFAULT_OPENROUTER_MODEL), + "OpenRouter saved rows must not appear as bare Together model choices" + ); } #[test] @@ -937,7 +920,7 @@ mod tests { } #[test] - fn picker_remaps_deepseek_off_when_highlighting_saved_codex_model() { + fn picker_excludes_saved_codex_model_from_deepseek_main_section() { let (mut app, _lock) = create_test_app(); app.api_provider = crate::config::ApiProvider::Deepseek; app.model = "deepseek-v4-pro".to_string(); @@ -946,34 +929,19 @@ mod tests { app.provider_models .insert("openai-codex".to_string(), "gpt-5.5".to_string()); - let mut view = ModelPickerView::new(&app); + let view = ModelPickerView::new(&app); assert_eq!(view.resolved_effort(), ReasoningEffort::Off); - - let effort = view.resolved_effort(); - view.selected_model_idx = view - .visible_model_rows() - .iter() - .position(|row| { - row.id == "gpt-5.5" && row.provider == Some(crate::config::ApiProvider::OpenaiCodex) - }) - .expect("saved Codex model row should be reachable"); - view.select_effort_for_current_model(effort); - - assert_eq!(view.resolved_model(), "gpt-5.5"); - assert_eq!(view.resolved_effort(), ReasoningEffort::Low); - assert_eq!(view.selected_effort_idx, 0); - let labels = view - .current_efforts() - .iter() - .map(|effort| { - effort.display_label_for_provider(crate::config::ApiProvider::OpenaiCodex) - }) - .collect::>(); - assert_eq!(labels, vec!["low", "medium", "high", "xhigh"]); + assert!( + view.visible_model_rows() + .iter() + .all(|row| row.provider.is_none() + || row.provider == Some(crate::config::ApiProvider::Deepseek)) + ); + assert!(!view.visible_model_ids().contains(&"gpt-5.5")); } #[test] - fn picker_remaps_deepseek_max_to_codex_xhigh_when_model_provider_changes() { + fn picker_does_not_switch_provider_when_moving_through_model_rows() { let (mut app, _lock) = create_test_app(); app.api_provider = crate::config::ApiProvider::Deepseek; app.model = "deepseek-v4-pro".to_string(); @@ -983,19 +951,14 @@ mod tests { .insert("openai-codex".to_string(), "gpt-5.5".to_string()); let mut view = ModelPickerView::new(&app); - while view.resolved_provider() != Some(crate::config::ApiProvider::OpenaiCodex) { - assert!( - view.move_down(), - "saved Codex model row should be reachable" + while view.move_down() { + assert_ne!( + view.resolved_provider(), + Some(crate::config::ApiProvider::OpenaiCodex) ); } - assert_eq!(view.resolved_effort(), ReasoningEffort::Max); - assert_eq!( - view.resolved_effort() - .display_label_for_provider(crate::config::ApiProvider::OpenaiCodex), - "xhigh" - ); + assert_eq!(view.initial_provider, crate::config::ApiProvider::Deepseek); } #[test] @@ -1160,9 +1123,7 @@ mod tests { } #[test] - fn picker_lists_saved_models_from_other_providers() { - // #2596: custom models saved under a non-active provider must be - // reachable from the picker, after the active provider's own models. + fn picker_excludes_saved_models_from_other_providers() { let (mut app, _lock) = create_test_app(); app.api_provider = crate::config::ApiProvider::XiaomiMimo; app.model = "mimo-v2.5-pro".to_string(); @@ -1183,55 +1144,17 @@ mod tests { // Active provider's own model stays present (and ahead of the tail). assert!(model_ids.contains(&"mimo-v2.5-pro")); - // Cross-provider saved models are now visible. - assert!(model_ids.contains(&"deepseek-v4-pro")); - assert!(model_ids.contains(&"kimi-k2.6")); - assert!(model_ids.contains(&"qwen-plus")); - assert!(model_ids.contains(&"custom-qianfan-service-id")); + // Cross-provider saved models are kept out of the provider-scoped list. + assert!(!model_ids.contains(&"deepseek-v4-pro")); + assert!(!model_ids.contains(&"kimi-k2.6")); + assert!(!model_ids.contains(&"qwen-plus")); + assert!(!model_ids.contains(&"custom-qianfan-service-id")); assert!(!view.show_custom_model_row); - - // Each cross-provider row carries its own provider so applying it - // switches CodeWhale to that provider (verified via build_event below). - let deepseek_row = view - .visible_model_rows() - .iter() - .find(|row| row.id == "deepseek-v4-pro") - .expect("deepseek-v4-pro row present"); - assert_eq!( - deepseek_row.provider, - Some(crate::config::ApiProvider::Deepseek) - ); - let dashscope_row = view - .visible_model_rows() - .iter() - .find(|row| row.id == "qwen-plus") - .expect("qwen-plus row present"); - assert_eq!( - dashscope_row.provider, - Some(crate::config::ApiProvider::Openai) - ); - let qianfan_row = view - .visible_model_rows() - .iter() - .find(|row| row.id == "custom-qianfan-service-id") - .expect("custom Qianfan row present"); - assert_eq!( - qianfan_row.provider, - Some(crate::config::ApiProvider::Qianfan) - ); - - // Active-provider model must appear before any cross-provider tail row. - let active_idx = model_ids - .iter() - .position(|id| *id == "mimo-v2.5-pro") - .expect("active model index"); - let cross_idx = model_ids - .iter() - .position(|id| *id == "kimi-k2.6") - .expect("cross-provider model index"); assert!( - active_idx < cross_idx, - "active provider models should precede cross-provider tail" + view.visible_model_rows() + .iter() + .all(|row| row.provider.is_none() + || row.provider == Some(crate::config::ApiProvider::XiaomiMimo)) ); } diff --git a/crates/tui/src/tui/slash_menu.rs b/crates/tui/src/tui/slash_menu.rs index 778ef5f88..0783cc4cc 100644 --- a/crates/tui/src/tui/slash_menu.rs +++ b/crates/tui/src/tui/slash_menu.rs @@ -11,8 +11,9 @@ use crate::commands; use super::app::{App, looks_like_slash_command_input}; +use super::model_picker::provider_scoped_model_completion_ids; use super::widgets::SlashMenuEntry; -use super::widgets::slash_completion_hints; +use super::widgets::slash_completion_hints_with_model_candidates; /// Return the slash-menu entries the composer should display, honouring /// `slash_menu_hidden` (set when the user dismisses the popup with Esc). @@ -26,13 +27,14 @@ pub fn visible_slash_menu_entries(app: &App, limit: usize) -> Vec bool { return false; } - let candidates = slash_completion_hints( + let model_candidates = provider_scoped_model_completion_ids(app); + let candidates = slash_completion_hints_with_model_candidates( &app.input, 128, &app.cached_skills, app.ui_locale, Some(&app.workspace), - app.api_provider, + &model_candidates, ) .into_iter() .map(|entry| entry.name) diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index bf51654af..fd62d94ad 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -6519,9 +6519,9 @@ async fn apply_model_picker_choice( } // Reject a model that does not belong to the active provider before we - // mutate session state or persist it (#3227). The picker can surface - // cross-provider saved models, so this is the in-session safety net for - // the same-provider path; cross-provider picks go through + // mutate session state or persist it (#3227/#3383). The provider-scoped + // picker should only emit active-provider rows; keep this guard as the + // in-session safety net. Explicit cross-provider route switches go through // `switch_provider`, which validates the route atomically. Skip the strict // check when the app accepts custom ids (pass-through provider or custom // DeepSeek-compatible base URL) — the upstream is the authority there. diff --git a/crates/tui/src/tui/ui/tests.rs b/crates/tui/src/tui/ui/tests.rs index 5122112ed..c0edc51a1 100644 --- a/crates/tui/src/tui/ui/tests.rs +++ b/crates/tui/src/tui/ui/tests.rs @@ -5706,6 +5706,32 @@ fn visible_slash_menu_entries_excludes_removed_commands() { assert!(!entries.iter().any(|entry| entry.name == "/codewhale")); } +#[test] +fn visible_slash_model_completions_are_provider_scoped() { + let mut app = create_test_app(); + app.api_provider = crate::config::ApiProvider::Together; + app.model = crate::config::DEFAULT_TOGETHER_MODEL.to_string(); + app.provider_models.insert( + "openrouter".to_string(), + crate::config::DEFAULT_OPENROUTER_MODEL.to_string(), + ); + app.input = "/model".to_string(); + app.cursor_position = app.input.chars().count(); + + let entries = visible_slash_menu_entries(&app, 128); + let names = entries + .iter() + .map(|entry| entry.name.as_str()) + .collect::>(); + + assert!(names.contains(&"/model deepseek-ai/DeepSeek-V4-Pro")); + let openrouter_completion = format!("/model {}", crate::config::DEFAULT_OPENROUTER_MODEL); + assert!( + !names.contains(&openrouter_completion.as_str()), + "OpenRouter saved rows must not appear as bare Together /model completions" + ); +} + #[test] fn slash_menu_up_wraps_from_first_to_last() { let mut app = create_test_app(); diff --git a/crates/tui/src/tui/widgets/mod.rs b/crates/tui/src/tui/widgets/mod.rs index 7a01134da..d132afc54 100644 --- a/crates/tui/src/tui/widgets/mod.rs +++ b/crates/tui/src/tui/widgets/mod.rs @@ -25,6 +25,11 @@ pub use renderable::Renderable; use std::collections::HashSet; use std::time::Duration; +use crate::commands; +#[cfg(test)] +use crate::config::ApiProvider; +#[cfg(test)] +use crate::config::model_completion_names_for_provider; use crate::localization::{Locale, MessageId, tr}; use crate::palette; use crate::tui::app::{App, AppMode, ComposerDensity, VimMode}; @@ -33,10 +38,6 @@ use crate::tui::approval::{ }; use crate::tui::history::{GenericToolCell, HistoryCell, ToolCell, ToolRun, ToolStatus}; use crate::tui::scrolling::TranscriptLineMeta; -use crate::{ - commands, - config::{ApiProvider, model_completion_names_for_provider}, -}; use ratatui::{ buffer::Buffer, layout::Rect, @@ -2330,6 +2331,7 @@ fn fuzzy_chars_in_order(needle: &str, haystack: &str) -> bool { false } +#[cfg(test)] pub(crate) fn slash_completion_hints( input: &str, limit: usize, @@ -2337,6 +2339,28 @@ pub(crate) fn slash_completion_hints( locale: crate::localization::Locale, workspace: Option<&std::path::Path>, api_provider: ApiProvider, +) -> Vec { + let model_candidates = model_completion_names_for_provider(api_provider) + .into_iter() + .map(str::to_string) + .collect::>(); + slash_completion_hints_with_model_candidates( + input, + limit, + cached_skills, + locale, + workspace, + &model_candidates, + ) +} + +pub(crate) fn slash_completion_hints_with_model_candidates( + input: &str, + limit: usize, + cached_skills: &[(String, String)], + locale: crate::localization::Locale, + workspace: Option<&std::path::Path>, + model_candidates: &[String], ) -> Vec { if !super::app::looks_like_slash_command_input(input) { return Vec::new(); @@ -2547,7 +2571,7 @@ pub(crate) fn slash_completion_hints( // Special: /model completions when only /model matches if entries.iter().any(|e| e.name == "/model") && prefix_lower.eq_ignore_ascii_case("model") { - for model_name in model_completion_names_for_provider(api_provider) { + for model_name in model_candidates { entries.push(SlashMenuEntry { name: format!("/model {model_name}"), description: String::from("Switch to this model"),