diff --git a/src/llm-coding-tools-agents/Cargo.toml b/src/llm-coding-tools-agents/Cargo.toml index 17205a5e..24f56215 100644 --- a/src/llm-coding-tools-agents/Cargo.toml +++ b/src/llm-coding-tools-agents/Cargo.toml @@ -42,3 +42,7 @@ rstest = "0.26" [[bench]] name = "parser" harness = false + +[[bench]] +name = "runtime_task" +harness = false diff --git a/src/llm-coding-tools-agents/benches/runtime_task.rs b/src/llm-coding-tools-agents/benches/runtime_task.rs new file mode 100644 index 00000000..378db301 --- /dev/null +++ b/src/llm-coding-tools-agents/benches/runtime_task.rs @@ -0,0 +1,135 @@ +//! Benchmarks for [`AgentRuntime`] task-delegation cache lookups. +//! +//! Measures the cost of [`AgentRuntime::allowed_tools`], +//! [`AgentRuntime::summarize_callable_targets`], and +//! [`AgentRuntime::can_delegate_to`] across varying agent counts. + +use ahash::AHashMap; +use core::hint::black_box; +use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion, Throughput}; +use indexmap::IndexMap; +use llm_coding_tools_agents::{ + AgentCatalog, AgentConfig, AgentMode, AgentRuntimeBuilder, AgentToolSettings, PermissionRule, +}; +use llm_coding_tools_core::permissions::PermissionAction; +use llm_coding_tools_core::tool_metadata::{read as read_meta, task as task_meta}; + +/// Build a minimal [`AgentConfig`] for benchmark fixtures. +/// +/// `permission` controls tool-access rules; all other fields are filled +/// with placeholder values suitable for performance measurement only. +fn build_agent( + name: &str, + mode: AgentMode, + permission: IndexMap, +) -> AgentConfig { + AgentConfig { + name: name.into(), + mode, + description: format!("{name} description").into(), + model: None, + hidden: false, + temperature: None, + top_p: None, + permission, + options: AHashMap::new(), + tool_settings: AgentToolSettings::default(), + prompt: Default::default(), + } +} + +/// Create a permission map that denies all tools by default, but allows +/// pattern-matched delegation to agents named `review-*` or `worker-*` +/// via the task tool, and blanket-allows the read tool. +fn patterned_task_permission() -> IndexMap { + let mut patterns = IndexMap::new(); + patterns.insert("*".to_string(), PermissionAction::Deny); + patterns.insert("review-*".to_string(), PermissionAction::Allow); + patterns.insert("worker-*".to_string(), PermissionAction::Allow); + + IndexMap::from([ + (task_meta::NAME.into(), PermissionRule::Pattern(patterns)), + ( + read_meta::NAME.into(), + PermissionRule::Action(PermissionAction::Allow), + ), + ]) +} + +/// Build an [`AgentRuntime`] with one `caller` primary agent and +/// `agent_count` subordinate agents. +/// +/// Subordinate names cycle through `review-NNN`, `worker-NNN`, and +/// `misc-NNN` prefixes. Every 11th subordinate is a primary-mode agent; +/// the rest are subagents. +fn build_runtime(agent_count: usize) -> llm_coding_tools_agents::AgentRuntime { + let mut agents = Vec::with_capacity(agent_count + 1); + agents.push(build_agent( + "caller", + AgentMode::Primary, + patterned_task_permission(), + )); + + for idx in 0..agent_count { + let name = match idx % 3 { + 0 => format!("review-{idx:03}"), + 1 => format!("worker-{idx:03}"), + _ => format!("misc-{idx:03}"), + }; + let mode = if idx % 11 == 0 { + AgentMode::Primary + } else { + AgentMode::Subagent + }; + agents.push(build_agent(&name, mode, IndexMap::new())); + } + + AgentRuntimeBuilder::new() + .catalog(AgentCatalog::from_entries(agents)) + .build() +} + +/// Benchmark cached delegation queries against runtimes of 16, 64, and 256 agents. +/// +/// Measures four operations: +/// - **allowed_tools** – full tool-set resolution for the `caller` agent. +/// - **summaries** – callable-target summary strings for `caller`. +/// - **can_delegate_hit** – pattern-match hit (`caller` → `review-003`). +/// - **can_delegate_miss** – pattern-match miss (`caller` → `misc-002`). +fn bench_runtime_task_caches(c: &mut Criterion) { + let mut group = c.benchmark_group("runtime/task_caches"); + + for agent_count in [16_usize, 64, 256] { + let runtime = build_runtime(agent_count); + group.throughput(Throughput::Elements(1)); + + group.bench_with_input( + BenchmarkId::new("allowed_tools", agent_count), + &runtime, + |b, runtime| b.iter(|| black_box(runtime.allowed_tools("caller"))), + ); + + group.bench_with_input( + BenchmarkId::new("summaries", agent_count), + &runtime, + |b, runtime| b.iter(|| black_box(runtime.summarize_callable_targets("caller"))), + ); + + group.bench_with_input( + BenchmarkId::new("can_delegate_hit", agent_count), + &runtime, + |b, runtime| b.iter(|| black_box(runtime.can_delegate_to("caller", "review-003"))), + ); + + group.bench_with_input( + BenchmarkId::new("can_delegate_miss", agent_count), + &runtime, + |b, runtime| b.iter(|| black_box(runtime.can_delegate_to("caller", "misc-002"))), + ); + } + + group.finish(); +} + +criterion_group!(benches, bench_runtime_task_caches); +criterion_main!(benches); diff --git a/src/llm-coding-tools-agents/src/extensions.rs b/src/llm-coding-tools-agents/src/extensions.rs index 434ac886..637c1130 100644 --- a/src/llm-coding-tools-agents/src/extensions.rs +++ b/src/llm-coding-tools-agents/src/extensions.rs @@ -37,11 +37,11 @@ pub trait RulesetExt { /// let ruleset = Ruleset::from_permission_config(&config); /// assert!(ruleset.is_allowed("bash", "*")); /// ``` - fn from_permission_config<'a>(config: &'a IndexMap) -> Ruleset<'a>; + fn from_permission_config(config: &IndexMap) -> Ruleset; } -impl RulesetExt for Ruleset<'_> { - fn from_permission_config<'a>(config: &'a IndexMap) -> Ruleset<'a> { +impl RulesetExt for Ruleset { + fn from_permission_config(config: &IndexMap) -> Ruleset { let mut ruleset = Ruleset::with_capacity(config.len() * 2); for (key, rule) in config { diff --git a/src/llm-coding-tools-agents/src/runtime/builder.rs b/src/llm-coding-tools-agents/src/runtime/builder.rs index 43f6117d..0003b4bc 100644 --- a/src/llm-coding-tools-agents/src/runtime/builder.rs +++ b/src/llm-coding-tools-agents/src/runtime/builder.rs @@ -80,9 +80,12 @@ mod tests { use super::AgentRuntimeBuilder; use crate::runtime::tool_catalog::{default_tools, ToolCatalogEntry, ToolCatalogKind}; use crate::runtime::AgentDefaults; - use crate::{AgentCatalog, AgentConfig, AgentMode, AgentToolSettings}; + use crate::{AgentCatalog, AgentConfig, AgentMode, AgentToolSettings, PermissionRule}; + use indexmap::IndexMap; + use llm_coding_tools_core::permissions::PermissionAction; use llm_coding_tools_core::tool_metadata::{glob as glob_meta, read as read_meta}; use llm_coding_tools_core::TaskSettings; + use std::sync::Arc; fn sample_config(name: &str, model: Option<&str>) -> AgentConfig { AgentConfig { @@ -147,4 +150,36 @@ mod tests { assert_eq!(runtime.task_settings(), TaskSettings::default()); assert_eq!(runtime.tools(), default_tools().as_slice()); } + + #[test] + fn builder_caches_permission_rulesets() { + let runtime = AgentRuntimeBuilder::new() + .catalog(AgentCatalog::from_entries([AgentConfig { + name: "planner".into(), + mode: AgentMode::Subagent, + description: "planner description".into(), + model: None, + hidden: false, + temperature: None, + top_p: None, + permission: IndexMap::from([( + read_meta::NAME.into(), + PermissionRule::Action(PermissionAction::Allow), + )]), + options: Default::default(), + tool_settings: AgentToolSettings::default(), + prompt: Default::default(), + }])) + .build(); + + let first = runtime + .permission_ruleset("planner") + .expect("cached ruleset should exist"); + let second = runtime + .permission_ruleset("planner") + .expect("cached ruleset should exist"); + + assert!(Arc::ptr_eq(&first, &second)); + assert!(first.is_allowed(read_meta::NAME, "*")); + } } diff --git a/src/llm-coding-tools-agents/src/runtime/state.rs b/src/llm-coding-tools-agents/src/runtime/state.rs index d35c2b31..bfc338da 100644 --- a/src/llm-coding-tools-agents/src/runtime/state.rs +++ b/src/llm-coding-tools-agents/src/runtime/state.rs @@ -5,10 +5,13 @@ //! - [`AgentRuntime`] — Container for loaded agents, defaults, Task settings, and tools. //! - [`AgentDefaults`] — Fallback settings when an agent doesn't specify them. -use super::task::resolve_allowed_tools; +use super::task::{build_runtime_task_caches, TaskTargetSummary}; use super::tool_catalog::ToolCatalogEntry; -use crate::AgentCatalog; +use crate::{AgentCatalog, RulesetExt}; +use ahash::AHashMap; +use llm_coding_tools_core::permissions::Ruleset; use llm_coding_tools_core::TaskSettings; +use std::sync::Arc; /// Default settings used when an agent doesn't specify them. #[derive(Debug, Clone, Default, PartialEq)] @@ -40,6 +43,9 @@ pub struct AgentRuntime { defaults: AgentDefaults, task_settings: TaskSettings, tools: Vec, + permission_rulesets: AHashMap>, + allowed_tools_by_caller: AHashMap>, + callable_target_summaries_by_caller: AHashMap>, } impl AgentRuntime { @@ -50,11 +56,26 @@ impl AgentRuntime { task_settings: TaskSettings, tools: Vec, ) -> Self { + let permission_rulesets = catalog + .iter() + .map(|agent| { + ( + agent.name.to_string(), + Arc::new(Ruleset::from_permission_config(&agent.permission)), + ) + }) + .collect(); + let (allowed_tools_by_caller, callable_target_summaries_by_caller) = + build_runtime_task_caches(&catalog, &permission_rulesets, &tools); + Self { catalog, defaults, task_settings, tools, + permission_rulesets, + allowed_tools_by_caller, + callable_target_summaries_by_caller, } } @@ -82,13 +103,74 @@ impl AgentRuntime { &self.tools } + /// Returns the cached permission ruleset for the named caller. + /// + /// The returned [`Arc`] is cheap to clone and reuses the ruleset built when + /// the runtime was constructed. + #[inline] + pub fn permission_ruleset(&self, caller_name: &str) -> Option> { + self.permission_rulesets.get(caller_name).cloned() + } + /// Returns the tool entries exposed to the named caller. /// /// Most tools use the standard wildcard permission check (`permission -> "*"`). /// `task` is only included when at least one `mode: all` or `mode: subagent` /// target remains callable after applying `permission.task`. #[inline] - pub fn allowed_tools(&self, caller_name: &str) -> Vec { - resolve_allowed_tools(self, caller_name) + pub fn allowed_tools(&self, caller_name: &str) -> &[ToolCatalogEntry] { + self.allowed_tools_by_caller + .get(caller_name) + .map(Vec::as_slice) + .unwrap_or(&[]) + } + + /// Returns stable summaries for every agent the named caller may delegate to via Task. + /// + /// Only agents with [`AgentMode::All`] or [`AgentMode::Subagent`] are + /// considered. When the caller defines `permission.task`, targets are + /// filtered by those rules; otherwise all non-primary agents are included. + /// Results are sorted alphabetically by target name. + /// + /// # Arguments + /// + /// * `caller_name` - Name of the agent that wants to delegate. + /// + /// # Returns + /// + /// A [`TaskTargetSummary`] per callable target. Empty if `caller_name` + /// is not in the catalog or no targets survive permission filtering. + /// + /// [`AgentMode::All`]: crate::AgentMode::All + /// [`AgentMode::Subagent`]: crate::AgentMode::Subagent + #[inline] + pub fn summarize_callable_targets(&self, caller_name: &str) -> &[TaskTargetSummary] { + self.callable_target_summaries_by_caller + .get(caller_name) + .map(Vec::as_slice) + .unwrap_or(&[]) + } + + /// Returns whether the named caller may delegate to the given target. + /// + /// Looks up the caller's filtered callable-target list and performs a + /// binary search for `target_name`. + /// + /// # Arguments + /// - `caller_name`: Name of the agent that would originate the delegation. + /// - `target_name`: Name of the candidate delegate target. + /// + /// # Returns + /// - `true` if `target_name` appears in the caller's permitted target list. + /// - `false` if the caller is not in the catalog or the target is absent. + #[inline] + pub fn can_delegate_to(&self, caller_name: &str, target_name: &str) -> bool { + self.callable_target_summaries_by_caller + .get(caller_name) + .is_some_and(|summaries| { + summaries + .binary_search_by(|summary| summary.name.as_ref().cmp(target_name)) + .is_ok() + }) } } diff --git a/src/llm-coding-tools-agents/src/runtime/task.rs b/src/llm-coding-tools-agents/src/runtime/task.rs index 974cf498..0820595f 100644 --- a/src/llm-coding-tools-agents/src/runtime/task.rs +++ b/src/llm-coding-tools-agents/src/runtime/task.rs @@ -5,11 +5,12 @@ //! - [`callable_targets`] - Returns the agents an active agent may delegate to via Task. //! - [`TaskTargetSummary`] - Stable Task UI metadata for a callable target. -use super::state::AgentRuntime; use super::tool_catalog::{ToolCatalogEntry, ToolCatalogKind}; use crate::{AgentCatalog, AgentConfig, AgentMode, RulesetExt}; +use ahash::AHashMap; use llm_coding_tools_core::permissions::Ruleset; use llm_coding_tools_core::tool_metadata::task as task_meta; +use std::sync::Arc; /// Compact metadata used to describe one callable Task target. #[derive(Debug, Clone, PartialEq, Eq)] @@ -34,18 +35,7 @@ pub fn summarize_callable_targets( catalog: &AgentCatalog, caller_name: &str, ) -> Vec { - let callable = callable_targets(catalog, caller_name); - let mut summaries = Vec::with_capacity(callable.len()); - - // Copy stable Task metadata into owned summaries. - for target in callable { - summaries.push(TaskTargetSummary { - name: target.name.clone(), - description: target.description.clone(), - }); - } - - summaries + summarize_targets(callable_targets(catalog, caller_name)) } /// Returns the agents that `caller_name` (the currently running agent) may delegate to via Task. @@ -66,20 +56,89 @@ pub fn callable_targets<'a>(catalog: &'a AgentCatalog, caller_name: &str) -> Vec let Some(caller) = catalog.by_name(caller_name) else { return Vec::new(); }; + let task_rules = Ruleset::from_permission_config(&caller.permission); + // Sort to give deterministic ordering regardless of catalog iteration order; + // the result feeds into LLM prompts and cached summaries that must be stable. + let agents = sorted_agents(catalog); + filter_callable_targets(&agents, caller, &task_rules) +} +/// Pre-compute per-agent task caches for the entire catalog in one pass. +/// +/// For every agent in `catalog`, resolves which targets it may delegate to via Task +/// and which tools it is allowed to invoke, then returns both mappings keyed by +/// agent name. +/// +/// # Arguments +/// - `catalog` - All registered agents. +/// - `permission_rulesets` - Pre-built [`Ruleset`] per agent name. Every agent +/// present in `catalog` **must** have an entry or the function panics. +/// - `tools` - The full tool catalog to filter per caller. +/// +/// # Returns +/// A tuple of: +/// - Allowed tools per caller agent name ([`Vec`]). +/// - Callable target summaries per caller agent name ([`Vec`]). +/// +/// # Panics +/// Panics if any agent in `catalog` lacks a corresponding entry in +/// `permission_rulesets`. +pub(super) fn build_runtime_task_caches( + catalog: &AgentCatalog, + permission_rulesets: &AHashMap>, + tools: &[ToolCatalogEntry], +) -> ( + AHashMap>, + AHashMap>, +) { + let mut allowed_tools_by_caller = AHashMap::with_capacity(permission_rulesets.len()); + let mut callable_target_summaries_by_caller = + AHashMap::with_capacity(permission_rulesets.len()); let agents = sorted_agents(catalog); - let task_rules = Ruleset::from_permission_config(&caller.permission); - let has_explicit_task_permission = caller.permission.contains_key(task_meta::NAME); - let mut targets = Vec::with_capacity(agents.len()); - // Keep only non-primary targets that survive `permission.task` filtering. - for target in agents { - if target_is_callable(target, &task_rules, has_explicit_task_permission) { - targets.push(target); - } + for caller in catalog.iter() { + let task_rules = permission_rulesets + .get(caller.name.as_ref()) + .map(Arc::as_ref) + .expect("every runtime agent must have a cached ruleset"); + let callable_targets = filter_callable_targets(&agents, caller, task_rules); + let task_is_callable = !callable_targets.is_empty(); + + callable_target_summaries_by_caller + .insert(caller.name.to_string(), summarize_targets(callable_targets)); + allowed_tools_by_caller.insert( + caller.name.to_string(), + collect_allowed_tools(tools, task_rules, task_is_callable), + ); + } + + (allowed_tools_by_caller, callable_target_summaries_by_caller) +} + +fn summarize_targets(callable: Vec<&AgentConfig>) -> Vec { + let mut summaries = Vec::with_capacity(callable.len()); + + for target in callable { + summaries.push(TaskTargetSummary { + name: target.name.clone(), + description: target.description.clone(), + }); } - targets + summaries +} + +fn filter_callable_targets<'a>( + agents: &[&'a AgentConfig], + caller: &AgentConfig, + task_rules: &Ruleset, +) -> Vec<&'a AgentConfig> { + let has_explicit_task_permission = caller.permission.contains_key(task_meta::NAME); + agents + .iter() + .copied() + .filter(|t| target_is_callable(t, task_rules, has_explicit_task_permission)) + .collect() } fn sorted_agents(catalog: &AgentCatalog) -> Vec<&AgentConfig> { @@ -90,7 +149,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) @@ -98,33 +157,9 @@ fn target_is_callable( || task_rules.is_allowed(task_meta::NAME, target.name.as_ref())) } -pub(super) fn resolve_allowed_tools( - runtime: &AgentRuntime, - caller_name: &str, -) -> Vec { - let Some(caller) = runtime.catalog().by_name(caller_name) else { - return Vec::new(); - }; - - let agents = sorted_agents(runtime.catalog()); - let task_rules = Ruleset::from_permission_config(&caller.permission); - let has_explicit_task_permission = caller.permission.contains_key(task_meta::NAME); - let mut task_is_callable = false; - - // Expose `task` only when at least one delegated target remains callable. - for target in agents { - if target_is_callable(target, &task_rules, has_explicit_task_permission) { - task_is_callable = true; - break; - } - } - - collect_allowed_tools(runtime.tools(), &task_rules, task_is_callable) -} - 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 1ed5b943..9b29c5e1 100644 --- a/src/llm-coding-tools-core/Cargo.toml +++ b/src/llm-coding-tools-core/Cargo.toml @@ -124,3 +124,7 @@ harness = false [[bench]] name = "path_resolvers" harness = false + +[[bench]] +name = "permissions" +harness = false diff --git a/src/llm-coding-tools-core/benches/permissions.rs b/src/llm-coding-tools-core/benches/permissions.rs new file mode 100644 index 00000000..4d80004e --- /dev/null +++ b/src/llm-coding-tools-core/benches/permissions.rs @@ -0,0 +1,150 @@ +//! Benchmarks for permission rule evaluation on the hot path. +//! +//! These benches focus on the checks that run on every gated tool call: +//! [`Ruleset::evaluate`] and [`OptionRulesetExt::check`]. +//! Cases cover exact matches, wildcard permission keys, wildcard subject +//! patterns, and longer rulesets where the winning rule is near the end. + +use core::hint::black_box; +use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion, Throughput}; +use llm_coding_tools_core::permissions::{PermissionAction, Rule, Ruleset}; +use llm_coding_tools_core::permissions_ext::OptionRulesetExt; + +/// A single benchmark scenario for permission rule evaluation. +/// +/// Each case pairs a tool name and subject path with a pre-built [`Ruleset`] +/// so that benches can iterate without constructing fixtures on every sample. +struct PermissionCase { + name: &'static str, + tool_name: &'static str, + subject: &'static str, + ruleset: Ruleset, +} + +/// Build a [`Ruleset`] with `rule_count - 1` deny rules followed by one +/// allow rule, so that the winning rule is always the last entry. +/// +/// # Arguments +/// - `rule_count`: Total number of rules. Must be at least 1. +/// - `final_permission`: Tool-name pattern for the last (allow) rule. +/// - `final_pattern`: Subject pattern for the last (allow) rule. +fn build_ruleset( + rule_count: usize, + final_permission: impl Into>, + final_pattern: impl Into>, +) -> Ruleset { + assert!(rule_count >= 1, "rule_count must be >= 1"); + let mut ruleset = Ruleset::with_capacity(rule_count); + + for idx in 0..(rule_count - 1) { + ruleset.push(Rule::new( + format!("tool-{idx}"), + format!("/workspace/other-{idx}.txt"), + PermissionAction::Deny, + )); + } + + ruleset.push(Rule::new( + final_permission, + final_pattern, + PermissionAction::Allow, + )); + ruleset +} + +/// Return the standard set of permission benchmark cases covering exact +/// matches at three rule-set sizes (1, 32, and 128 rules), plus wildcard +/// subject, wildcard permission key, and combined wildcard cases at +/// 32 rules and a combined wildcard case at 128 rules. +fn benchmark_cases() -> Vec { + vec![ + PermissionCase { + name: "exact_1_rule", + tool_name: "read", + subject: "/workspace/src/lib.rs", + ruleset: build_ruleset(1, "read", "/workspace/src/lib.rs"), + }, + PermissionCase { + name: "exact_32_rules", + tool_name: "read", + subject: "/workspace/src/lib.rs", + ruleset: build_ruleset(32, "read", "/workspace/src/lib.rs"), + }, + PermissionCase { + name: "exact_128_rules", + tool_name: "read", + subject: "/workspace/src/lib.rs", + ruleset: build_ruleset(128, "read", "/workspace/src/lib.rs"), + }, + PermissionCase { + name: "wildcard_subject_32_rules", + tool_name: "read", + subject: "/workspace/src/lib.rs", + ruleset: build_ruleset(32, "read", "/workspace/src/*.rs"), + }, + PermissionCase { + name: "wildcard_permission_32_rules", + tool_name: "read", + subject: "/workspace/src/lib.rs", + ruleset: build_ruleset(32, "re?d", "/workspace/src/lib.rs"), + }, + PermissionCase { + name: "wildcard_both_32_rules", + tool_name: "read", + subject: "/workspace/src/lib.rs", + ruleset: build_ruleset(32, "r*d", "/workspace/src/*.rs"), + }, + PermissionCase { + name: "wildcard_both_128_rules", + tool_name: "read", + subject: "/workspace/src/lib.rs", + ruleset: build_ruleset(128, "r*d", "/workspace/src/*.rs"), + }, + ] +} + +/// Benchmark [`Ruleset::evaluate`] across all [`benchmark_cases`]. +fn bench_ruleset_evaluate(c: &mut Criterion) { + let mut group = c.benchmark_group("permissions/evaluate"); + let cases = benchmark_cases(); + + group.throughput(Throughput::Elements(1)); + + for case in &cases { + group.bench_with_input(BenchmarkId::new("ruleset", case.name), case, |b, case| { + b.iter(|| { + black_box( + case.ruleset + .evaluate(black_box(case.tool_name), black_box(case.subject)), + ) + }) + }); + } + + group.finish(); +} + +/// Benchmark [`OptionRulesetExt::check`] (ruleset lookup plus optional default +/// fallthrough) across all [`benchmark_cases`]. +fn bench_check_permission(c: &mut Criterion) { + let mut group = c.benchmark_group("permissions/check_permission"); + let cases = benchmark_cases(); + + group.throughput(Throughput::Elements(1)); + + for case in &cases { + group.bench_with_input(BenchmarkId::new("ruleset", case.name), case, |b, case| { + b.iter(|| { + Some(black_box(&case.ruleset)) + .check(black_box(case.tool_name), black_box(case.subject)) + .expect("benchmark fixture should be allowed"); + black_box(()) + }) + }); + } + + group.finish(); +} + +criterion_group!(benches, bench_ruleset_evaluate, bench_check_permission); +criterion_main!(benches); diff --git a/src/llm-coding-tools-core/src/error.rs b/src/llm-coding-tools-core/src/error.rs index ead69b2b..9a5b4d5c 100644 --- a/src/llm-coding-tools-core/src/error.rs +++ b/src/llm-coding-tools-core/src/error.rs @@ -54,6 +54,15 @@ pub enum ToolError { /// JSON serialization/deserialization failed. #[error("JSON error: {0}")] Json(#[from] serde_json::Error), + + /// Permission denied for the requested operation. + #[error("permission denied for tool '{tool}' on '{subject}'")] + PermissionDenied { + /// Tool name that was denied. + tool: &'static str, + /// Path or command that was denied. + subject: String, + }, } /// Result type alias for tool operations. diff --git a/src/llm-coding-tools-core/src/lib.rs b/src/llm-coding-tools-core/src/lib.rs index 49c6d3d0..8371d24b 100644 --- a/src/llm-coding-tools-core/src/lib.rs +++ b/src/llm-coding-tools-core/src/lib.rs @@ -15,6 +15,7 @@ pub mod models; pub mod output; pub mod path; pub mod permissions; +pub mod permissions_ext; pub mod system_prompt; pub mod tool_metadata; pub mod tools; @@ -33,10 +34,10 @@ pub use system_prompt::SystemPromptBuilder; pub use tools::{ edit_file, execute_command, execute_command_with_mode, glob_files, grep_search, read_file, read_todos, write_file, write_todos, BashExecutionMode, BashOutput, BashRequest, BashSettings, - EditError, EditRequest, GlobOutput, GlobRequest, GlobSettings, GrepFileMatches, + EditError, EditRequest, EditSettings, GlobOutput, GlobRequest, GlobSettings, GrepFileMatches, GrepFormattingSettings, GrepLineMatch, GrepOutput, GrepRequest, GrepSettings, ReadRequest, ReadSettings, TaskInput, TaskOutput, TaskSettings, Todo, TodoPriority, TodoReadRequest, - TodoState, TodoStatus, TodoWriteRequest, WriteRequest, + TodoState, TodoStatus, TodoWriteRequest, WriteRequest, WriteSettings, }; // Re-export Linux sandbox types (Linux-only, requires linux-bubblewrap feature) diff --git a/src/llm-coding-tools-core/src/path/allowed_glob/policy.rs b/src/llm-coding-tools-core/src/path/allowed_glob/policy.rs index 0738dc95..c7e33e48 100644 --- a/src/llm-coding-tools-core/src/path/allowed_glob/policy.rs +++ b/src/llm-coding-tools-core/src/path/allowed_glob/policy.rs @@ -76,11 +76,17 @@ impl GlobPolicy { /// # Returns /// /// `true` if the path is allowed by the last matching rule, `false` otherwise. + #[inline] pub(crate) fn is_allowed(&self, normalized_path: &str) -> bool { if self.rules.is_empty() { return false; } + // Single-rule fast path: skip GlobSet + loop when there's only one rule. + if let [(matcher, action)] = self.rules.as_slice() { + return matcher.is_match(normalized_path) && matches!(action, RuleAction::Allow); + } + // Speedup: Match against all globs at once. if !self.glob_set.is_match(normalized_path) { return false; diff --git a/src/llm-coding-tools-core/src/permissions.rs b/src/llm-coding-tools-core/src/permissions.rs index 2b5e3515..2ff65199 100644 --- a/src/llm-coding-tools-core/src/permissions.rs +++ b/src/llm-coding-tools-core/src/permissions.rs @@ -77,38 +77,23 @@ pub enum PermissionAction { /// # Memory Layout /// /// Size is 56 bytes on 64-bit platforms: -/// - `permission`: 16 bytes (&str ptr + len) -/// - `pattern`: 16 bytes (&str ptr + len) +/// - `permission`: 16 bytes (`Box` ptr + len) +/// - `pattern`: 16 bytes (`Box` 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> { +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Rule { /// The permission key pattern (e.g., "bash", "*", "task-*") - permission: &'a str, + permission: Box, /// The subject pattern (e.g., "*", "orchestrator-*") - pattern: &'a str, + pattern: Box, /// Pre-computed hash of `permission` for fast exact-match comparison permission_hash: Hash64, - /// Pre-computed hash of `pattern` for potential fast-path usage + /// Pre-computed hash of `pattern` for fast exact-match comparison pattern_hash: Hash64, /// Whether `permission` uses `*` (any number of chars) or `?` (one char). permission_is_wildcard: bool, @@ -118,7 +103,7 @@ pub struct Rule<'a> { action: PermissionAction, } -impl<'a> Rule<'a> { +impl Rule { /// Creates a new rule with the provided permission and pattern. /// /// Permission keys with `*` or `?` are treated as patterns. @@ -136,29 +121,34 @@ impl<'a> Rule<'a> { /// // Wildcard permission key matches any tool /// let wildcard = Rule::new("*", "*", PermissionAction::Allow); /// ``` - #[inline] - pub fn new(permission: &'a str, pattern: &'a str, action: PermissionAction) -> Self { + pub fn new( + permission: impl Into>, + pattern: impl Into>, + action: PermissionAction, + ) -> Self { + let permission = permission.into(); + let pattern = pattern.into(); Self { - permission, - pattern, - permission_hash: hash_u64(permission), - pattern_hash: hash_u64(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('?'), + permission, + pattern, action, } } /// Returns the permission key pattern. #[inline] - pub fn permission(&self) -> &'a str { - self.permission + pub fn permission(&self) -> &str { + &self.permission } /// Returns the stored pattern. #[inline] - pub fn pattern(&self) -> &'a str { - self.pattern + pub fn pattern(&self) -> &str { + &self.pattern } /// Returns the action for this rule. @@ -173,6 +163,12 @@ impl<'a> Rule<'a> { self.permission_hash.as_u64() } + /// Returns the stored 64-bit pattern hash. + #[inline] + pub fn pattern_hash(&self) -> u64 { + self.pattern_hash.as_u64() + } + /// Returns true if the permission key contains wildcards. #[inline] pub fn permission_is_wildcard(&self) -> bool { @@ -192,12 +188,12 @@ impl<'a> Rule<'a> { /// /// 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<'a> { - rules: Vec>, +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct Ruleset { + rules: Vec, } -impl<'a> Ruleset<'a> { +impl Ruleset { /// Creates an empty ruleset. #[inline] pub fn new() -> Self { @@ -214,7 +210,7 @@ impl<'a> Ruleset<'a> { /// Appends a rule to the ruleset. #[inline] - pub fn push(&mut self, rule: Rule<'a>) { + pub fn push(&mut self, rule: Rule) { self.rules.push(rule); } @@ -232,7 +228,7 @@ impl<'a> Ruleset<'a> { /// Returns an iterator over the rules. #[inline] - pub fn iter(&self) -> impl Iterator> { + pub fn iter(&self) -> impl Iterator { self.rules.iter() } @@ -251,39 +247,41 @@ impl<'a> Ruleset<'a> { /// `*` 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 { + match self.rules.as_slice() { + [] => return PermissionAction::Deny, + [rule] => return evaluate_single_rule(rule, permission, subject), + _ => {} + } + let permission_hash = hash_u64(permission); - let subject_hash = hash_u64(subject); - let mut result = PermissionAction::Deny; + let mut subject_hash = None; - for rule in &self.rules { - let permission_matches = rule_matches( + // Last-match-wins means the hottest successful path is the tail of the + // ruleset, so scan in reverse and exit on the first match. + for rule in self.rules.iter().rev() { + if !rule_matches( permission, permission_hash, - rule.permission, + &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, - ); + let pattern_matches = if rule.pattern_is_wildcard { + wildcard_match(subject, &rule.pattern) + } else { + let subject_hash = *subject_hash.get_or_insert_with(|| hash_u64(subject)); + rule.pattern_hash == subject_hash && &*rule.pattern == subject + }; - if !pattern_matches { - continue; + if pattern_matches { + return rule.action; } - - result = rule.action; } - result + PermissionAction::Deny } /// Checks if a permission is allowed for the given subject. @@ -299,18 +297,15 @@ impl<'a> Ruleset<'a> { /// /// Rules from `other` are appended in order, giving them higher priority /// in last-match-wins evaluation. - pub fn merge(&mut self, other: &Ruleset<'a>) { + pub fn merge(&mut self, other: &Ruleset) { self.rules.reserve(other.rules.len()); - self.rules.extend_from_slice(&other.rules); + self.rules.extend(other.rules.iter().cloned()); } /// Creates a new ruleset by merging multiple rulesets. /// /// Rules are concatenated in order; later rulesets have higher priority. - pub fn merged<'b>(rulesets: impl IntoIterator>) -> Self - where - 'a: 'b, - { + pub fn merged<'a>(rulesets: impl IntoIterator) -> Self { let rulesets: Vec<_> = rulesets.into_iter().collect(); let capacity = rulesets.iter().map(|r| r.len()).sum(); let mut result = Self::with_capacity(capacity); @@ -334,7 +329,7 @@ impl<'a> Ruleset<'a> { /// assert!(wildcard_match("test", "te?t")); /// assert!(!wildcard_match("bash", "task")); /// ``` -pub(crate) fn wildcard_match(input: &str, pattern: &str) -> bool { +pub fn wildcard_match(input: &str, pattern: &str) -> bool { // Fast path: exact match or universal wildcard if pattern == "*" { return true; @@ -407,6 +402,32 @@ fn rule_matches( } } +#[inline(always)] +fn evaluate_single_rule(rule: &Rule, permission: &str, subject: &str) -> PermissionAction { + let permission_hash = hash_u64(permission); + if !rule_matches( + permission, + permission_hash, + &rule.permission, + rule.permission_hash, + rule.permission_is_wildcard, + ) { + return PermissionAction::Deny; + } + + let pattern_matches = if rule.pattern_is_wildcard { + wildcard_match(subject, &rule.pattern) + } else { + rule.pattern_hash == hash_u64(subject) && &*rule.pattern == subject + }; + + if pattern_matches { + rule.action + } else { + PermissionAction::Deny + } +} + #[cfg(test)] mod tests { use super::*; @@ -419,7 +440,7 @@ mod tests { ) -> PermissionAction { let mut ruleset = Ruleset::new(); for (perm, pat, action) in rules { - ruleset.push(Rule::new(perm, pat, *action)); + ruleset.push(Rule::new(*perm, *pat, *action)); } ruleset.evaluate(permission, subject) } @@ -512,6 +533,7 @@ mod tests { fn wildcard_subject_should_set_wildcard_flag() { let rule = Rule::new("bash", "orchestrator-*", PermissionAction::Allow); assert_eq!(rule.pattern(), "orchestrator-*"); + assert_eq!(rule.pattern_hash(), hash_u64("orchestrator-*").as_u64()); assert!(rule.pattern_is_wildcard()); assert!(!rule.permission_is_wildcard()); } @@ -521,6 +543,7 @@ mod tests { fn exact_subject_should_not_set_wildcard_flag() { let rule = Rule::new("bash", "exact-subject", PermissionAction::Allow); assert_eq!(rule.pattern(), "exact-subject"); + assert_eq!(rule.pattern_hash(), hash_u64("exact-subject").as_u64()); assert!(!rule.pattern_is_wildcard()); assert!(!rule.permission_is_wildcard()); } @@ -533,10 +556,18 @@ mod tests { } #[test] - fn rule_should_be_56_byte_copy() { - assert_eq!(std::mem::size_of::>(), 56); - fn assert_copy() {} - assert_copy::>(); + fn rule_pattern_hash_should_be_case_sensitive() { + let upper = Rule::new("bash", "SUBJECT", PermissionAction::Allow); + let lower = Rule::new("bash", "subject", PermissionAction::Allow); + assert_ne!(upper.pattern_hash(), lower.pattern_hash()); + } + + #[test] + fn rule_should_be_56_byte_clone() { + assert_eq!(std::mem::size_of::(), 56); + // Rule is Clone but not Copy (contains owned Box) + fn assert_clone() {} + assert_clone::(); } // --- Ruleset evaluate --- diff --git a/src/llm-coding-tools-core/src/permissions_ext.rs b/src/llm-coding-tools-core/src/permissions_ext.rs new file mode 100644 index 00000000..df61087c --- /dev/null +++ b/src/llm-coding-tools-core/src/permissions_ext.rs @@ -0,0 +1,94 @@ +//! Extension trait for optional ruleset permission checking. +//! +//! Provides a convenient way to check permissions on an optional ruleset, +//! returning `Ok(())` if no ruleset is configured. +//! +//! # Example +//! +//! ``` +//! use llm_coding_tools_core::permissions::{PermissionAction, Rule, Ruleset}; +//! use llm_coding_tools_core::permissions_ext::OptionRulesetExt; +//! +//! // With ruleset configured to allow all +//! let mut ruleset = Ruleset::new(); +//! ruleset.push(Rule::new("*", "*", PermissionAction::Allow)); +//! let result: Option<&Ruleset> = Some(&ruleset); +//! result.check("bash", "some-command").unwrap(); +//! +//! // Without ruleset (always allows) +//! let no_ruleset: Option<&Ruleset> = None; +//! no_ruleset.check("bash", "any-command").unwrap(); // Always Ok(()) +//! ``` + +use crate::error::{ToolError, ToolResult}; +use crate::permissions::{PermissionAction, Ruleset}; + +/// Extension trait for optional ruleset permission checking. +/// +/// Provides a convenient way to check permissions on an optional ruleset, +/// returning `Ok(())` if no ruleset is configured. +pub trait OptionRulesetExt { + /// Checks if the given subject is allowed, returning an error if denied. + /// + /// Returns `Ok(())` if no ruleset is configured or access is allowed. + fn check(&self, tool_name: &'static str, subject: &str) -> ToolResult<()>; +} + +impl OptionRulesetExt for Option<&Ruleset> { + #[inline(always)] + fn check(&self, tool_name: &'static str, subject: &str) -> ToolResult<()> { + match self { + Some(ruleset) => { + if ruleset.evaluate(tool_name, subject) == PermissionAction::Deny { + Err(ToolError::PermissionDenied { + tool: tool_name, + subject: subject.to_string(), + }) + } else { + Ok(()) + } + } + None => Ok(()), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::permissions::Rule; + + #[test] + fn option_ruleset_ext_without_ruleset_allows_access() { + let no_ruleset: Option<&Ruleset> = None; + assert!(no_ruleset.check("read", "/tmp/file.txt").is_ok()); + } + + #[test] + fn option_ruleset_ext_returns_permission_denied_for_denied_subject() { + let mut ruleset = Ruleset::new(); + ruleset.push(Rule::new( + "read", + "/tmp/allowed.txt", + PermissionAction::Allow, + )); + + let err = Some(&ruleset).check("read", "/tmp/denied.txt").unwrap_err(); + + assert!(matches!( + err, + ToolError::PermissionDenied { + tool: "read", + subject, + } if subject == "/tmp/denied.txt" + )); + } + + #[test] + fn option_ruleset_ext_returns_ok_for_allowed() { + let mut ruleset = Ruleset::new(); + ruleset.push(Rule::new("read", "*", PermissionAction::Allow)); + + assert!(Some(&ruleset).check("read", "/tmp/file.txt").is_ok()); + } +} diff --git a/src/llm-coding-tools-core/src/tools/bash/blocking_impl.rs b/src/llm-coding-tools-core/src/tools/bash/blocking_impl.rs index 0043b6ae..ff508ebc 100644 --- a/src/llm-coding-tools-core/src/tools/bash/blocking_impl.rs +++ b/src/llm-coding-tools-core/src/tools/bash/blocking_impl.rs @@ -6,6 +6,8 @@ use super::{ PIPE_BUFFER_CAPACITY, }; use crate::error::{ToolError, ToolResult}; +use crate::permissions_ext::OptionRulesetExt; +use crate::tool_metadata::bash as bash_meta; #[cfg(all(feature = "linux-bubblewrap", target_os = "linux"))] use llm_coding_tools_bubblewrap::wrap::blocking as linux_bwrap_wrap; use process_wrap::std::*; @@ -33,6 +35,7 @@ enum WaitOutcome { /// - Unix: Process groups /// /// # Errors +/// - Returns [`ToolError::PermissionDenied`] when the command is blocked by `settings.permission`. /// - Returns `ToolError::Validation` if timeout is 0 or exceeds max_timeout_ms. /// - Returns [`ToolError::InvalidPath`] if workdir is not absolute or doesn't exist. /// - Returns [`ToolError::Execution`] for sandbox mode when bwrap is missing or unusable. @@ -42,6 +45,10 @@ pub fn execute_command( request: super::BashRequest, settings: super::BashSettings<'_>, ) -> ToolResult { + settings + .permission + .check(bash_meta::NAME, &request.command)?; + let workdir = request .workdir .as_deref() @@ -224,6 +231,8 @@ fn build_host_wrap(command: &str, workdir: Option<&Path>) -> ToolResult { /// Default timeout when omitted from the request. @@ -90,6 +95,10 @@ pub struct BashSettings<'a> { pub max_timeout_ms: u32, /// Default working directory when omitted from the request. pub default_workdir: Option<&'a Path>, + /// Optional permission ruleset applied to command strings. + /// + /// When set, blocked commands return [`ToolError::PermissionDenied`]. + pub permission: Option<&'a Ruleset>, } /// Default buffer capacity for stdout/stderr pipe reads. diff --git a/src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs b/src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs index c6736773..ef72a4c6 100644 --- a/src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs +++ b/src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs @@ -6,6 +6,8 @@ use super::{ PIPE_BUFFER_CAPACITY, }; use crate::error::{ToolError, ToolResult}; +use crate::permissions_ext::OptionRulesetExt; +use crate::tool_metadata::bash as bash_meta; #[cfg(all(feature = "linux-bubblewrap", target_os = "linux"))] use llm_coding_tools_bubblewrap::wrap::tokio as linux_bwrap_wrap; use parking_lot::Mutex; @@ -91,6 +93,7 @@ async fn await_pipe_drain_task_with_grace(task: PipeDrainTask, grace: Duration) /// - Unix: Process groups /// /// # Errors +/// - Returns [`ToolError::PermissionDenied`] when the command is blocked by `settings.permission`. /// - Returns `ToolError::Validation` if timeout is 0 or exceeds max_timeout_ms. /// - Returns [`ToolError::InvalidPath`] if workdir is not absolute or doesn't exist. /// - Returns [`ToolError::Execution`] for sandbox mode when bwrap is missing or unusable. @@ -100,6 +103,10 @@ pub async fn execute_command( request: super::BashRequest, settings: super::BashSettings<'_>, ) -> ToolResult { + settings + .permission + .check(bash_meta::NAME, &request.command)?; + let workdir = request .workdir .as_deref() @@ -265,6 +272,8 @@ fn build_host_wrap(command: &str, workdir: Option<&Path>) -> ToolResult for ToolError { } } +/// Runtime settings that control permission filtering for edit requests. +/// +/// Wraps an optional [`Ruleset`] that gates which paths an [`edit_file`] +/// operation may target. +/// +/// [`Ruleset`]: crate::permissions::Ruleset +/// [`edit_file`]: edit_file +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct EditSettings { + permission: Option>, +} + +impl EditSettings { + /// Creates default edit settings with no extra permission filtering. + /// + /// # Returns + /// - An [`EditSettings`] with permission set to `None`. + #[must_use] + pub fn new() -> Self { + Self { permission: None } + } + + /// Attaches an optional permission ruleset to edit operations. + /// + /// # Arguments + /// - `permission` - An optional [`Arc`] controlling which paths + /// may be edited. Pass `None` to disable permission filtering. + /// + /// # Returns + /// - The modified [`EditSettings`] with the permission attached. + /// + /// [`Arc`]: std::sync::Arc + #[must_use] + pub fn with_permission(mut self, permission: Option>) -> Self { + self.permission = permission; + self + } + + /// Returns the permission ruleset applied to edit operations, if any. + /// + /// # Returns + /// - `Some(&`[`Ruleset`]`)` when a permission filter is configured. + /// - `None` when no permission filtering is applied. + /// + /// [`Ruleset`]: crate::permissions::Ruleset + #[must_use] + pub fn permission(&self) -> Option<&Ruleset> { + self.permission.as_deref() + } +} + /// Performs exact string replacement in a file. /// /// Returns success message with replacement count. @@ -84,6 +138,7 @@ impl From for ToolError { pub async fn edit_file( resolver: &R, request: EditRequest, + settings: &EditSettings, ) -> Result { if request.old_string.is_empty() { return Err(EditError::EmptyOldString); @@ -93,6 +148,10 @@ pub async fn edit_file( } let path = resolver.resolve(&request.file_path)?; + let subject = path.to_string_lossy(); + settings + .permission() + .check(edit_meta::NAME, subject.as_ref())?; let content = fs::read_to_string(&path).await?; let (new_content, replacement_count) = if request.replace_all { @@ -142,6 +201,7 @@ pub async fn edit_file( mod tests { use super::*; use crate::path::AbsolutePathResolver; + use crate::permissions::{PermissionAction, Rule}; use std::io::Write; use tempfile::NamedTempFile; @@ -165,6 +225,7 @@ mod tests { new_string: "rust".to_string(), replace_all: false, }, + &EditSettings::new(), ) .await .unwrap(); @@ -187,9 +248,42 @@ mod tests { new_string: "x".to_string(), replace_all: false, }, + &EditSettings::new(), ) .await .unwrap_err(); assert!(matches!(err, EditError::NotFound)); } + + #[maybe_async::test(feature = "blocking", async(feature = "tokio", tokio::test))] + async fn edit_rejects_denied_path() { + let file = create_temp_file("hello world"); + let resolver = AbsolutePathResolver; + + let mut ruleset = Ruleset::new(); + ruleset.push(Rule::new("edit", "*", PermissionAction::Allow)); + ruleset.push(Rule::new( + "edit", + file.path().to_string_lossy().into_owned(), + PermissionAction::Deny, + )); + + let err = edit_file( + &resolver, + EditRequest { + file_path: file.path().to_string_lossy().into_owned(), + old_string: "world".to_string(), + new_string: "rust".to_string(), + replace_all: false, + }, + &EditSettings::new().with_permission(Some(Arc::new(ruleset))), + ) + .await + .unwrap_err(); + + assert!(matches!( + err, + EditError::Tool(ToolError::PermissionDenied { tool: "edit", .. }) + )); + } } diff --git a/src/llm-coding-tools-core/src/tools/glob.rs b/src/llm-coding-tools-core/src/tools/glob.rs index 37737117..81cb23ab 100644 --- a/src/llm-coding-tools-core/src/tools/glob.rs +++ b/src/llm-coding-tools-core/src/tools/glob.rs @@ -2,11 +2,14 @@ use crate::error::{ToolError, ToolResult}; use crate::path::PathResolver; +use crate::permissions::Ruleset; +use crate::permissions_ext::OptionRulesetExt; use crate::tool_metadata::glob as glob_meta; use globset::Glob; use ignore::WalkBuilder; use serde::{Deserialize, Serialize}; use serde_json::Value; +use std::sync::Arc; use std::time::SystemTime; /// Serde-friendly glob request owned by the core crate. @@ -26,9 +29,10 @@ impl GlobRequest { /// Runtime settings applied to glob requests. /// /// The `limit` field caps the number of file paths returned. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct GlobSettings { limit: usize, + permission: Option>, } impl Default for GlobSettings { @@ -40,9 +44,10 @@ impl Default for GlobSettings { impl GlobSettings { /// Creates valid glob settings with the standard result limit. #[must_use] - pub const fn new() -> Self { + pub fn new() -> Self { Self { limit: glob_meta::MAX_RESULTS, + permission: None, } } @@ -64,11 +69,42 @@ impl GlobSettings { Ok(self) } + /// Attaches an optional permission ruleset to glob operations. + /// + /// # Arguments + /// - `permission` - An optional [`Arc`] controlling which paths + /// may be traversed. Pass `None` to disable permission filtering. + /// + /// # Returns + /// - The modified [`GlobSettings`] with the permission attached. + /// + /// [`Arc`]: std::sync::Arc + #[must_use] + pub fn with_permission(mut self, permission: Option>) -> Self { + self.permission = permission; + self + } + /// Returns the maximum number of files to return. + /// + /// # Returns + /// - The configured result limit. #[must_use] - pub const fn limit(self) -> usize { + pub const fn limit(&self) -> usize { self.limit } + + /// Returns the permission ruleset applied to glob operations, if any. + /// + /// # Returns + /// - `Some(&`[`Ruleset`]`)` when a permission filter is configured. + /// - `None` when no permission filtering is applied. + /// + /// [`Ruleset`]: crate::permissions::Ruleset + #[must_use] + pub fn permission(&self) -> Option<&Ruleset> { + self.permission.as_deref() + } } /// Output from glob file matching. @@ -94,9 +130,13 @@ pub struct GlobOutput { pub fn glob_files( resolver: &R, request: GlobRequest, - settings: GlobSettings, + settings: &GlobSettings, ) -> ToolResult { let path = resolver.resolve(&request.path)?; + let search_subject = path.to_string_lossy(); + settings + .permission() + .check(glob_meta::NAME, search_subject.as_ref())?; if !path.is_dir() { return Err(ToolError::InvalidPath(format!( @@ -160,6 +200,15 @@ pub fn glob_files( continue; } + // If target is in a location it's not allowed to access, it needs + // to be filtered out. + let entry_subject = entry.path().to_string_lossy(); + if let Some(ruleset) = settings.permission() { + if !ruleset.is_allowed(glob_meta::NAME, entry_subject.as_ref()) { + continue; + } + } + let mtime = entry .metadata() .ok() @@ -191,6 +240,7 @@ pub fn glob_files( mod tests { use super::*; use crate::path::AbsolutePathResolver; + use crate::permissions::{PermissionAction, Rule}; use rstest::rstest; use std::fs::{self, File, FileTimes}; use std::io::Write; @@ -242,7 +292,7 @@ mod tests { pattern: pattern.to_string(), path: dir.path().to_str().unwrap().to_string(), }, - GlobSettings::new().with_limit(1000).unwrap(), + &GlobSettings::new().with_limit(1000).unwrap(), ) .unwrap(); @@ -333,7 +383,7 @@ mod tests { pattern: "**/*.txt".to_string(), path: base.to_str().unwrap().to_string(), }, - GlobSettings::new().with_limit(1000).unwrap(), + &GlobSettings::new().with_limit(1000).unwrap(), ) .unwrap(); @@ -367,7 +417,7 @@ mod tests { pattern: "**/*.rs".to_string(), path: dir.path().to_str().unwrap().to_string(), }, - GlobSettings::new().with_limit(1000).unwrap(), + &GlobSettings::new().with_limit(1000).unwrap(), ) .unwrap(); @@ -381,4 +431,34 @@ mod tests { } assert!(result.files.iter().any(|f| f.contains('/'))); } + + #[test] + fn glob_filters_denied_files_before_returning_output() { + let dir = TempDir::new().unwrap(); + let resolver = AbsolutePathResolver; + let allowed = dir.path().join("allowed.txt"); + let denied = dir.path().join("denied.txt"); + File::create(&allowed).unwrap(); + File::create(&denied).unwrap(); + + let mut ruleset = Ruleset::new(); + ruleset.push(Rule::new(glob_meta::NAME, "*", PermissionAction::Allow)); + ruleset.push(Rule::new( + glob_meta::NAME, + denied.to_string_lossy().into_owned(), + PermissionAction::Deny, + )); + + let result = glob_files( + &resolver, + GlobRequest { + pattern: "**/*.txt".to_string(), + path: dir.path().to_string_lossy().into_owned(), + }, + &GlobSettings::new().with_permission(Some(Arc::new(ruleset))), + ) + .unwrap(); + + assert_eq!(result.files, vec!["allowed.txt".to_string()]); + } } diff --git a/src/llm-coding-tools-core/src/tools/grep.rs b/src/llm-coding-tools-core/src/tools/grep.rs index bf74ed1f..6c78ec7e 100644 --- a/src/llm-coding-tools-core/src/tools/grep.rs +++ b/src/llm-coding-tools-core/src/tools/grep.rs @@ -2,6 +2,8 @@ use crate::error::{ToolError, ToolResult}; use crate::path::PathResolver; +use crate::permissions::Ruleset; +use crate::permissions_ext::OptionRulesetExt; use crate::tool_metadata::grep as grep_meta; use crate::util::{truncate_line_with_ellipsis, TRUNCATION_ELLIPSIS}; use globset::Glob; @@ -13,6 +15,7 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; use std::fmt::Write; use std::path::Path; +use std::sync::Arc; use std::time::SystemTime; /// Default maximum line length (in characters) for formatted grep output. @@ -43,9 +46,10 @@ impl GrepRequest { /// /// The `max_limit` field caps the number of matching lines returned, even if /// the caller requests more. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct GrepSettings { max_limit: usize, + permission: Option>, } impl Default for GrepSettings { @@ -57,9 +61,10 @@ impl Default for GrepSettings { impl GrepSettings { /// Creates valid grep search settings with the standard defaults. #[must_use] - pub const fn new() -> Self { + pub fn new() -> Self { Self { max_limit: grep_meta::DEFAULT_LIMIT, + permission: None, } } @@ -81,11 +86,42 @@ impl GrepSettings { Ok(self) } + /// Attaches an optional permission ruleset to grep operations. + /// + /// # Arguments + /// - `permission` - An optional [`Arc`] controlling which paths + /// may be searched. Pass `None` to disable permission filtering. + /// + /// # Returns + /// - The modified [`GrepSettings`] with the permission attached. + /// + /// [`Arc`]: std::sync::Arc + #[must_use] + pub fn with_permission(mut self, permission: Option>) -> Self { + self.permission = permission; + self + } + /// Returns the upper bound on matching lines returned per search. + /// + /// # Returns + /// - The configured maximum line limit. #[must_use] - pub const fn max_limit(self) -> usize { + pub const fn max_limit(&self) -> usize { self.max_limit } + + /// Returns the permission ruleset applied to grep operations, if any. + /// + /// # Returns + /// - `Some(&`[`Ruleset`]`)` when a permission filter is configured. + /// - `None` when no permission filtering is applied. + /// + /// [`Ruleset`]: crate::permissions::Ruleset + #[must_use] + pub fn permission(&self) -> Option<&Ruleset> { + self.permission.as_deref() + } } /// Formatting settings for rendered grep output. @@ -254,7 +290,7 @@ impl GrepOutput { pub fn grep_search( resolver: &R, request: GrepRequest, - settings: GrepSettings, + settings: &GrepSettings, ) -> ToolResult { let pattern = request.pattern.trim(); if pattern.is_empty() { @@ -282,6 +318,10 @@ pub fn grep_search( }); let path = resolver.resolve(&request.path)?; + let search_subject = path.to_string_lossy(); + settings + .permission() + .check(grep_meta::NAME, search_subject.as_ref())?; let matcher = RegexMatcher::new(pattern).map_err(|e| ToolError::InvalidPattern(e.to_string()))?; @@ -322,6 +362,15 @@ pub fn grep_search( let entry_path = entry.path(); + // If target is in a location it's not allowed to access, it needs + // to be filtered out. + if let Some(ruleset) = settings.permission() { + let subject = entry_path.to_string_lossy(); + if !ruleset.is_allowed(grep_meta::NAME, subject.as_ref()) { + continue; + } + } + // Apply include glob to basename when requested. if let Some(ref matcher) = glob_matcher { let file_name = match entry_path.file_name().and_then(|n| n.to_str()) { @@ -423,6 +472,7 @@ fn collect_file_matches( mod tests { use super::*; use crate::path::AbsolutePathResolver; + use crate::permissions::{PermissionAction, Rule}; use rstest::rstest; use tempfile::tempdir; @@ -511,7 +561,7 @@ mod tests { include: include.map(|s| s.to_string()), limit: None, }, - GrepSettings::new().with_max_limit(10).unwrap(), + &GrepSettings::new().with_max_limit(10).unwrap(), ) .unwrap(); @@ -675,7 +725,7 @@ mod tests { include: None, limit: None, }, - GrepSettings::new().with_max_limit(10).unwrap(), + &GrepSettings::new().with_max_limit(10).unwrap(), ) .unwrap(); @@ -699,7 +749,7 @@ mod tests { include: None, limit: None, }, - GrepSettings::new().with_max_limit(10).unwrap(), + &GrepSettings::new().with_max_limit(10).unwrap(), ) .unwrap_err(); @@ -725,7 +775,7 @@ mod tests { include: Some(" ".into()), limit: Some(1), }, - GrepSettings::new().with_max_limit(1).unwrap(), + &GrepSettings::new().with_max_limit(1).unwrap(), ) .unwrap(); @@ -753,7 +803,7 @@ mod tests { include: None, limit: Some(100), // Request asks for 100 matches }, - GrepSettings::new().with_max_limit(2).unwrap(), // But max_limit is only 2 + &GrepSettings::new().with_max_limit(2).unwrap(), // But max_limit is only 2 ) .unwrap(); @@ -762,4 +812,38 @@ mod tests { assert_eq!(result.match_count, 2); assert!(result.truncated); } + + #[test] + fn grep_skips_denied_files_before_counting_matches() { + let temp = tempdir().unwrap(); + let allowed = temp.path().join("allowed.txt"); + let denied = temp.path().join("denied.txt"); + std::fs::write(&allowed, "hello\n").unwrap(); + std::fs::write(&denied, "hello\n").unwrap(); + let resolver = AbsolutePathResolver; + + let mut ruleset = Ruleset::new(); + ruleset.push(Rule::new(grep_meta::NAME, "*", PermissionAction::Allow)); + ruleset.push(Rule::new( + grep_meta::NAME, + denied.to_string_lossy().into_owned(), + PermissionAction::Deny, + )); + + let result = grep_search( + &resolver, + GrepRequest { + pattern: "hello".into(), + path: temp.path().to_string_lossy().into_owned(), + include: None, + limit: None, + }, + &GrepSettings::new().with_permission(Some(Arc::new(ruleset))), + ) + .unwrap(); + + assert_eq!(result.match_count, 1); + assert_eq!(result.files.len(), 1); + assert_eq!(result.files[0].path, allowed.to_string_lossy()); + } } diff --git a/src/llm-coding-tools-core/src/tools/mod.rs b/src/llm-coding-tools-core/src/tools/mod.rs index d3accd7f..ba3117a0 100644 --- a/src/llm-coding-tools-core/src/tools/mod.rs +++ b/src/llm-coding-tools-core/src/tools/mod.rs @@ -20,7 +20,7 @@ pub use bash::{ execute_command, execute_command_with_mode, BashExecutionMode, BashOutput, BashRequest, BashSettings, }; -pub use edit::{edit_file, EditError, EditRequest}; +pub use edit::{edit_file, EditError, EditRequest, EditSettings}; pub use glob::{glob_files, GlobOutput, GlobRequest, GlobSettings}; pub use grep::{ grep_search, GrepFileMatches, GrepFormattingSettings, GrepLineMatch, GrepOutput, GrepRequest, @@ -32,7 +32,7 @@ pub use todo::{ read_todos, write_todos, Todo, TodoPriority, TodoReadRequest, TodoState, TodoStatus, TodoWriteRequest, }; -pub use write::{write_file, WriteRequest}; +pub use write::{write_file, WriteRequest, WriteSettings}; // Webfetch available in both tokio and blocking modes #[cfg(any(feature = "tokio", feature = "blocking"))] diff --git a/src/llm-coding-tools-core/src/tools/read.rs b/src/llm-coding-tools-core/src/tools/read.rs index 64922868..ac7445d8 100644 --- a/src/llm-coding-tools-core/src/tools/read.rs +++ b/src/llm-coding-tools-core/src/tools/read.rs @@ -4,6 +4,8 @@ use crate::error::{ToolError, ToolResult}; use crate::fs; use crate::output::ToolOutput; use crate::path::PathResolver; +use crate::permissions::Ruleset; +use crate::permissions_ext::OptionRulesetExt; use crate::tool_metadata::read as read_meta; use crate::util::{truncate_line_with_ellipsis, ESTIMATED_CHARS_PER_LINE, TRUNCATION_ELLIPSIS}; use memchr::memchr; @@ -11,6 +13,7 @@ use serde::Deserialize; use serde_json::Value; use std::borrow::Cow; use std::fmt::Write; +use std::sync::Arc; /// Strips trailing CR from a line (for CRLF handling). #[inline] @@ -73,12 +76,13 @@ impl ReadRequest { /// - **Max limit**: hard cap applied regardless of what the caller requests. /// /// Additional settings control per-line truncation and line-number display. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct ReadSettings { default_limit: usize, max_limit: usize, max_line_length: usize, line_numbers: bool, + permission: Option>, } impl Default for ReadSettings { @@ -90,12 +94,13 @@ impl Default for ReadSettings { impl ReadSettings { /// Creates valid read settings with the standard defaults. #[must_use] - pub const fn new() -> Self { + pub fn new() -> Self { Self { default_limit: read_meta::DEFAULT_LIMIT, max_limit: read_meta::DEFAULT_LIMIT, max_line_length: read_meta::MAX_LINE_LENGTH, line_numbers: true, + permission: None, } } @@ -132,7 +137,8 @@ impl ReadSettings { /// /// [`MIN_LIMIT`]: crate::util::MIN_LIMIT pub fn with_default_limit(self, default_limit: usize) -> ToolResult { - self.with_limits(default_limit, self.max_limit) + let max_limit = self.max_limit; + self.with_limits(default_limit, max_limit) } /// Sets the hard cap on lines returned regardless of what the caller @@ -146,7 +152,8 @@ impl ReadSettings { /// /// [`MIN_LIMIT`]: crate::util::MIN_LIMIT pub fn with_max_limit(self, max_limit: usize) -> ToolResult { - self.with_limits(self.default_limit, max_limit) + let default_limit = self.default_limit; + self.with_limits(default_limit, max_limit) } /// Updates the per-line truncation length. @@ -163,35 +170,81 @@ impl ReadSettings { } /// Enables or disables line numbers in output. + /// + /// # Arguments + /// - `line_numbers` - `true` to prefix each line with its line number. + /// + /// # Returns + /// - The modified [`ReadSettings`] with the updated flag. #[must_use] - pub const fn with_line_numbers(mut self, line_numbers: bool) -> Self { + pub fn with_line_numbers(mut self, line_numbers: bool) -> Self { self.line_numbers = line_numbers; self } + /// Attaches an optional permission ruleset to read operations. + /// + /// # Arguments + /// - `permission` - An optional [`Arc`] controlling which paths + /// may be read. Pass `None` to disable permission filtering. + /// + /// # Returns + /// - The modified [`ReadSettings`] with the permission attached. + /// + /// [`Arc`]: std::sync::Arc + #[must_use] + pub fn with_permission(mut self, permission: Option>) -> Self { + self.permission = permission; + self + } + /// Returns the line count used when the caller doesn't specify one. + /// + /// # Returns + /// - The configured default line limit. #[must_use] - pub const fn default_limit(self) -> usize { + pub const fn default_limit(&self) -> usize { self.default_limit } /// Returns the hard cap on lines returned regardless of the request. + /// + /// # Returns + /// - The configured maximum line limit. #[must_use] - pub const fn max_limit(self) -> usize { + pub const fn max_limit(&self) -> usize { self.max_limit } /// Returns the maximum characters per line before truncation. + /// + /// # Returns + /// - The configured per-line truncation length. #[must_use] - pub const fn max_line_length(self) -> usize { + pub const fn max_line_length(&self) -> usize { self.max_line_length } /// Returns whether line numbers are included in output. + /// + /// # Returns + /// - `true` when line numbers are enabled. #[must_use] - pub const fn line_numbers(self) -> bool { + pub const fn line_numbers(&self) -> bool { self.line_numbers } + + /// Returns the permission ruleset applied to read operations, if any. + /// + /// # Returns + /// - `Some(&`[`Ruleset`]`)` when a permission filter is configured. + /// - `None` when no permission filtering is applied. + /// + /// [`Ruleset`]: crate::permissions::Ruleset + #[must_use] + pub fn permission(&self) -> Option<&Ruleset> { + self.permission.as_deref() + } } fn ensure_read_limits(default_limit: usize, max_limit: usize) -> ToolResult<()> { @@ -236,7 +289,7 @@ fn ensure_max_line_length(max_line_length: usize) -> ToolResult<()> { pub async fn read_file( resolver: &R, request: ReadRequest, - settings: ReadSettings, + settings: &ReadSettings, ) -> ToolResult { // Conditional trait import for consume() method #[cfg(feature = "blocking")] @@ -263,6 +316,10 @@ pub async fn read_file( } let path = resolver.resolve(&request.file_path)?; + let subject = path.to_string_lossy(); + settings + .permission() + .check(read_meta::NAME, subject.as_ref())?; let buf_capacity = (limit * ESTIMATED_CHARS_PER_LINE).next_power_of_two(); let mut reader = fs::open_buffered(&path, buf_capacity).await?; @@ -362,6 +419,7 @@ pub async fn read_file( mod tests { use super::*; use crate::path::AbsolutePathResolver; + use crate::permissions::{PermissionAction, Rule}; use rstest::rstest; use std::io::Write as _; use tempfile::NamedTempFile; @@ -387,7 +445,7 @@ mod tests { offset, limit: Some(limit), }, - settings, + &settings, ) .await } @@ -481,7 +539,7 @@ mod tests { offset: 1, limit: Some(10), }, - settings, + &settings, ) .await .unwrap(); @@ -511,7 +569,7 @@ mod tests { offset: 1, limit: Some(3), }, - settings, + &settings, ) .await .unwrap(); @@ -538,11 +596,43 @@ mod tests { offset: 1, limit: Some(0), // Request explicitly asks for 0 lines }, - settings, + &settings, ) .await .unwrap_err(); assert!(matches!(err, ToolError::Validation { .. })); } + + #[maybe_async::test(feature = "blocking", async(feature = "tokio", tokio::test))] + async fn read_request_rejects_denied_path() { + let mut temp = NamedTempFile::new().unwrap(); + temp.write_all(b"line1\n").unwrap(); + let resolver = AbsolutePathResolver; + + let mut ruleset = Ruleset::new(); + ruleset.push(Rule::new("read", "*", PermissionAction::Allow)); + ruleset.push(Rule::new( + "read", + temp.path().to_string_lossy().into_owned(), + PermissionAction::Deny, + )); + + let err = read_file( + &resolver, + ReadRequest { + file_path: temp.path().to_string_lossy().into_owned(), + offset: 1, + limit: Some(1), + }, + &ReadSettings::new().with_permission(Some(Arc::new(ruleset))), + ) + .await + .unwrap_err(); + + assert!(matches!( + err, + ToolError::PermissionDenied { tool: "read", .. } + )); + } } diff --git a/src/llm-coding-tools-core/src/tools/write.rs b/src/llm-coding-tools-core/src/tools/write.rs index a8b4a9d9..fb1bb6b8 100644 --- a/src/llm-coding-tools-core/src/tools/write.rs +++ b/src/llm-coding-tools-core/src/tools/write.rs @@ -3,8 +3,12 @@ use crate::error::{ToolError, ToolResult}; use crate::fs; use crate::path::PathResolver; +use crate::permissions::Ruleset; +use crate::permissions_ext::OptionRulesetExt; +use crate::tool_metadata::write as write_meta; use serde::Deserialize; use serde_json::Value; +use std::sync::Arc; /// Serde-friendly write request owned by the core crate. #[derive(Debug, Deserialize)] @@ -20,6 +24,57 @@ impl WriteRequest { } } +/// Runtime settings that control permission filtering for write requests. +/// +/// Wraps an optional [`Ruleset`] that gates which paths a [`write_file`] +/// operation may target. +/// +/// [`Ruleset`]: crate::permissions::Ruleset +/// [`write_file`]: write_file +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct WriteSettings { + permission: Option>, +} + +impl WriteSettings { + /// Creates default write settings with no extra permission filtering. + /// + /// # Returns + /// - A [`WriteSettings`] with permission set to `None`. + #[must_use] + pub fn new() -> Self { + Self { permission: None } + } + + /// Attaches an optional permission ruleset to write operations. + /// + /// # Arguments + /// - `permission` - An optional [`Arc`] controlling which paths + /// may be written to. Pass `None` to disable permission filtering. + /// + /// # Returns + /// - The modified [`WriteSettings`] with the permission attached. + /// + /// [`Arc`]: std::sync::Arc + #[must_use] + pub fn with_permission(mut self, permission: Option>) -> Self { + self.permission = permission; + self + } + + /// Returns the permission ruleset applied to write operations, if any. + /// + /// # Returns + /// - `Some(&`[`Ruleset`]`)` when a permission filter is configured. + /// - `None` when no permission filtering is applied. + /// + /// [`Ruleset`]: crate::permissions::Ruleset + #[must_use] + pub fn permission(&self) -> Option<&Ruleset> { + self.permission.as_deref() + } +} + /// Writes content to a file, creating parent directories if needed. /// /// Overwrites existing files. Returns a success message with byte count. @@ -27,8 +82,13 @@ impl WriteRequest { pub async fn write_file( resolver: &R, request: WriteRequest, + settings: &WriteSettings, ) -> ToolResult { let path = resolver.resolve(&request.file_path)?; + let subject = path.to_string_lossy(); + settings + .permission() + .check(write_meta::NAME, subject.as_ref())?; // Create parent directories if they don't exist if let Some(parent) = path.parent() { @@ -51,6 +111,7 @@ pub async fn write_file( mod tests { use super::*; use crate::path::AbsolutePathResolver; + use crate::permissions::{PermissionAction, Rule}; use tempfile::TempDir; #[maybe_async::test(feature = "blocking", async(feature = "tokio", tokio::test))] @@ -65,6 +126,7 @@ mod tests { file_path: file_path.to_str().unwrap().to_string(), content: "hello world".to_string(), }, + &WriteSettings::new(), ) .await .unwrap(); @@ -85,10 +147,42 @@ mod tests { file_path: file_path.to_str().unwrap().to_string(), content: "nested".to_string(), }, + &WriteSettings::new(), ) .await .unwrap(); assert!(file_path.exists()); } + + #[maybe_async::test(feature = "blocking", async(feature = "tokio", tokio::test))] + async fn write_rejects_denied_path() { + let temp = TempDir::new().unwrap(); + let file_path = temp.path().join("denied.txt"); + let resolver = AbsolutePathResolver; + + let mut ruleset = Ruleset::new(); + ruleset.push(Rule::new("write", "*", PermissionAction::Allow)); + ruleset.push(Rule::new( + "write", + file_path.to_string_lossy().into_owned(), + PermissionAction::Deny, + )); + + let err = write_file( + &resolver, + WriteRequest { + file_path: file_path.to_string_lossy().into_owned(), + content: "blocked".to_string(), + }, + &WriteSettings::new().with_permission(Some(Arc::new(ruleset))), + ) + .await + .unwrap_err(); + + assert!(matches!( + err, + ToolError::PermissionDenied { tool: "write", .. } + )); + } } diff --git a/src/llm-coding-tools-serdesai/src/agent_runtime/build.rs b/src/llm-coding-tools-serdesai/src/agent_runtime/build.rs index 61764878..74ac57a4 100644 --- a/src/llm-coding-tools-serdesai/src/agent_runtime/build.rs +++ b/src/llm-coding-tools-serdesai/src/agent_runtime/build.rs @@ -14,8 +14,9 @@ use crate::{ }; use llm_coding_tools_agents::{ AgentRuntime, AgentToolSettings, ModelResolutionError, TaskTargetSummary, ToolCatalogEntry, - ToolCatalogKind, summarize_callable_targets, + ToolCatalogKind, }; +use llm_coding_tools_core::permissions::Ruleset; use llm_coding_tools_core::tool_metadata::{ glob as glob_meta, grep as grep_meta, read as read_meta, webfetch as webfetch_meta, }; @@ -25,6 +26,7 @@ use llm_coding_tools_core::tools::{ use llm_coding_tools_core::{CredentialLookup, models::ModelCatalog}; use serdes_ai::AgentBuilder; use serdes_ai_models::BoxedModel; +use std::sync::Arc; /// Error returned when a build cannot produce a SerdesAI agent. #[derive(Debug, thiserror::Error)] @@ -80,6 +82,9 @@ pub(super) struct PreparedBuild { tool_settings: AgentToolSettings, /// Pre-computed callable Task target summaries for the Task tool description. callable_target_summaries: Vec, + /// Pre-built permission ruleset for tool access control. + /// None if agent has no permissions (backward compatibility). + permission: Option>, } impl PreparedBuild { @@ -88,14 +93,7 @@ impl PreparedBuild { pub(super) fn model(&self) -> &BoxedModel { &self.model } - - /// Returns the resolved callable Task target summaries. - #[inline] - pub(super) fn callable_target_summaries(&self) -> &[TaskTargetSummary] { - &self.callable_target_summaries - } } - /// Resolves model configuration and collects build parameters for an agent. pub(super) fn prepare_build( runtime: &AgentRuntime, @@ -113,13 +111,17 @@ where .ok_or_else(|| AgentBuildError::UnknownAgent { name: name.into() })?; let resolved = resolve_model(model_catalog, runtime.defaults(), agent)?; let serdes_model = build_serdes_model(model_catalog, &resolved, credentials)?; - let tools = runtime.allowed_tools(name); + let tools = runtime.allowed_tools(name).to_vec(); let callable_target_summaries = if with_summaries { - summarize_callable_targets(runtime.catalog(), name) + runtime.summarize_callable_targets(name).to_vec() } else { Vec::new() }; + let permission = runtime + .permission_ruleset(name) + .filter(|ruleset| !ruleset.is_empty()); + Ok(PreparedBuild { agent_name: agent.name.clone(), model: serdes_model.model, @@ -133,6 +135,7 @@ where tools, tool_settings: agent.tool_settings.clone(), callable_target_summaries, + permission, }) } @@ -156,44 +159,63 @@ where builder = builder.top_p(top_p); } - for entry in &prepared.tools { + // Use pre-built permission ruleset from PreparedBuild + // No need to rebuild - already constructed in prepare_build + let permission = prepared.permission.clone(); + + for entry in prepared.tools.iter() { match entry.kind { ToolCatalogKind::Read => { let settings = build_read_settings(&prepared.tool_settings.read)?; - builder = builder.tool( - prompt_builder.track(ReadTool::with_settings(AbsolutePathResolver, settings)), - ); + builder = + builder.tool(prompt_builder.track(ReadTool::with_settings_and_permission( + AbsolutePathResolver, + settings, + permission.clone(), + ))); } ToolCatalogKind::Write => { - builder = builder.tool(prompt_builder.track(WriteTool::new(AbsolutePathResolver))) + builder = builder.tool(prompt_builder.track( + WriteTool::new(AbsolutePathResolver).with_permission(permission.clone()), + )); } ToolCatalogKind::Edit => { - builder = builder.tool(prompt_builder.track(EditTool::new(AbsolutePathResolver))) + builder = builder.tool(prompt_builder.track( + EditTool::new(AbsolutePathResolver).with_permission(permission.clone()), + )); } ToolCatalogKind::Glob => { let settings = build_glob_settings(&prepared.tool_settings.glob)?; builder = builder.tool( - prompt_builder.track(GlobTool::with_settings(AbsolutePathResolver, settings)), + prompt_builder.track( + GlobTool::with_settings(AbsolutePathResolver, settings) + .with_permission(permission.clone()), + ), ); } ToolCatalogKind::Grep => { let (search_settings, formatting_settings) = build_grep_settings(&prepared.tool_settings.grep)?; - builder = builder.tool(prompt_builder.track(GrepTool::with_settings( - AbsolutePathResolver, - search_settings, - formatting_settings, - ))); + builder = builder.tool( + prompt_builder.track( + GrepTool::with_settings( + AbsolutePathResolver, + search_settings, + formatting_settings, + ) + .with_permission(permission.clone()), + ), + ); } ToolCatalogKind::Bash => { let settings = &prepared.tool_settings.bash; - builder = - builder.tool(prompt_builder.track( - BashTool::new().with_timeouts( - Some(settings.timeout_ms), - Some(settings.max_timeout_ms), - ), - )); + builder = builder.tool( + prompt_builder.track( + BashTool::new() + .with_timeouts(Some(settings.timeout_ms), Some(settings.max_timeout_ms)) + .with_permission(permission.clone()), + ), + ); } ToolCatalogKind::WebFetch => { let settings = build_webfetch_settings(&prepared.tool_settings.webfetch)?; @@ -207,11 +229,11 @@ where } ToolCatalogKind::Task => { if let Some(task_handle) = task_handle - && !prepared.callable_target_summaries().is_empty() + && !prepared.callable_target_summaries.is_empty() { builder = builder.tool(prompt_builder.track(TaskTool::new( prepared.agent_name.as_ref(), - prepared.callable_target_summaries().to_vec(), + prepared.callable_target_summaries.clone(), (*task_handle).clone(), ))); } diff --git a/src/llm-coding-tools-serdesai/src/convert.rs b/src/llm-coding-tools-serdesai/src/convert.rs index 3ae1e2e0..b3a4dfd0 100644 --- a/src/llm-coding-tools-serdesai/src/convert.rs +++ b/src/llm-coding-tools-serdesai/src/convert.rs @@ -77,6 +77,11 @@ pub(crate) fn core_error_to_serdes(tool_name: &str, err: CoreError) -> SerdesErr SerdesError::validation_error(tool_name, field.clone(), message.clone()) } CoreError::Json(msg) => SerdesError::validation_error(tool_name, None, msg.to_string()), + // Permission denied - runtime failure (policy/permission issue) + CoreError::PermissionDenied { tool, subject } => SerdesError::execution_failed(format!( + "Permission denied for tool '{}' on subject '{}'", + tool, subject + )), // Execution errors - runtime failures CoreError::Io(_) | CoreError::Http(_) diff --git a/src/llm-coding-tools-serdesai/src/task/handle.rs b/src/llm-coding-tools-serdesai/src/task/handle.rs index 98b93cf7..2455ba10 100644 --- a/src/llm-coding-tools-serdesai/src/task/handle.rs +++ b/src/llm-coding-tools-serdesai/src/task/handle.rs @@ -5,8 +5,7 @@ //! Each call is independent — no session state is kept between runs. use crate::agent_runtime::{TaskBuildContext, build_agent}; -use llm_coding_tools_agents::{AgentMode, RulesetExt}; -use llm_coding_tools_core::permissions::Ruleset; +use llm_coding_tools_agents::AgentMode; use llm_coding_tools_core::tool_metadata::task as task_meta; use llm_coding_tools_core::{CredentialLookup, CredentialResolver, TaskInput, TaskOutput}; use serdes_ai::tools::ToolError; @@ -105,6 +104,8 @@ where fn validate_target(&self, caller_name: &str, target_name: &str) -> Result<(), ToolError> { let catalog = self.context.runtime().catalog(); + + // Check if we can get caller & target let caller = catalog.by_name(caller_name).ok_or_else(|| { ToolError::execution_failed(format!( "delegating agent `{caller_name}` disappeared from the runtime catalog" @@ -118,6 +119,7 @@ where ) })?; + // Primary agents cannot be delegated to; they're main driver. if matches!(target.mode, AgentMode::Primary) { return Err(ToolError::validation_error( task_meta::NAME, @@ -128,14 +130,12 @@ where )); } - // `validate_target` only applies `Ruleset` filtering when `caller.permission` - // explicitly defines `task_meta::NAME`; without that opt-in, non-Primary - // targets remain callable for compatibility, while `AgentMode::Primary` - // targets are always denied above. - let has_explicit_task_permission = caller.permission.contains_key(task_meta::NAME); - if has_explicit_task_permission - && !Ruleset::from_permission_config(&caller.permission) - .is_allowed(task_meta::NAME, target_name) + // Check if caller is allowed to delegate to target + if caller.permission.contains_key(task_meta::NAME) + && !self + .context + .runtime() + .can_delegate_to(caller_name, target_name) { return Err(ToolError::validation_error( task_meta::NAME, diff --git a/src/llm-coding-tools-serdesai/src/tools/bash.rs b/src/llm-coding-tools-serdesai/src/tools/bash.rs index 4de03a17..ca893cfd 100644 --- a/src/llm-coding-tools-serdesai/src/tools/bash.rs +++ b/src/llm-coding-tools-serdesai/src/tools/bash.rs @@ -27,10 +27,12 @@ use crate::convert::{core_error_to_serdes, to_serdes_result}; use async_trait::async_trait; use llm_coding_tools_core::context::{ToolContext, ToolPrompt}; +use llm_coding_tools_core::permissions::Ruleset; use llm_coding_tools_core::tool_metadata::bash as bash_meta; use llm_coding_tools_core::tools::{BashExecutionMode, BashRequest, BashSettings, execute_command}; use serdes_ai::tools::{RunContext, SchemaBuilder, Tool, ToolDefinition, ToolResult}; use std::path::PathBuf; +use std::sync::Arc; #[cfg(all(feature = "linux-bubblewrap", target_os = "linux"))] use llm_coding_tools_bubblewrap::profile::{NetworkPolicy, Profile}; @@ -49,6 +51,8 @@ pub struct BashTool { max_timeout_ms: u32, /// Default working directory when not specified in args. default_workdir: Option, + /// Optional permission ruleset for command access control. + permission: Option>, } impl Default for BashTool { @@ -77,6 +81,7 @@ impl BashTool { default_timeout_ms: bash_meta::DEFAULT_TIMEOUT_MS, max_timeout_ms: bash_meta::MAX_TIMEOUT_MS, default_workdir: None, + permission: None, } } @@ -146,6 +151,18 @@ impl BashTool { self } + /// Sets the permission ruleset for this tool. + /// + /// # Arguments + /// + /// * `permission` - Optional [`Ruleset`] for command access control. + /// Use `None` to disable permission checking. + #[must_use] + pub fn with_permission(mut self, permission: Option>) -> Self { + self.permission = permission; + self + } + /// Runs commands inside a Linux sandbox using bubblewrap. /// /// Accepts an owned [`Profile`] or `Arc` to share one profile across @@ -200,6 +217,7 @@ impl Tool for BashTool { default_timeout_ms: self.default_timeout_ms, max_timeout_ms: self.max_timeout_ms, default_workdir: self.default_workdir.as_deref(), + permission: self.permission.as_deref(), }, ) .await; diff --git a/src/llm-coding-tools-serdesai/src/tools/edit.rs b/src/llm-coding-tools-serdesai/src/tools/edit.rs index 5c77a8b0..a2b590b9 100644 --- a/src/llm-coding-tools-serdesai/src/tools/edit.rs +++ b/src/llm-coding-tools-serdesai/src/tools/edit.rs @@ -13,9 +13,11 @@ use async_trait::async_trait; use llm_coding_tools_core::ToolContext; use llm_coding_tools_core::context::{PathMode, ToolPrompt}; use llm_coding_tools_core::path::PathResolver; +use llm_coding_tools_core::permissions::Ruleset; use llm_coding_tools_core::tool_metadata::edit as edit_meta; -use llm_coding_tools_core::tools::{EditRequest, edit_file}; +use llm_coding_tools_core::tools::{EditRequest, EditSettings, edit_file}; use serdes_ai::tools::{RunContext, SchemaBuilder, Tool, ToolDefinition, ToolResult, ToolReturn}; +use std::sync::Arc; use crate::convert::core_error_to_serdes; @@ -27,6 +29,7 @@ pub struct EditTool { definition: ToolDefinition, resolver: R, path_mode: PathMode, + settings: EditSettings, } impl EditTool { @@ -36,14 +39,38 @@ impl EditTool { /// /// * `R` - A path resolver implementing [`PathResolver`]. pub fn new(resolver: R) -> Self { + Self::with_settings(resolver, EditSettings::new()) + } + + /// Creates a new edit tool with custom settings. + /// + /// # Arguments + /// + /// * `resolver` - A [`PathResolver`] used to resolve and validate file paths. + /// * `settings` - [`EditSettings`] controlling edit behaviour such as + /// permission checks and replacement limits. + pub fn with_settings(resolver: R, settings: EditSettings) -> Self { let path_mode = R::PATH_MODE; Self { definition: build_definition(path_mode), resolver, path_mode, + settings, } } + /// Sets the permission ruleset for this tool. + /// + /// # Arguments + /// + /// * `permission` - Optional [`Ruleset`] for path access control. + /// Use `None` to disable permission checking. + #[must_use] + pub fn with_permission(mut self, permission: Option>) -> Self { + self.settings = self.settings.with_permission(permission); + self + } + /// Returns the path mode for this tool instance. #[must_use] pub fn path_mode(&self) -> PathMode { @@ -60,8 +87,7 @@ impl Tool for Ed async fn call(&self, _ctx: &RunContext, args: serde_json::Value) -> ToolResult { let args = EditRequest::parse(args).map_err(|e| core_error_to_serdes(edit_meta::NAME, e))?; - - let result = edit_file(&self.resolver, args).await; + let result = edit_file(&self.resolver, args, &self.settings).await; result .map(ToolReturn::text) diff --git a/src/llm-coding-tools-serdesai/src/tools/glob.rs b/src/llm-coding-tools-serdesai/src/tools/glob.rs index 47e20926..c59f4a0b 100644 --- a/src/llm-coding-tools-serdesai/src/tools/glob.rs +++ b/src/llm-coding-tools-serdesai/src/tools/glob.rs @@ -13,12 +13,14 @@ use async_trait::async_trait; use llm_coding_tools_core::ToolContext; use llm_coding_tools_core::context::{PathMode, ToolPrompt}; use llm_coding_tools_core::path::PathResolver; +use llm_coding_tools_core::permissions::Ruleset; use llm_coding_tools_core::tool_metadata::glob as glob_meta; use llm_coding_tools_core::tools::{GlobOutput, GlobRequest, GlobSettings, glob_files}; use serde_json::json; use serdes_ai::tools::{RunContext, SchemaBuilder, Tool, ToolDefinition, ToolResult, ToolReturn}; +use std::sync::Arc; -use crate::convert::{core_error_to_serdes, to_serdes_result}; +use crate::convert::core_error_to_serdes; /// Tool for finding files matching glob patterns. /// @@ -59,6 +61,18 @@ impl GlobTool { } } + /// Sets the permission ruleset for this tool. + /// + /// # Arguments + /// + /// * `permission` - Optional [`Ruleset`] for path access control. + /// Use `None` to disable permission checking. + #[must_use] + pub fn with_permission(mut self, permission: Option>) -> Self { + self.settings = self.settings.with_permission(permission); + self + } + /// Returns the path mode for this tool instance. #[must_use] pub fn path_mode(&self) -> PathMode { @@ -76,12 +90,10 @@ impl Tool for Gl let args = GlobRequest::parse(args).map_err(|e| core_error_to_serdes(glob_meta::NAME, e))?; - let result = glob_files(&self.resolver, args, self.settings); + let output = glob_files(&self.resolver, args, &self.settings) + .map_err(|e| core_error_to_serdes(glob_meta::NAME, e))?; - match result { - Err(e) => to_serdes_result(glob_meta::NAME, Err(e)), - Ok(output) => Ok(glob_output_to_return(output)), - } + Ok(glob_output_to_return(output)) } } diff --git a/src/llm-coding-tools-serdesai/src/tools/grep.rs b/src/llm-coding-tools-serdesai/src/tools/grep.rs index ee33cd3b..bc8e498c 100644 --- a/src/llm-coding-tools-serdesai/src/tools/grep.rs +++ b/src/llm-coding-tools-serdesai/src/tools/grep.rs @@ -14,12 +14,14 @@ use async_trait::async_trait; use llm_coding_tools_core::ToolContext; use llm_coding_tools_core::context::{PathMode, ToolPrompt}; use llm_coding_tools_core::path::PathResolver; +use llm_coding_tools_core::permissions::Ruleset; use llm_coding_tools_core::tool_metadata::grep as grep_meta; use llm_coding_tools_core::tools::{ GrepFormattingSettings, GrepOutput, GrepRequest, GrepSettings, grep_search, }; use serde_json::json; use serdes_ai::tools::{RunContext, SchemaBuilder, Tool, ToolDefinition, ToolResult, ToolReturn}; +use std::sync::Arc; use crate::convert::{core_error_to_serdes, to_serdes_result}; @@ -70,6 +72,18 @@ impl GrepTool { } } + /// Sets the permission ruleset for this tool. + /// + /// # Arguments + /// + /// * `permission` - Optional [`Ruleset`] for path access control. + /// Use `None` to disable permission checking. + #[must_use] + pub fn with_permission(mut self, permission: Option>) -> Self { + self.search_settings = self.search_settings.with_permission(permission); + self + } + /// Returns the path mode for this tool instance. #[must_use] pub fn path_mode(&self) -> PathMode { @@ -87,7 +101,7 @@ impl Tool for Gr let args = GrepRequest::parse(args).map_err(|e| core_error_to_serdes(grep_meta::NAME, e))?; - let result = grep_search(&self.resolver, args, self.search_settings); + let result = grep_search(&self.resolver, args, &self.search_settings); match result { Err(e) => to_serdes_result(grep_meta::NAME, Err(e)), @@ -441,7 +455,7 @@ mod tests { let resolver = AbsolutePathResolver; let search_settings = GrepSettings::new().with_max_limit(1).unwrap(); let formatting_settings = GrepFormattingSettings::new().with_line_numbers(false); - let tool = GrepTool::with_settings(resolver, search_settings, formatting_settings); + let tool = GrepTool::with_settings(resolver, search_settings.clone(), formatting_settings); assert_eq!(tool.search_settings, search_settings); assert_eq!(tool.formatting_settings, formatting_settings); } diff --git a/src/llm-coding-tools-serdesai/src/tools/read.rs b/src/llm-coding-tools-serdesai/src/tools/read.rs index ec685526..fc7c5750 100644 --- a/src/llm-coding-tools-serdesai/src/tools/read.rs +++ b/src/llm-coding-tools-serdesai/src/tools/read.rs @@ -27,9 +27,11 @@ use async_trait::async_trait; use llm_coding_tools_core::ToolContext; use llm_coding_tools_core::context::{PathMode, ToolPrompt}; use llm_coding_tools_core::path::PathResolver; +use llm_coding_tools_core::permissions::Ruleset; use llm_coding_tools_core::tool_metadata::read as read_meta; use llm_coding_tools_core::tools::{ReadRequest, ReadSettings, read_file}; use serdes_ai::tools::{RunContext, SchemaBuilder, Tool, ToolDefinition, ToolResult}; +use std::sync::Arc; use crate::convert::{core_error_to_serdes, to_serdes_result}; @@ -74,6 +76,33 @@ impl ReadTool { } } + /// Sets the permission ruleset for this tool. + /// + /// # Arguments + /// + /// * `permission` - Optional [`Ruleset`] for path access control. + /// Use `None` to disable permission checking. + #[must_use] + pub fn with_permission(mut self, permission: Option>) -> Self { + self.settings = self.settings.with_permission(permission); + self + } + + /// Creates a new read tool with settings and permission. + /// + /// # Arguments + /// + /// * `resolver` - The path resolver for path validation and resolution. + /// * `settings` - Core read settings for limits and formatting. + /// * `permission` - Optional permission ruleset for access control. + pub fn with_settings_and_permission( + resolver: R, + settings: ReadSettings, + permission: Option>, + ) -> Self { + Self::with_settings(resolver, settings.with_permission(permission)) + } + /// Returns the path mode for this tool instance. /// /// The path mode comes from the resolver implementation. @@ -93,7 +122,7 @@ impl Tool for Re let args = ReadRequest::parse(args).map_err(|e| core_error_to_serdes(read_meta::NAME, e))?; - let result = read_file(&self.resolver, args, self.settings).await; + let result = read_file(&self.resolver, args, &self.settings).await; to_serdes_result(read_meta::NAME, result) } } @@ -175,7 +204,7 @@ mod tests { .with_max_line_length(100) .unwrap() .with_line_numbers(false); - let tool = ReadTool::with_settings(AbsolutePathResolver, settings); + let tool = ReadTool::with_settings(AbsolutePathResolver, settings.clone()); assert_eq!(tool.settings, settings); } diff --git a/src/llm-coding-tools-serdesai/src/tools/write.rs b/src/llm-coding-tools-serdesai/src/tools/write.rs index 603c13b5..22fbdf09 100644 --- a/src/llm-coding-tools-serdesai/src/tools/write.rs +++ b/src/llm-coding-tools-serdesai/src/tools/write.rs @@ -12,10 +12,12 @@ use async_trait::async_trait; use llm_coding_tools_core::context::{PathMode, ToolPrompt}; use llm_coding_tools_core::path::PathResolver; +use llm_coding_tools_core::permissions::Ruleset; use llm_coding_tools_core::tool_metadata::write as write_meta; -use llm_coding_tools_core::tools::{WriteRequest, write_file}; +use llm_coding_tools_core::tools::{WriteRequest, WriteSettings, write_file}; use llm_coding_tools_core::{ToolContext, ToolOutput}; use serdes_ai::tools::{RunContext, SchemaBuilder, Tool, ToolDefinition, ToolResult}; +use std::sync::Arc; use crate::convert::{core_error_to_serdes, to_serdes_result}; @@ -27,6 +29,7 @@ pub struct WriteTool { definition: ToolDefinition, resolver: R, path_mode: PathMode, + settings: WriteSettings, } impl WriteTool { @@ -36,14 +39,38 @@ impl WriteTool { /// /// * `R` - A path resolver implementing [`PathResolver`]. pub fn new(resolver: R) -> Self { + Self::with_settings(resolver, WriteSettings::new()) + } + + /// Creates a new write tool with custom settings. + /// + /// # Arguments + /// + /// * `resolver` - A [`PathResolver`] used to resolve and validate file paths. + /// * `settings` - [`WriteSettings`] controlling write behaviour such as + /// permission checks and overwrite handling. + pub fn with_settings(resolver: R, settings: WriteSettings) -> Self { let path_mode = R::PATH_MODE; Self { definition: build_definition(path_mode), resolver, path_mode, + settings, } } + /// Sets the permission ruleset for this tool. + /// + /// # Arguments + /// + /// * `permission` - Optional [`Ruleset`] for path access control. + /// Use `None` to disable permission checking. + #[must_use] + pub fn with_permission(mut self, permission: Option>) -> Self { + self.settings = self.settings.with_permission(permission); + self + } + /// Returns the path mode for this tool instance. #[must_use] pub fn path_mode(&self) -> PathMode { @@ -61,7 +88,7 @@ impl Tool for Wr let args = WriteRequest::parse(args).map_err(|e| core_error_to_serdes(write_meta::NAME, e))?; - let result = write_file(&self.resolver, args).await; + let result = write_file(&self.resolver, args, &self.settings).await; to_serdes_result(write_meta::NAME, result.map(ToolOutput::new)) } }