From 755331425e1970ed9273e43891620b2aa87b5d45 Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Tue, 11 Nov 2025 19:22:31 -0500 Subject: [PATCH] refactor: implementing rule traits, rulematch enum --- codex-rs/execpolicy2/README.md | 12 ++-- codex-rs/execpolicy2/src/default.policy | 11 --- codex-rs/execpolicy2/src/error.rs | 8 +-- codex-rs/execpolicy2/src/parser.rs | 28 ++------ codex-rs/execpolicy2/src/policy.rs | 4 +- codex-rs/execpolicy2/src/rule.rs | 93 +++++++++++++++++++++---- codex-rs/execpolicy2/tests/basic.rs | 32 ++++----- 7 files changed, 111 insertions(+), 77 deletions(-) diff --git a/codex-rs/execpolicy2/README.md b/codex-rs/execpolicy2/README.md index d65856547a..148cea9825 100644 --- a/codex-rs/execpolicy2/README.md +++ b/codex-rs/execpolicy2/README.md @@ -1,17 +1,16 @@ # codex-execpolicy2 ## Overview -- Policy engine and CLI built around `prefix_rule(pattern=[...], decision?, match?, not_match?, id?)`. +- Policy engine and CLI built around `prefix_rule(pattern=[...], decision?, match?, not_match?)`. - This release covers only the prefix-rule subset of the planned execpolicy v2 language; a richer language will follow. - Tokens are matched in order; any `pattern` element may be a list to denote alternatives. `decision` defaults to `allow`; valid values: `allow`, `prompt`, `forbidden`. -- `match` / `not_match` supply example invocations that are validated at load time (think of them as unit tests). `id` is optional; auto-generated if omitted. +- `match` / `not_match` supply example invocations that are validated at load time (think of them as unit tests). - The CLI always prints the JSON serialization of the evaluation result (whether a match or not). ## Policy shapes - Prefix rules use Starlark syntax: ```starlark prefix_rule( - id = "rule_id", # optional; autogenerated if omitted pattern = ["cmd", ["alt1", "alt2"]], # ordered tokens; list entries denote alternatives decision = "prompt", # allow | prompt | forbidden; defaults to allow match = [["cmd", "alt1"]], # examples that must match this rule @@ -27,9 +26,10 @@ prefix_rule( "decision": "allow|prompt|forbidden", "matched_rules": [ { - "rule_id": "", - "matched_prefix": ["", "..."], - "decision": "allow|prompt|forbidden" + "PrefixRuleMatch": { + "matched_prefix": ["", "..."], + "decision": "allow|prompt|forbidden" + } } ] } diff --git a/codex-rs/execpolicy2/src/default.policy b/codex-rs/execpolicy2/src/default.policy index 7799ad7160..85ab123fba 100644 --- a/codex-rs/execpolicy2/src/default.policy +++ b/codex-rs/execpolicy2/src/default.policy @@ -1,5 +1,4 @@ prefix_rule( - id = "git_status", pattern = ["git", "status"], match = [ ["git", "status"], @@ -12,7 +11,6 @@ prefix_rule( ) prefix_rule( - id = "git_reset_hard", pattern = ["git", "reset", "--hard"], decision = "forbidden", match = [ @@ -21,7 +19,6 @@ prefix_rule( ) prefix_rule( - id = "ls", pattern = ["ls"], match = [ ["ls"], @@ -34,7 +31,6 @@ prefix_rule( ) prefix_rule( - id = "cat", pattern = ["cat"], match = [ ["cat", "file.txt"], @@ -46,7 +42,6 @@ prefix_rule( ) prefix_rule( - id = "cp_prompt", pattern = ["cp"], decision = "prompt", match = [ @@ -59,7 +54,6 @@ prefix_rule( ) prefix_rule( - id = "head", pattern = ["head"], match = [ ["head", "README.md"], @@ -71,7 +65,6 @@ prefix_rule( ) prefix_rule( - id = "printenv", pattern = ["printenv"], match = [ ["printenv"], @@ -83,7 +76,6 @@ prefix_rule( ) prefix_rule( - id = "pwd", pattern = ["pwd"], match = [ ["pwd"], @@ -94,7 +86,6 @@ prefix_rule( ) prefix_rule( - id = "rg", pattern = ["rg"], match = [ ["rg", "-n", "init"], @@ -106,7 +97,6 @@ prefix_rule( ) prefix_rule( - id = "sed_prompt", pattern = ["sed"], decision = "prompt", match = [ @@ -119,7 +109,6 @@ prefix_rule( ) prefix_rule( - id = "which", pattern = ["which"], match = [ ["which", "python3"], diff --git a/codex-rs/execpolicy2/src/error.rs b/codex-rs/execpolicy2/src/error.rs index 0ef2fe1856..8311418cba 100644 --- a/codex-rs/execpolicy2/src/error.rs +++ b/codex-rs/execpolicy2/src/error.rs @@ -10,10 +10,10 @@ pub enum Error { InvalidPattern(String), #[error("invalid example: {0}")] InvalidExample(String), - #[error("expected example to match rule `{rule_id}`: {example}")] - ExampleDidNotMatch { rule_id: String, example: String }, - #[error("expected example to not match rule `{rule_id}`: {example}")] - ExampleDidMatch { rule_id: String, example: String }, + #[error("expected example to match rule `{rule}`: {example}")] + ExampleDidNotMatch { rule: String, example: String }, + #[error("expected example to not match rule `{rule}`: {example}")] + ExampleDidMatch { rule: String, example: String }, #[error("starlark error: {0}")] Starlark(String), } diff --git a/codex-rs/execpolicy2/src/parser.rs b/codex-rs/execpolicy2/src/parser.rs index 3a05c39cf6..27caffb3f2 100644 --- a/codex-rs/execpolicy2/src/parser.rs +++ b/codex-rs/execpolicy2/src/parser.rs @@ -11,14 +11,13 @@ use starlark::values::Value; use starlark::values::list::ListRef; use starlark::values::list::UnpackList; use starlark::values::none::NoneType; -use std::sync::atomic::AtomicU64; -use std::sync::atomic::Ordering; use crate::decision::Decision; use crate::error::Error; use crate::error::Result; use crate::rule::PatternToken; use crate::rule::PrefixPattern; +use crate::rule::PrefixRule; use crate::rule::Rule; pub struct PolicyParser { @@ -56,26 +55,19 @@ impl PolicyParser { #[derive(Debug, ProvidesStaticType)] struct PolicyBuilder { rules_by_program: Mutex>, - next_auto_id: AtomicU64, } impl PolicyBuilder { fn new() -> Self { Self { rules_by_program: Mutex::new(MultiMap::new()), - next_auto_id: AtomicU64::new(0), } } - fn alloc_id(&self) -> String { - let id = self.next_auto_id.fetch_add(1, Ordering::Relaxed); - format!("rule_{id}") - } - fn add_rule(&self, rule: Rule) { self.rules_by_program .lock() - .insert(rule.pattern.first.clone(), rule); + .insert(rule.program().to_string(), rule); } fn build(&self) -> crate::policy::Policy { @@ -177,7 +169,6 @@ fn policy_builtins(builder: &mut GlobalsBuilder) { decision: Option<&'v str>, r#match: Option>>, not_match: Option>>, - id: Option<&'v str>, eval: &mut Evaluator<'v, '_, '_>, ) -> anyhow::Result { let decision = match decision { @@ -187,33 +178,28 @@ fn policy_builtins(builder: &mut GlobalsBuilder) { let pattern_tokens = parse_pattern(pattern)?; - let positive_examples: Vec> = + let matches: Vec> = r#match.map(parse_examples).transpose()?.unwrap_or_default(); - let negative_examples: Vec> = not_match + let not_matches: Vec> = not_match .map(parse_examples) .transpose()? .unwrap_or_default(); let builder = policy_builder(eval); - let id = id - .map(std::string::ToString::to_string) - .unwrap_or_else(|| builder.alloc_id()); - let (first_token, remaining_tokens) = pattern_tokens .split_first() .ok_or_else(|| Error::InvalidPattern("pattern cannot be empty".to_string()))?; for head in first_token.alternatives() { - let rule = Rule { - id: id.clone(), + let rule = Rule::Prefix(PrefixRule { pattern: PrefixPattern { first: head, rest: remaining_tokens.to_vec(), }, decision, - }; - rule.validate_examples(&positive_examples, &negative_examples)?; + }); + rule.validate_examples(&matches, ¬_matches)?; builder.add_rule(rule); } Ok(NoneType) diff --git a/codex-rs/execpolicy2/src/policy.rs b/codex-rs/execpolicy2/src/policy.rs index ce86e8718b..85faff02b4 100644 --- a/codex-rs/execpolicy2/src/policy.rs +++ b/codex-rs/execpolicy2/src/policy.rs @@ -42,8 +42,8 @@ impl Policy { for rule in rules { if let Some(matched) = rule.matches(cmd) { let decision = match strictest_decision { - None => matched.decision, - Some(current) => std::cmp::max(matched.decision, current), + None => matched.decision(), + Some(current) => std::cmp::max(matched.decision(), current), }; strictest_decision = Some(decision); matched_rules.push(matched); diff --git a/codex-rs/execpolicy2/src/rule.rs b/codex-rs/execpolicy2/src/rule.rs index 7cd3f04a69..eda25beda8 100644 --- a/codex-rs/execpolicy2/src/rule.rs +++ b/codex-rs/execpolicy2/src/rule.rs @@ -61,25 +61,61 @@ impl PrefixPattern { } #[derive(Clone, Debug)] -pub struct Rule { - pub id: String, +pub enum Rule { + Prefix(PrefixRule), +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct PrefixRule { pub pattern: PrefixPattern, pub decision: Decision, } #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] -pub struct RuleMatch { - pub rule_id: String, - pub matched_prefix: Vec, - pub decision: Decision, +pub enum RuleMatch { + PrefixRuleMatch { + matched_prefix: Vec, + decision: Decision, + }, +} + +impl RuleMatch { + pub fn decision(&self) -> Decision { + match self { + Self::PrefixRuleMatch { decision, .. } => *decision, + } + } } impl Rule { + pub fn program(&self) -> &str { + match self { + Self::Prefix(rule) => &rule.pattern.first, + } + } + + pub fn matches(&self, cmd: &[String]) -> Option { + match self { + Self::Prefix(rule) => rule.matches(cmd), + } + } + + pub fn validate_examples( + &self, + matches: &[Vec], + not_matches: &[Vec], + ) -> Result<()> { + match self { + Self::Prefix(rule) => rule.validate_examples(matches, not_matches), + } + } +} + +impl PrefixRule { pub fn matches(&self, cmd: &[String]) -> Option { self.pattern .matches_prefix(cmd) - .map(|matched_prefix| RuleMatch { - rule_id: self.id.clone(), + .map(|matched_prefix| RuleMatch::PrefixRuleMatch { matched_prefix, decision: self.decision, }) @@ -87,30 +123,59 @@ impl Rule { pub fn validate_examples( &self, - positive: &[Vec], - negative: &[Vec], + matches: &[Vec], + not_matches: &[Vec], ) -> Result<()> { - for example in positive { + for example in matches { if self.matches(example).is_none() { return Err(Error::ExampleDidNotMatch { - rule_id: self.id.clone(), + rule: self.description(), example: join_command(example), }); } } - for example in negative { + for example in not_matches { if self.matches(example).is_some() { return Err(Error::ExampleDidMatch { - rule_id: self.id.clone(), + rule: self.description(), example: join_command(example), }); } } Ok(()) } + + pub fn description(&self) -> String { + format!( + "prefix_rule(pattern = [{}], decision = {})", + render_pattern(&self.pattern), + render_decision(self.decision) + ) + } } fn join_command(command: &[String]) -> String { try_join(command.iter().map(String::as_str)) .unwrap_or_else(|_| "unable to render example".to_string()) } + +fn render_pattern(pattern: &PrefixPattern) -> String { + let mut tokens = vec![pattern.first.clone()]; + tokens.extend(pattern.rest.iter().map(render_pattern_token)); + tokens.join(", ") +} + +fn render_pattern_token(token: &PatternToken) -> String { + match token { + PatternToken::Single(value) => value.clone(), + PatternToken::Alts(values) => format!("[{}]", values.join(", ")), + } +} + +fn render_decision(decision: Decision) -> &'static str { + match decision { + Decision::Allow => "allow", + Decision::Prompt => "prompt", + Decision::Forbidden => "forbidden", + } +} diff --git a/codex-rs/execpolicy2/tests/basic.rs b/codex-rs/execpolicy2/tests/basic.rs index 962c108c32..8ac5586d11 100644 --- a/codex-rs/execpolicy2/tests/basic.rs +++ b/codex-rs/execpolicy2/tests/basic.rs @@ -3,6 +3,7 @@ use codex_execpolicy2::Evaluation; use codex_execpolicy2::PolicyParser; use codex_execpolicy2::RuleMatch; use codex_execpolicy2::rule::PatternToken; +use codex_execpolicy2::rule::Rule; fn tokens(cmd: &[&str]) -> Vec { cmd.iter().map(std::string::ToString::to_string).collect() @@ -12,7 +13,6 @@ fn tokens(cmd: &[&str]) -> Vec { fn basic_match() { let policy_src = r#" prefix_rule( - id = "git_status", pattern = ["git", "status"], ) "#; @@ -30,8 +30,7 @@ prefix_rule( assert_eq!(decision, Decision::Allow); assert_eq!( matched_rules, - vec![RuleMatch { - rule_id: "git_status".to_string(), + vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git", "status"]), decision: Decision::Allow, }] @@ -42,7 +41,6 @@ prefix_rule( fn only_first_token_alias_expands_to_multiple_rules() { let policy_src = r#" prefix_rule( - id = "shell", pattern = [["bash", "sh"], ["-c", "-l"]], ) "#; @@ -64,7 +62,10 @@ prefix_rule( let Evaluation::Match { matched_rules, .. } = policy.evaluate(&cmd) else { panic!("expected match"); }; - assert_eq!(matched_rules[0].matched_prefix, prefix); + let matched_prefix = match &matched_rules[0] { + RuleMatch::PrefixRuleMatch { matched_prefix, .. } => matched_prefix, + }; + assert_eq!(matched_prefix, &prefix); } } @@ -72,7 +73,6 @@ prefix_rule( fn tail_aliases_are_not_cartesian_expanded() { let policy_src = r#" prefix_rule( - id = "npm_install_variants", pattern = ["npm", ["i", "install"], ["--legacy-peer-deps", "--no-save"]], ) "#; @@ -81,7 +81,9 @@ prefix_rule( let rules = policy.rules().get_vec("npm").expect("npm rules"); assert_eq!(rules.len(), 1); - let rule = &rules[0]; + let rule = match &rules[0] { + Rule::Prefix(rule) => rule, + }; assert_eq!( rule.pattern.rest, vec![ @@ -102,7 +104,6 @@ prefix_rule( fn match_and_not_match_examples_are_enforced() { let policy_src = r#" prefix_rule( - id = "git_status", pattern = ["git", "status"], match = [["git", "status"]], not_match = [["git", "reset", "--hard"]], @@ -124,17 +125,14 @@ prefix_rule( fn strictest_decision_wins_across_matches() { let policy_src = r#" prefix_rule( - id = "allow_git_status", pattern = ["git", "status"], decision = "allow", ) prefix_rule( - id = "prompt_git", pattern = ["git"], decision = "prompt", ) prefix_rule( - id = "forbid_git_commit", pattern = ["git", "commit"], decision = "forbidden", ) @@ -154,13 +152,11 @@ prefix_rule( assert_eq!( status_matches, vec![ - RuleMatch { - rule_id: "allow_git_status".to_string(), + RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git", "status"]), decision: Decision::Allow, }, - RuleMatch { - rule_id: "prompt_git".to_string(), + RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git"]), decision: Decision::Prompt, } @@ -179,13 +175,11 @@ prefix_rule( assert_eq!( commit_matches, vec![ - RuleMatch { - rule_id: "prompt_git".to_string(), + RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git"]), decision: Decision::Prompt, }, - RuleMatch { - rule_id: "forbid_git_commit".to_string(), + RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git", "commit"]), decision: Decision::Forbidden, }