refactor: implementing rule traits, rulematch enum

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": "<rule id>",
-        "matched_prefix": ["<token>", "..."],
-        "decision": "allow|prompt|forbidden"
+        "PrefixRuleMatch": {
+          "matched_prefix": ["<token>", "..."],
+          "decision": "allow|prompt|forbidden"
+        }
       }
     ]
   }

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"],

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),
 }

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<MultiMap<String, Rule>>,
-    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<UnpackList<Value<'v>>>,
         not_match: Option<UnpackList<Value<'v>>>,
-        id: Option<&'v str>,
         eval: &mut Evaluator<'v, '_, '_>,
     ) -> anyhow::Result<NoneType> {
         let decision = match decision {
@@ -187,33 +178,28 @@ fn policy_builtins(builder: &mut GlobalsBuilder) {
 
         let pattern_tokens = parse_pattern(pattern)?;
 
-        let positive_examples: Vec<Vec<String>> =
+        let matches: Vec<Vec<String>> =
             r#match.map(parse_examples).transpose()?.unwrap_or_default();
-        let negative_examples: Vec<Vec<String>> = not_match
+        let not_matches: Vec<Vec<String>> = 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, &not_matches)?;
             builder.add_rule(rule);
         }
         Ok(NoneType)

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);

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<String>,
-    pub decision: Decision,
+pub enum RuleMatch {
+    PrefixRuleMatch {
+        matched_prefix: Vec<String>,
+        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<RuleMatch> {
+        match self {
+            Self::Prefix(rule) => rule.matches(cmd),
+        }
+    }
+
+    pub fn validate_examples(
+        &self,
+        matches: &[Vec<String>],
+        not_matches: &[Vec<String>],
+    ) -> Result<()> {
+        match self {
+            Self::Prefix(rule) => rule.validate_examples(matches, not_matches),
+        }
+    }
+}
+
+impl PrefixRule {
     pub fn matches(&self, cmd: &[String]) -> Option<RuleMatch> {
         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<String>],
-        negative: &[Vec<String>],
+        matches: &[Vec<String>],
+        not_matches: &[Vec<String>],
     ) -> 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",
+    }
+}

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<String> {
     cmd.iter().map(std::string::ToString::to_string).collect()
@@ -12,7 +13,6 @@ fn tokens(cmd: &[&str]) -> Vec<String> {
 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,
             }