Detailed changes
@@ -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";
@@ -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");
}
}
@@ -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<str> = 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(<dyn fs::Fs>::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);
}
}
});
@@ -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<Arc<str>>,
- /// 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<Arc<str>>,
}
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(),
}
}
}
@@ -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]
@@ -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]
@@ -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]
@@ -26,9 +26,6 @@ pub struct ExtensionSettingsContent {
///
/// Default: []
pub allowed_env_var_providers: Option<Vec<Arc<str>>>,
- /// 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<Vec<Arc<str>>>,
}
/// A capability for an extension.