From dd8da58105508f96dadb40507fea796370a96d13 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Wed, 3 Jun 2026 13:23:13 -0600 Subject: [PATCH] refactor: Support both CSV and list formats for enabled_mcp_servers --- config.example.yaml | 9 ++++- config.role.example.md | 4 +- src/config/agent.rs | 11 +++--- src/config/app_config.rs | 7 ++-- src/config/macros.rs | 2 +- src/config/mod.rs | 3 +- src/config/request_context.rs | 71 ++++++++++++++++++----------------- src/config/role.rs | 35 ++++++++++------- src/config/session.rs | 30 ++++++++++++--- src/config/skill.rs | 31 ++++++++++++--- src/config/skill_registry.rs | 19 ++++++---- src/function/skill.rs | 4 +- src/graph/llm.rs | 6 +-- src/graph/structured.rs | 7 +++- src/mcp/mod.rs | 32 ++++++++++------ 15 files changed, 174 insertions(+), 97 deletions(-) diff --git a/config.example.yaml b/config.example.yaml index 8aa7729..13dbb48 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -118,7 +118,14 @@ visible_tools: # Which tools are visible to be compiled (and a mcp_server_support: true # Enables or disables MCP servers (globally). mapping_mcp_servers: # Alias for an MCP server or set of servers git: github,gitmcp -enabled_mcp_servers: null # Which MCP servers to enable by default (e.g. 'github,slack,ddg-search') +enabled_mcp_servers: null # Which MCP servers to enable by default. + # Accepts either a YAML list or a comma-separated string. Use 'all' to enable everything. + # Example (list form): + # enabled_mcp_servers: + # - github + # - slack + # Example (comma-separated form): + # enabled_mcp_servers: github,slack,ddg-search # ---- Skills ---- # Skills are modular knowledge or capability packs the LLM can load and unload mid-conversation. diff --git a/config.role.example.md b/config.role.example.md index a1724cf..5790153 100644 --- a/config.role.example.md +++ b/config.role.example.md @@ -9,7 +9,9 @@ model: openai:gpt-4o # The model to use for this role temperature: 0.2 # The temperature to use for this role when querying the model top_p: 0 # The top_p to use for this role when querying the model enabled_tools: fs_ls,fs_cat # A comma-separated list of tools to enable for this role -enabled_mcp_servers: github,gitmcp # A comma-separated list of MCP servers to enable for this role +enabled_mcp_servers: # MCP servers to enable for this role. Accepts a YAML list (preferred) + - github # or a comma-separated string (e.g. `enabled_mcp_servers: github,gitmcp`). + - gitmcp # Use `all` to enable every configured MCP server. skills_enabled: true # Master switch for skills in this role (default: inherit from global). # Skills also require `function_calling_support: true` in the global config. enabled_skills: # Skills available when this role is active. Accepts a YAML list (preferred) diff --git a/src/config/agent.rs b/src/config/agent.rs index e93a513..c894b1e 100644 --- a/src/config/agent.rs +++ b/src/config/agent.rs @@ -552,8 +552,8 @@ impl RoleLike for Agent { None } - fn enabled_mcp_servers(&self) -> Option { - self.config.mcp_servers.clone().join(",").into() + fn enabled_mcp_servers(&self) -> Option> { + Some(self.config.mcp_servers.clone()) } fn set_model(&mut self, model: Model) { @@ -585,15 +585,14 @@ impl RoleLike for Agent { } } - fn set_enabled_mcp_servers(&mut self, value: Option) { + fn set_enabled_mcp_servers(&mut self, value: Option>) { match value { Some(servers) => { - let servers = servers - .split(',') + self.config.mcp_servers = servers + .into_iter() .map(|v| v.trim().to_string()) .filter(|v| !v.is_empty()) .collect::>(); - self.config.mcp_servers = servers; } None => { self.config.mcp_servers.clear(); diff --git a/src/config/app_config.rs b/src/config/app_config.rs index aa96770..215ef84 100644 --- a/src/config/app_config.rs +++ b/src/config/app_config.rs @@ -44,7 +44,8 @@ pub struct AppConfig { pub mcp_server_support: bool, pub mapping_mcp_servers: IndexMap, - pub enabled_mcp_servers: Option, + #[serde(default, deserialize_with = "super::deserialize_csv_or_vec")] + pub enabled_mcp_servers: Option>, pub auto_continue: bool, pub max_auto_continues: usize, @@ -413,7 +414,7 @@ impl AppConfig { self.mapping_mcp_servers = v; } if let Some(v) = super::read_env_value::(&get_env_name("enabled_mcp_servers")) { - self.enabled_mcp_servers = v; + self.enabled_mcp_servers = v.map(|raw| super::csv_to_vec(&raw)); } if let Some(v) = super::read_env_value::(&get_env_name("repl_prelude")) { @@ -520,7 +521,7 @@ impl AppConfig { } #[allow(dead_code)] - pub fn set_enabled_mcp_servers_default(&mut self, value: Option) { + pub fn set_enabled_mcp_servers_default(&mut self, value: Option>) { self.enabled_mcp_servers = value; } diff --git a/src/config/macros.rs b/src/config/macros.rs index 289952d..36a30ba 100644 --- a/src/config/macros.rs +++ b/src/config/macros.rs @@ -34,7 +34,7 @@ pub async fn macro_execute( app_config.temperature = role.temperature(); app_config.top_p = role.top_p(); app_config.enabled_tools = role.enabled_tools().clone(); - app_config.enabled_mcp_servers = role.enabled_mcp_servers().clone(); + app_config.enabled_mcp_servers = role.enabled_mcp_servers(); let mut app_state = (*ctx.app).clone(); app_state.config = Arc::new(app_config); diff --git a/src/config/mod.rs b/src/config/mod.rs index 51d3264..07b8640 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -206,7 +206,8 @@ pub struct Config { pub mcp_server_support: bool, pub mapping_mcp_servers: IndexMap, - pub enabled_mcp_servers: Option, + #[serde(default, deserialize_with = "deserialize_csv_or_vec")] + pub enabled_mcp_servers: Option>, pub auto_continue: bool, pub max_auto_continues: usize, diff --git a/src/config/request_context.rs b/src/config/request_context.rs index 2b11662..f11713b 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -710,7 +710,7 @@ impl RequestContext { } } - pub fn set_enabled_mcp_servers_on_role_like(&mut self, value: Option) -> bool { + pub fn set_enabled_mcp_servers_on_role_like(&mut self, value: Option>) -> bool { match self.role_like_mut() { Some(role_like) => { role_like.set_enabled_mcp_servers(value); @@ -858,7 +858,7 @@ impl RequestContext { ), ( "enabled_mcp_servers", - super::format_option_value(&role.enabled_mcp_servers()), + super::format_option_value(&role.enabled_mcp_servers().map(|v| v.join(","))), ), ( "max_output_tokens", @@ -1279,10 +1279,10 @@ impl RequestContext { } let mut server_names: HashSet = Default::default(); - if enabled_mcp_servers == "all" { + if enabled_mcp_servers.iter().any(|s| s.trim() == "all") { server_names.extend(mcp_declaration_names); } else { - for item in enabled_mcp_servers.split(',') { + for item in enabled_mcp_servers.iter() { let item = item.trim(); if item.is_empty() { continue; @@ -1733,8 +1733,9 @@ impl RequestContext { } } "enabled_mcp_servers" => { - let value: Option = super::parse_value(value)?; - if let Some(servers) = value.as_ref() { + let raw: Option = super::parse_value(value)?; + let parsed: Option> = raw.map(|s| super::csv_to_vec(&s)); + if let Some(servers) = parsed.as_ref() { let Some(mcp_config) = &self.app.mcp_config else { bail!( "No MCP servers are configured. Please configure MCP servers first before setting 'enabled_mcp_servers'." @@ -1746,7 +1747,7 @@ impl RequestContext { ); } - if !servers.split(',').all(|s| { + if !servers.iter().all(|s| { let server = s.trim(); server == "all" || mcp_config.mcp_servers.contains_key(server) }) { @@ -1755,8 +1756,8 @@ impl RequestContext { ); } } - if !self.set_enabled_mcp_servers_on_role_like(value.clone()) { - self.update_app_config(|app| app.enabled_mcp_servers = value.clone()); + if !self.set_enabled_mcp_servers_on_role_like(parsed.clone()) { + self.update_app_config(|app| app.enabled_mcp_servers = parsed.clone()); } if self.app.config.mcp_server_support { let app = Arc::clone(&self.app.config); @@ -2163,7 +2164,7 @@ impl RequestContext { async fn rebuild_tool_scope( &mut self, app: &AppConfig, - enabled_mcp_servers: Option, + enabled_mcp_servers: Option>, abort_signal: AbortSignal, ) -> Result<()> { let policy = SkillPolicy::effective( @@ -2175,21 +2176,23 @@ impl RequestContext { let enabled_mcp_servers = if policy.skills_enabled && app.mcp_server_support { let skill_mcps = self.skill_registry.loaded_mcp_servers(); - match (enabled_mcp_servers.as_deref(), skill_mcps.is_empty()) { - (Some("all"), _) | (_, true) => enabled_mcp_servers, - (base, false) => { - let mut merged: BTreeSet = skill_mcps; - if let Some(s) = base { - for token in s.split(',') { - let t = token.trim(); - if !t.is_empty() { - merged.insert(t.to_string()); - } + let has_all = enabled_mcp_servers + .as_ref() + .map(|v| v.iter().any(|s| s.trim() == "all")) + .unwrap_or(false); + if has_all || skill_mcps.is_empty() { + enabled_mcp_servers + } else { + let mut merged: BTreeSet = skill_mcps; + if let Some(servers) = &enabled_mcp_servers { + for token in servers { + let t = token.trim(); + if !t.is_empty() { + merged.insert(t.to_string()); } } - - Some(merged.into_iter().collect::>().join(",")) } + Some(merged.into_iter().collect()) } } else { enabled_mcp_servers @@ -2201,12 +2204,12 @@ impl RequestContext { && let Some(mcp_config) = &self.app.mcp_config { let server_ids: Vec = match &enabled_mcp_servers { - Some(servers) if servers == "all" => { + Some(servers) if servers.iter().any(|s| s.trim() == "all") => { mcp_config.mcp_servers.keys().cloned().collect() } Some(servers) => { let mut ids = Vec::new(); - for item in servers.split(',').map(|s| s.trim()) { + for item in servers.iter().map(|s| s.trim()) { if mcp_config.mcp_servers.contains_key(item) { ids.push(item.to_string()); } else if let Some(mapped) = app.mapping_mcp_servers.get(item) { @@ -2285,7 +2288,7 @@ impl RequestContext { if names.is_empty() { None } else { - Some(names.join(",")) + Some(names.to_vec()) } } else if let Some(role) = &self.role { role.enabled_mcp_servers() @@ -2445,7 +2448,7 @@ impl RequestContext { } let mcp_servers = if app.mcp_server_support { - (!agent.mcp_server_names().is_empty()).then(|| agent.mcp_server_names().join(",")) + (!agent.mcp_server_names().is_empty()).then(|| agent.mcp_server_names().to_vec()) } else { if !agent.mcp_server_names().is_empty() { bail!( @@ -2621,7 +2624,7 @@ impl RequestContext { let skill = Skill::load(name)?; let needs_mcps = skill .enabled_mcp_servers() - .map(|s| !s.trim().is_empty()) + .map(|v| !v.is_empty()) .unwrap_or(false); if needs_mcps && !self.app.config.mcp_server_support { @@ -2728,13 +2731,13 @@ impl RequestContext { &self, app: &AppConfig, start_mcp_servers: bool, - ) -> Option { + ) -> Option> { if !start_mcp_servers || !app.mcp_server_support { return None; } if let Some(agent) = self.agent.as_ref() { return (!agent.mcp_server_names().is_empty()) - .then(|| agent.mcp_server_names().join(",")); + .then(|| agent.mcp_server_names().to_vec()); } if let Some(session) = self.session.as_ref() { return session.enabled_mcp_servers(); @@ -3227,7 +3230,7 @@ mod tests { let app = ctx.app.config.clone(); let abort = utils::create_abort_signal(); - run_async(ctx.rebuild_tool_scope(&app, Some("all".to_string()), abort)).unwrap(); + run_async(ctx.rebuild_tool_scope(&app, Some(vec!["all".to_string()]), abort)).unwrap(); assert!(ctx.tool_scope.mcp_runtime.is_empty()); } @@ -3255,7 +3258,7 @@ mod tests { let app = ctx.app.config.clone(); let abort = utils::create_abort_signal(); - run_async(ctx.rebuild_tool_scope(&app, Some("all".to_string()), abort)).unwrap(); + run_async(ctx.rebuild_tool_scope(&app, Some(vec!["all".to_string()]), abort)).unwrap(); assert!(ctx.tool_scope.mcp_runtime.is_empty()); } @@ -3417,7 +3420,7 @@ mod tests { }; let ctx = RequestContext::new(app_state, WorkingMode::Cmd); let mut role = Role::new("r", "p"); - role.set_enabled_mcp_servers(Some("all".to_string())); + role.set_enabled_mcp_servers(Some(vec!["all".to_string()])); let result = ctx.select_enabled_mcp_servers(&role); assert!(result.is_empty()); } @@ -3430,7 +3433,7 @@ mod tests { .append_mcp_meta_functions(vec!["github".into(), "slack".into()]); let mut role = Role::new("r", "p"); - role.set_enabled_mcp_servers(Some("all".to_string())); + role.set_enabled_mcp_servers(Some(vec!["all".to_string()])); let fns = ctx.select_enabled_mcp_servers(&role); let names: Vec<&str> = fns.iter().map(|f| f.name.as_str()).collect(); @@ -3447,7 +3450,7 @@ mod tests { .append_mcp_meta_functions(vec!["github".into(), "slack".into()]); let mut role = Role::new("r", "p"); - role.set_enabled_mcp_servers(Some("github".to_string())); + role.set_enabled_mcp_servers(Some(vec!["github".to_string()])); let fns = ctx.select_enabled_mcp_servers(&role); let names: Vec<&str> = fns.iter().map(|f| f.name.as_str()).collect(); diff --git a/src/config/role.rs b/src/config/role.rs index b3f77b8..b4cac23 100644 --- a/src/config/role.rs +++ b/src/config/role.rs @@ -29,12 +29,12 @@ pub trait RoleLike { fn temperature(&self) -> Option; fn top_p(&self) -> Option; fn enabled_tools(&self) -> Option; - fn enabled_mcp_servers(&self) -> Option; + fn enabled_mcp_servers(&self) -> Option>; fn set_model(&mut self, model: Model); fn set_temperature(&mut self, value: Option); fn set_top_p(&mut self, value: Option); fn set_enabled_tools(&mut self, value: Option); - fn set_enabled_mcp_servers(&mut self, value: Option); + fn set_enabled_mcp_servers(&mut self, value: Option>); } #[derive(Debug, Clone, Default, Deserialize, Serialize)] @@ -53,8 +53,12 @@ pub struct Role { top_p: Option, #[serde(skip_serializing_if = "Option::is_none")] enabled_tools: Option, - #[serde(skip_serializing_if = "Option::is_none")] - enabled_mcp_servers: Option, + #[serde( + default, + skip_serializing_if = "Option::is_none", + deserialize_with = "super::deserialize_csv_or_vec" + )] + enabled_mcp_servers: Option>, #[serde(skip_serializing_if = "Option::is_none")] skills_enabled: Option, #[serde( @@ -104,10 +108,10 @@ impl Role { "top_p" => role.top_p = value.as_f64(), "enabled_tools" => role.enabled_tools = value.as_str().map(|v| v.to_string()), "enabled_mcp_servers" => { - role.enabled_mcp_servers = value.as_str().map(|v| v.to_string()) + role.enabled_mcp_servers = parse_string_or_array(value) } "skills_enabled" => role.skills_enabled = value.as_bool(), - "enabled_skills" => role.enabled_skills = parse_enabled_skills_value(value), + "enabled_skills" => role.enabled_skills = parse_string_or_array(value), "auto_continue" => role.auto_continue = value.as_bool(), "max_auto_continues" => { role.max_auto_continues = value.as_u64().map(|v| v as usize) @@ -154,8 +158,10 @@ impl Role { if let Some(enabled_tools) = self.enabled_tools() { metadata.push(format!("enabled_tools: {enabled_tools}")); } - if let Some(enabled_mcp_servers) = self.enabled_mcp_servers() { - metadata.push(format!("enabled_mcp_servers: {enabled_mcp_servers}")); + if let Some(enabled_mcp_servers) = &self.enabled_mcp_servers { + let inline = + serde_json::to_string(enabled_mcp_servers).unwrap_or_else(|_| "[]".to_string()); + metadata.push(format!("enabled_mcp_servers: {inline}")); } if let Some(skills_enabled) = self.skills_enabled { metadata.push(format!("skills_enabled: {skills_enabled}")); @@ -231,7 +237,7 @@ impl Role { temperature: Option, top_p: Option, enabled_tools: Option, - enabled_mcp_servers: Option, + enabled_mcp_servers: Option>, ) { self.set_model(model.clone()); if temperature.is_some() { @@ -369,7 +375,7 @@ impl RoleLike for Role { self.enabled_tools.clone() } - fn enabled_mcp_servers(&self) -> Option { + fn enabled_mcp_servers(&self) -> Option> { self.enabled_mcp_servers.clone() } @@ -392,12 +398,12 @@ impl RoleLike for Role { self.enabled_tools = value; } - fn set_enabled_mcp_servers(&mut self, value: Option) { + fn set_enabled_mcp_servers(&mut self, value: Option>) { self.enabled_mcp_servers = value; } } -fn parse_enabled_skills_value(value: &Value) -> Option> { +fn parse_string_or_array(value: &Value) -> Option> { if value.is_null() { return None; } @@ -500,7 +506,10 @@ mod tests { fn role_new_parses_enabled_mcp_servers() { let content = "---\nenabled_mcp_servers: github,jira\n---\nPrompt"; let role = Role::new("test", content); - assert_eq!(role.enabled_mcp_servers(), Some("github,jira".to_string())); + assert_eq!( + role.enabled_mcp_servers(), + Some(vec!["github".to_string(), "jira".to_string()]) + ); } #[test] diff --git a/src/config/session.rs b/src/config/session.rs index b098ff2..6ce5f3e 100644 --- a/src/config/session.rs +++ b/src/config/session.rs @@ -26,8 +26,12 @@ pub struct Session { top_p: Option, #[serde(skip_serializing_if = "Option::is_none")] enabled_tools: Option, - #[serde(skip_serializing_if = "Option::is_none")] - enabled_mcp_servers: Option, + #[serde( + default, + skip_serializing_if = "Option::is_none", + deserialize_with = "super::deserialize_csv_or_vec" + )] + enabled_mcp_servers: Option>, #[serde(skip_serializing_if = "Option::is_none")] skills_enabled: Option, #[serde( @@ -196,7 +200,13 @@ impl Session { data["enabled_tools"] = enabled_tools.into(); } if let Some(enabled_mcp_servers) = self.enabled_mcp_servers() { - data["enabled_mcp_servers"] = enabled_mcp_servers.into(); + data["enabled_mcp_servers"] = json!(enabled_mcp_servers); + } + if let Some(skills_enabled) = self.skills_enabled() { + data["skills_enabled"] = skills_enabled.into(); + } + if let Some(enabled_skills) = self.enabled_skills() { + data["enabled_skills"] = json!(enabled_skills); } if let Some(save_session) = self.save_session() { data["save_session"] = save_session.into(); @@ -257,7 +267,15 @@ impl Session { } if let Some(enabled_mcp_servers) = self.enabled_mcp_servers() { - items.push(("enabled_mcp_servers", enabled_mcp_servers)); + items.push(("enabled_mcp_servers", enabled_mcp_servers.join(","))); + } + + if let Some(skills_enabled) = self.skills_enabled() { + items.push(("skills_enabled", skills_enabled.to_string())); + } + + if let Some(enabled_skills) = self.enabled_skills() { + items.push(("enabled_skills", enabled_skills.join(","))); } if let Some(save_session) = self.save_session() { @@ -697,7 +715,7 @@ impl RoleLike for Session { self.enabled_tools.clone() } - fn enabled_mcp_servers(&self) -> Option { + fn enabled_mcp_servers(&self) -> Option> { self.enabled_mcp_servers.clone() } @@ -731,7 +749,7 @@ impl RoleLike for Session { } } - fn set_enabled_mcp_servers(&mut self, value: Option) { + fn set_enabled_mcp_servers(&mut self, value: Option>) { if self.enabled_mcp_servers != value { self.enabled_mcp_servers = value; self.dirty = true; diff --git a/src/config/skill.rs b/src/config/skill.rs index d60a52d..068c6a6 100644 --- a/src/config/skill.rs +++ b/src/config/skill.rs @@ -35,7 +35,7 @@ pub struct Skill { #[serde(skip_serializing_if = "Option::is_none")] enabled_tools: Option, #[serde(skip_serializing_if = "Option::is_none")] - enabled_mcp_servers: Option, + enabled_mcp_servers: Option>, #[serde(skip_serializing_if = "Option::is_none")] auto_unload: Option, } @@ -72,7 +72,7 @@ impl Skill { skill.enabled_tools = value.as_str().map(|v| v.to_string()); } "enabled_mcp_servers" => { - skill.enabled_mcp_servers = value.as_str().map(|v| v.to_string()); + skill.enabled_mcp_servers = parse_skill_string_or_array(value); } "auto_unload" => { skill.auto_unload = value.as_bool(); @@ -138,7 +138,7 @@ impl Skill { self.enabled_tools.as_deref() } - pub fn enabled_mcp_servers(&self) -> Option<&str> { + pub fn enabled_mcp_servers(&self) -> Option<&[String]> { self.enabled_mcp_servers.as_deref() } @@ -157,11 +157,29 @@ impl Skill { fn declares_mcp_servers(&self) -> bool { self.enabled_mcp_servers .as_deref() - .map(|s| !s.trim().is_empty()) + .map(|servers| !servers.is_empty()) .unwrap_or(false) } } +fn parse_skill_string_or_array(value: &Value) -> Option> { + if value.is_null() { + return None; + } + if let Some(s) = value.as_str() { + return Some(csv_to_vec(s)); + } + if let Some(arr) = value.as_array() { + let items: Vec = arr + .iter() + .filter_map(|v| v.as_str().map(|s| s.trim().to_string())) + .filter(|s| !s.is_empty()) + .collect(); + return Some(items); + } + None +} + #[cfg(test)] mod tests { use super::*; @@ -190,7 +208,10 @@ mod tests { assert_eq!(skill.name(), "git-master"); assert_eq!(skill.description(), "Atomic commits, rebase surgery"); assert_eq!(skill.enabled_tools(), Some("shell,fs")); - assert_eq!(skill.enabled_mcp_servers(), Some("github")); + assert_eq!( + skill.enabled_mcp_servers(), + Some(["github".to_string()].as_slice()) + ); assert!(skill.auto_unload()); assert_eq!(skill.body(), "You are a git expert"); } diff --git a/src/config/skill_registry.rs b/src/config/skill_registry.rs index 235a4fd..c0afa84 100644 --- a/src/config/skill_registry.rs +++ b/src/config/skill_registry.rs @@ -38,8 +38,8 @@ impl SkillRegistry { pub fn loaded_mcp_servers(&self) -> BTreeSet { let mut out = BTreeSet::new(); for skill in self.loaded.values() { - if let Some(csv) = skill.enabled_mcp_servers() { - for token in csv.split(',') { + if let Some(servers) = skill.enabled_mcp_servers() { + for token in servers { let t = token.trim(); if !t.is_empty() { out.insert(t.to_string()); @@ -70,11 +70,16 @@ impl SkillRegistry { let base_mcps_set = effective.enabled_mcp_servers().is_some(); let mut tools = parse_csv(effective.enabled_tools().as_deref()); - let mut mcps = parse_csv(effective.enabled_mcp_servers().as_deref()); + let mut mcps: BTreeSet = effective + .enabled_mcp_servers() + .map(|v| v.into_iter().collect()) + .unwrap_or_default(); for (_, skill) in &self.loaded { tools.extend(parse_csv(skill.enabled_tools())); - mcps.extend(parse_csv(skill.enabled_mcp_servers())); + if let Some(servers) = skill.enabled_mcp_servers() { + mcps.extend(servers.iter().cloned()); + } if !skip_body && !skill.body().is_empty() { let separator = if effective.is_empty_prompt() { "" @@ -91,7 +96,7 @@ impl SkillRegistry { } if base_mcps_set || !mcps.is_empty() { - effective.set_enabled_mcp_servers(Some(join_csv(&mcps))); + effective.set_enabled_mcp_servers(Some(mcps.into_iter().collect())); } effective @@ -231,8 +236,8 @@ mod tests { let tools: BTreeSet<&str> = tools_str.split(',').collect(); assert_eq!(tools, BTreeSet::from(["fs", "git", "shell", "web_search"])); - let mcps_str = effective.enabled_mcp_servers().unwrap(); - let mcps: BTreeSet<&str> = mcps_str.split(',').collect(); + let mcps_vec = effective.enabled_mcp_servers().unwrap(); + let mcps: BTreeSet<&str> = mcps_vec.iter().map(|s| s.as_str()).collect(); assert_eq!(mcps, BTreeSet::from(["github", "jira"])); } diff --git a/src/function/skill.rs b/src/function/skill.rs index 52d43c1..9132601 100644 --- a/src/function/skill.rs +++ b/src/function/skill.rs @@ -128,7 +128,7 @@ fn handle_list(ctx: &RequestContext, policy: &SkillPolicy) -> Result { "name": skill.name(), "description": skill.description(), "grants_tools": csv_to_vec(skill.enabled_tools()), - "grants_mcp_servers": csv_to_vec(skill.enabled_mcp_servers()), + "grants_mcp_servers": skill.enabled_mcp_servers().unwrap_or_default(), "loaded": ctx.skill_registry.is_loaded(skill.name()), })); } @@ -170,7 +170,7 @@ async fn handle_load( .unwrap_or(false); let mcps_declared = skill .enabled_mcp_servers() - .map(|s| !s.trim().is_empty()) + .map(|v| !v.is_empty()) .unwrap_or(false); if tools_declared && !function_calling_on { diff --git a/src/graph/llm.rs b/src/graph/llm.rs index 61d0141..4dbf600 100644 --- a/src/graph/llm.rs +++ b/src/graph/llm.rs @@ -257,7 +257,7 @@ fn build_inline_role( if node.tools.as_deref().unwrap_or_default().is_empty() { role.set_enabled_tools(Some(String::new())); - role.set_enabled_mcp_servers(Some(String::new())); + role.set_enabled_mcp_servers(Some(Vec::new())); } else { if !regular_tools.is_empty() { role.set_enabled_tools(Some(regular_tools.join(","))); @@ -265,9 +265,9 @@ fn build_inline_role( role.set_enabled_tools(Some(String::new())); } if !mcp_servers.is_empty() { - role.set_enabled_mcp_servers(Some(mcp_servers.join(","))); + role.set_enabled_mcp_servers(Some(mcp_servers.to_vec())); } else { - role.set_enabled_mcp_servers(Some(String::new())); + role.set_enabled_mcp_servers(Some(Vec::new())); } } diff --git a/src/graph/structured.rs b/src/graph/structured.rs index a590709..908a801 100644 --- a/src/graph/structured.rs +++ b/src/graph/structured.rs @@ -56,7 +56,7 @@ async fn extract_via_extractor( fn build_extractor_role() -> Result { let mut role = Role::new(EXTRACTOR_ROLE_NAME, EXTRACTOR_ROLE_PROMPT); role.set_enabled_tools(Some(String::new())); - role.set_enabled_mcp_servers(Some(String::new())); + role.set_enabled_mcp_servers(Some(Vec::new())); Ok(role) } @@ -184,6 +184,9 @@ mod tests { let role = build_extractor_role().expect("builtin role must exist"); assert_eq!(role.enabled_tools().as_deref(), Some("")); - assert_eq!(role.enabled_mcp_servers().as_deref(), Some("")); + assert_eq!( + role.enabled_mcp_servers().as_deref(), + Some([].as_slice()) + ); } } diff --git a/src/mcp/mod.rs b/src/mcp/mod.rs index 2528a52..22b29d6 100644 --- a/src/mcp/mod.rs +++ b/src/mcp/mod.rs @@ -146,7 +146,7 @@ impl McpRegistry { pub async fn init( log_path: Option, start_mcp_servers: bool, - enabled_mcp_servers: Option, + enabled_mcp_servers: Option>, abort_signal: AbortSignal, app_config: &AppConfig, vault: &Vault, @@ -216,7 +216,7 @@ impl McpRegistry { async fn start_select_mcp_servers( &mut self, - enabled_mcp_servers: Option, + enabled_mcp_servers: Option>, ) -> Result<()> { if self.config.is_none() { debug!( @@ -292,15 +292,15 @@ impl McpRegistry { Ok((id.to_string(), service, catalog)) } - fn resolve_server_ids(&self, enabled_mcp_servers: Option) -> Vec { + fn resolve_server_ids(&self, enabled_mcp_servers: Option>) -> Vec { if let Some(config) = &self.config && let Some(servers) = enabled_mcp_servers { - if servers == "all" { + if servers.iter().any(|s| s.trim() == "all") { config.mcp_servers.keys().cloned().collect() } else { let enabled_servers: HashSet = - servers.split(',').map(|s| s.trim().to_string()).collect(); + servers.into_iter().map(|s| s.trim().to_string()).collect(); config .mcp_servers .keys() @@ -754,7 +754,7 @@ mod tests { #[test] fn resolve_all_returns_all_configured_servers() { let registry = make_registry_with_config(&["github", "slack", "jira"]); - let mut ids = registry.resolve_server_ids(Some("all".to_string())); + let mut ids = registry.resolve_server_ids(Some(vec!["all".to_string()])); ids.sort(); assert_eq!(ids, vec!["github", "jira", "slack"]); } @@ -762,7 +762,8 @@ mod tests { #[test] fn resolve_comma_separated_returns_matching_servers() { let registry = make_registry_with_config(&["github", "slack", "jira"]); - let mut ids = registry.resolve_server_ids(Some("github, jira".to_string())); + let mut ids = registry + .resolve_server_ids(Some(vec!["github".to_string(), "jira".to_string()])); ids.sort(); assert_eq!(ids, vec!["github", "jira"]); } @@ -770,7 +771,7 @@ mod tests { #[test] fn resolve_single_server_name() { let registry = make_registry_with_config(&["github", "slack"]); - let ids = registry.resolve_server_ids(Some("slack".to_string())); + let ids = registry.resolve_server_ids(Some(vec!["slack".to_string()])); assert_eq!(ids, vec!["slack"]); } @@ -784,28 +785,35 @@ mod tests { #[test] fn resolve_no_config_returns_empty() { let registry = McpRegistry::default(); - let ids = registry.resolve_server_ids(Some("all".to_string())); + let ids = registry.resolve_server_ids(Some(vec!["all".to_string()])); assert!(ids.is_empty()); } #[test] fn resolve_nonexistent_server_filtered_out() { let registry = make_registry_with_config(&["github"]); - let ids = registry.resolve_server_ids(Some("github, nonexistent".to_string())); + let ids = registry.resolve_server_ids(Some(vec![ + "github".to_string(), + "nonexistent".to_string(), + ])); assert_eq!(ids, vec!["github"]); } #[test] fn resolve_all_nonexistent_returns_empty() { let registry = make_registry_with_config(&["github"]); - let ids = registry.resolve_server_ids(Some("foo, bar".to_string())); + let ids = + registry.resolve_server_ids(Some(vec!["foo".to_string(), "bar".to_string()])); assert!(ids.is_empty()); } #[test] fn resolve_trims_whitespace() { let registry = make_registry_with_config(&["github", "slack"]); - let mut ids = registry.resolve_server_ids(Some(" github , slack ".to_string())); + let mut ids = registry.resolve_server_ids(Some(vec![ + " github ".to_string(), + " slack ".to_string(), + ])); ids.sort(); assert_eq!(ids, vec!["github", "slack"]); }