From ee4faede38bee54cc5463ef61a097e24def6a925 Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Thu, 11 Dec 2025 00:22:38 -0500 Subject: [PATCH] Migrate on auto-load --- .../extension_host/src/anthropic_migration.rs | 93 +++------------- .../extension_host/src/copilot_migration.rs | 61 ++--------- crates/extension_host/src/extension_host.rs | 75 ++++++++----- .../extension_host/src/google_ai_migration.rs | 93 +++------------- .../src/open_router_migration.rs | 101 +++--------------- crates/extension_host/src/openai_migration.rs | 93 +++------------- .../src/wasm_host/llm_provider.rs | 10 +- 7 files changed, 107 insertions(+), 419 deletions(-) diff --git a/crates/extension_host/src/anthropic_migration.rs b/crates/extension_host/src/anthropic_migration.rs index 8d45344bfe66fee8eb81bd4bae66f38bbe44a9cf..71d8cc07ac7c672eb84958b8a067c57233aae3e0 100644 --- a/crates/extension_host/src/anthropic_migration.rs +++ b/crates/extension_host/src/anthropic_migration.rs @@ -1,11 +1,14 @@ use credentials_provider::CredentialsProvider; use gpui::App; -use util::ResultExt as _; const ANTHROPIC_EXTENSION_ID: &str = "anthropic"; const ANTHROPIC_PROVIDER_ID: &str = "anthropic"; const ANTHROPIC_DEFAULT_API_URL: &str = "https://api.anthropic.com"; +/// Migrates Anthropic API credentials from the old built-in provider location +/// to the new extension-based location. +/// +/// This should only be called during auto-install of the extension. pub fn migrate_anthropic_credentials_if_needed(extension_id: &str, cx: &mut App) { if extension_id != ANTHROPIC_EXTENSION_ID { return; @@ -19,17 +22,7 @@ pub fn migrate_anthropic_credentials_if_needed(extension_id: &str, cx: &mut App) let credentials_provider = ::global(cx); cx.spawn(async move |cx| { - let existing_credential = credentials_provider - .read_credentials(&extension_credential_key, &cx) - .await - .ok() - .flatten(); - - if existing_credential.is_some() { - log::debug!("Anthropic extension already has credentials, skipping migration"); - return; - } - + // Read from old location let old_credential = credentials_provider .read_credentials(ANTHROPIC_DEFAULT_API_URL, &cx) .await @@ -40,8 +33,8 @@ pub fn migrate_anthropic_credentials_if_needed(extension_id: &str, cx: &mut App) Some((_, key_bytes)) => match String::from_utf8(key_bytes) { Ok(key) if !key.is_empty() => key, Ok(_) => { - log::debug!("Existing Anthropic API key is empty, marking as migrated"); - String::new() + log::debug!("Existing Anthropic API key is empty, nothing to migrate"); + return; } Err(_) => { log::error!("Failed to decode Anthropic API key as UTF-8"); @@ -49,20 +42,11 @@ pub fn migrate_anthropic_credentials_if_needed(extension_id: &str, cx: &mut App) } }, None => { - log::debug!("No existing Anthropic API key found, marking as migrated"); - String::new() + log::debug!("No existing Anthropic API key found to migrate"); + return; } }; - if api_key.is_empty() { - // Write empty credentials as a marker that migration was attempted - credentials_provider - .write_credentials(&extension_credential_key, "Bearer", b"", &cx) - .await - .log_err(); - return; - } - log::info!("Migrating existing Anthropic API key to Anthropic extension"); match credentials_provider @@ -105,41 +89,7 @@ mod tests { } #[gpui::test] - async fn test_skips_migration_if_extension_already_has_credentials(cx: &mut TestAppContext) { - let old_api_key = "sk-ant-old-key"; - let existing_key = "sk-ant-existing-key"; - - cx.write_credentials(ANTHROPIC_DEFAULT_API_URL, "Bearer", old_api_key.as_bytes()); - cx.write_credentials( - "extension-llm-anthropic:anthropic", - "Bearer", - existing_key.as_bytes(), - ); - - 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"); - let (_, password) = credentials.unwrap(); - assert_eq!( - String::from_utf8(password).unwrap(), - existing_key, - "Should not overwrite existing credentials" - ); - } - - #[gpui::test] - 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""); - + async fn test_no_migration_if_no_old_credentials(cx: &mut TestAppContext) { cx.update(|cx| { migrate_anthropic_credentials_if_needed(ANTHROPIC_EXTENSION_ID, cx); }); @@ -147,29 +97,10 @@ mod tests { cx.run_until_parked(); let credentials = cx.read_credentials("extension-llm-anthropic:anthropic"); - let (_, password) = credentials.unwrap(); assert!( - 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" + credentials.is_none(), + "Should not create credentials if none existed" ); - 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/copilot_migration.rs b/crates/extension_host/src/copilot_migration.rs index 93cb91429f1057ae9a0db724685bf95840390fdf..dd5ede4687fb16723960920f5d14e26e49e09954 100644 --- a/crates/extension_host/src/copilot_migration.rs +++ b/crates/extension_host/src/copilot_migration.rs @@ -1,11 +1,14 @@ use credentials_provider::CredentialsProvider; use gpui::App; use std::path::PathBuf; -use util::ResultExt as _; const COPILOT_CHAT_EXTENSION_ID: &str = "copilot-chat"; const COPILOT_CHAT_PROVIDER_ID: &str = "copilot-chat"; +/// Migrates Copilot OAuth credentials from the GitHub Copilot config files +/// to the new extension-based credential location. +/// +/// This should only be called during auto-install of the extension. pub fn migrate_copilot_credentials_if_needed(extension_id: &str, cx: &mut App) { if extension_id != COPILOT_CHAT_EXTENSION_ID { return; @@ -18,27 +21,12 @@ pub fn migrate_copilot_credentials_if_needed(extension_id: &str, cx: &mut App) { let credentials_provider = ::global(cx); - cx.spawn(async move |cx| { - let existing_credential = credentials_provider - .read_credentials(&credential_key, &cx) - .await - .ok() - .flatten(); - - if existing_credential.is_some() { - log::debug!("Copilot Chat extension already has credentials, skipping migration"); - return; - } - + cx.spawn(async move |_cx| { + // Read from copilot config files let oauth_token = match read_copilot_oauth_token().await { Some(token) if !token.is_empty() => token, _ => { - log::debug!("No existing Copilot OAuth token found, marking as migrated"); - // Write empty credentials as a marker that migration was attempted - credentials_provider - .write_credentials(&credential_key, "api_key", b"", &cx) - .await - .log_err(); + log::debug!("No existing Copilot OAuth token found to migrate"); return; } }; @@ -46,7 +34,7 @@ pub fn migrate_copilot_credentials_if_needed(extension_id: &str, cx: &mut App) { log::info!("Migrating existing Copilot OAuth token to Copilot Chat extension"); match credentials_provider - .write_credentials(&credential_key, "api_key", oauth_token.as_bytes(), &cx) + .write_credentials(&credential_key, "api_key", oauth_token.as_bytes(), &_cx) .await { Ok(()) => { @@ -193,31 +181,6 @@ mod tests { assert_eq!(token, Some("github_token".to_string())); } - #[gpui::test] - async fn test_skips_migration_if_extension_already_has_credentials(cx: &mut TestAppContext) { - let existing_token = "existing_oauth_token"; - - cx.write_credentials( - "extension-llm-copilot-chat:copilot-chat", - "api_key", - existing_token.as_bytes(), - ); - - cx.update(|cx| { - migrate_copilot_credentials_if_needed(COPILOT_CHAT_EXTENSION_ID, cx); - }); - - cx.run_until_parked(); - - let credentials = cx.read_credentials("extension-llm-copilot-chat:copilot-chat"); - let (_, password) = credentials.unwrap(); - assert_eq!( - String::from_utf8(password).unwrap(), - existing_token, - "Should not overwrite existing credentials" - ); - } - #[gpui::test] async fn test_skips_migration_for_other_extensions(cx: &mut TestAppContext) { cx.update(|cx| { @@ -235,9 +198,7 @@ mod tests { // Note: Unlike the other migrations, copilot migration reads from the filesystem // (copilot config files), not from the credentials provider. In tests, these files - // don't exist, and the smol async filesystem operations don't integrate well with - // the GPUI test executor's run_until_parked(). So we test the original behavior: - // no config files = no credentials written. + // don't exist, so no migration occurs. #[gpui::test] async fn test_no_credentials_when_no_copilot_config_exists(cx: &mut TestAppContext) { cx.update(|cx| { @@ -247,11 +208,9 @@ mod tests { cx.run_until_parked(); let credentials = cx.read_credentials("extension-llm-copilot-chat:copilot-chat"); - // The async task that would write the marker doesn't complete in tests - // because smol filesystem operations use a different executor assert!( credentials.is_none(), - "No credentials should be written when copilot config doesn't exist (in test environment)" + "No credentials should be written when copilot config doesn't exist" ); } } diff --git a/crates/extension_host/src/extension_host.rs b/crates/extension_host/src/extension_host.rs index ddc2754341f2c69a1b2927f562297c34eb04f0c4..1b043c0dc1f71bf3db49cdf0dc4ef47c3958ba41 100644 --- a/crates/extension_host/src/extension_host.rs +++ b/crates/extension_host/src/extension_host.rs @@ -219,6 +219,8 @@ pub struct ExtensionStore { pub enum ExtensionOperation { Upgrade, Install, + /// Auto-install from settings - triggers legacy LLM provider migrations + AutoInstall, Remove, } @@ -755,7 +757,7 @@ impl ExtensionStore { } this.update(cx, |this, cx| { - this.install_latest_extension(extension_id.clone(), cx); + this.auto_install_latest_extension(extension_id.clone(), cx); }) .ok(); } @@ -910,13 +912,11 @@ impl ExtensionStore { this.update(cx, |this, cx| this.reload(Some(extension_id.clone()), cx))? .await; - if let ExtensionOperation::Install = operation { + if matches!( + operation, + ExtensionOperation::Install | ExtensionOperation::AutoInstall + ) { this.update(cx, |this, cx| { - // Check for legacy LLM provider migration - if let Some(manifest) = this.extension_manifest_for_id(&extension_id) { - migrate_legacy_llm_provider_env_var(&manifest, cx); - } - cx.emit(Event::ExtensionInstalled(extension_id.clone())); if let Some(events) = ExtensionEvents::try_global(cx) && let Some(manifest) = this.extension_manifest_for_id(&extension_id) @@ -926,15 +926,26 @@ impl ExtensionStore { }); } - // Run extension-specific migrations - copilot_migration::migrate_copilot_credentials_if_needed(&extension_id, cx); - anthropic_migration::migrate_anthropic_credentials_if_needed(&extension_id, cx); - google_ai_migration::migrate_google_ai_credentials_if_needed(&extension_id, cx); - openai_migration::migrate_openai_credentials_if_needed(&extension_id, cx); - open_router_migration::migrate_open_router_credentials_if_needed( - &extension_id, - cx, - ); + // Run legacy LLM provider migrations only for auto-installed extensions + if matches!(operation, ExtensionOperation::AutoInstall) { + if let Some(manifest) = this.extension_manifest_for_id(&extension_id) { + migrate_legacy_llm_provider_env_var(&manifest, cx); + } + copilot_migration::migrate_copilot_credentials_if_needed(&extension_id, cx); + anthropic_migration::migrate_anthropic_credentials_if_needed( + &extension_id, + cx, + ); + google_ai_migration::migrate_google_ai_credentials_if_needed( + &extension_id, + cx, + ); + openai_migration::migrate_openai_credentials_if_needed(&extension_id, cx); + open_router_migration::migrate_open_router_credentials_if_needed( + &extension_id, + cx, + ); + } }) .ok(); } @@ -944,6 +955,24 @@ impl ExtensionStore { } pub fn install_latest_extension(&mut self, extension_id: Arc, cx: &mut Context) { + self.install_latest_extension_with_operation(extension_id, ExtensionOperation::Install, cx); + } + + /// Auto-install an extension, triggering legacy LLM provider migrations. + fn auto_install_latest_extension(&mut self, extension_id: Arc, cx: &mut Context) { + self.install_latest_extension_with_operation( + extension_id, + ExtensionOperation::AutoInstall, + cx, + ); + } + + fn install_latest_extension_with_operation( + &mut self, + extension_id: Arc, + operation: ExtensionOperation, + cx: &mut Context, + ) { let schema_versions = schema_version_range(); let wasm_api_versions = wasm_api_version_range(ReleaseChannel::global(cx)); @@ -966,13 +995,8 @@ impl ExtensionStore { return; }; - self.install_or_upgrade_extension_at_endpoint( - extension_id, - url, - ExtensionOperation::Install, - cx, - ) - .detach_and_log_err(cx); + self.install_or_upgrade_extension_at_endpoint(extension_id, url, operation, cx) + .detach_and_log_err(cx); } pub fn upgrade_extension( @@ -1171,11 +1195,6 @@ impl ExtensionStore { this.update(cx, |this, cx| this.reload(None, cx))?.await; this.update(cx, |this, cx| { - // Run migration for legacy LLM provider env vars - if let Some(manifest) = this.extension_manifest_for_id(&extension_id) { - migrate_legacy_llm_provider_env_var(&manifest, cx); - } - cx.emit(Event::ExtensionInstalled(extension_id.clone())); if let Some(events) = ExtensionEvents::try_global(cx) && let Some(manifest) = this.extension_manifest_for_id(&extension_id) diff --git a/crates/extension_host/src/google_ai_migration.rs b/crates/extension_host/src/google_ai_migration.rs index 369b8678f52bcb30387bf1df563cc320dde9ec39..80f930ea91359b4d852c60f8f0b3d5d30ab6e372 100644 --- a/crates/extension_host/src/google_ai_migration.rs +++ b/crates/extension_host/src/google_ai_migration.rs @@ -1,11 +1,14 @@ use credentials_provider::CredentialsProvider; use gpui::App; -use util::ResultExt as _; const GOOGLE_AI_EXTENSION_ID: &str = "google-ai"; const GOOGLE_AI_PROVIDER_ID: &str = "google-ai"; const GOOGLE_AI_DEFAULT_API_URL: &str = "https://generativelanguage.googleapis.com"; +/// Migrates Google AI API credentials from the old built-in provider location +/// to the new extension-based location. +/// +/// This should only be called during auto-install of the extension. pub fn migrate_google_ai_credentials_if_needed(extension_id: &str, cx: &mut App) { if extension_id != GOOGLE_AI_EXTENSION_ID { return; @@ -19,17 +22,7 @@ pub fn migrate_google_ai_credentials_if_needed(extension_id: &str, cx: &mut App) let credentials_provider = ::global(cx); cx.spawn(async move |cx| { - let existing_credential = credentials_provider - .read_credentials(&extension_credential_key, &cx) - .await - .ok() - .flatten(); - - if existing_credential.is_some() { - log::debug!("Google AI extension already has credentials, skipping migration"); - return; - } - + // Read from old location let old_credential = credentials_provider .read_credentials(GOOGLE_AI_DEFAULT_API_URL, &cx) .await @@ -40,8 +33,8 @@ pub fn migrate_google_ai_credentials_if_needed(extension_id: &str, cx: &mut App) Some((_, key_bytes)) => match String::from_utf8(key_bytes) { Ok(key) if !key.is_empty() => key, Ok(_) => { - log::debug!("Existing Google AI API key is empty, marking as migrated"); - String::new() + log::debug!("Existing Google AI API key is empty, nothing to migrate"); + return; } Err(_) => { log::error!("Failed to decode Google AI API key as UTF-8"); @@ -49,20 +42,11 @@ pub fn migrate_google_ai_credentials_if_needed(extension_id: &str, cx: &mut App) } }, None => { - log::debug!("No existing Google AI API key found, marking as migrated"); - String::new() + log::debug!("No existing Google AI API key found to migrate"); + return; } }; - if api_key.is_empty() { - // Write empty credentials as a marker that migration was attempted - credentials_provider - .write_credentials(&extension_credential_key, "Bearer", b"", &cx) - .await - .log_err(); - return; - } - log::info!("Migrating existing Google AI API key to Google AI extension"); match credentials_provider @@ -105,41 +89,7 @@ mod tests { } #[gpui::test] - async fn test_skips_migration_if_extension_already_has_credentials(cx: &mut TestAppContext) { - let old_api_key = "AIzaSy-old-key"; - let existing_key = "AIzaSy-existing-key"; - - cx.write_credentials(GOOGLE_AI_DEFAULT_API_URL, "Bearer", old_api_key.as_bytes()); - cx.write_credentials( - "extension-llm-google-ai:google-ai", - "Bearer", - existing_key.as_bytes(), - ); - - cx.update(|cx| { - migrate_google_ai_credentials_if_needed(GOOGLE_AI_EXTENSION_ID, cx); - }); - - cx.run_until_parked(); - - let credentials = cx.read_credentials("extension-llm-google-ai:google-ai"); - let (_, password) = credentials.unwrap(); - assert_eq!( - String::from_utf8(password).unwrap(), - existing_key, - "Should not overwrite existing credentials" - ); - } - - #[gpui::test] - async fn test_skips_migration_if_empty_marker_exists(cx: &mut TestAppContext) { - let old_api_key = "AIzaSy-old-key"; - - // Old credentials exist - cx.write_credentials(GOOGLE_AI_DEFAULT_API_URL, "Bearer", old_api_key.as_bytes()); - // But empty marker already exists (from previous migration attempt) - cx.write_credentials("extension-llm-google-ai:google-ai", "Bearer", b""); - + async fn test_no_migration_if_no_old_credentials(cx: &mut TestAppContext) { cx.update(|cx| { migrate_google_ai_credentials_if_needed(GOOGLE_AI_EXTENSION_ID, cx); }); @@ -147,29 +97,10 @@ mod tests { cx.run_until_parked(); let credentials = cx.read_credentials("extension-llm-google-ai:google-ai"); - let (_, password) = credentials.unwrap(); assert!( - 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_google_ai_credentials_if_needed(GOOGLE_AI_EXTENSION_ID, cx); - }); - - cx.run_until_parked(); - - let credentials = cx.read_credentials("extension-llm-google-ai:google-ai"); - assert!( - credentials.is_some(), - "Should write empty credentials as migration marker" + credentials.is_none(), + "Should not create credentials if none existed" ); - 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 1f236d07860e99d1750cbdd21ab841e430b34149..1a7343293e54cfd0929c5c779f88ec400421ec35 100644 --- a/crates/extension_host/src/open_router_migration.rs +++ b/crates/extension_host/src/open_router_migration.rs @@ -1,11 +1,14 @@ use credentials_provider::CredentialsProvider; use gpui::App; -use util::ResultExt as _; const OPEN_ROUTER_EXTENSION_ID: &str = "openrouter"; const OPEN_ROUTER_PROVIDER_ID: &str = "openrouter"; const OPEN_ROUTER_DEFAULT_API_URL: &str = "https://openrouter.ai/api/v1"; +/// Migrates OpenRouter API credentials from the old built-in provider location +/// to the new extension-based location. +/// +/// This should only be called during auto-install of the extension. pub fn migrate_open_router_credentials_if_needed(extension_id: &str, cx: &mut App) { if extension_id != OPEN_ROUTER_EXTENSION_ID { return; @@ -19,17 +22,7 @@ pub fn migrate_open_router_credentials_if_needed(extension_id: &str, cx: &mut Ap let credentials_provider = ::global(cx); cx.spawn(async move |cx| { - let existing_credential = credentials_provider - .read_credentials(&extension_credential_key, &cx) - .await - .ok() - .flatten(); - - if existing_credential.is_some() { - log::debug!("OpenRouter extension already has credentials, skipping migration"); - return; - } - + // Read from old location let old_credential = credentials_provider .read_credentials(OPEN_ROUTER_DEFAULT_API_URL, &cx) .await @@ -40,8 +33,8 @@ pub fn migrate_open_router_credentials_if_needed(extension_id: &str, cx: &mut Ap Some((_, key_bytes)) => match String::from_utf8(key_bytes) { Ok(key) if !key.is_empty() => key, Ok(_) => { - log::debug!("Existing OpenRouter API key is empty, marking as migrated"); - String::new() + log::debug!("Existing OpenRouter API key is empty, nothing to migrate"); + return; } Err(_) => { log::error!("Failed to decode OpenRouter API key as UTF-8"); @@ -49,20 +42,11 @@ pub fn migrate_open_router_credentials_if_needed(extension_id: &str, cx: &mut Ap } }, None => { - log::debug!("No existing OpenRouter API key found, marking as migrated"); - String::new() + log::debug!("No existing OpenRouter API key found to migrate"); + return; } }; - if api_key.is_empty() { - // Write empty credentials as a marker that migration was attempted - credentials_provider - .write_credentials(&extension_credential_key, "Bearer", b"", &cx) - .await - .log_err(); - return; - } - log::info!("Migrating existing OpenRouter API key to OpenRouter extension"); match credentials_provider @@ -105,21 +89,7 @@ mod tests { } #[gpui::test] - async fn test_skips_migration_if_extension_already_has_credentials(cx: &mut TestAppContext) { - let old_api_key = "sk-or-old-key"; - let existing_key = "sk-or-existing-key"; - - cx.write_credentials( - OPEN_ROUTER_DEFAULT_API_URL, - "Bearer", - old_api_key.as_bytes(), - ); - cx.write_credentials( - "extension-llm-openrouter:openrouter", - "Bearer", - existing_key.as_bytes(), - ); - + async fn test_no_migration_if_no_old_credentials(cx: &mut TestAppContext) { cx.update(|cx| { migrate_open_router_credentials_if_needed(OPEN_ROUTER_EXTENSION_ID, cx); }); @@ -127,57 +97,10 @@ mod tests { cx.run_until_parked(); let credentials = cx.read_credentials("extension-llm-openrouter:openrouter"); - let (_, password) = credentials.unwrap(); - assert_eq!( - String::from_utf8(password).unwrap(), - existing_key, - "Should not overwrite existing credentials" - ); - } - - #[gpui::test] - async fn test_skips_migration_if_empty_marker_exists(cx: &mut TestAppContext) { - let old_api_key = "sk-or-old-key"; - - // Old credentials exist - cx.write_credentials( - OPEN_ROUTER_DEFAULT_API_URL, - "Bearer", - old_api_key.as_bytes(), - ); - // But empty marker already exists (from previous migration attempt) - cx.write_credentials("extension-llm-openrouter:openrouter", "Bearer", b""); - - cx.update(|cx| { - migrate_open_router_credentials_if_needed(OPEN_ROUTER_EXTENSION_ID, cx); - }); - - cx.run_until_parked(); - - let credentials = cx.read_credentials("extension-llm-openrouter:openrouter"); - let (_, password) = credentials.unwrap(); assert!( - 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_open_router_credentials_if_needed(OPEN_ROUTER_EXTENSION_ID, cx); - }); - - cx.run_until_parked(); - - let credentials = cx.read_credentials("extension-llm-openrouter:openrouter"); - assert!( - credentials.is_some(), - "Should write empty credentials as migration marker" + credentials.is_none(), + "Should not create credentials if none existed" ); - 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 5138e4610f9e1492b44d5a79ea71986fe2dfb8c0..2cd187f9d738b6a59950c69607fb8ce7f56c69b0 100644 --- a/crates/extension_host/src/openai_migration.rs +++ b/crates/extension_host/src/openai_migration.rs @@ -1,11 +1,14 @@ use credentials_provider::CredentialsProvider; use gpui::App; -use util::ResultExt as _; const OPENAI_EXTENSION_ID: &str = "openai"; const OPENAI_PROVIDER_ID: &str = "openai"; const OPENAI_DEFAULT_API_URL: &str = "https://api.openai.com/v1"; +/// Migrates OpenAI API credentials from the old built-in provider location +/// to the new extension-based location. +/// +/// This should only be called during auto-install of the extension. pub fn migrate_openai_credentials_if_needed(extension_id: &str, cx: &mut App) { if extension_id != OPENAI_EXTENSION_ID { return; @@ -19,17 +22,7 @@ pub fn migrate_openai_credentials_if_needed(extension_id: &str, cx: &mut App) { let credentials_provider = ::global(cx); cx.spawn(async move |cx| { - let existing_credential = credentials_provider - .read_credentials(&extension_credential_key, &cx) - .await - .ok() - .flatten(); - - if existing_credential.is_some() { - log::debug!("OpenAI extension already has credentials, skipping migration"); - return; - } - + // Read from old location let old_credential = credentials_provider .read_credentials(OPENAI_DEFAULT_API_URL, &cx) .await @@ -40,8 +33,8 @@ pub fn migrate_openai_credentials_if_needed(extension_id: &str, cx: &mut App) { Some((_, key_bytes)) => match String::from_utf8(key_bytes) { Ok(key) if !key.is_empty() => key, Ok(_) => { - log::debug!("Existing OpenAI API key is empty, marking as migrated"); - String::new() + log::debug!("Existing OpenAI API key is empty, nothing to migrate"); + return; } Err(_) => { log::error!("Failed to decode OpenAI API key as UTF-8"); @@ -49,20 +42,11 @@ pub fn migrate_openai_credentials_if_needed(extension_id: &str, cx: &mut App) { } }, None => { - log::debug!("No existing OpenAI API key found, marking as migrated"); - String::new() + log::debug!("No existing OpenAI API key found to migrate"); + return; } }; - if api_key.is_empty() { - // Write empty credentials as a marker that migration was attempted - credentials_provider - .write_credentials(&extension_credential_key, "Bearer", b"", &cx) - .await - .log_err(); - return; - } - log::info!("Migrating existing OpenAI API key to OpenAI extension"); match credentials_provider @@ -105,41 +89,7 @@ mod tests { } #[gpui::test] - async fn test_skips_migration_if_extension_already_has_credentials(cx: &mut TestAppContext) { - let old_api_key = "sk-old-key"; - let existing_key = "sk-existing-key"; - - cx.write_credentials(OPENAI_DEFAULT_API_URL, "Bearer", old_api_key.as_bytes()); - cx.write_credentials( - "extension-llm-openai:openai", - "Bearer", - existing_key.as_bytes(), - ); - - cx.update(|cx| { - migrate_openai_credentials_if_needed(OPENAI_EXTENSION_ID, cx); - }); - - cx.run_until_parked(); - - let credentials = cx.read_credentials("extension-llm-openai:openai"); - let (_, password) = credentials.unwrap(); - assert_eq!( - String::from_utf8(password).unwrap(), - existing_key, - "Should not overwrite existing credentials" - ); - } - - #[gpui::test] - async fn test_skips_migration_if_empty_marker_exists(cx: &mut TestAppContext) { - let old_api_key = "sk-old-key"; - - // Old credentials exist - cx.write_credentials(OPENAI_DEFAULT_API_URL, "Bearer", old_api_key.as_bytes()); - // But empty marker already exists (from previous migration attempt) - cx.write_credentials("extension-llm-openai:openai", "Bearer", b""); - + async fn test_no_migration_if_no_old_credentials(cx: &mut TestAppContext) { cx.update(|cx| { migrate_openai_credentials_if_needed(OPENAI_EXTENSION_ID, cx); }); @@ -147,29 +97,10 @@ mod tests { cx.run_until_parked(); let credentials = cx.read_credentials("extension-llm-openai:openai"); - let (_, password) = credentials.unwrap(); assert!( - 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_openai_credentials_if_needed(OPENAI_EXTENSION_ID, cx); - }); - - cx.run_until_parked(); - - let credentials = cx.read_credentials("extension-llm-openai:openai"); - assert!( - credentials.is_some(), - "Should write empty credentials as migration marker" + credentials.is_none(), + "Should not create credentials if none existed" ); - 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/wasm_host/llm_provider.rs b/crates/extension_host/src/wasm_host/llm_provider.rs index 1c1334f58dc07d00dfc6be29b894324f279c4d18..acfdc965ba522540f5789f8cb4c45d65cf51d02a 100644 --- a/crates/extension_host/src/wasm_host/llm_provider.rs +++ b/crates/extension_host/src/wasm_host/llm_provider.rs @@ -428,10 +428,7 @@ impl ExtensionProviderConfigurationView { .log_err() .flatten(); - // Treat empty credentials as not authenticated (used as migration marker) - let has_credentials = credentials - .map(|(_, password)| !password.is_empty()) - .unwrap_or(false); + let has_credentials = credentials.is_some(); // Update authentication state based on stored credentials cx.update(|cx| { @@ -539,10 +536,7 @@ impl ExtensionProviderConfigurationView { .log_err() .flatten(); - // Treat empty credentials as not authenticated (used as migration marker) - let has_credentials = credentials - .map(|(_, password)| !password.is_empty()) - .unwrap_or(false); + let has_credentials = credentials.is_some(); cx.update(|cx| { state.update(cx, |state, cx| {