diff --git a/assets/agents/coder/scripts/route_review_result.sh b/assets/agents/coder/scripts/route_review_result.sh index de9b80f..c3dc7ce 100755 --- a/assets/agents/coder/scripts/route_review_result.sh +++ b/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 diff --git a/assets/functions/tools/fs_grep.sh b/assets/functions/tools/fs_grep.sh index 5b44969..dc37216 100644 --- a/assets/functions/tools/fs_grep.sh +++ b/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' diff --git a/src/config/paths.rs b/src/config/paths.rs index 125fdf9..fa26cfb 100644 --- a/src/config/paths.rs +++ b/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 { + 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 c178d4a..daf9131 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -1737,6 +1737,18 @@ impl RequestContext { "enabled_skills" => { 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()); + 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}'."); diff --git a/src/function/skill.rs b/src/function/skill.rs index a53776c..ceab466 100644 --- a/src/function/skill.rs +++ b/src/function/skill.rs @@ -105,7 +105,7 @@ fn handle_list(ctx: &RequestContext, policy: &SkillPolicy) -> Result { 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 _ => 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}" diff --git a/src/graph/validator.rs b/src/graph/validator.rs index c209c29..60f5e4f 100644 --- a/src/graph/validator.rs +++ b/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) { diff --git a/src/vault/mod.rs b/src/vault/mod.rs index 8eda382..1ba32a7 100644 --- a/src/vault/mod.rs +++ b/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"); } diff --git a/src/vault/utils.rs b/src/vault/utils.rs index 94a0b97..f5aafe1 100644 --- a/src/vault/utils.rs +++ b/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(()) +}