From 717bf3548417a0f47810dab13416977fced13931 Mon Sep 17 00:00:00 2001 From: Bennet Bo Fenner Date: Wed, 11 Jun 2025 21:30:03 +0200 Subject: [PATCH] agent: Remove context server settings when uninstalling MCP extension (#32560) Release Notes: - agent: Automatically remove context server settings when uninstalling MCP extension --- crates/agent/src/agent.rs | 2 +- .../agent/src/context_server_configuration.rs | 29 +++- crates/extension/src/extension_events.rs | 1 + crates/extension_host/src/extension_host.rs | 14 ++ crates/project/src/lsp_store.rs | 1 + crates/settings/src/settings_store.rs | 143 ++++++++++++++++-- 6 files changed, 171 insertions(+), 19 deletions(-) diff --git a/crates/agent/src/agent.rs b/crates/agent/src/agent.rs index 0ac7869920..94edf76e97 100644 --- a/crates/agent/src/agent.rs +++ b/crates/agent/src/agent.rs @@ -162,7 +162,7 @@ pub fn init( assistant_slash_command::init(cx); thread_store::init(cx); agent_panel::init(cx); - context_server_configuration::init(language_registry, cx); + context_server_configuration::init(language_registry, fs.clone(), cx); register_slash_commands(cx); inline_assistant::init( diff --git a/crates/agent/src/context_server_configuration.rs b/crates/agent/src/context_server_configuration.rs index 49effbdc0f..aa9a92eef5 100644 --- a/crates/agent/src/context_server_configuration.rs +++ b/crates/agent/src/context_server_configuration.rs @@ -3,16 +3,21 @@ use std::sync::Arc; use anyhow::Context as _; use context_server::ContextServerId; use extension::{ContextServerConfiguration, ExtensionManifest}; +use fs::Fs; use gpui::Task; use language::LanguageRegistry; -use project::context_server_store::registry::ContextServerDescriptorRegistry; +use project::{ + context_server_store::registry::ContextServerDescriptorRegistry, + project_settings::ProjectSettings, +}; +use settings::update_settings_file; use ui::prelude::*; use util::ResultExt; use workspace::Workspace; use crate::agent_configuration::ConfigureContextServerModal; -pub(crate) fn init(language_registry: Arc, cx: &mut App) { +pub(crate) fn init(language_registry: Arc, fs: Arc, cx: &mut App) { cx.observe_new(move |_: &mut Workspace, window, cx| { let Some(window) = window else { return; @@ -21,6 +26,7 @@ pub(crate) fn init(language_registry: Arc, cx: &mut App) { if let Some(extension_events) = extension::ExtensionEvents::try_global(cx).as_ref() { cx.subscribe_in(extension_events, window, { let language_registry = language_registry.clone(); + let fs = fs.clone(); move |workspace, _, event, window, cx| match event { extension::Event::ExtensionInstalled(manifest) => { show_configure_mcp_modal( @@ -31,6 +37,13 @@ pub(crate) fn init(language_registry: Arc, cx: &mut App) { cx, ); } + extension::Event::ExtensionUninstalled(manifest) => { + remove_context_server_settings( + manifest.context_servers.keys().cloned().collect(), + fs.clone(), + cx, + ); + } extension::Event::ConfigureExtensionRequested(manifest) => { if !manifest.context_servers.is_empty() { show_configure_mcp_modal( @@ -55,6 +68,18 @@ pub(crate) fn init(language_registry: Arc, cx: &mut App) { .detach(); } +fn remove_context_server_settings( + context_server_ids: Vec>, + fs: Arc, + cx: &mut App, +) { + update_settings_file::(fs, cx, move |settings, _| { + settings + .context_servers + .retain(|server_id, _| !context_server_ids.contains(server_id)); + }); +} + pub enum Configuration { NotAvailable(ContextServerId, Option), Required( diff --git a/crates/extension/src/extension_events.rs b/crates/extension/src/extension_events.rs index 73075067fd..b151b3f412 100644 --- a/crates/extension/src/extension_events.rs +++ b/crates/extension/src/extension_events.rs @@ -36,6 +36,7 @@ impl ExtensionEvents { #[derive(Clone)] pub enum Event { ExtensionInstalled(Arc), + ExtensionUninstalled(Arc), ExtensionsInstalledChanged, ConfigureExtensionRequested(Arc), } diff --git a/crates/extension_host/src/extension_host.rs b/crates/extension_host/src/extension_host.rs index cd34521935..4e6b624d49 100644 --- a/crates/extension_host/src/extension_host.rs +++ b/crates/extension_host/src/extension_host.rs @@ -132,6 +132,7 @@ pub enum Event { ExtensionsUpdated, StartedReloading, ExtensionInstalled(Arc), + ExtensionUninstalled(Arc), ExtensionFailedToLoad(Arc), } @@ -842,6 +843,8 @@ impl ExtensionStore { let work_dir = self.wasm_host.work_dir.join(extension_id.as_ref()); let fs = self.fs.clone(); + let extension_manifest = self.extension_manifest_for_id(&extension_id).cloned(); + match self.outstanding_operations.entry(extension_id.clone()) { btree_map::Entry::Occupied(_) => return, btree_map::Entry::Vacant(e) => e.insert(ExtensionOperation::Remove), @@ -878,6 +881,17 @@ impl ExtensionStore { ) .await?; + this.update(cx, |_, cx| { + cx.emit(Event::ExtensionUninstalled(extension_id.clone())); + if let Some(events) = ExtensionEvents::try_global(cx) { + if let Some(manifest) = extension_manifest { + events.update(cx, |this, cx| { + this.emit(extension::Event::ExtensionUninstalled(manifest.clone()), cx) + }); + } + } + })?; + anyhow::Ok(()) }) .detach_and_log_err(cx) diff --git a/crates/project/src/lsp_store.rs b/crates/project/src/lsp_store.rs index 49663f09f2..5bf77d26bd 100644 --- a/crates/project/src/lsp_store.rs +++ b/crates/project/src/lsp_store.rs @@ -3934,6 +3934,7 @@ impl LspStore { ) { match evt { extension::Event::ExtensionInstalled(_) + | extension::Event::ExtensionUninstalled(_) | extension::Event::ConfigureExtensionRequested(_) => return, extension::Event::ExtensionsInstalledChanged => {} } diff --git a/crates/settings/src/settings_store.rs b/crates/settings/src/settings_store.rs index a7c295fc64..daf7a2c7d5 100644 --- a/crates/settings/src/settings_store.rs +++ b/crates/settings/src/settings_store.rs @@ -1349,16 +1349,23 @@ fn update_value_in_json_text<'a>( if let (Value::Object(old_object), Value::Object(new_object)) = (old_value, new_value) { for (key, old_sub_value) in old_object.iter() { key_path.push(key); - let new_sub_value = new_object.get(key).unwrap_or(&Value::Null); - update_value_in_json_text( - text, - key_path, - tab_size, - old_sub_value, - new_sub_value, - preserved_keys, - edits, - ); + if let Some(new_sub_value) = new_object.get(key) { + // Key exists in both old and new, recursively update + update_value_in_json_text( + text, + key_path, + tab_size, + old_sub_value, + new_sub_value, + preserved_keys, + edits, + ); + } else { + // Key was removed from new object, remove the entire key-value pair + let (range, replacement) = replace_value_in_json_text(text, key_path, 0, None); + text.replace_range(range.clone(), &replacement); + edits.push((range, replacement)); + } key_path.pop(); } for (key, new_sub_value) in new_object.iter() { @@ -1385,7 +1392,8 @@ fn update_value_in_json_text<'a>( if let Some(new_object) = new_value.as_object_mut() { new_object.retain(|_, v| !v.is_null()); } - let (range, replacement) = replace_value_in_json_text(text, key_path, tab_size, &new_value); + let (range, replacement) = + replace_value_in_json_text(text, key_path, tab_size, Some(&new_value)); text.replace_range(range.clone(), &replacement); edits.push((range, replacement)); } @@ -1395,7 +1403,7 @@ fn replace_value_in_json_text( text: &str, key_path: &[&str], tab_size: usize, - new_value: &Value, + new_value: Option<&Value>, ) -> (Range, String) { static PAIR_QUERY: LazyLock = LazyLock::new(|| { Query::new( @@ -1461,16 +1469,64 @@ fn replace_value_in_json_text( } } - // We found the exact key we want, insert the new value + // We found the exact key we want if depth == key_path.len() { - let new_val = to_pretty_json(&new_value, tab_size, tab_size * depth); - (existing_value_range, new_val) + if let Some(new_value) = new_value { + let new_val = to_pretty_json(new_value, tab_size, tab_size * depth); + (existing_value_range, new_val) + } else { + let mut removal_start = first_key_start.unwrap_or(existing_value_range.start); + let mut removal_end = existing_value_range.end; + + // Find the actual key position by looking for the key in the pair + // We need to extend the range to include the key, not just the value + if let Some(key_start) = text[..existing_value_range.start].rfind('"') { + if let Some(prev_key_start) = text[..key_start].rfind('"') { + removal_start = prev_key_start; + } else { + removal_start = key_start; + } + } + + // Look backward for a preceding comma first + let preceding_text = text.get(0..removal_start).unwrap_or(""); + if let Some(comma_pos) = preceding_text.rfind(',') { + // Check if there are only whitespace characters between the comma and our key + let between_comma_and_key = text.get(comma_pos + 1..removal_start).unwrap_or(""); + if between_comma_and_key.trim().is_empty() { + removal_start = comma_pos; + } + } else { + // No preceding comma, check for trailing comma + if let Some(remaining_text) = text.get(existing_value_range.end..) { + let mut chars = remaining_text.char_indices(); + while let Some((offset, ch)) = chars.next() { + if ch == ',' { + removal_end = existing_value_range.end + offset + 1; + // Also consume whitespace after the comma + while let Some((_, next_ch)) = chars.next() { + if next_ch.is_whitespace() { + removal_end += next_ch.len_utf8(); + } else { + break; + } + } + break; + } else if !ch.is_whitespace() { + break; + } + } + } + } + (removal_start..removal_end, String::new()) + } } else { // We have key paths, construct the sub objects let new_key = key_path[depth]; // We don't have the key, construct the nested objects - let mut new_value = serde_json::to_value(new_value).unwrap(); + let mut new_value = + serde_json::to_value(new_value.unwrap_or(&serde_json::Value::Null)).unwrap(); for key in key_path[(depth + 1)..].iter().rev() { new_value = serde_json::json!({ key.to_string(): new_value }); } @@ -1775,6 +1831,61 @@ mod tests { cx, ); + // entries removed + check_settings_update::( + &mut store, + r#"{ + "languages": { + "Rust": { + "language_setting_2": true + }, + "JSON": { + "language_setting_1": false + } + } + }"# + .unindent(), + |settings| { + settings.languages.remove("JSON").unwrap(); + }, + r#"{ + "languages": { + "Rust": { + "language_setting_2": true + } + } + }"# + .unindent(), + cx, + ); + + check_settings_update::( + &mut store, + r#"{ + "languages": { + "Rust": { + "language_setting_2": true + }, + "JSON": { + "language_setting_1": false + } + } + }"# + .unindent(), + |settings| { + settings.languages.remove("Rust").unwrap(); + }, + r#"{ + "languages": { + "JSON": { + "language_setting_1": false + } + } + }"# + .unindent(), + cx, + ); + // weird formatting check_settings_update::( &mut store,