diff --git a/lib/crates/fabro-agent/src/mcp_integration.rs b/lib/crates/fabro-agent/src/mcp_integration.rs index 65787f448..fe766562e 100644 --- a/lib/crates/fabro-agent/src/mcp_integration.rs +++ b/lib/crates/fabro-agent/src/mcp_integration.rs @@ -16,7 +16,6 @@ pub fn make_mcp_tools(manager: &Arc) -> Vec) -> Vec AnyResu // v2 MCPs live under `cli.exec.agent.mcps` (owner-specific) or // `run.agent.mcps`. For `fabro exec` we use the cli.exec path, falling // back to run.agent.mcps if unset. - let mcp_servers: Vec = if cli.exec.agent.mcps.is_empty() { - ctx.run_settings() + let mcp_servers: Vec = match cli.exec.agent.mcps.as_ref() { + Some(mcps) => mcps.values().cloned().collect(), + None => ctx + .run_settings() .map(|settings| settings.agent.mcps.values().cloned().collect()) - .unwrap_or_default() - } else { - cli.exec.agent.mcps.values().cloned().collect() + .unwrap_or_default(), }; // Resolve `{{ env.* }}` in MCP transport config at the exec boundary, // against the CLI process env — the mirror of the `fabro run` worker diff --git a/lib/crates/fabro-config/src/layers/run.rs b/lib/crates/fabro-config/src/layers/run.rs index af35ce44b..cc849fb23 100644 --- a/lib/crates/fabro-config/src/layers/run.rs +++ b/lib/crates/fabro-config/src/layers/run.rs @@ -467,6 +467,20 @@ pub enum McpEntryLayer { }, } +impl McpEntryLayer { + /// Whether this entry should be activated. Absent `enabled` defaults to + /// `true`; only an explicit `enabled = false` disables the entry. + #[must_use] + pub fn is_enabled(&self) -> bool { + let enabled = match self { + Self::Http { enabled, .. } + | Self::Stdio { enabled, .. } + | Self::Sandbox { enabled, .. } => enabled, + }; + enabled.unwrap_or(true) + } +} + /// A run hook entry. Exactly one of `script`, `command`, `url`, `prompt`, or /// `agent` fields determines the hook behavior. The `id` field, when set, is /// used for cross-layer replace-by-id merging. diff --git a/lib/crates/fabro-config/src/resolve/cli.rs b/lib/crates/fabro-config/src/resolve/cli.rs index ad6c63dc6..0c555b2ba 100644 --- a/lib/crates/fabro-config/src/resolve/cli.rs +++ b/lib/crates/fabro-config/src/resolve/cli.rs @@ -85,22 +85,9 @@ fn resolve_exec(exec: Option<&CliExecLayer>) -> CliExecSettings { }, agent: CliExecAgentSettings { permissions: exec.agent.as_ref().and_then(|agent| agent.permissions), - mcps: exec - .agent - .as_ref() - .map(|agent| { - agent - .mcps - .iter() - .map(|(name, entry)| { - ( - name.clone(), - super::run::resolve_mcp_entry(name.as_str(), entry), - ) - }) - .collect() - }) - .unwrap_or_default(), + mcps: exec.agent.as_ref().and_then(|agent| { + (!agent.mcps.is_empty()).then(|| super::run::resolve_enabled_mcps(&agent.mcps)) + }), }, } } diff --git a/lib/crates/fabro-config/src/resolve/run.rs b/lib/crates/fabro-config/src/resolve/run.rs index 00858fc97..2feb72897 100644 --- a/lib/crates/fabro-config/src/resolve/run.rs +++ b/lib/crates/fabro-config/src/resolve/run.rs @@ -1,3 +1,5 @@ +use std::collections::HashMap; + use fabro_types::settings::InterpString; use fabro_types::settings::run::{ ArtifactsSettings, GitAuthorSettings, HookDefinition, HookType, InterviewProviderSettings, @@ -16,7 +18,7 @@ use crate::{ NotificationRouteLayer, RunAgentLayer, RunArtifactsLayer, RunCheckpointLayer, RunCloneLayer, RunExecutionLayer, RunGitLayer, RunGoalLayer, RunIntegrationsLayer, RunLayer, RunMetaBranchLayer, RunModelLayer, RunPrepareLayer, RunPullRequestLayer, RunRunBranchLayer, - RunScmLayer, StringOrSplice, + RunScmLayer, StickyMap, StringOrSplice, }; pub fn resolve_run( @@ -286,14 +288,23 @@ fn resolve_agent(agent: Option<&RunAgentLayer>) -> RunAgentSettings { RunAgentSettings { fabro_tools: agent.fabro_tools.unwrap_or(false), permissions: agent.permissions, - mcps: agent - .mcps - .iter() - .map(|(name, entry)| (name.clone(), resolve_mcp_entry(name, entry))) - .collect(), + mcps: resolve_enabled_mcps(&agent.mcps), } } +/// Resolve an agent layer's inline MCP entries into runtime settings, dropping +/// any entry with an explicit `enabled = false`. Shared by the `run.agent` and +/// `cli.exec.agent` resolution paths so the enable check lives in one place and +/// any future inline-MCP site inherits it for free. +pub(crate) fn resolve_enabled_mcps( + mcps: &StickyMap, +) -> HashMap { + mcps.iter() + .filter(|(_, entry)| entry.is_enabled()) + .map(|(name, entry)| (name.clone(), resolve_mcp_entry(name, entry))) + .collect() +} + #[expect( clippy::disallowed_methods, reason = "intentional source preservation: MCP transport strings are carried in source form \ diff --git a/lib/crates/fabro-config/src/tests/resolve_cli.rs b/lib/crates/fabro-config/src/tests/resolve_cli.rs index 550da9271..58cfe8cb6 100644 --- a/lib/crates/fabro-config/src/tests/resolve_cli.rs +++ b/lib/crates/fabro-config/src/tests/resolve_cli.rs @@ -126,9 +126,64 @@ level = "debug" assert_eq!(cli.exec.model.provider.as_deref(), Some("openai")); assert_eq!(cli.exec.model.name.as_deref(), Some("gpt-5")); assert_eq!(cli.exec.agent.permissions, Some(AgentPermissions::ReadOnly)); - assert_eq!(cli.exec.agent.mcps["fs"].name, "fs"); + assert_eq!(cli.exec.agent.mcps.as_ref().unwrap()["fs"].name, "fs"); assert_eq!(cli.output.format, OutputFormat::Json); assert_eq!(cli.output.verbosity, OutputVerbosity::Verbose); assert!(!cli.updates.check); assert_eq!(cli.logging.level.as_deref(), Some("debug")); } + +#[test] +fn cli_exec_inline_mcp_with_enabled_false_is_skipped() { + let cli = UserSettingsBuilder::from_toml( + r#" +_version = 1 + +[cli.exec.agent.mcps.fs] +type = "stdio" +command = ["echo", "cli"] + +[cli.exec.agent.mcps.disabled] +type = "stdio" +enabled = false +command = ["never-launched"] +"#, + ) + .expect("cli settings should resolve") + .cli; + + let mcps = cli + .exec + .agent + .mcps + .as_ref() + .expect("cli MCP table should be marked configured"); + assert!(mcps.contains_key("fs")); + assert!( + !mcps.contains_key("disabled"), + "explicit `enabled = false` should drop the inline cli.exec MCP entry" + ); +} + +#[test] +fn cli_exec_all_disabled_mcps_preserves_configured_empty_set() { + let cli = UserSettingsBuilder::from_toml( + r#" +_version = 1 + +[cli.exec.agent.mcps.disabled] +type = "stdio" +enabled = false +command = ["never-launched"] +"#, + ) + .expect("cli settings should resolve") + .cli; + + let mcps = cli + .exec + .agent + .mcps + .expect("cli MCP table should be marked configured"); + assert!(mcps.is_empty()); +} diff --git a/lib/crates/fabro-config/src/tests/resolve_run.rs b/lib/crates/fabro-config/src/tests/resolve_run.rs index 7ff4b9b3e..5e8f547d5 100644 --- a/lib/crates/fabro-config/src/tests/resolve_run.rs +++ b/lib/crates/fabro-config/src/tests/resolve_run.rs @@ -1012,3 +1012,157 @@ exclude_globs = ["**/lower/**"] assert!(settings.checkpoint.skip_git_hooks); } } + +mod run_agent_mcps { + //! Layer + resolver tests for `[run.agent.mcps]`: same-key replacement + //! across layers (`StickyMap`) and honoring `enabled = false`. + + use fabro_types::settings::run::McpTransport; + + use crate::SettingsLayer; + use crate::layers::Combine; + + fn parse_settings(source: &str) -> SettingsLayer { + source + .parse::() + .expect("fixture should parse via SettingsLayer") + } + + fn stdio_command(transport: &McpTransport) -> &[String] { + match transport { + McpTransport::Stdio { command, .. } => command, + other => panic!("expected stdio transport, got {other:?}"), + } + } + + #[test] + fn higher_layer_replaces_same_key_mcp_entry() { + // Both layers define `[run.agent.mcps.fs]`; the higher (workflow) + // layer's entry must win wholesale via StickyMap same-key replacement. + let workflow = parse_settings( + r#" +_version = 1 + +[run.agent.mcps.fs] +type = "stdio" +command = ["fs-server", "--workflow"] +"#, + ); + let user = parse_settings( + r#" +_version = 1 + +[run.agent.mcps.fs] +type = "stdio" +command = ["fs-server", "--user"] + +[run.agent.mcps.extra] +type = "stdio" +command = ["extra-server"] +"#, + ); + let merged = workflow.combine(user); + + let mcps = super::workflow_settings_from_layer(merged) + .expect("merged settings should resolve") + .run + .agent + .mcps; + + // Same-key `fs` is replaced by the higher layer; different-key `extra` + // is additive and inherited from the lower layer. + assert_eq!(stdio_command(&mcps["fs"].transport), &[ + "fs-server".to_string(), + "--workflow".to_string() + ],); + assert!(mcps.contains_key("extra")); + } + + #[test] + fn inline_entry_with_enabled_false_is_skipped() { + let mcps = super::workflow_settings_from_toml( + r#" +_version = 1 + +[run.agent.mcps.fs] +type = "stdio" +command = ["fs-server"] + +[run.agent.mcps.disabled] +type = "stdio" +enabled = false +command = ["never-launched"] +"#, + ) + .expect("settings should resolve") + .run + .agent + .mcps; + + assert!(mcps.contains_key("fs")); + assert!( + !mcps.contains_key("disabled"), + "explicit `enabled = false` should drop the inline MCP entry" + ); + } + + #[test] + fn absent_enabled_keeps_inline_entry() { + let mcps = super::workflow_settings_from_toml( + r#" +_version = 1 + +[run.agent.mcps.fs] +type = "stdio" +command = ["fs-server"] +"#, + ) + .expect("settings should resolve") + .run + .agent + .mcps; + + assert!( + mcps.contains_key("fs"), + "an entry without `enabled` defaults to enabled" + ); + } + + #[test] + fn higher_layer_disable_shadows_lower_layer_entry() { + // Lower layer enables `fs`; higher layer redefines the same key with + // `enabled = false`. StickyMap replacement means the disabled entry + // wins and the server is dropped from the resolved map. + let workflow = parse_settings( + r#" +_version = 1 + +[run.agent.mcps.fs] +type = "stdio" +enabled = false +command = ["fs-server"] +"#, + ); + let user = parse_settings( + r#" +_version = 1 + +[run.agent.mcps.fs] +type = "stdio" +command = ["fs-server"] +"#, + ); + let merged = workflow.combine(user); + + let mcps = super::workflow_settings_from_layer(merged) + .expect("merged settings should resolve") + .run + .agent + .mcps; + + assert!( + !mcps.contains_key("fs"), + "a higher-layer `enabled = false` should shadow and disable the lower-layer entry" + ); + } +} diff --git a/lib/crates/fabro-mcp/src/connection_manager.rs b/lib/crates/fabro-mcp/src/connection_manager.rs index 3eff94b30..fa62865c8 100644 --- a/lib/crates/fabro-mcp/src/connection_manager.rs +++ b/lib/crates/fabro-mcp/src/connection_manager.rs @@ -84,9 +84,14 @@ pub struct ToolInfo { pub input_schema: serde_json::Value, } +struct ServerConnection { + client: Arc, + tool_timeout: Duration, +} + /// Manages connections to multiple MCP servers and their tools. pub struct McpConnectionManager { - clients: HashMap>, + clients: HashMap, tools: HashMap, } @@ -140,7 +145,10 @@ impl McpConnectionManager { }); } - self.clients.insert(config.name.clone(), Arc::new(client)); + self.clients.insert(config.name.clone(), ServerConnection { + client: Arc::new(client), + tool_timeout: config.tool_timeout(), + }); Ok(tool_count) } @@ -172,20 +180,20 @@ impl McpConnectionManager { &self, qualified_name: &str, arguments: serde_json::Value, - timeout: Duration, ) -> Result { let info = self .tools .get(qualified_name) .ok_or_else(|| anyhow::anyhow!("unknown MCP tool: {qualified_name}"))?; - let client = self + let connection = self .clients .get(&info.server_name) .ok_or_else(|| anyhow::anyhow!("no client for MCP server: {}", info.server_name))?; - client - .call_tool(&info.original_tool_name, arguments, timeout) + connection + .client + .call_tool(&info.original_tool_name, arguments, connection.tool_timeout) .await } } diff --git a/lib/crates/fabro-mcp/tests/stdio_integration.rs b/lib/crates/fabro-mcp/tests/stdio_integration.rs index e07d81aa6..843ea4187 100644 --- a/lib/crates/fabro-mcp/tests/stdio_integration.rs +++ b/lib/crates/fabro-mcp/tests/stdio_integration.rs @@ -155,7 +155,6 @@ async fn connection_manager_stdio_roundtrip() { .call_tool( "mcp__test_echo__echo", serde_json::json!({"message": "roundtrip"}), - Duration::from_secs(10), ) .await .unwrap(); @@ -164,6 +163,34 @@ async fn connection_manager_stdio_roundtrip() { assert_eq!(text, "roundtrip"); } +#[tokio::test] +async fn connection_manager_call_tool_uses_configured_tool_timeout() { + let mut config = test_server_config(); + config.tool_timeout_secs = 1; + + let mut mgr = McpConnectionManager::new(); + let results = mgr.start_servers(&[config]).await; + assert_eq!(results.len(), 1); + assert!( + results[0].1.is_ok(), + "server should start: {:?}", + results[0] + ); + + let err = mgr + .call_tool( + "mcp__test_echo__echo", + serde_json::json!({"message": "__sleep_ms:1500__"}), + ) + .await + .unwrap_err(); + assert!( + err.to_string() + .contains("timed out calling tool 'echo' on MCP server 'test-echo'"), + "unexpected error: {err}" + ); +} + #[tokio::test] async fn sse_client_initialize_and_call_tool() { #[derive(Clone)] diff --git a/lib/crates/fabro-mcp/tests/test_mcp_server.py b/lib/crates/fabro-mcp/tests/test_mcp_server.py index 63f95b689..c3dd8838d 100644 --- a/lib/crates/fabro-mcp/tests/test_mcp_server.py +++ b/lib/crates/fabro-mcp/tests/test_mcp_server.py @@ -7,6 +7,7 @@ Exposes a single tool: echo(message) -> message. import json import os import sys +import time SERVER_INFO = { "name": "test-echo-server", @@ -59,6 +60,10 @@ def handle_request(req): elif msg.startswith("__env:") and msg.endswith("__"): key = msg[len("__env:") : -len("__")] msg = os.environ.get(key, "") + elif msg.startswith("__sleep_ms:") and msg.endswith("__"): + milliseconds = int(msg[len("__sleep_ms:") : -len("__")]) + time.sleep(milliseconds / 1000) + msg = f"slept {milliseconds}ms" return { "jsonrpc": "2.0", "id": req_id, diff --git a/lib/crates/fabro-types/src/settings/cli.rs b/lib/crates/fabro-types/src/settings/cli.rs index 0d97d7b89..38a5dae63 100644 --- a/lib/crates/fabro-types/src/settings/cli.rs +++ b/lib/crates/fabro-types/src/settings/cli.rs @@ -50,7 +50,8 @@ pub struct CliExecModelSettings { #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] pub struct CliExecAgentSettings { pub permissions: Option, - pub mcps: HashMap, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub mcps: Option>, } #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] diff --git a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs index 346825e7e..692911491 100644 --- a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs @@ -2111,13 +2111,7 @@ async fn daytona_playwright_mcp_sandbox_transport() { .find(|k| k.ends_with("browser_install")) .expect("no browser_install tool found"); eprintln!("Calling tool: {install_tool}"); - let install_result = manager - .call_tool( - install_tool, - serde_json::json!({}), - std::time::Duration::from_mins(2), - ) - .await; + let install_result = manager.call_tool(install_tool, serde_json::json!({})).await; match &install_result { Ok(result) => eprintln!( "Install result: {}", @@ -2137,11 +2131,7 @@ async fn daytona_playwright_mcp_sandbox_transport() { .expect("no browser_navigate tool found"); eprintln!("Calling tool: {nav_tool}"); let nav_result = manager - .call_tool( - nav_tool, - serde_json::json!({"url": "https://example.com"}), - std::time::Duration::from_secs(30), - ) + .call_tool(nav_tool, serde_json::json!({"url": "https://example.com"})) .await; match &nav_result { Ok(result) => eprintln!( @@ -2162,13 +2152,7 @@ async fn daytona_playwright_mcp_sandbox_transport() { .find(|k| k.contains("snapshot")) .expect("no snapshot tool found"); eprintln!("Calling tool: {snap_tool}"); - let snap_result = manager - .call_tool( - snap_tool, - serde_json::json!({}), - std::time::Duration::from_secs(30), - ) - .await; + let snap_result = manager.call_tool(snap_tool, serde_json::json!({})).await; match &snap_result { Ok(result) => { let text = result