From 829db30f2dd83ef585894c80f57566f06876aee7 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Thu, 4 Jun 2026 13:43:40 -0600 Subject: [PATCH] feat: validate visible_skills field at config load time --- src/config/app_config.rs | 22 ++++++++++++++++++++-- src/config/paths.rs | 11 ----------- src/config/request_context.rs | 20 ++++++++++++++------ src/config/skill_policy.rs | 30 +++++++++++++++++++----------- src/function/skill.rs | 7 ++++++- src/graph/validator.rs | 34 +++++++++++++++++++++++----------- 6 files changed, 82 insertions(+), 42 deletions(-) diff --git a/src/config/app_config.rs b/src/config/app_config.rs index 281c8f7..c19f68b 100644 --- a/src/config/app_config.rs +++ b/src/config/app_config.rs @@ -3,7 +3,7 @@ use crate::render::{MarkdownRender, RenderOptions}; use crate::utils::{IS_STDOUT_TERMINAL, NO_COLOR, decode_bin, get_env_name}; use super::paths; -use anyhow::{Context, Result, anyhow}; +use anyhow::{Context, Result, anyhow, bail}; use gman::providers::SupportedProvider; use indexmap::IndexMap; use serde::Deserialize; @@ -216,6 +216,7 @@ impl AppConfig { clients: config.clients, }; app_config.load_envs(); + app_config.validate_visible_skills()?; if let Some(wrap) = app_config.wrap.clone() { app_config.set_wrap(&wrap)?; } @@ -225,11 +226,28 @@ impl AppConfig { Ok(app_config) } + fn validate_visible_skills(&self) -> Result<()> { + let Some(skills) = self.visible_skills.as_ref() else { + return Ok(()); + }; + + for name in skills { + paths::validate_skill_name(name) + .map_err(|e| anyhow!("invalid entry in visible_skills: {e}"))?; + + if !paths::has_skill(name) { + bail!("visible_skills references skill '{name}' which is not installed"); + } + } + + Ok(()) + } + pub fn resolve_model(&mut self) -> Result<()> { if self.model_id.is_empty() { let models = list_models(self, crate::client::ModelType::Chat); if models.is_empty() { - anyhow::bail!("No available model"); + bail!("No available model"); } self.model_id = models[0].id(); } diff --git a/src/config/paths.rs b/src/config/paths.rs index fa26cfb..125fdf9 100644 --- a/src/config/paths.rs +++ b/src/config/paths.rs @@ -289,17 +289,6 @@ pub fn has_skill(name: &str) -> bool { skill_file(name).is_file() } -pub fn list_visible_skills(visible: Option<&[String]>) -> Vec { - let installed = list_skills(); - match visible { - None => installed, - Some(allow) => installed - .into_iter() - .filter(|name| allow.iter().any(|v| v == name)) - .collect(), - } -} - pub fn local_models_override() -> Result> { let model_override_path = models_override_file(); let err = || { diff --git a/src/config/request_context.rs b/src/config/request_context.rs index daf9131..495d8b0 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -1738,14 +1738,22 @@ impl RequestContext { let raw: Option = super::parse_value(value)?; let parsed: Option> = raw.map(|s| super::csv_to_vec(&s)); if let Some(names) = parsed.as_ref() { - let visible = - paths::list_visible_skills(self.app.config.visible_skills.as_deref()); + let visible = self.app.config.visible_skills.as_deref(); for name in names { paths::validate_skill_name(name)?; - if !visible.iter().any(|s| s == name) { - bail!( - "enabled_skills references skill '{name}' which is not visible (check global 'visible_skills' and that the skill is installed)" - ); + match visible { + Some(vs) => { + if !vs.iter().any(|s| s == name) { + bail!( + "skill '{name}' is not in the global 'visible_skills' allow-list" + ); + } + } + None => { + if !paths::has_skill(name) { + bail!("skill '{name}' is not installed"); + } + } } } } diff --git a/src/config/skill_policy.rs b/src/config/skill_policy.rs index 1b631eb..c331bce 100644 --- a/src/config/skill_policy.rs +++ b/src/config/skill_policy.rs @@ -4,7 +4,7 @@ use super::paths; use super::role::Role; use super::session::Session; -use anyhow::{Result, bail}; +use anyhow::{Result, anyhow, bail}; use std::collections::HashSet; #[derive(Debug)] @@ -76,16 +76,24 @@ impl SkillPolicy { Some(explicit) => { let set: HashSet = explicit.into_iter().collect(); for name in &set { - if !skill_exists(name) { - bail!("enabled_skills references skill '{name}' which is not installed"); - } - - if let Some(vs) = &visible - && !vs.contains(name) - { - bail!( - "enabled_skills references skill '{name}' which is not in visible_skills" - ); + paths::validate_skill_name(name).map_err(|e| { + anyhow!("enabled_skills contains invalid name '{name}': {e}") + })?; + match &visible { + Some(vs) => { + if !vs.contains(name) { + bail!( + "enabled_skills references skill '{name}' which is not in the global 'visible_skills' allow-list" + ); + } + } + None => { + if !skill_exists(name) { + bail!( + "enabled_skills references skill '{name}' which is not installed" + ); + } + } } } set diff --git a/src/function/skill.rs b/src/function/skill.rs index ceab466..a90bf46 100644 --- a/src/function/skill.rs +++ b/src/function/skill.rs @@ -104,8 +104,13 @@ pub async fn handle_skill_tool( fn handle_list(ctx: &RequestContext, policy: &SkillPolicy) -> Result { let mcp_on = ctx.app.config.mcp_server_support; + let visible_names: Vec = match ctx.app.config.visible_skills.as_deref() { + Some(list) => list.to_vec(), + None => paths::list_skills(), + }; + let mut entries = Vec::new(); - for name in paths::list_visible_skills(ctx.app.config.visible_skills.as_deref()) { + for name in visible_names { if !policy.allows(&name) { continue; } diff --git a/src/graph/validator.rs b/src/graph/validator.rs index 60f5e4f..90e58b4 100644 --- a/src/graph/validator.rs +++ b/src/graph/validator.rs @@ -196,9 +196,25 @@ impl GraphValidator { .as_ref() .and_then(|c| c.app_config.visible_skills.as_deref()); - let is_visible = |name: &str| match visible_skills { - None => true, - Some(list) => list.iter().any(|s| s == name), + let check_visibility = |name: &str| -> Option { + match visible_skills { + Some(list) => { + if !list.iter().any(|s| s == name) { + Some(format!( + "'{name}' is not in the global 'visible_skills' allow-list" + )) + } else { + None + } + } + None => { + if !paths::has_skill(name) { + Some(format!("'{name}' is not installed")) + } else { + None + } + } + } }; if let Some(graph_skills) = &graph.enabled_skills { @@ -209,10 +225,9 @@ impl GraphValidator { )); continue; } - - if !is_visible(name) { + if let Some(reason) = check_visibility(name) { result.error(ValidationError::new(format!( - "graph 'enabled_skills' references '{name}' which is not in global 'visible_skills'" + "graph 'enabled_skills': {reason}" ))); } } @@ -234,13 +249,10 @@ impl GraphValidator { )); continue; } - - if !is_visible(name) { + if let Some(reason) = check_visibility(name) { result.error(ValidationError::with_node( node_id, - format!( - "llm node 'enabled_skills' references '{name}' which is not in global 'visible_skills'" - ), + format!("llm node 'enabled_skills': {reason}"), )); continue; }