From af3d1a106afcd535ea24a096c2722c8bed8f91b4 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Thu, 4 Jun 2026 15:32:31 -0600 Subject: [PATCH] test: improve vault password file errors by propagating up --- src/cli/completer.rs | 17 ++-- src/cli/mod.rs | 28 +++++++ src/config/app_state.rs | 2 +- src/config/mod.rs | 2 +- src/graph/llm.rs | 14 ++-- src/graph/validator.rs | 12 +++ src/main.rs | 14 ++-- src/vault/mod.rs | 7 +- src/vault/utils.rs | 176 +++++++++++++++++++++++++++++++++++++++- 9 files changed, 243 insertions(+), 29 deletions(-) diff --git a/src/cli/completer.rs b/src/cli/completer.rs index 31f5f3b..0a6e732 100644 --- a/src/cli/completer.rs +++ b/src/cli/completer.rs @@ -137,13 +137,16 @@ pub(super) fn session_completer(current: &OsStr) -> Vec { pub(super) fn secrets_completer(current: &OsStr) -> Vec { let cur = current.to_string_lossy(); match load_app_config_for_completion() { - Ok(app_config) => Vault::init(&app_config) - .list_secrets(false) - .unwrap_or_default() - .into_iter() - .filter(|s| s.starts_with(&*cur)) - .map(CompletionCandidate::new) - .collect(), + Ok(app_config) => match Vault::init(&app_config) { + Ok(vault) => vault + .list_secrets(false) + .unwrap_or_default() + .into_iter() + .filter(|s| s.starts_with(&*cur)) + .map(CompletionCandidate::new) + .collect(), + Err(_) => vec![], + }, Err(_) => vec![], } } diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 00c6bd3..ea3c234 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -10,6 +10,7 @@ use clap::ValueHint; use clap::{Parser, crate_authors, crate_description, crate_version}; use clap_complete::ArgValueCompleter; use is_terminal::IsTerminal; +use std::collections::HashSet; use std::io::{Read, stdin}; #[derive(Parser, Debug)] @@ -163,6 +164,18 @@ pub struct Cli { } impl Cli { + pub fn skills(&self) -> Vec { + let mut seen = HashSet::new(); + let mut out = Vec::with_capacity(self.skill.len()); + for name in &self.skill { + if seen.insert(name.clone()) { + out.push(name.clone()); + } + } + + out + } + pub fn text(&self) -> Result> { let mut stdin_text = String::new(); if !stdin().is_terminal() { @@ -323,6 +336,21 @@ mod tests { ); } + #[test] + fn skills_method_dedupes_preserving_first_occurrence() { + let cli = parse(&[ + "--skill", "alpha", "--skill", "beta", "--skill", "alpha", "--skill", "gamma", + "--skill", "beta", + ]); + + assert_eq!(cli.skills(), vec!["alpha", "beta", "gamma"]); + } + + #[test] + fn skills_method_returns_empty_when_no_flags() { + assert!(parse(&[]).skills().is_empty()); + } + #[test] fn parse_file_flag_single() { let cli = parse(&["-f", "file.txt", "question"]); diff --git a/src/config/app_state.rs b/src/config/app_state.rs index d3e8332..c1f4769 100644 --- a/src/config/app_state.rs +++ b/src/config/app_state.rs @@ -43,7 +43,7 @@ impl AppState { start_mcp_servers: bool, abort_signal: AbortSignal, ) -> Result { - let vault = Arc::new(Vault::init(&config)); + let vault = Arc::new(Vault::init(&config)?); let mcp_registry = McpRegistry::init( log_path, diff --git a/src/config/mod.rs b/src/config/mod.rs index 9d4fa8c..3860f43 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -490,7 +490,7 @@ impl Config { secrets_provider: config.secrets_provider.clone(), ..AppConfig::default() }; - let vault = Vault::init(&bootstrap_app); + let vault = Vault::init(&bootstrap_app)?; let (parsed_config, missing_secrets) = interpolate_secrets(&content, &vault)?; if !missing_secrets.is_empty() && !info_flag { debug!( diff --git a/src/graph/llm.rs b/src/graph/llm.rs index fa26c7f..1e09b88 100644 --- a/src/graph/llm.rs +++ b/src/graph/llm.rs @@ -129,13 +129,13 @@ async fn run( } }; - if policy.skills_enabled - && node - .tools - .as_deref() - .map(|t| !t.is_empty()) - .unwrap_or(false) - { + let node_has_enabled_skills = node + .enabled_skills + .as_deref() + .map(|s| !s.is_empty()) + .unwrap_or(false); + + if policy.skills_enabled && node_has_enabled_skills { let mut tools = role.enabled_tools().map(|v| v.to_vec()).unwrap_or_default(); for decl in skill_function_declarations() { if !tools.contains(&decl.name) { diff --git a/src/graph/validator.rs b/src/graph/validator.rs index b755a69..1de32b8 100644 --- a/src/graph/validator.rs +++ b/src/graph/validator.rs @@ -228,6 +228,12 @@ impl GraphValidator { )); continue; } + if let Err(e) = paths::validate_skill_name(name) { + result.error(ValidationError::new(format!( + "graph 'enabled_skills' contains an invalid skill name: '{name}': {e}" + ))); + continue; + } if let Some(reason) = check_visibility(name) { result.error(ValidationError::new(format!( "graph 'enabled_skills': {reason}" @@ -252,6 +258,12 @@ impl GraphValidator { )); continue; } + if let Err(e) = paths::validate_skill_name(name) { + result.error(ValidationError::new(format!( + "llm node 'enabled_skills' contains an invalid skill name: '{name}': {e}" + ))); + continue; + } if let Some(reason) = check_visibility(name) { result.error(ValidationError::with_node( node_id, diff --git a/src/main.rs b/src/main.rs index bf593b0..c130a88 100644 --- a/src/main.rs +++ b/src/main.rs @@ -113,7 +113,7 @@ async fn main() -> Result<()> { if vault_flags { let cfg = Config::load_with_interpolation(true).await?; let app_config = AppConfig::from_config(cfg)?; - let vault = Vault::init(&app_config); + let vault = Vault::init(&app_config)?; return Vault::handle_vault_flags(cli, &vault); } @@ -197,17 +197,17 @@ async fn run( println!("{skills}"); return Ok(()); } - if cli.skill.len() == 1 { - let name = &cli.skill[0]; + let skills = cli.skills(); + if skills.len() == 1 { + let name = &skills[0]; paths::validate_skill_name(name)?; if !paths::has_skill(name) { let app = Arc::clone(&ctx.app.config); ctx.upsert_skill(app.as_ref(), name)?; return Ok(()); } - } - if cli.skill.len() > 1 { - for name in &cli.skill { + } else if skills.len() > 1 { + for name in &skills { paths::validate_skill_name(name)?; if !paths::has_skill(name) { bail!("Skill '{name}' is not installed"); @@ -327,7 +327,7 @@ async fn run( .await?; } - for name in &cli.skill { + for name in &cli.skills() { ctx.load_skill_repl(name, abort_signal.clone()).await?; } diff --git a/src/vault/mod.rs b/src/vault/mod.rs index 1ba32a7..e267cdb 100644 --- a/src/vault/mod.rs +++ b/src/vault/mod.rs @@ -80,7 +80,7 @@ impl Vault { } } - pub fn init(config: &AppConfig) -> Self { + pub fn init(config: &AppConfig) -> Result { let mut provider = match &config.secrets_provider { Some(p) => p.clone(), None => SupportedProvider::Local { @@ -92,11 +92,10 @@ impl Vault { }; if let SupportedProvider::Local { provider_def } = &mut provider { - ensure_password_file_initialized(provider_def) - .expect("Failed to initialize password file"); + ensure_password_file_initialized(provider_def)?; } - Self { provider } + Ok(Self { provider }) } pub fn local_password_file(&self) -> Result { diff --git a/src/vault/utils.rs b/src/vault/utils.rs index 0a21050..dfd9c8e 100644 --- a/src/vault/utils.rs +++ b/src/vault/utils.rs @@ -355,6 +355,19 @@ fn required_cli_preflight(label: &str, cli: &str, install_url: &str) { } pub fn interpolate_secrets(content: &str, vault: &Vault) -> Result<(String, Vec)> { + interpolate_secrets_with(content, vault.auth_hint(), |name| { + vault.get_secret(name, false) + }) +} + +fn interpolate_secrets_with( + content: &str, + auth_hint: Option<&'static str>, + mut get_secret: F, +) -> Result<(String, Vec)> +where + F: FnMut(&str) -> Result, +{ let mut missing_secrets = vec![]; let mut fatal_error: Option = None; @@ -372,7 +385,7 @@ pub fn interpolate_secrets(content: &str, vault: &Vault) -> Result<(String, Vec< } let name = caps[1].trim(); - match vault.get_secret(name, false) { + match get_secret(name) { Ok(s) => s, Err(e) => match e.downcast_ref::() { Some(SecretError::NotFound { .. }) => { @@ -382,7 +395,7 @@ pub fn interpolate_secrets(content: &str, vault: &Vault) -> Result<(String, Vec< Some(SecretError::AuthFailed { .. }) => { let base = format!("Failed to fetch secret '{name}' from vault: {e}"); - let msg = match vault.auth_hint() { + let msg = match auth_hint { Some(hint) => format!("{base}\n\nHint: {hint}"), None => base, }; @@ -425,3 +438,162 @@ fn set_password_file_permissions(path: &Path) -> Result<()> { fn set_password_file_permissions(_path: &Path) -> Result<()> { Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use anyhow::Error; + use std::cell::RefCell; + + fn not_found(name: &str) -> Error { + Error::new(SecretError::NotFound { + key: name.to_string(), + provider: "test", + }) + } + + fn auth_failed() -> Error { + Error::new(SecretError::AuthFailed { + provider: "test", + source: anyhow!("auth failure"), + }) + } + + struct Calls(RefCell>); + + impl Calls { + fn new() -> Self { + Self(RefCell::new(Vec::new())) + } + + fn record(&self, name: &str) { + self.0.borrow_mut().push(name.to_string()); + } + + fn snapshot(&self) -> Vec { + self.0.borrow().clone() + } + } + + #[test] + fn interpolates_single_secret_per_line() { + let (out, missing) = + interpolate_secrets_with("api_key={{API_KEY}}", None, |name| match name { + "API_KEY" => Ok("sk-12345".to_string()), + other => panic!("unexpected lookup: {other}"), + }) + .unwrap(); + + assert_eq!(out, "api_key=sk-12345"); + assert!(missing.is_empty()); + } + + #[test] + fn regex_matches_each_secret_independently_when_one_per_line() { + let calls = Calls::new(); + let (out, missing) = interpolate_secrets_with("{{ONE}}\nmiddle\n{{TWO}}", None, |name| { + calls.record(name); + Ok(name.to_lowercase()) + }) + .unwrap(); + + assert_eq!(calls.snapshot(), vec!["ONE".to_string(), "TWO".to_string()]); + assert_eq!(out, "one\nmiddle\ntwo"); + assert!(missing.is_empty()); + } + + #[test] + fn skips_comment_lines() { + let calls = Calls::new(); + + let (out, missing) = + interpolate_secrets_with("# api_key={{NEVER_FETCHED}}\nreal={{S}}", None, |name| { + calls.record(name); + Ok("v".to_string()) + }) + .unwrap(); + + assert_eq!(out, "# api_key={{NEVER_FETCHED}}\nreal=v"); + assert!(missing.is_empty()); + assert_eq!(calls.snapshot(), vec!["S".to_string()]); + } + + #[test] + fn missing_secrets_become_empty_strings_and_are_reported() { + let (out, missing) = interpolate_secrets_with( + "a={{HAVE}}\nb={{MISSING_1}}\nc={{MISSING_2}}", + None, + |name| match name { + "HAVE" => Ok("present".to_string()), + missing => Err(not_found(missing)), + }, + ) + .unwrap(); + + assert_eq!(out, "a=present\nb=\nc="); + assert_eq!( + missing, + vec!["MISSING_1".to_string(), "MISSING_2".to_string()] + ); + } + + #[test] + fn fatal_failure_short_circuits_remaining_lines() { + let calls = Calls::new(); + let result = + interpolate_secrets_with("a={{S1}}\nb={{S2}}\nc={{S3}}\nd={{S4}}", None, |name| { + calls.record(name); + match name { + "S1" => Ok("first".to_string()), + "S2" => Err(auth_failed()), + other => Ok(format!("late-{other}")), + } + }); + + let err = result.unwrap_err().to_string(); + assert!( + err.contains("S2"), + "error should name the offending secret, got: {err}" + ); + assert_eq!( + calls.snapshot(), + vec!["S1".to_string(), "S2".to_string()], + "lookups must stop at the failing secret - S3 and S4 should never be fetched" + ); + } + + #[test] + fn auth_failure_appends_hint_when_provided() { + let result = interpolate_secrets_with( + "k={{K}}", + Some("run `coyote --authenticate` to reauth"), + |_| Err(auth_failed()), + ); + + let err = result.unwrap_err().to_string(); + + assert!(err.contains("Hint:"), "expected hint in error, got: {err}"); + assert!( + err.contains("coyote --authenticate"), + "expected hint contents, got: {err}" + ); + } + + #[test] + fn regex_greedy_capture_collapses_multi_secret_line_into_single_lookup() { + let calls = Calls::new(); + let (out, missing) = interpolate_secrets_with("url={{URL}} key={{KEY}}", None, |name| { + calls.record(name); + Err(not_found(name)) + }) + .unwrap(); + + assert_eq!( + calls.snapshot(), + vec!["URL}} key={{KEY".to_string()], + "greedy regex spans first {{ to last }}, collapsing the whole line into one bogus lookup" + ); + assert_eq!(out, "url="); + assert_eq!(missing, vec!["URL}} key={{KEY".to_string()]); + } +}