diff --git a/crates/extension_host/src/anthropic_migration.rs b/crates/extension_host/src/anthropic_migration.rs index 76734bd48ce1811ad98af46aff808a21a3029935..8d45344bfe66fee8eb81bd4bae66f38bbe44a9cf 100644 --- a/crates/extension_host/src/anthropic_migration.rs +++ b/crates/extension_host/src/anthropic_migration.rs @@ -132,7 +132,14 @@ mod tests { } #[gpui::test] - async fn test_skips_migration_if_no_old_credentials(cx: &mut TestAppContext) { + async fn test_skips_migration_if_empty_marker_exists(cx: &mut TestAppContext) { + let old_api_key = "sk-ant-old-key"; + + // Old credentials exist + cx.write_credentials(ANTHROPIC_DEFAULT_API_URL, "Bearer", old_api_key.as_bytes()); + // But empty marker already exists (from previous migration attempt) + cx.write_credentials("extension-llm-anthropic:anthropic", "Bearer", b""); + cx.update(|cx| { migrate_anthropic_credentials_if_needed(ANTHROPIC_EXTENSION_ID, cx); }); @@ -140,12 +147,31 @@ mod tests { cx.run_until_parked(); let credentials = cx.read_credentials("extension-llm-anthropic:anthropic"); + let (_, password) = credentials.unwrap(); assert!( - credentials.is_none(), - "Should not create credentials if none existed" + password.is_empty(), + "Should not overwrite empty marker with old credentials" ); } + #[gpui::test] + async fn test_writes_empty_marker_if_no_old_credentials(cx: &mut TestAppContext) { + cx.update(|cx| { + migrate_anthropic_credentials_if_needed(ANTHROPIC_EXTENSION_ID, cx); + }); + + cx.run_until_parked(); + + let credentials = cx.read_credentials("extension-llm-anthropic:anthropic"); + assert!( + credentials.is_some(), + "Should write empty credentials as migration marker" + ); + let (username, password) = credentials.unwrap(); + assert_eq!(username, "Bearer"); + assert!(password.is_empty(), "Password should be empty marker"); + } + #[gpui::test] async fn test_skips_migration_for_other_extensions(cx: &mut TestAppContext) { let api_key = "sk-ant-test-key"; diff --git a/crates/extension_host/src/copilot_migration.rs b/crates/extension_host/src/copilot_migration.rs index 8d0e84a6096bbe589b985e875bf8e246829ce946..8b005d83f29eac6c733b3e4ebdb94a4dff534c4d 100644 --- a/crates/extension_host/src/copilot_migration.rs +++ b/crates/extension_host/src/copilot_migration.rs @@ -234,7 +234,7 @@ mod tests { } #[gpui::test] - async fn test_no_migration_when_no_copilot_config_exists(cx: &mut TestAppContext) { + async fn test_writes_empty_marker_when_no_copilot_config_exists(cx: &mut TestAppContext) { cx.update(|cx| { migrate_copilot_credentials_if_needed(COPILOT_CHAT_EXTENSION_ID, cx); }); @@ -243,8 +243,11 @@ mod tests { let credentials = cx.read_credentials("extension-llm-copilot-chat:copilot-chat"); assert!( - credentials.is_none(), - "Should not create credentials when no copilot config exists" + credentials.is_some(), + "Should write empty credentials as migration marker" ); + let (username, password) = credentials.unwrap(); + assert_eq!(username, "api_key"); + assert!(password.is_empty(), "Password should be empty marker"); } } diff --git a/crates/extension_host/src/extension_host.rs b/crates/extension_host/src/extension_host.rs index c99a75bccf414c3ba893d68edf1e1e4d8c14ae6e..ddc2754341f2c69a1b2927f562297c34eb04f0c4 100644 --- a/crates/extension_host/src/extension_host.rs +++ b/crates/extension_host/src/extension_host.rs @@ -17,6 +17,7 @@ use async_tar::Archive; use client::ExtensionProvides; use client::{Client, ExtensionMetadata, GetExtensionsResponse, proto, telemetry::Telemetry}; use collections::{BTreeMap, BTreeSet, HashSet, btree_map}; + pub use extension::ExtensionManifest; use extension::extension_builder::{CompileExtensionOptions, ExtensionBuilder}; use extension::{ @@ -98,8 +99,9 @@ const LEGACY_LLM_EXTENSION_IDS: &[&str] = &[ /// Migrates legacy LLM provider extensions by auto-enabling env var reading /// if the env var is currently present in the environment. /// -/// This migration only runs once per provider - we track which providers have been -/// migrated in `migrated_llm_providers` to avoid overriding user preferences. +/// This is idempotent: if the provider is already in `allowed_env_var_providers`, +/// we skip. This means if a user explicitly removes it, it will be re-added on +/// next launch if the env var is still set - but that's predictable behavior. fn migrate_legacy_llm_provider_env_var(manifest: &ExtensionManifest, cx: &mut App) { // Only apply migration to known legacy LLM extensions if !LEGACY_LLM_EXTENSION_IDS.contains(&manifest.id.as_ref()) { @@ -117,51 +119,39 @@ fn migrate_legacy_llm_provider_env_var(manifest: &ExtensionManifest, cx: &mut Ap let full_provider_id: Arc = format!("{}:{}", manifest.id, provider_id).into(); - // Check if we've already run migration for this provider (regardless of outcome) - let already_migrated = ExtensionSettings::get_global(cx) - .migrated_llm_providers - .contains(full_provider_id.as_ref()); - - if already_migrated { - continue; - } - // Check if the env var is present and non-empty let env_var_is_set = std::env::var(env_var_name) .map(|v| !v.is_empty()) .unwrap_or(false); - // Mark as migrated regardless of whether we enable env var reading - let should_enable_env_var = env_var_is_set; + // If env var isn't set, no need to do anything + if !env_var_is_set { + continue; + } + + // Check if already enabled in settings + let already_enabled = ExtensionSettings::get_global(cx) + .allowed_env_var_providers + .contains(full_provider_id.as_ref()); + + if already_enabled { + continue; + } + + // Enable env var reading since the env var is set settings::update_settings_file(::global(cx), cx, { let full_provider_id = full_provider_id.clone(); move |settings, _| { - // Always mark as migrated - let migrated = settings + let providers = settings .extension - .migrated_llm_providers + .allowed_env_var_providers .get_or_insert_with(Vec::new); - if !migrated + if !providers .iter() .any(|id| id.as_ref() == full_provider_id.as_ref()) { - migrated.push(full_provider_id.clone()); - } - - // Only enable env var reading if the env var is set - if should_enable_env_var { - let providers = settings - .extension - .allowed_env_var_providers - .get_or_insert_with(Vec::new); - - if !providers - .iter() - .any(|id| id.as_ref() == full_provider_id.as_ref()) - { - providers.push(full_provider_id); - } + providers.push(full_provider_id); } } }); diff --git a/crates/extension_host/src/extension_settings.rs b/crates/extension_host/src/extension_settings.rs index 36777cb1727c95082b6a6e403ece1acb9e490729..3322ea4068cc08ef8f5257e670ad8cb7088b57b7 100644 --- a/crates/extension_host/src/extension_settings.rs +++ b/crates/extension_host/src/extension_settings.rs @@ -20,9 +20,6 @@ pub struct ExtensionSettings { /// from environment variables. Each entry is a provider ID in the format /// "extension_id:provider_id". pub allowed_env_var_providers: HashSet>, - /// Tracks which legacy LLM providers have been migrated. - /// This prevents the migration from running multiple times and overriding user preferences. - pub migrated_llm_providers: HashSet>, } impl ExtensionSettings { @@ -74,13 +71,6 @@ impl Settings for ExtensionSettings { .unwrap_or_default() .into_iter() .collect(), - migrated_llm_providers: content - .extension - .migrated_llm_providers - .clone() - .unwrap_or_default() - .into_iter() - .collect(), } } } diff --git a/crates/extension_host/src/google_ai_migration.rs b/crates/extension_host/src/google_ai_migration.rs index 0addf8f26bf3bc24119ec01777bc12c52b4083fe..dbb6289b46e7dca0c3043ef92ec1c02e8985740b 100644 --- a/crates/extension_host/src/google_ai_migration.rs +++ b/crates/extension_host/src/google_ai_migration.rs @@ -132,7 +132,7 @@ mod tests { } #[gpui::test] - async fn test_skips_migration_if_no_old_credentials(cx: &mut TestAppContext) { + async fn test_writes_empty_marker_if_no_old_credentials(cx: &mut TestAppContext) { cx.update(|cx| { migrate_google_ai_credentials_if_needed(GOOGLE_AI_EXTENSION_ID, cx); }); @@ -141,9 +141,12 @@ mod tests { let credentials = cx.read_credentials("extension-llm-google-ai:google-ai"); assert!( - credentials.is_none(), - "Should not create credentials if none existed" + credentials.is_some(), + "Should write empty credentials as migration marker" ); + let (username, password) = credentials.unwrap(); + assert_eq!(username, "Bearer"); + assert!(password.is_empty(), "Password should be empty marker"); } #[gpui::test] diff --git a/crates/extension_host/src/open_router_migration.rs b/crates/extension_host/src/open_router_migration.rs index 67788b1142e8a84b3b3207f8c5b7573b26de114a..07dde2ec7b50782404cdda38d5886a7210ed92b2 100644 --- a/crates/extension_host/src/open_router_migration.rs +++ b/crates/extension_host/src/open_router_migration.rs @@ -136,7 +136,7 @@ mod tests { } #[gpui::test] - async fn test_skips_migration_if_no_old_credentials(cx: &mut TestAppContext) { + async fn test_writes_empty_marker_if_no_old_credentials(cx: &mut TestAppContext) { cx.update(|cx| { migrate_open_router_credentials_if_needed(OPEN_ROUTER_EXTENSION_ID, cx); }); @@ -145,9 +145,12 @@ mod tests { let credentials = cx.read_credentials("extension-llm-openrouter:openrouter"); assert!( - credentials.is_none(), - "Should not create credentials if none existed" + credentials.is_some(), + "Should write empty credentials as migration marker" ); + let (username, password) = credentials.unwrap(); + assert_eq!(username, "Bearer"); + assert!(password.is_empty(), "Password should be empty marker"); } #[gpui::test] diff --git a/crates/extension_host/src/openai_migration.rs b/crates/extension_host/src/openai_migration.rs index d226e33920d654c261fa60abc4d9f6e4133091d9..f20f81a39b2591a577e7a6d5bfd934df7ed6ddef 100644 --- a/crates/extension_host/src/openai_migration.rs +++ b/crates/extension_host/src/openai_migration.rs @@ -132,7 +132,7 @@ mod tests { } #[gpui::test] - async fn test_skips_migration_if_no_old_credentials(cx: &mut TestAppContext) { + async fn test_writes_empty_marker_if_no_old_credentials(cx: &mut TestAppContext) { cx.update(|cx| { migrate_openai_credentials_if_needed(OPENAI_EXTENSION_ID, cx); }); @@ -141,9 +141,12 @@ mod tests { let credentials = cx.read_credentials("extension-llm-openai:openai"); assert!( - credentials.is_none(), - "Should not create credentials if none existed" + credentials.is_some(), + "Should write empty credentials as migration marker" ); + let (username, password) = credentials.unwrap(); + assert_eq!(username, "Bearer"); + assert!(password.is_empty(), "Password should be empty marker"); } #[gpui::test] diff --git a/crates/settings/src/settings_content/extension.rs b/crates/settings/src/settings_content/extension.rs index b405103e8c311dbd5c83a100002bae7689d11591..64df163f4ec961cf6bfc469c18ac0f8884c39f0b 100644 --- a/crates/settings/src/settings_content/extension.rs +++ b/crates/settings/src/settings_content/extension.rs @@ -26,9 +26,6 @@ pub struct ExtensionSettingsContent { /// /// Default: [] pub allowed_env_var_providers: Option>>, - /// Tracks which legacy LLM providers have been migrated. This is an internal - /// setting used to prevent the migration from running multiple times. - pub migrated_llm_providers: Option>>, } /// A capability for an extension.