feat: validate visible_skills field at config load time

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();
         }

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<String> {
-    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<Vec<ProviderModels>> {
     let model_override_path = models_override_file();
     let err = || {

src/config/request_context.rs

@@ -1738,14 +1738,22 @@ impl RequestContext {
                 let raw: Option<String> = super::parse_value(value)?;
                 let parsed: Option<Vec<String>> = 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");
+                                }
+                            }
                         }
                     }
                 }

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<String> = 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

src/function/skill.rs

@@ -104,8 +104,13 @@ pub async fn handle_skill_tool(
 fn handle_list(ctx: &RequestContext, policy: &SkillPolicy) -> Result<Value> {
     let mcp_on = ctx.app.config.mcp_server_support;
 
+    let visible_names: Vec<String> = 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;
         }

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<String> {
+            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;
                 }