From 0c84eea70535566c8f981bf21cae3fba8441c6c0 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Wed, 3 Jun 2026 13:58:24 -0600 Subject: [PATCH] refactor: support both CSV and list formats for enabled_tools --- config.example.yaml | 9 +++++++- config.role.example.md | 4 +++- src/config/agent.rs | 9 ++++---- src/config/app_config.rs | 7 +++--- src/config/macros.rs | 2 +- src/config/mod.rs | 3 ++- src/config/request_context.rs | 21 +++++++++-------- src/config/role.rs | 31 ++++++++++++++++--------- src/config/session.rs | 16 ++++++++----- src/config/skill.rs | 11 +++++---- src/config/skill_registry.rs | 43 ++++++++++++++--------------------- src/function/skill.rs | 27 ++-------------------- src/graph/llm.rs | 6 ++--- src/graph/structured.rs | 7 ++++-- 14 files changed, 97 insertions(+), 99 deletions(-) diff --git a/config.example.yaml b/config.example.yaml index 13dbb48..4068864 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -82,7 +82,14 @@ vault_password_file: null # Path to a file containing the password for th function_calling_support: true # Enables or disables function calling (Globally). mapping_tools: # Alias for a tool or toolset fs: 'fs_cat,fs_ls,fs_mkdir,fs_rm,fs_write,fs_read,fs_glob,fs_grep' -enabled_tools: null # Which tools to enable by default. (e.g. 'fs,web_search_coyote') +enabled_tools: null # Which tools to enable by default. + # Accepts either a YAML list or a comma-separated string. Use 'all' to enable everything. + # Example (list form): + # enabled_tools: + # - fs + # - web_search_coyote + # Example (comma-separated form): + # enabled_tools: fs,web_search_coyote visible_tools: # Which tools are visible to be compiled (and are thus able to be defined in 'enabled_tools') # - demo_py.py # - demo_sh.sh diff --git a/config.role.example.md b/config.role.example.md index 5790153..84ffef7 100644 --- a/config.role.example.md +++ b/config.role.example.md @@ -8,7 +8,9 @@ name: # The name of the role 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_tools: # Tools to enable for this role. Accepts a YAML list (preferred) + - fs_ls # or a comma-separated string (e.g. `enabled_tools: fs_ls,fs_cat`). + - fs_cat # Use `all` to enable every visible tool. 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. diff --git a/src/config/agent.rs b/src/config/agent.rs index c894b1e..c04106d 100644 --- a/src/config/agent.rs +++ b/src/config/agent.rs @@ -548,7 +548,7 @@ impl RoleLike for Agent { self.config.top_p } - fn enabled_tools(&self) -> Option { + fn enabled_tools(&self) -> Option> { None } @@ -569,15 +569,14 @@ impl RoleLike for Agent { self.config.top_p = value; } - fn set_enabled_tools(&mut self, value: Option) { + fn set_enabled_tools(&mut self, value: Option>) { match value { Some(tools) => { - let tools = tools - .split(',') + self.config.global_tools = tools + .into_iter() .map(|v| v.trim().to_string()) .filter(|v| !v.is_empty()) .collect::>(); - self.config.global_tools = tools; } None => { self.config.global_tools.clear(); diff --git a/src/config/app_config.rs b/src/config/app_config.rs index 215ef84..281c8f7 100644 --- a/src/config/app_config.rs +++ b/src/config/app_config.rs @@ -34,7 +34,8 @@ pub struct AppConfig { pub function_calling_support: bool, pub mapping_tools: IndexMap, - pub enabled_tools: Option, + #[serde(default, deserialize_with = "super::deserialize_csv_or_vec")] + pub enabled_tools: Option>, pub visible_tools: Option>, pub skills_enabled: bool, @@ -394,7 +395,7 @@ impl AppConfig { self.mapping_tools = v; } if let Some(v) = super::read_env_value::(&get_env_name("enabled_tools")) { - self.enabled_tools = v; + self.enabled_tools = v.map(|raw| super::csv_to_vec(&raw)); } if let Some(Some(v)) = super::read_env_bool(&get_env_name("skills_enabled")) { @@ -516,7 +517,7 @@ impl AppConfig { } #[allow(dead_code)] - pub fn set_enabled_tools_default(&mut self, value: Option) { + pub fn set_enabled_tools_default(&mut self, value: Option>) { self.enabled_tools = value; } diff --git a/src/config/macros.rs b/src/config/macros.rs index 36a30ba..d4ea27b 100644 --- a/src/config/macros.rs +++ b/src/config/macros.rs @@ -33,7 +33,7 @@ pub async fn macro_execute( let mut app_config = (*ctx.app.config).clone(); app_config.temperature = role.temperature(); app_config.top_p = role.top_p(); - app_config.enabled_tools = role.enabled_tools().clone(); + app_config.enabled_tools = role.enabled_tools(); app_config.enabled_mcp_servers = role.enabled_mcp_servers(); let mut app_state = (*ctx.app).clone(); diff --git a/src/config/mod.rs b/src/config/mod.rs index 07b8640..d77d099 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -196,7 +196,8 @@ pub struct Config { pub function_calling_support: bool, pub mapping_tools: IndexMap, - pub enabled_tools: Option, + #[serde(default, deserialize_with = "deserialize_csv_or_vec")] + pub enabled_tools: Option>, pub visible_tools: Option>, pub skills_enabled: bool, diff --git a/src/config/request_context.rs b/src/config/request_context.rs index f11713b..1277c6b 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -700,7 +700,7 @@ impl RequestContext { } } - pub fn set_enabled_tools_on_role_like(&mut self, value: Option) -> bool { + pub fn set_enabled_tools_on_role_like(&mut self, value: Option>) -> bool { match self.role_like_mut() { Some(role_like) => { role_like.set_enabled_tools(value); @@ -854,7 +854,7 @@ impl RequestContext { ("top_p", super::format_option_value(&role.top_p())), ( "enabled_tools", - super::format_option_value(&role.enabled_tools()), + super::format_option_value(&role.enabled_tools().map(|v| v.join(","))), ), ( "enabled_mcp_servers", @@ -1148,10 +1148,10 @@ impl RequestContext { } let mut tool_names: HashSet = Default::default(); - if enabled_tools == "all" { + if enabled_tools.iter().any(|s| s.trim() == "all") { tool_names.extend(declaration_names); } else { - for item in enabled_tools.split(',') { + for item in enabled_tools.iter() { let item = item.trim(); if item.is_empty() { continue; @@ -1714,9 +1714,10 @@ impl RequestContext { } } "enabled_tools" => { - let value = super::parse_value(value)?; - if !self.set_enabled_tools_on_role_like(value.clone()) { - self.update_app_config(|app| app.enabled_tools = value); + let raw: Option = super::parse_value(value)?; + let parsed: Option> = raw.map(|s| super::csv_to_vec(&s)); + if !self.set_enabled_tools_on_role_like(parsed.clone()) { + self.update_app_config(|app| app.enabled_tools = parsed.clone()); } } "enabled_skills" => { @@ -3366,7 +3367,7 @@ mod tests { }; let ctx = RequestContext::new(app_state, WorkingMode::Cmd); let mut role = Role::new("r", "p"); - role.set_enabled_tools(Some("all".to_string())); + role.set_enabled_tools(Some(vec!["all".to_string()])); assert!(ctx.select_functions(&role).is_none()); } @@ -3377,7 +3378,7 @@ mod tests { ctx.tool_scope.functions.append_user_interaction_functions(); let mut role = Role::new("r", "p"); - role.set_enabled_tools(Some("all".to_string())); + role.set_enabled_tools(Some(vec!["all".to_string()])); let fns = ctx.select_functions(&role).unwrap(); let names: Vec<&str> = fns.iter().map(|f| f.name.as_str()).collect(); @@ -3391,7 +3392,7 @@ mod tests { ctx.tool_scope.functions.append_todo_functions(); let mut role = Role::new("r", "p"); - role.set_enabled_tools(Some("todo__init, todo__add".to_string())); + role.set_enabled_tools(Some(vec!["todo__init".to_string(), "todo__add".to_string()])); let fns = ctx.select_functions(&role).unwrap(); 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 b4cac23..b8b0048 100644 --- a/src/config/role.rs +++ b/src/config/role.rs @@ -28,12 +28,12 @@ pub trait RoleLike { fn model(&self) -> &Model; fn temperature(&self) -> Option; fn top_p(&self) -> Option; - fn enabled_tools(&self) -> Option; + fn enabled_tools(&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_tools(&mut self, value: Option>); fn set_enabled_mcp_servers(&mut self, value: Option>); } @@ -51,8 +51,12 @@ pub struct Role { temperature: Option, #[serde(skip_serializing_if = "Option::is_none")] top_p: Option, - #[serde(skip_serializing_if = "Option::is_none")] - enabled_tools: Option, + #[serde( + default, + skip_serializing_if = "Option::is_none", + deserialize_with = "super::deserialize_csv_or_vec" + )] + enabled_tools: Option>, #[serde( default, skip_serializing_if = "Option::is_none", @@ -106,7 +110,7 @@ impl Role { "model" => role.model_id = value.as_str().map(|v| v.to_string()), "temperature" => role.temperature = value.as_f64(), "top_p" => role.top_p = value.as_f64(), - "enabled_tools" => role.enabled_tools = value.as_str().map(|v| v.to_string()), + "enabled_tools" => role.enabled_tools = parse_string_or_array(value), "enabled_mcp_servers" => { role.enabled_mcp_servers = parse_string_or_array(value) } @@ -155,8 +159,10 @@ impl Role { if let Some(top_p) = self.top_p() { metadata.push(format!("top_p: {top_p}")); } - if let Some(enabled_tools) = self.enabled_tools() { - metadata.push(format!("enabled_tools: {enabled_tools}")); + if let Some(enabled_tools) = &self.enabled_tools { + let inline = + serde_json::to_string(enabled_tools).unwrap_or_else(|_| "[]".to_string()); + metadata.push(format!("enabled_tools: {inline}")); } if let Some(enabled_mcp_servers) = &self.enabled_mcp_servers { let inline = @@ -236,7 +242,7 @@ impl Role { model: &Model, temperature: Option, top_p: Option, - enabled_tools: Option, + enabled_tools: Option>, enabled_mcp_servers: Option>, ) { self.set_model(model.clone()); @@ -371,7 +377,7 @@ impl RoleLike for Role { self.top_p } - fn enabled_tools(&self) -> Option { + fn enabled_tools(&self) -> Option> { self.enabled_tools.clone() } @@ -394,7 +400,7 @@ impl RoleLike for Role { self.top_p = value; } - fn set_enabled_tools(&mut self, value: Option) { + fn set_enabled_tools(&mut self, value: Option>) { self.enabled_tools = value; } @@ -499,7 +505,10 @@ mod tests { fn role_new_parses_enabled_tools() { let content = "---\nenabled_tools: tool1,tool2\n---\nPrompt"; let role = Role::new("test", content); - assert_eq!(role.enabled_tools(), Some("tool1,tool2".to_string())); + assert_eq!( + role.enabled_tools(), + Some(vec!["tool1".to_string(), "tool2".to_string()]) + ); } #[test] diff --git a/src/config/session.rs b/src/config/session.rs index 6ce5f3e..d2cb53c 100644 --- a/src/config/session.rs +++ b/src/config/session.rs @@ -24,8 +24,12 @@ pub struct Session { temperature: Option, #[serde(skip_serializing_if = "Option::is_none")] top_p: Option, - #[serde(skip_serializing_if = "Option::is_none")] - enabled_tools: Option, + #[serde( + default, + skip_serializing_if = "Option::is_none", + deserialize_with = "super::deserialize_csv_or_vec" + )] + enabled_tools: Option>, #[serde( default, skip_serializing_if = "Option::is_none", @@ -197,7 +201,7 @@ impl Session { data["top_p"] = top_p.into(); } if let Some(enabled_tools) = self.enabled_tools() { - data["enabled_tools"] = enabled_tools.into(); + data["enabled_tools"] = json!(enabled_tools); } if let Some(enabled_mcp_servers) = self.enabled_mcp_servers() { data["enabled_mcp_servers"] = json!(enabled_mcp_servers); @@ -263,7 +267,7 @@ impl Session { } if let Some(enabled_tools) = self.enabled_tools() { - items.push(("enabled_tools", enabled_tools)); + items.push(("enabled_tools", enabled_tools.join(","))); } if let Some(enabled_mcp_servers) = self.enabled_mcp_servers() { @@ -711,7 +715,7 @@ impl RoleLike for Session { self.top_p } - fn enabled_tools(&self) -> Option { + fn enabled_tools(&self) -> Option> { self.enabled_tools.clone() } @@ -742,7 +746,7 @@ impl RoleLike for Session { } } - fn set_enabled_tools(&mut self, value: Option) { + fn set_enabled_tools(&mut self, value: Option>) { if self.enabled_tools != value { self.enabled_tools = value; self.dirty = true; diff --git a/src/config/skill.rs b/src/config/skill.rs index 068c6a6..0ead4b4 100644 --- a/src/config/skill.rs +++ b/src/config/skill.rs @@ -33,7 +33,7 @@ pub struct Skill { #[serde(default)] body: String, #[serde(skip_serializing_if = "Option::is_none")] - enabled_tools: Option, + enabled_tools: Option>, #[serde(skip_serializing_if = "Option::is_none")] enabled_mcp_servers: Option>, #[serde(skip_serializing_if = "Option::is_none")] @@ -69,7 +69,7 @@ impl Skill { } } "enabled_tools" => { - skill.enabled_tools = value.as_str().map(|v| v.to_string()); + skill.enabled_tools = parse_skill_string_or_array(value); } "enabled_mcp_servers" => { skill.enabled_mcp_servers = parse_skill_string_or_array(value); @@ -134,7 +134,7 @@ impl Skill { &self.body } - pub fn enabled_tools(&self) -> Option<&str> { + pub fn enabled_tools(&self) -> Option<&[String]> { self.enabled_tools.as_deref() } @@ -207,7 +207,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_tools(), + Some(["shell".to_string(), "fs".to_string()].as_slice()) + ); assert_eq!( skill.enabled_mcp_servers(), Some(["github".to_string()].as_slice()) diff --git a/src/config/skill_registry.rs b/src/config/skill_registry.rs index c0afa84..d7c8f64 100644 --- a/src/config/skill_registry.rs +++ b/src/config/skill_registry.rs @@ -69,14 +69,19 @@ impl SkillRegistry { let base_tools_set = effective.enabled_tools().is_some(); let base_mcps_set = effective.enabled_mcp_servers().is_some(); - let mut tools = parse_csv(effective.enabled_tools().as_deref()); + let mut tools: BTreeSet = effective + .enabled_tools() + .map(|v| v.into_iter().collect()) + .unwrap_or_default(); 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())); + if let Some(skill_tools) = skill.enabled_tools() { + tools.extend(skill_tools.iter().cloned()); + } if let Some(servers) = skill.enabled_mcp_servers() { mcps.extend(servers.iter().cloned()); } @@ -92,7 +97,7 @@ impl SkillRegistry { } if base_tools_set || !tools.is_empty() { - effective.set_enabled_tools(Some(join_csv(&tools))); + effective.set_enabled_tools(Some(tools.into_iter().collect())); } if base_mcps_set || !mcps.is_empty() { @@ -103,23 +108,6 @@ impl SkillRegistry { } } -fn parse_csv(s: Option<&str>) -> BTreeSet { - let mut set = BTreeSet::new(); - if let Some(raw) = s { - for token in raw.split(',') { - let trimmed = token.trim(); - if !trimmed.is_empty() { - set.insert(trimmed.to_string()); - } - } - } - set -} - -fn join_csv(set: &BTreeSet) -> String { - set.iter().cloned().collect::>().join(",") -} - #[cfg(test)] impl SkillRegistry { fn insert_for_test(&mut self, skill: Skill) { @@ -199,7 +187,7 @@ mod tests { assert_eq!(effective.prompt(), "Process: __INPUT__"); let tools = effective.enabled_tools().expect("tools set by skill"); - assert!(tools.contains("shell")); + assert!(tools.iter().any(|s| s == "shell")); } #[test] @@ -228,12 +216,12 @@ mod tests { )); let mut base = Role::new("test", "body"); - base.set_enabled_tools(Some("web_search".to_string())); + base.set_enabled_tools(Some(vec!["web_search".to_string()])); let effective = registry.effective_role(&base); - let tools_str = effective.enabled_tools().unwrap(); - let tools: BTreeSet<&str> = tools_str.split(',').collect(); + let tools_vec = effective.enabled_tools().unwrap(); + let tools: BTreeSet<&str> = tools_vec.iter().map(|s| s.as_str()).collect(); assert_eq!(tools, BTreeSet::from(["fs", "git", "shell", "web_search"])); let mcps_vec = effective.enabled_mcp_servers().unwrap(); @@ -259,10 +247,13 @@ mod tests { registry.insert_for_test(make_skill("knowledge", "", "Pure knowledge")); let mut base = Role::new("test", "Base"); - base.set_enabled_tools(Some(String::new())); + base.set_enabled_tools(Some(Vec::new())); let effective = registry.effective_role(&base); - assert_eq!(effective.enabled_tools().as_deref(), Some("")); + assert_eq!( + effective.enabled_tools().as_deref(), + Some([].as_slice()) + ); } #[test] diff --git a/src/function/skill.rs b/src/function/skill.rs index 9132601..63f7146 100644 --- a/src/function/skill.rs +++ b/src/function/skill.rs @@ -127,7 +127,7 @@ fn handle_list(ctx: &RequestContext, policy: &SkillPolicy) -> Result { entries.push(json!({ "name": skill.name(), "description": skill.description(), - "grants_tools": csv_to_vec(skill.enabled_tools()), + "grants_tools": skill.enabled_tools().unwrap_or_default(), "grants_mcp_servers": skill.enabled_mcp_servers().unwrap_or_default(), "loaded": ctx.skill_registry.is_loaded(skill.name()), })); @@ -166,7 +166,7 @@ async fn handle_load( let tools_declared = skill .enabled_tools() - .map(|s| !s.trim().is_empty()) + .map(|v| !v.is_empty()) .unwrap_or(false); let mcps_declared = skill .enabled_mcp_servers() @@ -226,16 +226,6 @@ async fn handle_unload(ctx: &mut RequestContext, args: &Value) -> Result })) } -fn csv_to_vec(csv: Option<&str>) -> Vec { - csv.map(|raw| { - raw.split(',') - .map(|t| t.trim().to_string()) - .filter(|t| !t.is_empty()) - .collect() - }) - .unwrap_or_default() -} - #[cfg(test)] mod tests { use super::*; @@ -294,17 +284,4 @@ mod tests { assert!(required, "skill__list should have no required parameters"); } - #[test] - fn csv_to_vec_empty_input() { - assert!(csv_to_vec(None).is_empty()); - assert!(csv_to_vec(Some("")).is_empty()); - assert!(csv_to_vec(Some(" ")).is_empty()); - } - - #[test] - fn csv_to_vec_parses_and_trims() { - let v = csv_to_vec(Some("a, b ,c,, d")); - - assert_eq!(v, vec!["a", "b", "c", "d"]); - } } diff --git a/src/graph/llm.rs b/src/graph/llm.rs index 4dbf600..d8e31b4 100644 --- a/src/graph/llm.rs +++ b/src/graph/llm.rs @@ -256,13 +256,13 @@ fn build_inline_role( } if node.tools.as_deref().unwrap_or_default().is_empty() { - role.set_enabled_tools(Some(String::new())); + role.set_enabled_tools(Some(Vec::new())); role.set_enabled_mcp_servers(Some(Vec::new())); } else { if !regular_tools.is_empty() { - role.set_enabled_tools(Some(regular_tools.join(","))); + role.set_enabled_tools(Some(regular_tools.to_vec())); } else { - role.set_enabled_tools(Some(String::new())); + role.set_enabled_tools(Some(Vec::new())); } if !mcp_servers.is_empty() { role.set_enabled_mcp_servers(Some(mcp_servers.to_vec())); diff --git a/src/graph/structured.rs b/src/graph/structured.rs index 908a801..7c62a1a 100644 --- a/src/graph/structured.rs +++ b/src/graph/structured.rs @@ -55,7 +55,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_tools(Some(Vec::new())); role.set_enabled_mcp_servers(Some(Vec::new())); Ok(role) } @@ -183,7 +183,10 @@ mod tests { fn build_extractor_role_disables_tools_and_mcp() { let role = build_extractor_role().expect("builtin role must exist"); - assert_eq!(role.enabled_tools().as_deref(), Some("")); + assert_eq!( + role.enabled_tools().as_deref(), + Some([].as_slice()) + ); assert_eq!( role.enabled_mcp_servers().as_deref(), Some([].as_slice())