diff --git a/.gitignore b/.gitignore index bcc08937..6af0c827 100644 --- a/.gitignore +++ b/.gitignore @@ -5,4 +5,5 @@ PROMPT.MD PROMPT-*.MD # Local Code Review -src/.vscode/local-reviews/ +.vscode/local-reviews +src/.vscode/local-reviews diff --git a/src/Cargo.lock b/src/Cargo.lock index aa1765f9..88314c4f 100644 --- a/src/Cargo.lock +++ b/src/Cargo.lock @@ -2052,7 +2052,6 @@ dependencies = [ "tempfile", "thiserror 2.0.18", "tinyvec", - "tinyvec_string", "tokio", "wiremock", ] @@ -3692,16 +3691,6 @@ version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1f3ccbac311fea05f86f61904b462b55fb3df8837a366dfc601a0161d0532f20" -[[package]] -name = "tinyvec_string" -version = "0.3.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "be28110ab94064f103a2a24597ed8f556f831d7334751ab991967de16c12d18f" -dependencies = [ - "tinyvec", - "tinyvec_macros", -] - [[package]] name = "tokio" version = "1.50.0" diff --git a/src/llm-coding-tools-agents/ARCHITECTURE.md b/src/llm-coding-tools-agents/ARCHITECTURE.md index 505b4610..d090e58c 100644 --- a/src/llm-coding-tools-agents/ARCHITECTURE.md +++ b/src/llm-coding-tools-agents/ARCHITECTURE.md @@ -226,6 +226,9 @@ and which other agents it can delegate to. Agent frontmatter may include a `permission` map: +In these patterns, `*` means any number of characters (including none), +and `?` means exactly one character. + ```yaml permission: read: allow @@ -243,6 +246,9 @@ PermissionRule::Action(Allow) -> Rule { key: "read", pattern: "*", action: PermissionRule::Action(Deny) -> Rule { key: "bash", pattern: "*", action: Deny } PermissionRule::Pattern({ .. }) -> Rule { key: "task", pattern: "*", action: Deny } Rule { key: "task", pattern: "review-*", action: Allow } + +# Wildcard permission keys are also supported +PermissionRule::Action(Allow) -> Rule { key: "*", pattern: "*", action: Allow } # matches any tool ``` Evaluation uses **last-match-wins** semantics. diff --git a/src/llm-coding-tools-agents/src/extensions.rs b/src/llm-coding-tools-agents/src/extensions.rs index 34abc44e..434ac886 100644 --- a/src/llm-coding-tools-agents/src/extensions.rs +++ b/src/llm-coding-tools-agents/src/extensions.rs @@ -12,7 +12,7 @@ use indexmap::IndexMap; use llm_coding_tools_core::permissions::{Rule, Ruleset}; /// Extension trait for building [`Ruleset`] from agent permission configs. -pub trait RulesetExt: Sized { +pub trait RulesetExt { /// Creates a [`Ruleset`] from frontmatter permission configuration. /// /// The config maps permission keys to either: @@ -37,21 +37,21 @@ pub trait RulesetExt: Sized { /// let ruleset = Ruleset::from_permission_config(&config); /// assert!(ruleset.is_allowed("bash", "*")); /// ``` - fn from_permission_config(config: &IndexMap) -> Self; + fn from_permission_config<'a>(config: &'a IndexMap) -> Ruleset<'a>; } -impl RulesetExt for Ruleset { - fn from_permission_config(config: &IndexMap) -> Self { - let mut ruleset = Self::with_capacity(config.len() * 2); +impl RulesetExt for Ruleset<'_> { + fn from_permission_config<'a>(config: &'a IndexMap) -> Ruleset<'a> { + let mut ruleset = Ruleset::with_capacity(config.len() * 2); for (key, rule) in config { match rule { PermissionRule::Action(action) => { - ruleset.push(Rule::new(key, "*", *action)); + ruleset.push(Rule::new(key.as_str(), "*", *action)); } PermissionRule::Pattern(patterns) => { for (pattern, action) in patterns { - ruleset.push(Rule::new(key, pattern, *action)); + ruleset.push(Rule::new(key.as_str(), pattern.as_str(), *action)); } } } @@ -102,4 +102,60 @@ mod tests { PermissionAction::Deny ); } + + /// Verifies that wildcard permission keys in config are correctly converted + /// to rules and match various permissions at runtime. + #[test] + fn from_permission_config_wildcard_permission_key() { + let mut config = IndexMap::new(); + // "*": allow - wildcard permission key matches any tool + config.insert( + "*".to_string(), + PermissionRule::Action(PermissionAction::Allow), + ); + // "bash": deny - specific tool denied (should override via last-match-wins if ordered after) + config.insert( + "bash".to_string(), + PermissionRule::Action(PermissionAction::Deny), + ); + + let ruleset = Ruleset::from_permission_config(&config); + + // Rules are added in IndexMap iteration order (preserved) + // Rule 0: ("*", "*", Allow) - wildcard + // Rule 1: ("bash", "*", Deny) - exact + + // "*" matches any permission key + assert!( + ruleset.is_allowed("read", "*"), + "wildcard should allow read" + ); + assert!( + ruleset.is_allowed("task", "*"), + "wildcard should allow task" + ); + + // "bash" exact rule comes last and wins over "*" wildcard + assert!( + !ruleset.is_allowed("bash", "*"), + "exact bash deny should win" + ); + + // Verify wildcard actually works (reverse order) + let mut config2 = IndexMap::new(); + config2.insert( + "bash".to_string(), + PermissionRule::Action(PermissionAction::Deny), + ); + config2.insert( + "*".to_string(), + PermissionRule::Action(PermissionAction::Allow), + ); + let ruleset2 = Ruleset::from_permission_config(&config2); + // Now "*" comes last and wins, so bash is allowed + assert!( + ruleset2.is_allowed("bash", "*"), + "wildcard last should allow bash" + ); + } } diff --git a/src/llm-coding-tools-agents/src/runtime/task.rs b/src/llm-coding-tools-agents/src/runtime/task.rs index ac384707..974cf498 100644 --- a/src/llm-coding-tools-agents/src/runtime/task.rs +++ b/src/llm-coding-tools-agents/src/runtime/task.rs @@ -90,7 +90,7 @@ fn sorted_agents(catalog: &AgentCatalog) -> Vec<&AgentConfig> { fn target_is_callable( target: &AgentConfig, - task_rules: &Ruleset, + task_rules: &Ruleset<'_>, has_explicit_task_permission: bool, ) -> bool { matches!(target.mode, AgentMode::All | AgentMode::Subagent) @@ -124,7 +124,7 @@ pub(super) fn resolve_allowed_tools( fn collect_allowed_tools( tools: &[ToolCatalogEntry], - task_rules: &Ruleset, + task_rules: &Ruleset<'_>, task_is_callable: bool, ) -> Vec { let mut allowed = Vec::with_capacity(tools.len()); diff --git a/src/llm-coding-tools-core/Cargo.toml b/src/llm-coding-tools-core/Cargo.toml index 1d58fe63..99efc146 100644 --- a/src/llm-coding-tools-core/Cargo.toml +++ b/src/llm-coding-tools-core/Cargo.toml @@ -56,8 +56,7 @@ ahash = "0.8" # Explicit low-level hash table for model catalog lookup hashbrown = "0.16" -# Inline string storage for patterns -tinyvec_string = { version = "0.3", features = ["alloc"] } +# Inline vector storage for ProviderEnvVars tinyvec = { version = "1.11", features = ["alloc"] } # Efficient immutable string table for provider URLs and env vars diff --git a/src/llm-coding-tools-core/README.md b/src/llm-coding-tools-core/README.md index 17516288..63a8dd97 100644 --- a/src/llm-coding-tools-core/README.md +++ b/src/llm-coding-tools-core/README.md @@ -233,16 +233,20 @@ Currently uses ~2000 tokens for full toolset, ~560 tokens for search-only. - [`Rule`] stores `(permission_key, subject_pattern, action)`. - [`Ruleset`] uses last-match-wins; no match defaults to [`PermissionAction::Deny`]. -- Permission keys are exact-match and case-sensitive; wildcard matching (`*`, `?`) applies to subject patterns. +- Both fields support patterns: + - `*` means any number of characters (including none) + - `?` means exactly one character + - **Permission keys** (exact or wildcard): `bash`, `task`, `webfetch-*` + - **Subject patterns** (wildcard only): `*`, `orchestrator-*`, `agent-?` Frontmatter-style config is typically translated into this model: ```yaml permission: - bash: allow + bash: allow # → ("bash", "*", allow) task: - orchestrator-*: allow - "*": deny + orchestrator-*: allow # → ("task", "orchestrator-*", allow) + "*": deny # → ("task", "*", deny) ``` With last-match-wins, the final `"*": deny` rule overrides earlier `task` matches. diff --git a/src/llm-coding-tools-core/src/internal/hash63.rs b/src/llm-coding-tools-core/src/internal/hash63.rs deleted file mode 100644 index a81934bc..00000000 --- a/src/llm-coding-tools-core/src/internal/hash63.rs +++ /dev/null @@ -1,36 +0,0 @@ -//! 63-bit hash type. -//! -//! A 63-bit hash value representing the upper 63 bits of an original hash. -//! More specifically, a 64-bit hash value which has been `>> 1`. -//! i.e. 64th bit is always 0. - -/// A 63-bit hash value representing the upper 63 bits of an original hash. -/// Result of right shifting a hash `>> 1`. -/// -/// # Bit Layout -/// - Bits 0-62: Original hash bits 1-63 (upper 63 bits) -/// - Bit 63: Always 0 -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub(crate) struct Hash63(u64); - -impl Hash63 { - /// Creates a new Hash63 from a raw u64 value. - /// - /// The caller is responsible for ensuring bit 63 is 0. - #[inline] - pub(crate) const fn from_u64(value: u64) -> Self { - Self(value) - } - - /// Returns the underlying u64 value. - #[inline] - pub(crate) const fn as_u64(&self) -> u64 { - self.0 - } - - /// Creates a Hash63 from a Hash64 by extracting upper 63 bits. - #[inline] - pub(crate) fn from_hash64(hash: crate::internal::hash64::Hash64) -> Self { - Self(hash.as_u64() >> 1) - } -} diff --git a/src/llm-coding-tools-core/src/internal/mod.rs b/src/llm-coding-tools-core/src/internal/mod.rs index 5b2bf28d..850d924f 100644 --- a/src/llm-coding-tools-core/src/internal/mod.rs +++ b/src/llm-coding-tools-core/src/internal/mod.rs @@ -3,6 +3,4 @@ //! This module contains private implementation details that are not part of //! the public API. Items here may change without notice. -pub mod hash63; pub mod hash64; -pub mod packed_permission; diff --git a/src/llm-coding-tools-core/src/internal/packed_permission.rs b/src/llm-coding-tools-core/src/internal/packed_permission.rs deleted file mode 100644 index 0d717b64..00000000 --- a/src/llm-coding-tools-core/src/internal/packed_permission.rs +++ /dev/null @@ -1,76 +0,0 @@ -//! Bit-packed permission storage. -//! -//! Packs permission hash and action into a single u64. -//! - Lower 63 bits: hash63 (upper 63 bits of original hash) -//! - Upper 1 bit (bit 63): PermissionAction - -use crate::internal::hash63::Hash63; -use crate::internal::hash64::Hash64; -use crate::permissions::PermissionAction; - -/// Action bit mask - highest bit (bit 63). -const ACTION_MASK: u64 = 1u64 << 63; - -/// Hash mask - lower 63 bits. -const HASH_MASK: u64 = !ACTION_MASK; - -// Compile-time assertion: PermissionAction must be 1 byte for bit-packing -const _: () = assert!( - std::mem::size_of::() == 1, - "PermissionAction must be 1 byte for bit-packing" -); - -/// A u64 containing both permission hash and action. -/// -/// Layout: -/// - Bits 0-62: hash63 (upper 63 bits of original hash) -/// - Bit 63: PermissionAction -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub(crate) struct PackedPermission(u64); - -impl PackedPermission { - /// Creates a packed permission from hash and action. - #[inline] - pub(crate) fn new(hash: Hash64, action: PermissionAction) -> Self { - let hash63 = Hash63::from_hash64(hash); - let action_bit = (action as u64) << 63; - Self(hash63.as_u64() | action_bit) - } - - /// Returns the hash portion (lower 63 bits) as a [`Hash63`]. - /// Use `Hash63::from_hash64()` to compare with an original Hash64. - #[inline] - pub(crate) fn hash(&self) -> Hash63 { - Hash63::from_u64(self.0 & HASH_MASK) - } - - /// Returns the PermissionAction stored in bit 63. - #[inline] - pub(crate) fn action(&self) -> PermissionAction { - unsafe { std::mem::transmute(((self.0 >> 63) & 1) as u8) } - } -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::internal::hash63::Hash63; - use crate::internal::hash64::Hash64; - - #[test] - fn pack_unpack_roundtrip() { - // Use distinctive pattern: 0x1122334455667788 (easily detect if bits are lost) - let hash = Hash64::from_u64(0x1122334455667788u64); - let hash_shifted = Hash63::from_hash64(hash); - - // Test roundtrip for Allow - let packed_allow = PackedPermission::new(hash, PermissionAction::Allow); - assert_eq!(packed_allow.hash(), hash_shifted); - assert_eq!(packed_allow.action(), PermissionAction::Allow); - - // Test roundtrip for Deny - let packed_deny = PackedPermission::new(hash, PermissionAction::Deny); - assert_eq!(packed_deny.hash(), hash_shifted); - assert_eq!(packed_deny.action(), PermissionAction::Deny); - } -} diff --git a/src/llm-coding-tools-core/src/permissions.rs b/src/llm-coding-tools-core/src/permissions.rs index 90a69bcf..2b5e3515 100644 --- a/src/llm-coding-tools-core/src/permissions.rs +++ b/src/llm-coding-tools-core/src/permissions.rs @@ -8,8 +8,10 @@ //! - If nothing matches, the result is [`PermissionAction::Deny`]. //! //! Matching behaviour: -//! - Permission key: exact match (case-sensitive), no wildcard expansion. -//! - Subject pattern: wildcard matching with `*` (many chars) and `?` (one char). +//! - Permission key: exact text, or a pattern using `*` (any number of +//! characters, including none) and `?` (exactly one character). +//! - Subject pattern: same pattern rules: `*` means any number of +//! characters (including none), and `?` means exactly one character. //! //! # Mapping config to rules //! @@ -17,6 +19,7 @@ //! frontmatter) into this model: //! //! - `bash: allow` maps to `Rule::new("bash", "*", Allow)`. +//! - `*: allow` maps to `Rule::new("*", "*", Allow)` (matches any tool). //! - Pattern maps like `task: { "*": deny, "orchestrator-*": allow }` //! become one rule per pattern, in declaration order. //! @@ -24,7 +27,8 @@ //! //! ```yaml //! permission: -//! bash: allow +//! "*": allow # Allow all tools by default +//! bash: deny # But deny bash specifically (last match wins) //! task: //! "*": deny //! orchestrator-*: allow @@ -34,25 +38,24 @@ //! use llm_coding_tools_core::permissions::{PermissionAction, Rule, Ruleset}; //! //! let mut rules = Ruleset::new(); -//! rules.push(Rule::new("bash", "*", PermissionAction::Allow)); -//! rules.push(Rule::new("task", "*", PermissionAction::Deny)); -//! rules.push(Rule::new("task", "orchestrator-*", PermissionAction::Allow)); +//! rules.push(Rule::new("*", "*", PermissionAction::Allow)); // Allow all +//! rules.push(Rule::new("bash", "*", PermissionAction::Deny)); // Except bash +//! rules.push(Rule::new("task", "*", PermissionAction::Deny)); // Except task +//! rules.push(Rule::new("task", "orchestrator-*", PermissionAction::Allow)); // But allow `orchestrator-*` task //! -//! assert_eq!(rules.evaluate("bash", "any-agent"), PermissionAction::Allow); +//! assert_eq!(rules.evaluate("bash", "any-agent"), PermissionAction::Deny); +//! assert_eq!(rules.evaluate("read", "any-agent"), PermissionAction::Allow); //! assert_eq!( //! rules.evaluate("task", "orchestrator-review"), //! PermissionAction::Allow //! ); //! assert_eq!(rules.evaluate("task", "other-agent"), PermissionAction::Deny); -//! assert_eq!(rules.evaluate("read", "any-agent"), PermissionAction::Deny); //! ``` use serde::{Deserialize, Serialize}; -use tinyvec_string::TinyString; -use crate::internal::hash63::Hash63; use crate::internal::hash64::hash_u64; -use crate::internal::packed_permission::PackedPermission; +use crate::internal::hash64::Hash64; /// Permission level for tool access. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] @@ -68,50 +71,118 @@ pub enum PermissionAction { /// A single permission rule with pattern-based matching. /// -/// Fields are private to enforce packing invariants. +/// Fields are private to enforce invariants. /// Use [`Rule::new`] to create rules. /// -/// # Memory Optimizations +/// # Memory Layout /// -/// See: -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct Rule { - /// Packed permission hash and action. - permission: PackedPermission, - /// Pattern to match against (e.g., "*", "orchestrator-*"). - pattern: TinyString<[u8; 14]>, +/// Size is 56 bytes on 64-bit platforms: +/// - `permission`: 16 bytes (&str ptr + len) +/// - `pattern`: 16 bytes (&str ptr + len) +/// - `permission_hash`: 8 bytes (Hash64) +/// - `pattern_hash`: 8 bytes (Hash64) +/// - `permission_is_wildcard`: 1 byte +/// - `pattern_is_wildcard`: 1 byte +/// - `action`: 1 byte +/// - padding: 5 bytes +/// +/// `Rule<'a>` is `Copy` to enable cheap bulk operations (e.g., `extend_from_slice()` +/// during ruleset merges) without explicit `.clone()` calls. +/// +/// # Miscellaneous Notes +/// +/// In this codebase, [`Rule<'a>`] and [`Ruleset<'a>`] are usually temporary: +/// - Built while deciding which tools an agent can use, then dropped. +/// - Built while preparing Task tool behaviour, then dropped. +/// - Built for one permission check during task delegation, then dropped. +/// +/// They borrow `&'a str` values, so they cannot outlive the source config data. +/// +/// We cache some discovered properties, e.g. `permission_is_wildcard`, to avoid +/// repeating the same checks on every evaluation. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Rule<'a> { + /// The permission key pattern (e.g., "bash", "*", "task-*") + permission: &'a str, + /// The subject pattern (e.g., "*", "orchestrator-*") + pattern: &'a str, + /// Pre-computed hash of `permission` for fast exact-match comparison + permission_hash: Hash64, + /// Pre-computed hash of `pattern` for potential fast-path usage + pattern_hash: Hash64, + /// Whether `permission` uses `*` (any number of chars) or `?` (one char). + permission_is_wildcard: bool, + /// Whether `pattern` uses `*` (any number of chars) or `?` (one char). + pattern_is_wildcard: bool, + /// The action (Allow/Deny) + action: PermissionAction, } -impl Rule { +impl<'a> Rule<'a> { /// Creates a new rule with the provided permission and pattern. + /// + /// Permission keys with `*` or `?` are treated as patterns. + /// `*` matches any number of characters (including none), and `?` + /// matches exactly one. + /// + /// # Examples + /// + /// ``` + /// use llm_coding_tools_core::permissions::{Rule, PermissionAction}; + /// + /// // Exact match on permission key + /// let exact = Rule::new("bash", "*", PermissionAction::Allow); + /// + /// // Wildcard permission key matches any tool + /// let wildcard = Rule::new("*", "*", PermissionAction::Allow); + /// ``` #[inline] - pub fn new( - permission: impl AsRef, - pattern: impl AsRef, - action: PermissionAction, - ) -> Self { + pub fn new(permission: &'a str, pattern: &'a str, action: PermissionAction) -> Self { Self { - permission: PackedPermission::new(hash_u64(permission.as_ref()), action), - pattern: TinyString::<[u8; 14]>::from(pattern.as_ref()), + permission, + pattern, + permission_hash: hash_u64(permission), + pattern_hash: hash_u64(pattern), + permission_is_wildcard: permission.contains('*') || permission.contains('?'), + pattern_is_wildcard: pattern.contains('*') || pattern.contains('?'), + action, } } - /// Returns the stored 63-bit permission hash. + /// Returns the permission key pattern. #[inline] - pub fn permission_hash(&self) -> u64 { - self.permission.hash().as_u64() + pub fn permission(&self) -> &'a str { + self.permission } /// Returns the stored pattern. #[inline] - pub fn pattern(&self) -> &str { - self.pattern.as_str() + pub fn pattern(&self) -> &'a str { + self.pattern } /// Returns the action for this rule. #[inline] pub fn action(&self) -> PermissionAction { - self.permission.action() + self.action + } + + /// Returns the stored 64-bit permission hash. + #[inline] + pub fn permission_hash(&self) -> u64 { + self.permission_hash.as_u64() + } + + /// Returns true if the permission key contains wildcards. + #[inline] + pub fn permission_is_wildcard(&self) -> bool { + self.permission_is_wildcard + } + + /// Returns true if the pattern contains wildcards. + #[inline] + pub fn pattern_is_wildcard(&self) -> bool { + self.pattern_is_wildcard } } @@ -122,11 +193,11 @@ impl Rule { /// When no rule matches, the default action is [`PermissionAction::Deny`]. /// To allow a permission, you must explicitly add an allow rule. #[derive(Debug, Clone, Default)] -pub struct Ruleset { - rules: Vec, +pub struct Ruleset<'a> { + rules: Vec>, } -impl Ruleset { +impl<'a> Ruleset<'a> { /// Creates an empty ruleset. #[inline] pub fn new() -> Self { @@ -143,7 +214,7 @@ impl Ruleset { /// Appends a rule to the ruleset. #[inline] - pub fn push(&mut self, rule: Rule) { + pub fn push(&mut self, rule: Rule<'a>) { self.rules.push(rule); } @@ -161,7 +232,7 @@ impl Ruleset { /// Returns an iterator over the rules. #[inline] - pub fn iter(&self) -> impl Iterator { + pub fn iter(&self) -> impl Iterator> { self.rules.iter() } @@ -170,27 +241,46 @@ impl Ruleset { /// Returns the action from the last matching rule, or [`PermissionAction::Deny`] /// if no rule matches (default deny). /// - /// Permission keys are matched with **exact equality**. - /// Patterns are matched against subjects using wildcard matching. + /// Permission keys can be exact text, or patterns. + /// In patterns, `*` means any number of characters (including none), + /// and `?` means exactly one. /// /// # Arguments /// - /// * `permission` - The permission key (tool name) to check (exact match) + /// * `permission` - The permission key (tool name) to check; + /// `*` means any number of chars, `?` means one char. /// * `subject` - The subject to match against rule patterns (e.g., agent name, path) pub fn evaluate(&self, permission: &str, subject: &str) -> PermissionAction { - let permission_hash = Hash63::from_hash64(hash_u64(permission)); - - // Last-match-wins: iterate forward, keep overwriting result + let permission_hash = hash_u64(permission); + let subject_hash = hash_u64(subject); let mut result = PermissionAction::Deny; for rule in &self.rules { - // Permission key: exact match only (no wildcards) - // Pattern: wildcard match against subject - if rule.permission.hash() == permission_hash - && wildcard_match(subject, rule.pattern.as_str()) - { - result = rule.permission.action(); + let permission_matches = rule_matches( + permission, + permission_hash, + rule.permission, + rule.permission_hash, + rule.permission_is_wildcard, + ); + + if !permission_matches { + continue; } + + let pattern_matches = rule_matches( + subject, + subject_hash, + rule.pattern, + rule.pattern_hash, + rule.pattern_is_wildcard, + ); + + if !pattern_matches { + continue; + } + + result = rule.action; } result @@ -209,19 +299,22 @@ impl Ruleset { /// /// Rules from `other` are appended in order, giving them higher priority /// in last-match-wins evaluation. - pub fn merge(&mut self, other: &Ruleset) { + pub fn merge(&mut self, other: &Ruleset<'a>) { self.rules.reserve(other.rules.len()); - self.rules.extend(other.rules.iter().cloned()); + self.rules.extend_from_slice(&other.rules); } /// Creates a new ruleset by merging multiple rulesets. /// /// Rules are concatenated in order; later rulesets have higher priority. - pub fn merged<'a>(rulesets: impl IntoIterator) -> Self { + pub fn merged<'b>(rulesets: impl IntoIterator>) -> Self + where + 'a: 'b, + { let rulesets: Vec<_> = rulesets.into_iter().collect(); let capacity = rulesets.iter().map(|r| r.len()).sum(); let mut result = Self::with_capacity(capacity); - for ruleset in rulesets { + for ruleset in &rulesets { result.merge(ruleset); } result @@ -230,7 +323,8 @@ impl Ruleset { /// Matches a string against a wildcard pattern. /// -/// Supports `*` (matches any sequence) and `?` (matches single char). +/// `*` matches any number of characters (including none), and `?` +/// matches exactly one character. /// /// # Examples /// @@ -293,331 +387,290 @@ fn wildcard_match_impl(input: &[u8], pattern: &[u8]) -> bool { p == pattern.len() } +/// Matches an input against a rule value using hash+string or wildcard matching. +/// +/// When `is_wildcard` is false, uses fast hash comparison followed by string +/// equality verification (for collision safety). When `is_wildcard` is true, +/// falls back to `wildcard_match()` for pattern matching. +#[inline(always)] +fn rule_matches( + input: &str, + input_hash: Hash64, + rule_value: &str, + rule_hash: Hash64, + is_wildcard: bool, +) -> bool { + if is_wildcard { + wildcard_match(input, rule_value) + } else { + rule_hash == input_hash && rule_value == input + } +} + #[cfg(test)] mod tests { use super::*; use rstest::rstest; - /// Verifies that wildcard patterns match inputs correctly for various - /// pattern types: star (*), question mark (?), and combinations. - #[rstest] - // Star matches any string including empty - #[case::star_matches_anything( - "anything", // input: any non-empty string - "*", // pattern: single star matches everything - true // matches: yes - )] - #[case::star_matches_empty( - "", // input: empty string - "*", // pattern: star matches empty too - true // matches: yes - )] - // Exact match requires identical strings - #[case::exact_match( - "bash", // input: exact string - "bash", // pattern: identical string - true // matches: yes (exact match) - )] - #[case::exact_mismatch( - "bash", // input: one string - "task", // pattern: different string - false // matches: no (not identical) - )] - // Prefix patterns match from start - #[case::prefix_star( - "orchestrator-builder", // input: starts with prefix - "orchestrator-*", // pattern: prefix + star suffix - true // matches: yes (prefix matches) - )] - #[case::prefix_star_match_empty_suffix( - "orchestrator-", // input: prefix only, empty suffix - "orchestrator-*", // pattern: prefix + star - true // matches: yes (star matches empty) - )] - #[case::prefix_star_no_match( - "other-builder", // input: different prefix - "orchestrator-*", // pattern: specific prefix required - false // matches: no (wrong prefix) - )] - // Suffix patterns match from end - #[case::suffix_star( - "pre-bash", // input: ends with suffix - "*-bash", // pattern: star prefix + suffix - true // matches: yes (suffix matches) - )] - #[case::suffix_star_match_empty_prefix( - "-bash", // input: suffix only, empty prefix - "*-bash", // pattern: star + suffix - true // matches: yes (star matches empty) - )] - #[case::suffix_star_no_match( - "bash-post", // input: different ending - "*-bash", // pattern: specific suffix required - false // matches: no (wrong suffix) - )] - // Middle star matches substring - #[case::middle_star( - "a-middle-z", // input: middle substring present - "a-*-z", // pattern: prefix + star + suffix - true // matches: yes (middle exists) - )] - #[case::middle_star_empty_middle( - "a--z", // input: empty middle (adjacent delimiters) - "a-*-z", // pattern: prefix + star + suffix - true // matches: yes (star matches empty) - )] - #[case::middle_star_no_match( - "a-middle", // input: missing suffix - "a-*-z", // pattern: requires suffix - false // matches: no (no suffix) - )] - // Question mark matches exactly one character - #[case::question_mark( - "test", // input: one char at position 3 - "te?t", // pattern: te + ? + t - true // matches: yes (s matches ?) - )] - #[case::question_mark_another( - "teat", // input: different char at position 3 - "te?t", // pattern: te + ? + t - true // matches: yes (a matches ?) - )] - #[case::question_mark_too_many( - "teest", // input: two chars at position 3 - "te?t", // pattern: exactly one char required - false // matches: no (too many chars) - )] - #[case::question_mark_too_few( - "tet", // input: zero chars at position 3 - "te?t", // pattern: exactly one char required - false // matches: no (too few chars) - )] - // Multiple stars work together - #[case::multiple_stars_path( - "a/b/c", // input: path with slash - "*/*", // pattern: two stars match components - true // matches: yes - )] - #[case::multiple_stars_complex( - "abc", // input: surrounded chars - "*a*c*", // pattern: stars on both sides - true // matches: yes - )] - // Empty input edge cases - #[case::empty_input_with_star( - "", // input: empty string - "*", // pattern: star matches empty - true // matches: yes - )] - #[case::empty_input_with_question( - "", // input: empty string - "?", // pattern: requires one char - false // matches: no (empty has zero) - )] - #[case::empty_input_with_literal( - "", // input: empty string - "a", // pattern: requires literal "a" - false // matches: no (not equal) - )] - fn wildcard_pattern_matching( - #[case] input: &str, - #[case] pattern: &str, - #[case] should_match: bool, - ) { - assert_eq!(wildcard_match(input, pattern), should_match); + fn build_and_eval( + rules: &[(&str, &str, PermissionAction)], + permission: &str, + subject: &str, + ) -> PermissionAction { + let mut ruleset = Ruleset::new(); + for (perm, pat, action) in rules { + ruleset.push(Rule::new(perm, pat, *action)); + } + ruleset.evaluate(permission, subject) } - /// Verifies that Rule preserves casing and computes correct hashes. + // --- wildcard_match --- + + /// Verifies [`wildcard_match`] semantics (`*`, `?`, exact, and case sensitivity). #[rstest] - #[case::pattern_casing( - "BASH", // permission: uppercase permission key - "PATTERN", // pattern: uppercase pattern string - "PATTERN" // expected: pattern unchanged (preserves casing) - )] - #[case::lowercase_pattern( - "bash", // permission: lowercase permission key - "pattern", // pattern: lowercase pattern string - "pattern" // expected: pattern unchanged (preserves casing) - )] - fn rule_preserves_casing( - #[case] permission: &str, - #[case] pattern: &str, - #[case] expected: &str, - ) { - let rule = Rule::new(permission, pattern, PermissionAction::Allow); - assert_eq!(rule.pattern(), expected); + #[case::star_should_match_empty( + "", // input: empty string + "*", // pattern: universal wildcard + true // expected: '*' matches any length, including empty + )] + #[case::exact_literals_should_match( + "bash", // input: exact literal + "bash", // pattern: same literal + true // expected: exact literals match + )] + #[case::different_literals_should_not_match( + "bash", // input: literal text + "task", // pattern: different literal text + false // expected: different literals do not match + )] + #[case::prefix_star_should_match_suffix( + "orchestrator-builder", // input: starts with required prefix + "orchestrator-*", // pattern: fixed prefix plus wildcard suffix + true // expected: suffix wildcard consumes the remainder + )] + #[case::prefix_star_should_not_match_wrong_prefix( + "other-builder", // input: wrong prefix + "orchestrator-*", // pattern: requires "orchestrator-" prefix + false // expected: prefix requirement fails + )] + #[case::multiple_stars_should_backtrack( + "ba-foobar-sh-ext", // input: requires star backtracking to fit both anchors + "ba*sh*", // pattern: anchored at "ba" then later "sh" + true // expected: matcher finds a valid star split + )] + #[case::question_mark_should_match_one_char( + "test", // input: exactly one char between "te" and "t" + "te?t", // pattern: single-char wildcard in the middle + true // expected: '?' matches exactly one character + )] + #[case::question_mark_should_not_match_two_chars( + "teest", // input: two chars between "te" and "t" + "te?t", // pattern: allows exactly one char in the middle + false // expected: '?' cannot consume two characters + )] + #[case::matching_should_be_case_sensitive( + "TOOL-read", // input: uppercase prefix + "tool-*", // pattern: lowercase prefix + false // expected: matching is case-sensitive + )] + fn wildcard_match_cases(#[case] input: &str, #[case] pattern: &str, #[case] expected: bool) { + assert_eq!(wildcard_match(input, pattern), expected); + } + + // --- Rule construction / metadata --- + + /// A plain permission key without `*` or `?` must not set the wildcard flag. + #[test] + fn exact_key_should_not_set_wildcard_flag() { + let rule = Rule::new("bash", "*", PermissionAction::Allow); + assert_eq!(rule.permission(), "bash"); + assert_eq!(rule.permission_hash(), hash_u64("bash").as_u64()); + assert!(!rule.permission_is_wildcard()); + assert_eq!(rule.action(), PermissionAction::Allow); + } + + /// A lone `*` permission key must set the wildcard flag. + #[test] + fn star_key_should_set_wildcard_flag() { + let rule = Rule::new("*", "*", PermissionAction::Allow); + assert_eq!(rule.permission(), "*"); + assert_eq!(rule.permission_hash(), hash_u64("*").as_u64()); + assert!(rule.permission_is_wildcard()); } + /// A permission key like `"bash*"` ends with a wildcard and must set the flag. #[test] - fn rule_permission_hash_is_case_sensitive() { + fn partial_wildcard_key_should_set_wildcard_flag() { + let rule = Rule::new("bash*", "*", PermissionAction::Allow); + assert_eq!(rule.permission(), "bash*"); + assert_eq!(rule.permission_hash(), hash_u64("bash*").as_u64()); + assert!(rule.permission_is_wildcard()); + } + + /// A subject pattern containing `*` must set the pattern wildcard flag, leaving permission untouched. + #[test] + fn wildcard_subject_should_set_wildcard_flag() { + let rule = Rule::new("bash", "orchestrator-*", PermissionAction::Allow); + assert_eq!(rule.pattern(), "orchestrator-*"); + assert!(rule.pattern_is_wildcard()); + assert!(!rule.permission_is_wildcard()); + } + + /// A plain subject string without wildcards must not set the pattern wildcard flag. + #[test] + fn exact_subject_should_not_set_wildcard_flag() { + let rule = Rule::new("bash", "exact-subject", PermissionAction::Allow); + assert_eq!(rule.pattern(), "exact-subject"); + assert!(!rule.pattern_is_wildcard()); + assert!(!rule.permission_is_wildcard()); + } + + #[test] + fn rule_permission_hash_should_be_case_sensitive() { let upper = Rule::new("BASH", "*", PermissionAction::Allow); let lower = Rule::new("bash", "*", PermissionAction::Allow); assert_ne!(upper.permission_hash(), lower.permission_hash()); } #[test] - fn rule_getters_return_correct_values() { - let rule = Rule::new("task", "orchestrator-*", PermissionAction::Allow); - assert_eq!(rule.pattern(), "orchestrator-*"); - assert_eq!(rule.action(), PermissionAction::Allow); + fn rule_should_be_56_byte_copy() { + assert_eq!(std::mem::size_of::>(), 56); + fn assert_copy() {} + assert_copy::>(); } + // --- Ruleset evaluate --- + #[test] - fn rule_size_is_32_bytes() { - assert_eq!(std::mem::size_of::(), 32); + fn evaluate_when_no_rules_should_deny() { + assert_eq!( + build_and_eval(&[], "bash", "anything"), + PermissionAction::Deny, + ); } - /// Verifies that Ruleset evaluates rules correctly, including default deny - /// and last-match-wins behaviour. - #[rstest] - // Default is deny when no rules match - #[case::default_deny( - vec![], // rules: empty ruleset - ("bash", "anything"), // evaluate: permission="bash", subject="anything" - PermissionAction::Deny // expected: default deny (no matching rules) - )] - // Simple allow rule permits matching permission - #[case::simple_allow( - vec![("bash", "*", PermissionAction::Allow)], // rules: allow all for "bash" permission - ("bash", "anything"), // evaluate: matches permission="bash" - PermissionAction::Allow // expected: allow (rule matches) - )] - // Non-matching permission is still denied - #[case::simple_allow_nonmatch( - vec![("bash", "*", PermissionAction::Allow)], // rules: allow all for "bash" only - ("task", "anything"), // evaluate: different permission="task" - PermissionAction::Deny // expected: deny (no matching rules) - )] - // Last match wins when multiple rules apply - #[case::last_match_wins_allow( - vec![ - ("task", "*", PermissionAction::Deny), // rules[0]: deny all task - ("task", "orchestrator-*", PermissionAction::Allow) // rules[1]: allow orchestrator-* - ], - ("task", "orchestrator-builder"), // evaluate: matches both rules - PermissionAction::Allow // expected: allow (rules[1] wins) - )] - #[case::last_match_wins_deny( - vec![ - ("task", "orchestrator-*", PermissionAction::Allow), // rules[0]: allow orchestrator-* - ("task", "*", PermissionAction::Deny) // rules[1]: deny all task - ], - ("task", "orchestrator-builder"), // evaluate: matches both rules - PermissionAction::Deny // expected: deny (rules[1] wins) - )] - // Only first rule applies when second doesn't match - #[case::partial_match_first_rule( - vec![ - ("task", "*", PermissionAction::Deny), // rules[0]: deny all task (matches) - ("task", "orchestrator-*", PermissionAction::Allow) // rules[1]: allow orchestrator-* (doesn't match) - ], - ("task", "random-agent"), // evaluate: only matches rules[0] - PermissionAction::Deny // expected: deny (only first rule matches) - )] - fn ruleset_evaluate( - #[case] rules: Vec<(&str, &str, PermissionAction)>, - #[case] (permission, target): (&str, &str), - #[case] expected: PermissionAction, - ) { - let mut ruleset = Ruleset::new(); - for (perm, pat, action) in rules { - ruleset.push(Rule::new(perm, pat, action)); - } - assert_eq!(ruleset.evaluate(permission, target), expected); + #[test] + fn evaluate_exact_match_should_allow() { + assert_eq!( + build_and_eval( + &[("bash", "*", PermissionAction::Allow)], + "bash", + "anything" + ), + PermissionAction::Allow, + ); } - /// Verifies that permission keys and patterns are case-sensitive. - #[rstest] - #[case::permission_uppercase( - vec![("BASH", "*", PermissionAction::Allow)], // rules: allow all for "BASH" permission - ("BASH", "test"), // evaluate: exact case match - PermissionAction::Allow // expected: allow (exact case match) - )] - #[case::permission_lowercase_no_match( - vec![("BASH", "*", PermissionAction::Allow)], // rules: allow all for "BASH" only - ("bash", "test"), // evaluate: different case "bash" - PermissionAction::Deny // expected: deny (case mismatch) - )] - #[case::pattern_uppercase( - vec![("task", "AGENT-*", PermissionAction::Allow)], // rules: allow AGENT-* pattern - ("task", "AGENT-foo"), // evaluate: exact case match - PermissionAction::Allow // expected: allow (exact case match) - )] - #[case::pattern_lowercase_no_match( - vec![("task", "AGENT-*", PermissionAction::Allow)], // rules: allow AGENT-* only - ("task", "agent-foo"), // evaluate: different case "agent-foo" - PermissionAction::Deny // expected: deny (case mismatch) - )] - fn ruleset_evaluate_case_sensitive( - #[case] rules: Vec<(&str, &str, PermissionAction)>, - #[case] (permission, target): (&str, &str), - #[case] expected: PermissionAction, - ) { - let mut ruleset = Ruleset::new(); - for (perm, pat, action) in rules { - ruleset.push(Rule::new(perm, pat, action)); - } - assert_eq!(ruleset.evaluate(permission, target), expected); + #[test] + fn evaluate_should_be_case_sensitive() { + assert_eq!( + build_and_eval( + &[("BASH", "*", PermissionAction::Allow)], + "bash", + "anything" + ), + PermissionAction::Deny, + ); } - /// Verifies that wildcards in the first parameter of Rule::new() (permission) - /// do NOT match other keys. - /// - /// The permission field requires exact match only - "*" is treated as a literal - /// string, not a wildcard. So a rule with permission="*" only matches when the - /// evaluated permission is exactly "*", not "bash" or any other value. - /// - /// This differs from the second parameter (pattern/target) which does support - /// wildcards. - #[rstest] - #[case::star_permission_not_bash( - vec![("*", "*", PermissionAction::Allow)], // rules: allow all for permission="*" only - ("bash", "anything"), // evaluate: different permission="bash" - PermissionAction::Deny // expected: deny (no match, must be exact) - )] - #[case::star_permission_matches_star( - vec![("*", "*", PermissionAction::Allow)], // rules: allow all for permission="*" - ("*", "anything"), // evaluate: exact match on permission="*" - PermissionAction::Allow // expected: allow (exact permission match) - )] - #[case::bash_star_permission_not_bash( - vec![("bash*", "*", PermissionAction::Allow)], // rules: allow all for permission="bash*" - ("bash", "anything"), // evaluate: different permission="bash" - PermissionAction::Deny // expected: deny (must be exact) - )] - #[case::bash_star_permission_not_bash_extended( - vec![("bash*", "*", PermissionAction::Allow)], // rules: allow all for permission="bash*" - ("bash-extended", "anything"), // evaluate: different permission="bash-extended" - PermissionAction::Deny // expected: deny (must be exact) - )] - #[case::bash_star_permission_matches_exact( - vec![("bash*", "*", PermissionAction::Allow)], // rules: allow all for permission="bash*" - ("bash*", "anything"), // evaluate: exact match on permission="bash*" - PermissionAction::Allow // expected: allow (exact permission match) - )] - fn ruleset_evaluate_permission_exact_match_only( - #[case] rules: Vec<(&str, &str, PermissionAction)>, - #[case] (permission, target): (&str, &str), - #[case] expected: PermissionAction, - ) { - let mut ruleset = Ruleset::new(); - for (perm, pat, action) in rules { - ruleset.push(Rule::new(perm, pat, action)); - } - assert_eq!(ruleset.evaluate(permission, target), expected); + #[test] + fn evaluate_last_matching_rule_should_win() { + assert_eq!( + build_and_eval( + &[ + ("task", "*", PermissionAction::Deny), + ("task", "orchestrator-*", PermissionAction::Allow), + ], + "task", + "orchestrator-builder", + ), + PermissionAction::Allow, + ); + } + + #[test] + fn evaluate_non_matching_rule_should_not_override() { + assert_eq!( + build_and_eval( + &[ + ("task", "*", PermissionAction::Deny), + ("task", "orchestrator-*", PermissionAction::Allow), + ], + "task", + "random-agent", + ), + PermissionAction::Deny, + ); } #[test] - fn ruleset_is_allowed_convenience() { + fn evaluate_star_permission_should_match_any_tool() { + assert_eq!( + build_and_eval(&[("*", "*", PermissionAction::Allow)], "bash", "anything"), + PermissionAction::Allow, + ); + } + + #[test] + fn evaluate_prefix_wildcard_permission_should_match() { + assert_eq!( + build_and_eval( + &[("bash*", "*", PermissionAction::Allow)], + "bash-extended", + "anything" + ), + PermissionAction::Allow, + ); + } + + #[test] + fn evaluate_question_mark_permission_should_match() { + assert_eq!( + build_and_eval( + &[("te?t", "*", PermissionAction::Allow)], + "test", + "anything" + ), + PermissionAction::Allow, + ); + } + + #[test] + fn evaluate_wildcard_subject_should_match() { + assert_eq!( + build_and_eval( + &[("task", "orchestrator-*", PermissionAction::Allow)], + "task", + "orchestrator-builder", + ), + PermissionAction::Allow, + ); + } + + #[test] + fn evaluate_both_fields_must_match() { + assert_eq!( + build_and_eval( + &[("*", "orchestrator-*", PermissionAction::Allow)], + "bash", + "other-agent", + ), + PermissionAction::Deny, + ); + } + + // --- Ruleset convenience --- + + #[test] + fn is_allowed_should_reflect_evaluate() { let mut ruleset = Ruleset::new(); ruleset.push(Rule::new("bash", "*", PermissionAction::Allow)); - assert!(ruleset.is_allowed("bash", "any")); assert!(!ruleset.is_allowed("task", "any")); } #[test] - fn ruleset_merge() { + fn merge_should_append_and_override() { let mut base = Ruleset::new(); base.push(Rule::new("bash", "*", PermissionAction::Deny)); @@ -625,13 +678,11 @@ mod tests { override_rules.push(Rule::new("bash", "*", PermissionAction::Allow)); base.merge(&override_rules); - - // After merge, the allow rule comes last and wins assert_eq!(base.evaluate("bash", "any"), PermissionAction::Allow); } #[test] - fn ruleset_merged_multiple() { + fn merged_should_concatenate_in_order() { let mut r1 = Ruleset::new(); r1.push(Rule::new("a", "*", PermissionAction::Deny)); diff --git a/src/llm-coding-tools-serdesai/examples/agents/task-demo/orchestrator.md b/src/llm-coding-tools-serdesai/examples/agents/task-demo/orchestrator.md index 00ebe167..b736e407 100644 --- a/src/llm-coding-tools-serdesai/examples/agents/task-demo/orchestrator.md +++ b/src/llm-coding-tools-serdesai/examples/agents/task-demo/orchestrator.md @@ -13,6 +13,7 @@ permission: todoread: deny todowrite: deny task: + # `*` means any delegate name; "reader" is an exact name. "*": deny "reader": allow ---