From 7ed0d27ea0ef804ad74521818602e09d5c4fda20 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Sun, 12 Jul 2026 21:37:50 -0400 Subject: [PATCH 01/17] feat(desktop): buzz-agent MCP server configuration Add local-only MCP server layering (global < definition < agent) for buzz-agent. Other runtimes manage their own MCP config and are unchanged. - McpServerConfig/McpServerTransport types with validate/merge/replace helpers - BUZZ_ACP_MCP_SERVERS reserved env key; desktop injects transport JSON only for buzz-agent children, explicitly removes it for all others - spawn_config_hash includes effective MCP layer so config drift triggers restart - build_deploy_payload forwards resolved Mcp transport to providers - agent_snapshot excludes mcp_servers (credentials in env vars) - GlobalAgentConfig.mcp_servers: inherits to every buzz-agent instance - AgentDefinition.mcp_servers: definition layer (not copied into record on mint) - ManagedAgentRecord.mcp_servers: per-agent override layer - buzz-acp: ConfiguredMcpServer deserialized from BUZZ_ACP_MCP_SERVERS; appended after built-in buzz-dev-mcp in build_mcp_servers - Max 15 user-defined servers (slot 16 is reserved for buzz-dev-mcp) - IPC validation at every save boundary: empty name, duplicate names, enabled with empty command, NUL bytes, per-field length cap, total payload cap, reserved env key guard Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-acp/src/config.rs | 62 ++++++ crates/buzz-acp/src/lib.rs | 68 ++++++- desktop/scripts/check-file-sizes.mjs | 3 +- .../src-tauri/src/commands/agent_config.rs | 3 + .../src-tauri/src/commands/agent_models.rs | 4 + desktop/src-tauri/src/commands/agents.rs | 2 + .../src-tauri/src/commands/agents_deploy.rs | 11 + .../src-tauri/src/commands/agents_tests.rs | 14 +- .../commands/personas/delete_cascade_tests.rs | 1 + .../src/commands/personas/inbound_tests.rs | 3 + .../src-tauri/src/commands/personas/mod.rs | 6 + .../personas/name_propagation_tests.rs | 1 + .../src/commands/personas/snapshot/import.rs | 2 + .../src/commands/personas/snapshot/tests.rs | 1 + .../src/managed_agents/agent_events.rs | 13 ++ .../src/managed_agents/agent_snapshot.rs | 24 ++- .../config_bridge/reader_tests.rs | 1 + .../src/managed_agents/discovery/tests.rs | 2 + .../src-tauri/src/managed_agents/env_vars.rs | 3 + .../src/managed_agents/global_config/mod.rs | 10 +- .../src/managed_agents/global_config/tests.rs | 26 +++ .../src/managed_agents/nest/tests.rs | 2 + .../src/managed_agents/persona_events.rs | 1 + .../managed_agents/persona_events/tests.rs | 23 ++- .../src-tauri/src/managed_agents/personas.rs | 2 + .../src/managed_agents/personas/tests.rs | 5 + .../src-tauri/src/managed_agents/readiness.rs | 1 + .../src/managed_agents/relay_mesh.rs | 1 + .../src-tauri/src/managed_agents/runtime.rs | 24 +++ .../src/managed_agents/runtime/tests.rs | 26 +++ .../src/managed_agents/spawn_hash.rs | 16 ++ .../src/managed_agents/spawn_hash/tests.rs | 87 +++++++- .../src/managed_agents/teams_tests.rs | 1 + desktop/src-tauri/src/managed_agents/types.rs | 15 ++ .../src/managed_agents/types/mcp_servers.rs | 192 ++++++++++++++++++ .../src/managed_agents/types/requests.rs | 15 +- .../src/managed_agents/types/tests.rs | 98 ++++++++- 37 files changed, 753 insertions(+), 16 deletions(-) create mode 100644 desktop/src-tauri/src/managed_agents/types/mcp_servers.rs diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index e8e8fdd51..8708c9dbb 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -4,6 +4,8 @@ //! Config file (TOML) for complex subscription rules. use std::collections::{HashMap, HashSet}; + +use serde::{Deserialize, Serialize}; use std::path::PathBuf; use clap::Parser; @@ -37,6 +39,9 @@ pub(crate) const MAX_TURN_DURATION_CEILING_SECS: u64 = 604_800; #[derive(Debug, Error)] pub enum ConfigError { + #[error("invalid BUZZ_ACP_MCP_SERVERS: {0}")] + McpServers(String), + #[error("failed to parse nostr keys: {0}")] KeyParse(#[from] nostr::key::Error), @@ -193,6 +198,34 @@ pub struct ModelsArgs { pub json: bool, } +/// User-configured stdio MCP server received from desktop via +/// `BUZZ_ACP_MCP_SERVERS`. The desktop resolves `enabled` as local layering +/// metadata before serializing this transport shape. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct ConfiguredMcpServer { + pub name: String, + pub command: String, + #[serde(default)] + pub args: Vec, + #[serde(default)] + pub env: Vec, +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct ConfiguredMcpEnvVar { + pub name: String, + pub value: String, +} + +pub fn parse_configured_mcp_servers( + raw: Option<&str>, +) -> Result, ConfigError> { + let Some(raw) = raw.filter(|value| !value.trim().is_empty()) else { + return Ok(Vec::new()); + }; + serde_json::from_str(raw).map_err(|error| ConfigError::McpServers(error.to_string())) +} + #[derive(Debug, Parser)] #[command( name = "buzz-acp", @@ -447,6 +480,8 @@ pub struct Config { pub agent_command: String, pub agent_args: Vec, pub mcp_command: String, + /// Desktop-resolved MCP servers, appended after the built-in Buzz server. + pub configured_mcp_servers: Vec, pub idle_timeout_secs: u64, pub max_turn_duration_secs: u64, pub agents: u32, @@ -917,12 +952,16 @@ impl Config { validate_multiple_event_handling(args.multiple_event_handling, args.dedup)?; + let configured_mcp_servers = + parse_configured_mcp_servers(std::env::var("BUZZ_ACP_MCP_SERVERS").ok().as_deref())?; + let config = Config { keys, relay_url: args.relay_url, agent_command, agent_args, mcp_command: args.mcp_command, + configured_mcp_servers, idle_timeout_secs, max_turn_duration_secs, agents: args.agents, @@ -1300,6 +1339,7 @@ mod tests { agent_command: "goose".into(), agent_args: vec!["acp".into()], mcp_command: "".into(), + configured_mcp_servers: vec![], idle_timeout_secs: DEFAULT_IDLE_TIMEOUT_SECS, max_turn_duration_secs: DEFAULT_MAX_TURN_DURATION_SECS, agents: 1, @@ -1706,6 +1746,28 @@ mod tests { assert!(f.kinds.is_none(), "wildcard should propagate"); } + #[test] + fn parse_configured_mcp_servers_accepts_json_transport() { + let servers = parse_configured_mcp_servers(Some( + r#"[{"name":"github","command":"npx","args":["github-mcp"],"env":[{"name":"TOKEN","value":"secret"}]}]"#, + )) + .unwrap(); + + assert_eq!(servers.len(), 1); + assert_eq!(servers[0].name, "github"); + assert_eq!(servers[0].args, vec!["github-mcp"]); + assert_eq!(servers[0].env[0].name, "TOKEN"); + assert_eq!( + serde_json::to_value(&servers).unwrap()[0]["enabled"], + serde_json::Value::Null + ); + } + + #[test] + fn parse_configured_mcp_servers_rejects_invalid_json() { + assert!(parse_configured_mcp_servers(Some("not-json")).is_err()); + } + #[test] fn test_config_mode_no_matching_rules_empty_result() { let config = test_config(SubscribeMode::Config); diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index e459c9f57..50245354c 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -3402,10 +3402,11 @@ async fn run_models(args: ModelsArgs) -> Result<()> { } fn build_mcp_servers(config: &Config) -> Vec { + let mut servers = Vec::new(); if config.mcp_command.is_empty() { - return vec![]; + return servers; } - vec![McpServer { + servers.push(McpServer { name: std::path::Path::new(&config.mcp_command) .file_stem() .and_then(|s| s.to_str()) @@ -3443,7 +3444,30 @@ fn build_mcp_servers(config: &Config) -> Vec { } env }, - }] + }); + servers.extend( + config + .configured_mcp_servers + .iter() + .map(configured_mcp_server), + ); + servers +} + +fn configured_mcp_server(server: &config::ConfiguredMcpServer) -> McpServer { + McpServer { + name: server.name.clone(), + command: server.command.clone(), + args: server.args.clone(), + env: server + .env + .iter() + .map(|variable| EnvVar { + name: variable.name.clone(), + value: variable.value.clone(), + }) + .collect(), + } } #[cfg(test)] @@ -3875,6 +3899,7 @@ mod build_mcp_servers_tests { agent_command: "goose".into(), agent_args: vec!["acp".into()], mcp_command: "test-mcp-server".into(), + configured_mcp_servers: vec![], idle_timeout_secs: config::DEFAULT_IDLE_TIMEOUT_SECS, max_turn_duration_secs: config::DEFAULT_MAX_TURN_DURATION_SECS, agents: 1, @@ -3911,6 +3936,28 @@ mod build_mcp_servers_tests { } } + #[test] + fn build_mcp_servers_appends_configured_servers_after_builtin() { + let mut config = test_config(); + config.configured_mcp_servers = vec![config::ConfiguredMcpServer { + name: "github".into(), + command: "npx".into(), + args: vec!["github-mcp".into()], + env: vec![config::ConfiguredMcpEnvVar { + name: "TOKEN".into(), + value: "secret".into(), + }], + }]; + + let servers = build_mcp_servers(&config); + assert_eq!(servers.len(), 2); + assert_eq!(servers[0].name, "test-mcp-server"); + assert_eq!(servers[1].name, "github"); + assert_eq!(servers[1].command, "npx"); + assert_eq!(servers[1].args, vec!["github-mcp"]); + assert_eq!(servers[1].env[0].name, "TOKEN"); + } + #[test] fn session_new_mcp_server_has_required_fields() { let config = test_config(); @@ -3961,13 +4008,19 @@ mod build_mcp_servers_tests { } #[test] - fn empty_mcp_command_returns_no_servers() { + fn empty_mcp_command_returns_no_servers_even_when_user_servers_are_configured() { let mut config = test_config(); config.mcp_command = "".into(); - let servers = build_mcp_servers(&config); + config.configured_mcp_servers = vec![config::ConfiguredMcpServer { + name: "github".into(), + command: "npx".into(), + args: vec![], + env: vec![], + }]; + assert!( - servers.is_empty(), - "empty mcp_command should produce no MCP servers" + build_mcp_servers(&config).is_empty(), + "without the built-in MCP server, user servers must not reach an invalid ACP invocation" ); } @@ -4040,6 +4093,7 @@ mod error_outcome_emission_tests { agent_command: "true".into(), agent_args: vec![], mcp_command: "test-mcp-server".into(), + configured_mcp_servers: vec![], idle_timeout_secs: config::DEFAULT_IDLE_TIMEOUT_SECS, max_turn_duration_secs: config::DEFAULT_MAX_TURN_DURATION_SECS, agents: 1, diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 2b75d8b72..5578c4a7d 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -79,7 +79,8 @@ const overrides = new Map([ // 3-phase (stage/stop/commit) + commit_cascade_agents injectable helper for // retry-safety. Load-bearing reviewer-required change; queued to split. // Consolidation removed the legacy persona-card import/export codecs. - ["src-tauri/src/commands/personas/mod.rs", 984], + // +6: local-only MCP layer validation/update wiring. Load-bearing; queued to split. + ["src-tauri/src/commands/personas/mod.rs", 990], // #1418 read-path fix: get_thread_replies' blocker fix (shared TIMELINE_KINDS // const + build_thread_replies_filter helper, mirroring the channel sibling so // the two p-gate filters can't drift) plus two guard unit tests. The file was diff --git a/desktop/src-tauri/src/commands/agent_config.rs b/desktop/src-tauri/src/commands/agent_config.rs index f6141a53e..7b9577ba2 100644 --- a/desktop/src-tauri/src/commands/agent_config.rs +++ b/desktop/src-tauri/src/commands/agent_config.rs @@ -607,6 +607,7 @@ mod tests { fn agent_record() -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: vec![], pubkey: "agent".to_string(), name: "Agent".to_string(), persona_id: Some("persona-1".to_string()), @@ -663,6 +664,7 @@ mod tests { fn persona_with_model(model: &str) -> AgentDefinition { AgentDefinition { + mcp_servers: vec![], id: "persona-1".to_string(), display_name: "Persona".to_string(), avatar_url: None, @@ -844,6 +846,7 @@ mod tests { let personas: Vec = vec![]; let cache = session_cache("model-y", true); let global = crate::managed_agents::GlobalAgentConfig { + mcp_servers: vec![], model: Some("global-model".to_string()), ..Default::default() }; diff --git a/desktop/src-tauri/src/commands/agent_models.rs b/desktop/src-tauri/src/commands/agent_models.rs index 9d180539d..107392dc3 100644 --- a/desktop/src-tauri/src/commands/agent_models.rs +++ b/desktop/src-tauri/src/commands/agent_models.rs @@ -872,6 +872,10 @@ pub async fn update_managed_agent( crate::managed_agents::validate_user_env_keys(&env_vars)?; record.env_vars = env_vars; } + if let Some(mcp_servers) = input.mcp_servers { + crate::managed_agents::validate_mcp_servers(&mcp_servers)?; + record.mcp_servers = mcp_servers; + } // Inbound author gate: merge patch onto current values, then validate // the merged state. This lets a single update switch to Allowlist AND diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index daa448cc6..a9e15d35e 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -412,6 +412,7 @@ pub async fn create_managed_agent( } } crate::managed_agents::validate_user_env_keys(&input.env_vars)?; + crate::managed_agents::validate_mcp_servers(&input.mcp_servers)?; // Validate & normalize the respond-to allowlist BEFORE any side effects. // The harness has its own validator (buzz-acp/src/config.rs) but we want @@ -716,6 +717,7 @@ pub async fn create_managed_agent( persona_team_dir: None, persona_name_in_team: None, env_vars: input.env_vars.clone(), + mcp_servers: input.mcp_servers.clone(), created_at: now_iso(), updated_at: now_iso(), last_started_at: None, diff --git a/desktop/src-tauri/src/commands/agents_deploy.rs b/desktop/src-tauri/src/commands/agents_deploy.rs index be132225e..e1911bef6 100644 --- a/desktop/src-tauri/src/commands/agents_deploy.rs +++ b/desktop/src-tauri/src/commands/agents_deploy.rs @@ -93,6 +93,13 @@ pub(super) fn build_deploy_payload( effective_model.map(str::to_string), effective_provider.map(str::to_string), ); + let effective_command = crate::managed_agents::record_agent_command(record, &personas); + let effective_mcp_servers = crate::managed_agents::effective_buzz_agent_mcp_servers( + record, + &personas, + &global_config.mcp_servers, + &effective_command, + )?; Ok(deploy_payload_json( record, @@ -103,6 +110,7 @@ pub(super) fn build_deploy_payload( effective_model, effective_provider, merged_env, + crate::managed_agents::mcp_server_transport(effective_mcp_servers), )) } @@ -115,6 +123,7 @@ pub(super) fn deploy_payload_json( effective_model: Option, effective_provider: Option, merged_env: std::collections::BTreeMap, + effective_mcp_servers: Vec, ) -> serde_json::Value { serde_json::json!({ "name": &record.name, @@ -145,5 +154,7 @@ pub(super) fn deploy_payload_json( // Merged persona + agent env vars. Providers that don't read this // field will simply ignore it — no protocol break. "env_vars": merged_env, + // Local MCP configuration resolves exactly like the local spawn path. + "mcp_servers": effective_mcp_servers, }) } diff --git a/desktop/src-tauri/src/commands/agents_tests.rs b/desktop/src-tauri/src/commands/agents_tests.rs index eb673353f..517001800 100644 --- a/desktop/src-tauri/src/commands/agents_tests.rs +++ b/desktop/src-tauri/src/commands/agents_tests.rs @@ -1,5 +1,5 @@ use super::*; -use crate::managed_agents::AgentDefinition; +use crate::managed_agents::{mcp_server_transport, AgentDefinition, McpServerConfig}; fn bare_agent_record( persona_id: Option<&str>, @@ -9,6 +9,7 @@ fn bare_agent_record( use crate::managed_agents::{BackendKind, RespondTo}; use std::collections::BTreeMap; ManagedAgentRecord { + mcp_servers: vec![], pubkey: "agent".to_string(), name: "Agent".to_string(), persona_id: persona_id.map(str::to_string), @@ -65,6 +66,7 @@ fn bare_agent_record( fn persona_record(id: &str, model: Option<&str>, provider: Option<&str>) -> AgentDefinition { use std::collections::BTreeMap; AgentDefinition { + mcp_servers: vec![], id: id.to_string(), display_name: "Test Persona".to_string(), avatar_url: None, @@ -140,6 +142,7 @@ fn deploy_resolver_falls_back_to_global_when_persona_and_record_have_none() { let record = bare_agent_record(Some("p1"), None, None); let personas = vec![persona_record("p1", None, None)]; let global = crate::managed_agents::GlobalAgentConfig { + mcp_servers: vec![], model: Some("global-model".to_string()), provider: Some("global-prov".to_string()), ..Default::default() @@ -365,6 +368,13 @@ fn deploy_payload_carries_the_full_behavioral_quad() { Some("gpt-x".to_string()), Some("openai".to_string()), std::collections::BTreeMap::new(), + mcp_server_transport(vec![McpServerConfig { + name: "example".to_string(), + command: "example-mcp".to_string(), + args: vec![], + env: vec![], + enabled: true, + }]), ); assert_eq!(payload["parallelism"], 4); @@ -373,4 +383,6 @@ fn deploy_payload_carries_the_full_behavioral_quad() { assert_eq!(payload["model"], "gpt-x"); assert_eq!(payload["provider"], "openai"); assert_eq!(payload["relay_url"], "wss://relay.example"); + assert_eq!(payload["mcp_servers"][0]["name"], "example"); + assert!(payload["mcp_servers"][0].get("enabled").is_none()); } diff --git a/desktop/src-tauri/src/commands/personas/delete_cascade_tests.rs b/desktop/src-tauri/src/commands/personas/delete_cascade_tests.rs index 316af5f72..a4a0a3f8c 100644 --- a/desktop/src-tauri/src/commands/personas/delete_cascade_tests.rs +++ b/desktop/src-tauri/src/commands/personas/delete_cascade_tests.rs @@ -17,6 +17,7 @@ fn make_agent( runtime_pid: Option, ) -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: vec![], pubkey: pubkey.to_string(), name: "Test Agent".to_string(), persona_id: persona_id.map(str::to_string), diff --git a/desktop/src-tauri/src/commands/personas/inbound_tests.rs b/desktop/src-tauri/src/commands/personas/inbound_tests.rs index 6c7012e7e..b15d67f44 100644 --- a/desktop/src-tauri/src/commands/personas/inbound_tests.rs +++ b/desktop/src-tauri/src/commands/personas/inbound_tests.rs @@ -10,6 +10,7 @@ const UUID: &str = "11111111-2222-3333-4444-555555555555"; /// IS its UUID id. Carries env_vars + source_team that must survive a patch. fn local_in_app() -> AgentDefinition { AgentDefinition { + mcp_servers: vec![], id: UUID.to_string(), display_name: "Local".to_string(), avatar_url: None, @@ -35,6 +36,7 @@ fn local_in_app() -> AgentDefinition { /// slug = Some(d-tag), empty env_vars, source_team None. fn inbound_for(d_tag: &str, display_name: &str) -> AgentDefinition { AgentDefinition { + mcp_servers: vec![], id: d_tag.to_string(), display_name: display_name.to_string(), avatar_url: Some("https://example.com/a.png".to_string()), @@ -155,6 +157,7 @@ const AGENT_PUBKEY: &str = "agentpubkeyhex00000000000000000000000000000000000000 /// event must NEVER be able to overwrite. fn local_agent() -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: vec![], pubkey: AGENT_PUBKEY.to_string(), name: "Local Agent".to_string(), persona_id: Some("persona-local".to_string()), diff --git a/desktop/src-tauri/src/commands/personas/mod.rs b/desktop/src-tauri/src/commands/personas/mod.rs index 5ae6fbf48..ad3f0b0f7 100644 --- a/desktop/src-tauri/src/commands/personas/mod.rs +++ b/desktop/src-tauri/src/commands/personas/mod.rs @@ -77,6 +77,7 @@ pub async fn create_persona( .filter(|s| !s.is_empty()) .collect(); crate::managed_agents::validate_user_env_keys(&input.env_vars)?; + crate::managed_agents::validate_mcp_servers(&input.mcp_servers)?; let mut persona = AgentDefinition { id: Uuid::new_v4().to_string(), display_name, @@ -91,6 +92,7 @@ pub async fn create_persona( source_team: None, source_team_persona_slug: None, env_vars: input.env_vars, + mcp_servers: input.mcp_servers, respond_to: None, respond_to_allowlist: Vec::new(), parallelism: None, @@ -200,6 +202,10 @@ pub async fn update_persona( crate::managed_agents::validate_user_env_keys(&env_vars)?; persona.env_vars = env_vars; } + crate::managed_agents::replace_mcp_servers( + &mut persona.mcp_servers, + &input.mcp_servers, + )?; apply_persona_behavior(persona, input.behavior)?; persona.updated_at = now_iso(); diff --git a/desktop/src-tauri/src/commands/personas/name_propagation_tests.rs b/desktop/src-tauri/src/commands/personas/name_propagation_tests.rs index ba855ccbd..2ee21fbe3 100644 --- a/desktop/src-tauri/src/commands/personas/name_propagation_tests.rs +++ b/desktop/src-tauri/src/commands/personas/name_propagation_tests.rs @@ -5,6 +5,7 @@ use super::*; fn agent(persona_id: &str, name: &str, display_name: Option<&str>) -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: vec![], pubkey: format!("pubkey-{name}"), name: name.to_string(), persona_id: Some(persona_id.to_string()), diff --git a/desktop/src-tauri/src/commands/personas/snapshot/import.rs b/desktop/src-tauri/src/commands/personas/snapshot/import.rs index 9e2b8b392..bf2392c4d 100644 --- a/desktop/src-tauri/src/commands/personas/snapshot/import.rs +++ b/desktop/src-tauri/src/commands/personas/snapshot/import.rs @@ -411,6 +411,7 @@ pub async fn confirm_agent_snapshot_import( source_team: None, source_team_persona_slug: None, env_vars: std::collections::BTreeMap::new(), + mcp_servers: Vec::new(), respond_to: respond_to_wire.clone(), respond_to_allowlist: minted.respond_to_allowlist.clone(), parallelism: minted_parallelism, @@ -453,6 +454,7 @@ pub async fn confirm_agent_snapshot_import( provider: snapshot.definition.provider.clone(), persona_source_version: None, env_vars: std::collections::BTreeMap::new(), + mcp_servers: Vec::new(), start_on_app_launch: false, auto_restart_on_config_change: true, runtime_pid: None, diff --git a/desktop/src-tauri/src/commands/personas/snapshot/tests.rs b/desktop/src-tauri/src/commands/personas/snapshot/tests.rs index b1d19f06b..58de423bc 100644 --- a/desktop/src-tauri/src/commands/personas/snapshot/tests.rs +++ b/desktop/src-tauri/src/commands/personas/snapshot/tests.rs @@ -42,6 +42,7 @@ fn make_definition(slug: &str) -> ManagedAgentRecord { provider: None, persona_source_version: None, env_vars: BTreeMap::new(), + mcp_servers: Vec::new(), start_on_app_launch: false, auto_restart_on_config_change: false, runtime_pid: None, diff --git a/desktop/src-tauri/src/managed_agents/agent_events.rs b/desktop/src-tauri/src/managed_agents/agent_events.rs index ba4407d16..03709a4f8 100644 --- a/desktop/src-tauri/src/managed_agents/agent_events.rs +++ b/desktop/src-tauri/src/managed_agents/agent_events.rs @@ -179,6 +179,16 @@ mod tests { provider: Some("anthropic".to_string()), persona_source_version: Some("abc123".to_string()), env_vars: BTreeMap::from([("OPENAI_API_KEY".to_string(), "sk-secret".to_string())]), + mcp_servers: vec![super::super::McpServerConfig { + name: "secret-mcp".to_string(), + command: "mcp-command".to_string(), + args: vec!["--secret".to_string()], + env: vec![super::super::McpServerEnvVar { + name: "MCP_TOKEN".to_string(), + value: "mcp-secret".to_string(), + }], + enabled: true, + }], start_on_app_launch: true, auto_restart_on_config_change: true, runtime_pid: Some(4242), @@ -260,6 +270,9 @@ mod tests { assert!(!json.contains("OPENAI_API_KEY"), "leaked env var key"); assert!(!json.contains("sk-secret"), "leaked env var value"); assert!(!json.contains("env_vars"), "leaked env var field"); + assert!(!json.contains("mcp_servers"), "leaked MCP server field"); + assert!(!json.contains("secret-mcp"), "leaked MCP server name"); + assert!(!json.contains("mcp-secret"), "leaked MCP server secret"); assert!( !json.contains("sk-provider-secret"), "leaked provider config secret" diff --git a/desktop/src-tauri/src/managed_agents/agent_snapshot.rs b/desktop/src-tauri/src/managed_agents/agent_snapshot.rs index b0bf8f599..f42a78367 100644 --- a/desktop/src-tauri/src/managed_agents/agent_snapshot.rs +++ b/desktop/src-tauri/src/managed_agents/agent_snapshot.rs @@ -21,7 +21,7 @@ //! The following fields are NEVER serialized: //! - `private_key_nsec` / any private key material //! - `auth_tag` (NIP-OA) -//! - `env_vars` (API keys / credentials) +//! - `env_vars` and `mcp_servers` (API keys / credentials) //! - `relay_url` (machine-local endpoint) //! - `acp_command` / `agent_command` / `agent_command_override` / `agent_args` //! (machine-local harness paths) @@ -500,6 +500,16 @@ mod tests { m.insert("API_KEY".to_string(), "secret123".to_string()); // MUST NOT appear m }, + mcp_servers: vec![crate::managed_agents::McpServerConfig { + name: "SENTINEL_MCP_NAME".to_string(), + command: "SENTINEL_MCP_COMMAND".to_string(), + args: Vec::new(), + env: vec![crate::managed_agents::McpServerEnvVar { + name: "SENTINEL_MCP_TOKEN".to_string(), + value: "SENTINEL_MCP_SECRET".to_string(), + }], + enabled: true, + }], // MUST NOT appear in snapshot start_on_app_launch: true, auto_restart_on_config_change: true, runtime_pid: Some(12345), // MUST NOT appear @@ -744,6 +754,18 @@ mod tests { ); } + #[test] + fn secret_exclusion_mcp_servers_absent() { + let json = snapshot_json_string(&minimal_record()); + assert!( + !json.contains("mcpServers") + && !json.contains("mcp_servers") + && !json.contains("SENTINEL_MCP_NAME") + && !json.contains("SENTINEL_MCP_SECRET"), + "local MCP configuration must not appear in snapshots" + ); + } + #[test] fn secret_exclusion_relay_url_absent() { let record = minimal_record(); diff --git a/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests.rs b/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests.rs index f5fc7c389..85a8b7ae6 100644 --- a/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests.rs +++ b/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests.rs @@ -81,6 +81,7 @@ fn test_record() -> ManagedAgentRecord { system_prompt: None, model: None, env_vars: BTreeMap::new(), + mcp_servers: Vec::new(), start_on_app_launch: false, auto_restart_on_config_change: true, runtime_pid: None, diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests.rs b/desktop/src-tauri/src/managed_agents/discovery/tests.rs index 608b511fa..94c12d766 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests.rs @@ -191,6 +191,7 @@ fn classifies_cli_missing_when_adapter_found_but_cli_absent() { fn persona_with_runtime(id: &str, runtime: Option<&str>) -> crate::managed_agents::AgentDefinition { crate::managed_agents::AgentDefinition { + mcp_servers: vec![], id: id.to_string(), display_name: id.to_string(), avatar_url: None, @@ -230,6 +231,7 @@ fn record_with( override_cmd: Option<&str>, ) -> crate::managed_agents::types::ManagedAgentRecord { crate::managed_agents::types::ManagedAgentRecord { + mcp_servers: vec![], pubkey: String::new(), name: "r".to_string(), persona_id: persona_id.map(str::to_string), diff --git a/desktop/src-tauri/src/managed_agents/env_vars.rs b/desktop/src-tauri/src/managed_agents/env_vars.rs index 6318a5c83..08f382389 100644 --- a/desktop/src-tauri/src/managed_agents/env_vars.rs +++ b/desktop/src-tauri/src/managed_agents/env_vars.rs @@ -71,6 +71,9 @@ pub(crate) const RESERVED_ENV_KEYS: &[&str] = &[ "BUZZ_ACP_AGENT_COMMAND", "BUZZ_ACP_AGENT_ARGS", "BUZZ_ACP_MCP_COMMAND", + // Structured MCP configuration transport. Must only be set by desktop + // after resolving local-only global/definition/agent layers. + "BUZZ_ACP_MCP_SERVERS", // Security gates: respond-to mode + allowlist + legacy owner-only // fallback. Overriding would make the running agent's gate diverge // from the saved/UI-visible settings. diff --git a/desktop/src-tauri/src/managed_agents/global_config/mod.rs b/desktop/src-tauri/src/managed_agents/global_config/mod.rs index cf8629f82..70ddb6777 100644 --- a/desktop/src-tauri/src/managed_agents/global_config/mod.rs +++ b/desktop/src-tauri/src/managed_agents/global_config/mod.rs @@ -30,7 +30,9 @@ use crate::managed_agents::env_vars::{ validate_user_env_keys, DERIVED_PROVIDER_MODEL_ENV_KEYS, MAX_ENV_VALUE_BYTES, }; use crate::managed_agents::storage::{atomic_write_json_restricted, managed_agents_base_dir}; -use crate::managed_agents::types::{AgentDefinition, ManagedAgentRecord}; +use crate::managed_agents::types::{ + validate_mcp_servers, AgentDefinition, ManagedAgentRecord, McpServerConfig, +}; /// The global agent configuration record. /// @@ -50,6 +52,10 @@ pub struct GlobalAgentConfig { #[serde(default)] pub env_vars: BTreeMap, + /// Local-only MCP servers inherited by every buzz-agent instance. + #[serde(default)] + pub mcp_servers: Vec, + /// Global fallback provider (e.g. `"databricks_v2"`, `"anthropic"`). /// /// Used only when neither the agent record nor the linked persona specifies @@ -93,6 +99,8 @@ pub fn validate_global_config(config: &GlobalAgentConfig) -> Result<(), String> // Standard env-var key validation (POSIX shape, reserved-key check, NUL/size caps). validate_user_env_keys(&non_empty)?; + validate_mcp_servers(&config.mcp_servers)?; + // Reject derived provider/model keys in global env_vars. let derived: Vec<&str> = non_empty .keys() diff --git a/desktop/src-tauri/src/managed_agents/global_config/tests.rs b/desktop/src-tauri/src/managed_agents/global_config/tests.rs index 24027b75f..f4e4c068f 100644 --- a/desktop/src-tauri/src/managed_agents/global_config/tests.rs +++ b/desktop/src-tauri/src/managed_agents/global_config/tests.rs @@ -8,6 +8,7 @@ use crate::managed_agents::{AgentDefinition, BackendKind, ManagedAgentRecord, Re fn config_with_env(pairs: &[(&str, &str)]) -> GlobalAgentConfig { GlobalAgentConfig { + mcp_servers: vec![], env_vars: pairs .iter() .map(|(k, v)| (k.to_string(), v.to_string())) @@ -92,6 +93,7 @@ fn validate_ignores_empty_values_for_reserved_key_check() { #[test] fn validate_rejects_provider_with_nul_byte() { let config = GlobalAgentConfig { + mcp_servers: vec![], provider: Some("anthropic\0evil".to_string()), ..Default::default() }; @@ -105,6 +107,7 @@ fn validate_rejects_provider_with_nul_byte() { #[test] fn validate_rejects_model_with_nul_byte() { let config = GlobalAgentConfig { + mcp_servers: vec![], model: Some("claude-opus-4\0evil".to_string()), ..Default::default() }; @@ -119,6 +122,7 @@ fn validate_rejects_model_with_nul_byte() { fn validate_rejects_provider_exceeding_size_cap() { use crate::managed_agents::env_vars::MAX_ENV_VALUE_BYTES; let config = GlobalAgentConfig { + mcp_servers: vec![], provider: Some("x".repeat(MAX_ENV_VALUE_BYTES + 1)), ..Default::default() }; @@ -133,6 +137,7 @@ fn validate_rejects_provider_exceeding_size_cap() { fn validate_rejects_model_exceeding_size_cap() { use crate::managed_agents::env_vars::MAX_ENV_VALUE_BYTES; let config = GlobalAgentConfig { + mcp_servers: vec![], model: Some("x".repeat(MAX_ENV_VALUE_BYTES + 1)), ..Default::default() }; @@ -146,6 +151,7 @@ fn validate_rejects_model_exceeding_size_cap() { #[test] fn validate_accepts_valid_provider_and_model() { let config = GlobalAgentConfig { + mcp_servers: vec![], provider: Some("anthropic".to_string()), model: Some("claude-opus-4-5".to_string()), ..Default::default() @@ -158,6 +164,7 @@ fn validate_accepts_valid_provider_and_model() { #[test] fn normalize_some_empty_provider_becomes_none() { let mut config = GlobalAgentConfig { + mcp_servers: vec![], provider: Some("".to_string()), ..Default::default() }; @@ -171,6 +178,7 @@ fn normalize_some_empty_provider_becomes_none() { #[test] fn normalize_whitespace_only_provider_becomes_none() { let mut config = GlobalAgentConfig { + mcp_servers: vec![], provider: Some(" ".to_string()), ..Default::default() }; @@ -184,6 +192,7 @@ fn normalize_whitespace_only_provider_becomes_none() { #[test] fn normalize_some_empty_model_becomes_none() { let mut config = GlobalAgentConfig { + mcp_servers: vec![], model: Some("".to_string()), ..Default::default() }; @@ -197,6 +206,7 @@ fn normalize_some_empty_model_becomes_none() { #[test] fn normalize_whitespace_only_model_becomes_none() { let mut config = GlobalAgentConfig { + mcp_servers: vec![], model: Some(" \t ".to_string()), ..Default::default() }; @@ -210,6 +220,7 @@ fn normalize_whitespace_only_model_becomes_none() { #[test] fn normalize_valid_provider_and_model_unchanged() { let mut config = GlobalAgentConfig { + mcp_servers: vec![], provider: Some("anthropic".to_string()), model: Some("claude-opus-4-5".to_string()), ..Default::default() @@ -256,6 +267,7 @@ fn strip_is_idempotent_on_all_non_empty() { fn default_config_is_all_none_empty() { let config = GlobalAgentConfig::default(); assert!(config.env_vars.is_empty()); + assert!(config.mcp_servers.is_empty()); assert!(config.provider.is_none()); assert!(config.model.is_none()); } @@ -263,6 +275,7 @@ fn default_config_is_all_none_empty() { #[test] fn roundtrip_serialization() { let config = GlobalAgentConfig { + mcp_servers: vec![], env_vars: BTreeMap::from([("ANTHROPIC_API_KEY".to_string(), "sk-test".to_string())]), provider: Some("anthropic".to_string()), model: Some("claude-opus-4".to_string()), @@ -284,6 +297,10 @@ fn default_global_config_serializes_all_fields() { json.contains("\"env_vars\""), "serialized JSON must always include env_vars; got: {json}" ); + assert!( + json.contains("\"mcp_servers\""), + "serialized JSON must always include mcp_servers; got: {json}" + ); assert!( json.contains("\"provider\""), "serialized JSON must always include provider; got: {json}" @@ -298,6 +315,7 @@ fn default_global_config_serializes_all_fields() { fn bare_record() -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: vec![], pubkey: "agent".to_string(), name: "Agent".to_string(), persona_id: None, @@ -354,6 +372,7 @@ fn bare_record() -> ManagedAgentRecord { fn persona(id: &str, model: Option<&str>, provider: Option<&str>) -> AgentDefinition { AgentDefinition { + mcp_servers: vec![], id: id.to_string(), display_name: "Test Persona".to_string(), avatar_url: None, @@ -390,6 +409,7 @@ fn resolve_agent_record_wins_over_persona_and_global() { Some("persona-provider"), )]; let global = GlobalAgentConfig { + mcp_servers: vec![], model: Some("global-model".to_string()), provider: Some("global-provider".to_string()), ..Default::default() @@ -419,6 +439,7 @@ fn resolve_persona_fallback_when_record_has_none() { Some("persona-provider"), )]; let global = GlobalAgentConfig { + mcp_servers: vec![], model: Some("global-model".to_string()), provider: Some("global-provider".to_string()), ..Default::default() @@ -449,6 +470,7 @@ fn resolve_global_fallback_when_record_and_persona_have_none() { // record.model / provider = None; persona.model / provider = None let personas = vec![persona("p1", None, None)]; let global = GlobalAgentConfig { + mcp_servers: vec![], model: Some("global-model".to_string()), provider: Some("global-provider".to_string()), ..Default::default() @@ -475,6 +497,7 @@ fn resolve_global_fallback_when_no_persona_linked() { let record = bare_record(); // persona_id = None, model/provider = None let personas: Vec = vec![]; let global = GlobalAgentConfig { + mcp_servers: vec![], model: Some("global-model".to_string()), provider: Some("global-provider".to_string()), ..Default::default() @@ -519,6 +542,7 @@ fn resolve_each_field_resolves_independently_through_tiers() { // persona.model = None → global fills model if record also had none, but // record has model here so global is not needed for model. let global = GlobalAgentConfig { + mcp_servers: vec![], model: Some("global-model".to_string()), provider: Some("global-provider".to_string()), ..Default::default() @@ -541,6 +565,7 @@ fn resolve_each_field_resolves_independently_through_tiers() { #[test] fn populated_global_config_round_trips() { let original = GlobalAgentConfig { + mcp_servers: vec![], env_vars: [("ANTHROPIC_API_KEY".to_string(), "sk-test".to_string())] .into_iter() .collect(), @@ -572,6 +597,7 @@ fn record_runtime_wins_over_persona_runtime_for_command_resolution() { record.persona_id = Some("p1".to_string()); let persona = AgentDefinition { + mcp_servers: vec![], id: "p1".to_string(), display_name: "Goose persona".to_string(), avatar_url: None, diff --git a/desktop/src-tauri/src/managed_agents/nest/tests.rs b/desktop/src-tauri/src/managed_agents/nest/tests.rs index a95938160..46a0bbad3 100644 --- a/desktop/src-tauri/src/managed_agents/nest/tests.rs +++ b/desktop/src-tauri/src/managed_agents/nest/tests.rs @@ -412,6 +412,7 @@ fn ensure_cli_symlink_does_not_clobber_regular_file_dev() { fn make_persona(id: &str, display_name: &str) -> AgentDefinition { AgentDefinition { + mcp_servers: vec![], id: id.to_string(), display_name: display_name.to_string(), avatar_url: None, @@ -435,6 +436,7 @@ fn make_persona(id: &str, display_name: &str) -> AgentDefinition { fn make_agent(name: &str, persona_id: Option<&str>) -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: vec![], pubkey: String::new(), name: name.to_string(), persona_id: persona_id.map(|s| s.to_string()), diff --git a/desktop/src-tauri/src/managed_agents/persona_events.rs b/desktop/src-tauri/src/managed_agents/persona_events.rs index 02b281523..fe760431b 100644 --- a/desktop/src-tauri/src/managed_agents/persona_events.rs +++ b/desktop/src-tauri/src/managed_agents/persona_events.rs @@ -191,6 +191,7 @@ pub fn persona_from_event(event: &nostr::Event) -> Result AgentDefinition { source_team: None, source_team_persona_slug: Some("test-slug".to_string()), env_vars: BTreeMap::from([("KEY".to_string(), "value".to_string())]), + mcp_servers: vec![crate::managed_agents::McpServerConfig { + name: "local-mcp".to_string(), + command: "mcp-command".to_string(), + args: vec![], + env: vec![crate::managed_agents::McpServerEnvVar { + name: "MCP_TOKEN".to_string(), + value: "mcp-secret".to_string(), + }], + enabled: true, + }], respond_to: None, respond_to_allowlist: Vec::new(), parallelism: None, @@ -132,9 +142,13 @@ fn round_trip_serialization() { assert_eq!(restored.model, Some("claude-opus-4".to_string())); assert_eq!(restored.provider, Some("anthropic".to_string())); assert_eq!(restored.name_pool, vec!["Alpha", "Beta"]); - // env_vars are not included in public persona events (secrets travel - // via NIP-44-encrypted engrams only). + // Env vars and MCP servers are local-only and absent from public events. assert!(restored.env_vars.is_empty()); + assert!(restored.mcp_servers.is_empty()); + let wire = serde_json::to_string(&persona_event_content(&record)).unwrap(); + assert!(!wire.contains("mcp_servers")); + assert!(!wire.contains("local-mcp")); + assert!(!wire.contains("mcp-secret")); assert_eq!( restored.source_team_persona_slug, Some("test-slug".to_string()) @@ -208,6 +222,7 @@ fn content_matches_nip_ap_vector() { // signed content, so a second implementer following the spec computes // the same NIP-01 id. let record = AgentDefinition { + mcp_servers: vec![], id: "test-agent".to_string(), display_name: "Test Agent".to_string(), avatar_url: Some("https://example.com/avatar.png".to_string()), @@ -237,6 +252,7 @@ fn content_matches_nip_ap_vector() { #[test] fn round_trip_minimal_persona() { let record = AgentDefinition { + mcp_servers: vec![], id: "minimal".to_string(), display_name: "Minimal".to_string(), avatar_url: None, @@ -332,6 +348,7 @@ fn behavioral_defaults_survive_record_round_trip() { #[test] fn quad_absent_definition_hash_stable_across_activation() { let record = AgentDefinition { + mcp_servers: vec![], id: "quad-absent".to_string(), display_name: "Test".to_string(), avatar_url: None, @@ -374,6 +391,7 @@ fn quad_absent_definition_hash_stable_across_activation() { /// way `persona_from_event` maps fields, without needing a signed event. fn persona_from_event_content_for_test(content: PersonaEventContent) -> AgentDefinition { AgentDefinition { + mcp_servers: vec![], id: "staged".to_string(), display_name: content.display_name, avatar_url: content.avatar_url, @@ -506,6 +524,7 @@ fn snapshot_runtime_verbatim_from_persona() { /// Helper: a persona with no model/provider configured. fn blank_model_persona() -> AgentDefinition { AgentDefinition { + mcp_servers: vec![], model: None, provider: None, ..sample_persona() diff --git a/desktop/src-tauri/src/managed_agents/personas.rs b/desktop/src-tauri/src/managed_agents/personas.rs index 605148b19..5359f3a9c 100644 --- a/desktop/src-tauri/src/managed_agents/personas.rs +++ b/desktop/src-tauri/src/managed_agents/personas.rs @@ -237,6 +237,7 @@ fn built_in_persona_records(now: &str) -> Vec { source_team: None, source_team_persona_slug: None, env_vars: std::collections::BTreeMap::new(), + mcp_servers: Vec::new(), respond_to: None, respond_to_allowlist: Vec::new(), parallelism: None, @@ -295,6 +296,7 @@ fn merge_personas(mut stored: Vec, now: &str) -> (Vec AgentDefinition { AgentDefinition { + mcp_servers: vec![], id: id.to_string(), display_name: display_name.to_string(), avatar_url: Some("https://example.com/avatar.png".to_string()), @@ -330,6 +331,7 @@ fn migrate_retires_unmodified_personas() { let mut stored: Vec = RETIRED_PERSONAS .iter() .map(|(id, prompt)| AgentDefinition { + mcp_servers: vec![], id: id.to_string(), system_prompt: prompt.to_string(), is_builtin: false, // already demoted by merge_personas @@ -365,6 +367,7 @@ fn migrate_retires_unmodified_personas() { fn migrate_preserves_customized_personas() { let now = "2026-04-01T00:00:00Z"; let mut stored = vec![AgentDefinition { + mcp_servers: vec![], id: "builtin:researcher".to_string(), display_name: "My Researcher".to_string(), system_prompt: "My custom research workflow with special instructions".to_string(), @@ -398,6 +401,7 @@ fn migrate_is_idempotent() { // 2. Already-retired persona (display_name ends with " (retired)") — no-op. let mut stored_with_retired = vec![AgentDefinition { + mcp_servers: vec![], id: "builtin:researcher".to_string(), display_name: "Researcher (retired)".to_string(), system_prompt: "My custom prompt".to_string(), @@ -413,6 +417,7 @@ fn migrate_is_idempotent() { // 3. Retired persona still marked is_builtin: true (pre-demotion). // migrate_retired_personas should still soft-deprecate it. let mut stored_pre_demotion = vec![AgentDefinition { + mcp_servers: vec![], id: "builtin:reviewer".to_string(), display_name: "Reviewer".to_string(), system_prompt: "Custom review prompt".to_string(), diff --git a/desktop/src-tauri/src/managed_agents/readiness.rs b/desktop/src-tauri/src/managed_agents/readiness.rs index 6a5af8925..e0dbfc04e 100644 --- a/desktop/src-tauri/src/managed_agents/readiness.rs +++ b/desktop/src-tauri/src/managed_agents/readiness.rs @@ -1369,6 +1369,7 @@ mod tests { // Minimal record: only the fields resolve_effective_agent_env reads. let record = crate::managed_agents::types::ManagedAgentRecord { + mcp_servers: vec![], pubkey: "test-pubkey".to_string(), name: "test-agent".to_string(), persona_id: None, diff --git a/desktop/src-tauri/src/managed_agents/relay_mesh.rs b/desktop/src-tauri/src/managed_agents/relay_mesh.rs index 4c58344d6..94e9ecda5 100644 --- a/desktop/src-tauri/src/managed_agents/relay_mesh.rs +++ b/desktop/src-tauri/src/managed_agents/relay_mesh.rs @@ -62,6 +62,7 @@ mod tests { fn fixture() -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: vec![], pubkey: "p".into(), name: "n".into(), persona_id: None, diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 3745fe571..7f90f7421 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -1465,6 +1465,20 @@ pub(crate) fn build_respond_to_env( Ok((set, remove)) } +/// Set the desktop-resolved MCP transport only for the bundled `buzz-agent`. +/// Explicitly remove any inherited parent value for other runtimes. +fn set_buzz_agent_mcp_servers_env( + command: &mut std::process::Command, + effective_command: &str, + servers_json: String, +) { + if known_acp_runtime(effective_command).is_some_and(|runtime| runtime.id == "buzz-agent") { + command.env("BUZZ_ACP_MCP_SERVERS", servers_json); + } else { + command.env_remove("BUZZ_ACP_MCP_SERVERS"); + } +} + /// Spawn an agent process without holding any locks on records or runtimes. /// Returns the child process and log path on success. The caller is responsible /// for updating `ManagedAgentRecord` fields and inserting into the runtimes map. @@ -1505,6 +1519,15 @@ pub fn spawn_agent_child( // and for the env-var merge at spawn time. let global = crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); let effective_command = super::record_agent_command(record, &personas); + let effective_mcp_servers = super::effective_buzz_agent_mcp_servers( + record, + &personas, + &global.mcp_servers, + &effective_command, + )?; + let effective_mcp_servers_json = + serde_json::to_string(&super::mcp_server_transport(effective_mcp_servers)) + .map_err(|error| format!("failed to serialize effective MCP servers: {error}"))?; let agent_args = normalize_agent_args(&effective_command, record.agent_args.clone()); let resolved_acp_command = resolve_command(&record.acp_command) .ok_or_else(|| missing_command_message(&record.acp_command, "ACP harness command"))?; @@ -1573,6 +1596,7 @@ pub fn spawn_agent_child( command.env("BUZZ_RELAY_URL", &effective_relay_url); command.env("BUZZ_ACP_AGENT_COMMAND", &resolved_agent_command); command.env("BUZZ_ACP_AGENT_ARGS", agent_args.join(",")); + set_buzz_agent_mcp_servers_env(&mut command, &effective_command, effective_mcp_servers_json); match &resolved_mcp_command { Some(mcp_cmd) => { command.env("BUZZ_ACP_MCP_COMMAND", mcp_cmd); diff --git a/desktop/src-tauri/src/managed_agents/runtime/tests.rs b/desktop/src-tauri/src/managed_agents/runtime/tests.rs index 06d3ac6db..89c8043fc 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/tests.rs @@ -123,6 +123,7 @@ fn fixture( auth_tag: Option, ) -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: vec![], pubkey: "p".into(), name: "n".into(), persona_id: None, @@ -277,6 +278,7 @@ fn persona_with_provider( provider: Option<&str>, ) -> crate::managed_agents::AgentDefinition { crate::managed_agents::AgentDefinition { + mcp_servers: vec![], id: id.to_string(), display_name: id.to_string(), avatar_url: None, @@ -549,6 +551,30 @@ fn runtime_metadata_env_vars_injects_model_even_with_acp_model_switching() { // ── name_matches_known_binary / name_matches_interpreter tests ─────────── +#[test] +fn mcp_transport_is_absent_for_non_buzz_agent_children() { + let mut command = std::process::Command::new("true"); + command.env("BUZZ_ACP_MCP_SERVERS", "inherited"); + + super::set_buzz_agent_mcp_servers_env(&mut command, "goose", "[]".into()); + assert!( + !command + .get_envs() + .any(|(key, value)| key == "BUZZ_ACP_MCP_SERVERS" && value.is_some()), + "non-buzz-agent child must not receive MCP transport" + ); +} + +#[test] +fn mcp_transport_is_set_for_buzz_agent_children() { + let mut command = std::process::Command::new("true"); + super::set_buzz_agent_mcp_servers_env(&mut command, "buzz-agent", "[]".into()); + + assert!(command.get_envs().any(|(key, value)| { + key == "BUZZ_ACP_MCP_SERVERS" && value == Some(std::ffi::OsStr::new("[]")) + })); +} + #[test] fn name_matches_known_binary_rejects_node() { // `node` must NOT be in KNOWN_AGENT_BINARIES — adding it there would diff --git a/desktop/src-tauri/src/managed_agents/spawn_hash.rs b/desktop/src-tauri/src/managed_agents/spawn_hash.rs index 63a57ce5d..7f543de65 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash.rs @@ -88,6 +88,21 @@ pub(crate) fn spawn_config_hash( let effective_command = crate::managed_agents::record_agent_command(record, personas); let runtime_meta = known_acp_runtime(&effective_command); let effective = resolve_effective_agent_env(record, personas, runtime_meta, global); + let effective_mcp_servers = (runtime_meta.is_some_and(|runtime| runtime.id == "buzz-agent")) + .then(|| { + super::effective_buzz_agent_mcp_servers( + record, + personas, + &global.mcp_servers, + &effective_command, + ) + // Invalid legacy disk state must still affect restart detection rather than + // panicking the summary path. A real spawn rejects the same configuration. + .unwrap_or_else(|error| { + eprintln!("buzz-desktop: invalid persisted MCP server configuration: {error}"); + Vec::new() + }) + }); let mut hasher = DefaultHasher::new(); @@ -103,6 +118,7 @@ pub(crate) fn spawn_config_hash( // Effective env layering (baked floor → runtime metadata → user env). // BTreeMap iteration is ordered, so this is deterministic. effective.env.hash(&mut hasher); + effective_mcp_servers.hash(&mut hasher); // Record fields the spawn env writes read directly. The relay is hashed // resolved: a blank record relay spawns on the workspace relay, so a diff --git a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs index 9b7988162..342ef05f7 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs @@ -1,9 +1,25 @@ use super::*; -use crate::managed_agents::types::RespondTo; +use crate::managed_agents::{ + types::RespondTo, AgentDefinition, GlobalAgentConfig, McpServerConfig, McpServerEnvVar, +}; use std::collections::BTreeMap; +fn mcp_server(name: &str) -> McpServerConfig { + McpServerConfig { + name: name.into(), + command: "mcp-command".into(), + args: vec![], + env: vec![McpServerEnvVar { + name: "MCP_TOKEN".into(), + value: "secret".into(), + }], + enabled: true, + } +} + fn record() -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: vec![], pubkey: "p".repeat(64), name: "agent".into(), persona_id: None, @@ -60,6 +76,7 @@ fn record() -> ManagedAgentRecord { fn persona(id: &str, runtime: Option<&str>, prompt: &str) -> AgentDefinition { AgentDefinition { + mcp_servers: vec![], id: id.into(), display_name: id.into(), avatar_url: None, @@ -124,6 +141,74 @@ fn materializing_runtime_keeps_hash_stable() { ); } +#[test] +fn buzz_agent_mcp_edit_changes_hash_but_other_runtime_ignores_it() { + let mut buzz_agent = record(); + buzz_agent.runtime = Some("buzz-agent".into()); + let mut edited = buzz_agent.clone(); + edited.mcp_servers = vec![mcp_server("local-mcp")]; + + assert_ne!( + spawn_config_hash( + &buzz_agent, + &[], + "wss://ws.example", + &GlobalAgentConfig::default() + ), + spawn_config_hash( + &edited, + &[], + "wss://ws.example", + &GlobalAgentConfig::default() + ) + ); + + let mut goose = buzz_agent; + goose.runtime = Some("goose".into()); + let mut goose_edited = goose.clone(); + goose_edited.mcp_servers = vec![mcp_server("local-mcp")]; + assert_eq!( + spawn_config_hash( + &goose, + &[], + "wss://ws.example", + &GlobalAgentConfig::default() + ), + spawn_config_hash( + &goose_edited, + &[], + "wss://ws.example", + &GlobalAgentConfig::default() + ) + ); +} + +#[test] +fn definition_mcp_edit_changes_buzz_agent_hash_without_becoming_agent_override() { + let mut record = record(); + record.runtime = Some("buzz-agent".into()); + record.persona_id = Some("persona".into()); + let persona = persona("persona", Some("buzz-agent"), "prompt"); + let mut edited_persona = persona.clone(); + edited_persona.mcp_servers = vec![mcp_server("definition-mcp")]; + + assert_ne!( + spawn_config_hash( + &record, + &[persona], + "wss://ws.example", + &GlobalAgentConfig::default() + ), + spawn_config_hash( + &record, + &[edited_persona], + "wss://ws.example", + &GlobalAgentConfig::default() + ) + ); + assert!(record.mcp_servers.is_empty()); +} + #[test] fn record_env_var_edit_changes_hash() { let rec = record(); diff --git a/desktop/src-tauri/src/managed_agents/teams_tests.rs b/desktop/src-tauri/src/managed_agents/teams_tests.rs index 7e9c7656b..d2a2c5b2e 100644 --- a/desktop/src-tauri/src/managed_agents/teams_tests.rs +++ b/desktop/src-tauri/src/managed_agents/teams_tests.rs @@ -176,6 +176,7 @@ fn managed_agent(name: &str) -> ManagedAgentRecord { agent_command_override: None, agent_args: vec![], mcp_command: String::new(), + mcp_servers: vec![], turn_timeout_seconds: 300, idle_timeout_seconds: None, max_turn_duration_seconds: None, diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index 9cb235476..24ce7b702 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -1,6 +1,9 @@ use serde::{Deserialize, Serialize}; use std::{collections::BTreeMap, path::PathBuf, process::Child}; +mod mcp_servers; +pub(crate) use mcp_servers::*; + #[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq)] #[serde(tag = "type", rename_all = "snake_case")] pub enum BackendKind { @@ -63,6 +66,9 @@ pub struct AgentDefinition { /// Stored as a BTreeMap for deterministic on-disk ordering. #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] pub env_vars: BTreeMap, + /// Local-only MCP server layer. Never included in kind:30175 content. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub mcp_servers: Vec, /// NIP-AP behavioral defaults, stored in WIRE shape (kebab-case string, /// not the `RespondTo` enum) so `persona_event_content` is a verbatim /// copy and quad-absent records serialize byte-identically to the @@ -106,6 +112,10 @@ impl AgentDefinition { provider: self.provider, persona_source_version: None, env_vars: self.env_vars, + // Definition MCP servers stay on the definition layer and are + // resolved live at spawn/deploy. Copying them here would turn + // inheritance into a per-agent override and block later edits. + mcp_servers: Vec::new(), start_on_app_launch: false, auto_restart_on_config_change: true, runtime_pid: None, @@ -164,6 +174,7 @@ impl ManagedAgentRecord { source_team: self.source_team.clone(), source_team_persona_slug: self.source_team_persona_slug.clone(), env_vars: self.env_vars.clone(), + mcp_servers: self.mcp_servers.clone(), respond_to: self.definition_respond_to.clone(), respond_to_allowlist: self.definition_respond_to_allowlist.clone(), parallelism: self.definition_parallelism, @@ -281,6 +292,10 @@ pub struct ManagedAgentRecord { /// To "override" a persona env var: set the same key here. #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] pub env_vars: BTreeMap, + /// Local-only per-agent MCP overrides. Higher entries replace definition + /// and global entries by name; disabled entries mask inherited servers. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub mcp_servers: Vec, #[serde(default = "default_start_on_app_launch")] pub start_on_app_launch: bool, /// Auto-restart this agent when its effective spawn config drifts from diff --git a/desktop/src-tauri/src/managed_agents/types/mcp_servers.rs b/desktop/src-tauri/src/managed_agents/types/mcp_servers.rs new file mode 100644 index 000000000..13aadbfc8 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/types/mcp_servers.rs @@ -0,0 +1,192 @@ +use std::collections::{BTreeMap, HashSet}; + +use serde::{Deserialize, Serialize}; + +/// Maximum number of enabled user-configured MCP servers. The bundled +/// `buzz-dev-mcp` occupies the sixteenth `buzz-agent` MCP slot. +pub(crate) const MAX_USER_MCP_SERVERS: usize = 15; + +/// An environment variable passed to an MCP subprocess. +/// +/// This deliberately mirrors the ACP `EnvVar` wire shape so the effective +/// configuration can be serialized without translating local secrets through +/// relay event content. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Hash)] +pub struct McpServerEnvVar { + pub name: String, + pub value: String, +} + +/// A locally persisted stdio MCP server configuration. +/// +/// Entries are layered `global < definition < agent` by name. A higher layer +/// replaces an entire entry; setting `enabled` to `false` masks a lower entry. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Hash)] +pub struct McpServerConfig { + pub name: String, + /// Required for enabled servers. Disabled entries may omit it because + /// their only purpose is to mask a lower-precedence server by name. + #[serde(default)] + pub command: String, + #[serde(default)] + pub args: Vec, + #[serde(default)] + pub env: Vec, + #[serde(default = "default_mcp_server_enabled")] + pub enabled: bool, +} + +/// The resolved MCP shape sent to `buzz-acp` and provider deployments. +/// `enabled` is local layering metadata, so it deliberately does not cross +/// the process or provider boundary. +#[derive(Debug, Clone, Serialize, PartialEq, Eq)] +pub(crate) struct McpServerTransport { + pub name: String, + pub command: String, + pub args: Vec, + pub env: Vec, +} + +impl From for McpServerTransport { + fn from(server: McpServerConfig) -> Self { + Self { + name: server.name, + command: server.command, + args: server.args, + env: server.env, + } + } +} + +pub(crate) fn mcp_server_transport(servers: Vec) -> Vec { + servers.into_iter().map(Into::into).collect() +} + +fn default_mcp_server_enabled() -> bool { + true +} + +/// Validate one persisted MCP layer at every IPC save boundary. +pub(crate) fn validate_mcp_servers(servers: &[McpServerConfig]) -> Result<(), String> { + let mut names = HashSet::new(); + let mut total_bytes = 0usize; + for server in servers { + if server.name.trim().is_empty() { + return Err("MCP server name is required".to_string()); + } + if !names.insert(server.name.as_str()) { + return Err(format!("MCP server names must be unique: {}", server.name)); + } + if server.enabled && server.command.trim().is_empty() { + return Err(format!("MCP server `{}` command is required", server.name)); + } + + for (field, value) in std::iter::once(("name", &server.name)) + .chain((!server.command.is_empty()).then_some(("command", &server.command))) + .chain(server.args.iter().map(|arg| ("argument", arg))) + { + if value.contains('\0') { + return Err(format!( + "MCP server `{}` {field} cannot contain NUL bytes", + server.name + )); + } + if value.len() > crate::managed_agents::env_vars::MAX_ENV_VALUE_BYTES { + return Err(format!( + "MCP server `{}` {field} exceeds the maximum allowed length ({} bytes)", + server.name, + crate::managed_agents::env_vars::MAX_ENV_VALUE_BYTES + )); + } + total_bytes = total_bytes.saturating_add(value.len()); + } + + let mut env = BTreeMap::new(); + for variable in &server.env { + if env + .insert(variable.name.clone(), variable.value.clone()) + .is_some() + { + return Err(format!( + "MCP server `{}` has duplicate env var `{}`", + server.name, variable.name + )); + } + } + crate::managed_agents::validate_user_env_keys(&env)?; + total_bytes = total_bytes.saturating_add( + env.iter() + .map(|(key, value)| key.len() + value.len()) + .sum::(), + ); + } + + if total_bytes > crate::managed_agents::env_vars::MAX_ENV_TOTAL_BYTES { + return Err(format!( + "total MCP server payload is {total_bytes} bytes; limit is {}", + crate::managed_agents::env_vars::MAX_ENV_TOTAL_BYTES + )); + } + Ok(()) +} + +/// Validate and replace an optional MCP layer from an update request. +pub(crate) fn replace_mcp_servers( + current: &mut Vec, + replacement: &Option>, +) -> Result<(), String> { + if let Some(replacement) = replacement { + validate_mcp_servers(replacement)?; + *current = replacement.clone(); + } + Ok(()) +} + +/// Resolve global, definition, and per-agent MCP layers. Higher layers replace +/// whole entries by name, including disabled entries, so a disabled override +/// masks an inherited server instead of allowing it to leak through. +pub(crate) fn merge_mcp_servers( + global: &[McpServerConfig], + definition: &[McpServerConfig], + agent: &[McpServerConfig], +) -> Result, String> { + let mut merged = BTreeMap::new(); + for layer in [global, definition, agent] { + for server in layer { + merged.insert(server.name.clone(), server.clone()); + } + } + let effective: Vec = merged + .into_values() + .filter(|server| server.enabled) + .collect(); + if effective.len() > MAX_USER_MCP_SERVERS { + return Err(format!( + "effective MCP server count is {} but the maximum is {MAX_USER_MCP_SERVERS}", + effective.len() + )); + } + Ok(effective) +} + +/// Resolve MCP configuration only for the bundled `buzz-agent` runtime. +/// Other ACP runtimes keep their MCP configuration in their own config files. +pub(crate) fn effective_buzz_agent_mcp_servers( + record: &super::ManagedAgentRecord, + personas: &[super::AgentDefinition], + global: &[McpServerConfig], + effective_command: &str, +) -> Result, String> { + if super::super::known_acp_runtime(effective_command).map(|runtime| runtime.id) + != Some("buzz-agent") + { + return Ok(Vec::new()); + } + let definition = record + .persona_id + .as_deref() + .and_then(|id| personas.iter().find(|persona| persona.id == id)) + .map(|persona| persona.mcp_servers.as_slice()) + .unwrap_or_default(); + merge_mcp_servers(global, definition, &record.mcp_servers) +} diff --git a/desktop/src-tauri/src/managed_agents/types/requests.rs b/desktop/src-tauri/src/managed_agents/types/requests.rs index 58d60218a..8f53a2085 100644 --- a/desktop/src-tauri/src/managed_agents/types/requests.rs +++ b/desktop/src-tauri/src/managed_agents/types/requests.rs @@ -7,7 +7,7 @@ use serde::Deserialize; use super::{ default_start_on_app_launch, validate_respond_to_allowlist, AgentDefinition, BackendKind, - RelayMeshConfig, RespondTo, + McpServerConfig, RelayMeshConfig, RespondTo, }; /// The NIP-AP behavioral group as one grouped request field. @@ -88,6 +88,9 @@ pub struct CreatePersonaRequest { /// Environment variables for agents created from this persona. #[serde(default)] pub env_vars: BTreeMap, + /// Local-only MCP server layer inherited by agents created from this definition. + #[serde(default)] + pub mcp_servers: Vec, /// NIP-AP behavioral group. Absent = behavior group stays unset. #[serde(default)] pub behavior: Option, @@ -116,6 +119,9 @@ pub struct UpdatePersonaRequest { /// stored credentials when an unrelated field is edited. #[serde(default)] pub env_vars: Option>, + /// Absent = don't touch. Present = replace this local-only MCP layer. + #[serde(default)] + pub mcp_servers: Option>, /// NIP-AP behavioral group. Same absent-vs-present contract as `env_vars`: /// absent = don't touch the stored behavior group (legacy callers don't send it), /// present = validate and replace the fields as a unit. @@ -166,6 +172,9 @@ pub struct CreateManagedAgentRequest { /// Environment variables for this agent. Layered on top of persona env. #[serde(default)] pub env_vars: BTreeMap, + /// Local-only MCP overrides for this agent. + #[serde(default)] + pub mcp_servers: Vec, #[serde(default)] pub spawn_after_create: bool, #[serde(default = "default_start_on_app_launch")] @@ -207,6 +216,9 @@ pub struct UpdateManagedAgentRequest { /// Absent = don't touch. Present = replace the env_vars map entirely. #[serde(default)] pub env_vars: Option>, + /// Absent = don't touch. Present = replace this local-only MCP layer. + #[serde(default)] + pub mcp_servers: Option>, #[serde(default)] pub parallelism: Option, /// Accepted for wire compatibility; not applied to the stored record. @@ -265,6 +277,7 @@ mod tests { fn record_without_quad() -> AgentDefinition { AgentDefinition { + mcp_servers: vec![], id: "p-1".to_string(), display_name: "Test".to_string(), avatar_url: None, diff --git a/desktop/src-tauri/src/managed_agents/types/tests.rs b/desktop/src-tauri/src/managed_agents/types/tests.rs index 667a41a53..a93db0ae6 100644 --- a/desktop/src-tauri/src/managed_agents/types/tests.rs +++ b/desktop/src-tauri/src/managed_agents/types/tests.rs @@ -1,4 +1,7 @@ -use super::{AgentDefinition, ManagedAgentRecord}; +use super::{ + merge_mcp_servers, replace_mcp_servers, validate_mcp_servers, AgentDefinition, + ManagedAgentRecord, McpServerConfig, McpServerEnvVar, MAX_USER_MCP_SERVERS, +}; use std::path::PathBuf; #[test] @@ -472,6 +475,7 @@ fn sample_agent_record() -> ManagedAgentRecord { fn sample_persona() -> AgentDefinition { AgentDefinition { + mcp_servers: vec![], id: "custom:helper".to_string(), display_name: "Helper".to_string(), avatar_url: Some("https://example.com/a.png".to_string()), @@ -504,6 +508,10 @@ fn persona_into_agent_record_is_keyless_and_slugged() { assert_eq!(record.runtime.as_deref(), Some("goose")); assert_eq!(record.source_team.as_deref(), Some("team-1")); assert_eq!(record.env_vars.get("K").map(String::as_str), Some("v")); + assert!( + record.mcp_servers.is_empty(), + "definition MCP servers remain inherited" + ); } #[test] @@ -649,3 +657,91 @@ fn mint_rejects_out_of_range_input_parallelism() { "input-branch error must not blame the definition: {err}" ); } + +fn mcp_server(name: &str, command: &str, enabled: bool) -> McpServerConfig { + McpServerConfig { + name: name.into(), + command: command.into(), + args: vec![], + env: vec![], + enabled, + } +} + +#[test] +fn merge_mcp_servers_higher_layer_replaces_same_name() { + let merged = merge_mcp_servers( + &[mcp_server("shared", "global", true)], + &[mcp_server("shared", "persona", true)], + &[mcp_server("shared", "agent", true)], + ) + .unwrap(); + + assert_eq!(merged, vec![mcp_server("shared", "agent", true)]); +} + +#[test] +fn merge_mcp_servers_disabled_override_masks_lower_server() { + let merged = merge_mcp_servers( + &[mcp_server("shared", "global", true)], + &[], + &[mcp_server("shared", "agent", false)], + ) + .unwrap(); + + assert!(merged.is_empty()); +} + +#[test] +fn disabled_mask_requires_only_a_name() { + let mask = McpServerConfig { + name: "shared".into(), + command: String::new(), + args: vec![], + env: vec![], + enabled: false, + }; + validate_mcp_servers(&[mask.clone()]).unwrap(); + assert!( + merge_mcp_servers(&[mcp_server("shared", "global", true)], &[], &[mask]) + .unwrap() + .is_empty() + ); +} + +#[test] +fn replace_mcp_servers_preserves_absent_and_replaces_present_empty_layer() { + let mut current = vec![mcp_server("existing", "mcp", true)]; + replace_mcp_servers(&mut current, &None).unwrap(); + assert_eq!(current, vec![mcp_server("existing", "mcp", true)]); + + replace_mcp_servers(&mut current, &Some(vec![])).unwrap(); + assert!(current.is_empty()); +} + +#[test] +fn merge_mcp_servers_rejects_more_than_fifteen_enabled_servers_after_merge() { + let global: Vec<_> = (0..MAX_USER_MCP_SERVERS) + .map(|index| mcp_server(&format!("global-{index}"), "mcp", true)) + .collect(); + let error = merge_mcp_servers(&global, &[mcp_server("persona", "mcp", true)], &[]) + .expect_err("post-merge cap must include servers from all layers"); + + assert!(error.contains("effective MCP server count is 16")); +} + +#[test] +fn validate_mcp_servers_rejects_empty_duplicate_and_reserved_env_inputs() { + assert!(validate_mcp_servers(&[mcp_server("", "mcp", true)]).is_err()); + assert!(validate_mcp_servers(&[ + mcp_server("same", "mcp", true), + mcp_server("same", "other", true), + ]) + .is_err()); + let mut server = mcp_server("server", "mcp", true); + server.env.push(McpServerEnvVar { + name: "BUZZ_ACP_MCP_SERVERS".into(), + value: "spoof".into(), + }); + assert!(validate_mcp_servers(&[server]).is_err()); +} From b1ba4559969b546956b81b87c9491bfe813ff5de Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Sun, 12 Jul 2026 22:00:41 -0400 Subject: [PATCH 02/17] fix(desktop): address pass-1 review findings on MCP server configuration F1: Extend env_secrets_from_request in backend.rs to harvest mcp_servers[*].env[*].value alongside env_vars, so provider errors that echo MCP env values are redacted from desktop-visible last_error. Add two regression tests: one verifying harvest coverage, one verifying end-to-end redaction. F2: Extend validate_mcp_servers in types/mcp_servers.rs to mirror the buzz-agent name grammar (1-128 bytes, ASCII alnum/_/-, no __) and reserve the built-in buzz-dev-mcp name. Both checks fire at every IPC save boundary, so names that would fail at spawn are rejected at edit time. Add tests covering invalid chars, double-underscore, reserved name, valid grammar. F3: Restructure build_mcp_servers in buzz-acp/src/lib.rs so the built-in server push is conditional on mcp_command being non-empty, but configured_mcp_servers are always appended. Previously the early-return on empty mcp_command silently discarded user-configured servers even though they don't depend on the built-in slot. Update the test that pinned the old drop behavior; add a complementary empty-with-no-servers test. F4: Add an enabled-server count check to validate_mcp_servers: a layer with more than MAX_USER_MCP_SERVERS enabled servers is rejected at save time rather than only at merge/spawn. Add boundary tests (15 passes, 16 fails). MINOR: Add mcp_servers to the MUST NEVER list in agent_events.rs so future edits don't miss the local-only invariant. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-acp/src/lib.rs | 98 ++++++++------- .../src/managed_agents/agent_events.rs | 1 + .../src-tauri/src/managed_agents/backend.rs | 112 ++++++++++++++++-- .../src/managed_agents/types/mcp_servers.rs | 64 +++++++++- .../src/managed_agents/types/tests.rs | 47 ++++++++ 5 files changed, 270 insertions(+), 52 deletions(-) diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index 50245354c..59635dcbb 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -3403,48 +3403,47 @@ async fn run_models(args: ModelsArgs) -> Result<()> { fn build_mcp_servers(config: &Config) -> Vec { let mut servers = Vec::new(); - if config.mcp_command.is_empty() { - return servers; - } - servers.push(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, - }); + if !config.mcp_command.is_empty() { + servers.push(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, + }); + } } - } - env - }, - }); + env + }, + }); + } servers.extend( config .configured_mcp_servers @@ -4008,7 +4007,7 @@ mod build_mcp_servers_tests { } #[test] - fn empty_mcp_command_returns_no_servers_even_when_user_servers_are_configured() { + fn empty_mcp_command_omits_builtin_but_includes_user_servers() { let mut config = test_config(); config.mcp_command = "".into(); config.configured_mcp_servers = vec![config::ConfiguredMcpServer { @@ -4018,9 +4017,22 @@ mod build_mcp_servers_tests { env: vec![], }]; + let servers = build_mcp_servers(&config); + // The built-in slot is omitted (mcp_command is empty), but user-configured + // servers are always included — they do not depend on the built-in server. + assert_eq!(servers.len(), 1); + assert_eq!(servers[0].name, "github"); + } + + #[test] + fn empty_mcp_command_with_no_user_servers_returns_empty() { + let mut config = test_config(); + config.mcp_command = "".into(); + config.configured_mcp_servers = vec![]; + assert!( build_mcp_servers(&config).is_empty(), - "without the built-in MCP server, user servers must not reach an invalid ACP invocation" + "no built-in and no user servers should yield an empty list" ); } diff --git a/desktop/src-tauri/src/managed_agents/agent_events.rs b/desktop/src-tauri/src/managed_agents/agent_events.rs index 03709a4f8..0737a2830 100644 --- a/desktop/src-tauri/src/managed_agents/agent_events.rs +++ b/desktop/src-tauri/src/managed_agents/agent_events.rs @@ -18,6 +18,7 @@ //! - `private_key_nsec` — the agent's secret key. //! - `auth_tag` — the NIP-OA owner attestation. //! - `env_vars` — may hold API keys / credentials. +//! - `mcp_servers` — env entries may hold API keys / credentials. //! - `backend` — `Provider { config }` is an opaque blob that may hold secrets. //! - any runtime field (`runtime_pid`, `last_*`, `backend_agent_id`, …) — these //! mutate on every start/stop and describe transient process state. diff --git a/desktop/src-tauri/src/managed_agents/backend.rs b/desktop/src-tauri/src/managed_agents/backend.rs index 0eada757d..2ab467769 100644 --- a/desktop/src-tauri/src/managed_agents/backend.rs +++ b/desktop/src-tauri/src/managed_agents/backend.rs @@ -318,14 +318,22 @@ fn redact_secrets_with(s: &str, extras: &[&str]) -> String { result } -/// Collect string values from `request["agent"]["env_vars"]` (if present) -/// to feed into [`redact_secrets_with`]. Returns an empty Vec if the -/// request shape doesn't match, which is fine — falls back to the default -/// prefix-based scrubbing. +/// Collect secret values from a provider deploy request to feed into +/// [`redact_secrets_with`]. Harvests: +/// - `request["agent"]["env_vars"]` — flat string map of env values. +/// - `request["agent"]["mcp_servers"][*]["env"][*]["value"]` — per-server env +/// values, which may hold API keys for third-party MCP servers. +/// +/// Returns an empty Vec if the request shape doesn't match, which is fine — +/// falls back to the default prefix-based scrubbing. fn env_secrets_from_request(request: &serde_json::Value) -> Vec { - request - .get("agent") - .and_then(|a| a.get("env_vars")) + let agent = match request.get("agent") { + Some(a) => a, + None => return Vec::new(), + }; + + let mut secrets: Vec = agent + .get("env_vars") .and_then(|e| e.as_object()) .map(|obj| { obj.values() @@ -334,7 +342,25 @@ fn env_secrets_from_request(request: &serde_json::Value) -> Vec { .map(String::from) .collect() }) - .unwrap_or_default() + .unwrap_or_default(); + + if let Some(servers) = agent.get("mcp_servers").and_then(|s| s.as_array()) { + for server in servers { + if let Some(env) = server.get("env").and_then(|e| e.as_array()) { + for entry in env { + if let Some(value) = entry + .get("value") + .and_then(|v| v.as_str()) + .filter(|s| !s.is_empty()) + { + secrets.push(value.to_string()); + } + } + } + } + } + + secrets } /// Public-in-crate helper: redact every non-empty value from `env` (plus @@ -584,6 +610,76 @@ mod tests { ); } + #[test] + fn env_secrets_from_request_extracts_mcp_server_env_values() { + let req = serde_json::json!({ + "op": "deploy", + "agent": { + "env_vars": { + "ANTHROPIC_API_KEY": "sk-ant-agent", + }, + "mcp_servers": [ + { + "name": "github", + "command": "npx", + "args": [], + "env": [ + {"name": "GITHUB_TOKEN", "value": "ghp-secret"}, + {"name": "EMPTY_VAR", "value": ""}, + ], + }, + { + "name": "no-env", + "command": "npx", + "args": [], + }, + ], + }, + }); + let secrets = env_secrets_from_request(&req); + assert!( + secrets.iter().any(|v| v == "sk-ant-agent"), + "env_vars value missing" + ); + assert!( + secrets.iter().any(|v| v == "ghp-secret"), + "mcp server env value missing" + ); + // Empty values must be filtered out. + assert!( + !secrets.iter().any(|v| v.is_empty()), + "empty value must not be collected" + ); + assert_eq!(secrets.len(), 2); + } + + #[test] + fn mcp_server_env_secret_is_redacted_from_provider_error() { + // Regression test for F1: a provider that echoes an MCP env value in + // its stderr must not leak the secret into the desktop-visible error. + let secret = "ghp-very-secret-token"; + let request = serde_json::json!({ + "agent": { + "env_vars": {}, + "mcp_servers": [{ + "name": "github", + "command": "npx", + "args": [], + "env": [{"name": "GITHUB_TOKEN", "value": secret}], + }], + }, + }); + let secrets = env_secrets_from_request(&request); + let secret_refs: Vec<&str> = secrets.iter().map(String::as_str).collect(); + let stderr = format!("provider failed: invalid token {secret}"); + let redacted = redact_secrets_with(&stderr, &secret_refs); + assert!( + !redacted.contains(secret), + "secret must be redacted from provider error" + ); + assert!(redacted.contains("[REDACTED]")); + } + #[test] fn redact_env_values_in_scrubs_map_values() { let mut env = std::collections::BTreeMap::new(); diff --git a/desktop/src-tauri/src/managed_agents/types/mcp_servers.rs b/desktop/src-tauri/src/managed_agents/types/mcp_servers.rs index 13aadbfc8..d8694febf 100644 --- a/desktop/src-tauri/src/managed_agents/types/mcp_servers.rs +++ b/desktop/src-tauri/src/managed_agents/types/mcp_servers.rs @@ -6,6 +6,15 @@ use serde::{Deserialize, Serialize}; /// `buzz-dev-mcp` occupies the sixteenth `buzz-agent` MCP slot. pub(crate) const MAX_USER_MCP_SERVERS: usize = 15; +/// Maximum byte length of an MCP server name. Mirrors `MAX_NAME_LEN` in +/// `buzz-agent/src/mcp.rs`. +const MAX_SERVER_NAME_LEN: usize = 128; + +/// The built-in MCP server name reserved by the buzz-dev-mcp sidecar. +/// User-configured servers must not use this name — buzz-acp always appends +/// the built-in after user servers, making it a duplicate at spawn time. +const RESERVED_SERVER_NAME: &str = "buzz-dev-mcp"; + /// An environment variable passed to an MCP subprocess. /// /// This deliberately mirrors the ACP `EnvVar` wire shape so the effective @@ -67,19 +76,67 @@ fn default_mcp_server_enabled() -> bool { } /// Validate one persisted MCP layer at every IPC save boundary. +/// +/// Checks: +/// - Names match the buzz-agent grammar: 1–128 bytes, ASCII alphanumeric, +/// `_`, or `-` only, no `__` substring. This mirrors `valid_name` in +/// `buzz-agent/src/mcp.rs` so save-time rejection matches spawn-time +/// rejection. +/// - Names must not be `buzz-dev-mcp` — that slot is reserved for the +/// bundled sidecar and would become a duplicate when buzz-acp appends it. +/// - No duplicate names within the layer. +/// - Enabled servers must have a non-empty command. +/// - No NUL bytes; individual values under the length ceiling. +/// - Total payload under the overall byte budget. +/// - At most `MAX_USER_MCP_SERVERS` enabled entries (layer-level cap; +/// the merged effective-server cap is enforced separately in +/// `merge_mcp_servers`). pub(crate) fn validate_mcp_servers(servers: &[McpServerConfig]) -> Result<(), String> { let mut names = HashSet::new(); + let mut enabled_count = 0usize; let mut total_bytes = 0usize; for server in servers { - if server.name.trim().is_empty() { + // Grammar validation — must match buzz-agent's `valid_name` exactly. + if server.name.is_empty() { return Err("MCP server name is required".to_string()); } + if server.name.len() > MAX_SERVER_NAME_LEN { + return Err(format!( + "MCP server name `{}` exceeds the maximum length ({MAX_SERVER_NAME_LEN} bytes)", + server.name + )); + } + if !server + .name + .bytes() + .all(|b| b.is_ascii_alphanumeric() || b == b'_' || b == b'-') + { + return Err(format!( + "MCP server name `{}` contains invalid characters (only ASCII alphanumeric, `_`, and `-` are allowed)", + server.name + )); + } + if server.name.contains("__") { + return Err(format!( + "MCP server name `{}` cannot contain `__`", + server.name + )); + } + if server.name == RESERVED_SERVER_NAME { + return Err(format!( + "MCP server name `{RESERVED_SERVER_NAME}` is reserved" + )); + } + if !names.insert(server.name.as_str()) { return Err(format!("MCP server names must be unique: {}", server.name)); } if server.enabled && server.command.trim().is_empty() { return Err(format!("MCP server `{}` command is required", server.name)); } + if server.enabled { + enabled_count += 1; + } for (field, value) in std::iter::once(("name", &server.name)) .chain((!server.command.is_empty()).then_some(("command", &server.command))) @@ -121,6 +178,11 @@ pub(crate) fn validate_mcp_servers(servers: &[McpServerConfig]) -> Result<(), St ); } + if enabled_count > MAX_USER_MCP_SERVERS { + return Err(format!( + "MCP server layer has {enabled_count} enabled servers; limit is {MAX_USER_MCP_SERVERS}" + )); + } if total_bytes > crate::managed_agents::env_vars::MAX_ENV_TOTAL_BYTES { return Err(format!( "total MCP server payload is {total_bytes} bytes; limit is {}", diff --git a/desktop/src-tauri/src/managed_agents/types/tests.rs b/desktop/src-tauri/src/managed_agents/types/tests.rs index a93db0ae6..f58c7ef17 100644 --- a/desktop/src-tauri/src/managed_agents/types/tests.rs +++ b/desktop/src-tauri/src/managed_agents/types/tests.rs @@ -745,3 +745,50 @@ fn validate_mcp_servers_rejects_empty_duplicate_and_reserved_env_inputs() { }); assert!(validate_mcp_servers(&[server]).is_err()); } + +#[test] +fn validate_mcp_servers_rejects_invalid_name_characters() { + // Space, slash, dot — not in the buzz-agent grammar. + for bad_name in &["bad name", "bad/name", "bad.name", "bad@name"] { + assert!( + validate_mcp_servers(&[mcp_server(bad_name, "cmd", true)]).is_err(), + "expected error for name {bad_name:?}" + ); + } +} + +#[test] +fn validate_mcp_servers_rejects_double_underscore_in_name() { + assert!(validate_mcp_servers(&[mcp_server("bad__name", "cmd", true)]).is_err()); +} + +#[test] +fn validate_mcp_servers_rejects_reserved_name() { + assert!(validate_mcp_servers(&[mcp_server("buzz-dev-mcp", "cmd", true)]).is_err()); +} + +#[test] +fn validate_mcp_servers_accepts_valid_grammar() { + // Alphanumeric, hyphens, underscores (but not double) are all OK. + assert!(validate_mcp_servers(&[mcp_server("github", "cmd", true)]).is_ok()); + assert!(validate_mcp_servers(&[mcp_server("my-server", "cmd", true)]).is_ok()); + assert!(validate_mcp_servers(&[mcp_server("my_server", "cmd", true)]).is_ok()); + assert!(validate_mcp_servers(&[mcp_server("Server123", "cmd", true)]).is_ok()); +} + +#[test] +fn validate_mcp_servers_rejects_more_than_fifteen_enabled_in_one_layer() { + let servers: Vec<_> = (0..=MAX_USER_MCP_SERVERS) + .map(|i| mcp_server(&format!("server-{i}"), "cmd", true)) + .collect(); + let err = validate_mcp_servers(&servers).expect_err("layer cap must fire at 16 enabled"); + assert!(err.contains("enabled servers"), "error: {err}"); +} + +#[test] +fn validate_mcp_servers_allows_fifteen_enabled_in_one_layer() { + let servers: Vec<_> = (0..MAX_USER_MCP_SERVERS) + .map(|i| mcp_server(&format!("server-{i}"), "cmd", true)) + .collect(); + assert!(validate_mcp_servers(&servers).is_ok()); +} From f75a9a986543480560ca53ba1a730e5aadb8512d Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Mon, 13 Jul 2026 11:46:37 -0400 Subject: [PATCH 03/17] fix(desktop): initialize MCP defaults after rebase New snapshot construction sites added after the original MCP change require empty local-only MCP configuration. Raise the discovery fixture-file exception by the two required fields. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/scripts/check-file-sizes.mjs | 7 ++++--- desktop/src-tauri/src/commands/team_snapshot.rs | 1 + desktop/src-tauri/src/commands/team_snapshot/tests.rs | 2 ++ desktop/src-tauri/src/managed_agents/team_snapshot.rs | 1 + 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 5578c4a7d..c8d8d1ee8 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -290,9 +290,10 @@ const overrides = new Map([ // candidates, cli_install_commands_for_os PowerShell selection, login_shell_path // None regression, .cmd shim resolution, no-git-bash error hint. // +32: deterministic .cmd resolver + no-registry + install_shell_from tests. - // team-instructions-first-class: record_with test fixture gained the new - // ManagedAgentRecord.team_id field (+1 line) alongside persona_team_dir. - ["src-tauri/src/managed_agents/discovery/tests.rs", 1271], + // `team-instructions-first-class` added `team_id`; MCP configuration added + // two required `mcp_servers` test-fixture fields. The gate counts the + // trailing newline, so 1,272 physical lines are 1,273 counted lines. + ["src-tauri/src/managed_agents/discovery/tests.rs", 1273], // identity-import-keyring: the identity resolution state machine's behavioral // matrix (46 tests over FakeIdentityStore — probe × marker × file cells, // adoption / read-back-corruption / marker-failure arms, recovery-mode diff --git a/desktop/src-tauri/src/commands/team_snapshot.rs b/desktop/src-tauri/src/commands/team_snapshot.rs index dc376ca06..c42ceb057 100644 --- a/desktop/src-tauri/src/commands/team_snapshot.rs +++ b/desktop/src-tauri/src/commands/team_snapshot.rs @@ -132,6 +132,7 @@ fn definition_from_snapshot( source_team: None, source_team_persona_slug: None, env_vars: Default::default(), + mcp_servers: Default::default(), respond_to, respond_to_allowlist: behavior.respond_to_allowlist, parallelism: behavior.parallelism, diff --git a/desktop/src-tauri/src/commands/team_snapshot/tests.rs b/desktop/src-tauri/src/commands/team_snapshot/tests.rs index ca7dc6183..d7d2c39c3 100644 --- a/desktop/src-tauri/src/commands/team_snapshot/tests.rs +++ b/desktop/src-tauri/src/commands/team_snapshot/tests.rs @@ -67,6 +67,7 @@ fn team_export_round_trip_preserves_team_and_excludes_member_memory() { source_team: None, source_team_persona_slug: None, env_vars: Default::default(), + mcp_servers: vec![], respond_to: None, respond_to_allowlist: vec![], parallelism: None, @@ -87,6 +88,7 @@ fn team_export_round_trip_preserves_team_and_excludes_member_memory() { source_team: None, source_team_persona_slug: None, env_vars: Default::default(), + mcp_servers: vec![], respond_to: None, respond_to_allowlist: vec![], parallelism: None, diff --git a/desktop/src-tauri/src/managed_agents/team_snapshot.rs b/desktop/src-tauri/src/managed_agents/team_snapshot.rs index d88a36272..173480c7a 100644 --- a/desktop/src-tauri/src/managed_agents/team_snapshot.rs +++ b/desktop/src-tauri/src/managed_agents/team_snapshot.rs @@ -278,6 +278,7 @@ mod tests { m.insert("API_KEY".to_string(), "secret123".to_string()); // MUST NOT appear m }, + mcp_servers: vec![], start_on_app_launch: false, auto_restart_on_config_change: true, runtime_pid: None, From 812301592aa514d46d8d6021491fe24a6b9f8179 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Mon, 13 Jul 2026 12:12:32 -0400 Subject: [PATCH 04/17] fix(desktop): satisfy MCP test clippy lint Borrow the disabled mask for validation before moving it into the merge assertion, avoiding an unnecessary clone. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/managed_agents/types/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/desktop/src-tauri/src/managed_agents/types/tests.rs b/desktop/src-tauri/src/managed_agents/types/tests.rs index f58c7ef17..ee376d019 100644 --- a/desktop/src-tauri/src/managed_agents/types/tests.rs +++ b/desktop/src-tauri/src/managed_agents/types/tests.rs @@ -701,7 +701,7 @@ fn disabled_mask_requires_only_a_name() { env: vec![], enabled: false, }; - validate_mcp_servers(&[mask.clone()]).unwrap(); + validate_mcp_servers(std::slice::from_ref(&mask)).unwrap(); assert!( merge_mcp_servers(&[mcp_server("shared", "global", true)], &[], &[mask]) .unwrap() From c3386a96279620698eca039ba2f4203b008115ca Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Mon, 13 Jul 2026 17:53:32 -0400 Subject: [PATCH 05/17] feat(desktop): surface effective buzz-agent MCP servers on config surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add buzz_agent_mcp_servers to RuntimeConfigSurface, populated by resolve_config_surface via the existing effective_buzz_agent_mcp_servers merge (global < definition < agent, enabled-only) for the buzz-agent runtime only. Every other runtime keeps the field empty and continues to surface its servers via extensions. This is the WYSIWYG read path the PR3 UI's read-only buzzAgentSlot will consume — the effective list is now reachable from the config-bridge surface the frontend already reads, matching what actually runs at spawn (spawn_hash.rs, runtime.rs) rather than just the agent's own record layer. Also bumps two pre-existing file-size overrides (agent_config.rs, readiness.rs) to their gate's own split-count, matching this diff and a prior unaccounted +1 line already on the branch. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/scripts/check-file-sizes.mjs | 13 +- .../src-tauri/src/commands/agent_config.rs | 117 ++++++++++++++++++ .../managed_agents/config_bridge/reader.rs | 4 + .../src/managed_agents/config_bridge/types.rs | 8 ++ 4 files changed, 138 insertions(+), 4 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index c8d8d1ee8..2fccfb0a9 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -166,9 +166,10 @@ const overrides = new Map([ // setup-mode requirements. The Windows-only requirement and serialization // test add eight lines; split remains queued with the existing file debt. // Windows Doctor install fix: cli_install_commands_windows field added to test stubs. - // team-instructions-first-class: ManagedAgentRecord fixture gains the new - // team_id field (+1 line). - ["src-tauri/src/managed_agents/readiness.rs", 1765], + // `team-instructions-first-class` added `team_id`; MCP configuration added + // the required `mcp_servers` minimal-fixture field. The gate counts the + // trailing newline, so 1,765 physical lines are 1,766 counted lines. + ["src-tauri/src/managed_agents/readiness.rs", 1766], // applyWorkspace reposDir parameter plus the validateReposDir binding, // threaded through Tauri invokes for configurable repos_dir, plus the // harness-persona-sync `harnessOverride` create-input bit — load-bearing @@ -429,7 +430,11 @@ const overrides = new Map([ // is_safe_to_reveal allowlist + baked_env_thinking_effort_is_unmasked test. // +1: doctor-install-reliability: login_hint: None added to goose_runtime test stub. // +1: doctor-install-reliability review fixes: auth_probe_args: None added to stub. - ["src-tauri/src/commands/agent_config.rs", 1021], + // MCP config surface: `resolve_config_surface` receives the effective command + // and exposes the effective `buzz_agent_mcp_servers` merge; coverage pins the + // merge, disabled-mask, and non-buzz-agent cases. The gate counts trailing + // newlines, so 1,139 physical lines are 1,140 counted lines. + ["src-tauri/src/commands/agent_config.rs", 1140], // codex-install-auto-restart review-fixes: should_restart_after_install // takes pid_alive:bool (pure predicate, no OS-dependent call); 3 racy // cache tests replaced with 6 pure availability_drift predicate tests; diff --git a/desktop/src-tauri/src/commands/agent_config.rs b/desktop/src-tauri/src/commands/agent_config.rs index 7b9577ba2..5a250491c 100644 --- a/desktop/src-tauri/src/commands/agent_config.rs +++ b/desktop/src-tauri/src/commands/agent_config.rs @@ -55,6 +55,7 @@ fn resolve_config_surface( mut record: ManagedAgentRecord, personas: &[AgentDefinition], runtime_meta: Option<&KnownAcpRuntime>, + effective_command: &str, session_cache: Option<&SessionConfigCache>, global: &GlobalAgentConfig, ) -> RuntimeConfigSurface { @@ -152,6 +153,19 @@ fn resolve_config_surface( baseline.as_ref().map(|(m, o)| (m.as_str(), o.clone())), ); + if runtime_meta.is_some_and(|m| m.id == "buzz-agent") { + surface.buzz_agent_mcp_servers = crate::managed_agents::effective_buzz_agent_mcp_servers( + &record, + personas, + &global.mcp_servers, + effective_command, + ) + .unwrap_or_else(|error| { + eprintln!("buzz-desktop: invalid persisted MCP server configuration: {error}"); + Vec::new() + }); + } + // Re-tag persona-sourced fields from BuzzExplicit to PersonaDefault. if !had_prompt { retag_persona_default(&mut surface.normalized.system_prompt); @@ -375,6 +389,7 @@ pub async fn get_agent_config_surface( record, &personas, runtime_meta, + &effective_cmd, session_cache.as_ref(), &global, )) @@ -713,6 +728,7 @@ mod tests { record, &personas, Some(goose_runtime()), + "goose", None, &Default::default(), ); @@ -740,6 +756,7 @@ mod tests { record, &personas, Some(goose_runtime()), + "goose", Some(&cache), &Default::default(), ); @@ -768,6 +785,7 @@ mod tests { record, &personas, Some(goose_runtime()), + "goose", Some(&cache), &Default::default(), ); @@ -795,6 +813,7 @@ mod tests { record, &personas, Some(goose_runtime()), + "goose", Some(&cache), &Default::default(), ); @@ -819,6 +838,7 @@ mod tests { record, &personas, Some(goose_runtime()), + "goose", Some(&cache), &Default::default(), ); @@ -855,6 +875,7 @@ mod tests { record, &personas, Some(goose_runtime()), + "goose", Some(&cache), &global, ); @@ -876,6 +897,102 @@ mod tests { ); } + // ── buzz_agent_mcp_servers surface tests ──────────────────────────────── + + fn mcp_server( + name: &str, + command: &str, + enabled: bool, + ) -> crate::managed_agents::McpServerConfig { + crate::managed_agents::McpServerConfig { + name: name.to_string(), + command: command.to_string(), + args: vec![], + env: vec![], + enabled, + } + } + + /// The buzz-agent runtime surfaces its effective-merged MCP servers onto + /// `RuntimeConfigSurface.buzz_agent_mcp_servers`, matching what + /// `effective_buzz_agent_mcp_servers` (the spawn-time source of truth) + /// would compute for the same record/persona/global layers. + #[test] + fn buzz_agent_runtime_surfaces_effective_merged_mcp_servers() { + let mut record = agent_record(); + record.persona_id = None; + record.mcp_servers = vec![mcp_server("agent-server", "agent-cmd", true)]; + let personas: Vec = vec![]; + let global = crate::managed_agents::GlobalAgentConfig { + mcp_servers: vec![mcp_server("global-server", "global-cmd", true)], + ..Default::default() + }; + + let surface = resolve_config_surface( + record, + &personas, + known_acp_runtime("buzz-agent"), + "buzz-agent", + None, + &global, + ); + + let mut names: Vec<&str> = surface + .buzz_agent_mcp_servers + .iter() + .map(|s| s.name.as_str()) + .collect(); + names.sort_unstable(); + assert_eq!(names, vec!["agent-server", "global-server"]); + } + + /// A disabled agent-layer entry masks a same-named global entry — the + /// merged effective list must NOT include it. Fails against a variant + /// that surfaces the raw record layer instead of the merged result. + #[test] + fn buzz_agent_disabled_override_masks_inherited_server_in_surface() { + let mut record = agent_record(); + record.persona_id = None; + record.mcp_servers = vec![mcp_server("shared", "", false)]; + let personas: Vec = vec![]; + let global = crate::managed_agents::GlobalAgentConfig { + mcp_servers: vec![mcp_server("shared", "global-cmd", true)], + ..Default::default() + }; + + let surface = resolve_config_surface( + record, + &personas, + known_acp_runtime("buzz-agent"), + "buzz-agent", + None, + &global, + ); + + assert!(surface.buzz_agent_mcp_servers.is_empty()); + } + + /// Non-buzz-agent runtimes (e.g. goose) must never populate + /// `buzz_agent_mcp_servers` — they surface their servers via `extensions` + /// instead. Fails against a variant that populates the field unconditionally. + #[test] + fn non_buzz_agent_runtime_leaves_mcp_surface_empty() { + let mut record = agent_record(); + record.mcp_servers = vec![mcp_server("agent-server", "agent-cmd", true)]; + let personas = vec![persona_with_model("persona-model")]; + + let surface = resolve_config_surface( + record, + &personas, + Some(goose_runtime()), + "goose", + None, + &Default::default(), + ); + + assert!(surface.buzz_agent_mcp_servers.is_empty()); + } + // ── get_baked_build_env / is_secret_key tests ────────────────────────── /// Build a `BakedEnvEntry` vec from a synthetic map, mirroring what diff --git a/desktop/src-tauri/src/managed_agents/config_bridge/reader.rs b/desktop/src-tauri/src/managed_agents/config_bridge/reader.rs index 372d2cfde..be6393e40 100644 --- a/desktop/src-tauri/src/managed_agents/config_bridge/reader.rs +++ b/desktop/src-tauri/src/managed_agents/config_bridge/reader.rs @@ -209,6 +209,10 @@ pub(crate) fn read_config_surface( normalized, advanced, extensions, + // Populated by `resolve_config_surface` for the buzz-agent runtime only; + // this reader has no access to the global/persona MCP layers needed to + // compute the effective merge. + buzz_agent_mcp_servers: Vec::new(), sources, } } diff --git a/desktop/src-tauri/src/managed_agents/config_bridge/types.rs b/desktop/src-tauri/src/managed_agents/config_bridge/types.rs index 15ccb718e..a2a6ee7d9 100644 --- a/desktop/src-tauri/src/managed_agents/config_bridge/types.rs +++ b/desktop/src-tauri/src/managed_agents/config_bridge/types.rs @@ -2,6 +2,8 @@ use std::collections::BTreeMap; use serde::{Deserialize, Serialize}; +use crate::managed_agents::types::McpServerConfig; + /// Where a config value came from — determines precedence and UI annotations. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -136,6 +138,12 @@ pub struct RuntimeConfigSurface { pub normalized: NormalizedConfig, pub advanced: Vec, pub extensions: Vec, + /// Effective (global < definition < agent merged, enabled-only) buzz-agent + /// MCP servers — "what runs." Populated only for the `buzz-agent` runtime; + /// empty for every other runtime, which surface their servers via + /// `extensions` instead. + #[serde(default)] + pub buzz_agent_mcp_servers: Vec, pub sources: ConfigSourceReport, } From c7bfd0d80aa87e2087e904673e744567c6d8b293 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Mon, 13 Jul 2026 18:55:50 -0400 Subject: [PATCH 06/17] feat(desktop): thread mcpServers through FE types and invoke plumbing Add McpServerConfig/McpServerEnvVar FE types (camelCase mirror of the Rust McpServerConfig) and mcpServers fields on ManagedAgent, AgentPersona, GlobalAgentConfig, and the create/update managed-agent and persona IPC inputs. Threads mcpServers through fromRawManagedAgent/fromRawPersona and the invoke builders in tauri.ts/tauriPersonas.ts with the same absent-vs-present contract as envVars (create sends mcpServers ?? [], update sends it only when provided). Adds the buzzAgentMcpServers surface field (populated on the backend by 859b8bbbe) to RuntimeConfigSurface. Wires the e2eBridge.ts mock to match: mcp_servers on the local RawManagedAgent/RawPersona types, a cloneMcpServer helper mirroring the existing clone* helpers, mcp_servers handling in the create/update persona and managed-agent mock handlers, and buzzAgentMcpServers on every buildMockConfigSurface fixture (a sample entry on the buzz-agent fixture so the read-only display has something to render by default). Bumps two file-size overrides (tauri.ts, types.ts) to the gate's own split-count for this diff. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/scripts/check-file-sizes.mjs | 18 +++--- .../agents/ui/GlobalAgentConfigFields.tsx | 1 + .../features/agents/useGlobalAgentConfig.ts | 1 + desktop/src/shared/api/tauri.ts | 4 ++ desktop/src/shared/api/tauriPersonas.ts | 6 ++ desktop/src/shared/api/types.ts | 38 +++++++++++++ desktop/src/testing/e2eBridge.ts | 55 +++++++++++++++++-- 7 files changed, 110 insertions(+), 13 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 2fccfb0a9..778013122 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -202,9 +202,10 @@ const overrides = new Map([ // RawInstallRuntimeResult + fromRawInstallRuntimeResult mapper (+2). // Git Bash Doctor discovery adds the raw Tauri response and its camelCase // mapper. This is the existing API boundary; split remains queued. - // team-instructions-first-class: createManagedAgent Tauri bridge threads the - // new teamId input through to the backend (+1 line). - ["src/shared/api/tauri.ts", 1305], + // `team-instructions-first-class` added the create `teamId` wire field; MCP + // configuration adds the raw/read/create `mcpServers` plumbing. The gate + // counts trailing newlines, so 1,308 physical lines are 1,309 counted lines. + ["src/shared/api/tauri.ts", 1309], // doctor-npm-eacces-preflight: hint field added to InstallStepResult (+1 line). // codex-acp-package-swap: "adapter_outdated" variant added to AcpAvailabilityStatus (+1 line). // doctor-install-reliability: AuthStatus tagged union + nodeRequired/authStatus/ @@ -214,12 +215,11 @@ const overrides = new Map([ // mcp-readonly-view rebase: PR2 MCP config surface FE-type fields force +1 over the grandfathered ceiling. // Git Bash prerequisite payload adds four fields to the shared Tauri API // contract. This is the canonical type location; split remains queued. - // signout-wipe: resetFailed field added to Identity type (+6 lines). - // team-instructions-first-class: CreateManagedAgentInput.teamId (+2, incl. - // doc comment) and AgentTeam/CreateTeamInput/UpdateTeamInput.instructions - // (+3) — the new team-id spawn link and the runtime-layered instructions - // field. - ["src/shared/api/types.ts", 1047], + // `signout-wipe` added `Identity.resetFailed`; `team-instructions-first-class` + // added team/instructions fields; MCP config adds the shared server types, + // layer fields, IPC input fields, and effective-surface field. The gate + // counts trailing newlines, so 1,076 physical lines are 1,077 counted lines. + ["src/shared/api/types.ts", 1077], // readiness-gate: PersonaDialog.tsx threads computeLocalModeGate + // requiredCredentialEnvKeys + RequiredFieldLabel so the "New agent" dialog // shows required markers and credential amber rows (parity with diff --git a/desktop/src/features/agents/ui/GlobalAgentConfigFields.tsx b/desktop/src/features/agents/ui/GlobalAgentConfigFields.tsx index a4e6afacd..b84ead485 100644 --- a/desktop/src/features/agents/ui/GlobalAgentConfigFields.tsx +++ b/desktop/src/features/agents/ui/GlobalAgentConfigFields.tsx @@ -37,6 +37,7 @@ import { SettingsOptionGroup } from "@/features/settings/ui/SettingsOptionGroup" /** Sentinel value for an unconfigured global agent config. */ export const EMPTY_GLOBAL_CONFIG: GlobalAgentConfig = { env_vars: {}, + mcp_servers: [], provider: null, model: null, }; diff --git a/desktop/src/features/agents/useGlobalAgentConfig.ts b/desktop/src/features/agents/useGlobalAgentConfig.ts index 375931d3e..ec9fa065d 100644 --- a/desktop/src/features/agents/useGlobalAgentConfig.ts +++ b/desktop/src/features/agents/useGlobalAgentConfig.ts @@ -16,6 +16,7 @@ import type { GlobalAgentConfig } from "@/shared/api/types"; const EMPTY_CONFIG: GlobalAgentConfig = { env_vars: {}, + mcp_servers: [], provider: null, model: null, }; diff --git a/desktop/src/shared/api/tauri.ts b/desktop/src/shared/api/tauri.ts index 9dc9e69a5..14f1b4d65 100644 --- a/desktop/src/shared/api/tauri.ts +++ b/desktop/src/shared/api/tauri.ts @@ -16,6 +16,7 @@ import type { HomeFeedResponse, ManagedAgent, ManagedAgentBackend, + McpServerConfig, RelayAgent, RelayMember, RelayMemberRole, @@ -184,6 +185,7 @@ export type RawManagedAgent = { persona_orphaned: boolean; needs_restart: boolean; env_vars?: Record; + mcp_servers?: McpServerConfig[]; status: ManagedAgent["status"]; pid: number | null; created_at: string; @@ -872,6 +874,7 @@ export function fromRawManagedAgent(agent: RawManagedAgent): ManagedAgent { personaOrphaned: agent.persona_orphaned ?? false, needsRestart: agent.needs_restart ?? false, envVars: agent.env_vars ?? {}, + mcpServers: agent.mcp_servers ?? [], status: agent.status, pid: agent.pid, createdAt: agent.created_at, @@ -1031,6 +1034,7 @@ export async function createManagedAgent(input: CreateManagedAgentInput) { avatarUrl: input.avatarUrl, model: input.model, envVars: input.envVars ?? {}, + mcpServers: input.mcpServers ?? [], spawnAfterCreate: input.spawnAfterCreate, startOnAppLaunch: input.startOnAppLaunch, backend: input.backend, diff --git a/desktop/src/shared/api/tauriPersonas.ts b/desktop/src/shared/api/tauriPersonas.ts index bd3be9240..c1cf53336 100644 --- a/desktop/src/shared/api/tauriPersonas.ts +++ b/desktop/src/shared/api/tauriPersonas.ts @@ -2,6 +2,7 @@ import { invokeTauri } from "@/shared/api/tauri"; import type { AgentPersona, CreatePersonaInput, + McpServerConfig, RespondToMode, UpdatePersonaInput, } from "@/shared/api/types"; @@ -19,6 +20,7 @@ export type RawPersona = { is_active?: boolean; source_team?: string | null; env_vars?: Record; + mcp_servers?: McpServerConfig[]; respond_to?: string | null; respond_to_allowlist?: string[]; parallelism?: number | null; @@ -42,6 +44,7 @@ export function fromRawPersona(persona: RawPersona): AgentPersona { isActive: persona.is_active ?? true, sourceTeam: persona.source_team ?? null, envVars: persona.env_vars ?? {}, + mcpServers: persona.mcp_servers ?? [], respondTo: (persona.respond_to as RespondToMode | undefined) ?? null, respondToAllowlist: persona.respond_to_allowlist ?? [], parallelism: persona.parallelism ?? null, @@ -68,6 +71,7 @@ export async function createPersona( provider: input.provider, namePool: input.namePool ?? [], envVars: input.envVars ?? {}, + mcpServers: input.mcpServers ?? [], behavior: input.behavior, }, }), @@ -91,6 +95,8 @@ export async function updatePersona( // tells the backend "don't touch the stored env vars" so editing // unrelated fields can't silently wipe saved credentials. envVars: input.envVars, + // Same absent-vs-present contract as envVars for the local MCP layer. + mcpServers: input.mcpServers, // Same absent-vs-present contract as envVars for the behavioral quad. behavior: input.behavior, }, diff --git a/desktop/src/shared/api/types.ts b/desktop/src/shared/api/types.ts index 8d125d56e..3b298907e 100644 --- a/desktop/src/shared/api/types.ts +++ b/desktop/src/shared/api/types.ts @@ -426,6 +426,9 @@ export type ManagedAgent = { needsRestart: boolean; /** Per-agent env vars. Layered on top of persona envVars. */ envVars: Record; + /** Local-only MCP server layer. Layered on top of persona mcpServers + * (global < definition < agent, by name). */ + mcpServers: McpServerConfig[]; status: "running" | "stopped" | "deployed" | "not_deployed"; pid: number | null; createdAt: string; @@ -499,6 +502,8 @@ export type CreateManagedAgentInput = { model?: string; provider?: string; envVars?: Record; + /** Local-only MCP server layer. Absent = empty (matches backend default). */ + mcpServers?: McpServerConfig[]; spawnAfterCreate?: boolean; startOnAppLaunch?: boolean; backend?: ManagedAgentBackend; @@ -697,6 +702,24 @@ export type ConfigSourceReport = { export type ExtensionEntry = { name: string; kind: string; enabled: boolean }; +/** An env var passed to an MCP server subprocess. Mirrors the Rust `McpServerEnvVar`. */ +export type McpServerEnvVar = { name: string; value: string }; + +/** + * A locally persisted stdio MCP server configuration, layered + * `global < definition < agent` by name. Mirrors the Rust `McpServerConfig`. + * STDIO only — no SSE/HTTP, no timeout, no headers. + */ +export type McpServerConfig = { + name: string; + /** Required for enabled servers. Disabled entries may omit it — their + * only purpose is to mask a lower-precedence server by name. */ + command: string; + args: string[]; + env: McpServerEnvVar[]; + enabled: boolean; +}; + export type NormalizedConfig = { model: NormalizedField | null; provider: NormalizedField | null; @@ -714,6 +737,10 @@ export type RuntimeConfigSurface = { normalized: NormalizedConfig; advanced: ConfigField[]; extensions: ExtensionEntry[]; + /** Effective (global < definition < agent merged, enabled-only) buzz-agent + * MCP servers — "what runs." Empty for every other runtime, which + * surface their servers via `extensions` instead. */ + buzzAgentMcpServers: McpServerConfig[]; sources: ConfigSourceReport; }; @@ -725,6 +752,8 @@ export type UpdateManagedAgentInput = { systemPrompt?: string | null; /** Absent = don't touch. Present = replace the env_vars map entirely. */ envVars?: Record; + /** Absent = don't touch. Present = replace this local-only MCP layer. */ + mcpServers?: McpServerConfig[]; parallelism?: number; turnTimeoutSeconds?: number; relayUrl?: string; @@ -766,6 +795,9 @@ export type AgentPersona = { /** Environment variables injected for agents created from this persona. * Layered as: desktop parent env < persona envVars < agent envVars. */ envVars: Record; + /** Local-only MCP server layer inherited by agents created from this + * definition. Layered as: global < persona mcpServers < agent mcpServers. */ + mcpServers: McpServerConfig[]; /** NIP-AP behavioral defaults (wire shape). Null/empty = unset. */ respondTo: RespondToMode | null; respondToAllowlist: string[]; @@ -794,6 +826,8 @@ export type CreatePersonaInput = { provider?: string; namePool?: string[]; envVars?: Record; + /** Local-only MCP server layer. Absent = empty (matches backend default). */ + mcpServers?: McpServerConfig[]; behavior?: PersonaBehaviorInput; }; @@ -807,6 +841,8 @@ export type UpdatePersonaInput = { provider?: string; namePool?: string[]; envVars?: Record; + /** Absent = don't touch. Present = replace this local-only MCP layer. */ + mcpServers?: McpServerConfig[]; behavior?: PersonaBehaviorInput; }; @@ -1025,6 +1061,8 @@ export type ChannelMessagesPageResponse = { export type GlobalAgentConfig = { /** Global env vars injected into all agents unconditionally. */ env_vars: Record; + /** Local-only MCP server layer inherited by every buzz-agent instance. */ + mcp_servers: McpServerConfig[]; /** Global fallback provider (e.g. "anthropic", "databricks_v2"). Null = no global default. */ provider: string | null; /** Global fallback model identifier. Null = no global default. */ diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index eba0c053a..2efc231da 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -6,7 +6,7 @@ import { parse as yamlParse } from "yaml"; import { relayClient } from "@/shared/api/relayClient"; import type { ConnectionState } from "@/shared/api/relayClientShared"; -import type { RelayEvent } from "@/shared/api/types"; +import type { McpServerConfig, RelayEvent } from "@/shared/api/types"; import { getMarkdownParseCount } from "@/shared/ui/markdown/nodeCache"; import { syncAgentTurnsFromEvents } from "@/features/agents/activeAgentTurnsStore"; import { recordTimeoutFromRejection } from "@/features/moderation/lib/timeoutStore"; @@ -229,11 +229,13 @@ type E2eConfig = { identityLocked?: boolean; /** * Global agent config returned by `get_global_agent_config`. Defaults to - * an empty config (no provider, model, or env vars) if not specified. - * Pass a config with a provider to test Inherit-from-global behavior. + * an empty config (no provider, model, env vars, or MCP servers) if not + * specified. Pass a config with a provider to test Inherit-from-global + * behavior. */ globalAgentConfig?: { env_vars: Record; + mcp_servers?: McpServerConfig[]; provider: string | null; model: string | null; }; @@ -549,6 +551,7 @@ type RawManagedAgent = { avatar_url: string | null; model: string | null; env_vars?: Record; + mcp_servers?: McpServerConfig[]; status: "running" | "stopped" | "deployed" | "not_deployed"; pid: number | null; created_at: string; @@ -621,6 +624,7 @@ type RawPersona = { is_active: boolean; source_team?: string | null; env_vars?: Record; + mcp_servers?: McpServerConfig[]; respond_to?: string | null; respond_to_allowlist?: string[]; parallelism?: number | null; @@ -1198,6 +1202,14 @@ function cloneRelayAgent(agent: RawRelayAgent): RawRelayAgent { }; } +function cloneMcpServer(server: McpServerConfig): McpServerConfig { + return { + ...server, + args: [...server.args], + env: server.env.map((envVar) => ({ ...envVar })), + }; +} + function cloneManagedAgent(agent: MockManagedAgent): RawManagedAgent { return { pubkey: agent.pubkey, @@ -1216,6 +1228,7 @@ function cloneManagedAgent(agent: MockManagedAgent): RawManagedAgent { avatar_url: agent.avatar_url ?? null, model: agent.model, env_vars: { ...(agent.env_vars ?? {}) }, + mcp_servers: (agent.mcp_servers ?? []).map(cloneMcpServer), status: agent.status, pid: agent.pid, created_at: agent.created_at, @@ -1295,6 +1308,7 @@ function buildMockConfigSurface(pubkey: string): { normalized: Record; advanced: unknown[]; extensions: unknown[]; + buzzAgentMcpServers: McpServerConfig[]; sources: Record; } { // Goose running — mixed origins, override on model @@ -1360,6 +1374,7 @@ function buildMockConfigSurface(pubkey: string): { { name: "web_search", kind: "stdio", enabled: true }, { name: "memory", kind: "stdio", enabled: false }, ], + buzzAgentMcpServers: [], sources: { acpNative: "available", acpConfigOptions: "available", @@ -1424,6 +1439,7 @@ function buildMockConfigSurface(pubkey: string): { { name: "filesystem", kind: "mcp", enabled: true }, { name: "github", kind: "mcp", enabled: true }, ], + buzzAgentMcpServers: [], sources: { acpNative: "available", acpConfigOptions: "available", @@ -1481,6 +1497,7 @@ function buildMockConfigSurface(pubkey: string): { }, advanced: [], extensions: [{ name: "developer", kind: "stdio", enabled: true }], + buzzAgentMcpServers: [], sources: { acpNative: "pending", acpConfigOptions: "pending", @@ -1560,6 +1577,7 @@ function buildMockConfigSurface(pubkey: string): { { name: "filesystem", kind: "mcp", enabled: true }, { name: "github", kind: "mcp", enabled: true }, ], + buzzAgentMcpServers: [], sources: { acpNative: "notApplicable", acpConfigOptions: "notApplicable", @@ -1619,6 +1637,7 @@ function buildMockConfigSurface(pubkey: string): { }, advanced: [], extensions: [{ name: "web_search", kind: "stdio", enabled: true }], + buzzAgentMcpServers: [], sources: { acpNative: "available", acpConfigOptions: "available", @@ -1679,6 +1698,7 @@ function buildMockConfigSurface(pubkey: string): { }, advanced: [], extensions: [], + buzzAgentMcpServers: [], sources: { acpNative: "available", acpConfigOptions: "available", @@ -1695,6 +1715,17 @@ function buildMockConfigSurface(pubkey: string): { runtimeLabel: "Buzz Agent", advanced: [], extensions: [], + // Effective merged (global < definition < agent, enabled-only) servers — + // "what runs." Exercises the WYSIWYG read-only display by default. + buzzAgentMcpServers: [ + { + name: "filesystem", + command: "npx", + args: ["-y", "@modelcontextprotocol/server-filesystem", "/tmp"], + env: [], + enabled: true, + }, + ] satisfies McpServerConfig[], sources: { ...gooseSurface.sources, configFilePath: null, @@ -6633,6 +6664,7 @@ async function handleCreatePersona(args: { avatarUrl?: string; systemPrompt: string; envVars?: Record; + mcpServers?: McpServerConfig[]; behavior?: PersonaBehaviorInput; }; }): Promise { @@ -6646,6 +6678,7 @@ async function handleCreatePersona(args: { is_active: true, source_team: null, env_vars: { ...(args.input.envVars ?? {}) }, + mcp_servers: (args.input.mcpServers ?? []).map(cloneMcpServer), created_at: now, updated_at: now, }; @@ -6661,6 +6694,7 @@ async function handleUpdatePersona(args: { avatarUrl?: string; systemPrompt: string; envVars?: Record; + mcpServers?: McpServerConfig[]; behavior?: PersonaBehaviorInput; }; }): Promise { @@ -6681,6 +6715,10 @@ async function handleUpdatePersona(args: { // Absent = preserve; present = replace entirely (matches Rust handler). persona.env_vars = { ...args.input.envVars }; } + if (args.input.mcpServers !== undefined) { + // Same absent-vs-present contract as envVars. + persona.mcp_servers = args.input.mcpServers.map(cloneMcpServer); + } applyMockPersonaBehavior(persona, args.input.behavior); persona.updated_at = new Date().toISOString(); @@ -6924,6 +6962,7 @@ async function handleCreateManagedAgent( avatarUrl?: string; model?: string; envVars?: Record; + mcpServers?: McpServerConfig[]; spawnAfterCreate?: boolean; startOnAppLaunch?: boolean; backend?: @@ -6997,6 +7036,7 @@ async function handleCreateManagedAgent( avatar_url: avatarUrl, model: args.input.model?.trim() || null, env_vars: { ...(args.input.envVars ?? {}) }, + mcp_servers: (args.input.mcpServers ?? []).map(cloneMcpServer), status: args.input.spawnAfterCreate ? "running" : "stopped", pid: args.input.spawnAfterCreate ? 42000 + mockManagedAgents.length : null, created_at: now, @@ -7195,6 +7235,7 @@ async function handleUpdateManagedAgent(args: { model?: string | null; systemPrompt?: string | null; envVars?: Record; + mcpServers?: McpServerConfig[]; respondTo?: "owner-only" | "allowlist" | "anyone"; respondToAllowlist?: string[]; }; @@ -7212,6 +7253,10 @@ async function handleUpdateManagedAgent(args: { if (args.input.envVars !== undefined) { agent.env_vars = { ...args.input.envVars }; } + if (args.input.mcpServers !== undefined) { + // Same absent-vs-present contract as envVars. + agent.mcp_servers = args.input.mcpServers.map(cloneMcpServer); + } if (args.input.respondTo !== undefined) { agent.respond_to = args.input.respondTo; } @@ -9258,10 +9303,11 @@ export function maybeInstallE2eTauriMocks() { } case "get_global_agent_config": { // Return the mock global agent config if provided; otherwise return - // an empty config (no global provider, model, or env vars). + // an empty config (no global provider, model, env vars, or MCP servers). return ( config?.mock?.globalAgentConfig ?? { env_vars: {}, + mcp_servers: [], provider: null, model: null, } @@ -9275,6 +9321,7 @@ export function maybeInstallE2eTauriMocks() { payload as { config: { env_vars: Record; + mcp_servers?: McpServerConfig[]; provider: string | null; model: string | null; }; From 95afd5996c61deacf288105e906e1cf60de87c43 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Mon, 13 Jul 2026 20:18:08 -0400 Subject: [PATCH 07/17] feat(desktop): surface effective-merged buzz-agent MCP servers in read-only panel Wires the buzzAgentSlot prop that McpServersSection has exposed since #1780 with the RuntimeConfigSurface.buzzAgentMcpServers field added in P0. AgentConfigPanel now renders the effective merged (global < definition < agent) enabled-server list via a new BuzzAgentMcpServerRow, giving Will's WYSIWYG requirement (Q2) its read-only half. Also fixes McpServersSection's fallback text, which previously rendered unreachable for buzz-agent (the extensions-based branch never executes there since buzz-agent always reports empty extensions). Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../features/agents/ui/AgentConfigPanel.tsx | 26 +++++++- .../features/agents/ui/McpServersSection.tsx | 65 +++++++++++++++++-- 2 files changed, 84 insertions(+), 7 deletions(-) diff --git a/desktop/src/features/agents/ui/AgentConfigPanel.tsx b/desktop/src/features/agents/ui/AgentConfigPanel.tsx index 26ecab3c0..c68563425 100644 --- a/desktop/src/features/agents/ui/AgentConfigPanel.tsx +++ b/desktop/src/features/agents/ui/AgentConfigPanel.tsx @@ -17,7 +17,7 @@ import { useAgentConfigSurface } from "../hooks"; import { cn } from "@/shared/lib/cn"; import { copyTextToClipboard } from "@/shared/lib/clipboard"; import { Spinner } from "@/shared/ui/spinner"; -import { McpServersSection } from "./McpServersSection"; +import { McpServersSection, BuzzAgentMcpServerRow } from "./McpServersSection"; import type { ConfigField, ConfigOrigin, @@ -366,8 +366,15 @@ export function AgentConfigPanel({ ); } - const { normalized, advanced, extensions, runtimeId, sources, isPreSpawn } = - data; + const { + normalized, + advanced, + extensions, + runtimeId, + sources, + isPreSpawn, + buzzAgentMcpServers, + } = data; const configFilePath = sources.configFilePath; const normalizedEntries = ( @@ -419,6 +426,19 @@ export function AgentConfigPanel({ 0 ? ( +
+ {buzzAgentMcpServers.map((server) => ( + + ))} +
+ ) : undefined + } extensions={extensions} runtimeId={runtimeId} variant={advancedMode === "flat" ? "profile" : "compact"} diff --git a/desktop/src/features/agents/ui/McpServersSection.tsx b/desktop/src/features/agents/ui/McpServersSection.tsx index 3e6c93ca5..f20beda4f 100644 --- a/desktop/src/features/agents/ui/McpServersSection.tsx +++ b/desktop/src/features/agents/ui/McpServersSection.tsx @@ -1,6 +1,6 @@ import type * as React from "react"; import { CheckCircle2, CircleSlash } from "lucide-react"; -import type { ExtensionEntry } from "@/shared/api/types"; +import type { ExtensionEntry, McpServerConfig } from "@/shared/api/types"; import { cn } from "@/shared/lib/cn"; type McpServersSectionProps = { @@ -38,9 +38,18 @@ export function McpServersSection({ MCP Servers

- {isBuzzAgent && buzzAgentSlot ? buzzAgentSlot : null} - - {extensions.length > 0 ? ( + {isBuzzAgent ? ( + (buzzAgentSlot ?? ( +

+ No custom servers configured +

+ )) + ) : extensions.length > 0 ? (
{extensions.map((extension) => ( ); } + +/** + * Read-only row for an *effective merged* buzz-agent MCP server — "what + * runs." Shows the resolved command line rather than `kind`/enabled state + * (unlike `McpServerRow`), since every entry here is already enabled by + * contract (`RuntimeConfigSurface.buzzAgentMcpServers` is enabled-only). + */ +export function BuzzAgentMcpServerRow({ + server, + variant, +}: { + server: McpServerConfig; + variant: "compact" | "profile"; +}) { + const StatusIcon = server.enabled ? CheckCircle2 : CircleSlash; + const commandLine = [server.command, ...server.args] + .filter(Boolean) + .join(" "); + + return ( +
+ + + + + + {server.name} + + + {commandLine || "stdio"} + + +
+ ); +} From 7db5831597219e02606382abc231d58753e8158e Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Mon, 13 Jul 2026 21:29:57 -0400 Subject: [PATCH 08/17] feat(desktop): add McpServersEditor component (P2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New row-based editor for the local buzz-agent MCP server layer, modeled on EnvVarsEditor: per-server Name/Command/repeatable-Args/repeatable-Env rows plus an Enabled toggle, client-side validation mirroring the Rust validate_mcp_servers grammar (name charset, length, __, reserved name, uniqueness, command-required-when-enabled), and read-only inherited-layer rows a same-named local row overrides or masks. Command and Args are separate fields (Q1) — no combined command string, no shell-quote dependency. Not yet mounted in any dialog (P3). Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../agents/ui/McpServersEditor.test.mjs | 303 ++++++++ .../features/agents/ui/McpServersEditor.tsx | 699 ++++++++++++++++++ 2 files changed, 1002 insertions(+) create mode 100644 desktop/src/features/agents/ui/McpServersEditor.test.mjs create mode 100644 desktop/src/features/agents/ui/McpServersEditor.tsx diff --git a/desktop/src/features/agents/ui/McpServersEditor.test.mjs b/desktop/src/features/agents/ui/McpServersEditor.test.mjs new file mode 100644 index 000000000..446111003 --- /dev/null +++ b/desktop/src/features/agents/ui/McpServersEditor.test.mjs @@ -0,0 +1,303 @@ +/** + * Unit tests for the McpServersEditor state helpers. + * + * Tests four invariants: + * + * 1. validateMcpServerName / validateMcpServerRow mirror the Rust + * `validate_mcp_servers` grammar (name charset, length, `__`, + * reserved name, uniqueness, command-required-when-enabled). + * 2. toRows / toServers round-trip a server list, and toServers skips + * empty-name rows and blank arg/env rows (no phantom empty args or + * env entries reach the emitted config). + * 3. serversEqual detects the resync-guard transitions the effect relies + * on (reordering, field-level diffs, arg/env diffs). + * 4. mergeMcpServersByName replaces whole entries by name across layers, + * in the same order the Rust `merge_mcp_servers` BTreeMap produces. + * + * These are pure-logic tests — no React renderer needed. + */ + +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { + validateMcpServerName, + validateMcpServerRow, + toRows, + toServers, + serversEqual, + mergeMcpServersByName, + MCP_SERVER_NAME_MAX_LEN, + MCP_RESERVED_SERVER_NAME, +} from "./McpServersEditor.tsx"; + +// ── Invariant 1: name grammar mirrors validate_mcp_servers ───────────────── + +test("validateMcpServerName_empty_name_is_required", () => { + assert.equal(validateMcpServerName(""), "Name is required."); +}); + +test("validateMcpServerName_valid_name_returns_null", () => { + assert.equal(validateMcpServerName("my-server_1"), null); +}); + +test("validateMcpServerName_over_max_length_rejected", () => { + const tooLong = "a".repeat(MCP_SERVER_NAME_MAX_LEN + 1); + const error = validateMcpServerName(tooLong); + assert.match(error, /exceeds the maximum length/); +}); + +test("validateMcpServerName_at_max_length_accepted", () => { + const atMax = "a".repeat(MCP_SERVER_NAME_MAX_LEN); + assert.equal(validateMcpServerName(atMax), null); +}); + +test("validateMcpServerName_rejects_non_ascii_alnum_characters", () => { + // Rust grammar: ASCII alphanumeric, `_`, `-` only. Space, dot, and + // non-ASCII letters must all be rejected. + assert.match(validateMcpServerName("my server"), /Only letters/); + assert.match(validateMcpServerName("my.server"), /Only letters/); + assert.match(validateMcpServerName("café"), /Only letters/); +}); + +test("validateMcpServerName_rejects_double_underscore", () => { + assert.match(validateMcpServerName("my__server"), /cannot contain/); +}); + +test("validateMcpServerName_rejects_reserved_name", () => { + const error = validateMcpServerName(MCP_RESERVED_SERVER_NAME); + assert.match(error, /is reserved/); +}); + +test("validateMcpServerRow_flags_duplicate_name_in_layer", () => { + const row = { name: "shared", command: "npx", enabled: true }; + const error = validateMcpServerRow(row, new Set(["shared"])); + assert.equal(error, "Server names must be unique."); +}); + +test("validateMcpServerRow_enabled_without_command_is_rejected", () => { + const row = { name: "srv", command: "", enabled: true }; + const error = validateMcpServerRow(row, new Set()); + assert.equal(error, "Command is required for an enabled server."); +}); + +test("validateMcpServerRow_disabled_without_command_is_valid", () => { + // A disabled row's only purpose may be to mask a lower-precedence + // server by name, so it may omit command (mirrors Rust: the + // command-required check is gated on `server.enabled`). + const row = { name: "srv", command: "", enabled: false }; + assert.equal(validateMcpServerRow(row, new Set()), null); +}); + +test("validateMcpServerRow_valid_enabled_row_returns_null", () => { + const row = { name: "srv", command: "npx", enabled: true }; + assert.equal(validateMcpServerRow(row, new Set(["other"])), null); +}); + +// ── Invariant 2: toRows / toServers round-trip and blank-row skipping ────── + +test("toRows_produces_one_row_per_server_with_stable_shape", () => { + const servers = [ + { + name: "fs", + command: "npx", + args: ["-y", "@modelcontextprotocol/server-filesystem"], + env: [{ name: "ROOT", value: "/tmp" }], + enabled: true, + }, + ]; + const rows = toRows(servers); + assert.equal(rows.length, 1); + assert.equal(rows[0].name, "fs"); + assert.equal(rows[0].command, "npx"); + assert.deepEqual( + rows[0].args.map((a) => a.value), + ["-y", "@modelcontextprotocol/server-filesystem"], + ); + assert.deepEqual( + rows[0].env.map((e) => [e.name, e.value]), + [["ROOT", "/tmp"]], + ); + assert.equal(rows[0].enabled, true); + assert.equal(typeof rows[0].id, "string"); +}); + +test("toServers_round_trips_toRows_output", () => { + const servers = [ + { + name: "fs", + command: "npx", + args: ["-y", "server"], + env: [{ name: "ROOT", value: "/tmp" }], + enabled: true, + }, + { name: "masked", command: "", args: [], env: [], enabled: false }, + ]; + const rows = toRows(servers); + assert.ok(serversEqual(toServers(rows), servers)); +}); + +test("toServers_skips_empty_name_rows", () => { + // A freshly-added row with no name typed yet must not become a + // phantom entry in the emitted config. + const rows = toRows([]); + rows.push({ + id: "new", + name: "", + command: "npx", + args: [], + env: [], + enabled: true, + }); + assert.deepEqual(toServers(rows), []); +}); + +test("toServers_skips_blank_arg_rows", () => { + // An unfilled "Add argument" row must not inject an empty-string arg + // into the spawned command. + const rows = toRows([ + { name: "srv", command: "npx", args: ["-y"], env: [], enabled: true }, + ]); + rows[0].args.push({ id: "blank", value: "" }); + const [server] = toServers(rows); + assert.deepEqual(server.args, ["-y"]); +}); + +test("toServers_skips_blank_env_rows_but_keeps_named_ones", () => { + const rows = toRows([ + { name: "srv", command: "npx", args: [], env: [], enabled: true }, + ]); + rows[0].env.push({ id: "blank", name: "", value: "leftover" }); + rows[0].env.push({ id: "named", name: "TOKEN", value: "abc" }); + const [server] = toServers(rows); + assert.deepEqual(server.env, [{ name: "TOKEN", value: "abc" }]); +}); + +test("toServers_duplicate_env_names_collapse_last_write_wins", () => { + // The backend's per-server env is a Vec, not a map, so it rejects + // duplicate names outright (`mcp_servers.rs` "has duplicate env var"). + // The editor must dedupe client-side before that round-trip, the same + // way EnvVarsEditor's object-keyed `toRecord` does implicitly. + const rows = toRows([ + { name: "srv", command: "npx", args: [], env: [], enabled: true }, + ]); + rows[0].env.push({ id: "a", name: "TOKEN", value: "first" }); + rows[0].env.push({ id: "b", name: "TOKEN", value: "second" }); + const [server] = toServers(rows); + assert.deepEqual(server.env, [{ name: "TOKEN", value: "second" }]); +}); + +// ── Invariant 3: serversEqual — resync-guard transitions ─────────────────── + +test("serversEqual_identical_lists_are_equal", () => { + const servers = [ + { name: "a", command: "npx", args: ["-y"], env: [], enabled: true }, + ]; + assert.ok(serversEqual(servers, structuredClone(servers))); +}); + +test("serversEqual_different_lengths_are_not_equal", () => { + const a = [{ name: "a", command: "npx", args: [], env: [], enabled: true }]; + assert.equal(serversEqual(a, []), false); +}); + +test("serversEqual_detects_arg_diff", () => { + const a = [ + { name: "a", command: "npx", args: ["-y"], env: [], enabled: true }, + ]; + const b = [ + { name: "a", command: "npx", args: ["-x"], env: [], enabled: true }, + ]; + assert.equal(serversEqual(a, b), false); +}); + +test("serversEqual_detects_env_diff", () => { + const a = [ + { + name: "a", + command: "npx", + args: [], + env: [{ name: "K", value: "1" }], + enabled: true, + }, + ]; + const b = [ + { + name: "a", + command: "npx", + args: [], + env: [{ name: "K", value: "2" }], + enabled: true, + }, + ]; + assert.equal(serversEqual(a, b), false); +}); + +test("serversEqual_detects_enabled_diff", () => { + const a = [{ name: "a", command: "npx", args: [], env: [], enabled: true }]; + const b = [{ name: "a", command: "npx", args: [], env: [], enabled: false }]; + assert.equal(serversEqual(a, b), false); +}); + +// ── Invariant 4: mergeMcpServersByName — layered replace-by-name ────────── + +test("mergeMcpServersByName_higher_layer_replaces_whole_entry", () => { + const global = [ + { + name: "fs", + command: "npx", + args: ["-y", "global-pkg"], + env: [], + enabled: true, + }, + ]; + const agent = [ + { + name: "fs", + command: "npx", + args: ["-y", "agent-pkg"], + env: [], + enabled: true, + }, + ]; + const merged = mergeMcpServersByName(global, agent); + assert.equal(merged.length, 1); + assert.deepEqual(merged[0].args, ["-y", "agent-pkg"]); +}); + +test("mergeMcpServersByName_disabled_higher_layer_masks_lower_layer", () => { + const global = [ + { name: "fs", command: "npx", args: ["-y"], env: [], enabled: true }, + ]; + const definition = [ + { name: "fs", command: "", args: [], env: [], enabled: false }, + ]; + const merged = mergeMcpServersByName(global, definition); + assert.equal(merged.length, 1); + assert.equal( + merged[0].enabled, + false, + "disabled override must mask, not disappear", + ); +}); + +test("mergeMcpServersByName_distinct_names_from_all_layers_are_kept", () => { + const global = [ + { name: "g", command: "npx", args: [], env: [], enabled: true }, + ]; + const definition = [ + { name: "d", command: "npx", args: [], env: [], enabled: true }, + ]; + const agent = [ + { name: "a", command: "npx", args: [], env: [], enabled: true }, + ]; + const merged = mergeMcpServersByName(global, definition, agent); + assert.deepEqual( + merged.map((s) => s.name), + ["a", "d", "g"], + ); +}); + +test("mergeMcpServersByName_empty_layers_produce_empty_result", () => { + assert.deepEqual(mergeMcpServersByName([], [], []), []); +}); diff --git a/desktop/src/features/agents/ui/McpServersEditor.tsx b/desktop/src/features/agents/ui/McpServersEditor.tsx new file mode 100644 index 000000000..70e55c640 --- /dev/null +++ b/desktop/src/features/agents/ui/McpServersEditor.tsx @@ -0,0 +1,699 @@ +/** + * Editor for the local-only buzz-agent MCP server layer (Q1/Q3, PLANS/PR3). + * + * Modeled on `EnvVarsEditor`: an ordered row list keyed by stable ids (so + * duplicate/empty names don't collapse mid-edit), inherited-layer read-only + * rows that a same-named local row overrides or masks, and pure emit/resync + * helpers exported for unit testing. + * + * STDIO only — no SSE/HTTP, no timeout, no headers (the backend doesn't + * support them). Command and Args are separate fields (Q1): no combined + * command string, no shell-quote dependency. + */ +import { + AlertCircle, + CheckCircle2, + CircleSlash, + Lock, + Plus, + X, +} from "lucide-react"; +import * as React from "react"; + +import type { McpServerConfig } from "@/shared/api/types"; +import { Button } from "@/shared/ui/button"; +import { Input } from "@/shared/ui/input"; +import { Switch } from "@/shared/ui/switch"; +import { cn } from "@/shared/lib/cn"; +import { + PERSONA_FIELD_CONTROL_CLASS, + PERSONA_FIELD_SHELL_CLASS, +} from "./personaDialogPickers"; + +export type McpServersValue = McpServerConfig[]; + +/** Max byte length of a server name. Mirrors `MAX_SERVER_NAME_LEN` in `mcp_servers.rs`. */ +export const MCP_SERVER_NAME_MAX_LEN = 128; +/** The bundled sidecar's reserved name. Mirrors `RESERVED_SERVER_NAME`. */ +export const MCP_RESERVED_SERVER_NAME = "buzz-dev-mcp"; +/** Effective/per-layer enabled-server cap. Mirrors `MAX_USER_MCP_SERVERS`. */ +export const MAX_USER_MCP_SERVERS = 15; + +const NAME_CHARS_RE = /^[A-Za-z0-9_-]*$/; + +/** + * Client-side mirror of the Rust `validate_mcp_servers` name grammar, for + * inline feedback before a save round-trip. Returns `null` when valid. + * Exported for unit tests. + */ +export function validateMcpServerName(name: string): string | null { + if (name.length === 0) return "Name is required."; + if (name.length > MCP_SERVER_NAME_MAX_LEN) { + return `Name exceeds the maximum length (${MCP_SERVER_NAME_MAX_LEN} bytes).`; + } + if (!NAME_CHARS_RE.test(name)) { + return "Only letters, numbers, `_`, and `-` are allowed."; + } + if (name.includes("__")) return "Name cannot contain `__`."; + if (name === MCP_RESERVED_SERVER_NAME) { + return `"${MCP_RESERVED_SERVER_NAME}" is reserved.`; + } + return null; +} + +/** + * Full per-row validation: name grammar + uniqueness within the layer + + * command-required-when-enabled. `otherNames` is every OTHER row's name in + * the same layer (excluding this row), so a collision flags both rows. + * Exported for unit tests. + */ +export function validateMcpServerRow( + row: { name: string; command: string; enabled: boolean }, + otherNames: ReadonlySet, +): string | null { + const nameError = validateMcpServerName(row.name); + if (nameError) return nameError; + if (otherNames.has(row.name)) return "Server names must be unique."; + if (row.enabled && row.command.trim().length === 0) { + return "Command is required for an enabled server."; + } + return null; +} + +type ArgRow = { id: string; value: string }; +type EnvRow = { id: string; name: string; value: string }; +type ServerRow = { + id: string; + name: string; + command: string; + args: ArgRow[]; + env: EnvRow[]; + enabled: boolean; +}; + +/** Build a rows array from a server-config list. Exported for unit tests. */ +export function toRows(servers: McpServersValue): ServerRow[] { + return servers.map((server) => ({ + id: crypto.randomUUID(), + name: server.name, + command: server.command, + args: server.args.map((value) => ({ id: crypto.randomUUID(), value })), + env: server.env.map((entry) => ({ + id: crypto.randomUUID(), + name: entry.name, + value: entry.value, + })), + enabled: server.enabled, + })); +} + +/** + * Collapse rows back to a server-config list. Rows with an empty name are + * mid-edit (the user hasn't identified the server yet) and are skipped, + * mirroring `EnvVarsEditor`'s empty-key skip. Blank arg/env rows are skipped + * too — an unfilled "Add argument" row must never inject an empty-string + * arg into the spawned command. Duplicate env var names within a server + * collapse last-write-wins, matching `EnvVarsEditor`'s `toRecord` (its + * object-keyed shape does this automatically; the per-server `env` here is + * an array per the Rust `Vec` shape, so it needs the same + * dedupe applied explicitly to avoid a save-time rejection from the Rust + * validator's duplicate-env-var check). Exported for unit tests. + */ +export function toServers(rows: readonly ServerRow[]): McpServersValue { + const out: McpServerConfig[] = []; + for (const row of rows) { + if (row.name.length === 0) continue; + const env = new Map(); + for (const entry of row.env) { + if (entry.name.length > 0) env.set(entry.name, entry.value); + } + out.push({ + name: row.name, + command: row.command, + args: row.args.map((a) => a.value).filter((v) => v.length > 0), + env: Array.from(env, ([name, value]) => ({ name, value })), + enabled: row.enabled, + }); + } + return out; +} + +/** True iff two server lists are element-wise identical. Exported for unit tests. */ +export function serversEqual(a: McpServersValue, b: McpServersValue): boolean { + if (a.length !== b.length) return false; + for (let i = 0; i < a.length; i++) { + const x = a[i]; + const y = b[i]; + if ( + x.name !== y.name || + x.command !== y.command || + x.enabled !== y.enabled + ) { + return false; + } + if ( + x.args.length !== y.args.length || + x.args.some((v, j) => v !== y.args[j]) + ) { + return false; + } + if (x.env.length !== y.env.length) return false; + for (let j = 0; j < x.env.length; j++) { + if ( + x.env[j].name !== y.env[j].name || + x.env[j].value !== y.env[j].value + ) { + return false; + } + } + } + return true; +} + +/** + * Merge server layers by name — later layers replace earlier entries whole, + * matching the Rust `merge_mcp_servers` replace-by-name rule. Unlike the + * Rust helper this keeps disabled entries and does not enforce the + * effective-server cap: it is a DISPLAY merge for the editor's read-only + * "inherited" rows, not the runtime WYSIWYG surface (that's + * `RuntimeConfigSurface.buzzAgentMcpServers`, computed server-side). Sorted + * by name for the same deterministic order the backend's `BTreeMap`-backed + * merge produces. Exported for unit tests and dialog callers. + */ +export function mergeMcpServersByName( + ...layers: readonly McpServersValue[] +): McpServersValue { + const merged = new Map(); + for (const layer of layers) { + for (const server of layer) { + merged.set(server.name, server); + } + } + return Array.from(merged.values()).sort((a, b) => + a.name.localeCompare(b.name), + ); +} + +/** True when a row has any content beyond a bare name — used to decide + * whether an empty-name row's "Name is required" hint should show (a + * pristine just-added row stays quiet; a row with typed content but no name + * yet warns, so filling in command/args/env doesn't silently go unsaved). */ +function rowHasContent(row: ServerRow): boolean { + return ( + row.command.trim().length > 0 || + row.args.some((a) => a.value.trim().length > 0) || + row.env.some( + (e) => e.name.trim().length > 0 || e.value.trim().length > 0, + ) || + !row.enabled + ); +} + +function commandLine(command: string, args: readonly string[]): string { + return [command, ...args].filter(Boolean).join(" "); +} + +export type McpServersEditorProps = { + /** This layer's own MCP server list. */ + value: McpServersValue; + /** Called with the next list whenever the user edits a row. */ + onChange: (next: McpServersValue) => void; + /** Read-only servers inherited from lower layer(s), already merged + * (see `mergeMcpServersByName`). A local row with a matching name + * overrides (or, if disabled, masks) the inherited entry. */ + inheritedServers?: McpServersValue; + /** Label for the inherited source (e.g. "global" or "global + persona"). */ + inheritedLabel?: string; + /** Section header. Defaults to "MCP servers". */ + label?: string; + /** Short description below the header. */ + helperText?: string; + /** Disables all editing. */ + disabled?: boolean; +}; + +/** + * Per-server row editor: Name / Command / Args (repeatable) / Env + * (repeatable) / Enabled toggle, plus read-only inherited-layer rows. + */ +export function McpServersEditor({ + value, + onChange, + inheritedServers = [], + inheritedLabel = "inherited", + label = "MCP servers", + helperText, + disabled = false, +}: McpServersEditorProps) { + const [rows, setRows] = React.useState(() => toRows(value)); + const lastEmitted = React.useRef(value); + + // Resync from `value` when the parent supplies a value we did not just + // emit (e.g. dialog reopened against a different persona/agent). Mirrors + // EnvVarsEditor's `recordsEqual` resync guard. + React.useEffect(() => { + if (!serversEqual(lastEmitted.current, value)) { + lastEmitted.current = value; + setRows(toRows(value)); + } + }, [value]); + + function emit(nextRows: ServerRow[]) { + setRows(nextRows); + const servers = toServers(nextRows); + lastEmitted.current = servers; + onChange(servers); + } + + function updateRow(id: string, patch: Partial) { + emit(rows.map((row) => (row.id === id ? { ...row, ...patch } : row))); + } + + function removeRow(id: string) { + emit(rows.filter((row) => row.id !== id)); + } + + function addRow(seed?: Partial>) { + emit([ + ...rows, + { + id: crypto.randomUUID(), + name: seed?.name ?? "", + command: "", + args: [], + env: [], + enabled: seed?.enabled ?? true, + }, + ]); + } + + function addArg(rowId: string) { + const row = rows.find((r) => r.id === rowId); + if (!row) return; + updateRow(rowId, { + args: [...row.args, { id: crypto.randomUUID(), value: "" }], + }); + } + + function updateArg(rowId: string, argId: string, argValue: string) { + const row = rows.find((r) => r.id === rowId); + if (!row) return; + updateRow(rowId, { + args: row.args.map((a) => + a.id === argId ? { ...a, value: argValue } : a, + ), + }); + } + + function removeArg(rowId: string, argId: string) { + const row = rows.find((r) => r.id === rowId); + if (!row) return; + updateRow(rowId, { args: row.args.filter((a) => a.id !== argId) }); + } + + function addEnvRow(rowId: string) { + const row = rows.find((r) => r.id === rowId); + if (!row) return; + updateRow(rowId, { + env: [...row.env, { id: crypto.randomUUID(), name: "", value: "" }], + }); + } + + function updateEnvRow( + rowId: string, + envId: string, + patch: Partial>, + ) { + const row = rows.find((r) => r.id === rowId); + if (!row) return; + updateRow(rowId, { + env: row.env.map((e) => (e.id === envId ? { ...e, ...patch } : e)), + }); + } + + function removeEnvRow(rowId: string, envId: string) { + const row = rows.find((r) => r.id === rowId); + if (!row) return; + updateRow(rowId, { env: row.env.filter((e) => e.id !== envId) }); + } + + const enabledCount = rows.filter((r) => r.enabled).length; + const atCap = enabledCount >= MAX_USER_MCP_SERVERS; + const visibleInherited = inheritedServers.filter( + (server) => !rows.some((row) => row.name === server.name), + ); + + return ( +
+
+
{label}
+ {helperText ? ( +

{helperText}

+ ) : null} +
+
+ {visibleInherited.map((server) => ( + addRow({ name: server.name, enabled: false })} + disabled={disabled} + server={server} + /> + ))} + + {rows.length === 0 && visibleInherited.length === 0 ? ( +

+ No MCP servers configured. +

+ ) : null} + + {rows.map((row) => { + const otherNames = new Set( + rows.filter((r) => r.id !== row.id).map((r) => r.name), + ); + const rowError = validateMcpServerRow(row, otherNames); + const showError = Boolean( + rowError && (row.name.length > 0 || rowHasContent(row)), + ); + const overridesInherited = inheritedServers.find( + (s) => s.name === row.name, + ); + + return ( +
+
+
+ + updateRow(row.id, { name: event.target.value }) + } + placeholder="server-name" + value={row.name} + /> +
+
+ + updateRow(row.id, { command: event.target.value }) + } + placeholder="npx" + value={row.command} + /> +
+ + updateRow(row.id, { enabled: checked }) + } + /> + +
+ + {showError ? ( +

+ + {rowError} +

+ ) : null} + {!showError && overridesInherited ? ( +

+ {row.enabled ? "Overrides" : "Masks"} {inheritedLabel} server{" "} + + {commandLine( + overridesInherited.command, + overridesInherited.args, + ) || "stdio"} + +

+ ) : null} + + {/* Args */} +
+

+ Args +

+ {row.args.map((arg) => ( +
+
+ + updateArg(row.id, arg.id, event.target.value) + } + value={arg.value} + /> +
+ +
+ ))} + +
+ + {/* Env */} +
+

+ Environment +

+ {row.env.map((entry) => ( +
+
+ + updateEnvRow(row.id, entry.id, { + name: event.target.value, + }) + } + placeholder="VARIABLE_NAME" + value={entry.name} + /> +
+
+ + updateEnvRow(row.id, entry.id, { + value: event.target.value, + }) + } + placeholder="value" + value={entry.value} + /> +
+ +
+ ))} + +
+
+ ); + })} + +
+ + {atCap ? ( +

+ {MAX_USER_MCP_SERVERS}-server limit reached for this layer. +

+ ) : null} +
+
+
+ ); +} + +function InheritedServerRow({ + disabled, + inheritedLabel, + onMask, + server, +}: { + disabled: boolean; + inheritedLabel: string; + onMask: () => void; + server: McpServerConfig; +}) { + const StatusIcon = server.enabled ? CheckCircle2 : CircleSlash; + const line = commandLine(server.command, server.args); + + return ( +
+
+ + + + + {server.name} + + + Inherited from {inheritedLabel} + + {line ? ( + + {line} + + ) : null} + +
+ +
+ ); +} From 17893e4c7f335080ec0e446325ab6c2b65875f4b Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Mon, 13 Jul 2026 21:54:52 -0400 Subject: [PATCH 09/17] feat(desktop): mount McpServersEditor in all three agent dialogs (P3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wires the McpServersEditor into the global config, definition, and instance-edit dialogs — completing the UI layer for the editable MCP servers feature. - GlobalAgentConfigFields: global base-layer editor (no inherited rows) - PersonaAdvancedFields: definition-layer editor with global as inherited - AgentDefinitionDialog: state, init reset, advanced-expand gate, submit - EditAgentAdvancedFields: per-instance editor with merged global+persona inherited rows; new mcpServers/inheritedMcpServers/onMcpServersChange props - AgentInstanceEditDialog: mcpServers state, inheritedMcpServers memo (mergeMcpServersByName(global, persona)), absent-preserves submit wire, and EditAgentAdvancedFields prop threading - check-file-sizes.mjs: bump AgentInstanceEditDialog override 1180→1198 (+18 lines from load-bearing P3 prop threading) --- desktop/scripts/check-file-sizes.mjs | 4 ++- .../agents/ui/AgentDefinitionDialog.tsx | 10 +++++++ .../agents/ui/AgentInstanceEditDialog.tsx | 26 +++++++++++++++++++ .../agents/ui/EditAgentAdvancedFields.tsx | 20 ++++++++++++++ .../agents/ui/GlobalAgentConfigFields.tsx | 18 +++++++++++++ .../agents/ui/PersonaAdvancedFields.tsx | 19 ++++++++++++++ 6 files changed, 96 insertions(+), 1 deletion(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 778013122..b4f8a9933 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -486,7 +486,9 @@ const overrides = new Map([ // +2 provider-aware effort: model/provider props threaded to BuzzAgentModelTuningFields. // +15 provider/model dropdown fixes: useBakedBuildEnvKeysQuery + hideProviderIds // for Databricks v1 gate; prospectiveRuntimeId default fallback for builtins. - ["src/features/agents/ui/AgentInstanceEditDialog.tsx", 1180], + // +18 PR3 MCP servers UI: mcpServers state, inheritedMcpServers memo, + // submit wire, and EditAgentAdvancedFields prop threading. + ["src/features/agents/ui/AgentInstanceEditDialog.tsx", 1198], // AgentDefinitionDialog grew past 1000 with the following load-bearing fixes: // isRuntimeAutoSeededRef tracking for edit-mode seeding (Fizz shows models); // runtimeSupportsLlmProviderSelection guard on discovery provider (codex fix); diff --git a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx index ef7b80354..e2983b3d7 100644 --- a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx +++ b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx @@ -16,6 +16,7 @@ import { Textarea } from "@/shared/ui/textarea"; import { AgentCreationPreview } from "./AgentCreationPreview"; import { PersonaDropdownField } from "./PersonaDropdownField"; import type { EnvVarsValue } from "./EnvVarsEditor"; +import type { McpServersValue } from "./McpServersEditor"; import { PersonaAdvancedFields } from "./PersonaAdvancedFields"; import { PersonaModelField } from "./PersonaModelField"; import { @@ -128,6 +129,7 @@ export function AgentDefinitionDialog({ React.useState(false); const [namePoolText, setNamePoolText] = React.useState(""); const [envVars, setEnvVars] = React.useState({}); + const [mcpServers, setMcpServers] = React.useState([]); const [behaviorDraft, setBehaviorDraft] = React.useState( emptyPersonaBehaviorDraft, ); @@ -179,14 +181,18 @@ export function AgentDefinitionDialog({ : ""; const nextEnvVars = "envVars" in initialValues ? (initialValues.envVars ?? {}) : {}; + const nextMcpServers = + "mcpServers" in initialValues ? (initialValues.mcpServers ?? []) : []; const nextBehaviorDraft = draftFromBehavior(initialValues.behavior); behaviorSeedRef.current = draftFromBehavior(initialValues.behavior); setBehaviorDraft(nextBehaviorDraft); setNamePoolText(nextNamePoolText); setEnvVars(nextEnvVars); + setMcpServers(nextMcpServers); setShowAdvancedFields( nextNamePoolText.trim().length > 0 || Object.keys(nextEnvVars).length > 0 || + nextMcpServers.length > 0 || nextBehaviorDraft.respondTo !== null || nextBehaviorDraft.parallelism.trim().length > 0, ); @@ -282,6 +288,7 @@ export function AgentDefinitionDialog({ provider: providerForSubmit, namePool: namePoolInput, envVars, + mcpServers, behavior: behaviorForSubmit( behaviorDraft, behaviorSeedRef.current, @@ -867,11 +874,14 @@ export function AgentDefinitionDialog({ envVars={envVars} fileSatisfiedEnvKeys={localModeGate.fileSatisfiedEnvKeys} inheritedEnvVars={globalConfig.env_vars} + inheritedMcpServers={globalConfig.mcp_servers} + mcpServers={mcpServers} model={model} modelTuningRuntimeId={runtime} namePoolText={namePoolText} onBehaviorDraftChange={setBehaviorDraft} onEnvVarsChange={setEnvVars} + onMcpServersChange={setMcpServers} onNamePoolTextChange={setNamePoolText} provider={provider} requiredEnvKeys={requiredEnvKeys} diff --git a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx index 52f839431..fe792876b 100644 --- a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx +++ b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx @@ -63,6 +63,11 @@ import { } from "./runtimeModelProviderSelection"; import { AgentCreationPreview } from "./AgentCreationPreview"; import type { EnvVarsValue } from "./EnvVarsEditor"; +import { + mergeMcpServersByName, + serversEqual, + type McpServersValue, +} from "./McpServersEditor"; import { useRequiredCredentialState } from "./useRequiredCredentialState"; import { CreateAgentRespondToField } from "./RespondToField"; import { PersonaDropdownField } from "./PersonaDropdownField"; @@ -125,6 +130,9 @@ export function AgentInstanceEditDialog({ const [isCustomProviderEditing, setIsCustomProviderEditing] = React.useState(false); const [envVars, setEnvVars] = React.useState(agent.envVars); + const [mcpServers, setMcpServers] = React.useState( + agent.mcpServers, + ); const [autoRestartOnConfigChange, setAutoRestartOnConfigChange] = React.useState(agent.autoRestartOnConfigChange); const personasQuery = usePersonasQuery(); @@ -175,6 +183,7 @@ export function AgentInstanceEditDialog({ setProvider(agent.provider ?? ""); setIsCustomProviderEditing(false); setEnvVars(agent.envVars); + setMcpServers(agent.mcpServers); setAutoRestartOnConfigChange(agent.autoRestartOnConfigChange); setRespondTo(agent.respondTo); setRespondToAllowlist(agent.respondToAllowlist); @@ -424,6 +433,17 @@ export function AgentInstanceEditDialog({ return { ...globalConfig.env_vars, ...inheritedEnvVars }; }, [globalConfig.env_vars, inheritedEnvVars]); + // Merge global + persona MCP servers for the inherited display hint in + // McpServersEditor. Persona-layer servers override same-named global ones. + const inheritedMcpServers = React.useMemo( + () => + mergeMcpServersByName( + globalConfig.mcp_servers, + linkedPersona?.mcpServers ?? [], + ), + [globalConfig.mcp_servers, linkedPersona?.mcpServers], + ); + // Auto-expand Advanced whenever the prospective runtime is buzz-agent so the // model-tuning knobs are reachable even when no required key is missing. // Fires once per dialog-open cycle; does not re-open if the user manually @@ -684,6 +704,9 @@ export function AgentInstanceEditDialog({ envVars: envVarsChanged(submitEnvVars, agent.envVars) ? submitEnvVars : undefined, + mcpServers: serversEqual(mcpServers, agent.mcpServers) + ? undefined + : mcpServers, respondTo: respondTo !== agent.respondTo ? respondTo : undefined, // The allowlist is preserved across mode toggles in local UI state // (so a user can flip away from allowlist and back without losing @@ -1081,8 +1104,10 @@ export function AgentInstanceEditDialog({ : undefined } inheritedEnvVars={inheritedWithGlobal} + inheritedMcpServers={inheritedMcpServers} inheritHarness={inheritHarness} linkedPersona={linkedPersona} + mcpServers={mcpServers} model={model} modelTuningRuntimeId={prospectiveRuntimeId} parallelism={parallelism} @@ -1097,6 +1122,7 @@ export function AgentInstanceEditDialog({ onAutoRestartChange={setAutoRestartOnConfigChange} onEnvVarsChange={setEnvVars} onInheritHarnessChange={setInheritHarness} + onMcpServersChange={setMcpServers} onParallelismChange={setParallelism} onRelayUrlChange={setRelayUrl} onSystemPromptChange={setSystemPrompt} diff --git a/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx b/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx index 614c44434..2d75aff10 100644 --- a/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx +++ b/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx @@ -2,6 +2,7 @@ import { cn } from "@/shared/lib/cn"; import { Input } from "@/shared/ui/input"; import { Textarea } from "@/shared/ui/textarea"; import { EnvVarsEditor, type EnvVarsValue } from "./EnvVarsEditor"; +import { McpServersEditor, type McpServersValue } from "./McpServersEditor"; import { PERSONA_FIELD_CONTROL_CLASS, PERSONA_FIELD_SHELL_CLASS, @@ -21,8 +22,10 @@ export function EditAgentAdvancedFields({ fileSatisfiedEnvKeys, focusKey, inheritedEnvVars, + inheritedMcpServers = [], inheritHarness, linkedPersona, + mcpServers, model, modelTuningRuntimeId, parallelism, @@ -36,6 +39,7 @@ export function EditAgentAdvancedFields({ onAgentCommandChange, onEnvVarsChange, onInheritHarnessChange, + onMcpServersChange, onParallelismChange, onRelayUrlChange, onAutoRestartChange, @@ -51,7 +55,11 @@ export function EditAgentAdvancedFields({ /** When set, EnvVarsEditor scrolls and focuses this key's input on mount. */ focusKey?: string; inheritedEnvVars: Record; + /** Read-only MCP servers inherited from the global + persona layers, merged. */ + inheritedMcpServers?: McpServersValue; inheritHarness: boolean; + /** This agent instance's own MCP server layer. */ + mcpServers: McpServersValue; linkedPersona: AgentPersona | null; /** Active LLM model — forwarded to BuzzAgentModelTuningFields for effort filtering. */ model?: string; @@ -73,6 +81,7 @@ export function EditAgentAdvancedFields({ onAgentCommandChange: (value: string) => void; onEnvVarsChange: (value: EnvVarsValue) => void; onInheritHarnessChange: (value: boolean) => void; + onMcpServersChange: (value: McpServersValue) => void; onParallelismChange: (value: string) => void; onRelayUrlChange: (value: string) => void; onAutoRestartChange: (value: boolean) => void; @@ -312,6 +321,17 @@ export function EditAgentAdvancedFields({ value={envVars} /> + {/* MCP servers */} + + {/* Tier-1 buzz-agent model-tuning knobs — only shown for buzz-agent. */} {isBuzzAgentRuntime(modelTuningRuntimeId) ? ( ) { const effort = config.env_vars[BUZZ_AGENT_THINKING_EFFORT]; const merged = @@ -284,6 +292,16 @@ export function GlobalAgentConfigFields({ )} />
+ + {/* MCP servers */} +
+ +
); } diff --git a/desktop/src/features/agents/ui/PersonaAdvancedFields.tsx b/desktop/src/features/agents/ui/PersonaAdvancedFields.tsx index 113e5e5a7..47a235208 100644 --- a/desktop/src/features/agents/ui/PersonaAdvancedFields.tsx +++ b/desktop/src/features/agents/ui/PersonaAdvancedFields.tsx @@ -1,6 +1,7 @@ import { Input } from "@/shared/ui/input"; import { cn } from "@/shared/lib/cn"; import { EnvVarsEditor, type EnvVarsValue } from "./EnvVarsEditor"; +import { McpServersEditor, type McpServersValue } from "./McpServersEditor"; import { CreateAgentRespondToField } from "./RespondToField"; import type { PersonaBehaviorDraft } from "./personaBehaviorDraft"; import { isBuzzAgentRuntime } from "./buzzAgentConfig"; @@ -16,11 +17,14 @@ export function PersonaAdvancedFields({ disabled, envVars, inheritedEnvVars = {}, + mcpServers, + inheritedMcpServers = [], model, modelTuningRuntimeId = "", namePoolText, onBehaviorDraftChange, onEnvVarsChange, + onMcpServersChange, onNamePoolTextChange, provider, requiredEnvKeys = [], @@ -32,6 +36,10 @@ export function PersonaAdvancedFields({ /** Env vars to display as inherited defaults in tuning-field placeholders. * For templates, pass `globalConfig.env_vars` (the fallback layer). */ inheritedEnvVars?: EnvVarsValue; + /** This definition's local-only MCP server layer. */ + mcpServers: McpServersValue; + /** Read-only servers inherited from the global layer, already merged. */ + inheritedMcpServers?: McpServersValue; /** Active LLM model — forwarded to BuzzAgentModelTuningFields for effort filtering. */ model?: string; /** Runtime id for the buzz-agent tuning knobs visibility gate. */ @@ -39,6 +47,7 @@ export function PersonaAdvancedFields({ namePoolText: string; onBehaviorDraftChange: (value: PersonaBehaviorDraft) => void; onEnvVarsChange: (value: EnvVarsValue) => void; + onMcpServersChange: (value: McpServersValue) => void; onNamePoolTextChange: (value: string) => void; /** Active LLM provider id — forwarded to BuzzAgentModelTuningFields for effort filtering. */ provider?: string; @@ -144,6 +153,16 @@ export function PersonaAdvancedFields({ value={envVars} /> + + {/* Tier-1 buzz-agent model-tuning knobs — only shown for buzz-agent. */} {isBuzzAgentRuntime(modelTuningRuntimeId) ? ( Date: Mon, 13 Jul 2026 22:02:52 -0400 Subject: [PATCH 10/17] test(desktop): add MCP servers E2E and bridge seeding support (P4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds invoke-layer round-trip tests + UI interaction tests for the editable MCP servers feature (P3), covering all three dialog layers. Round-trip tests (invokeTauri, behavior not impl): - persona create+update mcp_servers with add/change/drop - update_persona absent-preserves contract (omit → keeps, []→ clears) - agent create+update mcp_servers round-trip + absent-preserves UI tests with screenshots: - McpServersEditor in PersonaDialog (buzz-agent auto-expands Advanced) - McpServersEditor in global config card with pre-seeded servers - McpServersEditor in agent-instance edit dialog with pre-seeded servers Bridge changes: - MockManagedAgentSeed: add agentCommand (default "goose" preserved) and mcpServers seeding, wired into buildSeededManagedAgent - bridge.ts helper: add agentCommand/mcpServers to mock seed type; add mcp_servers to globalAgentConfig mock shape --- desktop/src/testing/e2eBridge.ts | 7 +- desktop/tests/e2e/mcp-servers.spec.ts | 414 ++++++++++++++++++++++++++ desktop/tests/helpers/bridge.ts | 22 +- 3 files changed, 440 insertions(+), 3 deletions(-) create mode 100644 desktop/tests/e2e/mcp-servers.spec.ts diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index 2efc231da..40155247b 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -73,6 +73,10 @@ type MockManagedAgentSeed = { autoRestartOnConfigChange?: boolean; respondTo?: RawManagedAgent["respond_to"]; respondToAllowlist?: string[]; + /** Override the default "goose" agent command (e.g. "buzz-agent"). */ + agentCommand?: string; + /** Pre-seeded MCP server layer for the agent record. */ + mcpServers?: McpServerConfig[]; }; type MockRelayAgentSeed = { @@ -1768,7 +1772,7 @@ function buildSeededManagedAgent(seed: MockManagedAgentSeed): MockManagedAgent { persona_id: seed.personaId ?? null, relay_url: DEFAULT_RELAY_WS_URL, acp_command: "buzz-acp", - agent_command: "goose", + agent_command: seed.agentCommand ?? "goose", agent_args: ["acp"], mcp_command: "", turn_timeout_seconds: 320, @@ -1779,6 +1783,7 @@ function buildSeededManagedAgent(seed: MockManagedAgentSeed): MockManagedAgent { avatar_url: seed.avatarUrl ?? null, model: null, env_vars: {}, + mcp_servers: (seed.mcpServers ?? []).map(cloneMcpServer), status, pid: status === "running" ? 42000 + mockManagedAgents.length : null, created_at: now, diff --git a/desktop/tests/e2e/mcp-servers.spec.ts b/desktop/tests/e2e/mcp-servers.spec.ts new file mode 100644 index 000000000..fa4d90ea8 --- /dev/null +++ b/desktop/tests/e2e/mcp-servers.spec.ts @@ -0,0 +1,414 @@ +import { expect, test } from "@playwright/test"; + +import { waitForAnimations } from "../helpers/animations"; +import { installMockBridge, TEST_IDENTITIES } from "../helpers/bridge"; + +test.beforeEach(async ({ page }) => { + await installMockBridge(page); +}); + +// ── helpers ───────────────────────────────────────────────────────────────── + +async function gotoApp(page: import("@playwright/test").Page) { + await page.goto("/", { waitUntil: "domcontentloaded" }); + await waitForInvokeBridge(page); + await expect(page.getByTestId("open-agents-view")).toBeVisible({ + timeout: 10_000, + }); +} + +async function waitForInvokeBridge(page: import("@playwright/test").Page) { + await page.waitForFunction( + () => { + const w = window as Window & { + __BUZZ_E2E_INVOKE_MOCK_COMMAND__?: unknown; + __TAURI_INTERNALS__?: { invoke?: unknown }; + }; + return ( + typeof w.__BUZZ_E2E_INVOKE_MOCK_COMMAND__ === "function" || + typeof w.__TAURI_INTERNALS__?.invoke === "function" + ); + }, + null, + { timeout: 5_000 }, + ); +} + +async function invokeTauri( + page: import("@playwright/test").Page, + command: string, + payload?: Record, +): Promise { + await waitForInvokeBridge(page); + return page.evaluate( + async ({ command: c, payload: p }) => { + const w = window as Window & { + __BUZZ_E2E_INVOKE_MOCK_COMMAND__?: ( + c: string, + p?: Record, + ) => Promise; + __TAURI_INTERNALS__?: { + invoke?: (c: string, p?: Record) => Promise; + }; + }; + const invoke = + w.__BUZZ_E2E_INVOKE_MOCK_COMMAND__ ?? w.__TAURI_INTERNALS__?.invoke; + if (!invoke) throw new Error("Mock invoke bridge is unavailable."); + return (await invoke(c, p)) as T; + }, + { command, payload }, + ); +} + +const AGENT_PUBKEY = TEST_IDENTITIES.tyler.pubkey; +const AGENT_NAME = "MCP Test Agent"; + +async function openEditDialog(page: import("@playwright/test").Page) { + await page.goto("/"); + await page.getByTestId("open-agents-view").click(); + const agentButton = page.getByRole("button", { + name: `${AGENT_NAME} agent profile`, + }); + await expect(agentButton).toBeVisible({ timeout: 10_000 }); + await agentButton.click(); + await expect(page.getByTestId("user-profile-panel")).toBeVisible({ + timeout: 10_000, + }); + await page.getByTestId("user-profile-edit-agent").click(); + await expect(page.getByTestId("edit-agent-dialog")).toBeVisible({ + timeout: 10_000, + }); + // Wait for the runtime catalog to load and form to settle (buzz-agent auto- + // expands Advanced, which makes the MCP servers editor immediately visible). + await expect(page.getByTestId("mcp-servers-editor").first()).toBeVisible({ + timeout: 10_000, + }); +} + +// ── round-trip tests ───────────────────────────────────────────────────────── + +test("persona mcp_servers round-trip through create_persona + update_persona", async ({ + page, +}) => { + await gotoApp(page); + + const fsServer = { + name: "filesystem", + command: "npx", + args: ["-y", "@modelcontextprotocol/server-filesystem", "/tmp"], + env: [], + enabled: true, + }; + const fetchServer = { + name: "fetch-mcp", + command: "uvx", + args: ["mcp-server-fetch"], + env: [], + enabled: true, + }; + + const created = await invokeTauri<{ + id: string; + mcp_servers?: unknown[]; + }>(page, "create_persona", { + input: { + displayName: "MCP Persona", + systemPrompt: "You use MCP.", + mcpServers: [fsServer, fetchServer], + }, + }); + + expect(created.mcp_servers).toHaveLength(2); + expect(created.mcp_servers?.[0]).toMatchObject({ name: "filesystem" }); + expect(created.mcp_servers?.[1]).toMatchObject({ name: "fetch-mcp" }); + + // Update: drop one, change one, add one. + const dbServer = { + name: "sqlite-db", + command: "uvx", + args: ["mcp-server-sqlite", "--db", "/tmp/test.db"], + env: [], + enabled: true, + }; + const updated = await invokeTauri<{ mcp_servers?: unknown[] }>( + page, + "update_persona", + { + input: { + id: created.id, + displayName: "MCP Persona", + systemPrompt: "You use MCP.", + mcpServers: [ + { ...fsServer, command: "bunx" }, // changed command + dbServer, // added + // fetch-mcp dropped + ], + }, + }, + ); + + expect(updated.mcp_servers).toHaveLength(2); + expect(updated.mcp_servers?.[0]).toMatchObject({ + name: "filesystem", + command: "bunx", + }); + expect(updated.mcp_servers?.[1]).toMatchObject({ name: "sqlite-db" }); +}); + +test("update_persona preserves mcp_servers when caller omits the field", async ({ + page, +}) => { + await gotoApp(page); + + const created = await invokeTauri<{ + id: string; + mcp_servers?: unknown[]; + }>(page, "create_persona", { + input: { + displayName: "MCP Keeper", + systemPrompt: "You keep MCP.", + mcpServers: [ + { + name: "filesystem", + command: "npx", + args: ["-y", "@modelcontextprotocol/server-filesystem", "/tmp"], + env: [], + enabled: true, + }, + ], + }, + }); + expect(created.mcp_servers).toHaveLength(1); + + // Update WITHOUT including mcpServers. Stored list must survive. + const preserved = await invokeTauri<{ mcp_servers?: unknown[] }>( + page, + "update_persona", + { + input: { + id: created.id, + displayName: "MCP Keeper (renamed)", + systemPrompt: "You keep MCP.", + // mcpServers intentionally omitted + }, + }, + ); + expect(preserved.mcp_servers).toHaveLength(1); + + // Explicit empty array still clears (intentional). + const cleared = await invokeTauri<{ mcp_servers?: unknown[] }>( + page, + "update_persona", + { + input: { + id: created.id, + displayName: "MCP Keeper (renamed)", + systemPrompt: "You keep MCP.", + mcpServers: [], + }, + }, + ); + expect(cleared.mcp_servers ?? []).toHaveLength(0); +}); + +test("agent mcp_servers round-trip through create_managed_agent + update_managed_agent", async ({ + page, +}) => { + await gotoApp(page); + + const created = await invokeTauri<{ + agent: { pubkey: string; mcp_servers?: unknown[] }; + }>(page, "create_managed_agent", { + input: { + name: "mcp-e2e-agent", + backend: { type: "local" }, + mcpServers: [ + { + name: "filesystem", + command: "npx", + args: ["-y", "@modelcontextprotocol/server-filesystem", "/tmp"], + env: [], + enabled: true, + }, + ], + }, + }); + expect(created.agent.mcp_servers).toHaveLength(1); + expect(created.agent.mcp_servers?.[0]).toMatchObject({ + name: "filesystem", + command: "npx", + }); + + // Update mcpServers — replaces entire list. + const updated = await invokeTauri<{ + agent: { mcp_servers?: unknown[] }; + }>(page, "update_managed_agent", { + input: { + pubkey: created.agent.pubkey, + mcpServers: [ + { + name: "fetch-mcp", + command: "uvx", + args: ["mcp-server-fetch"], + env: [], + enabled: true, + }, + ], + }, + }); + expect(updated.agent.mcp_servers).toHaveLength(1); + expect(updated.agent.mcp_servers?.[0]).toMatchObject({ + name: "fetch-mcp", + command: "uvx", + }); + + // Omitting mcpServers on a subsequent update preserves the stored list. + const preserved = await invokeTauri<{ + agent: { mcp_servers?: unknown[] }; + }>(page, "update_managed_agent", { + input: { + pubkey: created.agent.pubkey, + name: "mcp-e2e-agent-renamed", + // mcpServers intentionally omitted + }, + }); + expect(preserved.agent.mcp_servers).toHaveLength(1); + expect(preserved.agent.mcp_servers?.[0]).toMatchObject({ name: "fetch-mcp" }); +}); + +// ── UI tests ───────────────────────────────────────────────────────────────── + +test("mcp servers editor renders in PersonaDialog new-persona form", async ({ + page, +}) => { + await gotoApp(page); + + // Open the Agents view, click New > New agent. + await page.getByTestId("open-agents-view").click(); + await page.getByTestId("new-agent-card").click(); + await page.getByRole("menuitem", { name: /^New agent$/ }).click(); + + const dialog = page.getByRole("dialog"); + + // The default runtime is buzz-agent — Advanced auto-expands so the MCP + // servers editor is immediately visible without a click. + await expect(dialog.getByTestId("mcp-servers-editor")).toBeVisible({ + timeout: 10_000, + }); + // Initially empty — no rows. + await expect(dialog.getByTestId("mcp-servers-row")).toHaveCount(0); + + // Add a row and fill the name + command fields. + await dialog.getByTestId("mcp-servers-add").click(); + await expect(dialog.getByTestId("mcp-servers-row")).toHaveCount(1); + + await dialog.getByTestId("mcp-servers-name").fill("filesystem"); + await dialog.getByTestId("mcp-servers-command").fill("npx"); + + // Add a second row to verify multi-row rendering. + await dialog.getByTestId("mcp-servers-add").click(); + await dialog.getByTestId("mcp-servers-name").last().fill("fetch-mcp"); + await dialog.getByTestId("mcp-servers-command").last().fill("uvx"); + + await waitForAnimations(page); + await dialog.screenshot({ + path: "test-results/mcp-servers-persona-dialog.png", + }); + + // Remove the first row — verify per-row removal works. + await dialog.getByTestId("mcp-servers-remove").first().click(); + await expect(dialog.getByTestId("mcp-servers-row")).toHaveCount(1); +}); + +test("mcp servers editor renders in global agent config card", async ({ + page, +}) => { + await installMockBridge(page, { + globalAgentConfig: { + env_vars: {}, + mcp_servers: [ + { + name: "filesystem", + command: "npx", + args: ["-y", "@modelcontextprotocol/server-filesystem", "/tmp"], + env: [], + enabled: true, + }, + ], + provider: null, + model: null, + }, + }); + + await page.goto("/"); + await page.getByTestId("open-agents-view").click(); + await expect(page.getByTestId("settings-global-agent-config")).toBeVisible({ + timeout: 10_000, + }); + + const card = page.getByTestId("settings-global-agent-config"); + + // The McpServersEditor is inside the global config card. + await expect(card.getByTestId("mcp-servers-editor")).toBeVisible({ + timeout: 5_000, + }); + + // The existing server from the mock shows as a pre-populated row. + await expect(card.getByTestId("mcp-servers-row")).toHaveCount(1); + await expect(card.getByTestId("mcp-servers-name")).toHaveValue("filesystem"); + + await card.scrollIntoViewIfNeeded(); + await waitForAnimations(page); + await card.screenshot({ + path: "test-results/mcp-servers-global-config-card.png", + }); +}); + +test("mcp servers editor renders in agent-instance edit dialog", async ({ + page, +}) => { + await installMockBridge(page, { + managedAgents: [ + { + pubkey: AGENT_PUBKEY, + name: AGENT_NAME, + status: "stopped", + channelNames: ["agents"], + agentCommand: "buzz-agent", + mcpServers: [ + { + name: "filesystem", + command: "npx", + args: ["-y", "@modelcontextprotocol/server-filesystem", "/tmp"], + env: [], + enabled: true, + }, + ], + }, + ], + }); + + await openEditDialog(page); + + const dialog = page.getByRole("dialog"); + + // buzz-agent auto-expands Advanced — the MCP servers editor is already visible. + const editor = dialog.getByTestId("mcp-servers-editor"); + await expect(editor).toBeVisible(); + + // The agent's pre-existing MCP server renders as an editable row. + await expect(editor.getByTestId("mcp-servers-row")).toHaveCount(1); + await expect(editor.getByTestId("mcp-servers-name")).toHaveValue( + "filesystem", + ); + + // Add a second row. + await editor.getByTestId("mcp-servers-add").click(); + await editor.getByTestId("mcp-servers-name").last().fill("fetch-mcp"); + await editor.getByTestId("mcp-servers-command").last().fill("uvx"); + + await waitForAnimations(page); + await dialog.screenshot({ + path: "test-results/mcp-servers-edit-agent-dialog.png", + }); +}); diff --git a/desktop/tests/helpers/bridge.ts b/desktop/tests/helpers/bridge.ts index cbbef3c00..5c041aa9f 100644 --- a/desktop/tests/helpers/bridge.ts +++ b/desktop/tests/helpers/bridge.ts @@ -58,6 +58,16 @@ type MockManagedAgentSeed = { autoRestartOnConfigChange?: boolean; respondTo?: "owner-only" | "allowlist" | "anyone"; respondToAllowlist?: string[]; + /** Override the default "goose" agent command (e.g. "buzz-agent"). */ + agentCommand?: string; + /** Pre-seeded MCP server layer for the agent record. */ + mcpServers?: Array<{ + name: string; + command: string; + args: string[]; + env: Array<{ name: string; value: string }>; + enabled: boolean; + }>; }; type MockSearchProfileSeed = { @@ -283,11 +293,19 @@ type MockBridgeOptions = { identityLocked?: boolean; /** * Global agent config returned by `get_global_agent_config`. Defaults to - * an empty config (no provider, model, or env vars) if not specified. - * Pass a config with a provider to test Inherit-from-global behavior. + * an empty config (no provider, model, env vars, or MCP servers) if not + * specified. Pass a config with a provider to test Inherit-from-global + * behavior. */ globalAgentConfig?: { env_vars: Record; + mcp_servers?: Array<{ + name: string; + command: string; + args: string[]; + env: Array<{ name: string; value: string }>; + enabled: boolean; + }>; provider: string | null; model: string | null; }; From 4a60c5ed55b5cd5566c621920e940b0b522e643f Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Mon, 13 Jul 2026 22:52:02 -0400 Subject: [PATCH 11/17] =?UTF-8?q?fix(desktop):=20address=20review=20findin?= =?UTF-8?q?gs=20D1=E2=80=93D5=20for=20MCP=20servers=20UI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit D1 CRITICAL: default each field in get_global_agent_config bridge handler so partial seeds don't crash McpServersEditor; add defense-in-depth coercion at the McpServersEditor value boundary. D2 IMPORTANT: split buzzAgentSurface into empty/populated fixtures so test 06 ("empty MCP servers") doesn't collide with the filesystem row; add test 06b covering populated WYSIWYG read-only display. D3 IMPORTANT: count effective enabled servers (local + inherited not overridden) for the cap, gating both Add button and Enabled switch; extract effectiveEnabledCount helper with 6 unit tests. D4 IMPORTANT: add validateMcpServerEnvEntry mirroring Rust's validate_user_env_keys boundary (POSIX key format, reserved keys, NUL-free values, per-value byte cap); render inline subrow errors; 12 unit tests covering all rejection paths. D5 MINOR: replace localeCompare with ASCII byte-order comparator in mergeMcpServersByName to match Rust BTreeMap ordering; add mixed-case /symbol test. --- .../agents/ui/McpServersEditor.test.mjs | 163 +++++++++++ .../features/agents/ui/McpServersEditor.tsx | 252 +++++++++++++----- desktop/src/testing/e2eBridge.ts | 52 ++-- .../e2e/config-bridge-screenshots.spec.ts | 26 ++ 4 files changed, 400 insertions(+), 93 deletions(-) diff --git a/desktop/src/features/agents/ui/McpServersEditor.test.mjs b/desktop/src/features/agents/ui/McpServersEditor.test.mjs index 446111003..e79a73eb4 100644 --- a/desktop/src/features/agents/ui/McpServersEditor.test.mjs +++ b/desktop/src/features/agents/ui/McpServersEditor.test.mjs @@ -23,12 +23,17 @@ import assert from "node:assert/strict"; import { validateMcpServerName, validateMcpServerRow, + validateMcpServerEnvEntry, toRows, toServers, serversEqual, mergeMcpServersByName, + effectiveEnabledCount, + MAX_USER_MCP_SERVERS, + MAX_ENV_VALUE_BYTES, MCP_SERVER_NAME_MAX_LEN, MCP_RESERVED_SERVER_NAME, + RESERVED_ENV_KEYS, } from "./McpServersEditor.tsx"; // ── Invariant 1: name grammar mirrors validate_mcp_servers ───────────────── @@ -301,3 +306,161 @@ test("mergeMcpServersByName_distinct_names_from_all_layers_are_kept", () => { test("mergeMcpServersByName_empty_layers_produce_empty_result", () => { assert.deepEqual(mergeMcpServersByName([], [], []), []); }); + +test("mergeMcpServersByName_sorts_by_ascii_byte_order_not_locale", () => { + // Rust BTreeMap orders by byte value: '_' (0x5F) > 'Z' (0x5A) + // > 'A' (0x41), and uppercase < lowercase. localeCompare would put + // these in a different order (e.g. case-insensitive or locale-dependent). + const servers = [ + { name: "beta", command: "", args: [], env: [], enabled: true }, + { name: "Alpha", command: "", args: [], env: [], enabled: true }, + { name: "_underscore", command: "", args: [], env: [], enabled: true }, + { name: "alpha", command: "", args: [], env: [], enabled: true }, + ]; + const merged = mergeMcpServersByName(servers); + // ASCII: 'A'(65) < '_'(95) < 'a'(97) < 'b'(98) + assert.deepEqual( + merged.map((s) => s.name), + ["Alpha", "_underscore", "alpha", "beta"], + ); +}); + +// ── Invariant 5: effectiveEnabledCount — cap across layers ────────────── + +function makeServer(name, enabled = true) { + return { name, command: "npx", args: [], env: [], enabled }; +} + +test("effectiveEnabledCount_local_only", () => { + const local = [makeServer("a"), makeServer("b", false)]; + assert.equal(effectiveEnabledCount(local, []), 1); +}); + +test("effectiveEnabledCount_inherited_not_overridden", () => { + const local = [makeServer("a")]; + const inherited = [makeServer("b"), makeServer("c")]; + assert.equal(effectiveEnabledCount(local, inherited), 3); +}); + +test("effectiveEnabledCount_inherited_overridden_by_local_not_counted", () => { + // Local row overrides inherited — only the local row's enabled state + // counts, not the inherited one. + const local = [makeServer("a", false)]; + const inherited = [makeServer("a"), makeServer("b")]; + // "a" overridden (local disabled → 0), "b" inherited enabled → 1 + assert.equal(effectiveEnabledCount(local, inherited), 1); +}); + +test("effectiveEnabledCount_disabled_inherited_not_counted", () => { + const local = []; + const inherited = [makeServer("a", false), makeServer("b")]; + assert.equal(effectiveEnabledCount(local, inherited), 1); +}); + +test("effectiveEnabledCount_15_inherited_plus_1_local_hits_cap", () => { + const inherited = Array.from({ length: MAX_USER_MCP_SERVERS }, (_, i) => + makeServer(`inh-${i}`), + ); + const local = [makeServer("local-1")]; + assert.equal( + effectiveEnabledCount(local, inherited), + MAX_USER_MCP_SERVERS + 1, + ); +}); + +test("effectiveEnabledCount_toggling_disabled_to_16th_hits_cap", () => { + // Simulates toggling a disabled local row to enabled when 14 inherited + + // 1 other local are already enabled = 15. The 16th would exceed the cap. + const inherited = Array.from({ length: 14 }, (_, i) => + makeServer(`inh-${i}`), + ); + const local = [makeServer("local-1"), makeServer("local-2", false)]; + // Before toggle: 1 local enabled + 14 inherited = 15 (at cap) + assert.equal(effectiveEnabledCount(local, inherited), MAX_USER_MCP_SERVERS); + // After toggle: 2 local enabled + 14 inherited = 16 (exceeds cap) + const toggled = local.map((s) => + s.name === "local-2" ? { ...s, enabled: true } : s, + ); + assert.equal( + effectiveEnabledCount(toggled, inherited), + MAX_USER_MCP_SERVERS + 1, + ); +}); + +// ── Invariant 6: validateMcpServerEnvEntry — per-server env boundary ──── + +test("validateMcpServerEnvEntry_valid_entry_returns_null", () => { + assert.equal( + validateMcpServerEnvEntry({ name: "MY_TOKEN", value: "abc" }), + null, + ); +}); + +test("validateMcpServerEnvEntry_empty_name_returns_null_skipped", () => { + // Blank rows are skipped by toServers, so no error shown. + assert.equal( + validateMcpServerEnvEntry({ name: "", value: "anything" }), + null, + ); +}); + +test("validateMcpServerEnvEntry_malformed_key_rejected", () => { + const err = validateMcpServerEnvEntry({ name: "1BAD", value: "" }); + assert.match(err, /must match/); +}); + +test("validateMcpServerEnvEntry_key_with_equals_rejected", () => { + const err = validateMcpServerEnvEntry({ name: "KEY=val", value: "" }); + assert.match(err, /must match/); +}); + +test("validateMcpServerEnvEntry_key_with_spaces_rejected", () => { + const err = validateMcpServerEnvEntry({ name: "MY KEY", value: "" }); + assert.match(err, /must match/); +}); + +test("validateMcpServerEnvEntry_reserved_key_rejected", () => { + const err = validateMcpServerEnvEntry({ + name: "BUZZ_ACP_MCP_SERVERS", + value: "", + }); + assert.match(err, /reserved/); +}); + +test("validateMcpServerEnvEntry_reserved_key_case_insensitive", () => { + const err = validateMcpServerEnvEntry({ + name: "buzz_private_key", + value: "", + }); + assert.match(err, /reserved/); +}); + +test("validateMcpServerEnvEntry_nul_in_value_rejected", () => { + const err = validateMcpServerEnvEntry({ name: "TOKEN", value: "abc\0def" }); + assert.match(err, /NUL/); +}); + +test("validateMcpServerEnvEntry_oversized_value_rejected", () => { + const err = validateMcpServerEnvEntry({ + name: "TOKEN", + value: "x".repeat(MAX_ENV_VALUE_BYTES + 1), + }); + assert.match(err, /exceeds/); +}); + +test("validateMcpServerEnvEntry_at_limit_value_accepted", () => { + assert.equal( + validateMcpServerEnvEntry({ + name: "TOKEN", + value: "x".repeat(MAX_ENV_VALUE_BYTES), + }), + null, + ); +}); + +test("validateMcpServerEnvEntry_all_reserved_keys_are_rejected", () => { + for (const key of RESERVED_ENV_KEYS) { + const err = validateMcpServerEnvEntry({ name: key, value: "" }); + assert.ok(err, `Expected "${key}" to be rejected as reserved`); + } +}); diff --git a/desktop/src/features/agents/ui/McpServersEditor.tsx b/desktop/src/features/agents/ui/McpServersEditor.tsx index 70e55c640..9aafbbe6e 100644 --- a/desktop/src/features/agents/ui/McpServersEditor.tsx +++ b/desktop/src/features/agents/ui/McpServersEditor.tsx @@ -39,7 +39,39 @@ export const MCP_RESERVED_SERVER_NAME = "buzz-dev-mcp"; /** Effective/per-layer enabled-server cap. Mirrors `MAX_USER_MCP_SERVERS`. */ export const MAX_USER_MCP_SERVERS = 15; +/** Per-value byte cap. Mirrors `MAX_ENV_VALUE_BYTES` in `env_vars.rs`. */ +export const MAX_ENV_VALUE_BYTES = 32 * 1024; + +/** + * Reserved env keys (case-insensitive). Mirrors `RESERVED_ENV_KEYS` in + * `env_vars.rs`. Keys that override agent identity, code-execution surface, + * security gates, or structured transport must never be user-settable. + */ +export const RESERVED_ENV_KEYS: readonly string[] = [ + "BUZZ_PRIVATE_KEY", + "NOSTR_PRIVATE_KEY", + "BUZZ_AUTH_TAG", + "BUZZ_API_TOKEN", + "BUZZ_ACP_PRIVATE_KEY", + "BUZZ_ACP_API_TOKEN", + "BUZZ_RELAY_URL", + "BUZZ_ACP_AGENT_COMMAND", + "BUZZ_ACP_AGENT_ARGS", + "BUZZ_ACP_MCP_COMMAND", + "BUZZ_ACP_MCP_SERVERS", + "BUZZ_ACP_RESPOND_TO", + "BUZZ_ACP_RESPOND_TO_ALLOWLIST", + "BUZZ_ACP_AGENT_OWNER", + "BUZZ_ACP_SETUP_PAYLOAD", +]; + +const RESERVED_ENV_KEYS_UPPER = new Set( + RESERVED_ENV_KEYS.map((k) => k.toUpperCase()), +); + const NAME_CHARS_RE = /^[A-Za-z0-9_-]*$/; +/** POSIX env key: `[A-Za-z_][A-Za-z0-9_]*`. Mirrors `is_well_formed_env_key`. */ +const ENV_KEY_RE = /^[A-Za-z_][A-Za-z0-9_]*$/; /** * Client-side mirror of the Rust `validate_mcp_servers` name grammar, for @@ -80,6 +112,32 @@ export function validateMcpServerRow( return null; } +/** + * Validate a single per-server env var entry. Mirrors the Rust + * `validate_user_env_keys` boundary (`env_vars.rs`): POSIX key format, + * reserved-key check (case-insensitive), NUL-free values, and per-value + * byte cap. Returns `null` when valid. Exported for unit tests. + */ +export function validateMcpServerEnvEntry(entry: { + name: string; + value: string; +}): string | null { + if (entry.name.length === 0) return null; // blank rows are skipped by toServers + if (!ENV_KEY_RE.test(entry.name)) { + return "Key must match [A-Za-z_][A-Za-z0-9_]*."; + } + if (RESERVED_ENV_KEYS_UPPER.has(entry.name.toUpperCase())) { + return `"${entry.name}" is reserved by Buzz.`; + } + if (entry.value.includes("\0")) { + return "Value cannot contain NUL bytes."; + } + if (new TextEncoder().encode(entry.value).length > MAX_ENV_VALUE_BYTES) { + return `Value exceeds the ${MAX_ENV_VALUE_BYTES}-byte limit.`; + } + return null; +} + type ArgRow = { id: string; value: string }; type EnvRow = { id: string; name: string; value: string }; type ServerRow = { @@ -177,8 +235,8 @@ export function serversEqual(a: McpServersValue, b: McpServersValue): boolean { * effective-server cap: it is a DISPLAY merge for the editor's read-only * "inherited" rows, not the runtime WYSIWYG surface (that's * `RuntimeConfigSurface.buzzAgentMcpServers`, computed server-side). Sorted - * by name for the same deterministic order the backend's `BTreeMap`-backed - * merge produces. Exported for unit tests and dialog callers. + * by ASCII byte order (`<` / `>`) to match Rust's `BTreeMap` + * ordering. Exported for unit tests and dialog callers. */ export function mergeMcpServersByName( ...layers: readonly McpServersValue[] @@ -190,10 +248,28 @@ export function mergeMcpServersByName( } } return Array.from(merged.values()).sort((a, b) => - a.name.localeCompare(b.name), + a.name < b.name ? -1 : a.name > b.name ? 1 : 0, ); } +/** + * Compute the effective enabled count across local and inherited layers. + * Local enabled rows plus inherited enabled rows NOT overridden/masked by + * a local row with the same name. Mirrors the Rust effective-merge cap + * check (`mcp_servers.rs:225-229`). Exported for unit tests. + */ +export function effectiveEnabledCount( + local: McpServersValue, + inherited: McpServersValue, +): number { + const localNames = new Set(local.map((s) => s.name)); + const localEnabled = local.filter((s) => s.enabled).length; + const inheritedEnabled = inherited.filter( + (s) => s.enabled && !localNames.has(s.name), + ).length; + return localEnabled + inheritedEnabled; +} + /** True when a row has any content beyond a bare name — used to decide * whether an empty-name row's "Name is required" hint should show (a * pristine just-added row stays quiet; a row with typed content but no name @@ -245,18 +321,24 @@ export function McpServersEditor({ helperText, disabled = false, }: McpServersEditorProps) { - const [rows, setRows] = React.useState(() => toRows(value)); - const lastEmitted = React.useRef(value); + // Defense-in-depth: coerce at the exported boundary so a caller passing + // `undefined` (e.g. a partial test bridge seed) degrades to empty rather + // than crashing inside `toRows`. Production never produces `undefined` + // (Rust `#[serde(default)]` + `EMPTY_CONFIG`), but a single coercion + // here is cheaper than scattering `?? []` across every mount site. + const safeValue = value ?? []; + const [rows, setRows] = React.useState(() => toRows(safeValue)); + const lastEmitted = React.useRef(safeValue); // Resync from `value` when the parent supplies a value we did not just // emit (e.g. dialog reopened against a different persona/agent). Mirrors // EnvVarsEditor's `recordsEqual` resync guard. React.useEffect(() => { - if (!serversEqual(lastEmitted.current, value)) { - lastEmitted.current = value; - setRows(toRows(value)); + if (!serversEqual(lastEmitted.current, safeValue)) { + lastEmitted.current = safeValue; + setRows(toRows(safeValue)); } - }, [value]); + }, [safeValue]); function emit(nextRows: ServerRow[]) { setRows(nextRows); @@ -337,11 +419,16 @@ export function McpServersEditor({ updateRow(rowId, { env: row.env.filter((e) => e.id !== envId) }); } - const enabledCount = rows.filter((r) => r.enabled).length; - const atCap = enabledCount >= MAX_USER_MCP_SERVERS; + const localNames = new Set(rows.map((r) => r.name)); const visibleInherited = inheritedServers.filter( - (server) => !rows.some((row) => row.name === server.name), + (server) => !localNames.has(server.name), ); + const localEnabledCount = rows.filter((r) => r.enabled).length; + const inheritedEnabledCount = visibleInherited.filter( + (s) => s.enabled, + ).length; + const atCap = + localEnabledCount + inheritedEnabledCount >= MAX_USER_MCP_SERVERS; return (
@@ -433,7 +520,7 @@ export function McpServersEditor({ aria-label={row.enabled ? "Disable server" : "Enable server"} checked={row.enabled} data-testid="mcp-servers-enabled" - disabled={disabled} + disabled={disabled || (!row.enabled && atCap)} onCheckedChange={(checked) => updateRow(row.id, { enabled: checked }) } @@ -530,67 +617,84 @@ export function McpServersEditor({

Environment

- {row.env.map((entry) => ( -
-
- - updateEnvRow(row.id, entry.id, { - name: event.target.value, - }) - } - placeholder="VARIABLE_NAME" - value={entry.name} - /> + {row.env.map((entry) => { + const envError = validateMcpServerEnvEntry(entry); + return ( +
+
+
+ + updateEnvRow(row.id, entry.id, { + name: event.target.value, + }) + } + placeholder="VARIABLE_NAME" + value={entry.name} + /> +
+
+ + updateEnvRow(row.id, entry.id, { + value: event.target.value, + }) + } + placeholder="value" + value={entry.value} + /> +
+ +
+ {envError ? ( +

+ + {envError} +

+ ) : null}
-
- - updateEnvRow(row.id, entry.id, { - value: event.target.value, - }) - } - placeholder="value" - value={entry.value} - /> -
- -
- ))} + ); + })}
diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index 40155247b..f544597cf 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -1713,14 +1713,29 @@ function buildMockConfigSurface(pubkey: string): { }, }; - const buzzAgentSurface = { + const buzzAgentBase = { ...gooseSurface, runtimeId: "buzz-agent", runtimeLabel: "Buzz Agent", advanced: [], extensions: [], - // Effective merged (global < definition < agent, enabled-only) servers — - // "what runs." Exercises the WYSIWYG read-only display by default. + sources: { + ...gooseSurface.sources, + configFilePath: null, + mcpConfigFilePath: null, + }, + }; + + // Empty WYSIWYG surface — "No custom servers configured" path. + const buzzAgentEmptySurface = { + ...buzzAgentBase, + buzzAgentMcpServers: [] satisfies McpServerConfig[], + }; + + // Populated WYSIWYG surface — exercises the read-only BuzzAgentMcpServerRow + // rendering with an effective-merged server list. + const buzzAgentPopulatedSurface = { + ...buzzAgentBase, buzzAgentMcpServers: [ { name: "filesystem", @@ -1730,11 +1745,6 @@ function buildMockConfigSurface(pubkey: string): { enabled: true, }, ] satisfies McpServerConfig[], - sources: { - ...gooseSurface.sources, - configFilePath: null, - mcpConfigFilePath: null, - }, }; // Map well-known test pubkeys to specific fixtures. @@ -1743,6 +1753,8 @@ function buildMockConfigSurface(pubkey: string): { "abc1230000000000000000000000000000000000000000000000000000000def"; const PUBKEY_BUZZ_AGENT = "b0220000000000000000000000000000000000000000000000000000000000a9"; + const PUBKEY_BUZZ_AGENT_POPULATED = + "b0220000000000000000000000000000000000000000000000000000000000b1"; switch (pubkey) { case ALICE_PUBKEY: @@ -1756,7 +1768,9 @@ function buildMockConfigSurface(pubkey: string): { case PUBKEY_MULTI_ORIGIN: return multiOriginSurface; case PUBKEY_BUZZ_AGENT: - return buzzAgentSurface; + return buzzAgentEmptySurface; + case PUBKEY_BUZZ_AGENT_POPULATED: + return buzzAgentPopulatedSurface; default: return gooseSurface; } @@ -9307,16 +9321,16 @@ export function maybeInstallE2eTauriMocks() { return null; } case "get_global_agent_config": { - // Return the mock global agent config if provided; otherwise return - // an empty config (no global provider, model, env vars, or MCP servers). - return ( - config?.mock?.globalAgentConfig ?? { - env_vars: {}, - mcp_servers: [], - provider: null, - model: null, - } - ); + // Return the mock global agent config, defaulting each field + // individually so a partial seed (e.g. {env_vars, provider, model} + // without mcp_servers) still produces a valid shape. + return { + env_vars: {}, + mcp_servers: [], + provider: null, + model: null, + ...config?.mock?.globalAgentConfig, + }; } case "set_global_agent_config": { // Echo back the submitted config as the saved value (mirrors the diff --git a/desktop/tests/e2e/config-bridge-screenshots.spec.ts b/desktop/tests/e2e/config-bridge-screenshots.spec.ts index 11d4a73fa..94477e229 100644 --- a/desktop/tests/e2e/config-bridge-screenshots.spec.ts +++ b/desktop/tests/e2e/config-bridge-screenshots.spec.ts @@ -14,6 +14,8 @@ const MULTI_ORIGIN_PUBKEY = "abc1230000000000000000000000000000000000000000000000000000000def"; const BUZZ_AGENT_PUBKEY = "b0220000000000000000000000000000000000000000000000000000000000a9"; +const BUZZ_AGENT_POPULATED_PUBKEY = + "b0220000000000000000000000000000000000000000000000000000000000b1"; const MANAGED_AGENTS = [ { @@ -281,6 +283,30 @@ test.describe("config bridge screenshots", () => { ); }); + test("06b — buzz-agent populated MCP servers", async ({ page }) => { + await installMockBridge(page, { + managedAgents: [ + { + pubkey: BUZZ_AGENT_POPULATED_PUBKEY, + name: "Buzz Populated", + status: "running" as const, + channelNames: ["agents"], + }, + ], + }); + + const panel = await openAgentProfileFromChannel(page, "Buzz Populated"); + + // The populated WYSIWYG surface includes a "filesystem" server row. + await expect(panel.getByText("filesystem", { exact: true })).toBeVisible(); + await expect( + panel.getByText("npx -y @modelcontextprotocol/server-filesystem /tmp"), + ).toBeVisible(); + await expect(panel.getByText("MCP Servers", { exact: true })).toHaveCount( + 1, + ); + }); + test("07 — profile side panel — Configuration section", async ({ page }) => { // charlie (554cef…) is the well-known test pubkey that the mock bridge // seeds as a bot owned by the test viewer, so isBot + isOwner + managedAgent From e27200baa243aa1e31786f93dc873183d9d7fa2a Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Mon, 13 Jul 2026 23:48:18 -0400 Subject: [PATCH 12/17] fix(desktop): add effective-cap save-backstop and complete client validation mirror MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1: Effective-merge MCP cap check at agent create/update save time. validate_effective_mcp_cap calls effective_buzz_agent_mcp_servers (the same resolver used at spawn) and rejects when the three-layer merge (global < definition < agent) exceeds MAX_USER_MCP_SERVERS. Prevents a per-layer-valid record from silently breaking at spawn or emptying the WYSIWYG surface. Editor now shows a destructive error whenever effective count exceeds the cap (fires on rename too). 4 Rust unit tests: 15-global+1-local → Err, at-cap → Ok, rename-unmask → Err, non-buzz-agent skip. F2: Complete the client-side Rust mirror — validateMcpServerRow now checks command NUL + ≤32KB; new validateMcpServerArg checks arg NUL + ≤32KB with inline subrow errors; new validateMcpServerListPayload checks aggregate total payload ≤256KB (name+command+args+env bytes across all servers) with editor-level error. Adds MAX_ENV_TOTAL_BYTES constant. 9 unit tests: command NUL/oversize/at-limit, arg NUL/oversize /valid, payload under/over/at-limit. check-file-sizes.mjs: agent_models.rs override bumped 1079→1082 (+3 lines for the effective-cap block). --- desktop/scripts/check-file-sizes.mjs | 4 +- .../src-tauri/src/commands/agent_models.rs | 16 ++ desktop/src-tauri/src/commands/agents.rs | 13 ++ .../src/managed_agents/types/mcp_servers.rs | 17 ++ .../src/managed_agents/types/tests.rs | 79 +++++++- .../agents/ui/McpServersEditor.test.mjs | 100 ++++++++++ .../features/agents/ui/McpServersEditor.tsx | 173 ++++++++++++++---- 7 files changed, 359 insertions(+), 43 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index b4f8a9933..f3b7d9d3d 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -422,7 +422,9 @@ const overrides = new Map([ // (if let Some(provider_update) = input.provider { record.provider = provider_update; }). // +8: harness_override thread-through in update_managed_agent so a deliberate // Custom pin routes to update_time_agent_command_override (comment + call). - ["src-tauri/src/commands/agent_models.rs", 1079], + // +3: effective-merge MCP cap backstop in update_managed_agent — validates + // the three-layer merge stays within MAX_USER_MCP_SERVERS at save time. + ["src-tauri/src/commands/agent_models.rs", 1082], // global-agent-config: get_agent_config_surface / write_agent_config_field / // put_agent_session_config commands + GlobalAgentConfig serde types. New file // in this PR; queued to split with the command module refactor. diff --git a/desktop/src-tauri/src/commands/agent_models.rs b/desktop/src-tauri/src/commands/agent_models.rs index 107392dc3..a4bab7735 100644 --- a/desktop/src-tauri/src/commands/agent_models.rs +++ b/desktop/src-tauri/src/commands/agent_models.rs @@ -900,6 +900,22 @@ pub async fn update_managed_agent( record.respond_to_allowlist = prospective_allowlist; } + // Effective-merge cap: the record now carries the updated fields — + // validate that the three-layer merge stays within the cap. A rename + // can un-mask an inherited server, pushing the effective count over. + { + let personas = load_personas(&app).unwrap_or_default(); + let effective_command = crate::managed_agents::record_agent_command(record, &personas); + let global_config = + crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(); + crate::managed_agents::validate_effective_mcp_cap( + record, + &personas, + &global_config.mcp_servers, + &effective_command, + )?; + } + record.updated_at = now_iso(); save_managed_agents(&app, &records)?; diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index a9e15d35e..f626058d3 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -741,6 +741,19 @@ pub async fn create_managed_agent( relay_mesh: relay_mesh.clone(), }; + // Effective-merge cap: validate the prospective three-layer merge + // (global < definition < agent) stays within MAX_USER_MCP_SERVERS. + // Per-layer validation already ran, but a per-layer-valid agent can + // still exceed the cap when layered on top of inherited servers. + let global_config = + crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(); + crate::managed_agents::validate_effective_mcp_cap( + &record, + &personas, + &global_config.mcp_servers, + &record.agent_command, + )?; + records.push(record); save_managed_agents(&app, &records)?; diff --git a/desktop/src-tauri/src/managed_agents/types/mcp_servers.rs b/desktop/src-tauri/src/managed_agents/types/mcp_servers.rs index d8694febf..633abbe45 100644 --- a/desktop/src-tauri/src/managed_agents/types/mcp_servers.rs +++ b/desktop/src-tauri/src/managed_agents/types/mcp_servers.rs @@ -252,3 +252,20 @@ pub(crate) fn effective_buzz_agent_mcp_servers( .unwrap_or_default(); merge_mcp_servers(global, definition, &record.mcp_servers) } + +/// Validate that the prospective effective-merge count stays within the +/// [`MAX_USER_MCP_SERVERS`] cap. Called at agent create/update to prevent a +/// record that passes per-layer validation but exceeds the cap after the +/// global < definition < agent merge. Non–buzz-agent runtimes skip the check +/// (they don't use `BUZZ_ACP_MCP_SERVERS`). +pub(crate) fn validate_effective_mcp_cap( + record: &super::ManagedAgentRecord, + personas: &[super::AgentDefinition], + global: &[McpServerConfig], + effective_command: &str, +) -> Result<(), String> { + // effective_buzz_agent_mcp_servers returns Ok(empty) for non-buzz-agent + // runtimes, so the cap check only fires for buzz-agent. + effective_buzz_agent_mcp_servers(record, personas, global, effective_command)?; + Ok(()) +} diff --git a/desktop/src-tauri/src/managed_agents/types/tests.rs b/desktop/src-tauri/src/managed_agents/types/tests.rs index ee376d019..573a0cb98 100644 --- a/desktop/src-tauri/src/managed_agents/types/tests.rs +++ b/desktop/src-tauri/src/managed_agents/types/tests.rs @@ -1,6 +1,6 @@ use super::{ - merge_mcp_servers, replace_mcp_servers, validate_mcp_servers, AgentDefinition, - ManagedAgentRecord, McpServerConfig, McpServerEnvVar, MAX_USER_MCP_SERVERS, + merge_mcp_servers, replace_mcp_servers, validate_effective_mcp_cap, validate_mcp_servers, + AgentDefinition, ManagedAgentRecord, McpServerConfig, McpServerEnvVar, MAX_USER_MCP_SERVERS, }; use std::path::PathBuf; @@ -792,3 +792,78 @@ fn validate_mcp_servers_allows_fifteen_enabled_in_one_layer() { .collect(); assert!(validate_mcp_servers(&servers).is_ok()); } + +// ── validate_effective_mcp_cap — save-time effective merge cap ────────── + +/// Build a minimal buzz-agent ManagedAgentRecord with the given mcp_servers +/// layer. `agent_command = "buzz-agent"` so the effective check fires. +fn buzz_agent_record(mcp_servers: Vec) -> ManagedAgentRecord { + serde_json::from_value(serde_json::json!({ + "pubkey": "aabbccdd", + "name": "test", + "relay_url": "", + "acp_command": "buzz-acp", + "agent_command": "buzz-agent", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "mcp_servers": mcp_servers, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + })) + .expect("minimal record should deserialize") +} + +#[test] +fn effective_cap_rejects_15_global_plus_1_local() { + let global: Vec<_> = (0..MAX_USER_MCP_SERVERS) + .map(|i| mcp_server(&format!("global-{i}"), "cmd", true)) + .collect(); + let record = buzz_agent_record(vec![mcp_server("local-0", "cmd", true)]); + let err = validate_effective_mcp_cap(&record, &[], &global, "buzz-agent") + .expect_err("effective cap must reject 16 enabled"); + assert!(err.contains("effective MCP server count"), "error: {err}"); +} + +#[test] +fn effective_cap_allows_at_cap() { + let global: Vec<_> = (0..MAX_USER_MCP_SERVERS - 1) + .map(|i| mcp_server(&format!("global-{i}"), "cmd", true)) + .collect(); + let record = buzz_agent_record(vec![mcp_server("local-0", "cmd", true)]); + validate_effective_mcp_cap(&record, &[], &global, "buzz-agent") + .expect("15 effective should pass"); +} + +#[test] +fn effective_cap_rejects_rename_unmask() { + // Global has 15 enabled. The agent has a same-name override that masks + // one of them. If the override is "renamed" (simulated by changing the + // name to something unique), the masked global server is un-masked and + // the effective count goes to 16. + let global: Vec<_> = (0..MAX_USER_MCP_SERVERS) + .map(|i| mcp_server(&format!("global-{i}"), "cmd", true)) + .collect(); + // Agent overrides global-0 (masks it) — effective = 15. + let record_masked = buzz_agent_record(vec![mcp_server("global-0", "", false)]); + validate_effective_mcp_cap(&record_masked, &[], &global, "buzz-agent") + .expect("masked state should be at-cap and valid"); + + // Agent renamed its override → unique name, un-masks global-0 → effective = 16. + let record_unmasked = buzz_agent_record(vec![mcp_server("unique-name", "cmd", true)]); + let err = validate_effective_mcp_cap(&record_unmasked, &[], &global, "buzz-agent") + .expect_err("rename-unmask must reject"); + assert!(err.contains("effective MCP server count"), "error: {err}"); +} + +#[test] +fn effective_cap_skips_non_buzz_agent_runtime() { + let global: Vec<_> = (0..=MAX_USER_MCP_SERVERS) + .map(|i| mcp_server(&format!("global-{i}"), "cmd", true)) + .collect(); + let record = buzz_agent_record(vec![mcp_server("local-0", "cmd", true)]); + // Non-buzz-agent runtime → effective check returns Ok (skip). + validate_effective_mcp_cap(&record, &[], &global, "goose") + .expect("non-buzz-agent runtime should skip the cap"); +} diff --git a/desktop/src/features/agents/ui/McpServersEditor.test.mjs b/desktop/src/features/agents/ui/McpServersEditor.test.mjs index e79a73eb4..3cdcfc440 100644 --- a/desktop/src/features/agents/ui/McpServersEditor.test.mjs +++ b/desktop/src/features/agents/ui/McpServersEditor.test.mjs @@ -23,7 +23,9 @@ import assert from "node:assert/strict"; import { validateMcpServerName, validateMcpServerRow, + validateMcpServerArg, validateMcpServerEnvEntry, + validateMcpServerListPayload, toRows, toServers, serversEqual, @@ -31,6 +33,7 @@ import { effectiveEnabledCount, MAX_USER_MCP_SERVERS, MAX_ENV_VALUE_BYTES, + MAX_ENV_TOTAL_BYTES, MCP_SERVER_NAME_MAX_LEN, MCP_RESERVED_SERVER_NAME, RESERVED_ENV_KEYS, @@ -464,3 +467,100 @@ test("validateMcpServerEnvEntry_all_reserved_keys_are_rejected", () => { assert.ok(err, `Expected "${key}" to be rejected as reserved`); } }); + +// ── Invariant 7: validateMcpServerRow — command NUL + size ────────────── + +test("validateMcpServerRow_command_nul_rejected", () => { + const err = validateMcpServerRow( + { name: "srv", command: "npx\0bad", enabled: true }, + new Set(), + ); + assert.match(err, /NUL/); +}); + +test("validateMcpServerRow_command_oversize_rejected", () => { + const err = validateMcpServerRow( + { + name: "srv", + command: "x".repeat(MAX_ENV_VALUE_BYTES + 1), + enabled: true, + }, + new Set(), + ); + assert.match(err, /exceeds/); +}); + +test("validateMcpServerRow_command_at_limit_accepted", () => { + assert.equal( + validateMcpServerRow( + { name: "srv", command: "x".repeat(MAX_ENV_VALUE_BYTES), enabled: true }, + new Set(), + ), + null, + ); +}); + +// ── Invariant 8: validateMcpServerArg — arg NUL + size ────────────────── + +test("validateMcpServerArg_nul_rejected", () => { + assert.match(validateMcpServerArg("arg\0val"), /NUL/); +}); + +test("validateMcpServerArg_oversize_rejected", () => { + assert.match( + validateMcpServerArg("x".repeat(MAX_ENV_VALUE_BYTES + 1)), + /exceeds/, + ); +}); + +test("validateMcpServerArg_valid_accepted", () => { + assert.equal(validateMcpServerArg("--flag"), null); +}); + +// ── Invariant 9: validateMcpServerListPayload — aggregate cap ─────────── + +test("validateMcpServerListPayload_under_limit_accepted", () => { + assert.equal( + validateMcpServerListPayload([ + { name: "srv", command: "npx", args: ["-y"], env: [], enabled: true }, + ]), + null, + ); +}); + +test("validateMcpServerListPayload_over_limit_rejected", () => { + // Nine servers, each with an env value just under 32KB → total > 256KB. + const servers = Array.from({ length: 9 }, (_, i) => ({ + name: `srv-${i}`, + command: "npx", + args: [], + env: [{ name: "DATA", value: "x".repeat(MAX_ENV_VALUE_BYTES - 1) }], + enabled: true, + })); + const err = validateMcpServerListPayload(servers); + assert.ok(err, "aggregate payload should be rejected"); + assert.match(err, /limit/); +}); + +test("validateMcpServerListPayload_at_limit_accepted", () => { + // A single server whose total payload is exactly at the limit. + // Total = name + command + env-key + env-value bytes. + const budget = MAX_ENV_TOTAL_BYTES; + const name = "s"; + const command = "c"; + const envKey = "K"; + const overhead = name.length + command.length + envKey.length; + const value = "x".repeat(budget - overhead); + assert.equal( + validateMcpServerListPayload([ + { + name, + command, + args: [], + env: [{ name: envKey, value }], + enabled: true, + }, + ]), + null, + ); +}); diff --git a/desktop/src/features/agents/ui/McpServersEditor.tsx b/desktop/src/features/agents/ui/McpServersEditor.tsx index 9aafbbe6e..2e6b7acc8 100644 --- a/desktop/src/features/agents/ui/McpServersEditor.tsx +++ b/desktop/src/features/agents/ui/McpServersEditor.tsx @@ -42,6 +42,9 @@ export const MAX_USER_MCP_SERVERS = 15; /** Per-value byte cap. Mirrors `MAX_ENV_VALUE_BYTES` in `env_vars.rs`. */ export const MAX_ENV_VALUE_BYTES = 32 * 1024; +/** Aggregate payload cap across all servers. Mirrors `MAX_ENV_TOTAL_BYTES`. */ +export const MAX_ENV_TOTAL_BYTES = 256 * 1024; + /** * Reserved env keys (case-insensitive). Mirrors `RESERVED_ENV_KEYS` in * `env_vars.rs`. Keys that override agent identity, code-execution surface, @@ -95,9 +98,9 @@ export function validateMcpServerName(name: string): string | null { /** * Full per-row validation: name grammar + uniqueness within the layer + - * command-required-when-enabled. `otherNames` is every OTHER row's name in - * the same layer (excluding this row), so a collision flags both rows. - * Exported for unit tests. + * command-required-when-enabled + command NUL/size. `otherNames` is every + * OTHER row's name in the same layer (excluding this row), so a collision + * flags both rows. Exported for unit tests. */ export function validateMcpServerRow( row: { name: string; command: string; enabled: boolean }, @@ -109,6 +112,58 @@ export function validateMcpServerRow( if (row.enabled && row.command.trim().length === 0) { return "Command is required for an enabled server."; } + if (row.command.includes("\0")) { + return "Command cannot contain NUL bytes."; + } + if ( + row.command.length > 0 && + new TextEncoder().encode(row.command).length > MAX_ENV_VALUE_BYTES + ) { + return `Command exceeds the ${MAX_ENV_VALUE_BYTES}-byte limit.`; + } + return null; +} + +/** + * Validate a single arg value. Mirrors `validate_mcp_servers`'s per-arg + * NUL + size check (`mcp_servers.rs:141-157`). Returns `null` when valid. + * Exported for unit tests. + */ +export function validateMcpServerArg(value: string): string | null { + if (value.includes("\0")) return "Argument cannot contain NUL bytes."; + if (new TextEncoder().encode(value).length > MAX_ENV_VALUE_BYTES) { + return `Argument exceeds the ${MAX_ENV_VALUE_BYTES}-byte limit.`; + } + return null; +} + +/** + * Validate aggregate payload across all servers. Mirrors the Rust + * `validate_mcp_servers` total-payload cap (`mcp_servers.rs:186-190`): + * sum of name+command+args+env-key+env-value bytes must stay under + * `MAX_ENV_TOTAL_BYTES`. Returns `null` when valid. Exported for unit tests. + */ +export function validateMcpServerListPayload( + servers: McpServersValue, +): string | null { + const encoder = new TextEncoder(); + let total = 0; + for (const server of servers) { + total += encoder.encode(server.name).length; + if (server.command.length > 0) { + total += encoder.encode(server.command).length; + } + for (const arg of server.args) { + total += encoder.encode(arg).length; + } + for (const entry of server.env) { + total += encoder.encode(entry.name).length; + total += encoder.encode(entry.value).length; + } + } + if (total > MAX_ENV_TOTAL_BYTES) { + return `Total MCP server payload is ${total} bytes; limit is ${MAX_ENV_TOTAL_BYTES}.`; + } return null; } @@ -427,8 +482,10 @@ export function McpServersEditor({ const inheritedEnabledCount = visibleInherited.filter( (s) => s.enabled, ).length; - const atCap = - localEnabledCount + inheritedEnabledCount >= MAX_USER_MCP_SERVERS; + const currentEffective = localEnabledCount + inheritedEnabledCount; + const atCap = currentEffective >= MAX_USER_MCP_SERVERS; + const overCap = currentEffective > MAX_USER_MCP_SERVERS; + const payloadError = validateMcpServerListPayload(toServers(rows)); return (
@@ -564,41 +621,58 @@ export function McpServersEditor({

Args

- {row.args.map((arg) => ( -
-
- - updateArg(row.id, arg.id, event.target.value) - } - value={arg.value} - /> + {row.args.map((arg) => { + const argError = validateMcpServerArg(arg.value); + return ( +
+
+
+ + updateArg(row.id, arg.id, event.target.value) + } + value={arg.value} + /> +
+ +
+ {argError ? ( +

+ + {argError} +

+ ) : null}
- -
- ))} + ); + })} - {atCap ? ( + {overCap ? ( +

+ + {currentEffective} enabled servers exceed the{" "} + {MAX_USER_MCP_SERVERS}-server limit (including inherited). Save + will be rejected. +

+ ) : atCap ? (

{MAX_USER_MCP_SERVERS}-server limit reached (including inherited).

) : null} + {payloadError ? ( +

+ + {payloadError} +

+ ) : null}
From 203a18d5868f94aa09048be83910cfde5ea55fbc Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Tue, 14 Jul 2026 09:17:38 -0400 Subject: [PATCH 13/17] fix(desktop): validate effective MCP cap at global and persona save An inherited-layer mutation (global config or persona definition) could silently push an existing buzz-agent over the 15-server effective cap, breaking it at next spawn. Add pre-save cross-checks at both save paths so the save is rejected atomically with the offending agent named. --- desktop/scripts/check-file-sizes.mjs | 4 +- .../src/commands/global_agent_config.rs | 10 ++ .../src-tauri/src/commands/personas/mod.rs | 24 ++++ .../src/managed_agents/persona_events.rs | 3 + .../src/managed_agents/types/mcp_servers.rs | 26 ++++ .../src/managed_agents/types/tests.rs | 121 +++++++++++++++++- 6 files changed, 185 insertions(+), 3 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index f3b7d9d3d..0761e97f3 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -80,7 +80,9 @@ const overrides = new Map([ // retry-safety. Load-bearing reviewer-required change; queued to split. // Consolidation removed the legacy persona-card import/export codecs. // +6: local-only MCP layer validation/update wiring. Load-bearing; queued to split. - ["src-tauri/src/commands/personas/mod.rs", 990], + // MCP validation and inherited-layer effective-cap save gate; the gate counts + // trailing newlines, so 994 physical lines are 995 counted lines. + ["src-tauri/src/commands/personas/mod.rs", 995], // #1418 read-path fix: get_thread_replies' blocker fix (shared TIMELINE_KINDS // const + build_thread_replies_filter helper, mirroring the channel sibling so // the two p-gate filters can't drift) plus two guard unit tests. The file was diff --git a/desktop/src-tauri/src/commands/global_agent_config.rs b/desktop/src-tauri/src/commands/global_agent_config.rs index 37a1be522..1bc447fb8 100644 --- a/desktop/src-tauri/src/commands/global_agent_config.rs +++ b/desktop/src-tauri/src/commands/global_agent_config.rs @@ -76,6 +76,16 @@ pub async fn set_global_agent_config( let phase1 = tokio::task::spawn_blocking(move || { validate_global_config(&config)?; + // Reject global save if the prospective MCP config would push any + // existing buzz-agent record over the effective-server cap. + let records = load_managed_agents(&app_for_write)?; + let personas = load_personas(&app_for_write)?; + crate::managed_agents::validate_effective_mcp_cap_for_records( + &records, + &personas, + &config.mcp_servers, + )?; + let old_global = load_global_agent_config(&app_for_write).unwrap_or_default(); save_global_agent_config(&app_for_write, &config)?; diff --git a/desktop/src-tauri/src/commands/personas/mod.rs b/desktop/src-tauri/src/commands/personas/mod.rs index ad3f0b0f7..e5925418b 100644 --- a/desktop/src-tauri/src/commands/personas/mod.rs +++ b/desktop/src-tauri/src/commands/personas/mod.rs @@ -209,7 +209,31 @@ pub async fn update_persona( apply_persona_behavior(persona, input.behavior)?; persona.updated_at = now_iso(); + // Clone before the cap check so the mutable borrow from + // `personas.iter_mut().find()` ends here. let result = persona.clone(); + + // Reject persona save if the prospective MCP config would push + // any existing buzz-agent referencing this persona over the + // effective-server cap. + { + let agents = load_managed_agents(&app)?; + let global_config = + crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(); + // Filter to agents referencing this persona — the rest are + // unaffected and skipped by the resolver (wrong persona_id). + let referencing: Vec<_> = agents + .iter() + .filter(|a| a.persona_id.as_deref() == Some(input.id.as_str())) + .cloned() + .collect(); + crate::managed_agents::validate_effective_mcp_cap_for_records( + &referencing, + &personas, + &global_config.mcp_servers, + )?; + } + save_personas(&app, &personas)?; retain_persona_pending(&app, &state, &result); diff --git a/desktop/src-tauri/src/managed_agents/persona_events.rs b/desktop/src-tauri/src/managed_agents/persona_events.rs index fe760431b..31c94b906 100644 --- a/desktop/src-tauri/src/managed_agents/persona_events.rs +++ b/desktop/src-tauri/src/managed_agents/persona_events.rs @@ -191,6 +191,9 @@ pub fn persona_from_event(event: &nostr::Event) -> Result Result<(), String> { + for record in records { + let effective_cmd = crate::managed_agents::record_agent_command(record, personas); + if let Err(merge_err) = + effective_buzz_agent_mcp_servers(record, personas, global, &effective_cmd) + { + return Err(format!( + "saving would push agent `{}` over the MCP server limit: {merge_err}", + record.name + )); + } + } + Ok(()) +} diff --git a/desktop/src-tauri/src/managed_agents/types/tests.rs b/desktop/src-tauri/src/managed_agents/types/tests.rs index 573a0cb98..838824426 100644 --- a/desktop/src-tauri/src/managed_agents/types/tests.rs +++ b/desktop/src-tauri/src/managed_agents/types/tests.rs @@ -1,6 +1,7 @@ use super::{ - merge_mcp_servers, replace_mcp_servers, validate_effective_mcp_cap, validate_mcp_servers, - AgentDefinition, ManagedAgentRecord, McpServerConfig, McpServerEnvVar, MAX_USER_MCP_SERVERS, + merge_mcp_servers, replace_mcp_servers, validate_effective_mcp_cap, + validate_effective_mcp_cap_for_records, validate_mcp_servers, AgentDefinition, + ManagedAgentRecord, McpServerConfig, McpServerEnvVar, MAX_USER_MCP_SERVERS, }; use std::path::PathBuf; @@ -867,3 +868,119 @@ fn effective_cap_skips_non_buzz_agent_runtime() { validate_effective_mcp_cap(&record, &[], &global, "goose") .expect("non-buzz-agent runtime should skip the cap"); } + +// ── validate_effective_mcp_cap_for_records — inherited-layer gates ─────── + +/// Build a buzz-agent record with a persona reference and custom mcp_servers. +fn buzz_agent_record_with_persona( + name: &str, + persona_id: &str, + mcp_servers: Vec, +) -> ManagedAgentRecord { + serde_json::from_value(serde_json::json!({ + "pubkey": format!("pk-{name}"), + "name": name, + "persona_id": persona_id, + "relay_url": "", + "acp_command": "buzz-acp", + "agent_command": "buzz-agent", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "mcp_servers": mcp_servers, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + })) + .expect("minimal record should deserialize") +} + +/// Build a minimal AgentDefinition (persona) with the given mcp_servers. +fn persona_with_mcp(id: &str, mcp_servers: Vec) -> AgentDefinition { + serde_json::from_value(serde_json::json!({ + "id": id, + "display_name": format!("Persona {id}"), + "system_prompt": "", + "mcp_servers": mcp_servers, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + })) + .expect("minimal persona should deserialize") +} + +#[test] +fn inherited_gate_rejects_global_14_to_15_with_1_local_agent() { + // Agent has 1 local enabled server. Global goes from 14 → 15. Effective + // would be 16 — the gate must reject with the agent's name. + let record = buzz_agent_record(vec![mcp_server("local-0", "cmd", true)]); + let prospective_global: Vec<_> = (0..MAX_USER_MCP_SERVERS) + .map(|i| mcp_server(&format!("global-{i}"), "cmd", true)) + .collect(); + let err = validate_effective_mcp_cap_for_records(&[record], &[], &prospective_global) + .expect_err("global 14→15 with 1 local must reject"); + assert!( + err.contains("test"), + "error must name the offending agent: {err}" + ); + assert!( + err.contains("saving would push agent"), + "error must use the required phrasing: {err}" + ); +} + +#[test] +fn inherited_gate_allows_global_14_with_1_local_agent() { + // Agent has 1 local. Global stays at 14. Effective = 15 (at cap) — OK. + let record = buzz_agent_record(vec![mcp_server("local-0", "cmd", true)]); + let prospective_global: Vec<_> = (0..MAX_USER_MCP_SERVERS - 1) + .map(|i| mcp_server(&format!("global-{i}"), "cmd", true)) + .collect(); + validate_effective_mcp_cap_for_records(&[record], &[], &prospective_global) + .expect("15 effective should pass"); +} + +#[test] +fn inherited_gate_rejects_persona_unmask_pushing_agent_over_cap() { + // Agent has 1 local enabled. Global has 14. Persona goes from 0 → 1 + // unique enabled server. Effective = 16 — reject. + let persona_id = "p1"; + let record = buzz_agent_record_with_persona( + "agent-a", + persona_id, + vec![mcp_server("local-0", "cmd", true)], + ); + let global: Vec<_> = (0..MAX_USER_MCP_SERVERS - 1) + .map(|i| mcp_server(&format!("global-{i}"), "cmd", true)) + .collect(); + let persona = persona_with_mcp(persona_id, vec![mcp_server("persona-0", "cmd", true)]); + let err = validate_effective_mcp_cap_for_records(&[record], &[persona], &global) + .expect_err("persona add pushing over cap must reject"); + assert!( + err.contains("agent-a"), + "error must name the offending agent: {err}" + ); +} + +#[test] +fn inherited_gate_skips_non_buzz_agent_records() { + // A goose-runtime agent should be unaffected by the cap. + let mut record = buzz_agent_record(vec![mcp_server("local-0", "cmd", true)]); + // record_agent_command resolves via agent_command_override first. + record.agent_command_override = Some("goose".to_string()); + let prospective_global: Vec<_> = (0..=MAX_USER_MCP_SERVERS) + .map(|i| mcp_server(&format!("global-{i}"), "cmd", true)) + .collect(); + validate_effective_mcp_cap_for_records(&[record], &[], &prospective_global) + .expect("non-buzz-agent runtime should skip the cap"); +} + +#[test] +fn inherited_gate_allows_unaffected_agent() { + // Agent with no local servers. Global at 15 → effective = 15. OK. + let record = buzz_agent_record(vec![]); + let prospective_global: Vec<_> = (0..MAX_USER_MCP_SERVERS) + .map(|i| mcp_server(&format!("global-{i}"), "cmd", true)) + .collect(); + validate_effective_mcp_cap_for_records(&[record], &[], &prospective_global) + .expect("agent with no local servers at cap should pass"); +} From 4d3544dc0e131c2a0140b7ee68c2583801684d69 Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Tue, 14 Jul 2026 14:16:08 -0400 Subject: [PATCH 14/17] fix(desktop): shell-split MCP command field and retry on spawn failure D2: The MCP editor's command field is passed verbatim to Command::new(), so "uv run /path/to/jambot" tries to find a single binary with that full string as its name. Shell-split in configured_mcp_server(): first whitespace token becomes the executable, remaining tokens prepend before user-supplied args. D1: When a user-configured MCP server fails to spawn, the ACP agent returns a JSON-RPC error that previously killed the entire turn. Catch MCP-related AgentError in create_session_and_apply_model and retry session creation with empty mcp_servers so the agent can still respond. Surface the original error via observer as a warning. --- crates/buzz-acp/src/lib.rs | 46 +++++++++++++++++++++++++++++++++++-- crates/buzz-acp/src/pool.rs | 29 +++++++++++++++++++++-- 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index 59635dcbb..71afa0a9c 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -3454,10 +3454,20 @@ fn build_mcp_servers(config: &Config) -> Vec { } fn configured_mcp_server(server: &config::ConfiguredMcpServer) -> McpServer { + // Shell-split: users may type `uv run /path/to/jambot` as the command. + // First whitespace-delimited token is the executable; remaining tokens + // are prepended before `server.args`. + let mut parts = server.command.split_whitespace(); + let command = parts.next().unwrap_or_default().to_string(); + let prefix_args: Vec = parts.map(String::from).collect(); + + let mut args = prefix_args; + args.extend(server.args.iter().cloned()); + McpServer { name: server.name.clone(), - command: server.command.clone(), - args: server.args.clone(), + command, + args, env: server .env .iter() @@ -4068,6 +4078,38 @@ mod build_mcp_servers_tests { "Path::new(\".\").file_stem() is None — should fall back to \"mcp\"" ); } + + #[test] + fn configured_mcp_server_shell_splits_command() { + let server = config::ConfiguredMcpServer { + name: "jambot".into(), + command: "uv run /path/to/jambot".into(), + args: vec!["--port".into(), "8080".into()], + env: vec![], + }; + + let result = super::configured_mcp_server(&server); + assert_eq!(result.command, "uv"); + assert_eq!( + result.args, + vec!["run", "/path/to/jambot", "--port", "8080"], + ); + assert_eq!(result.name, "jambot"); + } + + #[test] + fn configured_mcp_server_bare_command_unchanged() { + let server = config::ConfiguredMcpServer { + name: "github".into(), + command: "npx".into(), + args: vec!["github-mcp".into()], + env: vec![], + }; + + let result = super::configured_mcp_server(&server); + assert_eq!(result.command, "npx"); + assert_eq!(result.args, vec!["github-mcp"]); + } } #[cfg(test)] diff --git a/crates/buzz-acp/src/pool.rs b/crates/buzz-acp/src/pool.rs index e57f44882..6b6f931c0 100644 --- a/crates/buzz-acp/src/pool.rs +++ b/crates/buzz-acp/src/pool.rs @@ -707,14 +707,39 @@ async fn create_session_and_apply_model( None }; - let resp = agent + let resp = match agent .acp .session_new_full( &ctx.cwd, ctx.mcp_servers.clone(), combined_system_prompt.as_deref(), ) - .await?; + .await + { + Ok(r) => r, + Err(AcpError::AgentError { ref message, .. }) + if !ctx.mcp_servers.is_empty() && message.to_lowercase().contains("mcp") => + { + // MCP spawn failure — retry without MCP servers so the agent can + // still respond. Surface the original error as a warning. + tracing::warn!( + target: "pool::mcp", + "MCP server spawn failed ({message}); retrying session without MCP servers" + ); + agent.acp.observe( + "mcp_spawn_warning", + serde_json::json!({ + "error": message, + "action": "retried_without_mcp_servers", + }), + ); + agent + .acp + .session_new_full(&ctx.cwd, vec![], combined_system_prompt.as_deref()) + .await? + } + Err(e) => return Err(e), + }; // Populate model capabilities on first session creation. if agent.model_capabilities.is_none() { From 1b58093059e340a233da90aa6edec3d9b4e9b4d5 Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Tue, 14 Jul 2026 14:38:20 -0400 Subject: [PATCH 15/17] fix(buzz-acp): use shlex for MCP command split and add fallback tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1: Replace `split_whitespace()` with `shlex::split()` in `configured_mcp_server` so quoted paths with internal spaces (e.g. `uv run "/My Bot/jambot"`) survive the split. On unmatched quotes, fall back to verbatim command — D1 surfaces the spawn failure as a warning instead of a dead agent. Added `shlex` 1.x as a direct dep (already in Cargo.lock as a transitive dep). F2: Add focused regression tests for `create_session_and_apply_model` MCP fallback path — scripted AcpClient returns an MCP-flavored error on first session/new, success on retry with empty mcpServers. Also covers non-MCP AgentError propagation (no retry triggered). --- Cargo.lock | 1 + crates/buzz-acp/Cargo.toml | 3 + crates/buzz-acp/src/lib.rs | 50 ++++++++-- crates/buzz-acp/src/pool.rs | 186 ++++++++++++++++++++++++++++++++++++ mobile/pubspec.lock | 16 ++-- 5 files changed, 242 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 96e1bf1ed..67e956711 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -751,6 +751,7 @@ dependencies = [ "serde", "serde_json", "sha2 0.11.0", + "shlex", "thiserror 2.0.18", "tokio", "tokio-tungstenite 0.29.0", diff --git a/crates/buzz-acp/Cargo.toml b/crates/buzz-acp/Cargo.toml index d3afb5062..d26b39bf1 100644 --- a/crates/buzz-acp/Cargo.toml +++ b/crates/buzz-acp/Cargo.toml @@ -71,6 +71,9 @@ toml = "1.0" # Filter expressions evalexpr = { workspace = true } +# Shell-word splitting (MCP server command field) +shlex = "1" + # Process-group kill (safe wrapper around killpg) — Unix-only; kill_process_group # has a #[cfg(not(unix))] fallback in acp.rs. [target.'cfg(unix)'.dependencies] diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index 71afa0a9c..db15a141a 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -3454,12 +3454,17 @@ fn build_mcp_servers(config: &Config) -> Vec { } fn configured_mcp_server(server: &config::ConfiguredMcpServer) -> McpServer { - // Shell-split: users may type `uv run /path/to/jambot` as the command. - // First whitespace-delimited token is the executable; remaining tokens - // are prepended before `server.args`. - let mut parts = server.command.split_whitespace(); - let command = parts.next().unwrap_or_default().to_string(); - let prefix_args: Vec = parts.map(String::from).collect(); + // Shell-word split: users may type `uv run "/My Bot/jambot"` as the command. + // shlex handles quoting so paths with spaces survive. On parse failure + // (unmatched quote), pass the whole string as the command verbatim — the + // spawn will fail, and D1 surfaces MCP spawn errors as warnings. + let (command, prefix_args) = match shlex::split(&server.command) { + Some(mut tokens) if !tokens.is_empty() => { + let cmd = tokens.remove(0); + (cmd, tokens) + } + _ => (server.command.clone(), vec![]), + }; let mut args = prefix_args; args.extend(server.args.iter().cloned()); @@ -4110,6 +4115,39 @@ mod build_mcp_servers_tests { assert_eq!(result.command, "npx"); assert_eq!(result.args, vec!["github-mcp"]); } + + #[test] + fn configured_mcp_server_quoted_path_with_spaces() { + let server = config::ConfiguredMcpServer { + name: "jambot".into(), + command: r#"uv run "/Users/me/My Bot/.venv/bin/jambot""#.into(), + args: vec!["--port".into(), "8080".into()], + env: vec![], + }; + + let result = super::configured_mcp_server(&server); + assert_eq!(result.command, "uv"); + assert_eq!( + result.args, + vec!["run", "/Users/me/My Bot/.venv/bin/jambot", "--port", "8080"], + "quoted path must preserve internal spaces and strip quotes" + ); + } + + #[test] + fn configured_mcp_server_unmatched_quote_falls_back_to_verbatim() { + let server = config::ConfiguredMcpServer { + name: "broken".into(), + command: r#"uv run "/unclosed/path"#.into(), + args: vec!["--flag".into()], + env: vec![], + }; + + let result = super::configured_mcp_server(&server); + // Unmatched quote → shlex returns None → verbatim fallback + assert_eq!(result.command, r#"uv run "/unclosed/path"#); + assert_eq!(result.args, vec!["--flag"]); + } } #[cfg(test)] diff --git a/crates/buzz-acp/src/pool.rs b/crates/buzz-acp/src/pool.rs index 6b6f931c0..a1332590d 100644 --- a/crates/buzz-acp/src/pool.rs +++ b/crates/buzz-acp/src/pool.rs @@ -4900,4 +4900,190 @@ mod tests { "timestamp must not use +00:00 offset" ); } + + // ── MCP fallback (D1) regression tests ─────────────────────────────────── + + use crate::acp::{AcpClient, McpServer}; + use crate::relay::RestClient; + + /// Spawn a bash script as an AcpClient, matching the acp.rs test helper. + async fn spawn_script(script: &str) -> AcpClient { + AcpClient::spawn("bash", &["-c".into(), script.into()], &[], false) + .await + .expect("failed to spawn test script") + } + + /// Build a minimal `PromptContext` with the given MCP servers. + /// Only `mcp_servers` and `cwd` are exercised by `create_session_and_apply_model`; + /// other fields are inert defaults. + fn test_prompt_context(mcp_servers: Vec) -> PromptContext { + let keys = Keys::generate(); + PromptContext { + mcp_servers, + initial_message: None, + idle_timeout: Duration::from_secs(60), + max_turn_duration: Duration::from_secs(60), + turn_liveness_interval: Duration::ZERO, + dedup_mode: crate::config::DedupMode::Drop, + system_prompt: None, + heartbeat_prompt: None, + base_prompt: None, + cwd: "/tmp".to_string(), + rest_client: RestClient { + http: reqwest::Client::new(), + base_url: String::new(), + keys: keys.clone(), + auth_tag_json: None, + }, + channel_info: std::collections::HashMap::new(), + context_message_limit: 0, + max_turns_per_session: 0, + permission_mode: crate::config::PermissionMode::Default, + agent_keys: keys, + agent_owner_pubkey: None, + memory_enabled: false, + harness_name: "test".to_string(), + } + } + + /// Build a minimal `OwnedAgent` from a pre-initialized `AcpClient`. + fn test_owned_agent(acp: AcpClient) -> OwnedAgent { + OwnedAgent { + index: 0, + acp, + state: SessionState { + sessions: HashMap::new(), + heartbeat_session: None, + turn_counts: HashMap::new(), + heartbeat_turn_count: 0, + core_sections: HashMap::new(), + canvas_sections: HashMap::new(), + }, + model_capabilities: None, + desired_model: None, + model_overridden: false, + protocol_version: 1, + } + } + + #[tokio::test] + async fn test_mcp_spawn_error_retries_without_mcp_servers() { + // Script: respond to initialize, then return an MCP error on the first + // session/new, and a valid session on the second (which should carry + // empty mcpServers). + let script = r#" + read -t 2 _init + echo '{"jsonrpc":"2.0","id":0,"result":{"protocolVersion":1,"agentCapabilities":{}}}' + read -t 2 REQ1 + echo '{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"mcp: spawn jambot: No such file or directory"}}' + read -t 2 REQ2 + echo '{"jsonrpc":"2.0","id":2,"result":{"sessionId":"ses_fallback","_retryRequest":'"$REQ2"'}}' + sleep 1 + "#; + let mut client = spawn_script(script).await; + client + .initialize() + .await + .expect("initialize should succeed"); + + let mut agent = test_owned_agent(client); + let ctx = test_prompt_context(vec![McpServer { + name: "jambot".into(), + command: "jambot".into(), + args: vec![], + env: vec![], + }]); + + let result = create_session_and_apply_model(&mut agent, &ctx, None, None).await; + let session_id = result.expect("fallback session should succeed"); + assert_eq!(session_id, "ses_fallback"); + } + + #[tokio::test] + async fn test_mcp_retry_sends_empty_mcp_servers() { + // Capture the retry request to a temp file and assert mcpServers: []. + let capture_file = + std::env::temp_dir().join(format!("buzz_mcp_retry_test_{}.json", std::process::id())); + let capture_path = capture_file.display().to_string(); + let script = format!( + r#" + read -t 2 _init + echo '{{"jsonrpc":"2.0","id":0,"result":{{"protocolVersion":1,"agentCapabilities":{{}}}}}}' + read -t 2 _req1 + echo '{{"jsonrpc":"2.0","id":1,"error":{{"code":-32000,"message":"mcp: spawn failed"}}}}' + read -t 2 REQ2 + echo "$REQ2" > {capture_path} + echo '{{"jsonrpc":"2.0","id":2,"result":{{"sessionId":"ses_ok"}}}}' + sleep 1 + "# + ); + let mut client = spawn_script(&script).await; + client + .initialize() + .await + .expect("initialize should succeed"); + + let mut agent = test_owned_agent(client); + let ctx = test_prompt_context(vec![McpServer { + name: "broken".into(), + command: "broken-server".into(), + args: vec![], + env: vec![], + }]); + + let result = create_session_and_apply_model(&mut agent, &ctx, None, None).await; + let session_id = result.expect("retry should succeed"); + assert_eq!(session_id, "ses_ok"); + + // Read the captured retry request and verify mcpServers is empty. + let captured = std::fs::read_to_string(&capture_file) + .expect("retry request should have been written to capture file"); + std::fs::remove_file(&capture_file).ok(); + let parsed: serde_json::Value = + serde_json::from_str(captured.trim()).expect("captured request should be valid JSON"); + let mcp_servers = parsed + .pointer("/params/mcpServers") + .expect("retry request must contain params.mcpServers"); + assert_eq!( + mcp_servers, + &serde_json::json!([]), + "retry must send empty mcpServers" + ); + } + + #[tokio::test] + async fn test_non_mcp_agent_error_propagates() { + // Non-MCP AgentError must NOT trigger the retry — it should propagate. + let script = r#" + read -t 2 _init + echo '{"jsonrpc":"2.0","id":0,"result":{"protocolVersion":1,"agentCapabilities":{}}}' + read -t 2 _req + echo '{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"authentication failed"}}' + sleep 1 + "#; + let mut client = spawn_script(script).await; + client + .initialize() + .await + .expect("initialize should succeed"); + + let mut agent = test_owned_agent(client); + let ctx = test_prompt_context(vec![McpServer { + name: "server".into(), + command: "some-server".into(), + args: vec![], + env: vec![], + }]); + + let result = create_session_and_apply_model(&mut agent, &ctx, None, None).await; + assert!( + result.is_err(), + "non-MCP error must propagate, not trigger retry" + ); + let err = result.unwrap_err(); + assert!( + err.to_string().contains("authentication failed"), + "error message must be preserved: {err}" + ); + } } diff --git a/mobile/pubspec.lock b/mobile/pubspec.lock index 260de02c1..a58476302 100644 --- a/mobile/pubspec.lock +++ b/mobile/pubspec.lock @@ -708,10 +708,10 @@ packages: dependency: transitive description: name: meta - sha256: "23f08335362185a5ea2ad3a4e597f1375e78bce8a040df5c600c8d3552ef2394" + sha256: "1741988757a65eb6b36abe716829688cf01910bbf91c34354ff7ec1c3de2b349" url: "https://pub.dev" source: hosted - version: "1.17.0" + version: "1.18.0" mime: dependency: transitive description: @@ -1137,26 +1137,26 @@ packages: dependency: transitive description: name: test - sha256: "280d6d890011ca966ad08df7e8a4ddfab0fb3aa49f96ed6de56e3521347a9ae7" + sha256: "8d9ceddbab833f180fbefed08afa76d7c03513dfdba87ffcec2718b02bbcbf20" url: "https://pub.dev" source: hosted - version: "1.30.0" + version: "1.31.0" test_api: dependency: transitive description: name: test_api - sha256: "8161c84903fd860b26bfdefb7963b3f0b68fee7adea0f59ef805ecca346f0c7a" + sha256: "949a932224383300f01be9221c39180316445ecb8e7547f70a41a35bf421fb9e" url: "https://pub.dev" source: hosted - version: "0.7.10" + version: "0.7.11" test_core: dependency: transitive description: name: test_core - sha256: "0381bd1585d1a924763c308100f2138205252fb90c9d4eeaf28489ee65ccde51" + sha256: "1991d4cfe85d5043241acac92962c3977c8d2f2add1ee73130c7b286417d1d34" url: "https://pub.dev" source: hosted - version: "0.6.16" + version: "0.6.17" tuple: dependency: transitive description: From b2bc6ea7626e44f00c0d5fb6e1bb912593e767bb Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Tue, 14 Jul 2026 17:22:59 -0400 Subject: [PATCH 16/17] fix: restore post-rebase MCP compatibility checks Current main added `PromptContext.team_instructions`, so the MCP fallback test fixture must explicitly initialize it. The MCP snapshot exclusion coverage and upstream shared type growth also require narrowly documented desktop file-size exceptions. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-acp/src/pool.rs | 1 + desktop/scripts/check-file-sizes.mjs | 9 +++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/buzz-acp/src/pool.rs b/crates/buzz-acp/src/pool.rs index a1332590d..8a050fe6b 100644 --- a/crates/buzz-acp/src/pool.rs +++ b/crates/buzz-acp/src/pool.rs @@ -4926,6 +4926,7 @@ mod tests { turn_liveness_interval: Duration::ZERO, dedup_mode: crate::config::DedupMode::Drop, system_prompt: None, + team_instructions: None, heartbeat_prompt: None, base_prompt: None, cwd: "/tmp".to_string(), diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 0761e97f3..b3b41e6a6 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -220,8 +220,13 @@ const overrides = new Map([ // `signout-wipe` added `Identity.resetFailed`; `team-instructions-first-class` // added team/instructions fields; MCP config adds the shared server types, // layer fields, IPC input fields, and effective-surface field. The gate - // counts trailing newlines, so 1,076 physical lines are 1,077 counted lines. - ["src/shared/api/types.ts", 1077], + // counts trailing newlines. Upstream added eight non-MCP type lines after the + // prior ceiling, and the MCP snapshot exclusion added no production types. + ["src/shared/api/types.ts", 1085], + // MCP snapshot-exclusion coverage adds a credential-bearing fixture and a + // negative serialization assertion. `agent_snapshot.rs` was already exactly + // at the 1,000-line limit; the gate counts trailing newlines. + ["src-tauri/src/managed_agents/agent_snapshot.rs", 1010], // readiness-gate: PersonaDialog.tsx threads computeLocalModeGate + // requiredCredentialEnvKeys + RequiredFieldLabel so the "New agent" dialog // shows required markers and credential amber rows (parity with From a2ccaa67a35e3af0434dbb9f3a13f51aafee5b43 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Tue, 14 Jul 2026 17:28:14 -0400 Subject: [PATCH 17/17] fix(desktop): repair MCP team rebase integration The team snapshot refactor introduced constructors after the MCP layer, and the team argument was added to spawn hashes. Keep local MCP configuration absent from imported shared snapshots while preserving regression coverage. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/commands/team_snapshot.rs | 3 +++ desktop/src-tauri/src/commands/team_snapshot/tests.rs | 3 +++ desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs | 6 ++++++ 3 files changed, 12 insertions(+) diff --git a/desktop/src-tauri/src/commands/team_snapshot.rs b/desktop/src-tauri/src/commands/team_snapshot.rs index c42ceb057..f7fb383e1 100644 --- a/desktop/src-tauri/src/commands/team_snapshot.rs +++ b/desktop/src-tauri/src/commands/team_snapshot.rs @@ -573,6 +573,9 @@ pub async fn confirm_team_snapshot_import( provider: member.definition.provider.clone(), persona_source_version: None, env_vars: std::collections::BTreeMap::new(), + // MCP configuration is local-only, never part of a shared team snapshot. + // Imported instances therefore inherit no local override. + mcp_servers: Vec::new(), start_on_app_launch: false, auto_restart_on_config_change: true, runtime_pid: None, diff --git a/desktop/src-tauri/src/commands/team_snapshot/tests.rs b/desktop/src-tauri/src/commands/team_snapshot/tests.rs index d7d2c39c3..039119a12 100644 --- a/desktop/src-tauri/src/commands/team_snapshot/tests.rs +++ b/desktop/src-tauri/src/commands/team_snapshot/tests.rs @@ -150,6 +150,7 @@ fn team_export_with_instance_and_memory_level_uses_supplied_entries() { source_team: None, source_team_persona_slug: None, env_vars: Default::default(), + mcp_servers: vec![], respond_to: None, respond_to_allowlist: vec![], parallelism: None, @@ -196,6 +197,7 @@ fn team_export_with_instance_and_memory_level_uses_supplied_entries() { provider: None, persona_source_version: None, env_vars: Default::default(), + mcp_servers: vec![], start_on_app_launch: false, auto_restart_on_config_change: true, runtime_pid: None, @@ -317,6 +319,7 @@ fn team_import_definitions_are_built_for_all_members() { definition.id.len() == 36 && definition.source_team.is_none() && definition.env_vars.is_empty() + && definition.mcp_servers.is_empty() && definition.respond_to_allowlist.is_empty() })); assert_eq!(definitions[0].system_prompt, "Alice prompt"); diff --git a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs index 342ef05f7..ebe52f822 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs @@ -152,12 +152,14 @@ fn buzz_agent_mcp_edit_changes_hash_but_other_runtime_ignores_it() { spawn_config_hash( &buzz_agent, &[], + &[], "wss://ws.example", &GlobalAgentConfig::default() ), spawn_config_hash( &edited, &[], + &[], "wss://ws.example", &GlobalAgentConfig::default() ) @@ -171,12 +173,14 @@ fn buzz_agent_mcp_edit_changes_hash_but_other_runtime_ignores_it() { spawn_config_hash( &goose, &[], + &[], "wss://ws.example", &GlobalAgentConfig::default() ), spawn_config_hash( &goose_edited, &[], + &[], "wss://ws.example", &GlobalAgentConfig::default() ) @@ -196,12 +200,14 @@ fn definition_mcp_edit_changes_buzz_agent_hash_without_becoming_agent_override() spawn_config_hash( &record, &[persona], + &[], "wss://ws.example", &GlobalAgentConfig::default() ), spawn_config_hash( &record, &[edited_persona], + &[], "wss://ws.example", &GlobalAgentConfig::default() )