diff --git a/crates/forge_api/src/api.rs b/crates/forge_api/src/api.rs index 3f5a2e4aca..806b6af8e8 100644 --- a/crates/forge_api/src/api.rs +++ b/crates/forge_api/src/api.rs @@ -122,9 +122,6 @@ 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<()>; - /// Updates the caller's default provider and model together, ensuring all /// commands resolve a consistent pair without requiring a follow-up model /// selection call. diff --git a/crates/forge_api/src/forge_api.rs b/crates/forge_api/src/forge_api.rs index 2b89dee487..69fc791343 100644 --- a/crates/forge_api/src/forge_api.rs +++ b/crates/forge_api/src/forge_api.rs @@ -225,13 +225,6 @@ impl anyhow::Result<()> { - let result = self.services.set_default_provider(provider_id).await; - // Invalidate cache for agents - let _ = self.services.reload_agents().await; - result - } - async fn user_info(&self) -> Result> { let provider = self.get_default_provider().await?; if let Some(api_key) = provider.api_key() { diff --git a/crates/forge_app/src/command_generator.rs b/crates/forge_app/src/command_generator.rs index 42ca8b0080..dcc744e9be 100644 --- a/crates/forge_app/src/command_generator.rs +++ b/crates/forge_app/src/command_generator.rs @@ -251,10 +251,6 @@ mod tests { Ok(ProviderId::OPENAI) } - async fn set_default_provider(&self, _provider_id: ProviderId) -> Result<()> { - Ok(()) - } - async fn get_provider_model( &self, _provider_id: Option<&ProviderId>, diff --git a/crates/forge_app/src/git_app.rs b/crates/forge_app/src/git_app.rs index bf50f44a80..91c9f5ae34 100644 --- a/crates/forge_app/src/git_app.rs +++ b/crates/forge_app/src/git_app.rs @@ -312,8 +312,10 @@ impl GitApp { // Resolve provider and model: commit config takes priority over agent defaults. // If the configured provider is unavailable (e.g. logged out), fall back to the // agent's provider/model with a warning. - let (provider, model) = match commit_config.and_then(|c| c.provider.zip(c.model)) { - Some((provider_id, commit_model)) => { + let (provider, model) = match commit_config { + Some(c) => { + let provider_id = c.provider; + let commit_model = c.model; match self.services.get_provider(provider_id).await { Ok(provider) => { match self.services.refresh_provider_credential(provider).await { diff --git a/crates/forge_app/src/services.rs b/crates/forge_app/src/services.rs index e0983996b2..f45dfb7848 100644 --- a/crates/forge_app/src/services.rs +++ b/crates/forge_app/src/services.rs @@ -185,12 +185,6 @@ 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( - &self, - provider_id: forge_domain::ProviderId, - ) -> anyhow::Result<()>; - /// Gets the user's default model for a specific provider or the currently /// active provider. When provider_id is None, uses the currently active /// provider. @@ -978,15 +972,6 @@ impl AppConfigService for I { self.config_service().get_default_provider().await } - async fn set_default_provider( - &self, - provider_id: forge_domain::ProviderId, - ) -> anyhow::Result<()> { - self.config_service() - .set_default_provider(provider_id) - .await - } - async fn get_provider_model( &self, provider_id: Option<&forge_domain::ProviderId>, diff --git a/crates/forge_config/src/legacy.rs b/crates/forge_config/src/legacy.rs index 22f35ce52b..cefd3ef185 100644 --- a/crates/forge_config/src/legacy.rs +++ b/crates/forge_config/src/legacy.rs @@ -57,18 +57,23 @@ impl LegacyConfig { /// Converts a [`LegacyConfig`] into the fields of [`ForgeConfig`] that it /// covers, leaving all other fields at their defaults (`None`). fn into_forge_config(self) -> ForgeConfig { - let session = self.provider.as_deref().map(|provider_id| { - let model_id = self.model.get(provider_id).cloned(); - ModelConfig { provider_id: Some(provider_id.to_string()), model_id } + let session = self.provider.as_deref().and_then(|provider_id| { + self.model + .get(provider_id) + .map(|model_id| ModelConfig::new(provider_id, model_id.as_str())) }); - let commit = self - .commit - .map(|c| ModelConfig { provider_id: c.provider, model_id: c.model }); + let commit = self.commit.and_then(|c| { + c.provider + .zip(c.model) + .map(|(pid, mid)| ModelConfig::new(pid, mid)) + }); - let suggest = self - .suggest - .map(|s| ModelConfig { provider_id: s.provider, model_id: s.model }); + let suggest = self.suggest.and_then(|s| { + s.provider + .zip(s.model) + .map(|(pid, mid)| ModelConfig::new(pid, mid)) + }); ForgeConfig { session, commit, suggest, ..Default::default() } } diff --git a/crates/forge_config/src/model.rs b/crates/forge_config/src/model.rs index c993222700..c3e8acc6b5 100644 --- a/crates/forge_config/src/model.rs +++ b/crates/forge_config/src/model.rs @@ -9,13 +9,18 @@ pub type ProviderId = String; pub type ModelId = String; /// Pairs a provider and model together for a specific operation. -#[derive( - Default, Debug, Setters, Clone, PartialEq, Serialize, Deserialize, JsonSchema, fake::Dummy, -)] -#[setters(strip_option, into)] +#[derive(Debug, Setters, Clone, PartialEq, Serialize, Deserialize, JsonSchema, fake::Dummy)] +#[setters(into)] pub struct ModelConfig { /// The provider to use for this operation. - pub provider_id: Option, + pub provider_id: String, /// The model to use for this operation. - pub model_id: Option, + pub model_id: String, +} + +impl ModelConfig { + /// Creates a new [`ModelConfig`] with the given provider and model IDs. + pub fn new(provider_id: impl Into, model_id: impl Into) -> Self { + Self { provider_id: provider_id.into(), model_id: model_id.into() } + } } diff --git a/crates/forge_config/src/reader.rs b/crates/forge_config/src/reader.rs index 54a09375c7..0c510d49e1 100644 --- a/crates/forge_config/src/reader.rs +++ b/crates/forge_config/src/reader.rs @@ -197,10 +197,7 @@ mod tests { // carries session/commit/suggest (all other fields are None) and layer // it on top of the embedded defaults. The default values must survive. let legacy = ForgeConfig { - session: Some(ModelConfig { - provider_id: Some("anthropic".to_string()), - model_id: Some("claude-3".to_string()), - }), + session: Some(ModelConfig::new("anthropic", "claude-3")), ..Default::default() }; let legacy_toml = toml_edit::ser::to_string_pretty(&legacy).unwrap(); @@ -215,10 +212,7 @@ mod tests { // Session should come from the legacy layer assert_eq!( actual.session, - Some(ModelConfig { - provider_id: Some("anthropic".to_string()), - model_id: Some("claude-3".to_string()), - }) + Some(ModelConfig::new("anthropic", "claude-3")) ); // Default values from .forge.toml must be retained, not reset to zero @@ -242,10 +236,7 @@ mod tests { .build() .unwrap(); - let expected = Some(ModelConfig { - provider_id: Some("fake-provider".to_string()), - model_id: Some("fake-model".to_string()), - }); + let expected = Some(ModelConfig::new("fake-provider", "fake-model")); assert_eq!(actual.session, expected); } } diff --git a/crates/forge_domain/src/commit_config.rs b/crates/forge_domain/src/commit_config.rs index f075a0ed2d..dd5251def6 100644 --- a/crates/forge_domain/src/commit_config.rs +++ b/crates/forge_domain/src/commit_config.rs @@ -1,5 +1,4 @@ use derive_setters::Setters; -use merge::Merge; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -11,19 +10,19 @@ use crate::{ModelId, ProviderId}; /// generation, instead of using the active agent's provider and model. This is /// useful when you want to use a cheaper or faster model for simple commit /// message generation. -#[derive(Default, Debug, Clone, Serialize, Deserialize, Merge, Setters, JsonSchema, PartialEq)] -#[setters(strip_option, into)] +#[derive(Debug, Clone, Serialize, Deserialize, Setters, JsonSchema, PartialEq)] +#[setters(into)] pub struct CommitConfig { /// Provider ID to use for commit message generation. - /// If not specified, the active agent's provider will be used. - #[serde(default, skip_serializing_if = "Option::is_none")] - #[merge(strategy = crate::merge::option)] - pub provider: Option, + pub provider: ProviderId, /// Model ID to use for commit message generation. - /// If not specified, the provider's default model or the active agent's - /// model will be used. - #[serde(default, skip_serializing_if = "Option::is_none")] - #[merge(strategy = crate::merge::option)] - pub model: Option, + pub model: ModelId, +} + +impl CommitConfig { + /// Creates a new [`CommitConfig`] with the given provider and model. + pub fn new(provider: impl Into, model: impl Into) -> Self { + Self { provider: provider.into(), model: model.into() } + } } diff --git a/crates/forge_domain/src/env.rs b/crates/forge_domain/src/env.rs index 3119ebb6bb..33abc3378b 100644 --- a/crates/forge_domain/src/env.rs +++ b/crates/forge_domain/src/env.rs @@ -11,13 +11,20 @@ use crate::{Effort, ModelId, ProviderId}; /// /// Used to represent an active session, decoupled from the on-disk /// configuration format. -#[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize, Setters)] -#[setters(strip_option, into)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Setters)] +#[setters(into)] pub struct SessionConfig { /// The active provider ID (e.g. `"anthropic"`). - pub provider_id: Option, + pub provider_id: String, /// The model ID to use with this provider. - pub model_id: Option, + pub model_id: String, +} + +impl SessionConfig { + /// Creates a new [`SessionConfig`] with the given provider and model IDs. + pub fn new(provider_id: impl Into, model_id: impl Into) -> Self { + Self { provider_id: provider_id.into(), model_id: model_id.into() } + } } /// All discrete mutations that can be applied to the application configuration. @@ -27,9 +34,7 @@ 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. + /// Set the model for the given provider, replacing any existing session. SetModel(ProviderId, ModelId), /// Set the commit-message generation configuration. SetCommitConfig(crate::CommitConfig), diff --git a/crates/forge_infra/src/env.rs b/crates/forge_infra/src/env.rs index cffa3f5081..3eb9c8cd67 100644 --- a/crates/forge_infra/src/env.rs +++ b/crates/forge_infra/src/env.rs @@ -32,36 +32,17 @@ pub fn to_environment(cwd: PathBuf) -> Environment { /// persisted config without an intermediate `Environment` round-trip. fn apply_config_op(fc: &mut ForgeConfig, op: ConfigOperation) { match op { - ConfigOperation::SetProvider(pid) => { - let session = fc.session.get_or_insert_with(ModelConfig::default); - session.provider_id = Some(pid.as_ref().to_string()); - } ConfigOperation::SetModel(pid, mid) => { - let pid_str = pid.as_ref().to_string(); - let mid_str = mid.to_string(); - let session = fc.session.get_or_insert_with(ModelConfig::default); - if session.provider_id.as_deref() == Some(&pid_str) { - session.model_id = Some(mid_str); - } else { - fc.session = - Some(ModelConfig { provider_id: Some(pid_str), model_id: Some(mid_str) }); - } + fc.session = Some(ModelConfig::new(&**pid, mid.as_str())); } ConfigOperation::SetCommitConfig(commit) => { - fc.commit = commit - .provider - .as_ref() - .zip(commit.model.as_ref()) - .map(|(pid, mid)| ModelConfig { - provider_id: Some(pid.as_ref().to_string()), - model_id: Some(mid.to_string()), - }); + fc.commit = Some(ModelConfig::new(&**commit.provider, commit.model.as_str())); } ConfigOperation::SetSuggestConfig(suggest) => { - fc.suggest = Some(ModelConfig { - provider_id: Some(suggest.provider.as_ref().to_string()), - model_id: Some(suggest.model.to_string()), - }); + fc.suggest = Some(ModelConfig::new( + &**suggest.provider, + suggest.model.as_str(), + )); } ConfigOperation::SetReasoningEffort(effort) => { let config_effort = match effort { @@ -228,22 +209,25 @@ mod tests { } #[test] - fn test_apply_config_op_set_provider() { - use forge_domain::ProviderId; + fn test_apply_config_op_set_model_creates_complete_session() { + use forge_domain::{ModelId, ProviderId}; let mut fixture = ForgeConfig::default(); apply_config_op( &mut fixture, - ConfigOperation::SetProvider(ProviderId::ANTHROPIC), + ConfigOperation::SetModel( + ProviderId::ANTHROPIC, + ModelId::new("claude-3-5-sonnet-20241022"), + ), ); - let actual = fixture - .session - .as_ref() - .and_then(|s| s.provider_id.as_deref()); - let expected = Some("anthropic"); + let actual_provider = fixture.session.as_ref().map(|s| s.provider_id.as_str()); + let actual_model = fixture.session.as_ref().map(|s| s.model_id.as_str()); + let expected_provider = Some("anthropic"); + let expected_model = Some("claude-3-5-sonnet-20241022"); - assert_eq!(actual, expected); + assert_eq!(actual_provider, expected_provider); + assert_eq!(actual_model, expected_model); } #[test] @@ -251,10 +235,7 @@ mod tests { use forge_domain::{ModelId, ProviderId}; let mut fixture = ForgeConfig { - session: Some(ModelConfig { - provider_id: Some("anthropic".to_string()), - model_id: None, - }), + session: Some(ModelConfig::new("anthropic", "old-model")), ..Default::default() }; @@ -266,7 +247,7 @@ mod tests { ), ); - let actual = fixture.session.as_ref().and_then(|s| s.model_id.as_deref()); + let actual = fixture.session.as_ref().map(|s| s.model_id.as_str()); let expected = Some("claude-3-5-sonnet-20241022"); assert_eq!(actual, expected); @@ -277,10 +258,7 @@ mod tests { use forge_domain::{ModelId, ProviderId}; let mut fixture = ForgeConfig { - session: Some(ModelConfig { - provider_id: Some("openai".to_string()), - model_id: Some("gpt-4".to_string()), - }), + session: Some(ModelConfig::new("openai", "gpt-4")), ..Default::default() }; @@ -292,11 +270,8 @@ mod tests { ), ); - let actual_provider = fixture - .session - .as_ref() - .and_then(|s| s.provider_id.as_deref()); - let actual_model = fixture.session.as_ref().and_then(|s| s.model_id.as_deref()); + let actual_provider = fixture.session.as_ref().map(|s| s.provider_id.as_str()); + let actual_model = fixture.session.as_ref().map(|s| s.model_id.as_str()); assert_eq!(actual_provider, Some("anthropic")); assert_eq!(actual_model, Some("claude-3-5-sonnet-20241022")); diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 36caa17c60..6f866f82af 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -1339,13 +1339,11 @@ impl A + Send + Sync> UI let commit_config = self.api.get_commit_config().await.ok().flatten(); let commit_provider = commit_config .as_ref() - .and_then(|c| c.provider.as_ref()) - .map(|p| p.to_string()) + .map(|c| c.provider.as_ref().to_string()) .unwrap_or_else(|| markers::EMPTY.to_string()); let commit_model = commit_config .as_ref() - .and_then(|c| c.model.as_ref()) - .map(|m| m.as_str().to_string()) + .map(|c| c.model.as_str().to_string()) .unwrap_or_else(|| markers::EMPTY.to_string()); let suggest_config = self.api.get_suggest_config().await.ok().flatten(); @@ -2858,14 +2856,14 @@ 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; - let needs_model_selection = match current_model { + let needs_model_selection = match ¤t_model { None => true, Some(current_model) => { let provider_models = self.api.get_all_provider_models().await?; let model_available = provider_models .iter() .find(|pm| pm.provider_id == provider.id) - .map(|pm| pm.models.iter().any(|m| m.id == current_model)) + .map(|pm| pm.models.iter().any(|m| &m.id == current_model)) .unwrap_or(false); !model_available } @@ -2881,9 +2879,11 @@ impl A + Send + Sync> UI return Ok(()); } } else { - // Set the provider via API - // Only reaches here if model is confirmed — safe to write provider now - self.api.set_default_provider(provider.id.clone()).await?; + // Model is compatible with the new provider — set both atomically + let model_id = current_model.expect("current_model is Some in the else branch"); + self.api + .set_default_provider_and_model(provider.id.clone(), model_id) + .await?; self.writeln_title( TitleFormat::action(format!("{}", provider.id)) @@ -3544,9 +3544,8 @@ impl A + Send + Sync> UI 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?; - let commit_config = forge_domain::CommitConfig::default() - .provider(provider.clone()) - .model(validated_model.clone()); + let commit_config = + forge_domain::CommitConfig::new(provider.clone(), validated_model.clone()); self.api.set_commit_config(commit_config).await?; self.writeln_title( TitleFormat::action(validated_model.as_str()) @@ -3609,16 +3608,8 @@ impl A + Send + Sync> UI let commit_config = self.api.get_commit_config().await?; match commit_config { Some(config) => { - let provider = config - .provider - .map(|p| p.as_ref().to_string()) - .unwrap_or_else(|| "Not set".to_string()); - let model = config - .model - .map(|m| m.as_str().to_string()) - .unwrap_or_else(|| "Not set".to_string()); - self.writeln(provider)?; - self.writeln(model)?; + self.writeln(config.provider.as_ref().to_string())?; + self.writeln(config.model.as_str().to_string())?; } None => self.writeln("Commit: Not set")?, } diff --git a/crates/forge_services/src/agent_registry.rs b/crates/forge_services/src/agent_registry.rs index dc718e3682..22060f74fd 100644 --- a/crates/forge_services/src/agent_registry.rs +++ b/crates/forge_services/src/agent_registry.rs @@ -79,18 +79,8 @@ impl ForgeAgentRegistryService { .session .as_ref() .ok_or(Error::NoDefaultProvider)?; - let provider_id = session - .provider_id - .as_ref() - .map(|id| ProviderId::from(id.clone())) - .ok_or(Error::NoDefaultProvider)?; - let model_id = session - .model_id - .as_ref() - .map(|id| ModelId::new(id.clone())) - .ok_or_else(|| { - anyhow::anyhow!("No default model configured for provider {}", provider_id) - })?; + let provider_id = ProviderId::from(session.provider_id.clone()); + let model_id = ModelId::new(session.model_id.clone()); let agents = self.repository.get_agents(provider_id, model_id).await?; diff --git a/crates/forge_services/src/app_config.rs b/crates/forge_services/src/app_config.rs index eaefedd83f..eb0dcc3984 100644 --- a/crates/forge_services/src/app_config.rs +++ b/crates/forge_services/src/app_config.rs @@ -37,20 +37,10 @@ impl AppConfigService config .session .as_ref() - .and_then(|s| s.provider_id.as_ref()) - .map(|id| ProviderId::from(id.clone())) + .map(|s| ProviderId::from(s.provider_id.clone())) .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.clone())) - .await?; - let mut config = self.config.lock().unwrap(); - let session = config.session.get_or_insert_with(Default::default); - session.provider_id = Some(provider_id.as_ref().to_string()); - Ok(()) - } - async fn get_provider_model( &self, provider_id: Option<&ProviderId>, @@ -62,26 +52,17 @@ impl AppConfigService .as_ref() .ok_or(forge_domain::Error::NoDefaultProvider)?; - let active_provider = session - .provider_id - .as_ref() - .map(|id| ProviderId::from(id.clone())); + let active_provider = ProviderId::from(session.provider_id.clone()); let provider_id = match provider_id { Some(id) => id, - None => active_provider - .as_ref() - .ok_or(forge_domain::Error::NoDefaultProvider)?, + None => &active_provider, }; // Only return the model if the session's provider matches the requested // provider - if session.provider_id.as_deref() == Some(provider_id.as_ref()) { - session - .model_id - .as_ref() - .map(ModelId::new) - .ok_or_else(|| forge_domain::Error::no_default_model(provider_id.clone()).into()) + if session.provider_id == **provider_id { + Ok(ModelId::new(session.model_id.as_str())) } else { Err(forge_domain::Error::no_default_model(provider_id.clone()).into()) } @@ -93,8 +74,7 @@ impl AppConfigService config .session .as_ref() - .and_then(|s| s.provider_id.as_ref()) - .map(|id| ProviderId::from(id.clone())) + .map(|s| ProviderId::from(s.provider_id.clone())) .ok_or(forge_domain::Error::NoDefaultProvider)? }; @@ -104,8 +84,9 @@ impl AppConfigService )) .await?; let mut config = self.config.lock().unwrap(); - let session = config.session.get_or_insert_with(Default::default); - session.model_id = Some(model.to_string()); + if let Some(session) = config.session.as_mut() { + session.model_id = model.to_string(); + } Ok(()) } @@ -114,15 +95,24 @@ impl AppConfigService provider_id: ProviderId, model: ModelId, ) -> anyhow::Result<()> { - self.update(ConfigOperation::SetModel(provider_id, model)) - .await + self.update(ConfigOperation::SetModel( + provider_id.clone(), + model.clone(), + )) + .await?; + let mut config = self.config.lock().unwrap(); + config.session = Some(forge_config::ModelConfig::new( + &**provider_id, + model.as_str(), + )); + Ok(()) } async fn get_commit_config(&self) -> anyhow::Result> { let config = self.config.lock().unwrap(); Ok(config.commit.clone().map(|mc| CommitConfig { - provider: mc.provider_id.map(ProviderId::from), - model: mc.model_id.map(ModelId::new), + provider: ProviderId::from(mc.provider_id), + model: ModelId::new(mc.model_id), })) } @@ -136,13 +126,9 @@ impl AppConfigService async fn get_suggest_config(&self) -> anyhow::Result> { let config = self.config.lock().unwrap(); - Ok(config.suggest.clone().and_then(|mc| { - mc.provider_id - .zip(mc.model_id) - .map(|(pid, mid)| SuggestConfig { - provider: ProviderId::from(pid), - model: ModelId::new(mid), - }) + Ok(config.suggest.clone().map(|mc| SuggestConfig { + provider: ProviderId::from(mc.provider_id), + model: ModelId::new(mc.model_id), })) } @@ -283,41 +269,18 @@ mod tests { let mut config = config.lock().unwrap(); for op in ops { match op { - ConfigOperation::SetProvider(pid) => { - let pid_str = pid.as_ref().to_string(); - config.session = Some(match config.session.take() { - Some(mc) => mc.provider_id(pid_str), - None => ModelConfig::default().provider_id(pid_str), - }); - } ConfigOperation::SetModel(pid, mid) => { - let pid_str = pid.as_ref().to_string(); - let mid_str = mid.to_string(); - config.session = Some(match config.session.take() { - Some(mc) if mc.provider_id.as_deref() == Some(&pid_str) => { - mc.model_id(mid_str) - } - _ => ModelConfig::default() - .provider_id(pid_str) - .model_id(mid_str), - }); + config.session = Some(ModelConfig::new(&**pid, mid.as_str())); } ConfigOperation::SetCommitConfig(commit) => { config.commit = - commit.provider.as_ref().zip(commit.model.as_ref()).map( - |(pid, mid)| { - ModelConfig::default() - .provider_id(pid.as_ref().to_string()) - .model_id(mid.to_string()) - }, - ); + Some(ModelConfig::new(&**commit.provider, commit.model.as_str())); } ConfigOperation::SetSuggestConfig(suggest) => { - config.suggest = Some( - ModelConfig::default() - .provider_id(suggest.provider.as_ref().to_string()) - .model_id(suggest.model.to_string()), - ); + config.suggest = Some(ModelConfig::new( + &**suggest.provider, + suggest.model.as_str(), + )); } ConfigOperation::SetReasoningEffort(_) => { // No-op in tests @@ -432,7 +395,9 @@ mod tests { let fixture = MockInfra::new(); let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); - service.set_default_provider(ProviderId::ANTHROPIC).await?; + service + .set_default_provider_and_model(ProviderId::ANTHROPIC, ModelId::new("claude-3")) + .await?; let actual = service.get_default_provider().await?; let expected = ProviderId::ANTHROPIC; @@ -449,7 +414,9 @@ mod tests { let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); // Set OpenAI as the default provider in config - service.set_default_provider(ProviderId::OPENAI).await?; + service + .set_default_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 @@ -459,20 +426,6 @@ mod tests { Ok(()) } - #[tokio::test] - async fn test_set_default_provider() -> anyhow::Result<()> { - let fixture = MockInfra::new(); - let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); - - service.set_default_provider(ProviderId::ANTHROPIC).await?; - - let actual = service.get_default_provider().await?; - let expected = ProviderId::ANTHROPIC; - - assert_eq!(actual, expected); - Ok(()) - } - #[tokio::test] async fn test_get_default_model_when_none_set() -> anyhow::Result<()> { let fixture = MockInfra::new(); @@ -489,10 +442,9 @@ mod tests { let fixture = MockInfra::new(); let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); - // Set OpenAI as the default provider first - service.set_default_provider(ProviderId::OPENAI).await?; + // Set OpenAI as the default provider and model service - .set_default_model("gpt-4".to_string().into()) + .set_default_provider_and_model(ProviderId::OPENAI, ModelId::new("gpt-4")) .await?; let actual = service .get_provider_model(Some(&ProviderId::OPENAI)) @@ -508,8 +460,10 @@ mod tests { let fixture = MockInfra::new(); let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); - // Set OpenAI as the default provider first - service.set_default_provider(ProviderId::OPENAI).await?; + // Set OpenAI as the default provider and model first + service + .set_default_provider_and_model(ProviderId::OPENAI, ModelId::new("gpt-4")) + .await?; service .set_default_model("gpt-4".to_string().into()) .await?; @@ -529,15 +483,13 @@ mod tests { let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); // Set model for OpenAI first - service.set_default_provider(ProviderId::OPENAI).await?; service - .set_default_model("gpt-4".to_string().into()) + .set_default_provider_and_model(ProviderId::OPENAI, ModelId::new("gpt-4")) .await?; // Then switch to Anthropic and set its model - service.set_default_provider(ProviderId::ANTHROPIC).await?; service - .set_default_model("claude-3".to_string().into()) + .set_default_provider_and_model(ProviderId::ANTHROPIC, ModelId::new("claude-3")) .await?; // ForgeConfig only tracks a single active session, so the last diff --git a/forge.schema.json b/forge.schema.json index f925ff6a5f..9ffc7067d8 100644 --- a/forge.schema.json +++ b/forge.schema.json @@ -575,19 +575,17 @@ "properties": { "model_id": { "description": "The model to use for this operation.", - "type": [ - "string", - "null" - ] + "type": "string" }, "provider_id": { "description": "The provider to use for this operation.", - "type": [ - "string", - "null" - ] + "type": "string" } - } + }, + "required": [ + "provider_id", + "model_id" + ] }, "ProviderAuthMethod": { "description": "Authentication method supported by a provider.\n\nOnly the simple (non-OAuth) methods are available here; providers that\nrequire OAuth device or authorization-code flows must be configured via the\nfile-based `provider.json` override instead.", diff --git a/plans/2026-04-07-make-provider-model-required-v1.md b/plans/2026-04-07-make-provider-model-required-v1.md new file mode 100644 index 0000000000..eb2856cd73 --- /dev/null +++ b/plans/2026-04-07-make-provider-model-required-v1.md @@ -0,0 +1,188 @@ +# Make `provider_id` and `model_id` Required Throughout the Stack + +## Objective + +Remove the `Option` wrappers from `provider_id` and `model_id` in `ModelConfig` (`forge_config`) and propagate this guarantee upward through domain types, the infra layer, service layer, API trait, and CLI — ensuring these two fields are always present together whenever a config object exists. The outer `Option` in `ForgeConfig` (representing "not yet configured") is preserved; only the inner fields become required. + +**Key invariant being enforced:** A `ModelConfig`, `SessionConfig`, or `CommitConfig` instance is only valid when it carries both a provider ID and a model ID. Partial state (provider set, model absent) is no longer representable. + +--- + +## Implementation Plan + +### Phase 1 — Core Config Types (`forge_config`) + +- [x] Task 1. **`crates/forge_config/src/model.rs` — `ModelConfig` fields** + - Change `provider_id: Option` → `provider_id: String` + - Change `model_id: Option` → `model_id: String` + - Remove `Default` from the `#[derive(...)]` list (no meaningful default for required strings) + - Change `#[setters(strip_option, into)]` → `#[setters(into)]` (strip_option is Option-specific) + - Add an explicit `pub fn new(provider_id: impl Into, model_id: impl Into) -> Self` constructor for ergonomic construction + +- [x] Task 2. **`crates/forge_config/src/legacy.rs` — `LegacyConfig::into_forge_config`** + - Session construction: Use `and_then` so that a `ModelConfig` is only created when both the provider is set AND the model for that provider is found in the map. If the model lookup fails (provider in config but not in model map), silently produce `None` for `session`. + - Commit construction: Replace the direct `Option` field struct literal with `zip()` on `c.provider` and `c.model` — only create `ModelConfig::new(pid, mid)` when both are `Some`. + - Suggest construction: Same `zip()` pattern as commit. + +- [x] Task 3. **`crates/forge_config/src/reader.rs` — test fixtures** + - Update the test at line ~200 that constructs `ModelConfig { provider_id: Some("anthropic"), model_id: Some("claude-3") }` to use `ModelConfig::new("anthropic", "claude-3")`. + +--- + +### Phase 2 — Domain Types (`forge_domain`) + +- [x] Task 4. **`crates/forge_domain/src/env.rs` — `SessionConfig`** + - Change `provider_id: Option` → `provider_id: String` + - Change `model_id: Option` → `model_id: String` + - Remove `Default` from derives + - Change `#[setters(strip_option, into)]` → `#[setters(into)]` + - Add `pub fn new(provider_id: impl Into, model_id: impl Into) -> Self` constructor + +- [x] Task 5. **`crates/forge_domain/src/env.rs` — `ConfigOperation` enum** + - Remove the `SetProvider(ProviderId)` variant entirely. This variant was the only way to create the partial (provider-only, no model) state. With required fields, there is no valid partial operation — the single `SetModel(ProviderId, ModelId)` variant becomes the sole session-mutation operation. + +- [x] Task 6. **`crates/forge_domain/src/commit_config.rs` — `CommitConfig`** + - Change `provider: Option` → `provider: ProviderId` + - Change `model: Option` → `model: ModelId` + - Remove `Default` from derives + - Remove `#[serde(default, skip_serializing_if = "Option::is_none")]` from both fields (no longer optional) + - Remove `#[merge(strategy = crate::merge::option)]` from both fields and consider removing the `Merge` derive entirely since the Option-specific merge strategy no longer applies + - Change `#[setters(strip_option, into)]` → `#[setters(into)]` + - Add `pub fn new(provider: impl Into, model: impl Into) -> Self` constructor + +--- + +### Phase 3 — Infrastructure Layer (`forge_infra`) + +- [x] Task 7. **`crates/forge_infra/src/env.rs` — `apply_config_op` function** + - Remove the `ConfigOperation::SetProvider` arm from the match (the variant is gone) + - Simplify the `ConfigOperation::SetModel` arm: always create a fresh `ModelConfig::new(pid_str, mid_str)` — eliminate the conditional branch that checked whether the existing provider matched. The always-overwrite semantics are simpler and correct. + - Simplify the `ConfigOperation::SetCommitConfig` arm: `CommitConfig` now carries required fields, so replace the `zip()` pattern with a direct `Some(ModelConfig::new(commit.provider.as_ref(), commit.model.as_str()))`. + - `ConfigOperation::SetSuggestConfig` arm needs no logic change (it already creates a complete `ModelConfig`), but update the field access to use non-optional syntax. + +- [x] Task 8. **`crates/forge_infra/src/env.rs` — tests** + - Remove the `test_apply_config_op_set_provider` test (the operation no longer exists); replace with a test that verifies `SetModel` alone creates a complete session pair. + - Update `test_apply_config_op_set_model_matching_provider`: The fixture currently sets `session = Some(ModelConfig { provider_id: Some("anthropic"), model_id: None })` — this state is no longer representable. Change the fixture to use a complete starting pair (e.g., `ModelConfig::new("anthropic", "old-model")`) and verify the model is replaced. + - Update `test_apply_config_op_set_model_different_provider_replaces_session` to use `ModelConfig::new(...)` construction. + +--- + +### Phase 4 — Service Layer (`forge_services`) + +- [x] Task 9. **`crates/forge_services/src/app_config.rs` — `get_default_provider`** + - Simplify: `session.provider_id` is now a `String`. Remove the double `and_then`/`as_ref` chain; use a single `.map(|s| ProviderId::from(s.provider_id.clone()))`. + +- [x] Task 10. **`crates/forge_services/src/app_config.rs` — `get_provider_model`** + - Simplify: `session.provider_id` and `session.model_id` are no longer `Option`. Remove all inner `.as_ref()` / `.map(...)` option-unwrapping on these fields. The provider comparison becomes a direct string equality check. + +- [x] Task 11. **`crates/forge_services/src/app_config.rs` — remove `set_default_provider`** + - Remove the entire `set_default_provider` method implementation. With required fields there is no valid write operation that sets only the provider without a model. All callers must use `set_default_provider_and_model` instead. + +- [x] Task 12. **`crates/forge_services/src/app_config.rs` — `set_default_model`** + - Simplify: Reading `session.provider_id` no longer requires an `and_then` chain — it is a plain `String`. Update the `provider_id` extraction and the inline cache update (`session.model_id = model.to_string()`). + +- [x] Task 13. **`crates/forge_services/src/app_config.rs` — `get_commit_config`** + - Simplify: `CommitConfig.provider` and `CommitConfig.model` are now required fields — remove the `.map(ProviderId::from)` and `.map(ModelId::new)` Option wrappers. Direct field construction is now `CommitConfig { provider: mc.provider_id.into(), model: ModelId::new(mc.model_id) }`. + +- [x] Task 14. **`crates/forge_services/src/app_config.rs` — `get_suggest_config`** + - Simplify: Replace the `zip()` trick with a direct construction. Since `ModelConfig` always has both fields present, reading `mc.provider_id` and `mc.model_id` is direct, and the `SuggestConfig` is always constructed (no more `and_then`). + +- [x] Task 15. **`crates/forge_services/src/app_config.rs` — mock `update_environment` in tests** + - Remove the `ConfigOperation::SetProvider` arm from the mock match + - Simplify `ConfigOperation::SetModel` arm: always `config.session = Some(ModelConfig::new(pid_str, mid_str))` + - Simplify `ConfigOperation::SetCommitConfig` arm: `CommitConfig` now has required fields, replace `zip()` with direct field access + +- [x] Task 16. **`crates/forge_services/src/app_config.rs` — tests** + - Update all test methods that call `set_default_provider()` to instead call `set_default_provider_and_model(provider_id, model_id)` with a valid model for that provider + - Remove the `test_set_default_provider` test or replace it with a test for `set_default_provider_and_model` + - Update `test_get_default_provider_when_configured_provider_not_available` to use `set_default_provider_and_model` + +--- + +### Phase 5 — Application Layer (`forge_app`) + +- [x] Task 17. **`crates/forge_app/src/services.rs` — `AppConfigService` trait** + - Remove `set_default_provider()` from the trait declaration + +- [x] Task 18. **`crates/forge_app/src/services.rs` — delegating `impl AppConfigService for I`** + - Remove the `set_default_provider()` delegation method from this blanket impl + +- [x] Task 19. **`crates/forge_app/src/command_generator.rs` — `MockServices` impl** + - Remove `set_default_provider()` from the `AppConfigService` impl for `MockServices` + +--- + +### Phase 6 — API Layer (`forge_api`) + +- [x] Task 20. **`crates/forge_api/src/api.rs` — `API` trait** + - Remove `set_default_provider()` from the trait declaration + +- [x] Task 21. **`crates/forge_api/src/forge_api.rs` — `ForgeAPI` impl** + - Remove the `set_default_provider()` method implementation from `ForgeAPI` + +--- + +### Phase 7 — CLI / UI Layer (`forge_main`) + +- [x] Task 22. **`crates/forge_main/src/ui.rs` — `activate_provider_with_model` (line ~2886)** + - In the `else` branch (model is already compatible with the new provider), replace the call to `self.api.set_default_provider(provider.id.clone())` with `self.api.set_default_provider_and_model(provider.id.clone(), current_model_id)`. Restructure the surrounding code so that `current_model` (the `Option` captured on line ~2860) is accessible in this else branch — either by changing the match to not consume it, or by cloning it before the match. + +- [x] Task 23. **`crates/forge_main/src/ui.rs` — `handle_config_get` (line ~3608–3624)** + - In the `ConfigGetField::Commit` arm, simplify the `CommitConfig` field access: `provider` and `model` are no longer `Option`, so remove the `.map(...).unwrap_or_else(|| "Not set".to_string())` wrappers and use direct `.as_ref().to_string()` / `.as_str().to_string()`. + +- [x] Task 24. **`crates/forge_main/src/ui.rs` — `handle_config_set` (line ~3547–3549)** + - In the `ConfigSetField::Commit` arm, replace `forge_domain::CommitConfig::default().provider(...).model(...)` with the new `CommitConfig::new(provider, validated_model)` constructor (since `Default` is removed). + +--- + +### Phase 8 — Snapshot Regeneration and Verification + +- [x] Task 25. **Regenerate test snapshots** + - Run `cargo insta test --accept` to regenerate any snapshot tests that capture `ModelConfig`, `CommitConfig`, or related output that changed due to the field type change. + +- [x] Task 26. **Verify compilation and tests** + - Run `cargo check` across the workspace to catch any remaining sites that still use `Option` accessors on these fields. + - Run `cargo insta test --accept` to verify all tests pass with the updated type contracts. + +--- + +## Verification Criteria + +- `ModelConfig.provider_id` and `ModelConfig.model_id` are `String`, not `Option` — the compiler enforces their presence at every construction site +- `SessionConfig.provider_id` and `SessionConfig.model_id` are `String`, not `Option` +- `CommitConfig.provider` and `CommitConfig.model` are `ProviderId` and `ModelId`, not wrapped in `Option` +- `ConfigOperation::SetProvider` no longer exists — all session mutations go through `SetModel(ProviderId, ModelId)` +- `set_default_provider()` is removed from `AppConfigService` trait, `API` trait, and all implementations +- The legacy config reader (`legacy.rs`) only creates a `ModelConfig` for session when both provider and the model for that provider are present in the legacy JSON +- `apply_config_op` in infra is simplified to always produce a complete `ModelConfig` from `SetModel`, with no conditional branch +- All tests pass: no test relies on partial config state (provider set, model absent) +- `get_commit_config()` still returns `Option` (outer `Option` remains — commit config may not be configured), but when `Some`, both `provider` and `model` are guaranteed + +--- + +## Potential Risks and Mitigations + +1. **Legacy config files with provider but no model** + Mitigation: The `legacy.rs` change (Task 2) handles this silently by producing `session: None` instead of a partial `ModelConfig`. Users who had a provider set in the old `.config.json` but no corresponding model entry will find no default session — they will be prompted to configure provider+model together on first run. + +2. **Test suite breadth — many tests build `ForgeConfig` with partial `ModelConfig`** + Mitigation: The `Default` removal causes compile errors at every such site, making all affected tests immediately visible. Tasks 8 and 16 address the known test files; Task 25 catches snapshot regressions. + +3. **`set_default_model()` still requires an existing session** + Mitigation: `set_default_model()` is preserved but still requires a provider-set session (it reads `session.provider_id`). If called with no session, it returns `Error::NoDefaultProvider` — same behavior as before. This is correct and unchanged. + +4. **`activate_provider_with_model` code restructure (Task 22) introduces subtle scope issue** + Mitigation: The `current_model` variable must be made available in the else branch. The implementation can either `.clone()` the variable before the match or refactor the `needs_model_selection` computation to a helper that returns both the bool and the current model without consuming it. + +5. **`CommitConfig` field access in `handle_config_get`** + Mitigation: Since `get_commit_config()` still returns `Option`, the outer match on `Some(config)` is preserved. Only the inner field access changes from `.map(...).unwrap_or_else(...)` to direct access — a mechanical simplification. + +--- + +## Alternative Approaches + +1. **Keep `ModelConfig` with `Option` fields but add a validated wrapper type** — Introduce a `ValidatedModelConfig { provider_id: String, model_id: String }` and use it at the service/API boundary while keeping `ModelConfig` as a deserialization target. This avoids touching the legacy config path but adds an extra type conversion layer and duplicates the type hierarchy. + +2. **Keep `set_default_provider()` as a transitional method that errors** — Rather than removing `set_default_provider()`, change its body to always return `Err("Use set_default_provider_and_model instead")`. This preserves the API shape at the cost of runtime errors instead of compile errors. Not preferred since compile-time guarantees are stronger. + +3. **Make `CommitConfig` fields remain optional** — Only change `ModelConfig` and `SessionConfig`, leaving `CommitConfig.provider` and `CommitConfig.model` as `Option`. This avoids the simplification in `apply_config_op` for commit configs but is inconsistent with the stated goal of "APIs and other types are adjusted". The current `zip()` pattern in the infra already de-facto requires both, so making them required is the cleaner expression of the same invariant.