feat(execpolicy): add typed ask rule foundation#2404
Conversation
There was a problem hiding this comment.
Hmbown has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Code Review
This pull request introduces a typed ask-rule framework (ToolAskRule) to the execution policy engine, allowing specific tool invocations to require approval and rejecting them when the approval policy is set to Never. While the foundation is solid, two key issues were identified in the review: first, the path field in ExecPolicyContext is hardcoded to None during tool execution, which prevents path-specific rules from matching in production; second, the rule-matching logic flattens rulesets and ignores the priority layer of the ruleset (e.g., User vs. Agent), meaning layer precedence is not respected when determining the matching rule.
| let policy_tool = match &call.payload { | ||
| ToolPayload::LocalShell { .. } => "exec_shell", | ||
| _ => call.name.as_str(), | ||
| }; | ||
| let decision = self.exec_policy.check(ExecPolicyContext { | ||
| command: &command, | ||
| cwd: &policy_cwd, | ||
| tool: Some(policy_tool), | ||
| path: None, | ||
| ask_for_approval: approval_mode, | ||
| sandbox_mode: None, | ||
| })?; |
There was a problem hiding this comment.
The path field in ExecPolicyContext is hardcoded to None. This prevents any path-specific ToolAskRule (e.g., created via ToolAskRule::file_path) from matching during actual tool execution, rendering path-based policy rules non-functional in production.\n\nConsider extracting the target path from call.payload (for example, by implementing a helper method on ToolPayload that returns the path for file-related operations) and passing it here.
| let policy_tool = match &call.payload { | |
| ToolPayload::LocalShell { .. } => "exec_shell", | |
| _ => call.name.as_str(), | |
| }; | |
| let decision = self.exec_policy.check(ExecPolicyContext { | |
| command: &command, | |
| cwd: &policy_cwd, | |
| tool: Some(policy_tool), | |
| path: None, | |
| ask_for_approval: approval_mode, | |
| sandbox_mode: None, | |
| })?; | |
| let policy_tool = match &call.payload { | |
| ToolPayload::LocalShell { .. } => "exec_shell", | |
| _ => call.name.as_str(), | |
| }; | |
| let policy_path = call.payload.path(); | |
| let decision = self.exec_policy.check(ExecPolicyContext { | |
| command: &command, | |
| cwd: &policy_cwd, | |
| tool: Some(policy_tool), | |
| path: policy_path, | |
| ask_for_approval: approval_mode, | |
| sandbox_mode: None, | |
| })?; |
| 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 and selects the matching rule with the highest specificity, completely ignoring the priority layer of the ruleset (e.g., User vs Agent vs BuiltinDefault). In a layered policy engine, rules defined in higher-priority layers should take precedence over lower-priority layers, regardless of specificity.\n\nTo fix this, include the ruleset's layer in the max comparison key so that layer priority is respected first, and specificity is used as a tie-breaker within the same layer.
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()
Summary
Harvests @greyfreedom's smaller typed-ask foundation from #2401, rebased onto current
main, with the two Greptile nits fixed before merge:ToolAskRulerecords to execpolicy rulesetsAskForApproval::Never, because that mode cannot prompt the userdocs/TOOL_SURFACE.mdCredit: original implementation by @greyfreedom in #2401, extracted from the broader #2242 permission-rules work.
Related: #1186, #2242, #2401
Validation
cargo fmt --all -- --checkgit diff --checkCARGO_TARGET_DIR=/Volumes/VIXinSSD/codewhale-pr2401-target cargo test -p codewhale-execpolicy --all-features --lockedCARGO_TARGET_DIR=/Volumes/VIXinSSD/codewhale-pr2401-target cargo check -p codewhale-core -p codewhale-cli --all-features --locked