From 40a057f10c2199699fd674c943a9f2b0f9b84cee Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 11 May 2026 08:41:16 -0400 Subject: [PATCH] test(cli): cover fabro mcp server contract --- lib/crates/fabro-cli/src/commands/mcp/mod.rs | 4 ++- lib/crates/fabro-cli/tests/it/cmd/fabro.rs | 1 + lib/crates/fabro-cli/tests/it/cmd/mcp.rs | 32 ++++++++++++++++++++ lib/crates/fabro-mcp/src/client.rs | 30 ++++++++++++++++++ 4 files changed, 66 insertions(+), 1 deletion(-) diff --git a/lib/crates/fabro-cli/src/commands/mcp/mod.rs b/lib/crates/fabro-cli/src/commands/mcp/mod.rs index a7df70073..d7a82a7cc 100644 --- a/lib/crates/fabro-cli/src/commands/mcp/mod.rs +++ b/lib/crates/fabro-cli/src/commands/mcp/mod.rs @@ -1,3 +1,5 @@ +use std::fmt::Write as _; + use anyhow::{Context as _, Result}; use crate::args::{McpAgent, McpCommand, McpNamespace, ServerConnectionArgs}; @@ -10,7 +12,7 @@ pub(crate) async fn dispatch(ns: McpNamespace, base_ctx: &CommandContext) -> Res } McpCommand::Config(args) => { let json = fabro_mcp_server::config_json(&config_settings(&args.connection))?; - print!("{json}"); + let _ = write!(base_ctx.printer().stdout_important(), "{json}"); Ok(()) } McpCommand::Init(args) => { diff --git a/lib/crates/fabro-cli/tests/it/cmd/fabro.rs b/lib/crates/fabro-cli/tests/it/cmd/fabro.rs index ff151778f..6fc7aaff8 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/fabro.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/fabro.rs @@ -33,6 +33,7 @@ fn help() { archive Mark terminal runs as archived (reviewed, no further action needed). Archived runs are hidden from default listings unarchive Restore archived runs to their prior terminal status model List and test LLM models + mcp Model Context Protocol server server Server operations doctor Check environment and integration health version Show client and server version information diff --git a/lib/crates/fabro-cli/tests/it/cmd/mcp.rs b/lib/crates/fabro-cli/tests/it/cmd/mcp.rs index 9ede4bc43..1a2274f23 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/mcp.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/mcp.rs @@ -367,6 +367,10 @@ async fn stdio_server_initializes_and_lists_run_tools() { "tool should have input schema: {schema}" ); } + client + .shutdown() + .await + .expect("MCP client should shut down"); } #[test] @@ -417,6 +421,10 @@ async fn stdio_startup_and_list_tools_is_fast() { let tools = client.list_tools().await.unwrap(); assert_eq!(tools.len(), 5); assert!(start.elapsed() < std::time::Duration::from_secs(2)); + client + .shutdown() + .await + .expect("MCP client should shut down"); } #[tokio::test(flavor = "multi_thread")] @@ -481,6 +489,10 @@ async fn mcp_create_and_search_manage_real_runs_with_cli_auth() { } "#); + client + .shutdown() + .await + .expect("MCP client should shut down"); harness.shutdown().await; } @@ -589,6 +601,10 @@ async fn mcp_lifecycle_tools_manage_real_run() { "# ); + client + .shutdown() + .await + .expect("MCP client should shut down"); harness.shutdown().await; } @@ -609,6 +625,10 @@ async fn mcp_gather_rejects_too_many_runs() { assert!(error.contains("run_ids"), "{error}"); assert_eq!(client.list_tools().await.unwrap().len(), 5); + client + .shutdown() + .await + .expect("MCP client should shut down"); } #[tokio::test(flavor = "multi_thread")] @@ -639,6 +659,10 @@ async fn mcp_gather_returns_timeout_result() { assert!(start.elapsed() < std::time::Duration::from_secs(4)); assert_eq!(gather["runs"][0]["status"], "submitted"); + client + .shutdown() + .await + .expect("MCP client should shut down"); harness.shutdown().await; } @@ -656,6 +680,10 @@ async fn mcp_interact_error_does_not_stop_server() { assert!(error.contains("message"), "{error}"); assert_eq!(client.list_tools().await.unwrap().len(), 5); + client + .shutdown() + .await + .expect("MCP client should shut down"); } #[tokio::test(flavor = "multi_thread")] @@ -679,6 +707,10 @@ async fn mcp_tool_auth_error_mentions_login() { ); assert_eq!(client.list_tools().await.unwrap().len(), 5); + client + .shutdown() + .await + .expect("MCP client should shut down"); harness.shutdown().await; } diff --git a/lib/crates/fabro-mcp/src/client.rs b/lib/crates/fabro-mcp/src/client.rs index 1c4e537a6..7a3843a9c 100644 --- a/lib/crates/fabro-mcp/src/client.rs +++ b/lib/crates/fabro-mcp/src/client.rs @@ -22,6 +22,8 @@ enum ClientState { Connecting(Option), /// Handshake complete, ready for tool calls. Ready(Arc>), + /// Connection was explicitly closed. + Closed, } enum PendingTransport { @@ -116,6 +118,7 @@ impl McpClient { .take() .ok_or_else(|| anyhow!("client already initializing"))?, ClientState::Ready(_) => return Err(anyhow!("client already initialized")), + ClientState::Closed => return Err(anyhow!("MCP client is shut down")), }; // Drop the lock before the blocking handshake @@ -243,11 +246,38 @@ impl McpClient { Ok(result) } + pub async fn shutdown(self) -> Result<()> { + let service = { + let mut guard = self.state.lock().await; + match std::mem::replace(&mut *guard, ClientState::Closed) { + ClientState::Connecting(_) | ClientState::Closed => None, + ClientState::Ready(service) => Some(service), + } + }; + + if let Some(service) = service { + match Arc::try_unwrap(service) { + Ok(mut service) => { + service + .close_with_timeout(Duration::from_secs(2)) + .await + .context("failed to shut down MCP client")?; + } + Err(service) => { + service.cancellation_token().cancel(); + } + } + } + + Ok(()) + } + async fn service(&self) -> Result>> { let guard = self.state.lock().await; match &*guard { ClientState::Ready(service) => Ok(Arc::clone(service)), ClientState::Connecting(_) => Err(anyhow!("MCP client not initialized")), + ClientState::Closed => Err(anyhow!("MCP client is shut down")), } } }