diff --git a/.github/setup-workspace.sh b/.github/setup-workspace.sh index 7322a48..98e4800 100644 --- a/.github/setup-workspace.sh +++ b/.github/setup-workspace.sh @@ -9,6 +9,7 @@ echo "Replacing path dependencies with crates.io versions..." # core/Cargo.toml — internal crate deps sed -i.bak \ + -e 's|a3s-common = { version = "0.1.1", path = "../../common" }|a3s-common = "0.1.1"|' \ -e 's|a3s-common = { version = "0.1", path = "../../common" }|a3s-common = "0.1.1"|' \ -e 's|a3s-memory = { version = "0.1.1", path = "../../memory" }|a3s-memory = "0.1.1"|' \ -e 's|a3s-lane = { version = "0.4", path = "../../lane" }|a3s-lane = "0.4"|' \ diff --git a/README.md b/README.md index e7a10ed..0ba2618 100644 --- a/README.md +++ b/README.md @@ -78,6 +78,13 @@ default_model = "anthropic/claude-sonnet-4-20250514" providers "anthropic" { apiKey = env("ANTHROPIC_API_KEY") + + models "claude-sonnet-4-20250514" { + limit = { + context = 200000 + output = 8192 + } + } } ``` @@ -1267,6 +1274,13 @@ default_model = "anthropic/claude-sonnet-4-20250514" providers "anthropic" { apiKey = env("ANTHROPIC_API_KEY") + + models "claude-sonnet-4-20250514" { + limit = { + context = 200000 + output = 8192 + } + } } skill_dirs = ["./skills"] @@ -1279,6 +1293,10 @@ ahp = { } ``` +Model token limits use the `limit = { context = ..., output = ... }` object as the canonical ACL shape. +The flat `maxTokens` and `contextTokens` fields are accepted only as deprecated +migration aliases and emit warnings. + --- ## Development diff --git a/core/Cargo.toml b/core/Cargo.toml index 990d803..e411588 100644 --- a/core/Cargo.toml +++ b/core/Cargo.toml @@ -13,7 +13,7 @@ path = "src/lib.rs" [dependencies] # Internal crates -a3s-common = { version = "0.1", path = "../../common" } +a3s-common = { version = "0.1.1", path = "../../common" } a3s-memory = { version = "0.1.1", path = "../../memory" } a3s-lane = { version = "0.4", path = "../../lane" } a3s-search = { version = "1.2.3", path = "../../search", default-features = false, features = ["lightpanda"] } diff --git a/core/skills/code-review.md b/core/skills/code-review.md index 4fd6928..38a2210 100644 --- a/core/skills/code-review.md +++ b/core/skills/code-review.md @@ -1,6 +1,7 @@ --- name: code-review description: Review code for best practices, bugs, and improvements +allowed-tools: "grep(*), glob(*), read(*)" kind: instruction tags: - review diff --git a/core/skills/code-search.md b/core/skills/code-search.md index 42aad48..e6d8e1a 100644 --- a/core/skills/code-search.md +++ b/core/skills/code-search.md @@ -1,6 +1,7 @@ --- name: code-search description: Search codebase for patterns, functions, or types +allowed-tools: "grep(*), glob(*), read(*)" kind: instruction tags: - search diff --git a/core/skills/explain-code.md b/core/skills/explain-code.md index 1d51066..162e564 100644 --- a/core/skills/explain-code.md +++ b/core/skills/explain-code.md @@ -1,6 +1,7 @@ --- name: explain-code description: Explain how code works in clear, simple terms +allowed-tools: "grep(*), glob(*), read(*)" kind: instruction tags: - explain diff --git a/core/skills/find-bugs.md b/core/skills/find-bugs.md index 94b0254..ccbeea6 100644 --- a/core/skills/find-bugs.md +++ b/core/skills/find-bugs.md @@ -1,6 +1,7 @@ --- name: find-bugs description: Identify potential bugs, vulnerabilities, and code smells +allowed-tools: "grep(*), glob(*), read(*)" kind: instruction tags: - bugs diff --git a/core/src/agent.rs b/core/src/agent.rs index c4b7cad..ee6c356 100644 --- a/core/src/agent.rs +++ b/core/src/agent.rs @@ -627,15 +627,13 @@ impl SessionCommand for ToolCommand { async fn execute(&self) -> Result { // Check skill-based tool permissions if let Some(registry) = &self.skill_registry { - let instruction_skills = registry.by_kind(crate::skills::SkillKind::Instruction); - // If there are instruction skills with tool restrictions, check permissions - let has_restrictions = instruction_skills.iter().any(|s| s.allowed_tools.is_some()); + let restricting_skills = registry.global_tool_restricting_skills(); - if has_restrictions { + if !restricting_skills.is_empty() { let mut allowed = false; - for skill in &instruction_skills { + for skill in &restricting_skills { if skill.is_tool_allowed(&self.tool_name) { allowed = true; break; diff --git a/core/src/config/loader.rs b/core/src/config/loader.rs index d84d6ea..4dddf53 100644 --- a/core/src/config/loader.rs +++ b/core/src/config/loader.rs @@ -52,6 +52,24 @@ fn acl_usize_attr(block: &a3s_acl::Block, keys: &[&str]) -> Option { } } +fn acl_u32(value: &a3s_acl::Value) -> Option { + match value { + a3s_acl::Value::Number(value) if *value >= 0.0 => { + Some((*value as usize).min(u32::MAX as usize) as u32) + } + _ => None, + } +} + +fn acl_object_u32_attr(value: &a3s_acl::Value, key: &str) -> Option { + match value { + a3s_acl::Value::Object(pairs) => pairs + .iter() + .find_map(|(candidate, value)| (candidate == key).then(|| acl_u32(value)).flatten()), + _ => None, + } +} + fn acl_path_list_attr(block: &a3s_acl::Block, keys: &[&str]) -> Option> { let value = acl_attr(block, keys)?; match value { @@ -276,6 +294,37 @@ impl CodeConfig { model.release_date = Some(release_date); } } + "maxTokens" => { + tracing::warn!( + provider = %provider.name, + model = %model.id, + field = "maxTokens", + "Flat ACL model token limit fields are deprecated; use limit = {{ output = ..., context = ... }}" + ); + if let Some(output) = acl_u32(value) { + model.limit.output = output; + } + } + "contextTokens" => { + tracing::warn!( + provider = %provider.name, + model = %model.id, + field = "contextTokens", + "Flat ACL model token limit fields are deprecated; use limit = {{ output = ..., context = ... }}" + ); + if let Some(context) = acl_u32(value) { + model.limit.context = context; + } + } + "limit" => { + if let Some(output) = acl_object_u32_attr(value, "output") { + model.limit.output = output; + } + if let Some(context) = acl_object_u32_attr(value, "context") + { + model.limit.context = context; + } + } _ => {} } } diff --git a/core/src/config/tests.rs b/core/src/config/tests.rs index e33ba64..1a1ed80 100644 --- a/core/src/config/tests.rs +++ b/core/src/config/tests.rs @@ -111,6 +111,42 @@ fn test_config_supports_acl_style_provider_labels() { assert!(config.providers[0].models[0].tool_call); } +#[test] +fn test_config_parses_acl_model_token_limits() { + let config = CodeConfig::from_acl( + r#" + default_model = "openai/glm-5.1" + + providers "openai" { + api_key = "sk-test" + base_url = "http://127.0.0.1:18051/v1" + + models "glm-5.1" { + name = "GLM 5.1" + limit = { + output = 4096 + context = 32768 + } + } + + models "flat-alias" { + maxTokens = 8192 + contextTokens = 65536 + } + } + "#, + ) + .unwrap(); + + let flat = &config.providers[0].models[0].limit; + assert_eq!(flat.output, 4096); + assert_eq!(flat.context, 32768); + + let flat_alias = &config.providers[0].models[1].limit; + assert_eq!(flat_alias.output, 8192); + assert_eq!(flat_alias.context, 65536); +} + #[test] fn test_config_builder() { let config = CodeConfig::new() diff --git a/core/src/safety_gate.rs b/core/src/safety_gate.rs index beb41f7..8cd4f96 100644 --- a/core/src/safety_gate.rs +++ b/core/src/safety_gate.rs @@ -103,13 +103,12 @@ impl<'a> ToolSafetyGate<'a> { fn check_skill_restrictions(&self, tool_name: &str) -> Option { let registry = self.config.skill_registry.as_ref()?; - let instruction_skills = registry.by_kind(crate::skills::SkillKind::Instruction); - let has_restrictions = instruction_skills.iter().any(|s| s.allowed_tools.is_some()); - if !has_restrictions { + let restricting_skills = registry.global_tool_restricting_skills(); + if restricting_skills.is_empty() { return None; } - let allowed = instruction_skills + let allowed = restricting_skills .iter() .any(|skill| skill.is_tool_allowed(tool_name)); if allowed { @@ -220,6 +219,31 @@ mod tests { )); } + #[tokio::test] + async fn builtin_skill_permissions_do_not_restrict_default_session_tools() { + let config = AgentConfig { + skill_registry: Some(Arc::new(SkillRegistry::with_builtins())), + permission_checker: Some(Arc::new(StaticPermission(PermissionDecision::Allow))), + ..Default::default() + }; + let gate = ToolSafetyGate::new(&config); + + let decision = gate + .decide(ToolGateInput { + tool_name: "write", + args: &json!({"file_path": "x"}), + pre_tool_block: None, + }) + .await; + + assert!(matches!( + decision, + ToolGateDecision::Execute { + reason: ToolGateApproval::PermissionAllow, + } + )); + } + #[tokio::test] async fn hook_block_denies_before_permission_allow() { let config = AgentConfig { diff --git a/core/src/skills/builtin.rs b/core/src/skills/builtin.rs index b110516..9741b3b 100644 --- a/core/src/skills/builtin.rs +++ b/core/src/skills/builtin.rs @@ -69,8 +69,9 @@ mod tests { let skill = code_search_skill(); assert_eq!(skill.name, "code-search"); assert!(skill.content.contains("Code Search")); - assert!(skill.allowed_tools.is_none()); - assert!(skill.is_tool_allowed("write")); + assert!(skill.allowed_tools.is_some()); + assert!(skill.is_tool_allowed("read")); + assert!(!skill.is_tool_allowed("write")); } #[test] diff --git a/core/src/skills/mod.rs b/core/src/skills/mod.rs index df2fb4f..9b40f94 100644 --- a/core/src/skills/mod.rs +++ b/core/src/skills/mod.rs @@ -33,7 +33,7 @@ pub use validator::{ DefaultSkillValidator, SkillValidationError, SkillValidator, ValidationErrorKind, }; -use serde::{Deserialize, Serialize}; +use serde::{de, Deserialize, Deserializer, Serialize}; use std::collections::HashSet; use std::path::Path; @@ -101,7 +101,11 @@ pub struct Skill { pub description: String, /// Allowed tools (Claude Code format: "Bash(pattern:*), read(*)") - #[serde(default, rename = "allowed-tools")] + #[serde( + default, + rename = "allowed-tools", + deserialize_with = "deserialize_allowed_tools" + )] pub allowed_tools: Option, /// Whether to disable model invocation @@ -183,24 +187,59 @@ impl Skill { return permissions; }; - // Parse comma-separated tool permissions - for part in allowed.split(',') { + // Parse Claude-style comma-separated permissions, plus legacy + // whitespace-only tool lists such as "Read Write Edit Bash". + // A single Bash(...) permission may itself contain spaces, so try the + // canonical single-rule form before falling back to legacy splitting. + let parts: Vec<&str> = if allowed.contains(',') { + allowed.split(',').collect() + } else if ToolPermission::parse(allowed).is_some() { + vec![allowed.as_str()] + } else { + let parts: Vec<&str> = allowed.split_whitespace().collect(); + if parts.len() > 1 { + tracing::warn!( + skill = %self.name, + allowed_tools = %allowed, + "Legacy whitespace-separated allowed-tools is deprecated; use comma-separated permissions such as Read(*), Write(*), Bash(*) or a YAML list" + ); + } + parts + }; + for part in parts { let part = part.trim(); + if part.is_empty() { + continue; + } if let Some(perm) = ToolPermission::parse(part) { permissions.insert(perm); + } else { + permissions.insert(ToolPermission { + tool: part.to_string(), + pattern: "*".to_string(), + }); } } permissions } + /// True when a skill still uses the legacy whitespace-only tool list. + pub fn uses_legacy_allowed_tools_syntax(&self) -> bool { + let Some(allowed) = &self.allowed_tools else { + return false; + }; + !allowed.contains(',') + && ToolPermission::parse(allowed).is_none() + && allowed.split_whitespace().count() > 1 + } + /// Check if a tool is allowed by this skill pub fn is_tool_allowed(&self, tool_name: &str) -> bool { let permissions = self.parse_allowed_tools(); if permissions.is_empty() { - // No restrictions = all tools allowed - return true; + return false; } // Check if any permission matches @@ -218,6 +257,34 @@ impl Skill { } } +fn deserialize_allowed_tools<'de, D>(deserializer: D) -> Result, D::Error> +where + D: Deserializer<'de>, +{ + let value = Option::::deserialize(deserializer)?; + match value { + None | Some(serde_yaml::Value::Null) => Ok(None), + Some(serde_yaml::Value::String(s)) => Ok(Some(s)), + Some(serde_yaml::Value::Sequence(items)) => { + let mut tools = Vec::new(); + for item in items { + match item { + serde_yaml::Value::String(s) => tools.push(s), + other => { + return Err(de::Error::custom(format!( + "allowed-tools list entries must be strings, got {other:?}" + ))); + } + } + } + Ok(Some(tools.join(", "))) + } + Some(other) => Err(de::Error::custom(format!( + "allowed-tools must be a string or a list of strings, got {other:?}" + ))), + } +} + #[cfg(test)] mod tests { use super::*; @@ -270,6 +337,73 @@ You are a test assistant. assert_eq!(permissions.len(), 3); } + #[test] + fn test_parse_legacy_whitespace_allowed_tools() { + let skill = Skill { + name: "test".to_string(), + description: "test".to_string(), + allowed_tools: Some("Read Write Edit Bash".to_string()), + disable_model_invocation: false, + kind: SkillKind::Instruction, + content: String::new(), + tags: Vec::new(), + version: None, + }; + + let permissions = skill.parse_allowed_tools(); + assert_eq!(permissions.len(), 4); + assert!(skill.uses_legacy_allowed_tools_syntax()); + assert!(permissions + .iter() + .any(|perm| perm.tool == "Bash" && perm.pattern == "*")); + } + + #[test] + fn test_parse_single_allowed_tool_with_spaces() { + let skill = Skill { + name: "test".to_string(), + description: "test".to_string(), + allowed_tools: Some("Bash(uv run skills analyze-ci:*)".to_string()), + disable_model_invocation: false, + kind: SkillKind::Instruction, + content: String::new(), + tags: Vec::new(), + version: None, + }; + + let permissions = skill.parse_allowed_tools(); + assert_eq!(permissions.len(), 1); + assert!(permissions + .iter() + .any(|perm| { perm.tool == "Bash" && perm.pattern == "uv run skills analyze-ci:*" })); + assert!(!skill.uses_legacy_allowed_tools_syntax()); + } + + #[test] + fn test_parse_allowed_tools_yaml_list() { + let content = r#"--- +name: test-skill +description: A test skill +allowed-tools: + - Read + - Write + - Bash(uv run skills analyze-ci:*) +--- +# Instructions +"#; + + let skill = Skill::parse(content).unwrap(); + assert_eq!( + skill.allowed_tools.as_deref(), + Some("Read, Write, Bash(uv run skills analyze-ci:*)") + ); + let permissions = skill.parse_allowed_tools(); + assert_eq!(permissions.len(), 3); + assert!(permissions + .iter() + .any(|perm| perm.tool == "Read" && perm.pattern == "*")); + } + #[test] fn test_is_tool_allowed() { let skill = Skill { @@ -287,4 +421,20 @@ You are a test assistant. assert!(skill.is_tool_allowed("grep")); assert!(!skill.is_tool_allowed("write")); } + + #[test] + fn test_omitted_allowed_tools_does_not_allow_tools() { + let skill = Skill { + name: "test".to_string(), + description: "test".to_string(), + allowed_tools: None, + disable_model_invocation: false, + kind: SkillKind::Instruction, + content: String::new(), + tags: Vec::new(), + version: None, + }; + + assert!(!skill.is_tool_allowed("read")); + } } diff --git a/core/src/skills/registry.rs b/core/src/skills/registry.rs index 0f52fa3..96a9529 100644 --- a/core/src/skills/registry.rs +++ b/core/src/skills/registry.rs @@ -4,9 +4,9 @@ //! Integrates with `SkillValidator` as a safety gate for externally loaded skills. use super::validator::SkillValidator; -use super::Skill; +use super::{Skill, SkillKind}; use anyhow::Context; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use std::sync::{Arc, RwLock}; @@ -16,6 +16,7 @@ use std::sync::{Arc, RwLock}; /// Optionally validates skills before registration. pub struct SkillRegistry { skills: Arc>>>, + builtin_names: Arc>>, validator: Arc>>>, } @@ -24,6 +25,7 @@ impl SkillRegistry { pub fn new() -> Self { Self { skills: Arc::new(RwLock::new(HashMap::new())), + builtin_names: Arc::new(RwLock::new(HashSet::new())), validator: Arc::new(RwLock::new(None)), } } @@ -33,7 +35,7 @@ impl SkillRegistry { let registry = Self::new(); for skill in super::builtin::builtin_skills() { // Built-in skills bypass validation - registry.register_unchecked(skill); + registry.register_builtin(skill); } registry } @@ -45,8 +47,10 @@ impl SkillRegistry { /// that session and delegated-agent registries keep the same safety policy. pub fn fork(&self) -> Self { let skills = self.skills.read().unwrap().clone(); + let builtin_names = self.builtin_names.read().unwrap().clone(); Self { skills: Arc::new(RwLock::new(skills)), + builtin_names: Arc::new(RwLock::new(builtin_names)), validator: Arc::new(RwLock::new(self.validator.read().unwrap().clone())), } } @@ -72,12 +76,22 @@ impl SkillRegistry { Ok(()) } - /// Register a skill without validation (for built-in skills) + /// Register a skill without validation. + /// + /// If this replaces a built-in skill with the same name, it is treated as an + /// external skill for global tool-restriction purposes. pub fn register_unchecked(&self, skill: Arc) { + self.builtin_names.write().unwrap().remove(&skill.name); let mut skills = self.skills.write().unwrap(); skills.insert(skill.name.clone(), skill); } + fn register_builtin(&self, skill: Arc) { + let name = skill.name.clone(); + self.skills.write().unwrap().insert(name.clone(), skill); + self.builtin_names.write().unwrap().insert(name); + } + /// Get a skill by name pub fn get(&self, name: &str) -> Option> { let skills = self.skills.read().unwrap(); @@ -123,6 +137,19 @@ impl SkillRegistry { match Skill::from_file(&candidate) { Ok(skill) => { let name = skill.name.clone(); + if skill.allowed_tools.is_none() { + tracing::warn!( + skill = %name, + path = %candidate.display(), + "Skill omits allowed-tools; Skill invocation is fail-secure and will deny tool use until allowed-tools is declared" + ); + } else if skill.uses_legacy_allowed_tools_syntax() { + tracing::warn!( + skill = %name, + path = %candidate.display(), + "Skill uses legacy whitespace-separated allowed-tools; use comma-separated permissions such as Read(*), Write(*), Bash(*) or a YAML list" + ); + } let skill = Arc::new(skill); if self.get(&name).is_some() { tracing::warn!( @@ -222,6 +249,26 @@ impl SkillRegistry { .collect() } + /// Instruction skills that actively constrain normal session tool use. + /// + /// Built-in skills have local allowlists for explicit `Skill` invocation, + /// but those allowlists must not make the default registry globally + /// read-only. If a user replaces a built-in name through registration or a + /// skill directory, the replacement is no longer considered built-in. + pub fn global_tool_restricting_skills(&self) -> Vec> { + let skills = self.skills.read().unwrap(); + let builtin_names = self.builtin_names.read().unwrap(); + skills + .values() + .filter(|skill| { + skill.kind == SkillKind::Instruction + && skill.allowed_tools.is_some() + && !builtin_names.contains(&skill.name) + }) + .cloned() + .collect() + } + /// Get all skills with a specific tag pub fn by_tag(&self, tag: &str) -> Vec> { let skills = self.skills.read().unwrap(); @@ -451,6 +498,31 @@ mod tests { assert_eq!(persona_skills.len(), 0); } + #[test] + fn test_builtin_skills_do_not_restrict_global_tools() { + let registry = SkillRegistry::with_builtins(); + assert!(registry.global_tool_restricting_skills().is_empty()); + } + + #[test] + fn test_replacing_builtin_name_reenables_global_tool_restriction() { + let registry = SkillRegistry::with_builtins(); + registry.register_unchecked(Arc::new(Skill { + name: "code-review".to_string(), + description: "External replacement".to_string(), + allowed_tools: Some("read(*)".to_string()), + disable_model_invocation: false, + kind: SkillKind::Instruction, + content: String::new(), + tags: Vec::new(), + version: None, + })); + + let restricting = registry.global_tool_restricting_skills(); + assert_eq!(restricting.len(), 1); + assert_eq!(restricting[0].name, "code-review"); + } + #[test] fn test_by_tag() { let registry = SkillRegistry::with_builtins(); diff --git a/core/src/tools/skill.rs b/core/src/tools/skill.rs index af2507f..14f2b00 100644 --- a/core/src/tools/skill.rs +++ b/core/src/tools/skill.rs @@ -226,6 +226,20 @@ impl SkillTool { fn create_skill_permission_policy(skill: &Skill) -> PermissionPolicy { let permissions = skill.parse_allowed_tools(); + if permissions.is_empty() { + tracing::warn!( + skill = %skill.name, + "Skill has no allowed-tools grants; Skill invocation remains fail-secure and will deny tool use" + ); + return PermissionPolicy { + deny: Vec::new(), + allow: Vec::new(), + ask: Vec::new(), + default_decision: PermissionDecision::Deny, + enabled: true, + }; + } + // Convert skill permissions to PermissionRules let mut allow_rules = Vec::new(); for perm in permissions { @@ -453,6 +467,56 @@ mod tests { ); } + #[test] + fn test_skill_permission_policy_denies_when_unspecified() { + let skill = Skill { + name: "test-skill".to_string(), + description: "Test".to_string(), + allowed_tools: None, + disable_model_invocation: false, + kind: SkillKind::Instruction, + content: String::new(), + tags: Vec::new(), + version: None, + }; + + let policy = SkillTool::create_skill_permission_policy(&skill); + + assert_eq!( + policy.check("bash", &serde_json::json!({"command": "python --version"})), + PermissionDecision::Deny + ); + assert_eq!( + policy.check("read", &serde_json::json!({"file_path": "SKILL.md"})), + PermissionDecision::Deny + ); + } + + #[test] + fn test_skill_permission_policy_accepts_legacy_allowed_tools() { + let skill = Skill { + name: "test-skill".to_string(), + description: "Test".to_string(), + allowed_tools: Some("Read Write Edit Bash".to_string()), + disable_model_invocation: false, + kind: SkillKind::Instruction, + content: String::new(), + tags: Vec::new(), + version: None, + }; + + let policy = SkillTool::create_skill_permission_policy(&skill); + + assert_eq!( + policy.check("bash", &serde_json::json!({"command": "python --version"})), + PermissionDecision::Allow + ); + assert_eq!( + policy.check("grep", &serde_json::json!({"pattern": "x"})), + PermissionDecision::Deny + ); + } + #[test] fn test_skill_args_accepts_documented_shape() { let args =