From 35bcfee3a89edfdfc41179363cc88a38a4c19c5c Mon Sep 17 00:00:00 2001 From: Noah Shipley Date: Mon, 27 Jul 2026 15:03:50 -0500 Subject: [PATCH] feat(acp): allow BUZZ_ACP_MCP_COMMAND to run several MCP servers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `build_mcp_servers` collapsed the whole MCP surface into a single server, so an agent could hold exactly one. `buzz-agent` never had that limit — `McpRegistry::spawn_all` takes a slice and enforces MAX_MCP_SERVERS (16) — the ceiling of one existed only here. One slot forces a real trade-off. An agent given a data MCP loses the tools it acts with: it can read, then has no way to reply. Observed with an agent wired to a remote data MCP, which reasoned itself into a corner mid-turn — "I don't have shell access ... there's no buzz tool exposed here directly" — and silently produced no output, because the slot that previously carried its dev tools now carried the data bridge. `--mcp-command` / `BUZZ_ACP_MCP_COMMAND` now accepts a comma-separated list, matching how `--agent-args` / `BUZZ_ACP_AGENT_ARGS` already parses in the same struct. A single value behaves exactly as before, so existing configurations are unaffected. Two details worth noting: - Blank entries are skipped, so the `""` default (and a stray trailing comma) still yields no servers rather than a bogus one. - Names are derived from the command's file stem, and `spawn_all` rejects duplicates — so colliding stems (`/opt/a/bridge`, `/opt/b/bridge`) are suffixed `bridge`, `bridge-2` instead of aborting startup. The operator's intent there is unambiguous; failing would be unhelpful. Existing `build_mcp_servers` tests are migrated to the list form and extended: several commands produce one server each in order, every server receives the identity env, stems are disambiguated, blank entries are skipped without dropping valid ones, and an empty list produces nothing. Signed-off-by: Noah Shipley --- crates/buzz-acp/src/config.rs | 25 +++- crates/buzz-acp/src/lib.rs | 215 ++++++++++++++++++++++++---------- 2 files changed, 175 insertions(+), 65 deletions(-) diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index 9a1b74c276..dcda07b5ab 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -258,8 +258,17 @@ pub struct CliArgs { )] pub agent_args: Vec, - #[arg(long, env = "BUZZ_ACP_MCP_COMMAND", default_value = "")] - pub mcp_command: String, + /// MCP server binaries to run alongside the agent. Accepts a + /// comma-separated list so an agent can hold more than one server (e.g. a + /// data MCP plus the dev tools it replies with); a single value keeps the + /// previous behaviour. + #[arg( + long = "mcp-command", + env = "BUZZ_ACP_MCP_COMMAND", + default_value = "", + value_delimiter = ',' + )] + pub mcp_commands: Vec, /// Idle timeout: max seconds of silence before killing a turn. /// Resets on any agent stdout activity. @@ -494,7 +503,7 @@ pub struct Config { pub relay_url: String, pub agent_command: String, pub agent_args: Vec, - pub mcp_command: String, + pub mcp_commands: Vec, pub idle_timeout_secs: u64, pub max_turn_duration_secs: u64, pub agents: u32, @@ -1034,7 +1043,11 @@ impl Config { relay_url: args.relay_url, agent_command, agent_args, - mcp_command: args.mcp_command, + mcp_commands: args + .mcp_commands + .into_iter() + .filter(|c| !c.trim().is_empty()) + .collect(), idle_timeout_secs, max_turn_duration_secs, agents: args.agents, @@ -1104,7 +1117,7 @@ impl Config { self.keys.public_key().to_hex(), self.agent_command, self.agent_args.join(" "), - self.mcp_command, + self.mcp_commands.join(","), self.idle_timeout_secs, self.max_turn_duration_secs, self.agents, @@ -1412,7 +1425,7 @@ mod tests { relay_url: "ws://localhost:3000".into(), agent_command: "goose".into(), agent_args: vec!["acp".into()], - mcp_command: "".into(), + mcp_commands: vec![], idle_timeout_secs: DEFAULT_IDLE_TIMEOUT_SECS, max_turn_duration_secs: DEFAULT_MAX_TURN_DURATION_SECS, agents: 1, diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index b11d96d8f7..119087f11c 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -4139,61 +4139,89 @@ async fn run_models(args: ModelsArgs) -> Result<()> { Ok(()) } +/// Derive a stable, unique server name from a command path. +/// +/// `McpRegistry::spawn_all` rejects duplicate names, so two commands sharing a +/// file stem (`/opt/a/bridge` and `/opt/b/bridge`) would abort startup. Suffix +/// the later one rather than failing: the operator's intent is unambiguous. +fn mcp_server_name(command: &str, taken: &mut std::collections::HashSet) -> String { + let base = std::path::Path::new(command) + .file_stem() + .and_then(|s| s.to_str()) + .filter(|s| !s.is_empty()) + .unwrap_or("mcp") + .to_string(); + if taken.insert(base.clone()) { + return base; + } + for n in 2.. { + let candidate = format!("{base}-{n}"); + if taken.insert(candidate.clone()) { + return candidate; + } + } + unreachable!("suffix search is unbounded") +} + fn build_mcp_servers(config: &Config) -> Vec { - if config.mcp_command.is_empty() { - return vec![]; - } - vec![McpServer { - name: std::path::Path::new(&config.mcp_command) - .file_stem() - .and_then(|s| s.to_str()) - .unwrap_or("mcp") - .to_string(), - command: config.mcp_command.clone(), - args: vec![], - env: { - let mut env = vec![ - EnvVar { - name: "BUZZ_RELAY_URL".into(), - value: config.relay_url.clone(), - }, - EnvVar { - name: "BUZZ_PRIVATE_KEY".into(), - // bech32 encoding of a valid secret key is infallible. - // Panic here is correct: injecting a bogus secret would cause - // delayed, hard-to-diagnose agent failures downstream. - value: config - .keys - .secret_key() - .to_bech32() - .expect("secret key bech32 encoding should never fail"), - }, - ]; - // Forward BUZZ_AUTH_TAG (NIP-OA owner attestation credential) - // so the MCP server can attach it to every signed event. - if let Ok(auth_tag) = std::env::var("BUZZ_AUTH_TAG") { - if !auth_tag.is_empty() { - env.push(EnvVar { - name: "BUZZ_AUTH_TAG".into(), - value: auth_tag, - }); - } + // The env every MCP subprocess receives. buzz-agent clears the environment + // before spawning, so anything a server needs has to be handed over here. + let base_env = || { + let mut env = vec![ + EnvVar { + name: "BUZZ_RELAY_URL".into(), + value: config.relay_url.clone(), + }, + EnvVar { + name: "BUZZ_PRIVATE_KEY".into(), + // bech32 encoding of a valid secret key is infallible. + // Panic here is correct: injecting a bogus secret would cause + // delayed, hard-to-diagnose agent failures downstream. + value: config + .keys + .secret_key() + .to_bech32() + .expect("secret key bech32 encoding should never fail"), + }, + ]; + // Forward BUZZ_AUTH_TAG (NIP-OA owner attestation credential) + // so the MCP server can attach it to every signed event. + if let Ok(auth_tag) = std::env::var("BUZZ_AUTH_TAG") { + if !auth_tag.is_empty() { + env.push(EnvVar { + name: "BUZZ_AUTH_TAG".into(), + value: auth_tag, + }); } - // Forward the agent's display name so dev-mcp can use it as the git - // author name instead of the raw npub. Read from the process env - // rather than Config: this is a pass-through of a contract owned - // upstream, and absent simply means dev-mcp falls back to the npub. - if let Ok(display_name) = std::env::var("BUZZ_ACP_DISPLAY_NAME") { - if !display_name.is_empty() { - env.push(EnvVar { - name: "BUZZ_ACP_DISPLAY_NAME".into(), - value: display_name, - }); - } + } + // Forward the agent's display name so dev-mcp can use it as the git + // author name instead of the raw npub. Read from the process env + // rather than Config: this is a pass-through of a contract owned + // upstream, and absent simply means dev-mcp falls back to the npub. + if let Ok(display_name) = std::env::var("BUZZ_ACP_DISPLAY_NAME") { + if !display_name.is_empty() { + env.push(EnvVar { + name: "BUZZ_ACP_DISPLAY_NAME".into(), + value: display_name, + }); } - env - }, - }] + } + env + }; + + let mut taken = std::collections::HashSet::new(); + config + .mcp_commands + .iter() + .map(|c| c.trim()) + .filter(|c| !c.is_empty()) + .map(|command| McpServer { + name: mcp_server_name(command, &mut taken), + command: command.to_string(), + args: vec![], + env: base_env(), + }) + .collect() } #[cfg(test)] @@ -4962,7 +4990,7 @@ mod build_mcp_servers_tests { relay_url: "ws://localhost:3000".into(), agent_command: "goose".into(), agent_args: vec!["acp".into()], - mcp_command: "test-mcp-server".into(), + mcp_commands: vec!["test-mcp-server".into()], idle_timeout_secs: config::DEFAULT_IDLE_TIMEOUT_SECS, max_turn_duration_secs: config::DEFAULT_MAX_TURN_DURATION_SECS, agents: 1, @@ -5107,18 +5135,18 @@ mod build_mcp_servers_tests { #[test] fn empty_mcp_command_returns_no_servers() { let mut config = test_config(); - config.mcp_command = "".into(); + config.mcp_commands = vec!["".into()]; let servers = build_mcp_servers(&config); assert!( servers.is_empty(), - "empty mcp_command should produce no MCP servers" + "empty mcp command should produce no MCP servers" ); } #[test] fn absolute_path_mcp_command_uses_file_stem_as_name() { let mut config = test_config(); - config.mcp_command = "/opt/bin/my-mcp-server".into(); + config.mcp_commands = vec!["/opt/bin/my-mcp-server".into()]; let servers = build_mcp_servers(&config); assert_eq!(servers.len(), 1); assert_eq!(servers[0].name, "my-mcp-server"); @@ -5128,7 +5156,7 @@ mod build_mcp_servers_tests { fn mcp_command_with_no_stem_falls_back_to_mcp() { // Path::new("").file_stem() returns None — exercises the unwrap_or("mcp") path. let mut config = test_config(); - config.mcp_command = "".into(); + config.mcp_commands = vec!["".into()]; // Empty command returns no servers; test the stem logic directly. assert_eq!( std::path::Path::new("") @@ -5139,7 +5167,7 @@ mod build_mcp_servers_tests { ); // Confirm a non-empty command with no stem (e.g. just a dot) also falls back. - config.mcp_command = ".".into(); + config.mcp_commands = vec![".".into()]; let servers = build_mcp_servers(&config); assert_eq!(servers.len(), 1); assert_eq!( @@ -5147,6 +5175,75 @@ mod build_mcp_servers_tests { "Path::new(\".\").file_stem() is None — should fall back to \"mcp\"" ); } + + #[test] + fn multiple_mcp_commands_produce_one_server_each() { + // The motivating case: an agent needs a data MCP *and* the dev tools it + // replies with. A single slot forces a choice between reading and acting. + let mut config = test_config(); + config.mcp_commands = vec![ + "/usr/local/bin/buzz-dev-mcp".into(), + "/usr/local/bin/data-bridge".into(), + ]; + let servers = build_mcp_servers(&config); + assert_eq!(servers.len(), 2); + let names: Vec<&str> = servers.iter().map(|s| s.name.as_str()).collect(); + assert_eq!(names, vec!["buzz-dev-mcp", "data-bridge"]); + let commands: Vec<&str> = servers.iter().map(|s| s.command.as_str()).collect(); + assert_eq!( + commands, + vec!["/usr/local/bin/buzz-dev-mcp", "/usr/local/bin/data-bridge"] + ); + } + + #[test] + fn every_server_receives_the_identity_env() { + let mut config = test_config(); + config.mcp_commands = vec!["/opt/a/first".into(), "/opt/b/second".into()]; + let servers = build_mcp_servers(&config); + assert_eq!(servers.len(), 2); + for server in &servers { + let names: Vec<&str> = server.env.iter().map(|e| e.name.as_str()).collect(); + assert!( + names.contains(&"BUZZ_RELAY_URL") && names.contains(&"BUZZ_PRIVATE_KEY"), + "server {} missing identity env; got {names:?}", + server.name + ); + } + } + + /// `McpRegistry::spawn_all` rejects duplicate server names, so colliding + /// file stems must be disambiguated rather than aborting startup. + #[test] + fn colliding_file_stems_are_disambiguated() { + let mut config = test_config(); + config.mcp_commands = vec![ + "/opt/a/bridge".into(), + "/opt/b/bridge".into(), + "/opt/c/bridge".into(), + ]; + let servers = build_mcp_servers(&config); + let names: Vec<&str> = servers.iter().map(|s| s.name.as_str()).collect(); + assert_eq!(names, vec!["bridge", "bridge-2", "bridge-3"]); + let unique: std::collections::HashSet<&&str> = names.iter().collect(); + assert_eq!(unique.len(), names.len(), "server names must be unique"); + } + + #[test] + fn blank_entries_are_skipped_without_dropping_the_rest() { + let mut config = test_config(); + config.mcp_commands = vec!["".into(), " ".into(), "/opt/bin/real".into()]; + let servers = build_mcp_servers(&config); + assert_eq!(servers.len(), 1); + assert_eq!(servers[0].name, "real"); + } + + #[test] + fn no_mcp_commands_returns_no_servers() { + let mut config = test_config(); + config.mcp_commands = vec![]; + assert!(build_mcp_servers(&config).is_empty()); + } } #[cfg(test)] @@ -5183,7 +5280,7 @@ mod error_outcome_emission_tests { // feed emission under test. agent_command: "true".into(), agent_args: vec![], - mcp_command: "test-mcp-server".into(), + mcp_commands: vec!["test-mcp-server".into()], idle_timeout_secs: config::DEFAULT_IDLE_TIMEOUT_SECS, max_turn_duration_secs: config::DEFAULT_MAX_TURN_DURATION_SECS, agents: 1,