fix(mcp): honor inline enabled=false and per-server tool_timeout (#520)

## What

Two latent fixes to MCP server config handling, independent of any new
feature:

1. **`enabled = false` is now honored for inline MCP servers.** Entries
under `[run.agent.mcps.*]` and `[cli.exec.agent.mcps.*]` accepted an
`enabled` flag that resolution silently ignored, so a disabled server
still started. Disabled entries are now dropped from the resolved set.
Absent `enabled` still means enabled.
2. **Explicitly configured empty `cli.exec.agent.mcps` sets are
preserved.** If every `cli.exec` MCP entry is disabled, `fabro exec` now
treats that as an intentional empty override instead of falling back to
`run.agent.mcps`.
3. **Per-server `tool_timeout_secs` now applies to MCP tool calls.** The
value was carried through config but never reached the call path. The
connection manager now owns each server timeout and applies it when
calling tools.

## Testing

- New and updated tests cover StickyMap same-key replacement across
layers, `enabled = false` skipped for run and `cli.exec`, absent
`enabled` kept, higher-layer disable shadowing, explicit empty
`cli.exec` MCP overrides, and configured tool timeout behavior.
- `cargo +nightly-2026-04-14 fmt --check --all`
- `cargo nextest run -p fabro-config -p fabro-agent -p fabro-mcp`: 737
passed, 93 skipped.
- `cargo +nightly-2026-04-14 clippy -p fabro-config -p fabro-agent -p
fabro-mcp -p fabro-cli --all-targets -- -D warnings`
- `cargo test --locked -p fabro-workflow --test it --no-run`

## Notes

- **Behavior change** worth a changelog entry: disabled inline MCPs are
now actually disabled, explicit empty `cli.exec` MCP overrides are
respected, and per-server tool timeouts now take effect.
- First of a short series adding server-managed MCP servers; this PR is
self-contained and independent of the others.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

lib/crates/fabro-agent/src/mcp_integration.rs

@@ -16,7 +16,6 @@ pub fn make_mcp_tools(manager: &Arc<McpConnectionManager>) -> Vec<RegisteredTool
             let name = qualified_name.clone();
             let server_name = info.server_name.clone();
             let original_name = info.original_tool_name.clone();
-            let tool_timeout = std::time::Duration::from_mins(2);
 
             RegisteredTool {
                 definition: ToolDefinition {
@@ -27,10 +26,9 @@ pub fn make_mcp_tools(manager: &Arc<McpConnectionManager>) -> Vec<RegisteredTool
                 executor:   Arc::new(move |args, _ctx| {
                     let mgr = Arc::clone(&mgr);
                     let name = name.clone();
-                    let timeout = tool_timeout;
                     Box::pin(async move {
                         let result = mgr
-                            .call_tool(&name, args, timeout)
+                            .call_tool(&name, args)
                             .await
                             .map_err(|e| e.to_string())?;
                         call_result_to_string(&result)

lib/crates/fabro-cli/src/commands/exec.rs

@@ -313,12 +313,12 @@ pub(crate) async fn execute(mut args: ExecArgs, ctx: &CommandContext) -> 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<McpServerSettings> = if cli.exec.agent.mcps.is_empty() {
-        ctx.run_settings()
+    let mcp_servers: Vec<McpServerSettings> = 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

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.

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))
+            }),
         },
     }
 }

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<McpEntryLayer>,
+) -> HashMap<String, McpServerSettings> {
+    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 \

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());
+}

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::<SettingsLayer>()
+            .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"
+        );
+    }
+}

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<McpClient>,
+    tool_timeout: Duration,
+}
+
 /// Manages connections to multiple MCP servers and their tools.
 pub struct McpConnectionManager {
-    clients: HashMap<String, Arc<McpClient>>,
+    clients: HashMap<String, ServerConnection>,
     tools:   HashMap<String, ToolInfo>,
 }
 
@@ -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<CallToolResult> {
         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
     }
 }

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)]

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,

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<AgentPermissions>,
-    pub mcps:        HashMap<String, McpServerSettings>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub mcps:        Option<HashMap<String, McpServerSettings>>,
 }
 
 #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)]

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