feat(execpolicy): add typed ask rule foundation#2401
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces typed ToolAskRules to the execution policy engine, allowing specific tool invocations to require approval and rejecting them when the approval policy is set to Never. Feedback on these changes highlights three key areas for improvement: first, matching_ask_rule should respect RulesetLayer priority when selecting the matching rule; second, normalize_path_value should call .trim() before .trim_matches('/') to ensure correct path normalization when spaces are present; and third, the core runtime currently hardcodes the context's path to None, which prevents path-based rules from matching.
| self.rulesets | ||
| .iter() | ||
| .flat_map(|ruleset| ruleset.ask_rules.iter()) | ||
| .filter(|rule| rule.tool == tool) | ||
| .filter(|rule| match rule.command.as_deref() { | ||
| Some(command) => self.arity_dict.allow_rule_matches(command, ctx.command), | ||
| None => true, | ||
| }) | ||
| .filter(|rule| match (rule.path.as_deref(), ctx.path) { | ||
| (Some(pattern), Some(path)) => { | ||
| normalize_path_value(pattern) == normalize_path_value(path) | ||
| } | ||
| (Some(_), None) => false, | ||
| (None, _) => true, | ||
| }) | ||
| .max_by_key(|rule| ask_rule_specificity(rule)) | ||
| .cloned() |
There was a problem hiding this comment.
The current implementation of matching_ask_rule flattens all rulesets across all layers and simply finds the one with the maximum specificity. However, this ignores the RulesetLayer priority contract (where User priority should override Agent and BuiltinDefault priorities).
To preserve the layer priority hierarchy, the layer priority should be the primary sorting key, and the rule specificity should be the secondary key.
self.rulesets
.iter()
.flat_map(|ruleset| {
ruleset
.ask_rules
.iter()
.map(move |rule| (ruleset.layer, rule))
})
.filter(|(_, rule)| rule.tool == tool)
.filter(|(_, rule)| match rule.command.as_deref() {
Some(command) => self.arity_dict.allow_rule_matches(command, ctx.command),
None => true,
})
.filter(|(_, rule)| match (rule.path.as_deref(), ctx.path) {
(Some(pattern), Some(path)) => {
normalize_path_value(pattern) == normalize_path_value(path)
}
(Some(_), None) => false,
(None, _) => true,
})
.max_by_key(|&(layer, rule)| (layer, ask_rule_specificity(rule)))
.map(|(_, rule)| rule)
.cloned()| value | ||
| .replace('\\', "/") | ||
| .trim_matches('/') | ||
| .trim() | ||
| .to_ascii_lowercase() |
There was a problem hiding this comment.
In normalize_path_value, .trim_matches('/') is called before .trim(). If the input path has leading or trailing spaces (e.g., " /foo/bar "), .trim_matches('/') will fail to strip the slashes because the outermost characters are spaces. Calling .trim() first ensures that leading/trailing spaces are removed before stripping the slashes, resulting in consistent normalization.
| value | |
| .replace('\\', "/") | |
| .trim_matches('/') | |
| .trim() | |
| .to_ascii_lowercase() | |
| value | |
| .replace('\\', '/') | |
| .trim() | |
| .trim_matches('/') | |
| .to_ascii_lowercase() |
| command: &command, | ||
| cwd: &policy_cwd, | ||
| tool: Some(policy_tool), | ||
| path: None, |
There was a problem hiding this comment.
Currently, path is hardcoded to None when constructing ExecPolicyContext. This prevents any ToolAskRule with a path pattern (e.g., created via ToolAskRule::file_path) from matching, as the path comparison will always fail. Consider extracting the target file path from the ToolCall payload (for tools like read_file, write_file, etc.) and passing it here.
|
Thanks @greyfreedom — this is the right smaller slice from #2242. I found two small Greptile nits worth fixing before merge, and the Windows lane also tripped on unrelated-looking background shell timing tests. I tried to push the two-line fix + regression tests back here, but the fork is returning 404 to the maintainer token even though the PR is marked maintainer-editable. To keep the harvest moving without losing your work, I opened same-repo rescue PR #2404 from this branch plus those two fixes, rebased onto current |
Thanks for the update, and thanks for opening the rescue PR. I’m happy for #2404 to supersede this PR if that is the cleanest path to merge. I’ll stop pushing to this branch unless you need anything else from me. Appreciate the credit and the extra fixes. |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
Merged the same typed-ask foundation through #2404 after carrying this branch forward, fixing the two Greptile nits, and getting the full CI matrix green. Thank you again @greyfreedom — #2401 stays the reference PR for the original contribution. |
Summary
ToolAskRulerecords to execpolicy rulesets.AskForApproval::Never, because that mode cannot prompt the user.docs/TOOL_SURFACE.md.Motivation
This is the smallest foundation slice for persistent typed permission rules. It introduces explainable typed ask records without changing current allow/deny semantics or mid-session approval behavior.
Context
This PR is a smaller foundation slice extracted from the earlier larger permission-rules PR: #.
The previous PR covered typed allow/deny/ask rules, approval-flow integration, and TUI persistence. Per maintainer feedback, this PR intentionally narrows the scope to:
askrecords inexecpolicyAskForApproval::Neverhandling for matching ask rulesFollow-up work can build on this foundation in smaller PRs.
Related discussion:
Validation
cargo fmt --allcargo test -p codewhale-execpolicy --all-featurescargo test -p codewhale-core --all-featurescargo check -p codewhale-cli --all-featurescargo clippy -p codewhale-execpolicy -p codewhale-core --all-targets --all-features -- -D warningsGreptile Summary
This PR adds a typed
ToolAskRulerecord toexecpolicyrulesets as a narrow foundation for persistent permission rules. UnderAskForApproval::Never, any matching ask rule causes the command to be rejected with aForbiddendecision; all other approval modes are unaffected.ToolAskRulestruct inRulesetwithtooland optionalcommandfields, serialized to/from JSON with serde.matching_ask_ruleuses layer-then-specificity ordering viamax_by_keyto pick the most authoritative matching rule.AskForApproval::Neverenforcement: matching ask rules now produceForbiddeninstead ofSkip, withmatched_ruleinExecPolicyDecisionreporting the ask rule label rather than any coincidentally-matching trusted prefix.ExecPolicyContext.toolfield added to both the CLI and core call sites, mappingLocalShellpayloads to"exec_shell"and other payloads to their call name, enabling tool-aware rule matching.Confidence Score: 5/5
Safe to merge. The ask rule enforcement is narrowly scoped to AskForApproval::Never and leaves all other approval modes unchanged.
The matching_ask_rule logic is straightforward — layer-then-specificity ordering via max_by_key — and five new targeted tests cover the critical combinations: forbid-on-match, ignore-outside-never, layer-over-specificity, trusted+ask-rule overlap, and allow/deny precedence preservation. The matched_rule assignment correctly prioritises the ask rule label over any coincidentally-matching trusted prefix under Never mode. No existing behavior changes for other approval modes.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["check(ExecPolicyContext)"] --> B{Denied prefix match?} B -- Yes --> C["Forbidden: 'Command blocked by denied prefix'"] B -- No --> D["matching_ask_rule(ctx)\n(layer × specificity ordering)"] D --> E{AskForApproval mode?} E -- Never + ask_rule Some --> F["Forbidden: 'Typed ask rule requires approval...'"] E -- Never + ask_rule None --> G["Skip (allow)"] E -- UnlessTrusted + trusted --> G E -- OnFailure --> G E -- Reject rules=true --> H["Forbidden: 'Policy rejects rule-exceptions'"] E -- Other --> I["NeedsApproval"] F --> J["matched_rule = ask_rule.label()"] G --> K["matched_rule = trusted_rule"] I --> K H --> L["matched_rule = trusted_rule"]Reviews (2): Last reviewed commit: "fix(execpolicy): tighten typed ask rule ..." | Re-trigger Greptile