From fc264296bf8bdae7b768dc6965c6f5e35f3e74c0 Mon Sep 17 00:00:00 2001 From: Sun Zhenyuan Date: Thu, 2 Jul 2026 17:04:54 +0800 Subject: [PATCH] feat: add dynamic MCP server infrastructure to McpPool Add in-memory dynamic server support to McpPool for runtime-started MCP servers from conversation context. - New `dynamic_servers` field (parking_lot::RwLock) on McpPool - `add_runtime_server_config()` rejects duplicate names (static or dynamic) - `get_or_connect()` checks dynamic servers before static config - `server_names()` includes both static and dynamic servers - Conflict tests for static/dynamic name overlap Prerequisite for #3866 (start_mcp_server tool). Extracted as a focused infrastructure PR to isolate the trust-boundary surface (runtime MCP pool mutation) from the tool implementation. --- crates/tui/src/mcp.rs | 60 ++++++++++++++++++++---- crates/tui/src/mcp/tests.rs | 91 +++++++++++++++++++++++++++++++------ 2 files changed, 129 insertions(+), 22 deletions(-) diff --git a/crates/tui/src/mcp.rs b/crates/tui/src/mcp.rs index 1f21595585..4140f9d22f 100644 --- a/crates/tui/src/mcp.rs +++ b/crates/tui/src/mcp.rs @@ -9,10 +9,12 @@ use std::collections::HashMap; use std::fs; use std::io::Read; use std::path::{Component, Path, PathBuf}; +use std::sync::Arc; use std::sync::atomic::{AtomicU64, Ordering}; use std::time::Duration; use anyhow::{Context, Result}; +use parking_lot::RwLock; use serde::{Deserialize, Serialize}; mod headers; @@ -1432,6 +1434,9 @@ pub struct McpPool { config_hash: u64, /// Most recently observed mtime for `config_sources`. last_mtimes: Vec>, + /// Dynamically added MCP servers (from tool calls at runtime). + /// These are not persisted to disk and live for the process lifetime. + pub(crate) dynamic_servers: Arc>>, } impl McpPool { @@ -1446,6 +1451,7 @@ impl McpPool { workspace: None, config_hash, last_mtimes: Vec::new(), + dynamic_servers: Arc::new(RwLock::new(HashMap::new())), } } @@ -1589,12 +1595,14 @@ impl McpPool { self.drop_connection(server_name, "reconnect"); + // Check static config first, then dynamic servers let server_config = self .config .servers .get(server_name) - .ok_or_else(|| anyhow::anyhow!("Failed to find MCP server: {server_name}"))? - .clone(); + .cloned() + .or_else(|| self.dynamic_servers.read().get(server_name).cloned()) + .ok_or_else(|| anyhow::anyhow!("Failed to find MCP server: {server_name}"))?; if !server_config.is_enabled() { anyhow::bail!("Failed to connect MCP server '{server_name}': server is disabled"); @@ -2084,14 +2092,48 @@ impl McpPool { } } - /// Get list of configured server names + /// Get list of configured server names (static + dynamic) #[allow(dead_code)] // Public API for MCP consumers - pub fn server_names(&self) -> Vec<&str> { - self.config - .servers - .keys() - .map(std::string::String::as_str) - .collect() + pub fn server_names(&self) -> Vec { + let mut names: Vec = self.config.servers.keys().cloned().collect(); + let dynamic = self.dynamic_servers.read(); + for name in dynamic.keys() { + if !names.contains(name) { + names.push(name.clone()); + } + } + names + } + + /// Add a runtime server configuration (in-memory only, not persisted). + /// + /// This is used for dynamically started MCP servers from chat context. + /// Stored in `dynamic_servers` so it doesn't interfere with file-based config reload. + /// + /// Returns `Err` if a server with the same name already exists as a static config + /// or a dynamic config. The caller should surface the error to the LLM/user. + pub fn add_runtime_server_config( + &self, + name: String, + config: McpServerConfig, + ) -> Result<(), String> { + if self.config.servers.contains_key(&name) { + return Err(format!( + "MCP server '{}' already exists in the config file. \ + Remove it from the config first, or choose a different name.", + name + )); + } + let mut dynamic = self.dynamic_servers.write(); + if dynamic.contains_key(&name) { + return Err(format!( + "MCP server '{}' was already started earlier in this session. \ + Choose a different name.", + name + )); + } + dynamic.insert(name, config); + Ok(()) } /// Get list of connected server names diff --git a/crates/tui/src/mcp/tests.rs b/crates/tui/src/mcp/tests.rs index 045d430689..3fc7b7e2d8 100644 --- a/crates/tui/src/mcp/tests.rs +++ b/crates/tui/src/mcp/tests.rs @@ -765,7 +765,7 @@ async fn workspace_mcp_pool_reload_picks_up_project_config_creation() { .unwrap(); let mut pool = McpPool::from_config_path_with_workspace(&global_path, &workspace).unwrap(); - assert_eq!(pool.server_names(), vec!["global"]); + assert_eq!(pool.server_names(), vec!["global".to_string()]); fs::create_dir_all(&project_dir).unwrap(); fs::write( @@ -775,8 +775,11 @@ async fn workspace_mcp_pool_reload_picks_up_project_config_creation() { .unwrap(); assert!(pool.reload_if_config_changed().await.unwrap()); - let names: std::collections::BTreeSet<_> = pool.server_names().into_iter().collect(); - let expected: std::collections::BTreeSet<_> = ["global", "project"].into_iter().collect(); + let names: std::collections::BTreeSet = pool.server_names().into_iter().collect(); + let expected: std::collections::BTreeSet = + ["global".to_string(), "project".to_string()] + .into_iter() + .collect(); assert_eq!(names, expected); } @@ -800,13 +803,16 @@ async fn workspace_mcp_pool_reload_picks_up_project_config_after_workspace_trust .unwrap(); let mut pool = McpPool::from_config_path_with_workspace(&global_path, &workspace).unwrap(); - assert_eq!(pool.server_names(), vec!["global"]); + assert_eq!(pool.server_names(), vec!["global".to_string()]); write_workspace_trust_config(&trust_env.config_path, &workspace); assert!(pool.reload_if_config_changed().await.unwrap()); - let names: std::collections::BTreeSet<_> = pool.server_names().into_iter().collect(); - let expected: std::collections::BTreeSet<_> = ["global", "project"].into_iter().collect(); + let names: std::collections::BTreeSet = pool.server_names().into_iter().collect(); + let expected: std::collections::BTreeSet = + ["global".to_string(), "project".to_string()] + .into_iter() + .collect(); assert_eq!(names, expected); } @@ -830,14 +836,17 @@ async fn workspace_mcp_pool_reload_drops_project_config_after_workspace_trust_re .unwrap(); let mut pool = McpPool::from_config_path_with_workspace(&global_path, &workspace).unwrap(); - let names: std::collections::BTreeSet<_> = pool.server_names().into_iter().collect(); - let expected: std::collections::BTreeSet<_> = ["global", "project"].into_iter().collect(); + let names: std::collections::BTreeSet = pool.server_names().into_iter().collect(); + let expected: std::collections::BTreeSet = + ["global".to_string(), "project".to_string()] + .into_iter() + .collect(); assert_eq!(names, expected); fs::remove_file(&trust.config_path).unwrap(); assert!(pool.reload_if_config_changed().await.unwrap()); - assert_eq!(pool.server_names(), vec!["global"]); + assert_eq!(pool.server_names(), vec!["global".to_string()]); } #[tokio::test] @@ -861,14 +870,17 @@ async fn workspace_mcp_pool_reload_drops_project_config_after_deletion() { .unwrap(); let mut pool = McpPool::from_config_path_with_workspace(&global_path, &workspace).unwrap(); - let names: std::collections::BTreeSet<_> = pool.server_names().into_iter().collect(); - let expected: std::collections::BTreeSet<_> = ["global", "project"].into_iter().collect(); + let names: std::collections::BTreeSet = pool.server_names().into_iter().collect(); + let expected: std::collections::BTreeSet = + ["global".to_string(), "project".to_string()] + .into_iter() + .collect(); assert_eq!(names, expected); fs::remove_file(project_path).unwrap(); assert!(pool.reload_if_config_changed().await.unwrap()); - assert_eq!(pool.server_names(), vec!["global"]); + assert_eq!(pool.server_names(), vec!["global".to_string()]); } #[test] @@ -1345,7 +1357,7 @@ async fn reload_if_config_changed_swaps_config_on_content_change() { assert!(reloaded, "content-changed config must trigger reload"); let names = pool.server_names(); assert!( - names.contains(&"new"), + names.contains(&"new".to_string()), "expected new server in pool after reload, got {names:?}" ); } @@ -3154,3 +3166,56 @@ async fn custom_headers_applied_to_get_preflight() { "GET preflight must include user-configured custom headers" ); } + +// === add_runtime_server_config conflict tests === + +#[test] +fn add_runtime_server_config_rejects_static_conflict() { + let config: McpConfig = serde_json::from_str( + r#"{ + "servers": { + "existing": {"command": "node server.js"} + } + }"#, + ) + .unwrap(); + let pool = McpPool::new(config); + + let err = pool + .add_runtime_server_config( + "existing".to_string(), + serde_json::from_str(r#"{"command": "npx other"}"#).unwrap(), + ) + .unwrap_err(); + assert!(err.contains("already exists in the config file")); +} + +#[test] +fn add_runtime_server_config_rejects_dynamic_duplicate() { + let pool = McpPool::new(McpConfig::default()); + + pool.add_runtime_server_config( + "my_server".to_string(), + serde_json::from_str(r#"{"command": "node a.js"}"#).unwrap(), + ) + .unwrap(); + + let err = pool + .add_runtime_server_config( + "my_server".to_string(), + serde_json::from_str(r#"{"command": "node b.js"}"#).unwrap(), + ) + .unwrap_err(); + assert!(err.contains("already started earlier")); +} + +#[test] +fn add_runtime_server_config_accepts_new_name() { + let pool = McpPool::new(McpConfig::default()); + + pool.add_runtime_server_config( + "brand_new".to_string(), + serde_json::from_str(r#"{"command": "node x.js"}"#).unwrap(), + ) + .unwrap(); +}