From 7b7f93934614583b18bcf7401940e26dfddbf2f9 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Sun, 3 May 2026 07:53:53 -0500 Subject: [PATCH] chore(mcp): drop unused legacy sync API (340 LOC dead code) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `// === Backward Compatibility - Sync API (Legacy) ===` block in `mcp.rs` was tagged `TODO(integrate): Wire legacy sync API into CLI subcommands or remove` and had zero callers — the actual CLI flows went through the async `add_server_config` / `remove_server_config` helpers months ago. Delete the unused structs (`McpServerInput`, `LegacyMcpServer`, `LegacyMcpConfig`), pub fns (`list`, `add`, `remove`, `call_tool`), private helpers (`load_legacy`, `save_legacy`, `parse_env`, `send_request_sync`, `read_response_with_timeout`, `read_response_sync`, `next_id`), and the unix-only test that only exercised the dead timeout helper. Module doc loses the "backward compatibility with existing sync API" bullet. `std::io::{BufRead, BufReader, Write}`, `std::process::{Command, Stdio}`, `std::sync::{Arc, Mutex}`, and `std::time::{SystemTime, UNIX_EPOCH}` are no longer needed at the top level (the async path uses the tokio versions and only `Duration` from `std::time`). --- crates/tui/src/mcp.rs | 342 +----------------------------------------- 1 file changed, 1 insertion(+), 341 deletions(-) diff --git a/crates/tui/src/mcp.rs b/crates/tui/src/mcp.rs index 52c51346..e830db47 100644 --- a/crates/tui/src/mcp.rs +++ b/crates/tui/src/mcp.rs @@ -4,16 +4,12 @@ //! - Connection pooling for server reuse //! - Automatic tool discovery via `tools/list` //! - Configurable timeouts per-server and globally -//! - Backward compatibility with existing sync API use std::collections::HashMap; use std::fs; -use std::io::{BufRead, BufReader, Write}; use std::path::Path; -use std::process::{Command, Stdio}; use std::sync::atomic::{AtomicU64, Ordering}; -use std::sync::{Arc, Mutex}; -use std::time::{Duration, SystemTime, UNIX_EPOCH}; +use std::time::Duration; use anyhow::{Context, Result}; use serde::{Deserialize, Serialize}; @@ -1769,315 +1765,6 @@ pub fn format_tool_result(result: &serde_json::Value) -> String { } } -// === Backward Compatibility - Sync API (Legacy) === -// TODO(integrate): Wire legacy sync API into CLI subcommands or remove - -/// Legacy input struct for adding MCP servers -#[derive(Debug, Clone)] -#[allow(dead_code)] // Legacy sync API, not yet wired into CLI subcommands -pub struct McpServerInput { - pub name: String, - pub command: String, - pub args: Vec, - pub env: Vec, -} - -/// Legacy MCP server struct for internal use -#[derive(Debug, Serialize, Deserialize, Default)] -#[allow(dead_code)] // Legacy sync API -struct LegacyMcpServer { - command: String, - args: Vec, - env: HashMap, - #[serde(default)] - connect_timeout: Option, - #[serde(default)] - execute_timeout: Option, - #[serde(default)] - read_timeout: Option, -} - -/// Legacy config wrapper for backward compatibility -#[derive(Debug, Serialize, Deserialize, Default)] -#[allow(dead_code)] // Legacy sync API -struct LegacyMcpConfig { - #[serde(default, alias = "mcpServers")] - servers: HashMap, - #[serde(default)] - timeouts: McpTimeouts, -} - -/// List configured MCP servers (sync, for CLI) -#[allow(dead_code)] // Legacy sync API, not yet wired into CLI subcommands -pub fn list(path: &Path) -> Result<()> { - let config = load_legacy(path)?; - if config.servers.is_empty() { - println!("No MCP servers configured."); - return Ok(()); - } - - for (name, server) in config.servers { - println!("{} -> {} {}", name, server.command, server.args.join(" ")); - } - Ok(()) -} - -/// Add an MCP server to configuration (sync, for CLI) -#[allow(dead_code)] // Legacy sync API -pub fn add(path: &Path, input: McpServerInput) -> Result<()> { - let mut config = load_legacy(path)?; - let env = parse_env(&input.env)?; - config.servers.insert( - input.name.clone(), - LegacyMcpServer { - command: input.command, - args: input.args, - env, - connect_timeout: None, - execute_timeout: None, - read_timeout: None, - }, - ); - save_legacy(path, &config)?; - println!("Added MCP server: {}", input.name); - Ok(()) -} - -/// Remove an MCP server from configuration (sync, for CLI) -#[allow(dead_code)] // Legacy sync API -pub fn remove(path: &Path, name: &str) -> Result<()> { - let mut config = load_legacy(path)?; - if config.servers.remove(name).is_some() { - save_legacy(path, &config)?; - println!("Removed MCP server: {name}"); - } else { - println!("No MCP server named {name}."); - } - Ok(()) -} - -/// Call an MCP tool (sync, for backward compatibility) -#[allow(dead_code)] // Legacy sync API -pub fn call_tool( - path: &Path, - server: &str, - tool: &str, - args: &serde_json::Value, -) -> Result { - let config = load_legacy(path)?; - let Some(server_cfg) = config.servers.get(server) else { - anyhow::bail!("Failed to find MCP server: {server}"); - }; - let timeouts = config.timeouts; - let connect_timeout = server_cfg - .connect_timeout - .unwrap_or(timeouts.connect_timeout); - let execute_timeout = server_cfg - .execute_timeout - .unwrap_or(timeouts.execute_timeout); - let read_timeout = server_cfg.read_timeout.unwrap_or(timeouts.read_timeout); - - let mut cmd = Command::new(&server_cfg.command); - cmd.args(&server_cfg.args) - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()); - - for (key, value) in &server_cfg.env { - cmd.env(key, value); - } - - let mut child = cmd.spawn().with_context(|| "Failed to spawn MCP server")?; - let mut stdin = child.stdin.take().context("Failed to open MCP stdin")?; - let stdout = child.stdout.take().context("Failed to open MCP stdout")?; - let reader = Arc::new(Mutex::new(BufReader::new(stdout))); - let child = Arc::new(Mutex::new(child)); - - let init_id = next_id(); - let init_payload = serde_json::json!({ - "jsonrpc": "2.0", - "id": init_id, - "method": "initialize", - "params": { - "protocolVersion": "2024-11-05", - "clientInfo": { "name": "deepseek-tui", "version": env!("CARGO_PKG_VERSION") }, - "capabilities": {} - } - }); - send_request_sync(&mut stdin, &init_payload)?; - if let Err(e) = read_response_with_timeout( - &reader, - &child, - init_id, - Duration::from_secs(connect_timeout), - read_timeout, - ) { - if let Ok(mut child_guard) = child.lock() { - let _ = child_guard.kill(); - } - return Err(e); - } - let initialized_payload = serde_json::json!({ - "jsonrpc": "2.0", - "method": "initialized", - "params": {} - }); - send_request_sync(&mut stdin, &initialized_payload)?; - - let call_id = next_id(); - let call_payload = serde_json::json!({ - "jsonrpc": "2.0", - "id": call_id, - "method": "tools/call", - "params": { - "name": tool, - "arguments": args - } - }); - send_request_sync(&mut stdin, &call_payload)?; - let response = match read_response_with_timeout( - &reader, - &child, - call_id, - Duration::from_secs(execute_timeout), - read_timeout, - ) { - Ok(result) => result, - Err(e) => { - if let Ok(mut child_guard) = child.lock() { - let _ = child_guard.kill(); - } - return Err(e); - } - }; - - if let Ok(mut child_guard) = child.lock() { - let _ = child_guard.kill(); - } - - if let Some(result) = response.get("result") { - return Ok(serde_json::to_string_pretty(result)?); - } - if let Some(error) = response.get("error") { - return Ok(serde_json::to_string_pretty(error)?); - } - Ok(serde_json::to_string_pretty(&response)?) -} - -#[allow(dead_code)] // Legacy sync API -fn load_legacy(path: &Path) -> Result { - if path.exists() { - let contents = fs::read_to_string(path) - .with_context(|| format!("Failed to read {}", path.display()))?; - let config = serde_json::from_str(&contents) - .with_context(|| format!("Failed to parse {}", path.display()))?; - Ok(config) - } else { - Ok(LegacyMcpConfig::default()) - } -} - -#[allow(dead_code)] // Legacy sync API -fn save_legacy(path: &Path, config: &LegacyMcpConfig) -> Result<()> { - if let Some(parent) = path.parent() { - fs::create_dir_all(parent)?; - } - let contents = serde_json::to_string_pretty(config)?; - write_atomic(path, contents.as_bytes())?; - Ok(()) -} - -#[allow(dead_code)] // Legacy sync API -fn parse_env(items: &[String]) -> Result> { - let mut env = HashMap::new(); - for item in items { - let parts: Vec<&str> = item.splitn(2, '=').collect(); - if parts.len() != 2 { - anyhow::bail!("Failed to parse MCP env var '{item}': expected KEY=VALUE"); - } - env.insert(parts[0].to_string(), parts[1].to_string()); - } - Ok(env) -} - -#[allow(dead_code)] // Legacy sync API -fn send_request_sync(stdin: &mut impl Write, payload: &serde_json::Value) -> Result<()> { - let line = serde_json::to_string(payload)?; - stdin - .write_all(format!("{line}\n").as_bytes()) - .with_context(|| "Failed to write MCP request")?; - stdin.flush()?; - Ok(()) -} - -#[allow(dead_code)] // Legacy sync API -fn read_response_with_timeout( - reader: &Arc>>, - child: &Arc>, - id: u64, - timeout: Duration, - read_timeout: u64, -) -> Result { - let effective_timeout = Duration::from_secs(timeout.as_secs().min(read_timeout)); - let (tx, rx) = std::sync::mpsc::channel(); - - let reader_clone = Arc::clone(reader); - std::thread::spawn(move || { - let result = read_response_sync(&reader_clone, id); - let _ = tx.send(result); - }); - - if let Ok(result) = rx.recv_timeout(effective_timeout) { - result - } else { - if let Ok(mut child_guard) = child.lock() { - let _ = child_guard.kill(); - } - anyhow::bail!( - "Failed to read MCP response: timed out after {}s", - effective_timeout.as_secs() - ) - } -} - -#[allow(dead_code)] // Legacy sync API -fn read_response_sync( - reader: &Arc>>, - id: u64, -) -> Result { - let mut line = String::new(); - loop { - line.clear(); - let read = { - let mut guard = reader - .lock() - .map_err(|_| anyhow::anyhow!("MCP reader lock poisoned"))?; - guard.read_line(&mut line)? - }; - if read == 0 { - anyhow::bail!("Failed to read MCP response: server closed output before responding."); - } - let trimmed = line.trim(); - if trimmed.is_empty() { - continue; - } - if let Ok(value) = serde_json::from_str::(trimmed) - && value.get("id").and_then(serde_json::Value::as_u64) == Some(id) - { - return Ok(value); - } - } -} - -#[allow(dead_code)] // Legacy sync API -fn next_id() -> u64 { - let micros = SystemTime::now() - .duration_since(UNIX_EPOCH) - .unwrap_or_default() - .as_micros(); - u64::try_from(micros).unwrap_or(u64::MAX) -} - // === Unit Tests === #[cfg(test)] @@ -2253,33 +1940,6 @@ mod tests { assert!(formatted.contains("[image content]")); } - #[test] - #[cfg(unix)] - fn test_read_response_timeout_kills_child() { - let mut child = Command::new("sh") - .arg("-c") - .arg("sleep 5") - .stdout(Stdio::piped()) - .spawn() - .expect("spawn sleep"); - let stdout = child.stdout.take().expect("stdout"); - let reader = Arc::new(Mutex::new(BufReader::new(stdout))); - let child = Arc::new(Mutex::new(child)); - - let result = read_response_with_timeout(&reader, &child, 1, Duration::from_secs(1), 1); - - assert!(result.is_err()); - let err = result.unwrap_err().to_string(); - assert!(err.contains("timed out")); - - let status = child - .lock() - .expect("lock child") - .wait() - .expect("wait child"); - assert!(!status.success()); - } - #[tokio::test] async fn test_mcp_pool_empty_config() { let pool = McpPool::new(McpConfig::default());