diff --git a/codex-rs/execpolicy2/src/error.rs b/codex-rs/execpolicy2/src/error.rs index 8311418cba..d8d45c2e2b 100644 --- a/codex-rs/execpolicy2/src/error.rs +++ b/codex-rs/execpolicy2/src/error.rs @@ -2,7 +2,7 @@ use thiserror::Error; pub type Result = std::result::Result; -#[derive(Debug, Error)] +#[derive(Debug, Error, PartialEq, Eq)] pub enum Error { #[error("invalid decision: {0}")] InvalidDecision(String), @@ -10,8 +10,14 @@ pub enum Error { InvalidPattern(String), #[error("invalid example: {0}")] InvalidExample(String), - #[error("expected example to match rule `{rule}`: {example}")] - ExampleDidNotMatch { rule: String, example: String }, + #[error( + "expected every example to match at least one rule. rules: {rules:?}; unmatched examples: \ + {examples:?}" + )] + ExampleDidNotMatch { + rules: Vec, + examples: Vec, + }, #[error("expected example to not match rule `{rule}`: {example}")] ExampleDidMatch { rule: String, example: String }, #[error("starlark error: {0}")] diff --git a/codex-rs/execpolicy2/src/parser.rs b/codex-rs/execpolicy2/src/parser.rs index f61002560f..ad7390ace1 100644 --- a/codex-rs/execpolicy2/src/parser.rs +++ b/codex-rs/execpolicy2/src/parser.rs @@ -17,6 +17,7 @@ use std::sync::Arc; use crate::decision::Decision; use crate::error::Error; use crate::error::Result; +use crate::policy::validate_match_examples; use crate::rule::PatternToken; use crate::rule::PrefixPattern; use crate::rule::PrefixRule; @@ -203,17 +204,28 @@ fn policy_builtins(builder: &mut GlobalsBuilder) { let rest: Arc<[PatternToken]> = remaining_tokens.to_vec().into(); - for head in first_token.alternatives() { - let rule = Rule::Prefix(PrefixRule { - pattern: PrefixPattern { - first: Arc::from(head.as_str()), - rest: rest.clone(), - }, - decision, - }); - rule.validate_examples(&matches, ¬_matches)?; - builder.add_rule(rule); - } + let rules: Vec = first_token + .alternatives() + .iter() + .zip(std::iter::repeat(rest.clone())) + .map(|(head, rest)| { + Rule::Prefix(PrefixRule { + pattern: PrefixPattern { + first: Arc::from(head.as_str()), + rest, + }, + decision, + }) + }) + .collect(); + + rules + .iter() + .try_for_each(|rule| rule.validate_not_matches(¬_matches))?; + + validate_match_examples(&rules, &matches)?; + + rules.into_iter().for_each(|rule| builder.add_rule(rule)); Ok(NoneType) } } diff --git a/codex-rs/execpolicy2/src/policy.rs b/codex-rs/execpolicy2/src/policy.rs index dc1c8517b1..cd234f5b0f 100644 --- a/codex-rs/execpolicy2/src/policy.rs +++ b/codex-rs/execpolicy2/src/policy.rs @@ -1,9 +1,12 @@ use crate::decision::Decision; +use crate::error::Error; +use crate::error::Result; use crate::rule::Rule; use crate::rule::RuleMatch; use multimap::MultiMap; use serde::Deserialize; use serde::Serialize; +use shlex::try_join; #[derive(Clone, Debug)] pub struct Policy { @@ -55,3 +58,33 @@ impl Evaluation { matches!(self, Self::Match { .. }) } } + +pub(crate) fn validate_match_examples(rules: &[Rule], matches: &[Vec]) -> Result<()> { + let match_counts = rules.iter().fold(vec![0; matches.len()], |counts, rule| { + counts + .iter() + .zip(rule.validate_matches(matches)) + .map(|(count, matched)| if matched { count + 1 } else { *count }) + .collect() + }); + + let unmatched_examples: Vec = matches + .iter() + .zip(&match_counts) + .filter(|(_, count)| **count == 0) + .map(|(example, _)| { + try_join(example.iter().map(String::as_str)) + .unwrap_or_else(|_| "unable to render example".to_string()) + }) + .collect(); + + if unmatched_examples.is_empty() { + return Ok(()); + } + + let rules: Vec = rules.iter().map(|rule| format!("{rule:?}")).collect(); + Err(Error::ExampleDidNotMatch { + rules, + examples: unmatched_examples, + }) +} diff --git a/codex-rs/execpolicy2/src/rule.rs b/codex-rs/execpolicy2/src/rule.rs index 586d0c3124..5fb4bcf1ad 100644 --- a/codex-rs/execpolicy2/src/rule.rs +++ b/codex-rs/execpolicy2/src/rule.rs @@ -101,19 +101,14 @@ impl PrefixRule { }) } - pub fn validate_examples( - &self, - matches: &[Vec], - not_matches: &[Vec], - ) -> Result<()> { - for example in matches { - if self.matches(example).is_none() { - return Err(Error::ExampleDidNotMatch { - rule: format!("{self:?}"), - example: join_command(example), - }); - } - } + pub fn validate_matches(&self, matches: &[Vec]) -> Vec { + matches + .iter() + .map(|example| self.matches(example).is_some()) + .collect() + } + + pub fn validate_not_matches(&self, not_matches: &[Vec]) -> Result<()> { for example in not_matches { if self.matches(example).is_some() { return Err(Error::ExampleDidMatch { @@ -144,13 +139,15 @@ impl Rule { } } - pub fn validate_examples( - &self, - matches: &[Vec], - not_matches: &[Vec], - ) -> Result<()> { + pub fn validate_matches(&self, matches: &[Vec]) -> Vec { match self { - Self::Prefix(rule) => rule.validate_examples(matches, not_matches), + Self::Prefix(rule) => rule.validate_matches(matches), + } + } + + pub fn validate_not_matches(&self, not_matches: &[Vec]) -> Result<()> { + match self { + Self::Prefix(rule) => rule.validate_not_matches(not_matches), } } } diff --git a/codex-rs/execpolicy2/tests/basic.rs b/codex-rs/execpolicy2/tests/basic.rs index af1ae048f2..90fa9549a4 100644 --- a/codex-rs/execpolicy2/tests/basic.rs +++ b/codex-rs/execpolicy2/tests/basic.rs @@ -1,6 +1,7 @@ use std::sync::Arc; use codex_execpolicy2::Decision; +use codex_execpolicy2::Error; use codex_execpolicy2::Evaluation; use codex_execpolicy2::PolicyParser; use codex_execpolicy2::Rule; @@ -180,6 +181,40 @@ prefix_rule( assert_eq!(Evaluation::NoMatch, no_match_eval); } +#[test] +fn reports_unmatched_example_for_first_token_alternatives() { + let policy_src = r#" +prefix_rule( + pattern = [["bash", "sh"], "-c"], + match = [ + ["bash", "-c"], + ["zsh", "-c"], + ], +) + "#; + + let err = PolicyParser::parse("test.codexpolicy", policy_src) + .expect_err("expected unmatched example error"); + let expected_message = r#"Traceback (most recent call last): + * test.codexpolicy:2, in + prefix_rule( +error: expected every example to match at least one rule. rules: ["Prefix(PrefixRule { pattern: PrefixPattern { first: "bash", rest: [Single("-c")] }, decision: Allow })", "Prefix(PrefixRule { pattern: PrefixPattern { first: "sh", rest: [Single("-c")] }, decision: Allow })"]; unmatched examples: ["zsh -c"] + --> test.codexpolicy:2:1 + | +2 | / prefix_rule( +3 | | pattern = [["bash", "sh"], "-c"], +4 | | match = [ +5 | | ["bash", "-c"], +6 | | ["zsh", "-c"], +7 | | ], +8 | | ) + | |_^ + | +"#; + let expected_err = Error::Starlark(expected_message.to_string()); + assert_eq!(expected_err, err); +} + #[test] fn strictest_decision_wins_across_matches() { let policy_src = r#"