fix: enforced global visible_skills in llm node validation and improved skill loading error handling across the project

assets/agents/coder/scripts/route_review_result.sh

@@ -14,6 +14,21 @@ review_attempts=$(echo "$state" | jq -r '.review_attempts // 0')
 max_review_attempts=$(echo "$state" | jq -r '.max_review_attempts // 1')
 review_notes=$(echo "$state" | jq -r '.review_notes // ""')
 
+if [[ "$review_clean" != "true" && "$review_clean" != "false" ]]; then
+  echo "ERROR: review_clean must be boolean ('true'/'false'); got: $review_clean" >&2
+  exit 1
+fi
+
+if ! [[ "$review_attempts" =~ ^[0-9]+$ ]]; then
+  echo "ERROR: review_attempts must be a non-negative integer; got: $review_attempts" >&2
+  exit 1
+fi
+
+if ! [[ "$max_review_attempts" =~ ^[0-9]+$ ]]; then
+  echo "ERROR: max_review_attempts must be a non-negative integer; got: $max_review_attempts" >&2
+  exit 1
+fi
+
 if [[ "$review_clean" == "true" ]]; then
   jq -nc '{"_next": "end_success"}'
   exit 0

assets/functions/tools/fs_grep.sh

@@ -28,6 +28,8 @@ main() {
     local grep_args=(-nH --color=never)
 
     if [[ -d "$search_path" ]]; then
+        # Use -r (not -R) so symlinks to directories are NOT followed - this avoids
+        # infinite loops on pathological symlink cycles (e.g. `ln -s . loop`).
         grep_args+=(-r)
         grep_args+=(
             --exclude-dir='.git'

src/config/paths.rs

@@ -289,6 +289,17 @@ 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

@@ -1737,6 +1737,18 @@ impl RequestContext {
             "enabled_skills" => {
                 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());
+                    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)"
+                            );
+                        }
+                    }
+                }
                 self.update_app_config(|app| app.enabled_skills = parsed.clone());
             }
             "skills_enabled" => {
@@ -2650,7 +2662,9 @@ impl RequestContext {
 
         self.skill_registry.insert(skill)?;
         if let Err(e) = self.refresh_tool_scope(abort_signal).await {
-            let _ = self.skill_registry.unload(name);
+            if let Err(unload_err) = self.skill_registry.unload(name) {
+                warn!("Failed to unload skill '{name}' during error recovery: {unload_err}");
+            }
             bail!("Loaded skill '{name}' but failed to refresh tool scope: {e}");
         }
 
@@ -2659,10 +2673,15 @@ impl RequestContext {
     }
 
     pub async fn unload_skill_repl(&mut self, name: &str, abort_signal: AbortSignal) -> Result<()> {
-        self.skill_registry.unload(name)?;
+        let skill = self.skill_registry.unload(name)?;
 
         if let Err(e) = self.refresh_tool_scope(abort_signal).await {
-            eprintln!("Warning: unloaded skill '{name}' but tool scope refresh failed: {e}");
+            if let Err(restore_err) = self.skill_registry.insert(skill) {
+                warn!(
+                    "Failed to restore skill '{name}' after tool-scope refresh failure: {restore_err}"
+                );
+            }
+            bail!("Unloaded skill '{name}' but failed to refresh tool scope; restored: {e}");
         }
 
         println!("✓ Unloaded skill '{name}'.");

src/function/skill.rs

@@ -105,7 +105,7 @@ fn handle_list(ctx: &RequestContext, policy: &SkillPolicy) -> Result<Value> {
     let mcp_on = ctx.app.config.mcp_server_support;
 
     let mut entries = Vec::new();
-    for name in paths::list_skills() {
+    for name in paths::list_visible_skills(ctx.app.config.visible_skills.as_deref()) {
         if !policy.allows(&name) {
             continue;
         }
@@ -193,7 +193,10 @@ async fn handle_load(
     }
 
     if let Err(e) = ctx.refresh_tool_scope(create_abort_signal()).await {
-        let _ = ctx.skill_registry.unload(name);
+        if let Err(unload_err) = ctx.skill_registry.unload(name) {
+            warn!("Failed to unload skill '{name}' during error recovery: {unload_err}");
+        }
+
         return Ok(json!({
             "error": format!("Loaded skill '{name}' but failed to refresh tool scope: {e}")
         }));
@@ -212,13 +215,20 @@ async fn handle_unload(ctx: &mut RequestContext, args: &Value) -> Result<Value>
         _ => return Ok(json!({"error": "name is required"})),
     };
 
+    if let Err(e) = paths::validate_skill_name(name) {
+        return Ok(json!({"error": e.to_string()}));
+    }
+
     let skill = match ctx.skill_registry.unload(name) {
         Ok(s) => s,
         Err(e) => return Ok(json!({"error": e.to_string()})),
     };
 
     if let Err(e) = ctx.refresh_tool_scope(create_abort_signal()).await {
-        let _ = ctx.skill_registry.insert(skill);
+        if let Err(insert_err) = ctx.skill_registry.insert(skill) {
+            warn!("Failed to restore skill '{name}' after unload recovery: {insert_err}");
+        }
+
         return Ok(json!({
             "error": format!(
                 "Unloaded skill '{name}' but failed to refresh tool scope; restored: {e}"

src/graph/validator.rs

@@ -191,6 +191,33 @@ impl GraphValidator {
     }
 
     fn validate_llm_skills(&self, graph: &Graph, result: &mut ValidationResult) {
+        let visible_skills = self
+            .agent_ctx
+            .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),
+        };
+
+        if let Some(graph_skills) = &graph.enabled_skills {
+            for name in graph_skills {
+                if name.trim().is_empty() {
+                    result.error(ValidationError::new(
+                        "graph 'enabled_skills' contains an empty skill name",
+                    ));
+                    continue;
+                }
+
+                if !is_visible(name) {
+                    result.error(ValidationError::new(format!(
+                        "graph 'enabled_skills' references '{name}' which is not in global 'visible_skills'"
+                    )));
+                }
+            }
+        }
+
         for (node_id, node) in &graph.nodes {
             let NodeType::Llm(llm) = &node.node_type else {
                 continue;
@@ -207,6 +234,17 @@ impl GraphValidator {
                     ));
                     continue;
                 }
+
+                if !is_visible(name) {
+                    result.error(ValidationError::with_node(
+                        node_id,
+                        format!(
+                            "llm node 'enabled_skills' references '{name}' which is not in global 'visible_skills'"
+                        ),
+                    ));
+                    continue;
+                }
+
                 if let Some(graph_skills) = &graph.enabled_skills
                     && !graph_skills.iter().any(|g| g == name)
                 {

src/vault/mod.rs

@@ -17,6 +17,7 @@ use gman::providers::SecretProvider;
 use gman::providers::SupportedProvider;
 use gman::providers::local::LocalProvider;
 use inquire::{Password, PasswordDisplayMode, required};
+use log::warn;
 use serde_yaml::Value;
 use std::sync::{Arc, LazyLock};
 use tokio::runtime::Handle;
@@ -228,7 +229,9 @@ impl Vault {
                     .await
                     .with_context(|| "vault read probe failed")?;
                 if got != PROBE_VALUE {
-                    let _ = self.provider_ref().delete_secret(&probe_key).await;
+                    if let Err(cleanup_err) = self.provider_ref().delete_secret(&probe_key).await {
+                        warn!("vault probe cleanup failed for key '{probe_key}': {cleanup_err}");
+                    }
                     bail!("vault read probe returned an unexpected value");
                 }
 

src/vault/utils.rs

@@ -13,7 +13,7 @@ use gman::providers::one_password::OnePasswordProvider;
 use indoc::formatdoc;
 use inquire::validator::Validation;
 use inquire::{Confirm, Password, PasswordDisplayMode, Select, Text, min_length, required};
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use std::process::Command;
 
 pub fn ensure_password_file_initialized(local_provider: &mut LocalProvider) -> Result<()> {
@@ -91,6 +91,7 @@ pub fn create_vault_password_file(vault: &mut Vault) -> Result<()> {
         match password {
             Ok(pw) => {
                 std::fs::write(&vault_password_file, pw.as_bytes())?;
+                set_password_file_permissions(&vault_password_file)?;
                 println!(
                     "✓ Password file '{}' updated.",
                     vault_password_file.display()
@@ -162,6 +163,7 @@ pub fn create_vault_password_file(vault: &mut Vault) -> Result<()> {
         match password {
             Ok(pw) => {
                 std::fs::write(&password_file, pw.as_bytes())?;
+                set_password_file_permissions(&password_file)?;
                 local_provider.password_file = Some(password_file);
                 println!(
                     "✓ Password file '{}' created.",
@@ -406,3 +408,19 @@ pub fn interpolate_secrets(content: &str, vault: &Vault) -> Result<(String, Vec<
 
     Ok((parsed_content, missing_secrets))
 }
+
+#[cfg(unix)]
+fn set_password_file_permissions(path: &Path) -> Result<()> {
+    use std::os::unix::fs::PermissionsExt;
+    std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)).map_err(|e| {
+        anyhow!(
+            "Failed to set 0600 permissions on '{}': {e}",
+            path.display()
+        )
+    })
+}
+
+#[cfg(not(unix))]
+fn set_password_file_permissions(_path: &Path) -> Result<()> {
+    Ok(())
+}