Added: External directory permissions and cached rulesets#91
Conversation
- Make Rule/Ruleset owned types (Box<str>) so they can be cached across the runtime - Cache permission rulesets in AgentRuntime to avoid rebuilding on every tool call - Add external directory support to bash, edit, glob, grep, read, and write tools - Add PermissionDenied error variant and OptionRulesetExt helper trait - Add permission evaluation benchmarks
- Reverse-scan permission rules for last-match-wins, add a single-rule fast path, and defer subject hashing until an exact subject match is needed.
- Add a single-rule fast path for glob policy checks and fix the permissions benchmark after the Box<str> API change, extending it to 128-rule cases.
- Precompute allowed tools and callable Task target summaries at runtime build, add `can_delegate_to()` for cached Task validation, and benchmark the runtime Task path directly.
- Measured impact from the new benches: permission evaluation improved by about 33-43% on the common 32-rule cases (`exact_32_rules` 19.40ns -> 12.62ns, `wildcard_subject_32_rules` 31.81ns -> 18.11ns, `wildcard_both_32_rules` 34.30ns -> 21.32ns), while `check_permission` improved by about 36-42% on the same hot paths.
- Measured impact for runtime Task resolution: `allowed_tools("caller")` dropped from 2.36us to 5.6ns at 64 agents and from 10.46us to 5.7ns at 256 agents, while `summarize_callable_targets("caller")` improved from 4.88us to 1.67us at 64 agents and from 21.38us to 6.72us at 256 agents.
- Verified with `./.cargo/verify.sh` and the updated benchmark suite.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
WalkthroughThe PR implements a permissions-first refactor across crates: Rule and Ruleset move to owned types; a new Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/llm-coding-tools-core/src/tools/glob.rs (1)
171-177:⚠️ Potential issue | 🔴 CriticalDenied directories are still traversed.
Lines 171-177 skip every directory before any ruleset evaluation, and Lines 203-210 only check the leaf file path. That means
deny("/repo/secret")never prevents the walk from entering/repo/secret; with a broaderallow("*"), descendant files can still pass the later check and be returned. Directory permissions need to be enforced before descent so denied subtrees are pruned, not just filtered after the walk.Also applies to: 203-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/tools/glob.rs` around lines 171 - 177, The code currently skips directories unconditionally via entry.file_type() / ft.is_dir(), which allows denied directories (e.g., deny("/repo/secret")) to be traversed and only filters leaves later; change the logic so that when ft.is_dir() you evaluate the ruleset/permission check for that directory path (the same check used for files) and if the ruleset denies the directory, skip/descent (continue) to prune the subtree, otherwise allow descent; apply the same fix to the later leaf-only block so directory entries are checked against deny/allow rules before walking into them (i.e., enforce deny rules on directories before descent, not just on leaf file acceptance).src/llm-coding-tools-core/src/tools/grep.rs (1)
357-372:⚠️ Potential issue | 🔴 CriticalDenied directories are still searchable.
Lines 357-372 only apply the ruleset after the walker has already reached a file entry. An exact deny on a nested directory path is therefore ignored, so
grepcan still descend into that subtree and return matches from files that are only covered by a broad allow rule. The directory subject needs to be checked before descent, with denied subtrees pruned from the walk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/tools/grep.rs` around lines 357 - 372, The permission check is happening after the walker already descends into entries; instead, use a pre-descent filter so denied directories are pruned. Modify the walk initiation to call filter_entry (or equivalent pre-visit hook) and inside it call settings.permission() and ruleset.is_allowed(grep_meta::NAME, subject) on the entry.path() (string lossied) so directories that return false are not descended into; keep the existing per-file permission check for files but remove relying on post-descent logic to prevent searching denied subtrees.
🧹 Nitpick comments (2)
src/llm-coding-tools-core/src/path/allowed_glob/policy.rs (1)
85-88: Add explicit regression tests for single-rule fast-path semantics.Lines 85-88 introduce a distinct execution path; please add direct coverage for a single
Denyrule (match + non-match cases) to lock behavior.✅ Suggested test additions
#[test] fn invalid_glob_pattern_fails() { let result = GlobPolicy::builder().allow("[invalid"); assert!(result.is_err()); } + +#[test] +fn single_deny_rule_should_deny_match_and_non_match() { + let policy = GlobPolicy::builder() + .deny("secret/**") + .unwrap() + .build() + .unwrap(); + + assert!(!policy.is_allowed("secret/file.txt")); + assert!(!policy.is_allowed("public/file.txt")); +} + +#[test] +fn single_allow_rule_should_allow_only_matches() { + let policy = GlobPolicy::builder() + .allow("src/**") + .unwrap() + .build() + .unwrap(); + + assert!(policy.is_allowed("src/lib.rs")); + assert!(!policy.is_allowed("tests/test.rs")); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/path/allowed_glob/policy.rs` around lines 85 - 88, Add regression tests that exercise the single-rule fast path by creating a Policy (or the type that holds self.rules) with exactly one rule whose action is RuleAction::Deny, then assert the evaluator that uses matcher.is_match(normalized_path) returns false when the glob matches and true when it does not; ensure both match and non-match cases are covered so the branch that checks if let [(matcher, action)] = self.rules.as_slice() behaves correctly for Deny rules and normalized_path handling.src/llm-coding-tools-agents/src/runtime/state.rs (1)
121-151: Consider exposing borrowed cache views as well.Both accessors still clone their cached
Vecon every lookup. Adding borrowed variants (&[ToolCatalogEntry]/&[TaskTargetSummary]) or storingArc<[T]>would keep this hot path allocation-free and preserve more of the win from precomputing the caches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-agents/src/runtime/state.rs` around lines 121 - 151, The current accessors allowed_tools and summarize_callable_targets clone Vecs from the caches (allowed_tools_by_caller, callable_target_summaries_by_caller) on every lookup; change the API to provide allocation-free borrowed views or shared refs: add new methods (e.g., allowed_tools_ref(caller_name: &str) -> &[ToolCatalogEntry] and summarize_callable_targets_ref(caller_name: &str) -> &[TaskTargetSummary]) that return slices into the cached Vecs, or alternatively change the cache value types to Arc<[T]> and add returning methods that return Arc<[T]> to avoid cloning; update callers to use the new borrowed/shared-view methods and keep the existing cloning methods (allowed_tools, summarize_callable_targets) for backwards compatibility if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm-coding-tools-agents/benches/runtime_task.rs`:
- Around line 97-99: The benchmark uses a non-existent agent ID for the
"can_delegate_miss" case, so update the test data to target an existing misc
agent (e.g., change "misc-003" to "misc-002") wherever "can_delegate_miss" is
configured inside bench_runtime_task_caches (and the duplicate case around the
other occurrence referenced in the comment), ensuring the fixture and the
benchmark target the same existing agent ID so the test measures a permission
miss rather than a lookup of a nonexistent agent.
In `@src/llm-coding-tools-core/benches/permissions.rs`:
- Around line 54-57: The function benchmark_cases() documentation is out of
sync: it claims wildcard cases exist only at sizes 1 and 32 but the
implementation also includes wildcard_both_128_rules (and similar 128-rule cases
around lines 96-100); update the comment above benchmark_cases() to enumerate
the actual rule-set sizes covered (1, 32, and 128) and mention the presence of
wildcard-both (and other wildcard) cases so the doc matches the implemented
PermissionCase variants.
In `@src/llm-coding-tools-core/src/tools/bash/mod.rs`:
- Around line 94-95: Update the documentation for BashSettings to include the
permission-denied error: in the `# Errors` section for the module/type that
documents BashSettings and the `permission: Option<&'a Ruleset>` field, add a
note that operations may return `ToolError::PermissionDenied` when a command
string is blocked by the provided ruleset; reference `BashSettings` and
`ToolError::PermissionDenied` so the API docs clearly show the new error case.
In `@src/llm-coding-tools-serdesai/src/convert.rs`:
- Around line 80-88: The mapping for CoreError::PermissionDenied is incorrect:
change the conversion to produce a runtime execution failure instead of a
validation error by mapping PermissionDenied to SerdesError::execution_failed
(instead of SerdesError::validation_error) using the same parameters (tool_name,
None, and the formatted "Permission denied for tool '{}' on subject '{}'"
message); update the conversion in the match arm that handles
CoreError::PermissionDenied so PermissionDenied follows the same
execution/runtime failure path as Io/Execution/Timeout errors.
---
Outside diff comments:
In `@src/llm-coding-tools-core/src/tools/glob.rs`:
- Around line 171-177: The code currently skips directories unconditionally via
entry.file_type() / ft.is_dir(), which allows denied directories (e.g.,
deny("/repo/secret")) to be traversed and only filters leaves later; change the
logic so that when ft.is_dir() you evaluate the ruleset/permission check for
that directory path (the same check used for files) and if the ruleset denies
the directory, skip/descent (continue) to prune the subtree, otherwise allow
descent; apply the same fix to the later leaf-only block so directory entries
are checked against deny/allow rules before walking into them (i.e., enforce
deny rules on directories before descent, not just on leaf file acceptance).
In `@src/llm-coding-tools-core/src/tools/grep.rs`:
- Around line 357-372: The permission check is happening after the walker
already descends into entries; instead, use a pre-descent filter so denied
directories are pruned. Modify the walk initiation to call filter_entry (or
equivalent pre-visit hook) and inside it call settings.permission() and
ruleset.is_allowed(grep_meta::NAME, subject) on the entry.path() (string
lossied) so directories that return false are not descended into; keep the
existing per-file permission check for files but remove relying on post-descent
logic to prevent searching denied subtrees.
---
Nitpick comments:
In `@src/llm-coding-tools-agents/src/runtime/state.rs`:
- Around line 121-151: The current accessors allowed_tools and
summarize_callable_targets clone Vecs from the caches (allowed_tools_by_caller,
callable_target_summaries_by_caller) on every lookup; change the API to provide
allocation-free borrowed views or shared refs: add new methods (e.g.,
allowed_tools_ref(caller_name: &str) -> &[ToolCatalogEntry] and
summarize_callable_targets_ref(caller_name: &str) -> &[TaskTargetSummary]) that
return slices into the cached Vecs, or alternatively change the cache value
types to Arc<[T]> and add returning methods that return Arc<[T]> to avoid
cloning; update callers to use the new borrowed/shared-view methods and keep the
existing cloning methods (allowed_tools, summarize_callable_targets) for
backwards compatibility if needed.
In `@src/llm-coding-tools-core/src/path/allowed_glob/policy.rs`:
- Around line 85-88: Add regression tests that exercise the single-rule fast
path by creating a Policy (or the type that holds self.rules) with exactly one
rule whose action is RuleAction::Deny, then assert the evaluator that uses
matcher.is_match(normalized_path) returns false when the glob matches and true
when it does not; ensure both match and non-match cases are covered so the
branch that checks if let [(matcher, action)] = self.rules.as_slice() behaves
correctly for Deny rules and normalized_path handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87471eed-c0b1-4998-82ba-e191facd7e8d
📒 Files selected for processing (31)
src/llm-coding-tools-agents/Cargo.tomlsrc/llm-coding-tools-agents/benches/runtime_task.rssrc/llm-coding-tools-agents/src/extensions.rssrc/llm-coding-tools-agents/src/runtime/builder.rssrc/llm-coding-tools-agents/src/runtime/state.rssrc/llm-coding-tools-agents/src/runtime/task.rssrc/llm-coding-tools-core/Cargo.tomlsrc/llm-coding-tools-core/benches/permissions.rssrc/llm-coding-tools-core/src/error.rssrc/llm-coding-tools-core/src/lib.rssrc/llm-coding-tools-core/src/path/allowed_glob/policy.rssrc/llm-coding-tools-core/src/permissions.rssrc/llm-coding-tools-core/src/permissions_ext.rssrc/llm-coding-tools-core/src/tools/bash/blocking_impl.rssrc/llm-coding-tools-core/src/tools/bash/mod.rssrc/llm-coding-tools-core/src/tools/bash/tokio_impl.rssrc/llm-coding-tools-core/src/tools/edit.rssrc/llm-coding-tools-core/src/tools/glob.rssrc/llm-coding-tools-core/src/tools/grep.rssrc/llm-coding-tools-core/src/tools/mod.rssrc/llm-coding-tools-core/src/tools/read.rssrc/llm-coding-tools-core/src/tools/write.rssrc/llm-coding-tools-serdesai/src/agent_runtime/build.rssrc/llm-coding-tools-serdesai/src/convert.rssrc/llm-coding-tools-serdesai/src/task/handle.rssrc/llm-coding-tools-serdesai/src/tools/bash.rssrc/llm-coding-tools-serdesai/src/tools/edit.rssrc/llm-coding-tools-serdesai/src/tools/glob.rssrc/llm-coding-tools-serdesai/src/tools/grep.rssrc/llm-coding-tools-serdesai/src/tools/read.rssrc/llm-coding-tools-serdesai/src/tools/write.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Blocking Linux
- GitHub Check: Blocking Windows
- GitHub Check: Async Linux
- GitHub Check: Async macOS
- GitHub Check: Blocking macOS
- GitHub Check: Semver Checks (Serdesai Full)
- GitHub Check: Async Windows
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-04-07T11:44:05.110Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-04-07T11:44:05.110Z
Learning: If relevant to your review task, read `.cargo/performance.md` for performance guidelines
Applied to files:
src/llm-coding-tools-agents/Cargo.tomlsrc/llm-coding-tools-core/Cargo.tomlsrc/llm-coding-tools-agents/benches/runtime_task.rs
📚 Learning: 2026-03-28T02:14:04.465Z
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:04.465Z
Learning: In `src/llm-coding-tools-bubblewrap/src/profile/` (Rust, llm-coding-tools-bubblewrap crate), the `Builder` API paths (workspace, synthetic_home, cache_root, mount lists, overlays, etc.) are always set by trusted application/operator code — the library consumer is the trusted party. Path normalization and `..`-component hardening in validators like `validate_absolute_path` is therefore NOT required to defend against traversal attacks. Untrusted input (LLM-generated shell commands) only enters through `wrap_command`/`execute_command_with_mode`, not through the `Builder`.
Applied to files:
src/llm-coding-tools-core/src/tools/bash/blocking_impl.rssrc/llm-coding-tools-core/src/tools/bash/tokio_impl.rssrc/llm-coding-tools-serdesai/src/tools/bash.rssrc/llm-coding-tools-serdesai/src/tools/read.rssrc/llm-coding-tools-serdesai/src/agent_runtime/build.rssrc/llm-coding-tools-serdesai/src/tools/glob.rssrc/llm-coding-tools-core/src/tools/read.rs
🔇 Additional comments (33)
src/llm-coding-tools-core/src/path/allowed_glob/policy.rs (1)
79-80:#[inline]on the hot path looks good.Line 79 is a reasonable optimization hint for this small, frequently-invoked permission check.
src/llm-coding-tools-core/src/error.rs (1)
58-65:PermissionDeniedvariant is a solid addition.Error shape and message formatting are consistent with the rest of
ToolErrorand fit the new permission-check flow cleanly.src/llm-coding-tools-core/Cargo.toml (1)
128-130: Benchmark target registration looks good.The
permissionsbench entry is configured consistently with existing Criterion bench targets (harness = false).src/llm-coding-tools-agents/Cargo.toml (1)
46-48:runtime_taskbenchmark target wiring is clean.Manifest update is consistent with existing bench configuration and supports the runtime-cache perf work.
src/llm-coding-tools-core/src/tools/bash/blocking_impl.rs (1)
47-50: Blocking-path permission enforcement is correctly integrated.The pre-execution
permission.check(...)and the new denial test provide good coverage for the new authorization behavior.Also applies to: 260-290
src/llm-coding-tools-core/src/tools/mod.rs (1)
23-23: Re-export updates are appropriate.Exposing
EditSettingsandWriteSettingshere keeps the public tools surface consistent with the settings-based APIs.Also applies to: 35-35
src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs (1)
105-108: Async-path permission gating looks correct.The check is enforced before execution, and the new test validates denial behavior end-to-end for tokio mode.
Also applies to: 302-333
src/llm-coding-tools-serdesai/src/task/handle.rs (2)
1-148: LGTM - Clean migration to cached delegation API.The refactoring correctly replaces on-the-fly
Rulesetconstruction with the cachedruntime.can_delegate_to()API. The test coverage is comprehensive, covering unknown targets, primary agent rejection, permission denied scenarios, and max depth enforcement.
134-145: The implicit-allow behavior differs from the oldRulesetpattern: clarify the design choice.The guard at line 134 (
caller.permission.contains_key(task_meta::NAME)) means that if the caller has notaskpermission entry at all, the delegation check is skipped entirely, allowing delegation by default.However, this is a new feature (all task-related files are newly added in this commit). If the code had followed the old pattern of calling
Ruleset::is_allowedwith empty rules, the result would have beenfalse(restrictive), not permissive. The current design intentionally uses a permissive default—no permission rules = implicit allow.Ensure this implicit-allow semantic is intentional and documented (in comments or design notes) rather than an unintended consequence of the permission check guard.
src/llm-coding-tools-serdesai/src/tools/grep.rs (2)
75-85: LGTM - Clean builder pattern for permission configuration.The
with_permissionmethod follows the established builder pattern used across other tools, properly delegates toGrepSettings::with_permission, and includes#[must_use]for safety.
100-111: LGTM - Correct reference-based settings passing.The change from moving
self.search_settingsto passing&self.search_settingsis correct and aligns with the core API change. This allows the tool to be called multiple times without consuming the settings.src/llm-coding-tools-serdesai/src/tools/bash.rs (3)
54-56: LGTM - Well-documented permission field.The permission field is properly typed as
Option<Arc<Ruleset>>with clear documentation. TheArcallows efficient sharing of the ruleset across tool instances without copying.
154-164: LGTM - Consistent builder pattern.The
with_permissionmethod matches the pattern used inGrepToolandGlobTool, with proper#[must_use]annotation and clear documentation.
216-226: LGTM - Correct permission passing to BashSettings.Using
self.permission.as_deref()correctly convertsOption<Arc<Ruleset>>toOption<&Ruleset>for theBashSettingsstruct, maintaining the reference without unnecessary cloning.src/llm-coding-tools-agents/src/runtime/builder.rs (1)
154-184: LGTM - Excellent cache identity verification test.The test correctly validates that:
- The same
Arcinstance is returned for repeated calls (Arc::ptr_eq)- The cached ruleset correctly evaluates permissions (
is_allowed)Using
Arc::ptr_eqis the right approach to verify caching rather than just value equality.src/llm-coding-tools-core/src/lib.rs (2)
18-18: LGTM - Public permission extension module.Exposing
permissions_extpublicly enables consumers to use theOptionRulesetExttrait for permission checking on optional rulesets.
37-40: LGTM - Consistent re-export expansion.Adding
EditSettingsandWriteSettingsto the public re-exports aligns with the existing pattern for other tool settings types (GrepSettings,GlobSettings, etc.).src/llm-coding-tools-agents/src/extensions.rs (1)
40-61: LGTM - Clean migration to owned Ruleset.The signature change from
Ruleset<'a>to ownedRulesetaligns with the PR's core change to useBox<str>for rule storage. The implementation correctly:
- Preserves
IndexMapiteration order for rule precedence- Uses
with_capacityfor allocation efficiency- Converts string references to owned storage via
Rule::newsrc/llm-coding-tools-serdesai/src/tools/glob.rs (2)
64-74: LGTM - Consistent permission builder pattern.The
with_permissionmethod follows the established pattern across tools, with proper#[must_use]annotation and delegation toGlobSettings::with_permission.
89-97: LGTM - Simplified error handling flow.The refactored
callimplementation is cleaner, with a straightforward flow:
- Parse request (early return on error)
- Execute glob_files (early return on error)
- Return formatted output
Removing the intermediate
resultvariable and explicit match improves readability.src/llm-coding-tools-serdesai/src/agent_runtime/build.rs (4)
85-87: LGTM - Well-documented permission caching field.The
permissionfield documentation clearly explains theNonecase for backward compatibility with agents that have no permissions configured.
128-131: Good optimization: filtering empty rulesets.The
.filter(|ruleset| !ruleset.is_empty())optimization avoids unnecessary permission checks when no rules are configured, correctly mapping empty rulesets toNone(permissive mode).
169-225: LGTM - Consistent permission wiring across tools.The permission is correctly cloned and passed to each tool that supports path access control:
ReadToolviawith_settings_and_permissionWriteTool,EditTool,GlobTool,GrepTool,BashToolvia.with_permission(permission.clone())The
Arc::clone()is efficient (reference count increment only), and each tool correctly shares the same underlyingRulesetinstance.
227-246: WebFetch and Todo tools intentionally skip permission rulesets.
WebFetchTool,TodoRead, andTodoWritedo not receive permission rulesets by design:
- WebFetchTool operates on URLs rather than filesystem paths and is initialized with
with_settings()only- TodoRead and TodoWrite are pre-built stateless tools created via
create_todo_tools()and cloned without permission filtering- TaskTool delegates validation through its own runtime model (
can_delegate_toin TaskHandle), checking delegation permissions independentlyThis design is consistent with each tool's operational scope.
src/llm-coding-tools-serdesai/src/tools/edit.rs (1)
41-43: Permission-aware settings flow is wired correctly.The constructor split (
new→with_settings) andwith_permissionbuilder cleanly propagate settings intoedit_file, which keeps policy evaluation centralized in core.Also applies to: 45-73, 90-90
src/llm-coding-tools-serdesai/src/tools/read.rs (1)
79-105: ReadTool permission injection path looks good.
with_settings_and_permissionplusread_file(&self.resolver, args, &self.settings)makes runtime-injected permissions explicit and keeps behavior consistent with the core read API.Also applies to: 125-126
src/llm-coding-tools-core/src/tools/edit.rs (1)
83-132: Core edit permission gating is correctly implemented.
EditSettingsis cleanly modeled, and the pre-I/Osettings.permission().check(...)plus denied-path test gives good coverage of the new access-control behavior.Also applies to: 150-155, 258-288
src/llm-coding-tools-serdesai/src/tools/write.rs (1)
41-43: WriteTool settings/permission plumbing is solid.The move to
WriteSettingspluswith_permissionkeeps the serdes layer aligned with core permission enforcement and avoids per-call policy reconstruction.Also applies to: 45-73, 91-92
src/llm-coding-tools-core/src/permissions_ext.rs (1)
30-35: Nice centralization of optional-ruleset checks.This trait removes repeated permission boilerplate at call sites, and the tests validate the intended
None => allow,Some => evaluatebehavior.Also applies to: 37-53, 61-93
src/llm-coding-tools-core/src/tools/write.rs (1)
27-76: Write permission enforcement is correctly placed and covered.Checking permissions immediately after path resolution (before directory creation and writes) is the right ordering, and the denied-path test exercises the new failure mode.
Also applies to: 87-92, 158-187
src/llm-coding-tools-agents/src/runtime/task.rs (1)
86-116: Nice one-pass cache construction.Building both per-caller maps from a single sorted agent snapshot and the prebuilt ruleset map keeps the cached views deterministic and avoids repeating permission resolution on the runtime lookup path.
src/llm-coding-tools-core/src/permissions.rs (1)
249-285: The fast path looks sound.The empty/single-rule cases plus the reverse scan preserve last-match-wins semantics while avoiding unnecessary subject hashing on the common miss paths.
Also applies to: 405-429
src/llm-coding-tools-core/src/tools/read.rs (1)
318-324: Verify the permission subject is canonicalized before open.Line 319 derives the permission subject from the resolved path string, and Line 324 opens that path afterward. If
PathResolver::resolveis lexical-only andopen_bufferedfollows symlinks, an allowed symlink or alias path can still reach a denied target. The same escape would affect the other path-scoped tools wired the same way in this PR.Run this to inspect the resolver and open helpers; you want to see canonicalization or explicit symlink rejection before the subject is authorized/opened:
#!/bin/bash set -euo pipefail rg -n -C4 --type=rust 'trait PathResolver|struct AbsolutePathResolver|impl .*PathResolver|fn resolve\(' src rg -n -C4 --type=rust 'fn open_buffered|canonicalize|symlink_metadata|read_link|O_NOFOLLOW|follow_links' src/llm-coding-tools-core/src
- Map PermissionDenied errors to execution_failed in serdesai - Document PermissionDenied in bash module docstrings - Fix typo in runtime_task benchmark comment - Update permissions benchmark documentation Ref: #91
The allowed_tools and summarize_callable_targets methods now return &[T] instead of cloning Vecs on every lookup. Callers needing ownership explicitly call .to_vec() or .clone(). This eliminates allocations for read-only access patterns used by tests and iteration.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/llm-coding-tools-core/benches/permissions.rs (1)
126-143: Consider adding denied-path benchmark coverage.
bench_check_permissioncurrently exercises only the allowed path (Line 139). Adding at least one denied fixture would better track regressions forPermissionDeniedhot-path behavior introduced in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/benches/permissions.rs` around lines 126 - 143, bench_check_permission only measures the allowed path via OptionRulesetExt::check; add a denied-path benchmark to cover PermissionDenied hot path by adding at least one denied fixture to the benchmark_cases data (or create a separate denied_case) and then add a bench_with_input invocation in bench_check_permission that uses BenchmarkId::new("denied_ruleset", case.name) and calls .check(...) expecting an Err (use a small assertion that it returns a PermissionDenied) so the denied code path is exercised; locate the fixtures in benchmark_cases and the benchmark function bench_check_permission to implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm-coding-tools-core/benches/permissions.rs`:
- Around line 28-38: The function build_ruleset currently claims rule_count
"must be at least 1" but coerces 0 into a one-rule fixture; enforce the
precondition instead: add an assertion like assert!(rule_count >= 1, "rule_count
must be >= 1") at the start of build_ruleset, remove the .max(1) use in
Ruleset::with_capacity and pass rule_count directly, and replace the
saturating_sub loop range (for idx in 0..rule_count.saturating_sub(1)) with an
explicit 0..(rule_count - 1) iteration so the code fails fast when rule_count is
0; use the function name build_ruleset and the Ruleset::with_capacity call to
locate the changes.
---
Nitpick comments:
In `@src/llm-coding-tools-core/benches/permissions.rs`:
- Around line 126-143: bench_check_permission only measures the allowed path via
OptionRulesetExt::check; add a denied-path benchmark to cover PermissionDenied
hot path by adding at least one denied fixture to the benchmark_cases data (or
create a separate denied_case) and then add a bench_with_input invocation in
bench_check_permission that uses BenchmarkId::new("denied_ruleset", case.name)
and calls .check(...) expecting an Err (use a small assertion that it returns a
PermissionDenied) so the denied code path is exercised; locate the fixtures in
benchmark_cases and the benchmark function bench_check_permission to implement
this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aca8d33a-dcb4-4b1d-aa2b-517f3234b764
📒 Files selected for processing (8)
src/llm-coding-tools-agents/benches/runtime_task.rssrc/llm-coding-tools-agents/src/runtime/state.rssrc/llm-coding-tools-core/benches/permissions.rssrc/llm-coding-tools-core/src/tools/bash/blocking_impl.rssrc/llm-coding-tools-core/src/tools/bash/mod.rssrc/llm-coding-tools-core/src/tools/bash/tokio_impl.rssrc/llm-coding-tools-serdesai/src/agent_runtime/build.rssrc/llm-coding-tools-serdesai/src/convert.rs
✅ Files skipped from review due to trivial changes (1)
- src/llm-coding-tools-agents/benches/runtime_task.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/llm-coding-tools-serdesai/src/convert.rs
- src/llm-coding-tools-core/src/tools/bash/mod.rs
- src/llm-coding-tools-agents/src/runtime/state.rs
- src/llm-coding-tools-core/src/tools/bash/blocking_impl.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Async macOS
- GitHub Check: Async Windows
- GitHub Check: Blocking Windows
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-28T02:14:04.465Z
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:04.465Z
Learning: In `src/llm-coding-tools-bubblewrap/src/profile/` (Rust, llm-coding-tools-bubblewrap crate), the `Builder` API paths (workspace, synthetic_home, cache_root, mount lists, overlays, etc.) are always set by trusted application/operator code — the library consumer is the trusted party. Path normalization and `..`-component hardening in validators like `validate_absolute_path` is therefore NOT required to defend against traversal attacks. Untrusted input (LLM-generated shell commands) only enters through `wrap_command`/`execute_command_with_mode`, not through the `Builder`.
Applied to files:
src/llm-coding-tools-core/src/tools/bash/tokio_impl.rssrc/llm-coding-tools-serdesai/src/agent_runtime/build.rs
🔇 Additional comments (10)
src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs (4)
9-10: LGTM!Imports are appropriate for the new permission check functionality.
96-109: LGTM!The permission check is correctly placed before workdir/timeout validation for fail-fast behavior. The
OptionRulesetExt::checkimplementation ensures backward compatibility by returningOk(())whenpermissionisNone. Documentation accurately reflects the new error variant.
303-334: LGTM!The test correctly validates the last-match-wins semantics by placing a deny rule after an allow-all rule, then asserting that the specific command is blocked. The assertion properly pattern-matches the
PermissionDeniedvariant with the expected tool name.
275-278: LGTM!Test imports and settings updates are consistent across all test functions. The
permission: Nonemaintains backward compatibility while satisfying the new struct field requirement.Also applies to: 289-294
src/llm-coding-tools-serdesai/src/agent_runtime/build.rs (6)
17-29: LGTM!The added imports for
RulesetandArcare necessary for the new permission caching functionality, and the import cleanup looks correct.
85-88: LGTM!The
Option<Arc<Ruleset>>type is appropriate for shared, cached permission data. UsingArcenables efficient cloning for multiple tools, andOptionhandles the backward-compatibility case for agents without permissions.
114-139: LGTM!The permission caching logic is well-designed:
- Using
runtime.permission_ruleset(name).filter(|rs| !rs.is_empty())correctly converts empty rulesets toNonefor backward compatibility.- The
.to_vec()calls are necessary since the runtime APIs now return slices.
162-175: LGTM!The permission caching reuse is efficient. Cloning
Option<Arc<Ruleset>>is O(1) (atomic reference count increment), and the call toReadTool::with_settings_and_permissioncorrectly matches the signature from the relevant code snippet.
177-218: LGTM!The permission wiring is applied consistently to all filesystem-related tools (Write, Edit, Glob, Grep, Bash). The pattern
.with_permission(permission.clone())is clean and efficient withArc. Correctly,WebFetchdoes not receive permission as it operates on URLs rather than filesystem paths.
230-239: LGTM!The Task tool now correctly uses the pre-computed
callable_target_summariesfromPreparedBuild, consistent with the caching pattern introduced in this PR. The.is_empty()guard appropriately prevents creating a Task tool when there are no callable targets.
…cing 0 Add assertion that rule_count >= 1, remove .max(1) on with_capacity, and replace saturating_sub with plain subtraction so the function fails fast on invalid input.
1 participant
Summary
Rule/Rulesettypes - Convert from&'a strtoBox<str>so rulesets can be cached and shared across the runtime lifetime.AgentRuntimeconstruction, eliminating repeated rule checks on every tool call.bash,edit,glob,grep,read, andwritetools so they can operate on paths outside the primary workspace.Benchmark Results
Permission evaluation (32-rule cases):
exact_32_ruleswildcard_subject_32_ruleswildcard_both_32_rulescheck_permissionimproved by 36-42% on the same hot paths.Runtime Task resolution (64 agents):
allowed_tools("caller")summarize_callable_targets("caller")