diff --git a/lib/crates/fabro-cli/src/commands/exec.rs b/lib/crates/fabro-cli/src/commands/exec.rs index bb0fb4dad..c421632a8 100644 --- a/lib/crates/fabro-cli/src/commands/exec.rs +++ b/lib/crates/fabro-cli/src/commands/exec.rs @@ -17,6 +17,7 @@ use fabro_llm::types::{ use fabro_mcp::config::McpServerSettings; use fabro_model::ProviderId; use fabro_types::settings::cli::OutputFormat as SettingsOutputFormat; +use fabro_types::settings::run::ResolvedMcpEntry; use fabro_util::exit::{self, ErrorExt, ExitClass}; use futures::stream; use serde::Deserialize; @@ -289,6 +290,24 @@ fn process_env_var(name: &str) -> Option { std::env::var(name).ok() } +fn run_mcp_servers_for_exec( + mcps: &HashMap, +) -> AnyResult> { + mcps.iter() + .map(|(key, entry)| match entry { + ResolvedMcpEntry::Resolved(server) => Ok(server.clone()), + ResolvedMcpEntry::Reference(reference) => { + anyhow::bail!( + "fabro exec cannot resolve run.agent.mcps.{key} catalog reference \ + (id `{}`); define an inline server under [cli.exec.agent.mcps.{key}] or \ + remove the run-level reference", + reference.id + ); + } + }) + .collect() +} + pub(crate) async fn execute(mut args: ExecArgs, ctx: &CommandContext) -> AnyResult<()> { use fabro_agent::cli::PermissionLevel as AgentPermissionLevel; use fabro_types::settings::run::AgentPermissions; @@ -317,7 +336,9 @@ pub(crate) async fn execute(mut args: ExecArgs, ctx: &CommandContext) -> AnyResu Some(mcps) => mcps.values().cloned().collect(), None => ctx .run_settings() - .map(|settings| settings.agent.mcps.values().cloned().collect()) + .ok() + .map(|settings| run_mcp_servers_for_exec(&settings.agent.mcps)) + .transpose()? .unwrap_or_default(), }; // Resolve `{{ env.* }}` in MCP transport config at the exec boundary, @@ -368,3 +389,45 @@ pub(crate) async fn execute(mut args: ExecArgs, ctx: &CommandContext) -> AnyResu Ok(()) } + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use fabro_types::settings::run::{McpServerRef, McpServerSettings, ResolvedMcpEntry}; + + use super::run_mcp_servers_for_exec; + + #[test] + fn run_mcp_servers_for_exec_rejects_catalog_references() { + let err = run_mcp_servers_for_exec(&HashMap::from([( + "sentry".to_string(), + ResolvedMcpEntry::Reference(McpServerRef { + id: "catalog/sentry".to_string(), + enabled: None, + }), + )])) + .expect_err("fabro exec should reject unresolved run-level MCP references"); + + assert!( + err.to_string() + .contains("fabro exec cannot resolve run.agent.mcps.sentry catalog reference"), + "unexpected error: {err}" + ); + } + + #[test] + fn run_mcp_servers_for_exec_keeps_resolved_servers() { + let servers = run_mcp_servers_for_exec(&HashMap::from([( + "inline".to_string(), + ResolvedMcpEntry::Resolved(McpServerSettings { + name: "inline".to_string(), + ..McpServerSettings::default() + }), + )])) + .expect("resolved inline server should be usable by fabro exec"); + + assert_eq!(servers.len(), 1); + assert_eq!(servers[0].name, "inline"); + } +} diff --git a/lib/crates/fabro-config/src/resolve/mod.rs b/lib/crates/fabro-config/src/resolve/mod.rs index 17e7b4b15..ef2e8d917 100644 --- a/lib/crates/fabro-config/src/resolve/mod.rs +++ b/lib/crates/fabro-config/src/resolve/mod.rs @@ -101,7 +101,9 @@ pub(crate) fn warn_if_demoted_template(field: &str, value: Option<&str>) { mod tests { use std::collections::HashMap; - use fabro_types::settings::run::{HookType, McpHttpProtocol, McpTransport, TlsMode}; + use fabro_types::settings::run::{ + HookType, McpHttpProtocol, McpTransport, ResolvedMcpEntry, TlsMode, + }; use crate::SettingsLayer; use crate::tests::workflow_settings_from_layer; @@ -151,9 +153,14 @@ Authorization = "Bearer {{ env.HOOK_TOKEN }}" .expect("run settings should resolve") .run; let mcps = &resolved.agent.mcps; + let transport = |name: &str| { + mcps.get(name) + .and_then(ResolvedMcpEntry::as_resolved) + .map(|server| &server.transport) + }; assert_eq!( - mcps.get("stdio").map(|mcp| &mcp.transport), + transport("stdio"), Some(&McpTransport::Stdio { command: vec!["fabro-mcp".to_string(), "--stdio".to_string()], env: HashMap::from([( @@ -163,7 +170,7 @@ Authorization = "Bearer {{ env.HOOK_TOKEN }}" }) ); assert_eq!( - mcps.get("http").map(|mcp| &mcp.transport), + transport("http"), Some(&McpTransport::Http { protocol: McpHttpProtocol::default(), url: "https://mcp.example.com".to_string(), @@ -174,7 +181,7 @@ Authorization = "Bearer {{ env.HOOK_TOKEN }}" }) ); assert_eq!( - mcps.get("sandbox").map(|mcp| &mcp.transport), + transport("sandbox"), Some(&McpTransport::Sandbox { protocol: McpHttpProtocol::default(), command: vec!["fabro-mcp".to_string(), "--sandbox".to_string()], diff --git a/lib/crates/fabro-config/src/resolve/run.rs b/lib/crates/fabro-config/src/resolve/run.rs index 2feb72897..6a9623ea4 100644 --- a/lib/crates/fabro-config/src/resolve/run.rs +++ b/lib/crates/fabro-config/src/resolve/run.rs @@ -4,11 +4,11 @@ use fabro_types::settings::InterpString; use fabro_types::settings::run::{ ArtifactsSettings, GitAuthorSettings, HookDefinition, HookType, InterviewProviderSettings, McpServerSettings, McpTransport, MergeStrategy, NotificationProviderSettings, - NotificationRouteSettings, PullRequestSettings, RunAgentSettings, RunBranchSettings, - RunCheckpointSettings, RunCloneSettings, RunExecutionSettings, RunGitSettings, RunGoal, - RunIntegrationsGithubSettings, RunIntegrationsSettings, RunInterviewsSettings, - RunMetaBranchSettings, RunModelControls, RunModelSettings, RunNamespace, RunPrepareSettings, - RunScmSettings, ScmGitHubSettings, TlsMode, + NotificationRouteSettings, PullRequestSettings, ResolvedMcpEntry, RunAgentSettings, + RunBranchSettings, RunCheckpointSettings, RunCloneSettings, RunExecutionSettings, + RunGitSettings, RunGoal, RunIntegrationsGithubSettings, RunIntegrationsSettings, + RunInterviewsSettings, RunMetaBranchSettings, RunModelControls, RunModelSettings, RunNamespace, + RunPrepareSettings, RunScmSettings, ScmGitHubSettings, TlsMode, }; use super::{ResolveError, resolve_run_environment}; @@ -288,7 +288,9 @@ fn resolve_agent(agent: Option<&RunAgentLayer>) -> RunAgentSettings { RunAgentSettings { fabro_tools: agent.fabro_tools.unwrap_or(false), permissions: agent.permissions, - mcps: resolve_enabled_mcps(&agent.mcps), + mcps: enabled_mcp_settings(&agent.mcps) + .map(|(name, settings)| (name, ResolvedMcpEntry::Resolved(settings))) + .collect(), } } @@ -299,10 +301,15 @@ fn resolve_agent(agent: Option<&RunAgentLayer>) -> RunAgentSettings { pub(crate) fn resolve_enabled_mcps( mcps: &StickyMap, ) -> HashMap { + enabled_mcp_settings(mcps).collect() +} + +fn enabled_mcp_settings( + mcps: &StickyMap, +) -> impl Iterator + '_ { mcps.iter() .filter(|(_, entry)| entry.is_enabled()) .map(|(name, entry)| (name.clone(), resolve_mcp_entry(name, entry))) - .collect() } #[expect( diff --git a/lib/crates/fabro-config/src/tests/resolve_run.rs b/lib/crates/fabro-config/src/tests/resolve_run.rs index 5e8f547d5..9aba7fa7c 100644 --- a/lib/crates/fabro-config/src/tests/resolve_run.rs +++ b/lib/crates/fabro-config/src/tests/resolve_run.rs @@ -1017,7 +1017,7 @@ 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 fabro_types::settings::run::{McpTransport, ResolvedMcpEntry}; use crate::SettingsLayer; use crate::layers::Combine; @@ -1028,8 +1028,9 @@ mod run_agent_mcps { .expect("fixture should parse via SettingsLayer") } - fn stdio_command(transport: &McpTransport) -> &[String] { - match transport { + fn stdio_command(entry: &ResolvedMcpEntry) -> &[String] { + let server = entry.as_resolved().expect("expected resolved inline entry"); + match &server.transport { McpTransport::Stdio { command, .. } => command, other => panic!("expected stdio transport, got {other:?}"), } @@ -1071,7 +1072,7 @@ command = ["extra-server"] // 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), &[ + assert_eq!(stdio_command(&mcps["fs"]), &[ "fs-server".to_string(), "--workflow".to_string() ],); diff --git a/lib/crates/fabro-types/src/settings/mod.rs b/lib/crates/fabro-types/src/settings/mod.rs index c147b7829..aa2d7fb3e 100644 --- a/lib/crates/fabro-types/src/settings/mod.rs +++ b/lib/crates/fabro-types/src/settings/mod.rs @@ -37,12 +37,12 @@ pub use run::{ ArtifactsSettings, DockerfileSource, EnvironmentImageSettings, EnvironmentLifecycleSettings, EnvironmentNetworkMode, EnvironmentNetworkSettings, EnvironmentProvider, EnvironmentResourcesSettings, EnvironmentSettings, GitAuthorSettings, HookDefinition, HookType, - InterviewProviderSettings, McpServerSettings, McpTransport, NotificationProviderSettings, - NotificationRouteSettings, PullRequestSettings, RunAgentSettings, RunCheckpointSettings, - RunEnvironmentSettings, RunExecutionSettings, RunGitSettings, RunGoal, - RunIntegrationsGithubSettings, RunIntegrationsSettings, RunInterviewsSettings, - RunModelControls, RunModelSettings, RunNamespace, RunPrepareSettings, RunScmSettings, - ScmGitHubSettings, TlsMode, + InterviewProviderSettings, McpServerRef, McpServerSettings, McpTransport, + NotificationProviderSettings, NotificationRouteSettings, PullRequestSettings, ResolvedMcpEntry, + RunAgentSettings, RunCheckpointSettings, RunEnvironmentSettings, RunExecutionSettings, + RunGitSettings, RunGoal, RunIntegrationsGithubSettings, RunIntegrationsSettings, + RunInterviewsSettings, RunModelControls, RunModelSettings, RunNamespace, RunPrepareSettings, + RunScmSettings, ScmGitHubSettings, TlsMode, }; pub use server::{ GithubIntegrationSettings, IntegrationWebhooksSettings, LogDestination, ObjectStoreSettings, diff --git a/lib/crates/fabro-types/src/settings/run.rs b/lib/crates/fabro-types/src/settings/run.rs index ec2503642..c34fab921 100644 --- a/lib/crates/fabro-types/src/settings/run.rs +++ b/lib/crates/fabro-types/src/settings/run.rs @@ -10,6 +10,7 @@ use std::collections::HashMap; use std::path::PathBuf; use std::time::Duration as StdDuration; +use serde::de::{self, Deserializer}; use serde::ser::SerializeStruct; use serde::{Deserialize, Serialize}; @@ -106,9 +107,13 @@ impl RunNamespace { // run.scm.owner/repository were demoted and removed from this pass // (D2): values stay literal. substitute_string_vec(&mut self.prepare.commands, &mut lookup)?; - for mcp in self.agent.mcps.values_mut() { - substitute_string(&mut mcp.name, &mut lookup)?; - substitute_mcp_transport(&mut mcp.transport, &mut lookup)?; + // Only resolved inline servers carry substitutable templates; an + // unresolved reference holds just an id + enabled flag. + for entry in self.agent.mcps.values_mut() { + if let ResolvedMcpEntry::Resolved(mcp) = entry { + substitute_string(&mut mcp.name, &mut lookup)?; + substitute_mcp_transport(&mut mcp.transport, &mut lookup)?; + } } for hook in &mut self.hooks { substitute_option_string(&mut hook.name, &mut lookup)?; @@ -356,21 +361,24 @@ mod run_namespace_variable_substitution_tests { timeout_ms: 1_000, }, agent: super::RunAgentSettings { - mcps: HashMap::from([("http".to_string(), McpServerSettings { - name: "http".to_string(), - transport: McpTransport::Http { - protocol: McpHttpProtocol::default(), - url: "https://{{ vars.HOST }}/mcp".to_string(), - headers: HashMap::from([( - "X-Env".to_string(), - "{{ vars.ENV }}".to_string(), - )]), - }, - current_dir: None, - clear_env: false, - startup_timeout_secs: 10, - tool_timeout_secs: 60, - })]), + mcps: HashMap::from([( + "http".to_string(), + super::ResolvedMcpEntry::Resolved(McpServerSettings { + name: "http".to_string(), + transport: McpTransport::Http { + protocol: McpHttpProtocol::default(), + url: "https://{{ vars.HOST }}/mcp".to_string(), + headers: HashMap::from([( + "X-Env".to_string(), + "{{ vars.ENV }}".to_string(), + )]), + }, + current_dir: None, + clear_env: false, + startup_timeout_secs: 10, + tool_timeout_secs: 60, + }), + )]), ..super::RunAgentSettings::default() }, hooks: vec![HookDefinition { @@ -412,7 +420,9 @@ mod run_namespace_variable_substitution_tests { assert_eq!(run.prepare.commands, vec![ "echo prod {{ env.REGION }}".to_string() ]); - let mcp = &run.agent.mcps["http"]; + let mcp = run.agent.mcps["http"] + .as_resolved() + .expect("expected resolved inline mcp entry"); match &mcp.transport { McpTransport::Http { url, headers, .. } => { assert_eq!(url, "https://mcp.example/mcp"); @@ -1053,12 +1063,95 @@ pub struct RunAgentSettings { #[serde(default)] pub fabro_tools: bool, pub permissions: Option, - pub mcps: HashMap, + pub mcps: HashMap, +} + +/// An MCP entry in a run's agent settings: either an inline resolved server or +/// an unresolved reference to a server-side catalog definition. +/// +/// `Resolved` (de)serializes as a **bare** [`McpServerSettings`] - with no enum +/// tag - for backward compatibility with run specs persisted before this enum +/// existed. Serialization stays `#[serde(untagged)]`, and custom +/// deserialization preserves the same wire shapes while rejecting entries that +/// mix catalog-reference fields (`id`, `enabled`) with inline server fields. +/// +/// Invariant: no `Reference` survives run creation. References are resolved to +/// `Resolved` on the server's run-preparation path before the run spec is +/// persisted; any `Reference` reaching a post-persistence consumer is a bug. +#[derive(Debug, Clone, PartialEq, Serialize)] +#[serde(untagged)] +pub enum ResolvedMcpEntry { + Resolved(McpServerSettings), + Reference(McpServerRef), +} + +impl<'de> Deserialize<'de> for ResolvedMcpEntry { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let value = serde_json::Value::deserialize(deserializer)?; + let serde_json::Value::Object(map) = &value else { + return Err(de::Error::custom("MCP entry must be a table")); + }; + + let has_reference_fields = map.contains_key("id") || map.contains_key("enabled"); + let has_inline_server_fields = map.contains_key("name") + || map.contains_key("transport") + || map.contains_key("current_dir") + || map.contains_key("clear_env") + || map.contains_key("startup_timeout_secs") + || map.contains_key("tool_timeout_secs"); + + if has_reference_fields && has_inline_server_fields { + return Err(de::Error::custom( + "MCP entry cannot mix catalog reference fields (`id`, `enabled`) with inline \ + server fields", + )); + } + + if has_reference_fields { + serde_json::from_value(value) + .map(Self::Reference) + .map_err(de::Error::custom) + } else { + serde_json::from_value(value) + .map(Self::Resolved) + .map_err(de::Error::custom) + } + } +} + +impl ResolvedMcpEntry { + /// The inline resolved server, or `None` for an unresolved [`Reference`]. + /// + /// [`Reference`]: ResolvedMcpEntry::Reference + #[must_use] + pub fn as_resolved(&self) -> Option<&McpServerSettings> { + match self { + Self::Resolved(server) => Some(server), + Self::Reference(_) => None, + } + } +} + +/// An unresolved reference to a server-defined MCP catalog entry. +/// +/// `id` is kept as a plain `String` to keep `fabro-types` decoupled from the +/// `fabro-mcp-store` crate; the resolver maps it to a store id at resolve time. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct McpServerRef { + pub id: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub enabled: Option, } #[cfg(test)] mod run_agent_settings_tests { - use super::RunAgentSettings; + use super::{ + McpServerRef, McpServerSettings, McpTransport, ResolvedMcpEntry, RunAgentSettings, + }; #[test] fn deserializes_missing_fabro_tools_as_false() { @@ -1070,6 +1163,171 @@ mod run_agent_settings_tests { assert!(!settings.fabro_tools); } + + /// Critical back-compat guarantee: a run spec persisted before + /// `ResolvedMcpEntry` existed stores `mcps` as a map of bare + /// `McpServerSettings` (no enum tag). It must still deserialize, with every + /// entry landing as `ResolvedMcpEntry::Resolved`. + #[test] + fn deserializes_old_format_bare_mcps_as_resolved_json() { + let settings: RunAgentSettings = serde_json::from_value(serde_json::json!({ + "fabro_tools": true, + "permissions": null, + "mcps": { + "filesystem": { + "name": "filesystem", + "transport": { + "type": "stdio", + "command": ["npx", "server-filesystem"], + "env": {} + }, + "startup_timeout_secs": 10, + "tool_timeout_secs": 60 + } + } + })) + .expect("old-format run agent settings should deserialize"); + + let entry = settings + .mcps + .get("filesystem") + .expect("filesystem entry should be present"); + match entry { + ResolvedMcpEntry::Resolved(server) => { + assert_eq!(server.name, "filesystem"); + assert!(matches!(server.transport, McpTransport::Stdio { .. })); + } + ResolvedMcpEntry::Reference(_) => { + panic!("bare McpServerSettings must deserialize as Resolved, not Reference") + } + } + } + + /// The same guarantee via TOML, since run specs are also persisted/read as + /// TOML config. + #[test] + fn deserializes_old_format_bare_mcps_as_resolved_toml() { + let toml = r#" +fabro_tools = true + +[mcps.http_server] +name = "http_server" +startup_timeout_secs = 10 +tool_timeout_secs = 60 + +[mcps.http_server.transport] +type = "http" +url = "https://example.com/mcp" + +[mcps.http_server.transport.headers] +"#; + let settings: RunAgentSettings = + toml::from_str(toml).expect("old-format TOML run agent settings should deserialize"); + + match settings.mcps.get("http_server") { + Some(ResolvedMcpEntry::Resolved(server)) => { + assert_eq!(server.name, "http_server"); + assert!(matches!(server.transport, McpTransport::Http { .. })); + } + other => panic!("expected Resolved http_server entry, got {other:?}"), + } + } + + /// A `{ id, enabled }`-shaped value deserializes as `Reference` (it has no + /// `name`/`transport`, and `deny_unknown_fields` keeps it from matching a + /// server config), while a full server config deserializes as `Resolved`. + #[test] + fn distinguishes_reference_from_resolved() { + let settings: RunAgentSettings = serde_json::from_value(serde_json::json!({ + "mcps": { + "sentry": { "id": "sentry", "enabled": true }, + "linear": { "id": "linear" }, + "inline": { + "name": "inline", + "transport": { + "type": "stdio", + "command": ["my-server"], + "env": {} + }, + "startup_timeout_secs": 5, + "tool_timeout_secs": 30 + } + } + })) + .expect("mixed reference/resolved settings should deserialize"); + + assert_eq!( + settings.mcps.get("sentry"), + Some(&ResolvedMcpEntry::Reference(McpServerRef { + id: "sentry".to_string(), + enabled: Some(true), + })) + ); + assert_eq!( + settings.mcps.get("linear"), + Some(&ResolvedMcpEntry::Reference(McpServerRef { + id: "linear".to_string(), + enabled: None, + })) + ); + match settings.mcps.get("inline") { + Some(ResolvedMcpEntry::Resolved(server)) => assert_eq!(server.name, "inline"), + other => panic!("expected Resolved inline entry, got {other:?}"), + } + } + + #[test] + fn rejects_entry_mixing_reference_and_inline_server_fields() { + let err = serde_json::from_value::(serde_json::json!({ + "mcps": { + "mixed": { + "id": "catalog-server", + "name": "inline", + "transport": { + "type": "stdio", + "command": ["my-server"], + "env": {} + }, + "startup_timeout_secs": 5, + "tool_timeout_secs": 30 + } + } + })) + .expect_err("mixed reference/server entry should be rejected"); + + assert!( + err.to_string() + .contains("cannot mix catalog reference fields"), + "unexpected error: {err}" + ); + } + + /// `Resolved` serializes back out as a bare `McpServerSettings` (no enum + /// tag) so newly written run specs stay readable by any old reader and the + /// round-trip is stable. + #[test] + fn resolved_serializes_as_bare_server_settings() { + let server = McpServerSettings { + name: "demo".to_string(), + transport: McpTransport::Stdio { + command: vec!["demo-server".to_string()], + env: std::collections::HashMap::new(), + }, + ..McpServerSettings::default() + }; + let entry = ResolvedMcpEntry::Resolved(server.clone()); + + let value = serde_json::to_value(&entry).expect("entry should serialize"); + // No "Resolved" tag — it serializes as the bare server settings. + assert_eq!( + value, + serde_json::to_value(&server).expect("server serializes") + ); + + let round_tripped: ResolvedMcpEntry = + serde_json::from_value(value).expect("entry should round-trip"); + assert_eq!(round_tripped, entry); + } } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index a006e3ec2..6174bb64b 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -18,7 +18,7 @@ use fabro_static::EnvVars; use fabro_types::settings::run::{ ApprovalMode, HookDefinition as ResolvedHookDefinition, HookEvent as ResolvedHookEvent, HookType as ResolvedHookType, McpServerSettings as ResolvedMcpServerSettings, - PullRequestSettings, RunMode, RunModelSettings as ResolvedRunModelSettings, + PullRequestSettings, ResolvedMcpEntry, RunMode, RunModelSettings as ResolvedRunModelSettings, RunNamespace as ResolvedRunSettings, TlsMode as ResolvedTlsMode, }; use fabro_types::settings::{ModelRegistry, ResolvedModelRef}; @@ -377,8 +377,22 @@ impl RunSession { let mcp_servers = resolved .agent .mcps - .values() - .map(|settings| runtime_mcp_server(settings, process_env_var)) + .iter() + .map(|(key, entry)| match entry { + ResolvedMcpEntry::Resolved(server) => runtime_mcp_server(server, process_env_var), + // References must be resolved to concrete servers before the run + // spec is persisted (server-side run-preparation pass). Reaching + // worker startup with an unresolved reference is an invariant + // violation, so fail loudly rather than silently dropping it. + ResolvedMcpEntry::Reference(reference) => { + let message = format!( + "unresolved MCP server reference `{key}` (id `{}`) reached worker \ + startup; references must be resolved before the run spec is persisted", + reference.id + ); + Err(Error::engine(message)) + } + }) .collect::, _>>()?; let sandbox = match sandbox_provider {