diff --git a/crates/forge_api/src/api.rs b/crates/forge_api/src/api.rs index e2a9e7773f..2195cbcd44 100644 --- a/crates/forge_api/src/api.rs +++ b/crates/forge_api/src/api.rs @@ -236,11 +236,6 @@ pub trait API: Sync + Send { /// Initialize a new empty workspace async fn init_workspace(&self, path: PathBuf) -> Result; - /// Migrate environment variable-based credentials to file-based - /// credentials. This is a one-time migration that runs only if the - /// credentials file doesn't exist. - async fn migrate_env_credentials(&self) -> Result>; - async fn generate_data( &self, data_parameters: DataGenerationParameters, diff --git a/crates/forge_api/src/forge_api.rs b/crates/forge_api/src/forge_api.rs index ac0b9edf98..c13e8cca73 100644 --- a/crates/forge_api/src/forge_api.rs +++ b/crates/forge_api/src/forge_api.rs @@ -384,10 +384,6 @@ impl Result> { - Ok(self.services.migrate_env_credentials().await?) - } - async fn generate_data( &self, data_parameters: DataGenerationParameters, diff --git a/crates/forge_app/src/command_generator.rs b/crates/forge_app/src/command_generator.rs index abf4b91c88..d9dca61a94 100644 --- a/crates/forge_app/src/command_generator.rs +++ b/crates/forge_app/src/command_generator.rs @@ -246,10 +246,6 @@ mod tests { async fn remove_credential(&self, _id: &ProviderId) -> Result<()> { Ok(()) } - - async fn migrate_env_credentials(&self) -> anyhow::Result> { - Ok(None) - } } #[async_trait::async_trait] diff --git a/crates/forge_app/src/services.rs b/crates/forge_app/src/services.rs index c11b4fafe4..0bb4e80108 100644 --- a/crates/forge_app/src/services.rs +++ b/crates/forge_app/src/services.rs @@ -171,12 +171,6 @@ pub trait ProviderService: Send + Sync { credential: forge_domain::AuthCredential, ) -> anyhow::Result<()>; async fn remove_credential(&self, id: &forge_domain::ProviderId) -> anyhow::Result<()>; - /// Migrates environment variable-based credentials to file-based - /// credentials. Returns Some(MigrationResult) if credentials were migrated, - /// None if file already exists or no credentials to migrate. - async fn migrate_env_credentials( - &self, - ) -> anyhow::Result>; } /// Manages user preferences for default providers and models. @@ -672,12 +666,6 @@ impl ProviderService for I { async fn remove_credential(&self, id: &forge_domain::ProviderId) -> anyhow::Result<()> { self.provider_service().remove_credential(id).await } - - async fn migrate_env_credentials( - &self, - ) -> anyhow::Result> { - self.provider_service().migrate_env_credentials().await - } } #[async_trait::async_trait] diff --git a/crates/forge_domain/src/lib.rs b/crates/forge_domain/src/lib.rs index 13a6b18135..5cd3469518 100644 --- a/crates/forge_domain/src/lib.rs +++ b/crates/forge_domain/src/lib.rs @@ -26,7 +26,6 @@ mod mcp_servers; mod merge; mod message; mod message_pattern; -mod migration; mod model; mod node; mod point; @@ -82,7 +81,6 @@ pub use mcp::*; pub use mcp_servers::*; pub use message::*; pub use message_pattern::*; -pub use migration::*; pub use model::*; pub use node::*; pub use point::*; diff --git a/crates/forge_domain/src/migration.rs b/crates/forge_domain/src/migration.rs deleted file mode 100644 index 73e4c24911..0000000000 --- a/crates/forge_domain/src/migration.rs +++ /dev/null @@ -1,41 +0,0 @@ -use std::path::PathBuf; - -use crate::ProviderId; - -/// Result of credential migration from environment variables to file. -/// Only returned when credentials were actually migrated (Some). -/// None indicates file already exists or no credentials to migrate. -#[derive(Debug, Clone)] -pub struct MigrationResult { - /// Path to the credentials file - pub credentials_path: PathBuf, - /// Providers that were migrated - pub migrated_providers: Vec, -} - -impl MigrationResult { - /// Creates a result indicating successful migration - pub fn new(credentials_path: PathBuf, migrated_providers: Vec) -> Self { - Self { credentials_path, migrated_providers } - } -} - -#[cfg(test)] -mod tests { - use std::path::PathBuf; - - use pretty_assertions::assert_eq; - - use super::*; - - #[test] - fn test_migration_result() { - let path = PathBuf::from("/test/.credentials.json"); - let providers = vec![ProviderId::OPENAI, ProviderId::ANTHROPIC]; - - let actual = MigrationResult::new(path.clone(), providers.clone()); - - assert_eq!(actual.credentials_path, path); - assert_eq!(actual.migrated_providers, providers); - } -} diff --git a/crates/forge_domain/src/repo.rs b/crates/forge_domain/src/repo.rs index 4d6205f575..bbeafdc07b 100644 --- a/crates/forge_domain/src/repo.rs +++ b/crates/forge_domain/src/repo.rs @@ -5,8 +5,8 @@ use url::Url; use crate::{ AnyProvider, AuthCredential, ChatCompletionMessage, Context, Conversation, ConversationId, - MigrationResult, Model, ModelId, Provider, ProviderId, ProviderTemplate, ResultStream, - SearchMatch, Skill, Snapshot, WorkspaceAuth, WorkspaceId, + Model, ModelId, Provider, ProviderId, ProviderTemplate, ResultStream, SearchMatch, Skill, + Snapshot, WorkspaceAuth, WorkspaceId, }; /// Repository for managing file snapshots @@ -107,7 +107,6 @@ pub trait ProviderRepository: Send + Sync { async fn upsert_credential(&self, credential: AuthCredential) -> anyhow::Result<()>; async fn get_credential(&self, id: &ProviderId) -> anyhow::Result>; async fn remove_credential(&self, id: &ProviderId) -> anyhow::Result<()>; - async fn migrate_env_credentials(&self) -> anyhow::Result>; } /// Repository for managing workspace indexing and search operations diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index df739908fe..4fcffe673f 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -2910,8 +2910,6 @@ impl A + Send + Sync> UI { /// Initialize the state of the UI async fn init_state(&mut self, first: bool) -> Result<()> { - let _ = self.handle_migrate_credentials().await; - // Ensure we have a model selected before proceeding with initialization let active_agent = self.api.get_active_agent().await; @@ -4079,31 +4077,6 @@ impl A + Send + Sync> UI { Ok(()) } - /// Handle credential migration - async fn handle_migrate_credentials(&mut self) -> Result<()> { - // Perform the migration - self.spinner.start(Some("Migrating credentials"))?; - let result = self.api.migrate_env_credentials().await?; - self.spinner.stop(None)?; - - // Display results based on whether migration occurred - if let Some(result) = result { - self.writeln_title( - TitleFormat::warning("Forge no longer reads API keys from environment variables.") - .sub_title("Learn more: https://forgecode.dev/docs/custom-providers/"), - )?; - - let count = result.migrated_providers.len(); - let message = if count == 1 { - "Migrated 1 provider from environment variables".to_string() - } else { - format!("Migrated {count} providers from environment variables") - }; - self.writeln_title(TitleFormat::info(message))?; - } - Ok(()) - } - /// Silently install VS Code extension if in VS Code and extension not /// installed. /// NOTE: This is a non-cancellable and a slow task. We should only run this diff --git a/crates/forge_repo/src/forge_repo.rs b/crates/forge_repo/src/forge_repo.rs index ce203f6c4b..36a4aa853e 100644 --- a/crates/forge_repo/src/forge_repo.rs +++ b/crates/forge_repo/src/forge_repo.rs @@ -11,7 +11,7 @@ use forge_app::{ use forge_domain::{ AnyProvider, AuthCredential, ChatCompletionMessage, ChatRepository, CommandOutput, Context, Conversation, ConversationId, ConversationRepository, Environment, FileInfo, - FuzzySearchRepository, McpServerConfig, MigrationResult, Model, ModelId, Provider, ProviderId, + FuzzySearchRepository, McpServerConfig, Model, ModelId, Provider, ProviderId, ProviderRepository, ResultStream, SearchMatch, Skill, SkillRepository, Snapshot, SnapshotRepository, }; @@ -182,10 +182,6 @@ impl anyhow::Result> { - self.provider_repository.migrate_env_to_file().await - } } #[async_trait::async_trait] diff --git a/crates/forge_repo/src/provider/provider_repo.rs b/crates/forge_repo/src/provider/provider_repo.rs index 8af74b5b07..8fc08e349a 100644 --- a/crates/forge_repo/src/provider/provider_repo.rs +++ b/crates/forge_repo/src/provider/provider_repo.rs @@ -4,8 +4,8 @@ use bytes::Bytes; use forge_app::domain::{ProviderId, ProviderResponse}; use forge_app::{EnvironmentInfra, FileReaderInfra, FileWriterInfra, HttpInfra}; use forge_domain::{ - AnyProvider, ApiKey, AuthCredential, AuthDetails, Error, MigrationResult, Provider, - ProviderRepository, ProviderType, URLParam, URLParamSpec, URLParamValue, + AnyProvider, AuthCredential, Error, Provider, ProviderRepository, ProviderType, URLParam, + URLParamSpec, }; use merge::Merge; use serde::Deserialize; @@ -32,14 +32,6 @@ enum UrlParamVarConfig { } impl UrlParamVarConfig { - /// Returns the parameter name (used as env var name and template variable). - fn param_name(&self) -> &str { - match self { - Self::Plain(s) => s, - Self::WithOptions { name, .. } => name, - } - } - /// Converts into a `URLParamSpec` for use in the domain layer. fn into_spec(self) -> URLParamSpec { match self { @@ -196,99 +188,6 @@ impl providers } - /// Migrates environment variable-based credentials to file-based - /// credentials. This is a one-time migration that runs only if the - /// credentials file doesn't exist. - pub async fn migrate_env_to_file(&self) -> anyhow::Result> { - let path = self.infra.get_environment().credentials_path(); - - // Check if credentials file already exists - if self.infra.read_utf8(&path).await.is_ok() { - return Ok(None); - } - - let mut credentials = Vec::new(); - let mut migrated_providers = Vec::new(); - let configs = self.get_merged_configs().await; - - let has_openai_url = self.infra.get_env_var("OPENAI_URL").is_some(); - let has_anthropic_url = self.infra.get_env_var("ANTHROPIC_URL").is_some(); - - for config in configs { - // Skip Forge provider and ContextEngine providers - they're not configurable - // via env like other providers - if config.id == ProviderId::FORGE || config.provider_type == ProviderType::ContextEngine - { - continue; - } - - if config.id == ProviderId::OPENAI && has_openai_url { - continue; - } - if (config.id == ProviderId::OPENAI_COMPATIBLE - || config.id == ProviderId::OPENAI_RESPONSES_COMPATIBLE) - && !has_openai_url - { - continue; - } - if config.id == ProviderId::ANTHROPIC && has_anthropic_url { - continue; - } - if config.id == ProviderId::ANTHROPIC_COMPATIBLE && !has_anthropic_url { - continue; - } - - // Try to create credential from environment variables - if let Ok(credential) = self.create_credential_from_env(&config) { - migrated_providers.push(config.id); - credentials.push(credential); - } - } - - // Only write if we have credentials to migrate - if !credentials.is_empty() { - self.write_credentials(&credentials).await?; - Ok(Some(MigrationResult::new(path, migrated_providers))) - } else { - Ok(None) - } - } - - /// Creates a credential from environment variables for a given config - fn create_credential_from_env( - &self, - config: &ProviderConfig, - ) -> anyhow::Result { - // Check API key environment variable (if specified) - let api_key = if let Some(api_key_var) = &config.api_key_vars { - self.infra - .get_env_var(api_key_var) - .ok_or_else(|| Error::env_var_not_found(config.id.clone(), api_key_var))? - } else { - // For context engine, we don't use env vars for API key - String::new() - }; - - // Check URL parameter environment variables - let mut url_params = std::collections::HashMap::new(); - - for env_var in &config.url_param_vars { - let name = env_var.param_name(); - if let Some(value) = self.infra.get_env_var(name) { - url_params.insert(URLParam::from(name.to_string()), URLParamValue::from(value)); - } else { - return Err(Error::env_var_not_found(config.id.clone(), name).into()); - } - } - - // Create AuthCredential - Ok(AuthCredential { - id: config.id.clone(), - auth_details: AuthDetails::ApiKey(ApiKey::from(api_key)), - url_params, - }) - } - /// Creates a provider with template URLs (not rendered). /// The service layer is responsible for rendering templates. async fn create_provider( @@ -486,9 +385,15 @@ impl Ok(()) } +} - async fn migrate_env_credentials(&self) -> anyhow::Result> { - self.migrate_env_to_file().await +#[cfg(test)] +impl UrlParamVarConfig { + fn param_name(&self) -> &str { + match self { + Self::Plain(s) => s, + Self::WithOptions { name, .. } => name, + } } } @@ -692,7 +597,9 @@ mod env_tests { use forge_app::domain::{ ChatCompletionMessage, Context, Environment, Model, ModelId, ResultStream, }; - use forge_domain::{AnyProvider, ChatRepository, ProviderTemplate}; + use forge_domain::{ + AnyProvider, ApiKey, AuthDetails, ChatRepository, ProviderTemplate, URLParamValue, + }; use pretty_assertions::assert_eq; use url::Url; @@ -886,188 +793,6 @@ mod env_tests { async fn remove_credential(&self, _id: &ProviderId) -> anyhow::Result<()> { Ok(()) } - - async fn migrate_env_credentials( - &self, - ) -> anyhow::Result> { - Ok(None) - } - } - - #[tokio::test] - async fn test_migration_from_env_to_file() { - let mut env_vars = HashMap::new(); - env_vars.insert("OPENAI_API_KEY".to_string(), "test-openai-key".to_string()); - env_vars.insert( - "ANTHROPIC_API_KEY".to_string(), - "test-anthropic-key".to_string(), - ); - env_vars.insert( - "OPENAI_URL".to_string(), - "https://custom.openai.com/v1".to_string(), - ); - - let infra = Arc::new(MockInfra::new(env_vars)); - let registry = ForgeProviderRepository::new(infra.clone()); - - // Trigger migration - registry.migrate_env_to_file().await.unwrap(); - - // Verify credentials were written - let credentials_guard = infra.credentials.lock().await; - let credentials = credentials_guard.as_ref().unwrap(); - - // Should have migrated OpenAICompatible (not OpenAI) and Anthropic (not - // AnthropicCompatible) - assert!( - !credentials.iter().any(|c| c.id == ProviderId::OPENAI), - "Should NOT create OpenAI credential when OPENAI_URL is set" - ); - assert!( - credentials - .iter() - .any(|c| c.id == ProviderId::OPENAI_COMPATIBLE), - "Should create OpenAICompatible credential when OPENAI_URL is set" - ); - assert!( - credentials.iter().any(|c| c.id == ProviderId::ANTHROPIC), - "Should create Anthropic credential when ANTHROPIC_URL is NOT set" - ); - assert!( - !credentials - .iter() - .any(|c| c.id == ProviderId::ANTHROPIC_COMPATIBLE), - "Should NOT create AnthropicCompatible credential when ANTHROPIC_URL is NOT set" - ); - - // Verify OpenAICompatible credential - let openai_compat_cred = credentials - .iter() - .find(|c| c.id == ProviderId::OPENAI_COMPATIBLE) - .unwrap(); - match &openai_compat_cred.auth_details { - AuthDetails::ApiKey(key) => assert_eq!(key.as_str(), "test-openai-key"), - _ => panic!("Expected API key"), - } - - // Verify OpenAICompatible has URL param - assert!(!openai_compat_cred.url_params.is_empty()); - let url_params = &openai_compat_cred.url_params; - assert_eq!( - url_params - .get(&URLParam::from("OPENAI_URL".to_string())) - .map(|v| v.as_str()), - Some("https://custom.openai.com/v1") - ); - - // Verify Anthropic credential - let anthropic_cred = credentials - .iter() - .find(|c| c.id == ProviderId::ANTHROPIC) - .unwrap(); - match &anthropic_cred.auth_details { - AuthDetails::ApiKey(key) => assert_eq!(key.as_str(), "test-anthropic-key"), - _ => panic!("Expected API key"), - } - } - - #[tokio::test] - async fn test_migration_should_not_create_forge_services_credential() { - let mut env_vars = HashMap::new(); - env_vars.insert("OPENAI_API_KEY".to_string(), "test-key".to_string()); - - let infra = Arc::new(MockInfra::new(env_vars)); - let registry = ForgeProviderRepository::new(infra.clone()); - - // Trigger migration - registry.migrate_env_to_file().await.unwrap(); - - // Verify credentials were written - let credentials_guard = infra.credentials.lock().await; - let credentials = credentials_guard.as_ref().unwrap(); - - // Verify forge_services was NOT created during migration - assert!( - !credentials - .iter() - .any(|c| c.id == ProviderId::FORGE_SERVICES), - "Should NOT create forge_services credential during environment migration" - ); - - // Verify only OpenAI credential was created - assert_eq!( - credentials.len(), - 1, - "Should only have one credential (OpenAI)" - ); - assert!( - credentials.iter().any(|c| c.id == ProviderId::OPENAI), - "Should have OpenAI credential" - ); - } - - #[tokio::test] - async fn test_migration_both_compatible_urls() { - let mut env_vars = HashMap::new(); - env_vars.insert("OPENAI_API_KEY".to_string(), "test-openai-key".to_string()); - env_vars.insert( - "ANTHROPIC_API_KEY".to_string(), - "test-anthropic-key".to_string(), - ); - env_vars.insert( - "OPENAI_URL".to_string(), - "https://custom.openai.com/v1".to_string(), - ); - env_vars.insert( - "ANTHROPIC_URL".to_string(), - "https://custom.anthropic.com/v1".to_string(), - ); - - let infra = Arc::new(MockInfra::new(env_vars)); - let registry = ForgeProviderRepository::new(infra.clone()); - - // Trigger migration - registry.migrate_env_to_file().await.unwrap(); - - // Verify credentials were written - let credentials_guard = infra.credentials.lock().await; - let credentials = credentials_guard.as_ref().unwrap(); - - // Should have migrated only compatible versions - assert!( - !credentials.iter().any(|c| c.id == ProviderId::OPENAI), - "Should NOT create OpenAI credential when OPENAI_URL is set" - ); - assert!( - credentials - .iter() - .any(|c| c.id == ProviderId::OPENAI_COMPATIBLE), - "Should create OpenAICompatible credential when OPENAI_URL is set" - ); - assert!( - !credentials.iter().any(|c| c.id == ProviderId::ANTHROPIC), - "Should NOT create Anthropic credential when ANTHROPIC_URL is set" - ); - assert!( - credentials - .iter() - .any(|c| c.id == ProviderId::ANTHROPIC_COMPATIBLE), - "Should create AnthropicCompatible credential when ANTHROPIC_URL is set" - ); - - // Verify AnthropicCompatible has URL param - let anthropic_compat_cred = credentials - .iter() - .find(|c| c.id == ProviderId::ANTHROPIC_COMPATIBLE) - .unwrap(); - assert!(!anthropic_compat_cred.url_params.is_empty()); - let url_params = &anthropic_compat_cred.url_params; - assert_eq!( - url_params - .get(&URLParam::from("ANTHROPIC_URL".to_string())) - .map(|v| v.as_str()), - Some("https://custom.anthropic.com/v1") - ); } #[tokio::test] @@ -1090,8 +815,30 @@ mod env_tests { let infra = Arc::new(MockInfra::new(env_vars)); let registry = ForgeProviderRepository::new(infra); - // Trigger migration to populate credentials file - registry.migrate_env_to_file().await.unwrap(); + // Set up Azure credentials directly + registry + .upsert_credential(AuthCredential { + id: ProviderId::AZURE, + auth_details: AuthDetails::ApiKey(ApiKey::from("test-key-123".to_string())), + url_params: [ + ( + URLParam::from("AZURE_RESOURCE_NAME".to_string()), + URLParamValue::from("my-test-resource".to_string()), + ), + ( + URLParam::from("AZURE_DEPLOYMENT_NAME".to_string()), + URLParamValue::from("gpt-4-deployment".to_string()), + ), + ( + URLParam::from("AZURE_API_VERSION".to_string()), + URLParamValue::from("2024-02-01-preview".to_string()), + ), + ] + .into_iter() + .collect(), + }) + .await + .unwrap(); // Get Azure config from embedded configs let configs = get_provider_configs(); @@ -1147,8 +894,23 @@ mod env_tests { let infra = Arc::new(MockInfra::new(env_vars)); let registry = ForgeProviderRepository::new(infra); - // Migrate environment variables to credentials file - registry.migrate_env_to_file().await.unwrap(); + // Set up credentials directly + registry + .upsert_credential(AuthCredential { + id: ProviderId::OPENAI, + auth_details: AuthDetails::ApiKey(ApiKey::from("test-key".to_string())), + url_params: std::collections::HashMap::new(), + }) + .await + .unwrap(); + registry + .upsert_credential(AuthCredential { + id: ProviderId::ANTHROPIC, + auth_details: AuthDetails::ApiKey(ApiKey::from("test-key".to_string())), + url_params: std::collections::HashMap::new(), + }) + .await + .unwrap(); let providers = registry.get_all_providers().await.unwrap(); @@ -1371,12 +1133,6 @@ mod env_tests { async fn remove_credential(&self, _id: &ProviderId) -> anyhow::Result<()> { Ok(()) } - - async fn migrate_env_credentials( - &self, - ) -> anyhow::Result> { - Ok(None) - } } let infra = Arc::new(CustomMockInfra { env_vars, base_path }); diff --git a/crates/forge_services/src/app_config.rs b/crates/forge_services/src/app_config.rs index fb1e438e4b..087cd7f6ee 100644 --- a/crates/forge_services/src/app_config.rs +++ b/crates/forge_services/src/app_config.rs @@ -138,8 +138,8 @@ mod tests { use forge_config::{ForgeConfig, ModelConfig}; use forge_domain::{ - AnyProvider, ChatRepository, ConfigOperation, Environment, InputModality, MigrationResult, - Model, ModelSource, Provider, ProviderId, ProviderResponse, ProviderTemplate, + AnyProvider, ChatRepository, ConfigOperation, Environment, InputModality, Model, + ModelSource, Provider, ProviderId, ProviderResponse, ProviderTemplate, }; use pretty_assertions::assert_eq; use url::Url; @@ -365,10 +365,6 @@ mod tests { async fn remove_credential(&self, _id: &ProviderId) -> anyhow::Result<()> { Ok(()) } - - async fn migrate_env_credentials(&self) -> anyhow::Result> { - Ok(None) - } } #[tokio::test] diff --git a/crates/forge_services/src/provider_service.rs b/crates/forge_services/src/provider_service.rs index 39748dd3d6..8c8b9010a1 100644 --- a/crates/forge_services/src/provider_service.rs +++ b/crates/forge_services/src/provider_service.rs @@ -7,8 +7,8 @@ use forge_app::domain::{ AnyProvider, ChatCompletionMessage, Model, ModelId, ProviderId, ResultStream, }; use forge_domain::{ - AuthCredential, ChatRepository, Context, MigrationResult, ModelSource, Provider, - ProviderRepository, ProviderTemplate, + AuthCredential, ChatRepository, Context, ModelSource, Provider, ProviderRepository, + ProviderTemplate, }; use url::Url; @@ -128,10 +128,6 @@ impl ProviderService for ForgeProviderSe async fn remove_credential(&self, id: &ProviderId) -> Result<()> { self.repository.remove_credential(id).await } - - async fn migrate_env_credentials(&self) -> Result> { - self.repository.migrate_env_credentials().await - } } #[cfg(test)] @@ -198,10 +194,6 @@ mod tests { async fn remove_credential(&self, _id: &ProviderId) -> Result<()> { Ok(()) } - - async fn migrate_env_credentials(&self) -> Result> { - Ok(None) - } } fn test_provider() -> Provider {