From 609ed3025241b1e0e5cde086a5c051bcb312ff82 Mon Sep 17 00:00:00 2001 From: Tushar Date: Fri, 27 Mar 2026 17:12:08 +0530 Subject: [PATCH 1/2] feat(api/ui): set provider and model atomically --- crates/forge_api/src/api.rs | 14 ++-- crates/forge_api/src/forge_api.rs | 18 ++--- crates/forge_app/src/command_generator.rs | 10 +-- crates/forge_app/src/services.rs | 23 +++--- crates/forge_domain/src/env.rs | 76 +++++------------ crates/forge_main/src/cli.rs | 32 +------- crates/forge_main/src/ui.rs | 99 ++++++++++++----------- crates/forge_services/src/app_config.rs | 99 ++++++++--------------- shell-plugin/lib/actions/config.zsh | 20 ++--- 9 files changed, 146 insertions(+), 245 deletions(-) diff --git a/crates/forge_api/src/api.rs b/crates/forge_api/src/api.rs index 51ee3153ec..5dc0811e6f 100644 --- a/crates/forge_api/src/api.rs +++ b/crates/forge_api/src/api.rs @@ -121,8 +121,15 @@ pub trait API: Sync + Send { /// Retrieves the provider configuration for the default agent async fn get_default_provider(&self) -> anyhow::Result>; - /// Sets the default provider for all the agents - async fn set_default_provider(&self, provider_id: ProviderId) -> anyhow::Result<()>; + /// Sets the active provider and model simultaneously. + /// + /// Both values must be supplied together to ensure the configuration always + /// contains a consistent provider/model pair. + async fn set_provider_and_model( + &self, + provider_id: ProviderId, + model_id: ModelId, + ) -> anyhow::Result<()>; /// Retrieves information about the currently authenticated user async fn user_info(&self) -> anyhow::Result>; @@ -142,9 +149,6 @@ pub trait API: Sync + Send { /// Gets the default model async fn get_default_model(&self) -> Option; - /// Sets the operating model - async fn set_default_model(&self, model_id: ModelId) -> anyhow::Result<()>; - /// Gets the commit configuration (provider and model for commit message /// generation). async fn get_commit_config(&self) -> anyhow::Result>; diff --git a/crates/forge_api/src/forge_api.rs b/crates/forge_api/src/forge_api.rs index 6bcc9197c1..57973f6345 100644 --- a/crates/forge_api/src/forge_api.rs +++ b/crates/forge_api/src/forge_api.rs @@ -213,8 +213,15 @@ impl anyhow::Result<()> { - let result = self.services.set_default_provider(provider_id).await; + async fn set_provider_and_model( + &self, + provider_id: ProviderId, + model_id: ModelId, + ) -> anyhow::Result<()> { + let result = self + .services + .set_provider_and_model(provider_id, model_id) + .await; // Invalidate cache for agents let _ = self.services.reload_agents().await; result @@ -261,13 +268,6 @@ impl Option { self.services.get_provider_model(None).await.ok() } - async fn set_default_model(&self, model_id: ModelId) -> anyhow::Result<()> { - let result = self.services.set_default_model(model_id).await; - // Invalidate cache for agents - let _ = self.services.reload_agents().await; - - result - } async fn get_commit_config(&self) -> anyhow::Result> { self.services.get_commit_config().await diff --git a/crates/forge_app/src/command_generator.rs b/crates/forge_app/src/command_generator.rs index 7650deef19..69e207ab56 100644 --- a/crates/forge_app/src/command_generator.rs +++ b/crates/forge_app/src/command_generator.rs @@ -249,7 +249,11 @@ mod tests { Ok(ProviderId::OPENAI) } - async fn set_default_provider(&self, _provider_id: ProviderId) -> Result<()> { + async fn set_provider_and_model( + &self, + _provider_id: ProviderId, + _model: ModelId, + ) -> Result<()> { Ok(()) } @@ -260,10 +264,6 @@ mod tests { Ok(ModelId::new("test-model")) } - async fn set_default_model(&self, _model: ModelId) -> Result<()> { - Ok(()) - } - async fn get_commit_config(&self) -> Result> { Ok(None) } diff --git a/crates/forge_app/src/services.rs b/crates/forge_app/src/services.rs index 9d2b54788a..e1f726a746 100644 --- a/crates/forge_app/src/services.rs +++ b/crates/forge_app/src/services.rs @@ -186,10 +186,14 @@ pub trait AppConfigService: Send + Sync { /// Gets the user's default provider ID. async fn get_default_provider(&self) -> anyhow::Result; - /// Sets the user's default provider preference. - async fn set_default_provider( + /// Sets the active provider and model simultaneously. + /// + /// Both values must be supplied together to ensure the persisted + /// configuration always contains a consistent provider/model pair. + async fn set_provider_and_model( &self, provider_id: forge_domain::ProviderId, + model: ModelId, ) -> anyhow::Result<()>; /// Gets the user's default model for a specific provider or the currently @@ -206,12 +210,6 @@ pub trait AppConfigService: Send + Sync { provider_id: Option<&forge_domain::ProviderId>, ) -> anyhow::Result; - /// Sets the user's default model for the currently active provider. - /// - /// # Errors - /// Returns an error if no default provider is configured. - async fn set_default_model(&self, model: ModelId) -> anyhow::Result<()>; - /// Gets the commit configuration (provider and model for commit message /// generation). async fn get_commit_config(&self) -> anyhow::Result>; @@ -985,12 +983,13 @@ impl AppConfigService for I { self.config_service().get_default_provider().await } - async fn set_default_provider( + async fn set_provider_and_model( &self, provider_id: forge_domain::ProviderId, + model: ModelId, ) -> anyhow::Result<()> { self.config_service() - .set_default_provider(provider_id) + .set_provider_and_model(provider_id, model) .await } @@ -1001,10 +1000,6 @@ impl AppConfigService for I { self.config_service().get_provider_model(provider_id).await } - async fn set_default_model(&self, model: ModelId) -> anyhow::Result<()> { - self.config_service().set_default_model(model).await - } - async fn get_commit_config(&self) -> anyhow::Result> { self.config_service().get_commit_config().await } diff --git a/crates/forge_domain/src/env.rs b/crates/forge_domain/src/env.rs index 29dc710f5f..959486ea1f 100644 --- a/crates/forge_domain/src/env.rs +++ b/crates/forge_domain/src/env.rs @@ -29,10 +29,11 @@ pub struct SessionConfig { /// each in order, and persist the result atomically. #[derive(Debug, Clone, PartialEq)] pub enum ConfigOperation { - /// Set the active provider. - SetProvider(ProviderId), - /// Set the model for the given provider. - SetModel(ProviderId, ModelId), + /// Set the active provider and model together. + /// + /// Both must be supplied at once to ensure the configuration always + /// contains a consistent provider/model pair. + SetProviderModel(ProviderId, ModelId), /// Set the commit-message generation configuration. SetCommitConfig(CommitConfig), /// Set the shell-command suggestion configuration. @@ -172,20 +173,14 @@ impl Environment { /// Applies a single [`ConfigOperation`] to this environment in-place. pub fn apply_op(&mut self, op: ConfigOperation) { match op { - ConfigOperation::SetProvider(provider_id) => { - let pid = provider_id.as_ref().to_string(); - self.session = Some(match self.session.take() { - Some(sc) => sc.provider_id(pid), - None => SessionConfig::default().provider_id(pid), - }); - } - ConfigOperation::SetModel(provider_id, model_id) => { + ConfigOperation::SetProviderModel(provider_id, model_id) => { let pid = provider_id.as_ref().to_string(); let mid = model_id.to_string(); - self.session = Some(match self.session.take() { - Some(sc) if sc.provider_id.as_deref() == Some(&pid) => sc.model_id(mid), - _ => SessionConfig::default().provider_id(pid).model_id(mid), - }); + self.session = Some( + SessionConfig::default() + .provider_id(pid) + .model_id(mid), + ); } ConfigOperation::SetCommitConfig(commit) => { self.commit = @@ -315,59 +310,28 @@ mod tests { } #[test] - fn test_apply_op_set_provider_creates_session_when_absent() { - let mut fixture = fixture_env(); - fixture.apply_op(ConfigOperation::SetProvider(ProviderId::from( - "anthropic".to_string(), - ))); - let expected = SessionConfig::default().provider_id("anthropic".to_string()); - assert_eq!(fixture.session, Some(expected)); - } - - #[test] - fn test_apply_op_set_provider_updates_existing_session_keeping_model() { - let mut fixture = fixture_env(); - fixture.session = Some( - SessionConfig::default() - .provider_id("openai".to_string()) - .model_id("gpt-4".to_string()), - ); - fixture.apply_op(ConfigOperation::SetProvider(ProviderId::from( - "anthropic".to_string(), - ))); - let expected = SessionConfig::default() - .provider_id("anthropic".to_string()) - .model_id("gpt-4".to_string()); - assert_eq!(fixture.session, Some(expected)); - } - - #[test] - fn test_apply_op_set_model_for_matching_provider_updates_model() { + fn test_apply_op_set_provider_model_creates_session() { let mut fixture = fixture_env(); - fixture.session = Some( - SessionConfig::default() - .provider_id("openai".to_string()) - .model_id("gpt-3.5".to_string()), - ); - fixture.apply_op(ConfigOperation::SetModel( - ProviderId::from("openai".to_string()), - ModelId::new("gpt-4"), + fixture.session = None; + fixture.apply_op(ConfigOperation::SetProviderModel( + ProviderId::from("anthropic".to_string()), + ModelId::new("claude-3"), )); let expected = SessionConfig::default() - .provider_id("openai".to_string()) - .model_id("gpt-4".to_string()); + .provider_id("anthropic".to_string()) + .model_id("claude-3".to_string()); assert_eq!(fixture.session, Some(expected)); } #[test] - fn test_apply_op_set_model_for_different_provider_replaces_session() { + fn test_apply_op_set_provider_model_replaces_existing_session() { let mut fixture = fixture_env(); fixture.session = Some( SessionConfig::default() .provider_id("openai".to_string()) .model_id("gpt-4".to_string()), ); - fixture.apply_op(ConfigOperation::SetModel( + fixture.apply_op(ConfigOperation::SetProviderModel( ProviderId::from("anthropic".to_string()), ModelId::new("claude-3"), )); diff --git a/crates/forge_main/src/cli.rs b/crates/forge_main/src/cli.rs index 31a8ef47b4..cd1736db23 100644 --- a/crates/forge_main/src/cli.rs +++ b/crates/forge_main/src/cli.rs @@ -517,18 +517,13 @@ pub struct ConfigGetArgs { /// Type-safe subcommands for `forge config set`. #[derive(Subcommand, Debug, Clone)] pub enum ConfigSetField { - /// Set the active model. - Model { - /// Model ID to set as default. - model: ModelId, - }, - /// Set the active provider. + /// Set the active provider and model. Provider { /// Provider ID to set as default. provider: ProviderId, /// Optional model ID to set simultaneously, skipping interactive model - /// selection. + /// selection. When omitted, an interactive prompt is shown. #[arg(long)] model: Option, }, @@ -822,29 +817,6 @@ mod tests { assert_eq!(actual, expected); } - #[test] - fn test_config_set_with_model() { - let fixture = Cli::parse_from([ - "forge", - "config", - "set", - "model", - "anthropic/claude-sonnet-4", - ]); - let actual = match fixture.subcommands { - Some(TopLevelCommand::Config(config)) => match config.command { - ConfigCommand::Set(args) => match args.field { - ConfigSetField::Model { model } => Some(model.as_str().to_string()), - _ => None, - }, - _ => None, - }, - _ => None, - }; - let expected = Some("anthropic/claude-sonnet-4".to_string()); - assert_eq!(actual, expected); - } - #[test] fn test_config_set_with_provider() { let fixture = Cli::parse_from(["forge", "config", "set", "provider", "OpenAI"]); diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 1c935eb133..537f06c7fa 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -2025,7 +2025,7 @@ impl A + Send + Sync> UI { async fn select_model( &mut self, provider_filter: Option, - ) -> Result> { + ) -> Result> { // Check if provider is set otherwise first ask to select a provider if self.api.get_default_provider().await.is_err() { self.on_provider_selection().await?; @@ -2123,21 +2123,21 @@ impl A + Send + Sync> UI { return Ok(None); } - // Build a flat list of (ModelId, display_line) for the data rows. + // Build a flat list of (ProviderId, ModelId) for the data rows. // The first line is the header; data rows follow in the same order as // the Info entries (sorted by provider, then model within provider). - let mut model_ids: Vec = Vec::new(); + let mut provider_model_ids: Vec<(ProviderId, ModelId)> = Vec::new(); for pm in &all_provider_models { for model in &pm.models { - model_ids.push(model.id.clone()); + provider_model_ids.push((pm.provider_id.clone(), model.id.clone())); } } // Create display items: header line first, then data lines paired with - // model IDs. + // provider/model IDs. #[derive(Clone)] struct ModelRow { - model_id: Option, + provider_model: Option<(ProviderId, ModelId)>, display: String, } impl std::fmt::Display for ModelRow { @@ -2148,11 +2148,11 @@ impl A + Send + Sync> UI { let mut rows: Vec = Vec::with_capacity(all_lines.len()); // Header row (non-selectable via header_lines=1) - rows.push(ModelRow { model_id: None, display: all_lines[0].to_string() }); + rows.push(ModelRow { provider_model: None, display: all_lines[0].to_string() }); // Data rows for (i, line) in all_lines.iter().skip(1).enumerate() { rows.push(ModelRow { - model_id: model_ids.get(i).cloned(), + provider_model: provider_model_ids.get(i).cloned(), display: line.to_string(), }); } @@ -2165,7 +2165,11 @@ impl A + Send + Sync> UI { .await; let starting_cursor = current_model .as_ref() - .and_then(|current| model_ids.iter().position(|id| id == current)) + .and_then(|current| { + provider_model_ids + .iter() + .position(|(_, mid)| mid == current) + }) .unwrap_or(0); match ForgeWidget::select("Model", rows) @@ -2173,7 +2177,7 @@ impl A + Send + Sync> UI { .with_header_lines(1) .prompt()? { - Some(row) => Ok(row.model_id), + Some(row) => Ok(row.provider_model), None => Ok(None), } } @@ -2648,17 +2652,19 @@ impl A + Send + Sync> UI { &mut self, provider_filter: Option, ) -> Result> { - // Select a model - let model_option = self.select_model(provider_filter).await?; + // Select a provider+model pair + let selection = self.select_model(provider_filter).await?; // If no model was selected (user canceled), return early - let model = match model_option { - Some(model) => model, + let (provider_id, model) = match selection { + Some(pair) => pair, None => return Ok(None), }; - // Update the operating model via API - self.api.set_default_model(model.clone()).await?; + // Update both provider and model atomically via API + self.api + .set_provider_and_model(provider_id, model.clone()) + .await?; // Update the UI state with the new model self.update_model(Some(model.clone())); @@ -2714,8 +2720,8 @@ impl A + Send + Sync> UI { self.finalize_provider_activation(provider, model).await } - /// Finalizes provider activation by setting it as default and ensuring - /// a compatible model is selected. + /// Finalizes provider activation by ensuring a compatible model is + /// selected and then atomically setting both provider and model. /// When `model` is `Some`, the interactive model selection is skipped and /// the provided model is validated and set directly. async fn finalize_provider_activation( @@ -2723,21 +2729,19 @@ impl A + Send + Sync> UI { provider: Provider, model: Option, ) -> Result<()> { - // Set the provider via API - self.api.set_default_provider(provider.id.clone()).await?; - - self.writeln_title( - TitleFormat::action(format!("{}", provider.id)) - .sub_title("is now the default provider"), - )?; - - // If a model was pre-selected (e.g. from :model), validate and set it - // directly without prompting + // If a model was pre-selected (e.g. from :model), validate and set both + // provider and model atomically without prompting if let Some(model) = model { let model_id = self .validate_model(model.as_str(), Some(&provider.id)) .await?; - self.api.set_default_model(model_id.clone()).await?; + self.api + .set_provider_and_model(provider.id.clone(), model_id.clone()) + .await?; + self.writeln_title( + TitleFormat::action(format!("{}", provider.id)) + .sub_title("is now the default provider"), + )?; self.writeln_title( TitleFormat::action(model_id.as_str()).sub_title("is now the default model"), )?; @@ -2746,18 +2750,30 @@ impl A + Send + Sync> UI { // Check if the current model is available for the new provider let current_model = self.api.get_default_model().await; - if let Some(current_model) = current_model { + let need_model_selection = if let Some(current_model) = current_model { let models = self.get_models().await?; - let model_available = models.iter().any(|m| m.id == current_model); - - if !model_available { - // Prompt user to select a new model, scoped to the activated provider - self.writeln_title(TitleFormat::info("Please select a new model"))?; - self.on_model_selection(Some(provider.id.clone())).await?; - } + !models.iter().any(|m| m.id == current_model) } else { - // No model set, select one now scoped to the activated provider + true + }; + + if need_model_selection { + // Prompt user to select a new model, scoped to the activated provider. + // on_model_selection calls set_provider_and_model atomically. + self.writeln_title(TitleFormat::info("Please select a new model"))?; self.on_model_selection(Some(provider.id.clone())).await?; + } else { + // Current model is compatible — set provider and current model together + let current_model = self.api.get_default_model().await.expect( + "model must be present since need_model_selection is false", + ); + self.api + .set_provider_and_model(provider.id.clone(), current_model) + .await?; + self.writeln_title( + TitleFormat::action(format!("{}", provider.id)) + .sub_title("is now the default provider"), + )?; } Ok(()) @@ -3407,13 +3423,6 @@ impl A + Send + Sync> UI { let provider = self.api.get_provider(&provider).await?; self.activate_provider_with_model(provider, model).await?; } - ConfigSetField::Model { model } => { - let model_id = self.validate_model(model.as_str(), None).await?; - self.api.set_default_model(model_id.clone()).await?; - self.writeln_title( - TitleFormat::action(model_id.as_str()).sub_title("is now the default model"), - )?; - } ConfigSetField::Commit { provider, model } => { // Validate provider exists and model belongs to that specific provider let validated_model = self.validate_model(model.as_str(), Some(&provider)).await?; diff --git a/crates/forge_services/src/app_config.rs b/crates/forge_services/src/app_config.rs index 1ef9f182a0..94d815c6a0 100644 --- a/crates/forge_services/src/app_config.rs +++ b/crates/forge_services/src/app_config.rs @@ -39,8 +39,13 @@ impl AppConfigService .ok_or_else(|| forge_domain::Error::NoDefaultProvider.into()) } - async fn set_default_provider(&self, provider_id: ProviderId) -> anyhow::Result<()> { - self.update(ConfigOperation::SetProvider(provider_id)).await + async fn set_provider_and_model( + &self, + provider_id: ProviderId, + model: ModelId, + ) -> anyhow::Result<()> { + self.update(ConfigOperation::SetProviderModel(provider_id, model)) + .await } async fn get_provider_model( @@ -79,19 +84,6 @@ impl AppConfigService } } - async fn set_default_model(&self, model: ModelId) -> anyhow::Result<()> { - let env = self.infra.get_environment(); - let provider_id = env - .session - .as_ref() - .and_then(|s| s.provider_id.as_ref()) - .map(|id| ProviderId::from(id.clone())) - .ok_or(forge_domain::Error::NoDefaultProvider)?; - - self.update(ConfigOperation::SetModel(provider_id, model)) - .await - } - async fn get_commit_config(&self) -> anyhow::Result> { let env = self.infra.get_environment(); Ok(env.commit.map(|mc| CommitConfig { @@ -227,24 +219,14 @@ mod tests { let mut env = env.lock().unwrap(); for op in ops { match op { - ConfigOperation::SetProvider(pid) => { - let pid_str = pid.as_ref().to_string(); - env.session = Some(match env.session.take() { - Some(sc) => sc.provider_id(pid_str), - None => SessionConfig::default().provider_id(pid_str), - }); - } - ConfigOperation::SetModel(pid, mid) => { + ConfigOperation::SetProviderModel(pid, mid) => { let pid_str = pid.as_ref().to_string(); let mid_str = mid.to_string(); - env.session = Some(match env.session.take() { - Some(sc) if sc.provider_id.as_deref() == Some(&pid_str) => { - sc.model_id(mid_str) - } - _ => SessionConfig::default() + env.session = Some( + SessionConfig::default() .provider_id(pid_str) .model_id(mid_str), - }); + ); } ConfigOperation::SetCommitConfig(commit) => { env.commit = commit.provider.as_ref().zip(commit.model.as_ref()).map( @@ -372,7 +354,9 @@ mod tests { let fixture = MockInfra::new(); let service = ForgeAppConfigService::new(Arc::new(fixture.clone())); - service.set_default_provider(ProviderId::ANTHROPIC).await?; + service + .set_provider_and_model(ProviderId::ANTHROPIC, ModelId::new("claude-3")) + .await?; let actual = service.get_default_provider().await?; let expected = ProviderId::ANTHROPIC; @@ -389,7 +373,9 @@ mod tests { let service = ForgeAppConfigService::new(Arc::new(fixture.clone())); // Set OpenAI as the default provider in config - service.set_default_provider(ProviderId::OPENAI).await?; + service + .set_provider_and_model(ProviderId::OPENAI, ModelId::new("gpt-4")) + .await?; // Should return the provider ID even if provider is not available // Validation happens when getting the actual provider via ProviderService @@ -400,21 +386,24 @@ mod tests { } #[tokio::test] - async fn test_set_default_provider() -> anyhow::Result<()> { + async fn test_set_provider_and_model() -> anyhow::Result<()> { let fixture = MockInfra::new(); let service = ForgeAppConfigService::new(Arc::new(fixture.clone())); - service.set_default_provider(ProviderId::ANTHROPIC).await?; + service + .set_provider_and_model(ProviderId::ANTHROPIC, ModelId::new("claude-3")) + .await?; let env = fixture.get_environment(); - let actual = env + let actual_provider = env .session .as_ref() .and_then(|s| s.provider_id.as_ref()) .map(|id| ProviderId::from(id.clone())); - let expected = Some(ProviderId::ANTHROPIC); + let actual_model = env.session.as_ref().and_then(|s| s.model_id.as_deref()); - assert_eq!(actual, expected); + assert_eq!(actual_provider, Some(ProviderId::ANTHROPIC)); + assert_eq!(actual_model, Some("claude-3")); Ok(()) } @@ -434,10 +423,8 @@ mod tests { let fixture = MockInfra::new(); let service = ForgeAppConfigService::new(Arc::new(fixture.clone())); - // Set OpenAI as the default provider first - service.set_default_provider(ProviderId::OPENAI).await?; service - .set_default_model("gpt-4".to_string().into()) + .set_provider_and_model(ProviderId::OPENAI, "gpt-4".to_string().into()) .await?; let actual = service .get_provider_model(Some(&ProviderId::OPENAI)) @@ -449,43 +436,21 @@ mod tests { } #[tokio::test] - async fn test_set_default_model() -> anyhow::Result<()> { - let fixture = MockInfra::new(); - let service = ForgeAppConfigService::new(Arc::new(fixture.clone())); - - // Set OpenAI as the default provider first - service.set_default_provider(ProviderId::OPENAI).await?; - service - .set_default_model("gpt-4".to_string().into()) - .await?; - - let env = fixture.get_environment(); - let actual = env.session.as_ref().and_then(|s| s.model_id.as_deref()); - let expected = Some("gpt-4"); - - assert_eq!(actual, expected); - Ok(()) - } - - #[tokio::test] - async fn test_set_multiple_default_models() -> anyhow::Result<()> { + async fn test_set_provider_and_model_replaces_previous() -> anyhow::Result<()> { let fixture = MockInfra::new(); let service = ForgeAppConfigService::new(Arc::new(fixture.clone())); - // Set model for OpenAI first - service.set_default_provider(ProviderId::OPENAI).await?; + // Set OpenAI first service - .set_default_model("gpt-4".to_string().into()) + .set_provider_and_model(ProviderId::OPENAI, "gpt-4".to_string().into()) .await?; - // Then switch to Anthropic and set its model - service.set_default_provider(ProviderId::ANTHROPIC).await?; + // Then switch to Anthropic service - .set_default_model("claude-3".to_string().into()) + .set_provider_and_model(ProviderId::ANTHROPIC, "claude-3".to_string().into()) .await?; - // ForgeConfig only tracks a single active session, so the last - // provider/model pair wins + // The last provider/model pair wins let env = fixture.get_environment(); let actual_provider = env .session diff --git a/shell-plugin/lib/actions/config.zsh b/shell-plugin/lib/actions/config.zsh index a05a331b72..fa963afd11 100644 --- a/shell-plugin/lib/actions/config.zsh +++ b/shell-plugin/lib/actions/config.zsh @@ -148,7 +148,7 @@ function _forge_pick_model() { } # Action handler: Select model (across all configured providers) -# When the selected model belongs to a different provider, switches it first. +# Always calls `forge config set provider --model ` on selection. function _forge_action_model() { local input_text="$1" ( @@ -156,27 +156,19 @@ function _forge_action_model() { local current_model current_provider current_model=$($_FORGE_BIN config get model 2>/dev/null) # config get provider returns the display name (e.g. "OpenAI"), - # which corresponds to porcelain field 3 (provider display) + # used to pre-select the current entry in fzf (field 3 = provider display) current_provider=$($_FORGE_BIN config get provider 2>/dev/null) local selected selected=$(_forge_pick_model "Model ❯ " "$current_model" "$input_text" "$current_provider" 3) if [[ -n "$selected" ]]; then - # Field 1 = model_id (raw), field 3 = provider display name, - # field 4 = provider_id (raw, for config set) - local model_id provider_display provider_id - read -r model_id provider_display provider_id <<<$(echo "$selected" | awk -F ' +' '{print $1, $3, $4}') + # Field 1 = model_id (raw), field 4 = provider_id (raw, for config set) + local model_id provider_id + read -r model_id provider_id <<<$(echo "$selected" | awk -F ' +' '{print $1, $4}') model_id=${model_id//[[:space:]]/} provider_id=${provider_id//[[:space:]]/} - provider_display=${provider_display//[[:space:]]/} - # Switch provider first if it differs from the current one - # current_provider (fetched above) is the display name, compare against that - if [[ -n "$provider_display" && "$provider_display" != "$current_provider" ]]; then - _forge_exec_interactive config set provider "$provider_id" --model "$model_id" - return - fi - _forge_exec config set model "$model_id" + _forge_exec_interactive config set provider "$provider_id" --model "$model_id" fi ) } From 53fb829856d2d125ea0571dde1be3c326988e245 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Fri, 27 Mar 2026 11:44:03 +0000 Subject: [PATCH 2/2] [autofix.ci] apply automated fixes --- crates/forge_domain/src/env.rs | 6 +----- crates/forge_main/src/ui.rs | 8 +++++--- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/crates/forge_domain/src/env.rs b/crates/forge_domain/src/env.rs index 959486ea1f..35eae82a36 100644 --- a/crates/forge_domain/src/env.rs +++ b/crates/forge_domain/src/env.rs @@ -176,11 +176,7 @@ impl Environment { ConfigOperation::SetProviderModel(provider_id, model_id) => { let pid = provider_id.as_ref().to_string(); let mid = model_id.to_string(); - self.session = Some( - SessionConfig::default() - .provider_id(pid) - .model_id(mid), - ); + self.session = Some(SessionConfig::default().provider_id(pid).model_id(mid)); } ConfigOperation::SetCommitConfig(commit) => { self.commit = diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 537f06c7fa..fe98d4f03a 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -2764,9 +2764,11 @@ impl A + Send + Sync> UI { self.on_model_selection(Some(provider.id.clone())).await?; } else { // Current model is compatible — set provider and current model together - let current_model = self.api.get_default_model().await.expect( - "model must be present since need_model_selection is false", - ); + let current_model = self + .api + .get_default_model() + .await + .expect("model must be present since need_model_selection is false"); self.api .set_provider_and_model(provider.id.clone(), current_model) .await?;