From 38dacb8744babb430f0bb38f180f32c056db2ffd Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 9 Apr 2026 18:12:00 -0400 Subject: [PATCH] refactor(settings): stage 6.3b promote mcp runtime types into fabro-mcp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moves `McpServerEntry`, `McpServerSettings`, `McpTransport`, plus the `default_startup_timeout_secs` / `default_tool_timeout_secs` helpers from `fabro-types/src/settings/mcp.rs` into `fabro-mcp/src/config.rs`. fabro-mcp was already the only crate that re-exported them, so this deletes the `fabro-types` module entirely and drops the `pub use mcp::*` re-export from `settings/mod.rs`. `bridge_mcps` / `bridge_mcp_entry` (v2 `McpEntryLayer` → runtime `McpServerEntry` converters) also move to `fabro-mcp/src/config.rs`. `fabro-workflow::operations::start` and `fabro-cli::commands::exec` now import `bridge_mcp_entry` from `fabro_mcp::config::bridge_mcp_entry` instead of the v2 `to_runtime` module. Four of the seven legacy runtime type modules are now gone; three remain (run, sandbox, server). The `to_runtime.rs` module is down to just sandbox, pull-request, merge-strategy, artifacts, and worktree-mode helpers. 3,758 workspace tests pass. `cargo fmt --check --all` and `cargo clippy --workspace -- -D warnings` are clean. Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-cli/src/commands/exec.rs | 3 +- lib/crates/fabro-mcp/src/config.rs | 285 +++++++++++++++++- lib/crates/fabro-types/src/settings/mcp.rs | 186 ------------ lib/crates/fabro-types/src/settings/mod.rs | 5 - .../fabro-types/src/settings/v2/to_runtime.rs | 109 +------ .../fabro-workflow/src/operations/start.rs | 3 +- 6 files changed, 293 insertions(+), 298 deletions(-) delete mode 100644 lib/crates/fabro-types/src/settings/mcp.rs diff --git a/lib/crates/fabro-cli/src/commands/exec.rs b/lib/crates/fabro-cli/src/commands/exec.rs index 04d6fa619..47d1d10a1 100644 --- a/lib/crates/fabro-cli/src/commands/exec.rs +++ b/lib/crates/fabro-cli/src/commands/exec.rs @@ -2,9 +2,8 @@ use anyhow::Result; use fabro_agent::cli::{OutputFormat, run_with_args, run_with_args_and_client}; use fabro_llm::client::Client; use fabro_llm::providers::FabroServerAdapter; -use fabro_mcp::config::McpServerSettings; +use fabro_mcp::config::{McpServerSettings, bridge_mcp_entry}; use fabro_types::settings::v2::InterpString; -use fabro_types::settings::v2::to_runtime::bridge_mcp_entry; use std::collections::HashMap; use std::sync::Arc; diff --git a/lib/crates/fabro-mcp/src/config.rs b/lib/crates/fabro-mcp/src/config.rs index feed3623f..7787e5436 100644 --- a/lib/crates/fabro-mcp/src/config.rs +++ b/lib/crates/fabro-mcp/src/config.rs @@ -1,4 +1,281 @@ -pub use fabro_types::settings::mcp::{ - McpServerEntry, McpServerSettings, McpTransport, default_startup_timeout_secs, - default_tool_timeout_secs, -}; +//! MCP server configuration runtime types. +//! +//! The v2 parse tree lives in `fabro_types::settings::v2::run::McpEntryLayer`. +//! This module owns the runtime shape (flattened, with timeout helpers) that +//! the MCP client consumes at execution time. Conversion from the v2 shape +//! lives in [`bridge_mcp_entry`] / [`bridge_mcps`]. + +use std::collections::HashMap; +use std::time::Duration; + +use fabro_types::settings::v2::InterpString; +use fabro_types::settings::v2::run::McpEntryLayer; +use serde::{Deserialize, Serialize}; + +#[must_use] +pub fn default_startup_timeout_secs() -> u64 { + 10 +} + +#[must_use] +pub fn default_tool_timeout_secs() -> u64 { + 60 +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct McpServerSettings { + pub name: String, + pub transport: McpTransport, + #[serde(default = "default_startup_timeout_secs")] + pub startup_timeout_secs: u64, + #[serde(default = "default_tool_timeout_secs")] + pub tool_timeout_secs: u64, +} + +impl McpServerSettings { + #[must_use] + pub fn startup_timeout(&self) -> Duration { + Duration::from_secs(self.startup_timeout_secs) + } + + #[must_use] + pub fn tool_timeout(&self) -> Duration { + Duration::from_secs(self.tool_timeout_secs) + } +} + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(tag = "type", rename_all = "snake_case")] +pub enum McpTransport { + Stdio { + command: Vec, + #[serde(default)] + env: HashMap, + }, + Http { + url: String, + #[serde(default)] + headers: HashMap, + }, + /// MCP server that runs inside a sandbox and is accessed via HTTP preview URL. + /// During session init, the server is started inside the sandbox and this + /// variant is resolved into an `Http` transport using the sandbox's preview URL. + Sandbox { + command: Vec, + port: u16, + #[serde(default)] + env: HashMap, + }, +} + +/// MCP server entry as it appears in TOML config files (without a `name` field). +/// +/// Converted to [`McpServerSettings`] via [`McpServerEntry::into_config`]. +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +pub struct McpServerEntry { + #[serde(flatten)] + pub transport: McpTransport, + #[serde(default = "default_startup_timeout_secs")] + pub startup_timeout_secs: u64, + #[serde(default = "default_tool_timeout_secs")] + pub tool_timeout_secs: u64, +} + +impl McpServerEntry { + #[must_use] + pub fn into_config(self, name: String) -> McpServerSettings { + McpServerSettings { + name, + transport: self.transport, + startup_timeout_secs: self.startup_timeout_secs, + tool_timeout_secs: self.tool_timeout_secs, + } + } +} + +/// Convert a map of v2 `McpEntryLayer` entries into runtime `McpServerEntry`s. +#[must_use] +pub fn bridge_mcps(mcps: &HashMap) -> HashMap { + mcps.iter() + .map(|(name, entry)| (name.clone(), bridge_mcp_entry(entry))) + .collect() +} + +/// Convert a single v2 `McpEntryLayer` into the runtime `McpServerEntry`. +#[must_use] +pub fn bridge_mcp_entry(entry: &McpEntryLayer) -> McpServerEntry { + let transport = match entry { + McpEntryLayer::Stdio { + script, + command, + env, + .. + } => { + let command_vec: Vec = if let Some(script) = script { + vec!["sh".into(), "-c".into(), interp_to_string(script)] + } else if let Some(command) = command { + command.iter().map(interp_to_string).collect() + } else { + Vec::new() + }; + McpTransport::Stdio { + command: command_vec, + env: env + .iter() + .map(|(k, v)| (k.clone(), interp_to_string(v))) + .collect(), + } + } + McpEntryLayer::Http { url, headers, .. } => McpTransport::Http { + url: interp_to_string(url), + headers: headers + .iter() + .map(|(k, v)| (k.clone(), interp_to_string(v))) + .collect(), + }, + McpEntryLayer::Sandbox { + script, + command, + port, + env, + .. + } => { + let command_vec: Vec = if let Some(script) = script { + vec!["sh".into(), "-c".into(), interp_to_string(script)] + } else if let Some(command) = command { + command.iter().map(interp_to_string).collect() + } else { + Vec::new() + }; + McpTransport::Sandbox { + command: command_vec, + port: *port, + env: env + .iter() + .map(|(k, v)| (k.clone(), interp_to_string(v))) + .collect(), + } + } + }; + + let (startup_secs, tool_secs) = match entry { + McpEntryLayer::Http { + startup_timeout, + tool_timeout, + .. + } + | McpEntryLayer::Stdio { + startup_timeout, + tool_timeout, + .. + } + | McpEntryLayer::Sandbox { + startup_timeout, + tool_timeout, + .. + } => ( + startup_timeout.map_or(default_startup_timeout_secs(), |d| d.as_std().as_secs()), + tool_timeout.map_or(default_tool_timeout_secs(), |d| d.as_std().as_secs()), + ), + }; + + McpServerEntry { + transport, + startup_timeout_secs: startup_secs, + tool_timeout_secs: tool_secs, + } +} + +fn interp_to_string(value: &InterpString) -> String { + value.as_source() +} + +#[cfg(test)] +mod tests { + use super::*; + use std::collections::HashMap; + + #[test] + fn stdio_config_construction() { + let config = McpServerSettings { + name: "test-server".into(), + transport: McpTransport::Stdio { + command: vec![ + "npx".into(), + "-y".into(), + "@modelcontextprotocol/server-filesystem".into(), + ], + env: HashMap::new(), + }, + startup_timeout_secs: 10, + tool_timeout_secs: 60, + }; + assert_eq!(config.name, "test-server"); + assert_eq!(config.startup_timeout(), Duration::from_secs(10)); + assert_eq!(config.tool_timeout(), Duration::from_secs(60)); + } + + #[test] + fn http_config_construction() { + let config = McpServerSettings { + name: "remote-server".into(), + transport: McpTransport::Http { + url: "https://example.com/mcp".into(), + headers: HashMap::from([("Authorization".into(), "Bearer token".into())]), + }, + startup_timeout_secs: 30, + tool_timeout_secs: 60, + }; + assert_eq!(config.name, "remote-server"); + assert_eq!(config.startup_timeout(), Duration::from_secs(30)); + assert_eq!(config.tool_timeout(), Duration::from_secs(60)); + } + + #[test] + fn serde_round_trip_stdio() { + let config = McpServerSettings { + name: "fs".into(), + transport: McpTransport::Stdio { + command: vec!["node".into(), "server.js".into()], + env: HashMap::from([("NODE_ENV".into(), "production".into())]), + }, + startup_timeout_secs: 15, + tool_timeout_secs: 90, + }; + let json = serde_json::to_string(&config).unwrap(); + let deserialized: McpServerSettings = serde_json::from_str(&json).unwrap(); + assert_eq!(deserialized.name, "fs"); + assert_eq!(deserialized.startup_timeout_secs, 15); + assert_eq!(deserialized.tool_timeout_secs, 90); + assert!( + matches!(deserialized.transport, McpTransport::Stdio { command, .. } if command == vec!["node", "server.js"]) + ); + } + + #[test] + fn serde_round_trip_http() { + let config = McpServerSettings { + name: "remote".into(), + transport: McpTransport::Http { + url: "https://mcp.example.com".into(), + headers: HashMap::new(), + }, + startup_timeout_secs: 10, + tool_timeout_secs: 60, + }; + let json = serde_json::to_string(&config).unwrap(); + let deserialized: McpServerSettings = serde_json::from_str(&json).unwrap(); + assert_eq!(deserialized.name, "remote"); + assert!( + matches!(deserialized.transport, McpTransport::Http { url, .. } if url == "https://mcp.example.com") + ); + } + + #[test] + fn serde_defaults_applied() { + let json = r#"{"name":"minimal","transport":{"type":"stdio","command":["echo"]}}"#; + let config: McpServerSettings = serde_json::from_str(json).unwrap(); + assert_eq!(config.startup_timeout_secs, 10); + assert_eq!(config.tool_timeout_secs, 60); + } +} diff --git a/lib/crates/fabro-types/src/settings/mcp.rs b/lib/crates/fabro-types/src/settings/mcp.rs deleted file mode 100644 index 63d3bf5ac..000000000 --- a/lib/crates/fabro-types/src/settings/mcp.rs +++ /dev/null @@ -1,186 +0,0 @@ -use std::collections::HashMap; -use std::time::Duration; - -use serde::{Deserialize, Serialize}; - -use crate::combine::Combine; - -pub fn default_startup_timeout_secs() -> u64 { - 10 -} - -pub fn default_tool_timeout_secs() -> u64 { - 60 -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct McpServerSettings { - pub name: String, - pub transport: McpTransport, - #[serde(default = "default_startup_timeout_secs")] - pub startup_timeout_secs: u64, - #[serde(default = "default_tool_timeout_secs")] - pub tool_timeout_secs: u64, -} - -impl McpServerSettings { - #[must_use] - pub fn startup_timeout(&self) -> Duration { - Duration::from_secs(self.startup_timeout_secs) - } - - #[must_use] - pub fn tool_timeout(&self) -> Duration { - Duration::from_secs(self.tool_timeout_secs) - } -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -#[serde(tag = "type", rename_all = "snake_case")] -pub enum McpTransport { - Stdio { - command: Vec, - #[serde(default)] - env: HashMap, - }, - Http { - url: String, - #[serde(default)] - headers: HashMap, - }, - /// MCP server that runs inside a sandbox and is accessed via HTTP preview URL. - /// During session init, the server is started inside the sandbox and this - /// variant is resolved into an `Http` transport using the sandbox's preview URL. - Sandbox { - command: Vec, - port: u16, - #[serde(default)] - env: HashMap, - }, -} - -impl Combine for McpTransport { - fn combine(self, _other: Self) -> Self { - self - } -} - -/// MCP server entry as it appears in TOML config files (without a `name` field). -/// -/// Converted to [`McpServerSettings`] via [`McpServerEntry::into_config`]. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] -pub struct McpServerEntry { - #[serde(flatten)] - pub transport: McpTransport, - #[serde(default = "default_startup_timeout_secs")] - pub startup_timeout_secs: u64, - #[serde(default = "default_tool_timeout_secs")] - pub tool_timeout_secs: u64, -} - -impl McpServerEntry { - pub fn into_config(self, name: String) -> McpServerSettings { - McpServerSettings { - name, - transport: self.transport, - startup_timeout_secs: self.startup_timeout_secs, - tool_timeout_secs: self.tool_timeout_secs, - } - } -} - -impl Combine for McpServerEntry { - fn combine(self, _other: Self) -> Self { - self - } -} - -#[cfg(test)] -mod tests { - use super::*; - use std::collections::HashMap; - - #[test] - fn stdio_config_construction() { - let config = McpServerSettings { - name: "test-server".into(), - transport: McpTransport::Stdio { - command: vec![ - "npx".into(), - "-y".into(), - "@modelcontextprotocol/server-filesystem".into(), - ], - env: HashMap::new(), - }, - startup_timeout_secs: 10, - tool_timeout_secs: 60, - }; - assert_eq!(config.name, "test-server"); - assert_eq!(config.startup_timeout(), Duration::from_secs(10)); - assert_eq!(config.tool_timeout(), Duration::from_secs(60)); - } - - #[test] - fn http_config_construction() { - let config = McpServerSettings { - name: "remote-server".into(), - transport: McpTransport::Http { - url: "https://example.com/mcp".into(), - headers: HashMap::from([("Authorization".into(), "Bearer token".into())]), - }, - startup_timeout_secs: 30, - tool_timeout_secs: 60, - }; - assert_eq!(config.name, "remote-server"); - assert_eq!(config.startup_timeout(), Duration::from_secs(30)); - assert_eq!(config.tool_timeout(), Duration::from_secs(60)); - } - - #[test] - fn serde_round_trip_stdio() { - let config = McpServerSettings { - name: "fs".into(), - transport: McpTransport::Stdio { - command: vec!["node".into(), "server.js".into()], - env: HashMap::from([("NODE_ENV".into(), "production".into())]), - }, - startup_timeout_secs: 15, - tool_timeout_secs: 90, - }; - let json = serde_json::to_string(&config).unwrap(); - let deserialized: McpServerSettings = serde_json::from_str(&json).unwrap(); - assert_eq!(deserialized.name, "fs"); - assert_eq!(deserialized.startup_timeout_secs, 15); - assert_eq!(deserialized.tool_timeout_secs, 90); - assert!( - matches!(deserialized.transport, McpTransport::Stdio { command, .. } if command == vec!["node", "server.js"]) - ); - } - - #[test] - fn serde_round_trip_http() { - let config = McpServerSettings { - name: "remote".into(), - transport: McpTransport::Http { - url: "https://mcp.example.com".into(), - headers: HashMap::new(), - }, - startup_timeout_secs: 10, - tool_timeout_secs: 60, - }; - let json = serde_json::to_string(&config).unwrap(); - let deserialized: McpServerSettings = serde_json::from_str(&json).unwrap(); - assert_eq!(deserialized.name, "remote"); - assert!( - matches!(deserialized.transport, McpTransport::Http { url, .. } if url == "https://mcp.example.com") - ); - } - - #[test] - fn serde_defaults_applied() { - let json = r#"{"name":"minimal","transport":{"type":"stdio","command":["echo"]}}"#; - let config: McpServerSettings = serde_json::from_str(json).unwrap(); - assert_eq!(config.startup_timeout_secs, 10); - assert_eq!(config.tool_timeout_secs, 60); - } -} diff --git a/lib/crates/fabro-types/src/settings/mod.rs b/lib/crates/fabro-types/src/settings/mod.rs index 721371d01..383d65552 100644 --- a/lib/crates/fabro-types/src/settings/mod.rs +++ b/lib/crates/fabro-types/src/settings/mod.rs @@ -19,16 +19,11 @@ //! owning consumer crates or replace their call sites with v2-native //! accessors, at which point this module goes away. -pub mod mcp; pub mod run; pub mod sandbox; pub mod server; pub mod v2; -pub use mcp::{ - McpServerEntry, McpServerSettings, McpTransport, default_startup_timeout_secs, - default_tool_timeout_secs, -}; pub use run::{ ArtifactsSettings, CheckpointSettings, GitHubSettings, LlmSettings, MergeStrategy, PullRequestSettings, SetupSettings, diff --git a/lib/crates/fabro-types/src/settings/v2/to_runtime.rs b/lib/crates/fabro-types/src/settings/v2/to_runtime.rs index c3e3705f5..f27b251b5 100644 --- a/lib/crates/fabro-types/src/settings/v2/to_runtime.rs +++ b/lib/crates/fabro-types/src/settings/v2/to_runtime.rs @@ -1,22 +1,20 @@ //! v2 → runtime-type conversion helpers. //! -//! The runtime types in `fabro_types::settings::{mcp,run,sandbox}` are the -//! shapes that downstream crates (fabro-workflow, fabro-mcp, fabro-sandbox) -//! still consume at runtime. Each helper here reads the v2 parse tree and +//! The runtime types in `fabro_types::settings::{run,sandbox}` are the +//! shapes that downstream crates (fabro-workflow, fabro-sandbox) still +//! consume at runtime. Each helper here reads the v2 parse tree and //! builds the equivalent runtime value. //! -//! Hook bridging has moved to `fabro_hooks::config::bridge_hook`, which -//! owns its runtime shape. Consumer crates will pull the rest of these -//! helpers into their own crates in follow-up 6.3b passes. - -use std::collections::HashMap; +//! Hook bridging has moved to `fabro_hooks::config::bridge_hook`; MCP +//! bridging has moved to `fabro_mcp::config::{bridge_mcps, bridge_mcp_entry}`. +//! Consumer crates will pull the rest of these helpers into their own +//! crates in follow-up 6.3b passes. use super::interp::InterpString; use super::run::{ - McpEntryLayer, MergeStrategy as V2MergeStrategy, RunArtifactsLayer, RunPullRequestLayer, - RunSandboxLayer, WorktreeMode as V2WorktreeMode, + MergeStrategy as V2MergeStrategy, RunArtifactsLayer, RunPullRequestLayer, RunSandboxLayer, + WorktreeMode as V2WorktreeMode, }; -use crate::settings::mcp::{McpServerEntry, McpTransport}; use crate::settings::run::{ ArtifactsSettings, MergeStrategy as OldMergeStrategy, PullRequestSettings, }; @@ -116,95 +114,6 @@ pub fn bridge_run_artifacts(artifacts: &RunArtifactsLayer) -> ArtifactsSettings } } -pub fn bridge_mcps(mcps: &HashMap) -> HashMap { - mcps.iter() - .map(|(name, entry)| (name.clone(), bridge_mcp_entry(entry))) - .collect() -} - -pub fn bridge_mcp_entry(entry: &McpEntryLayer) -> McpServerEntry { - let transport = match entry { - McpEntryLayer::Stdio { - script, - command, - env, - .. - } => { - let command_vec: Vec = if let Some(script) = script { - vec!["sh".into(), "-c".into(), interp_to_string(script)] - } else if let Some(command) = command { - command.iter().map(interp_to_string).collect() - } else { - Vec::new() - }; - McpTransport::Stdio { - command: command_vec, - env: env - .iter() - .map(|(k, v)| (k.clone(), interp_to_string(v))) - .collect(), - } - } - McpEntryLayer::Http { url, headers, .. } => McpTransport::Http { - url: interp_to_string(url), - headers: headers - .iter() - .map(|(k, v)| (k.clone(), interp_to_string(v))) - .collect(), - }, - McpEntryLayer::Sandbox { - script, - command, - port, - env, - .. - } => { - let command_vec: Vec = if let Some(script) = script { - vec!["sh".into(), "-c".into(), interp_to_string(script)] - } else if let Some(command) = command { - command.iter().map(interp_to_string).collect() - } else { - Vec::new() - }; - McpTransport::Sandbox { - command: command_vec, - port: *port, - env: env - .iter() - .map(|(k, v)| (k.clone(), interp_to_string(v))) - .collect(), - } - } - }; - - let (startup_secs, tool_secs) = match entry { - McpEntryLayer::Http { - startup_timeout, - tool_timeout, - .. - } - | McpEntryLayer::Stdio { - startup_timeout, - tool_timeout, - .. - } - | McpEntryLayer::Sandbox { - startup_timeout, - tool_timeout, - .. - } => ( - startup_timeout.map_or(10, |d| d.as_std().as_secs()), - tool_timeout.map_or(60, |d| d.as_std().as_secs()), - ), - }; - - McpServerEntry { - transport, - startup_timeout_secs: startup_secs, - tool_timeout_secs: tool_secs, - } -} - fn interp_to_string(value: &InterpString) -> String { value.as_source() } diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index d59168d3b..b962b4d1e 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -7,13 +7,14 @@ use std::time::{Duration, Instant}; use fabro_config::project as project_config; use fabro_hooks::config::bridge_hook; use fabro_interview::{AutoApproveInterviewer, Interviewer}; +use fabro_mcp::config::bridge_mcp_entry; use fabro_model::{Catalog, FallbackTarget, Provider}; use fabro_sandbox::{SandboxProvider, SandboxSpec}; use fabro_types::RunId; use fabro_types::settings::sandbox::{self as sandbox_config, WorktreeMode}; use fabro_types::settings::v2::run::ModelRefOrSplice; use fabro_types::settings::v2::to_runtime::{ - bridge_mcp_entry, bridge_pull_request, bridge_sandbox, bridge_worktree_mode, + bridge_pull_request, bridge_sandbox, bridge_worktree_mode, }; use fabro_types::settings::v2::{InterpString, SettingsFile};