diff --git a/src/Cargo.lock b/src/Cargo.lock index f2c8d0f..4a69f56 100644 --- a/src/Cargo.lock +++ b/src/Cargo.lock @@ -2003,6 +2003,7 @@ dependencies = [ "serde", "serde_json", "serde_yaml", + "soft-canonicalize 0.4.5", "tempfile", "thiserror 2.0.18", ] @@ -2051,7 +2052,7 @@ dependencies = [ "serde_json", "serial_test", "shellexpand", - "soft-canonicalize", + "soft-canonicalize 0.5.5", "temp-env", "tempfile", "thiserror 2.0.18", @@ -3491,6 +3492,12 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "soft-canonicalize" +version = "0.4.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8d773c1dc6044d8c2065e16808d6aed9d840c172a8b6c3295658be1138b3b53b" + [[package]] name = "soft-canonicalize" version = "0.5.5" diff --git a/src/llm-coding-tools-agents/Cargo.toml b/src/llm-coding-tools-agents/Cargo.toml index 24f5621..e20d24f 100644 --- a/src/llm-coding-tools-agents/Cargo.toml +++ b/src/llm-coding-tools-agents/Cargo.toml @@ -33,6 +33,9 @@ ahash = { version = "0.8", features = ["serde"] } # Core library for permissions and tool types llm-coding-tools-core = { path = "../llm-coding-tools-core", version = "0.2.0" } +# Canonicalizes paths, resolving symlinks without requiring the full path to exist +soft-canonicalize = "0.4" + [dev-dependencies] tempfile = "3.27" criterion = "0.8" diff --git a/src/llm-coding-tools-agents/src/lib.rs b/src/llm-coding-tools-agents/src/lib.rs index 5314a22..cf04aee 100644 --- a/src/llm-coding-tools-agents/src/lib.rs +++ b/src/llm-coding-tools-agents/src/lib.rs @@ -4,6 +4,7 @@ mod catalog; mod extensions; mod loader; mod parser; +mod path; mod runtime; mod types; @@ -11,6 +12,7 @@ pub use catalog::AgentCatalog; pub use extensions::RulesetExt; pub use loader::AgentLoader; pub use parser::AgentParseError; +pub use path::{build_resolver_for_tool, FileToolResolver}; pub use runtime::{ callable_targets, default_tools, resolve_model_with_catalog, summarize_callable_targets, AgentDefaults, AgentRuntime, AgentRuntimeBuilder, ModelResolutionError, ResolvedModel, diff --git a/src/llm-coding-tools-agents/src/path/mod.rs b/src/llm-coding-tools-agents/src/path/mod.rs new file mode 100644 index 0000000..3caec21 --- /dev/null +++ b/src/llm-coding-tools-agents/src/path/mod.rs @@ -0,0 +1,8 @@ +//! File-tool path resolver construction. +//! +//! Re-exports [`FileToolResolver`] and [`build_resolver_for_tool`] from the +//! `resolver` submodule. See that module for optimisation-tier details. + +mod resolver; + +pub use resolver::{build_resolver_for_tool, FileToolResolver}; diff --git a/src/llm-coding-tools-agents/src/path/resolver.rs b/src/llm-coding-tools-agents/src/path/resolver.rs new file mode 100644 index 0000000..bf5e03d --- /dev/null +++ b/src/llm-coding-tools-agents/src/path/resolver.rs @@ -0,0 +1,562 @@ +//! File-tool path resolver construction. +//! +//! [`FileToolResolver`] is a closed enum of resolver types that avoids +//! `Box`. [`build_resolver_for_tool`] inspects permission +//! config and returns the cheapest resolver that satisfies it. +//! +//! # Optimisation tiers +//! +//! | Config pattern | Resolver | Cost | +//! |------------------------------------|-------------------------------|------------------| +//! | No config for tool | `AllowedPathResolver([root])` | prefix check | +//! | `Action(Allow)` | `AllowedPathResolver([root])` | prefix check | +//! | Pattern `**` with Allow | `AllowedPathResolver([root])` | prefix check | +//! | `/**` with Allow | `AbsolutePathResolver` | zero | +//! | Otherwise | `AllowedGlobResolver` | glob matching | + +use crate::types::PermissionRule; +use indexmap::IndexMap; +use llm_coding_tools_core::context::PathMode; +use llm_coding_tools_core::error::{ToolError, ToolResult}; +use llm_coding_tools_core::path::{ + expand_shell, AbsolutePathResolver, AllowedGlobResolver, AllowedPathResolver, GlobPolicy, + PathResolver, RuleAction, +}; +use llm_coding_tools_core::permissions::PermissionAction; +use soft_canonicalize::soft_canonicalize; +use std::path::{Path, PathBuf}; + +/// Closed enum of resolver types used for file tools. +/// +/// Avoids [`Box`] which cannot implement `Clone`. +/// All tool wrappers require `R: PathResolver + Clone`. +#[derive(Debug, Clone)] +pub enum FileToolResolver { + /// Unrestricted: any absolute path is allowed. + Absolute(AbsolutePathResolver), + /// Prefix-check: paths must start with one of the allowed directories. + Allowed(AllowedPathResolver), + /// Glob-filtered: paths must match configured glob patterns. + Glob(AllowedGlobResolver), +} + +impl PathResolver for FileToolResolver { + fn resolve(&self, path: &str) -> ToolResult { + match self { + Self::Absolute(r) => r.resolve(path), + Self::Allowed(r) => r.resolve(path), + Self::Glob(r) => r.resolve(path), + } + } + + fn is_path_allowed(&self, path: &Path) -> bool { + match self { + Self::Absolute(r) => r.is_path_allowed(path), + Self::Allowed(r) => r.is_path_allowed(path), + Self::Glob(r) => r.is_path_allowed(path), + } + } + + fn path_mode(&self) -> PathMode { + match self { + Self::Absolute(_) => PathMode::Absolute, + Self::Allowed(_) | Self::Glob(_) => PathMode::Allowed, + } + } +} + +/// Builds the cheapest resolver that satisfies the permission config. +/// +/// # Optimisation tiers +/// +/// - No config for tool -> `AllowedPathResolver([workspace_root])` (workspace only) +/// - `Action(Allow)` -> `AllowedPathResolver([workspace_root])` +/// - Pattern `"**"` with Allow -> `AllowedPathResolver([workspace_root])` (workspace only) +/// - `"/**"` -> `AbsolutePathResolver` (any absolute path) +/// - Otherwise -> `AllowedGlobResolver` with `GlobPolicy` +/// +/// # Arguments +/// +/// - `config` - Permission config mapping tool names to [`PermissionRule`]. +/// - `tool_name` - Name of the tool to look up in `config`. +/// - `workspace_root` - Workspace root used for relative-pattern resolution. +/// +/// # Returns +/// +/// The cheapest [`FileToolResolver`] variant satisfying the tool's permission config. +/// +/// # Errors +/// +/// - Returns [`ToolError::InvalidPath`] when shell expansion fails (e.g., unresolvable `$VAR` in a pattern). +/// - Returns [`ToolError::InvalidPattern`] when a glob pattern is syntactically invalid. +/// - Returns [`ToolError::InvalidPath`] when the workspace root does not exist or cannot be canonicalized. +pub fn build_resolver_for_tool( + config: &IndexMap, + tool_name: &str, + workspace_root: &Path, +) -> Result { + let workspace_root = soft_canonicalize(workspace_root).map_err(|e| { + ToolError::InvalidPath(format!( + "failed to resolve workspace root '{}': {}", + workspace_root.display(), + e + )) + })?; + + let Some(rule) = config.get(tool_name) else { + // Nothing specified: default to workspace only. + let resolver = AllowedPathResolver::from_canonical(vec![workspace_root]); + return Ok(FileToolResolver::Allowed(resolver)); + }; + match rule { + PermissionRule::Action(PermissionAction::Deny) => Err(ToolError::PermissionDenied { + tool: "file", + subject: format!("tool '{}' is disabled by configuration", tool_name), + }), + PermissionRule::Action(PermissionAction::Allow) => { + // Action(Allow): workspace only. + let resolver = AllowedPathResolver::from_canonical(vec![workspace_root]); + Ok(FileToolResolver::Allowed(resolver)) + } + PermissionRule::Pattern(patterns) => { + // Optimisation: all-allow patterns + if patterns.values().all(|a| *a == PermissionAction::Allow) { + // "/**" -> unrestricted access to any absolute path + if let Some(resolver) = try_globstar_optimisation(patterns)? { + return Ok(FileToolResolver::Absolute(resolver)); + } + // "**" -> workspace only (equivalent to Action(Allow)) + if is_bare_globstar(patterns) { + let resolver = AllowedPathResolver::from_canonical(vec![workspace_root]); + return Ok(FileToolResolver::Allowed(resolver)); + } + } + // Fall through to full glob policy + let resolver = AllowedGlobResolver::from_canonical(&workspace_root) + .with_policy(build_glob_policy(patterns, &workspace_root)?); + Ok(FileToolResolver::Glob(resolver)) + } + } +} + +/// Checks if any pattern is `/**` (unrestricted access to all absolute paths). +/// +/// Returns `Some(AbsolutePathResolver)` if found, `None` otherwise. +fn try_globstar_optimisation( + patterns: &IndexMap, +) -> Result, ToolError> { + for pattern in patterns.keys() { + let expanded = expand_shell(pattern)?; + if expanded.to_string_lossy() == "/**" { + return Ok(Some(AbsolutePathResolver)); + } + } + Ok(None) +} + +/// Checks if the pattern map contains exactly one pattern "**" (bare globstar). +/// +/// Returns `true` if there's a single pattern and it expands to "**", +/// indicating workspace-only access (equivalent to `Action(Allow)`). +fn is_bare_globstar(patterns: &IndexMap) -> bool { + if patterns.len() != 1 { + return false; + } + let pattern = patterns.keys().next().expect("len == 1"); + if let Ok(expanded) = expand_shell(pattern) { + return expanded.to_string_lossy() == "**"; + } + false +} + +/// Builds a `GlobPolicy` from a pattern map. +fn build_glob_policy( + patterns: &IndexMap, + workspace_root: &Path, +) -> Result { + let mut builder = GlobPolicy::builder_with_base(workspace_root)?; + for (pattern, action) in patterns { + let rule_action = match action { + PermissionAction::Allow => RuleAction::Allow, + PermissionAction::Deny => RuleAction::Deny, + }; + builder = builder.add(pattern, rule_action)?; + } + builder.build() +} + +#[cfg(test)] +mod tests { + use super::*; + use soft_canonicalize::soft_canonicalize; + + type TestResult = Result<(), ToolError>; + + // --------------------------------------------------------------- + // build_resolver_for_tool: no config for tool + // Default: workspace only (AllowedPathResolver). + // --------------------------------------------------------------- + + #[test] + fn no_config_returns_allowed() -> TestResult { + let temp = tempfile::TempDir::new().unwrap(); + + let config = IndexMap::new(); + let resolver = build_resolver_for_tool(&config, "read", temp.path())?; + + let FileToolResolver::Allowed(inner) = &resolver else { + panic!("expected Allowed, got {resolver:?}"); + }; + + // Allowed paths should be exactly [workspace_root]. + let expected = soft_canonicalize(temp.path())?; + assert_eq!(inner.allowed_paths(), &[expected]); + + Ok(()) + } + + // --------------------------------------------------------------- + // build_resolver_for_tool: Action(Allow) + // Scalar allow -> workspace only (AllowedPathResolver). + // --------------------------------------------------------------- + + #[test] + fn action_allow_returns_allowed() -> TestResult { + let temp = tempfile::TempDir::new().unwrap(); + + let mut config = IndexMap::new(); + config.insert( + "read".to_string(), + PermissionRule::Action(PermissionAction::Allow), + ); + + let resolver = build_resolver_for_tool(&config, "read", temp.path())?; + + let FileToolResolver::Allowed(inner) = &resolver else { + panic!("expected Allowed, got {resolver:?}"); + }; + + // Allowed paths should be exactly [workspace_root]. + let expected = soft_canonicalize(temp.path())?; + assert_eq!(inner.allowed_paths(), &[expected]); + + Ok(()) + } + + // --------------------------------------------------------------- + // build_resolver_for_tool: pattern "/**" + // Unrestricted: any absolute path is allowed (AbsolutePathResolver). + // --------------------------------------------------------------- + + #[test] + fn absolute_globstar_returns_absolute() -> TestResult { + let temp = tempfile::TempDir::new().unwrap(); + + let mut patterns = IndexMap::new(); + patterns.insert("/**".to_string(), PermissionAction::Allow); + let mut config = IndexMap::new(); + config.insert("read".to_string(), PermissionRule::Pattern(patterns)); + + let resolver = build_resolver_for_tool(&config, "read", temp.path())?; + + // Any absolute path is allowed, even outside the workspace. + assert!(resolver.is_path_allowed(Path::new("/etc/passwd"))); + + Ok(()) + } + + // --------------------------------------------------------------- + // build_resolver_for_tool: pattern "**" (bare globstar) + // Equivalent to Action(Allow): workspace only via AllowedPathResolver. + // --------------------------------------------------------------- + + #[test] + fn bare_globstar_returns_allowed_workspace_root() -> TestResult { + let temp = tempfile::TempDir::new().unwrap(); + + let mut patterns = IndexMap::new(); + patterns.insert("**".to_string(), PermissionAction::Allow); + let mut config = IndexMap::new(); + config.insert("read".to_string(), PermissionRule::Pattern(patterns)); + + let resolver = build_resolver_for_tool(&config, "read", temp.path())?; + + let FileToolResolver::Allowed(inner) = &resolver else { + panic!("expected Allowed, got {resolver:?}"); + }; + + // Bare "**" -> workspace root (same as Action(Allow)). + let expected = soft_canonicalize(temp.path())?; + assert_eq!(inner.allowed_paths(), &[expected.clone()]); + + // Any path within workspace should be allowed. + assert!( + resolver.is_path_allowed(&expected.join("src/lib.rs")), + "** should permit src/lib.rs" + ); + assert!( + resolver.is_path_allowed(&expected.join("any/path/file.txt")), + "** should permit any path" + ); + + Ok(()) + } + + // --------------------------------------------------------------- + // build_resolver_for_tool: pattern "src/**" + // Prefix patterns now use AllowedGlobResolver (prefix-globstar + // optimisation was removed to fix workspace-relative resolution). + // --------------------------------------------------------------- + + #[test] + fn prefix_globstar_returns_glob() -> TestResult { + let temp = tempfile::TempDir::new().unwrap(); + std::fs::create_dir_all(temp.path().join("src")).unwrap(); + + let mut patterns = IndexMap::new(); + patterns.insert("src/**".to_string(), PermissionAction::Allow); + let mut config = IndexMap::new(); + config.insert("read".to_string(), PermissionRule::Pattern(patterns)); + + let resolver = build_resolver_for_tool(&config, "read", temp.path())?; + + assert!( + matches!(resolver, FileToolResolver::Glob(_)), + "prefix patterns should use AllowedGlobResolver" + ); + + let root = soft_canonicalize(temp.path())?; + + // src/** allow should permit src/lib.rs. + assert!( + resolver.is_path_allowed(&root.join("src/lib.rs")), + "src/** should permit src/lib.rs" + ); + + // Paths outside src/ should be denied. + assert!( + !resolver.is_path_allowed(&root.join("tests/lib.rs")), + "paths outside src/ should be denied" + ); + + Ok(()) + } + + // --------------------------------------------------------------- + // build_resolver_for_tool: mixed allow + deny patterns + // Cannot optimise; falls through to GlobResolver. + // --------------------------------------------------------------- + + #[test] + fn mixed_allow_deny_returns_glob() -> TestResult { + let temp = tempfile::TempDir::new().unwrap(); + std::fs::create_dir_all(temp.path().join("src")).unwrap(); + + let mut patterns = IndexMap::new(); + patterns.insert("src/**".to_string(), PermissionAction::Allow); + patterns.insert("*.secret".to_string(), PermissionAction::Deny); + let mut config = IndexMap::new(); + config.insert("read".to_string(), PermissionRule::Pattern(patterns)); + + let resolver = build_resolver_for_tool(&config, "read", temp.path())?; + + assert!(matches!(resolver, FileToolResolver::Glob(_))); + + let root = soft_canonicalize(temp.path())?; + + // *.secret deny should block test.secret. + let secret_path = root.join("test.secret"); + assert!( + !resolver.is_path_allowed(secret_path.as_path()), + "*.secret deny should block test.secret" + ); + + // src/** allow should permit src/lib.rs. + let src_path = root.join("src/lib.rs"); + assert!( + resolver.is_path_allowed(src_path.as_path()), + "src/** allow should permit src/lib.rs" + ); + + Ok(()) + } + + // --------------------------------------------------------------- + // build_resolver_for_tool: non-** glob pattern ("**/*.rs") + // Not a simple prefix glob; falls through to GlobResolver. + // --------------------------------------------------------------- + + #[test] + fn non_globstar_pattern_returns_glob() -> TestResult { + let temp = tempfile::TempDir::new().unwrap(); + + let mut patterns = IndexMap::new(); + patterns.insert("**/*.rs".to_string(), PermissionAction::Allow); + let mut config = IndexMap::new(); + config.insert("read".to_string(), PermissionRule::Pattern(patterns)); + + let resolver = build_resolver_for_tool(&config, "read", temp.path())?; + + assert!(matches!(resolver, FileToolResolver::Glob(_))); + + Ok(()) + } + + // --------------------------------------------------------------- + // build_resolver_for_tool: multiple /** patterns + // Prefix patterns now use AllowedGlobResolver. + // --------------------------------------------------------------- + + #[test] + fn multiple_prefix_globstars_return_glob() -> TestResult { + let temp = tempfile::TempDir::new().unwrap(); + std::fs::create_dir_all(temp.path().join("src")).unwrap(); + std::fs::create_dir_all(temp.path().join("tests")).unwrap(); + + let mut patterns = IndexMap::new(); + patterns.insert("src/**".to_string(), PermissionAction::Allow); + patterns.insert("tests/**".to_string(), PermissionAction::Allow); + let mut config = IndexMap::new(); + config.insert("read".to_string(), PermissionRule::Pattern(patterns)); + + let resolver = build_resolver_for_tool(&config, "read", temp.path())?; + + assert!( + matches!(resolver, FileToolResolver::Glob(_)), + "multiple prefix globs should use AllowedGlobResolver" + ); + + let root = soft_canonicalize(temp.path())?; + + // src/** allow should permit src/lib.rs. + assert!( + resolver.is_path_allowed(&root.join("src/lib.rs")), + "src/** should permit src/lib.rs" + ); + + // tests/** allow should permit tests/lib.rs. + assert!( + resolver.is_path_allowed(&root.join("tests/lib.rs")), + "tests/** should permit tests/lib.rs" + ); + + // Paths outside both src/ and tests/ should be denied. + assert!( + !resolver.is_path_allowed(&root.join("docs/lib.rs")), + "paths outside src/ and tests/ should be denied" + ); + + Ok(()) + } + + // --------------------------------------------------------------- + // build_resolver_for_tool: empty pattern map + // No patterns to optimise; falls through to GlobResolver. + // --------------------------------------------------------------- + + #[test] + fn empty_pattern_map_returns_glob() -> TestResult { + let temp = tempfile::TempDir::new().unwrap(); + + let patterns = IndexMap::new(); + let mut config = IndexMap::new(); + config.insert("read".to_string(), PermissionRule::Pattern(patterns)); + + let resolver = build_resolver_for_tool(&config, "read", temp.path())?; + + assert!(matches!(resolver, FileToolResolver::Glob(_))); + + Ok(()) + } + + // --------------------------------------------------------------- + // build_resolver_for_tool: invalid shell variable + // Shell expansion fails; should return an error. + // --------------------------------------------------------------- + + #[test] + fn invalid_shell_variable_should_return_error() { + let temp = tempfile::TempDir::new().unwrap(); + + let mut patterns = IndexMap::new(); + patterns.insert( + "$DEFINITELY_NOT_SET_12345/**".to_string(), + PermissionAction::Allow, + ); + let mut config = IndexMap::new(); + config.insert("read".to_string(), PermissionRule::Pattern(patterns)); + + let result = build_resolver_for_tool(&config, "read", temp.path()); + assert!( + result.is_err(), + "unresolvable shell variable should produce an error" + ); + } + + // --------------------------------------------------------------- + // Regression: prefix-globstar optimisation breaks + // workspace-relative resolution for non-traversal tools. + // + // Pattern "src/**" creates an AllowedPathResolver whose only + // allowed base is workspace_root/src. When resolve() is called + // with "src/lib.rs" the resolver does base.join("src/lib.rs") + // which yields workspace_root/src/src/lib.rs — a doubled "src". + // --------------------------------------------------------------- + + #[test] + fn prefix_globstar_doubles_relative_prefix_for_non_traversal() -> TestResult { + let temp = tempfile::TempDir::new().unwrap(); + std::fs::create_dir_all(temp.path().join("src")).unwrap(); + std::fs::write(temp.path().join("src/lib.rs"), "").unwrap(); + + let mut patterns = IndexMap::new(); + patterns.insert("src/**".to_string(), PermissionAction::Allow); + let mut config = IndexMap::new(); + config.insert("read".to_string(), PermissionRule::Pattern(patterns)); + + let resolver = build_resolver_for_tool(&config, "read", temp.path())?; + let root = soft_canonicalize(temp.path())?; + + let resolved = resolver.resolve("src/lib.rs")?; + + assert_eq!( + resolved, + root.join("src/lib.rs"), + "resolve(\"src/lib.rs\") should be workspace_root/src/lib.rs, got {:?}", + resolved + ); + + Ok(()) + } + + // --------------------------------------------------------------- + // Regression: prefix-globstar optimisation is removed. + // All tools now use AllowedGlobResolver which correctly resolves + // "." to the workspace root (not a subdirectory). + // --------------------------------------------------------------- + + #[test] + fn prefix_patterns_resolve_dot_to_workspace_root() -> TestResult { + let temp = tempfile::TempDir::new().unwrap(); + std::fs::create_dir_all(temp.path().join("src")).unwrap(); + std::fs::create_dir_all(temp.path().join("tests")).unwrap(); + + let mut patterns = IndexMap::new(); + patterns.insert("src/**".to_string(), PermissionAction::Allow); + patterns.insert("tests/**".to_string(), PermissionAction::Allow); + let mut config = IndexMap::new(); + config.insert("read".to_string(), PermissionRule::Pattern(patterns)); + + let resolver = build_resolver_for_tool(&config, "read", temp.path())?; + let workspace_root = soft_canonicalize(temp.path())?; + let resolved = resolver.resolve(".")?; + + assert_eq!( + resolved, workspace_root, + "resolve('.') must return workspace root, got {:?}", + resolved + ); + + Ok(()) + } +} diff --git a/src/llm-coding-tools-core/src/path/absolute.rs b/src/llm-coding-tools-core/src/path/absolute.rs index 4505201..27359bf 100644 --- a/src/llm-coding-tools-core/src/path/absolute.rs +++ b/src/llm-coding-tools-core/src/path/absolute.rs @@ -1,8 +1,9 @@ //! Absolute path resolver implementation. use super::PathResolver; +use crate::context::PathMode; use crate::error::{ToolError, ToolResult}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; /// Path resolver that requires absolute paths. /// @@ -25,6 +26,14 @@ use std::path::PathBuf; pub struct AbsolutePathResolver; impl PathResolver for AbsolutePathResolver { + fn path_mode(&self) -> PathMode { + PathMode::Absolute + } + + fn is_path_allowed(&self, _path: &Path) -> bool { + true + } + fn resolve(&self, path: &str) -> ToolResult { let path = PathBuf::from(path); if !path.is_absolute() { @@ -91,4 +100,19 @@ mod tests { } } } + + #[test] + fn is_path_allowed_always_true() { + let resolver = AbsolutePathResolver; + // resolve succeeds for absolute paths + let ok_path = if cfg!(windows) { + "C:\\Users\\file.txt" + } else { + "/home/user/file.txt" + }; + let resolved = resolver.resolve(ok_path).unwrap(); + assert!(resolver.is_path_allowed(&resolved)); + // is_path_allowed returns true even for paths that resolve() would reject + assert!(resolver.is_path_allowed(Path::new("relative/path"))); + } } diff --git a/src/llm-coding-tools-core/src/path/allowed.rs b/src/llm-coding-tools-core/src/path/allowed.rs index af947b5..1b9531b 100644 --- a/src/llm-coding-tools-core/src/path/allowed.rs +++ b/src/llm-coding-tools-core/src/path/allowed.rs @@ -125,7 +125,13 @@ impl AllowedPathResolver { } impl PathResolver for AllowedPathResolver { - const PATH_MODE: PathMode = PathMode::Allowed; + fn path_mode(&self) -> PathMode { + PathMode::Allowed + } + + fn is_path_allowed(&self, path: &Path) -> bool { + self.allowed_paths.iter().any(|base| path.starts_with(base)) + } fn resolve(&self, path: &str) -> ToolResult { let input_path = Path::new(path); @@ -343,4 +349,24 @@ mod tests { let err = result.expect_err("external path should be rejected"); assert!(err.to_string().contains("not within allowed")); } + + #[test] + fn is_path_allowed_matches_resolve_outcome() { + let dir = setup_test_dir(); + let resolver = AllowedPathResolver::new(vec![dir.path().to_path_buf()]).unwrap(); + + // resolve succeeds for file inside allowed dir + let resolved = resolver.resolve("file.txt").unwrap(); + assert!( + resolver.is_path_allowed(&resolved), + "is_path_allowed must be true for resolved path" + ); + + // resolve fails for path outside allowed dir + let external = std::env::temp_dir().join("external.txt"); + assert!( + !resolver.is_path_allowed(&external), + "is_path_allowed must be false for external path" + ); + } } diff --git a/src/llm-coding-tools-core/src/path/allowed_glob/mod.rs b/src/llm-coding-tools-core/src/path/allowed_glob/mod.rs index 578e513..ae74d86 100644 --- a/src/llm-coding-tools-core/src/path/allowed_glob/mod.rs +++ b/src/llm-coding-tools-core/src/path/allowed_glob/mod.rs @@ -7,7 +7,7 @@ //! //! The entry point (`PathResolver::resolve`) rejects paths that lexically //! escape the workspace root, then joins relative paths with the workspace root -//! and dispatches to [`resolve_candidate`]. +//! and dispatches to `resolve_candidate`. //! //! ## `resolve_candidate` //! @@ -28,7 +28,7 @@ //! If no tier succeeds or policy denies the path, reject with //! "not within allowed directories". -pub(crate) mod normalize; +pub mod normalize; mod policy; use super::{path_analysis, resolve_new_file_fast, PathResolver}; @@ -154,7 +154,25 @@ impl AllowedGlobResolver { } impl PathResolver for AllowedGlobResolver { - const PATH_MODE: PathMode = PathMode::Allowed; + /// Fast per-entry check: is this absolute path allowed? + /// + /// Checks `starts_with(workspace_root)` and, if a policy is configured, + /// normalizes the path and checks against the glob policy. + fn is_path_allowed(&self, path: &Path) -> bool { + if !path.starts_with(&self.workspace_root) { + return false; + } + if let Some(policy) = &self.policy { + let normalized = normalize_path(path); + policy.is_allowed(normalized.as_ref()) + } else { + true + } + } + + fn path_mode(&self) -> PathMode { + PathMode::Allowed + } /// Resolves a path against the workspace root and checks glob policy. /// @@ -179,7 +197,6 @@ impl PathResolver for AllowedGlobResolver { /// - No resolution tier succeeds. fn resolve(&self, path: &str) -> ToolResult { let input_path = Path::new(path); - let policy = self.policy.as_deref(); let analysis = path_analysis(input_path); if analysis.escapes { @@ -187,7 +204,7 @@ impl PathResolver for AllowedGlobResolver { } let candidate = self.workspace_root.join(input_path); - resolve_candidate(&self.workspace_root, policy, path, &candidate) + resolve_candidate(self, path, &candidate) } } @@ -202,24 +219,23 @@ impl PathResolver for AllowedGlobResolver { /// After each tier, re-check glob policy against the normalized absolute path. /// Accept the first tier that passes both the containment check and policy. fn resolve_candidate( - workspace_root: &Path, - policy: Option<&GlobPolicy>, + resolver: &AllowedGlobResolver, path: &str, candidate: &Path, ) -> ToolResult { // Step 1: canonicalize for existing files - resolves symlinks and normalizes. if let Ok(resolved) = candidate.canonicalize() { - return validate_resolved(resolved, path, workspace_root, policy); + return validate_resolved(resolver, resolved, path); } // Step 2: fast path for new files in existing directories. if let Some(resolved) = resolve_new_file_fast(candidate) { - return validate_resolved(resolved, path, workspace_root, policy); + return validate_resolved(resolver, resolved, path); } // Step 3: fallback for paths with missing parent dirs. if let Ok(resolved) = soft_canonicalize(candidate) { - return validate_resolved(resolved, path, workspace_root, policy); + return validate_resolved(resolver, resolved, path); } Err(reject(path)) @@ -232,27 +248,19 @@ fn reject(path: &str) -> ToolError { /// Validates a resolved path against workspace containment and policy. /// -/// Checks that the resolved path: -/// 1. Is within the workspace root directory -/// 2. Passes the glob policy (if one is configured) -/// -/// Returns the resolved path if valid, or a rejection error otherwise. +/// Delegates to [`AllowedGlobResolver::is_path_allowed`] for a single source +/// of truth on the policy check. fn validate_resolved( + resolver: &AllowedGlobResolver, resolved: PathBuf, path: &str, - workspace_root: &Path, - policy: Option<&GlobPolicy>, ) -> ToolResult { - if !resolved.starts_with(workspace_root) { - return Err(reject(path)); - } - if let Some(policy) = policy { - let abs_str = normalize_path(&resolved); - if !policy.is_allowed(abs_str.as_ref()) { - return Err(reject(path)); - } + if resolved.as_path() == resolver.workspace_root.as_ref() || resolver.is_path_allowed(&resolved) + { + Ok(resolved) + } else { + Err(reject(path)) } - Ok(resolved) } #[cfg(test)] diff --git a/src/llm-coding-tools-core/src/path/allowed_glob/normalize.rs b/src/llm-coding-tools-core/src/path/allowed_glob/normalize.rs index 3bc63d5..2b71e12 100644 --- a/src/llm-coding-tools-core/src/path/allowed_glob/normalize.rs +++ b/src/llm-coding-tools-core/src/path/allowed_glob/normalize.rs @@ -38,9 +38,9 @@ pub(crate) fn expand_pattern( /// Expands shell-like patterns in a path string, returning a [`PathBuf`]. /// -/// Wraps [`expand_pattern`] with fail-fast error handling: returns +/// Wraps the internal expansion logic with fail-fast error handling: returns /// `ToolError::InvalidPath` if expansion fails (e.g., unset variable). -pub(crate) fn expand_shell(path: &str) -> ToolResult { +pub fn expand_shell(path: &str) -> ToolResult { expand_pattern(path) .map(|cow| PathBuf::from(cow.into_owned())) .map_err(|e| { 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 2aa2a98..2447ad7 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 @@ -124,7 +124,7 @@ impl GlobPolicy { /// /// `true` if the path is allowed by the last matching rule, `false` otherwise. #[inline] - pub(crate) fn is_allowed(&self, normalized_path: &str) -> bool { + pub fn is_allowed(&self, normalized_path: &str) -> bool { if self.rules.is_empty() { return false; } diff --git a/src/llm-coding-tools-core/src/path/mod.rs b/src/llm-coding-tools-core/src/path/mod.rs index 67e8620..b8df6d8 100644 --- a/src/llm-coding-tools-core/src/path/mod.rs +++ b/src/llm-coding-tools-core/src/path/mod.rs @@ -7,10 +7,11 @@ mod absolute; mod allowed; -pub(crate) mod allowed_glob; +pub mod allowed_glob; pub use absolute::AbsolutePathResolver; pub use allowed::AllowedPathResolver; +pub use allowed_glob::normalize::expand_shell; pub use allowed_glob::{AllowedGlobResolver, GlobPolicy, GlobPolicyBuilder, RuleAction}; use crate::context::PathMode; @@ -22,17 +23,24 @@ use std::path::{Component, Path, PathBuf}; /// Implementations control whether paths must be absolute, relative to /// allowed directories, or follow other constraints. pub trait PathResolver: Send + Sync { - /// Describes how tools should present paths for this resolver. - /// - /// Custom resolvers default to [`PathMode::Absolute`] unless they opt into - /// [`PathMode::Allowed`]. - const PATH_MODE: PathMode = PathMode::Absolute; - /// Resolves and validates a path string. /// /// Returns an absolute path (may or may not be canonical) if valid, /// or an error describing the issue. fn resolve(&self, path: &str) -> ToolResult; + + /// Fast per-entry check: is this absolute path allowed? + /// + /// WalkBuilder yields real absolute paths. No canonicalization needed. + /// Used by glob/grep to filter walked entries without [`resolve()`](Self::resolve) + /// overhead. + /// + /// Implementations must ensure paths where [`resolve()`](Self::resolve) succeeds + /// also satisfy `is_path_allowed()` - the two must be consistent. + fn is_path_allowed(&self, path: &Path) -> bool; + + /// Returns the path mode for this resolver instance. + fn path_mode(&self) -> PathMode; } /// Fast lexical check for whether a relative path would escape its base directory. diff --git a/src/llm-coding-tools-core/src/tools/edit.rs b/src/llm-coding-tools-core/src/tools/edit.rs index 8bfe007..781082d 100644 --- a/src/llm-coding-tools-core/src/tools/edit.rs +++ b/src/llm-coding-tools-core/src/tools/edit.rs @@ -3,12 +3,9 @@ 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::edit as edit_meta; use serde::Deserialize; use serde_json::Value; -use std::sync::Arc; use thiserror::Error; /// Errors specific to edit tools. @@ -80,54 +77,17 @@ impl From for ToolError { } } -/// Runtime settings that control permission filtering for edit requests. +/// Runtime settings 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 +/// Reserved for future use. #[derive(Debug, Clone, Default, PartialEq, Eq)] -pub struct EditSettings { - permission: Option>, -} +pub struct EditSettings {} impl EditSettings { - /// Creates default edit settings with no extra permission filtering. - /// - /// # Returns - /// - An [`EditSettings`] with permission set to `None`. + /// Creates default edit settings. #[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() + Self {} } } @@ -138,7 +98,7 @@ impl EditSettings { pub async fn edit_file( resolver: &R, request: EditRequest, - settings: &EditSettings, + _settings: &EditSettings, ) -> Result { if request.old_string.is_empty() { return Err(EditError::EmptyOldString); @@ -148,10 +108,6 @@ 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 { @@ -205,12 +161,9 @@ pub async fn edit_file( mod tests { use super::*; use crate::path::AbsolutePathResolver; - use crate::permissions::{ExpandError, PermissionAction, Rule}; use std::io::Write; use tempfile::NamedTempFile; - type TestResult = Result<(), ExpandError>; - fn create_temp_file(content: &str) -> NamedTempFile { let mut file = NamedTempFile::new().unwrap(); file.write_all(content.as_bytes()).unwrap(); @@ -284,37 +237,4 @@ mod tests { err ); } - - #[maybe_async::test(feature = "blocking", async(feature = "tokio", tokio::test))] - async fn edit_rejects_denied_path() -> TestResult { - 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", .. }) - )); - Ok(()) - } } diff --git a/src/llm-coding-tools-core/src/tools/glob.rs b/src/llm-coding-tools-core/src/tools/glob.rs index cab5da8..eb0c971 100644 --- a/src/llm-coding-tools-core/src/tools/glob.rs +++ b/src/llm-coding-tools-core/src/tools/glob.rs @@ -3,14 +3,11 @@ use crate::error::{ToolError, ToolResult}; use crate::path::allowed_glob::normalize::normalize_path; 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. @@ -36,8 +33,6 @@ impl GlobRequest { pub struct GlobSettings { /// Maximum number of file paths to return. limit: usize, - /// Optional permission ruleset controlling which paths may be traversed. - permission: Option>, } impl Default for GlobSettings { @@ -52,7 +47,6 @@ impl GlobSettings { pub fn new() -> Self { Self { limit: glob_meta::MAX_RESULTS, - permission: None, } } @@ -74,22 +68,6 @@ 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 @@ -98,18 +76,6 @@ impl GlobSettings { 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. @@ -134,9 +100,10 @@ pub struct GlobOutput { /// The `limit` parameter controls the maximum number of files returned. /// /// # Arguments -/// - `resolver` - [`PathResolver`] used to resolve `request.path` to an absolute directory. +/// - `resolver` - [`PathResolver`] used to resolve `request.path` to an absolute directory +/// and to filter walked entries via [`PathResolver::is_path_allowed`]. /// - `request` - The glob request containing the pattern and search directory. -/// - `settings` - Runtime settings including result limit and optional permission ruleset. +/// - `settings` - Runtime settings including result limit. /// /// # Returns /// - [`GlobOutput`] with matched file paths, sorted newest-first, plus truncation and @@ -144,9 +111,8 @@ pub struct GlobOutput { /// /// # Errors /// - Returns [`ToolError::InvalidPath`] when the resolved path is not a directory. -/// - Returns a [`ToolError`] when the glob pattern fails to compile. -/// - Returns a [`ToolError`] when path resolution fails. -/// - Returns a [`ToolError`] when the permission check denies access to the search directory. +/// - Returns [`ToolError::InvalidPattern`] when the glob pattern fails to compile. +/// - Returns [`ToolError::InvalidPath`] when path resolution fails. pub fn glob_files( resolver: &R, request: GlobRequest, @@ -154,12 +120,6 @@ pub fn glob_files( ) -> ToolResult { // Resolve the requested path to an absolute directory. let path = resolver.resolve(&request.path)?; - let search_subject = path.to_string_lossy(); - - // Check that the caller is allowed to glob inside this directory. - settings - .permission() - .check(glob_meta::NAME, search_subject.as_ref())?; if !path.is_dir() { return Err(ToolError::InvalidPath(format!( @@ -230,12 +190,9 @@ pub fn glob_files( continue; } - // Drop files the caller isn't allowed to see. - if let Some(ruleset) = settings.permission() { - let entry_subject = entry.path().to_string_lossy(); - if !ruleset.is_allowed(glob_meta::NAME, entry_subject.as_ref()) { - continue; - } + // Drop files the resolver doesn't allow. + if !resolver.is_path_allowed(entry.path()) { + continue; } // Read mtime, falling back to epoch when unavailable. @@ -277,18 +234,20 @@ pub fn glob_files( #[cfg(test)] mod tests { //! Tests for the [`glob_files`] operation covering pattern matching, sorting, - //! gitignore handling, permission filtering, limit enforcement, and serialization. + //! gitignore handling, limit enforcement, and serialization. use super::*; use crate::path::AbsolutePathResolver; - use crate::permissions::{ExpandError, PermissionAction, Rule}; + use crate::path::AllowedGlobResolver; + use crate::path::GlobPolicy; use rstest::rstest; + use soft_canonicalize::soft_canonicalize; use std::fs::{self, File, FileTimes}; use std::io::Write; use std::path::Path; use std::time::{Duration, SystemTime}; use tempfile::TempDir; - type TestResult = Result<(), ExpandError>; + type TestResult = Result<(), Box>; fn test_settings() -> GlobSettings { GlobSettings::new().with_limit(1000).unwrap() @@ -511,35 +470,43 @@ mod tests { assert!(result.files.iter().any(|f| f.contains('/'))); } + /// Verifies that glob_files filters results via `is_path_allowed` using both + /// relative and absolute search paths. #[rstest] - #[case::txt_files("txt")] - #[case::rs_files("rs")] - fn glob_permission_filtering_excludes_denied_files(#[case] ext: &str) -> TestResult { - let dir = TempDir::new().unwrap(); - let base = dir.path(); - let allowed = base.join(format!("allowed.{ext}")); - let denied = base.join(format!("denied.{ext}")); - 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 = run_glob_with_settings( - base, - &format!("**/*.{ext}"), - &GlobSettings::new() - .with_limit(1000) - .unwrap() - .with_permission(Some(Arc::new(ruleset))), - ); + #[case::relative_path(".")] + #[case::absolute_path_uses_workdir_as_param( + // Placeholder: replaced with the temp dir path inside the test body. + "ABSOLUTE" + )] + fn glob_filters_via_is_path_allowed(#[case] path_kind: &str) -> TestResult { + let dir = TempDir::new()?; + fs::create_dir_all(dir.path().join("src"))?; + File::create(dir.path().join("src/lib.rs"))?; + File::create(dir.path().join("Cargo.toml"))?; + + let root = soft_canonicalize(dir.path())?; + let policy = GlobPolicy::builder_with_base(&root)? + .allow("src/**")? + .build()?; + let resolver = AllowedGlobResolver::new(dir.path())?.with_policy(policy); + + let search_path = if path_kind == "ABSOLUTE" { + dir.path().to_str().unwrap().to_string() + } else { + path_kind.to_string() + }; + + let result = glob_files( + &resolver, + GlobRequest { + pattern: "**/*".to_string(), + path: search_path, + }, + &GlobSettings::new(), + )?; - assert_eq!(result.files, vec![format!("allowed.{ext}")]); + assert!(result.files.iter().any(|f| f.contains("lib.rs"))); + assert!(!result.files.iter().any(|f| f.contains("Cargo.toml"))); Ok(()) } diff --git a/src/llm-coding-tools-core/src/tools/grep.rs b/src/llm-coding-tools-core/src/tools/grep.rs index 126c495..932ccd5 100644 --- a/src/llm-coding-tools-core/src/tools/grep.rs +++ b/src/llm-coding-tools-core/src/tools/grep.rs @@ -2,8 +2,6 @@ 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::{push_usize, truncate_line_with_ellipsis, TRUNCATION_ELLIPSIS}; use globset::Glob; @@ -14,7 +12,6 @@ use ignore::WalkBuilder; use serde::{Deserialize, Serialize}; use serde_json::Value; use std::path::Path; -use std::sync::Arc; use std::time::SystemTime; /// Default maximum line length (in characters) for formatted grep output. @@ -48,7 +45,6 @@ impl GrepRequest { #[derive(Debug, Clone, PartialEq, Eq)] pub struct GrepSettings { max_limit: usize, - permission: Option>, } impl Default for GrepSettings { @@ -63,7 +59,6 @@ impl GrepSettings { pub fn new() -> Self { Self { max_limit: grep_meta::DEFAULT_LIMIT, - permission: None, } } @@ -85,22 +80,6 @@ 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 @@ -109,18 +88,6 @@ impl GrepSettings { 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. @@ -322,10 +289,6 @@ 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()))?; @@ -366,13 +329,9 @@ 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; - } + // Filter entries through the resolver's path policy. + if !resolver.is_path_allowed(entry_path) { + continue; } // Apply include glob to basename when requested. @@ -476,11 +435,14 @@ fn collect_file_matches( mod tests { use super::*; use crate::path::AbsolutePathResolver; - use crate::permissions::{ExpandError, PermissionAction, Rule}; + use crate::path::AllowedGlobResolver; + use crate::path::GlobPolicy; use rstest::rstest; + use soft_canonicalize::soft_canonicalize; + use std::fs; use tempfile::tempdir; - type TestResult = Result<(), ExpandError>; + type TestResult = Result<(), Box>; // GrepSettings and GrepFormattingSettings tests #[test] @@ -819,38 +781,51 @@ mod tests { assert!(result.truncated); } - #[test] - fn grep_skips_denied_files_before_counting_matches() -> TestResult { - 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, - )?); + /// Verifies that grep_search filters results via `is_path_allowed` using both + /// relative and absolute search paths. + #[rstest] + #[case::relative_path(".")] + #[case::absolute_path_uses_workdir_as_param( + // Placeholder: replaced with the temp dir path inside the test body. + "ABSOLUTE" + )] + fn grep_filters_via_is_path_allowed(#[case] path_kind: &str) -> TestResult { + let temp = tempdir()?; + fs::create_dir_all(temp.path().join("src"))?; + std::fs::write(temp.path().join("src/lib.rs"), "match_content\n")?; + std::fs::write(temp.path().join("Cargo.toml"), "match_content\n")?; + + let root = soft_canonicalize(temp.path())?; + let policy = GlobPolicy::builder_with_base(&root)? + .allow("src/**")? + .build()?; + let resolver = AllowedGlobResolver::new(temp.path())?.with_policy(policy); + + let search_path = if path_kind == "ABSOLUTE" { + temp.path().to_str().unwrap().to_string() + } else { + path_kind.to_string() + }; let result = grep_search( &resolver, GrepRequest { - pattern: "hello".into(), - path: temp.path().to_string_lossy().into_owned(), + pattern: "match_content".into(), + path: search_path, include: None, limit: None, }, - &GrepSettings::new().with_permission(Some(Arc::new(ruleset))), - ) - .unwrap(); + &GrepSettings::new().with_max_limit(100)?, + )?; - assert_eq!(result.match_count, 1); - assert_eq!(result.files.len(), 1); - assert_eq!(result.files[0].path, allowed.to_string_lossy()); + assert!( + result.files.iter().any(|f| f.path.contains("lib.rs")), + "src/lib.rs should appear in results" + ); + assert!( + !result.files.iter().any(|f| f.path.contains("Cargo.toml")), + "Cargo.toml should be filtered by src/** policy" + ); Ok(()) } } diff --git a/src/llm-coding-tools-core/src/tools/read.rs b/src/llm-coding-tools-core/src/tools/read.rs index 13cad76..d09b10d 100644 --- a/src/llm-coding-tools-core/src/tools/read.rs +++ b/src/llm-coding-tools-core/src/tools/read.rs @@ -4,8 +4,6 @@ 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::{ push_usize, truncate_line_with_ellipsis, ESTIMATED_CHARS_PER_LINE, TRUNCATION_ELLIPSIS, @@ -13,7 +11,6 @@ use crate::util::{ use memchr::{memchr, memchr_iter}; use serde::Deserialize; use serde_json::Value; -use std::sync::Arc; /// Serde-friendly read request owned by the core crate. #[derive(Debug, Deserialize)] @@ -45,7 +42,6 @@ pub struct ReadSettings { max_limit: usize, max_line_length: usize, line_numbers: bool, - permission: Option>, } impl Default for ReadSettings { @@ -63,7 +59,6 @@ impl ReadSettings { max_limit: read_meta::DEFAULT_LIMIT, max_line_length: read_meta::MAX_LINE_LENGTH, line_numbers: true, - permission: None, } } @@ -145,22 +140,6 @@ impl ReadSettings { 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 @@ -196,18 +175,6 @@ impl ReadSettings { 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() - } } #[cfg(feature = "blocking")] @@ -226,7 +193,7 @@ type BufFile = tokio::io::BufReader; /// # Arguments /// - `resolver`: [`PathResolver`] used to resolve `request.file_path` to a filesystem path. /// - `request`: [`ReadRequest`] carrying the file path, 1-indexed offset, and optional line limit. -/// - `settings`: [`ReadSettings`] controlling line numbers, max line length, and permission checks. +/// - `settings`: [`ReadSettings`] controlling line numbers and max line length. /// /// # Returns /// - [`ToolOutput`] containing the requested line range, each line optionally @@ -235,7 +202,6 @@ type BufFile = tokio::io::BufReader; /// # Errors /// - Returns [`ToolError::OutOfBounds`] when `offset` is `0` or exceeds the file's line count. /// - Returns [`ToolError::validation_for`] when `limit` resolves to `0`. -/// - Returns a permission error when the resolved path is denied by the configured [`Ruleset`]. /// - Returns an I/O error when the file cannot be opened or read. #[maybe_async::maybe_async] pub async fn read_file( @@ -270,12 +236,8 @@ pub async fn read_file( )); } - // Resolve the logical path to a filesystem path and check permissions. + // Resolve the logical path to a filesystem path. let path = resolver.resolve(&request.file_path)?; - let subject = path.to_string_lossy(); - settings - .permission() - .check(read_meta::NAME, subject.as_ref())?; // Open the file with a buffered reader sized proportionally to the // expected output, capped at 1 MiB to avoid over-allocating on huge limits. @@ -591,13 +553,10 @@ fn ensure_max_line_length(max_line_length: usize) -> ToolResult<()> { mod tests { use super::*; use crate::path::AbsolutePathResolver; - use crate::permissions::{ExpandError, PermissionAction, Rule}; use rstest::rstest; use std::io::Write as _; use tempfile::NamedTempFile; - type TestResult = Result<(), ExpandError>; - #[maybe_async::maybe_async] async fn read_temp_file( content: &[u8], @@ -792,37 +751,4 @@ mod tests { assert!(matches!(err, ToolError::Validation { .. })); } - - #[maybe_async::test(feature = "blocking", async(feature = "tokio", tokio::test))] - async fn read_request_rejects_denied_path() -> TestResult { - 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", .. } - )); - Ok(()) - } } diff --git a/src/llm-coding-tools-core/src/tools/write.rs b/src/llm-coding-tools-core/src/tools/write.rs index 0712db5..4ab20e6 100644 --- a/src/llm-coding-tools-core/src/tools/write.rs +++ b/src/llm-coding-tools-core/src/tools/write.rs @@ -3,12 +3,8 @@ 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)] @@ -24,54 +20,17 @@ impl WriteRequest { } } -/// Runtime settings that control permission filtering for write requests. +/// Runtime settings 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 +/// Reserved for future use. #[derive(Debug, Clone, Default, PartialEq, Eq)] -pub struct WriteSettings { - permission: Option>, -} +pub struct WriteSettings {} impl WriteSettings { - /// Creates default write settings with no extra permission filtering. - /// - /// # Returns - /// - A [`WriteSettings`] with permission set to `None`. + /// Creates default write settings. #[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() + Self {} } } @@ -82,13 +41,9 @@ impl WriteSettings { pub async fn write_file( resolver: &R, request: WriteRequest, - settings: &WriteSettings, + _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() { @@ -113,11 +68,8 @@ pub async fn write_file( mod tests { use super::*; use crate::path::AbsolutePathResolver; - use crate::permissions::{ExpandError, PermissionAction, Rule}; use tempfile::TempDir; - type TestResult = Result<(), ExpandError>; - #[maybe_async::test(feature = "blocking", async(feature = "tokio", tokio::test))] async fn write_creates_new_file() { let temp = TempDir::new().unwrap(); @@ -158,36 +110,4 @@ mod tests { assert!(file_path.exists()); } - - #[maybe_async::test(feature = "blocking", async(feature = "tokio", tokio::test))] - async fn write_rejects_denied_path() -> TestResult { - 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", .. } - )); - Ok(()) - } } diff --git a/src/llm-coding-tools-serdesai/Cargo.toml b/src/llm-coding-tools-serdesai/Cargo.toml index 594857b..0355ab8 100644 --- a/src/llm-coding-tools-serdesai/Cargo.toml +++ b/src/llm-coding-tools-serdesai/Cargo.toml @@ -84,6 +84,9 @@ reqwest = { version = "0.13", default-features = false, features = [ "rustls-native-certs", ] } +# Used for IndexMap in build.rs (permission config) +indexmap = "2" + [dev-dependencies] serial_test = "3" tokio = { version = "1", features = ["macros", "rt-multi-thread"] } diff --git a/src/llm-coding-tools-serdesai/README.md b/src/llm-coding-tools-serdesai/README.md index 8310a03..40bb620 100644 --- a/src/llm-coding-tools-serdesai/README.md +++ b/src/llm-coding-tools-serdesai/README.md @@ -119,6 +119,7 @@ let build_context = AgentBuildContext::new( Arc::new(runtime), Arc::new(load_result.catalog), Arc::new(CredentialResolver::new()), + Arc::from(llm_coding_tools_core::resolve_workspace_root()?.as_path()), ); let agent = build_context.build("planner")?; let response = agent.run("Say hello in one sentence.", ()).await?; diff --git a/src/llm-coding-tools-serdesai/examples/serdesai-agents.rs b/src/llm-coding-tools-serdesai/examples/serdesai-agents.rs index 68032b5..1720d18 100644 --- a/src/llm-coding-tools-serdesai/examples/serdesai-agents.rs +++ b/src/llm-coding-tools-serdesai/examples/serdesai-agents.rs @@ -10,7 +10,7 @@ //! cargo run --example serdesai-agents -p llm-coding-tools-serdesai use llm_coding_tools_agents::{AgentCatalog, AgentLoader, AgentRuntimeBuilder}; -use llm_coding_tools_core::CredentialResolver; +use llm_coding_tools_core::{CredentialResolver, resolve_workspace_root}; use llm_coding_tools_models_dev::ModelsDevCatalog; use llm_coding_tools_serdesai::{AgentBuildContext, AgentDefaults}; use std::{path::PathBuf, sync::Arc}; @@ -47,6 +47,7 @@ async fn main() -> Result<(), Box> { Arc::new(runtime), Arc::new(load_result.catalog), Arc::new(credentials), + Arc::from(resolve_workspace_root()?), ); println!( diff --git a/src/llm-coding-tools-serdesai/examples/serdesai-task.rs b/src/llm-coding-tools-serdesai/examples/serdesai-task.rs index e5c1054..f3b429c 100644 --- a/src/llm-coding-tools-serdesai/examples/serdesai-task.rs +++ b/src/llm-coding-tools-serdesai/examples/serdesai-task.rs @@ -9,7 +9,7 @@ use futures::StreamExt; use llm_coding_tools_agents::{AgentCatalog, AgentLoader, AgentRuntimeBuilder}; -use llm_coding_tools_core::{CredentialResolver, TaskInput}; +use llm_coding_tools_core::{CredentialResolver, TaskInput, resolve_workspace_root}; use llm_coding_tools_models_dev::ModelsDevCatalog; use llm_coding_tools_serdesai::{AgentBuildContext, AgentDefaults}; use serdes_ai::{AgentStreamEvent, UserContent}; @@ -56,6 +56,7 @@ async fn main() -> Result<(), Box> { Arc::new(runtime), Arc::new(load_result.catalog), Arc::new(credentials), + Arc::from(resolve_workspace_root()?), ); println!( diff --git a/src/llm-coding-tools-serdesai/src/agent_ext.rs b/src/llm-coding-tools-serdesai/src/agent_ext.rs index 7265fe0..db3821f 100644 --- a/src/llm-coding-tools-serdesai/src/agent_ext.rs +++ b/src/llm-coding-tools-serdesai/src/agent_ext.rs @@ -20,6 +20,7 @@ //! # } //! ``` +use crate::AgentBuildError; use async_trait::async_trait; use serde_json::Value as JsonValue; use serdes_ai::agent::ToolExecutor; @@ -84,3 +85,30 @@ where self.tool_with_executor(definition, ToolAsExecutor(tool)) } } + +/// Extension for converting [`ToolError`] results into [`AgentBuildError`]. +/// +/// This avoids repeating the full `ToolSettingsValidation` struct literal at +/// every `.map_err` call site. +/// +/// # Example +/// +/// ```no_run +/// use llm_coding_tools_serdesai::agent_ext::ToolResultExt; +/// # use llm_coding_tools_serdesai::AgentBuildError; +/// # fn demo(r: Result) -> Result<(), AgentBuildError> { +/// let value = r.with_tool("my_tool")?; +/// # Ok(()) +/// # } +/// ``` +pub trait ToolResultExt { + /// Maps a [`ToolError`](llm_coding_tools_core::ToolError) to + /// [`AgentBuildError::ToolSettingsValidation`]. + fn with_tool(self, tool: &'static str) -> Result; +} + +impl ToolResultExt for Result { + fn with_tool(self, tool: &'static str) -> Result { + self.map_err(|source| AgentBuildError::ToolSettingsValidation { tool, source }) + } +} 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 c8cdf7c..6a05be5 100644 --- a/src/llm-coding-tools-serdesai/src/agent_runtime/build.rs +++ b/src/llm-coding-tools-serdesai/src/agent_runtime/build.rs @@ -6,19 +6,21 @@ use super::model::resolve_model; use super::provider_bridge::build_serdes_model; -use crate::agent_ext::AgentBuilderExt; +use crate::agent_ext::{AgentBuilderExt, ToolResultExt}; use crate::task::{TaskHandle, TaskTool}; use crate::{ - AbsolutePathResolver, BashTool, EditTool, GlobTool, GrepTool, ReadTool, SystemPromptBuilder, - WebFetchTool, WriteTool, create_todo_tools, + BashTool, EditTool, GlobTool, GrepTool, ReadTool, SystemPromptBuilder, WebFetchTool, WriteTool, + create_todo_tools, }; +use indexmap::IndexMap; use llm_coding_tools_agents::{ - AgentRuntime, AgentToolSettings, ModelResolutionError, TaskTargetSummary, ToolCatalogEntry, - ToolCatalogKind, + AgentRuntime, AgentToolSettings, ModelResolutionError, PermissionRule, TaskTargetSummary, + ToolCatalogEntry, ToolCatalogKind, build_resolver_for_tool, }; 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, + edit as edit_meta, glob as glob_meta, grep as grep_meta, read as read_meta, + webfetch as webfetch_meta, write as write_meta, }; use llm_coding_tools_core::tools::{ GlobSettings, GrepFormattingSettings, GrepSettings, ReadSettings, WebFetchSettings, @@ -26,6 +28,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::path::Path; use std::sync::Arc; /// Error returned when a build cannot produce a SerdesAI agent. @@ -61,8 +64,7 @@ pub enum AgentBuildError { } /// Resolved build parameters ready for agent construction. -#[derive(Clone)] -pub(super) struct PreparedBuild { +pub(super) struct PreparedBuild<'a> { /// Agent name for [`AgentBuilder::name`]. agent_name: Box, /// Concrete SerdesAI model. @@ -85,23 +87,26 @@ pub(super) struct PreparedBuild { /// Pre-built permission ruleset for tool access control. /// None if agent has no permissions (backward compatibility). permission: Option>, + /// Raw permission config for file-tool resolver selection. + permission_config: &'a IndexMap, } -impl PreparedBuild { +impl PreparedBuild<'_> { /// Returns the resolved SerdesAI model for builder construction. #[inline] pub(super) fn model(&self) -> &BoxedModel { &self.model } } + /// Resolves model configuration and collects build parameters for an agent. -pub(super) fn prepare_build( - runtime: &AgentRuntime, +pub(super) fn prepare_build<'a, C>( + runtime: &'a AgentRuntime, name: &str, model_catalog: &ModelCatalog, credentials: &C, with_summaries: bool, -) -> Result +) -> Result, AgentBuildError> where C: CredentialLookup, { @@ -136,14 +141,16 @@ where tool_settings: agent.tool_settings.clone(), callable_target_summaries, permission, + permission_config: &agent.permission, }) } /// Attaches the standard runtime tools and prompt contexts without finalizing the builder. -pub(super) fn attach_standard_tools( +pub(super) fn attach_standard_tools<'a, C>( mut builder: AgentBuilder<(), String>, - prepared: &PreparedBuild, + prepared: &PreparedBuild<'a>, task_handle: Option<&TaskHandle>, + workspace_root: &Path, ) -> Result<(AgentBuilder<(), String>, SystemPromptBuilder), AgentBuildError> where C: CredentialLookup + Send + Sync + 'static, @@ -159,53 +166,51 @@ where builder = builder.top_p(top_p); } - // Use pre-built permission ruleset from PreparedBuild - // No need to rebuild - already constructed in prepare_build + // Use pre-built permission ruleset from PreparedBuild for non-file tools. let permission = prepared.permission.clone(); + let permission_config = &prepared.permission_config; for entry in prepared.tools.iter() { match entry.kind { ToolCatalogKind::Read => { + let resolver = + build_resolver_for_tool(permission_config, read_meta::NAME, workspace_root) + .with_tool(read_meta::NAME)?; let settings = build_read_settings(&prepared.tool_settings.read)?; builder = - builder.tool(prompt_builder.track(ReadTool::with_settings_and_permission( - AbsolutePathResolver, - settings, - permission.clone(), - ))); + builder.tool(prompt_builder.track(ReadTool::with_settings(resolver, settings))); } ToolCatalogKind::Write => { - builder = builder.tool(prompt_builder.track( - WriteTool::new(AbsolutePathResolver).with_permission(permission.clone()), - )); + let resolver = + build_resolver_for_tool(permission_config, write_meta::NAME, workspace_root) + .with_tool(write_meta::NAME)?; + builder = builder.tool(prompt_builder.track(WriteTool::new(resolver))); } ToolCatalogKind::Edit => { - builder = builder.tool(prompt_builder.track( - EditTool::new(AbsolutePathResolver).with_permission(permission.clone()), - )); + let resolver = + build_resolver_for_tool(permission_config, edit_meta::NAME, workspace_root) + .with_tool(edit_meta::NAME)?; + builder = builder.tool(prompt_builder.track(EditTool::new(resolver))); } ToolCatalogKind::Glob => { + let resolver = + build_resolver_for_tool(permission_config, glob_meta::NAME, workspace_root) + .with_tool(glob_meta::NAME)?; let settings = build_glob_settings(&prepared.tool_settings.glob)?; - builder = builder.tool( - prompt_builder.track( - GlobTool::with_settings(AbsolutePathResolver, settings) - .with_permission(permission.clone()), - ), - ); + builder = + builder.tool(prompt_builder.track(GlobTool::with_settings(resolver, settings))); } ToolCatalogKind::Grep => { + let resolver = + build_resolver_for_tool(permission_config, grep_meta::NAME, workspace_root) + .with_tool(grep_meta::NAME)?; 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, - ) - .with_permission(permission.clone()), - ), - ); + builder = builder.tool(prompt_builder.track(GrepTool::with_settings( + resolver, + search_settings, + formatting_settings, + ))); } ToolCatalogKind::Bash => { let settings = &prepared.tool_settings.bash; @@ -256,10 +261,7 @@ fn build_read_settings( .with_limits(settings.limit, settings.limit) .and_then(|value| value.with_max_line_length(settings.max_line_length)) .map(|value| value.with_line_numbers(settings.line_numbers)) - .map_err(|source| AgentBuildError::ToolSettingsValidation { - tool: read_meta::NAME, - source, - }) + .with_tool(read_meta::NAME) } fn build_grep_settings( @@ -267,18 +269,12 @@ fn build_grep_settings( ) -> Result<(GrepSettings, GrepFormattingSettings), AgentBuildError> { let search_settings = GrepSettings::new() .with_max_limit(settings.limit) - .map_err(|source| AgentBuildError::ToolSettingsValidation { - tool: grep_meta::NAME, - source, - })?; + .with_tool(grep_meta::NAME)?; let formatting_settings = GrepFormattingSettings::new() .with_max_line_length(settings.max_line_length) .map(|value| value.with_line_numbers(settings.line_numbers)) - .map_err(|source| AgentBuildError::ToolSettingsValidation { - tool: grep_meta::NAME, - source, - })?; + .with_tool(grep_meta::NAME)?; Ok((search_settings, formatting_settings)) } @@ -288,10 +284,7 @@ fn build_glob_settings( ) -> Result { GlobSettings::new() .with_limit(settings.limit) - .map_err(|source| AgentBuildError::ToolSettingsValidation { - tool: glob_meta::NAME, - source, - }) + .with_tool(glob_meta::NAME) } fn build_webfetch_settings( @@ -300,10 +293,7 @@ fn build_webfetch_settings( WebFetchSettings::new() .with_timeouts(settings.timeout_ms, settings.max_timeout_ms) .and_then(|value| value.with_max_response_size(settings.max_response_size)) - .map_err(|source| AgentBuildError::ToolSettingsValidation { - tool: webfetch_meta::NAME, - source, - }) + .with_tool(webfetch_meta::NAME) } #[cfg(test)] @@ -332,13 +322,16 @@ mod tests { /// Builds an agent using a mock model instead of a real one. fn build_with_mock( - prepared: &super::PreparedBuild, + prepared: &super::PreparedBuild<'_>, name: &str, ) -> serdes_ai::Agent<(), String> { + let workspace_root = + llm_coding_tools_core::resolve_workspace_root().expect("workspace root"); let (builder, prompt_builder) = attach_standard_tools::( AgentBuilder::<(), String>::new(MockModel::new(name)), prepared, None, + &workspace_root, ) .expect("build should succeed"); builder.system_prompt(prompt_builder.build()).build() diff --git a/src/llm-coding-tools-serdesai/src/agent_runtime/task.rs b/src/llm-coding-tools-serdesai/src/agent_runtime/task.rs index f310296..a9ee4cc 100644 --- a/src/llm-coding-tools-serdesai/src/agent_runtime/task.rs +++ b/src/llm-coding-tools-serdesai/src/agent_runtime/task.rs @@ -8,6 +8,7 @@ use crate::task::TaskHandle; use llm_coding_tools_agents::AgentRuntime; use llm_coding_tools_core::{CredentialLookup, CredentialResolver, models::ModelCatalog}; use serdes_ai::{Agent, AgentBuilder}; +use std::path::Path; use std::sync::Arc; /// Reusable shared inputs for building runnable SerdesAI agents. @@ -25,18 +26,21 @@ impl AgentBuildContext where C: CredentialLookup + Send + Sync + 'static, { - /// Creates a shared build context from runtime state, model catalog, and credentials. + /// Creates a shared build context from runtime state, model catalog, credentials, + /// and the workspace root directory. #[inline] pub fn new( runtime: Arc, model_catalog: Arc, credentials: Arc, + workspace_root: Arc, ) -> Self { Self { context: Arc::new(TaskBuildContext { runtime, model_catalog, credentials, + workspace_root, }), } } @@ -73,6 +77,7 @@ pub(crate) struct TaskBuildContext, model_catalog: Arc, credentials: Arc, + workspace_root: Arc, } impl TaskBuildContext @@ -96,11 +101,13 @@ where runtime: Arc, model_catalog: Arc, credentials: Arc, + workspace_root: Arc, ) -> Self { Self { runtime, model_catalog, credentials, + workspace_root, } } } @@ -126,8 +133,13 @@ where with_summaries, )?; let builder = AgentBuilder::<(), String>::from_arc(prepared.model().clone()); - let task_handle = TaskHandle::new(context, current_depth); - let (builder, prompt_builder) = attach_standard_tools(builder, &prepared, Some(&task_handle))?; + let task_handle = TaskHandle::new(context.clone(), current_depth); + let (builder, prompt_builder) = attach_standard_tools( + builder, + &prepared, + Some(&task_handle), + &context.workspace_root, + )?; Ok(builder.system_prompt(prompt_builder.build()).build()) } @@ -218,6 +230,10 @@ mod tests { Arc::new(resolver) } + fn workspace_root() -> Arc { + Arc::from(llm_coding_tools_core::resolve_workspace_root().expect("workspace root")) + } + #[test] fn build_agent_skips_task_tool_when_no_targets_are_callable() -> TestResult { let credentials = credentials(); @@ -240,6 +256,7 @@ mod tests { runtime: Arc::new(runtime), model_catalog, credentials, + workspace_root: workspace_root(), }); let agent = build_agent(context, "caller", 0).expect("build should succeed"); @@ -275,6 +292,7 @@ mod tests { runtime: Arc::new(runtime), model_catalog, credentials, + workspace_root: workspace_root(), }); let agent = build_agent(context, "caller", 0).expect("build should succeed"); @@ -309,6 +327,7 @@ mod tests { runtime: Arc::new(runtime), model_catalog, credentials, + workspace_root: workspace_root(), }); let agent = build_agent(context, "caller", 0).expect("build should succeed"); @@ -339,6 +358,7 @@ mod tests { runtime: Arc::new(runtime), model_catalog, credentials, + workspace_root: workspace_root(), }); let agent = build_agent(context, "caller", 0).expect("build should succeed"); @@ -366,7 +386,12 @@ mod tests { .defaults(AgentDefaults::with_model("openrouter/openai/gpt-4.1-mini")) .build()?; - let context = AgentBuildContext::new(Arc::new(runtime), model_catalog, credentials); + let context = AgentBuildContext::new( + Arc::new(runtime), + model_catalog, + credentials, + workspace_root(), + ); let agent = context.build("caller").expect("build should succeed"); let names: Vec<_> = agent.tools().iter().map(|t| t.name()).collect(); assert!(!names.contains(&task_meta::NAME)); @@ -401,6 +426,7 @@ mod tests { runtime: Arc::new(runtime), model_catalog, credentials, + workspace_root: workspace_root(), }); let agent = build_agent(context, "caller", 1).expect("build should succeed"); @@ -434,7 +460,12 @@ mod tests { .max_task_depth(0) .build()?; - let context = AgentBuildContext::new(Arc::new(runtime), model_catalog, credentials); + let context = AgentBuildContext::new( + Arc::new(runtime), + model_catalog, + credentials, + workspace_root(), + ); let agent = context.build("caller").expect("build should succeed"); let names: Vec<_> = agent.tools().iter().map(|t| t.name()).collect(); assert!(!names.contains(&task_meta::NAME)); diff --git a/src/llm-coding-tools-serdesai/src/task/handle.rs b/src/llm-coding-tools-serdesai/src/task/handle.rs index 86f18ab..1c76b1b 100644 --- a/src/llm-coding-tools-serdesai/src/task/handle.rs +++ b/src/llm-coding-tools-serdesai/src/task/handle.rs @@ -243,6 +243,7 @@ mod tests { Arc::new(runtime.expect("test fixture should not fail pattern expansion")), Arc::new(catalog()), credentials(), + Arc::from(llm_coding_tools_core::resolve_workspace_root().expect("workspace root")), )) } diff --git a/src/llm-coding-tools-serdesai/src/tools/edit.rs b/src/llm-coding-tools-serdesai/src/tools/edit.rs index a2b590b..9100cc5 100644 --- a/src/llm-coding-tools-serdesai/src/tools/edit.rs +++ b/src/llm-coding-tools-serdesai/src/tools/edit.rs @@ -13,11 +13,9 @@ 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, EditSettings, edit_file}; use serdes_ai::tools::{RunContext, SchemaBuilder, Tool, ToolDefinition, ToolResult, ToolReturn}; -use std::sync::Arc; use crate::convert::core_error_to_serdes; @@ -47,10 +45,9 @@ impl EditTool { /// # Arguments /// /// * `resolver` - A [`PathResolver`] used to resolve and validate file paths. - /// * `settings` - [`EditSettings`] controlling edit behaviour such as - /// permission checks and replacement limits. + /// * `settings` - [`EditSettings`] controlling replacement limits. pub fn with_settings(resolver: R, settings: EditSettings) -> Self { - let path_mode = R::PATH_MODE; + let path_mode = resolver.path_mode(); Self { definition: build_definition(path_mode), resolver, @@ -59,18 +56,6 @@ impl EditTool { } } - /// 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 { diff --git a/src/llm-coding-tools-serdesai/src/tools/glob.rs b/src/llm-coding-tools-serdesai/src/tools/glob.rs index c59f4a0..46c4234 100644 --- a/src/llm-coding-tools-serdesai/src/tools/glob.rs +++ b/src/llm-coding-tools-serdesai/src/tools/glob.rs @@ -13,12 +13,10 @@ 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; @@ -52,7 +50,7 @@ impl GlobTool { /// * `resolver` - The path resolver for path validation. /// * `settings` - Core glob settings for result limits. pub fn with_settings(resolver: R, settings: GlobSettings) -> Self { - let path_mode = R::PATH_MODE; + let path_mode = resolver.path_mode(); Self { definition: build_definition(path_mode), resolver, @@ -61,18 +59,6 @@ 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 { diff --git a/src/llm-coding-tools-serdesai/src/tools/grep.rs b/src/llm-coding-tools-serdesai/src/tools/grep.rs index bc8e498..5408653 100644 --- a/src/llm-coding-tools-serdesai/src/tools/grep.rs +++ b/src/llm-coding-tools-serdesai/src/tools/grep.rs @@ -14,14 +14,12 @@ 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}; @@ -62,7 +60,7 @@ impl GrepTool { search_settings: GrepSettings, formatting_settings: GrepFormattingSettings, ) -> Self { - let path_mode = R::PATH_MODE; + let path_mode = resolver.path_mode(); Self { definition: build_definition(path_mode, formatting_settings.line_numbers()), resolver, @@ -72,18 +70,6 @@ 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 { diff --git a/src/llm-coding-tools-serdesai/src/tools/read.rs b/src/llm-coding-tools-serdesai/src/tools/read.rs index fc7c575..5a91440 100644 --- a/src/llm-coding-tools-serdesai/src/tools/read.rs +++ b/src/llm-coding-tools-serdesai/src/tools/read.rs @@ -27,11 +27,9 @@ 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}; @@ -67,7 +65,7 @@ impl ReadTool { /// * `resolver` - The path resolver for path validation and resolution. /// * `settings` - Core read settings for limits and formatting. pub fn with_settings(resolver: R, settings: ReadSettings) -> Self { - let path_mode = R::PATH_MODE; + let path_mode = resolver.path_mode(); Self { definition: build_definition(path_mode, settings.line_numbers()), resolver, @@ -76,33 +74,6 @@ 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. diff --git a/src/llm-coding-tools-serdesai/src/tools/write.rs b/src/llm-coding-tools-serdesai/src/tools/write.rs index 22fbdf0..18597b6 100644 --- a/src/llm-coding-tools-serdesai/src/tools/write.rs +++ b/src/llm-coding-tools-serdesai/src/tools/write.rs @@ -12,12 +12,10 @@ 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, 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}; @@ -47,10 +45,9 @@ impl WriteTool { /// # Arguments /// /// * `resolver` - A [`PathResolver`] used to resolve and validate file paths. - /// * `settings` - [`WriteSettings`] controlling write behaviour such as - /// permission checks and overwrite handling. + /// * `settings` - [`WriteSettings`] controlling overwrite handling. pub fn with_settings(resolver: R, settings: WriteSettings) -> Self { - let path_mode = R::PATH_MODE; + let path_mode = resolver.path_mode(); Self { definition: build_definition(path_mode), resolver, @@ -59,18 +56,6 @@ impl WriteTool { } } - /// 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 {