5 Commits

7 changed files with 107 additions and 19 deletions
+52
View File
@@ -80,6 +80,19 @@ pub fn skill_file(name: &str) -> PathBuf {
skill_dir(name).join("SKILL.md") skill_dir(name).join("SKILL.md")
} }
pub fn validate_skill_name(name: &str) -> Result<()> {
if name.is_empty() {
bail!("Skill name cannot be empty");
}
if !name
.chars()
.all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_')
{
bail!("Invalid skill name '{name}': only letters, digits, '-', and '_' are allowed");
}
Ok(())
}
pub fn macros_dir() -> PathBuf { pub fn macros_dir() -> PathBuf {
match env::var(get_env_name("macros_dir")) { match env::var(get_env_name("macros_dir")) {
Ok(value) => PathBuf::from(value), Ok(value) => PathBuf::from(value),
@@ -286,3 +299,42 @@ pub fn local_models_override() -> Result<Vec<ProviderModels>> {
} }
Ok(models_override.list) Ok(models_override.list)
} }
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn validate_skill_name_accepts_alphanumerics_and_dashes() {
assert!(validate_skill_name("git-master").is_ok());
assert!(validate_skill_name("code_review").is_ok());
assert!(validate_skill_name("Skill1").is_ok());
}
#[test]
fn validate_skill_name_rejects_empty() {
let err = validate_skill_name("").unwrap_err();
assert!(err.to_string().contains("cannot be empty"));
}
#[test]
fn validate_skill_name_rejects_path_traversal() {
for bad in ["../escape", "..", "foo/bar", "foo\\bar", "./hidden"] {
let err = validate_skill_name(bad).unwrap_err();
assert!(
err.to_string().contains("Invalid skill name"),
"expected rejection for {bad:?}, got: {err}"
);
}
}
#[test]
fn validate_skill_name_rejects_other_special_chars() {
for bad in ["with space", "null\0byte", "weird?char", "dot.name"] {
assert!(
validate_skill_name(bad).is_err(),
"expected rejection for {bad:?}"
);
}
}
}
+11 -1
View File
@@ -627,7 +627,15 @@ impl RequestContext {
} }
} }
self.skill_registry.effective_role(&role) match SkillPolicy::effective(
app,
self.role.as_ref(),
self.agent.as_ref(),
self.session.as_ref(),
) {
Ok(policy) => self.skill_registry.effective_role(&role, &policy),
Err(_) => role,
}
} }
pub fn auto_continue_config(&self) -> AutoContinueConfig { pub fn auto_continue_config(&self) -> AutoContinueConfig {
@@ -2576,6 +2584,7 @@ impl RequestContext {
} }
pub fn upsert_skill(&self, app: &AppConfig, name: &str) -> Result<()> { pub fn upsert_skill(&self, app: &AppConfig, name: &str) -> Result<()> {
paths::validate_skill_name(name)?;
let path = paths::skill_file(name); let path = paths::skill_file(name);
ensure_parent_exists(&path)?; ensure_parent_exists(&path)?;
let is_new = !path.exists(); let is_new = !path.exists();
@@ -2594,6 +2603,7 @@ impl RequestContext {
} }
pub async fn load_skill_repl(&mut self, name: &str, abort_signal: AbortSignal) -> Result<()> { pub async fn load_skill_repl(&mut self, name: &str, abort_signal: AbortSignal) -> Result<()> {
paths::validate_skill_name(name)?;
if !self.app.config.function_calling_support { if !self.app.config.function_calling_support {
bail!( bail!(
"Skills require function calling, which is disabled. Enable function calling in your config then try again." "Skills require function calling, which is disabled. Enable function calling in your config then try again."
+1
View File
@@ -116,6 +116,7 @@ impl Skill {
} }
pub fn load(name: &str) -> Result<Self> { pub fn load(name: &str) -> Result<Self> {
paths::validate_skill_name(name)?;
let path = paths::skill_file(name); let path = paths::skill_file(name);
let content = read_to_string(&path) let content = read_to_string(&path)
.with_context(|| format!("Failed to read skill '{name}' at {}", path.display()))?; .with_context(|| format!("Failed to read skill '{name}' at {}", path.display()))?;
+24 -12
View File
@@ -1,5 +1,6 @@
use super::role::{Role, RoleLike}; use super::role::{Role, RoleLike};
use super::skill::Skill; use super::skill::Skill;
use super::skill_policy::SkillPolicy;
use anyhow::{Result, bail}; use anyhow::{Result, bail};
use indexmap::IndexMap; use indexmap::IndexMap;
@@ -58,8 +59,8 @@ impl SkillRegistry {
self.loaded.retain(|_, skill| !skill.auto_unload()); self.loaded.retain(|_, skill| !skill.auto_unload());
} }
pub fn effective_role(&self, base: &Role) -> Role { pub fn effective_role(&self, base: &Role, policy: &SkillPolicy) -> Role {
if self.loaded.is_empty() { if !policy.skills_enabled || self.loaded.is_empty() {
return base.clone(); return base.clone();
} }
@@ -78,7 +79,10 @@ impl SkillRegistry {
.map(|v| v.into_iter().collect()) .map(|v| v.into_iter().collect())
.unwrap_or_default(); .unwrap_or_default();
for (_, skill) in &self.loaded { for (name, skill) in &self.loaded {
if !policy.allows(name) {
continue;
}
if let Some(skill_tools) = skill.enabled_tools() { if let Some(skill_tools) = skill.enabled_tools() {
tools.extend(skill_tools.iter().cloned()); tools.extend(skill_tools.iter().cloned());
} }
@@ -113,6 +117,14 @@ impl SkillRegistry {
fn insert_for_test(&mut self, skill: Skill) { fn insert_for_test(&mut self, skill: Skill) {
self.loaded.insert(skill.name().to_string(), skill); self.loaded.insert(skill.name().to_string(), skill);
} }
fn effective_role_for_test(&self, base: &Role) -> Role {
let policy = SkillPolicy {
skills_enabled: true,
enabled: self.loaded.keys().cloned().collect(),
};
self.effective_role(base, &policy)
}
} }
#[cfg(test)] #[cfg(test)]
@@ -133,7 +145,7 @@ mod tests {
let base = Role::new("test", "You are a helper"); let base = Role::new("test", "You are a helper");
let registry = SkillRegistry::default(); let registry = SkillRegistry::default();
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
assert_eq!(effective.prompt(), base.prompt()); assert_eq!(effective.prompt(), base.prompt());
} }
@@ -144,7 +156,7 @@ mod tests {
registry.insert_for_test(make_skill("git-master", "description: D", "Git knowledge")); registry.insert_for_test(make_skill("git-master", "description: D", "Git knowledge"));
let base = Role::new("test", "You are a helper"); let base = Role::new("test", "You are a helper");
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
assert_eq!(effective.prompt(), "You are a helper\n\nGit knowledge"); assert_eq!(effective.prompt(), "You are a helper\n\nGit knowledge");
} }
@@ -156,7 +168,7 @@ mod tests {
registry.insert_for_test(make_skill("b", "", "Beta body")); registry.insert_for_test(make_skill("b", "", "Beta body"));
let base = Role::new("test", "Base"); let base = Role::new("test", "Base");
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
assert_eq!(effective.prompt(), "Base\n\nAlpha body\n\nBeta body"); assert_eq!(effective.prompt(), "Base\n\nAlpha body\n\nBeta body");
} }
@@ -168,7 +180,7 @@ mod tests {
registry.insert_for_test(make_skill("b", "", "Beta")); registry.insert_for_test(make_skill("b", "", "Beta"));
let base = Role::new("test", ""); let base = Role::new("test", "");
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
assert_eq!(effective.prompt(), "Alpha\n\nBeta"); assert_eq!(effective.prompt(), "Alpha\n\nBeta");
} }
@@ -183,7 +195,7 @@ mod tests {
)); ));
let base = Role::new("test", "Process: __INPUT__"); let base = Role::new("test", "Process: __INPUT__");
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
assert_eq!(effective.prompt(), "Process: __INPUT__"); assert_eq!(effective.prompt(), "Process: __INPUT__");
let tools = effective.enabled_tools().expect("tools set by skill"); let tools = effective.enabled_tools().expect("tools set by skill");
@@ -196,7 +208,7 @@ mod tests {
registry.insert_for_test(make_skill("knowledge", "enabled_tools: fs", "")); registry.insert_for_test(make_skill("knowledge", "enabled_tools: fs", ""));
let base = Role::new("test", "Base"); let base = Role::new("test", "Base");
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
assert_eq!(effective.prompt(), "Base"); assert_eq!(effective.prompt(), "Base");
} }
@@ -218,7 +230,7 @@ mod tests {
let mut base = Role::new("test", "body"); let mut base = Role::new("test", "body");
base.set_enabled_tools(Some(vec!["web_search".to_string()])); base.set_enabled_tools(Some(vec!["web_search".to_string()]));
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
let tools_vec = effective.enabled_tools().unwrap(); let tools_vec = effective.enabled_tools().unwrap();
let tools: BTreeSet<&str> = tools_vec.iter().map(|s| s.as_str()).collect(); let tools: BTreeSet<&str> = tools_vec.iter().map(|s| s.as_str()).collect();
@@ -235,7 +247,7 @@ mod tests {
registry.insert_for_test(make_skill("knowledge", "", "Pure knowledge")); registry.insert_for_test(make_skill("knowledge", "", "Pure knowledge"));
let base = Role::new("test", "Base"); let base = Role::new("test", "Base");
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
assert!(effective.enabled_tools().is_none()); assert!(effective.enabled_tools().is_none());
assert!(effective.enabled_mcp_servers().is_none()); assert!(effective.enabled_mcp_servers().is_none());
@@ -248,7 +260,7 @@ mod tests {
let mut base = Role::new("test", "Base"); let mut base = Role::new("test", "Base");
base.set_enabled_tools(Some(Vec::new())); base.set_enabled_tools(Some(Vec::new()));
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
assert_eq!(effective.enabled_tools().as_deref(), Some([].as_slice())); assert_eq!(effective.enabled_tools().as_deref(), Some([].as_slice()));
} }
+10 -2
View File
@@ -2,7 +2,7 @@ use super::state::StateManager;
use super::structured; use super::structured;
use super::types::LlmNode; use super::types::LlmNode;
use crate::client::{Model, ModelType, call_chat_completions}; use crate::client::{Model, ModelType, call_chat_completions};
use crate::config::{Input, RequestContext, Role, RoleLike}; use crate::config::{Input, RequestContext, Role, RoleLike, SkillPolicy};
use crate::utils::create_abort_signal; use crate::utils::create_abort_signal;
use anyhow::{Context, Error, Result, anyhow, bail}; use anyhow::{Context, Error, Result, anyhow, bail};
use serde_json::Value; use serde_json::Value;
@@ -115,7 +115,15 @@ async fn run(
let saved_agent_skill_state = swap_in_node_skill_policy(node, parent_ctx); let saved_agent_skill_state = swap_in_node_skill_policy(node, parent_ctx);
let composed_role = parent_ctx.skill_registry.effective_role(&role); let composed_role = match SkillPolicy::effective(
&parent_ctx.app.config,
parent_ctx.role.as_ref(),
parent_ctx.agent.as_ref(),
parent_ctx.session.as_ref(),
) {
Ok(policy) => parent_ctx.skill_registry.effective_role(&role, &policy),
Err(_) => role,
};
let saved_role = parent_ctx.role.clone(); let saved_role = parent_ctx.role.clone();
parent_ctx.role = Some(composed_role); parent_ctx.role = Some(composed_role);
+5 -4
View File
@@ -16,6 +16,7 @@ use gman::providers::local::LocalProvider;
use inquire::{Password, PasswordDisplayMode, required}; use inquire::{Password, PasswordDisplayMode, required};
use std::sync::{Arc, LazyLock}; use std::sync::{Arc, LazyLock};
use tokio::runtime::Handle; use tokio::runtime::Handle;
use uuid::Uuid;
pub static SECRET_RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"\{\{(.+)}}").unwrap()); pub static SECRET_RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"\{\{(.+)}}").unwrap());
@@ -175,22 +176,22 @@ impl Vault {
} }
pub fn validate_round_trip(&self) -> Result<()> { pub fn validate_round_trip(&self) -> Result<()> {
const PROBE_KEY: &str = "__coyote_setup_probe__";
const PROBE_VALUE: &str = "ok"; const PROBE_VALUE: &str = "ok";
let probe_key = format!("__coyote_setup_probe_{}__", Uuid::new_v4().simple());
let h = Handle::current(); let h = Handle::current();
let result: Result<()> = tokio::task::block_in_place(|| { let result: Result<()> = tokio::task::block_in_place(|| {
h.block_on(async { h.block_on(async {
self.provider_ref() self.provider_ref()
.set_secret(PROBE_KEY, PROBE_VALUE) .set_secret(&probe_key, PROBE_VALUE)
.await .await
.with_context(|| "vault write probe failed")?; .with_context(|| "vault write probe failed")?;
let got = self let got = self
.provider_ref() .provider_ref()
.get_secret(PROBE_KEY) .get_secret(&probe_key)
.await .await
.with_context(|| "vault read probe failed")?; .with_context(|| "vault read probe failed")?;
let _ = self.provider_ref().delete_secret(PROBE_KEY).await; let _ = self.provider_ref().delete_secret(&probe_key).await;
if got != PROBE_VALUE { if got != PROBE_VALUE {
bail!("vault read probe returned an unexpected value"); bail!("vault read probe returned an unexpected value");
} }
+4
View File
@@ -364,6 +364,10 @@ pub fn interpolate_secrets(content: &str, vault: &Vault) -> Result<(String, Vec<
SECRET_RE SECRET_RE
.replace_all(line, |caps: &fancy_regex::Captures<'_>| { .replace_all(line, |caps: &fancy_regex::Captures<'_>| {
if fatal_error.is_some() {
return String::new();
}
let name = caps[1].trim(); let name = caps[1].trim();
match vault.get_secret(name, false) { match vault.get_secret(name, false) {
Ok(s) => s, Ok(s) => s,