feat: directory-scoped shell command approval patterns#896
Open
Aaronontheweb wants to merge 9 commits intonetclaw-dev:devfrom
Open
feat: directory-scoped shell command approval patterns#896Aaronontheweb wants to merge 9 commits intonetclaw-dev:devfrom
Aaronontheweb wants to merge 9 commits intonetclaw-dev:devfrom
Conversation
Per-file approval patterns for path-aware verbs (ls, cat, grep, find, etc.) forced users to approve every individual file access separately — 11+ prompts in a single diagnostic session. Replace with directory-scoped patterns that cover all files under a parent directory when the user selects "Approve for this chat" or "Approve always". Key changes: - ShellTokenizer.ExtractDirectoryScope() scans all args for file paths (not just first positional), extracts parent directory with minimum depth of 2 segments to prevent overly broad scopes like / or /etc/ - ApprovalPatternMatching gains directory-prefix matching using PathUtility.IsWithinRoot() for boundary-safe containment - IToolApprovalMatcher extended with ExtractDirectoryPatterns(); implemented on ShellApprovalMatcher (with compound command + bash -c recursion), DefaultApprovalMatcher, and FilePathApprovalMatcher - DirectoryPatterns wired through ToolInteractionRequest → PendingToolInteraction → RecordApprovalAsync so B/C decisions store directory patterns - Approval option labels dynamically show directory scope (e.g., "Approve in /home/.netclaw/logs/ for this chat") Security: only relaxes the interactive approval gate. Hard deny list, ToolPathPolicy (protected paths), symlink resolution, and path traversal prevention layers remain unaffected.
…l code Add 8 new tests covering directory-scoped approval patterns: 3 in ToolApprovalGateTests (DirectoryPatterns population, directory-scoped labels, default labels for non-path commands) and 5 in ShellApprovalMatcherTests (ExtractDirectoryPatterns for simple paths, compound commands, verb-chain fallback, empty commands, mixed compounds). Fix label bug where verb-chain fallbacks like "git push" triggered directory-scope label formatting, producing nonsense labels like "Approve in push for this chat". Now checks for trailing '/' to identify actual directory-scoped patterns before customizing labels. Simplify code: unify CollectPatterns/CollectDirectoryPatterns into shared TraverseSegments helper with Func<string, string?> leaf extractor; narrow bare catch to ArgumentException|IOException; use PathUtility.ExpandAndNormalize instead of separate ExpandHome + TryNormalize calls; make DirectoryPatterns non-nullable on ToolApprovalContext. Include OpenSpec change artifacts (proposal, design, delta spec, tasks).
Aaronontheweb
added
security
Aaronontheweb
commented
May 6, 2026
Collaborator
Author
Aaronontheweb
left a comment
Aaronontheweb
left a comment
There was a problem hiding this comment.
Found some issues with the tests that make me think the rest of the code might need some belt and suspenders tuning
| new ToolName("shell_execute"), | ||
| Args("git push origin main")); | ||
| Assert.Single(patterns); | ||
| Assert.Equal("git push", patterns[0]); |
Collaborator
Author
There was a problem hiding this comment.
No directories to extract
| Args("cat /home/user/.netclaw/logs/crash.log && grep 'error' /home/user/.netclaw/logs/app.log")); | ||
| Assert.Equal(2, patterns.Count); | ||
| Assert.Contains(patterns, p => p.StartsWith("cat ")); | ||
| Assert.Contains(patterns, p => p.StartsWith("grep ")); |
Collaborator
Author
There was a problem hiding this comment.
This test needs to be hardened to include the actual directory we're supposed to use for both of these commands. BTW that's not normally how someone would write a command - it'd be piped usually.
Collaborator
Author
|
Goal of this PR is to prevent LLM fatigue, having to constantly approve new tool calls all the time, without obliterating our security perimeter around shell tool calls too. |
Collaborator
Author
|
Tracked Windows-native shell support follow-up in #899. |
Use resolved path operands for exact and directory approvals so grep, relative paths, and existing directory targets approve the intended scope. Keep prompt labels, DTO transport, and OpenSpec guidance aligned with the actual approval patterns users are granting.
Keep exact path-aware approval patterns for direct path operands while limiting directory-scoped grants to a small read/list allowlist. Fall back to exact patterns and generic prompts for non-allowlisted commands and redirected shell segments so broader approvals stay predictable and easier to reason about.
Preserve all directly identifiable path operands in exact shell approval patterns and keep those patterns exact-only instead of prefix-expanding to additional operands. Limit directory-scoped grants to single-path allowlisted reads while falling back to exact patterns and generic prompts for multi-path and redirected command shapes.
This reverts commit 1d3b202.
This reverts commit d40180f.
This reverts commit 54f77b8.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cat,grep,ls,find, etc.). When a user selects "Approve for this chat" or "Approve always", the system stores a directory-scoped pattern (e.g.,grep /home/.netclaw/logs/) instead of a per-file pattern. This dramatically reduces approval fatigue — in sessionD0AC6CKBK5K/1778085593.830269, 11 separate approvals would now be covered by ~2 directory-scoped approvals."Approve in /home/.netclaw/logs/ for this chat"instead of generic"Approve for this chat".ToolPathPolicyprotected paths, and symlink resolution all run after the approval gate. Minimum depth enforcement (2 segments) prevents overly broad scopes like/,/etc/,/home/.Changes
ShellTokenizer.csExtractDirectoryScope()— scans all args for path tokens, extracts parent directory with min-depth enforcementApprovalPatternMatching.csMatchesDirectoryScope()— trailing/triggersPathUtility.IsWithinRoot()containment check, verb-isolatedIToolApprovalMatcher.csExtractDirectoryPatterns()on interface +ShellApprovalMatcherimplementation withTraverseSegmentshelperSessionOutput.csDirectoryPatternsfield onToolInteractionRequestToolAccessPolicy.csDirectoryPatternsonToolApprovalContextLlmSessionActor.csSessionToolExecutionPipeline.cs,DispatchingToolExecutor.csDirectoryPatternsthrough approval flowBackward compatibility
tool-approvals.json) unchanged — directory patterns are strings with trailing/in the same listsTest plan
ShellTokenizerTests—ExtractDirectoryScopefor path commands, grep (path in 2nd arg), globs, shallow paths, non-path commandsShellApprovalMatcherTests—ExtractDirectoryPatternsfor simple paths, compound commands, verb-chain fallback, empty, mixed compoundApprovalPatternMatching— directory-prefix matching via[InlineData]theories (same dir, nested, sibling, verb mismatch, boundary safety)ToolApprovalGateTests— DirectoryPatterns population, directory-scoped labels, default labels for non-path commandsdotnet slopwatch analyze— 0 violations