diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index b6af898ca5..06b98452f7 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1194,6 +1194,7 @@ version = "0.0.0" dependencies = [ "anyhow", "clap", + "expect-test", "multimap", "parking_lot", "serde", @@ -2149,6 +2150,12 @@ dependencies = [ "syn 2.0.104", ] +[[package]] +name = "dissimilar" +version = "1.0.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8975ffdaa0ef3661bfe02dbdcc06c9f829dfafe6a3c474de366a8d5e44276921" + [[package]] name = "doc-comment" version = "0.3.3" @@ -2380,6 +2387,16 @@ dependencies = [ "pin-project-lite", ] +[[package]] +name = "expect-test" +version = "1.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "63af43ff4431e848fb47472a920f14fa71c24de13255a5692e93d4e90302acb0" +dependencies = [ + "dissimilar", + "once_cell", +] + [[package]] name = "eyre" version = "0.6.12" diff --git a/codex-rs/execpolicy2/Cargo.toml b/codex-rs/execpolicy2/Cargo.toml index 91ca1aff94..cb74b98920 100644 --- a/codex-rs/execpolicy2/Cargo.toml +++ b/codex-rs/execpolicy2/Cargo.toml @@ -25,3 +25,6 @@ multimap = { workspace = true } clap = { workspace = true, features = ["derive"] } parking_lot = { workspace = true } shlex = { workspace = true } + +[dev-dependencies] +expect-test = "1.5.0" diff --git a/codex-rs/execpolicy2/src/decision.rs b/codex-rs/execpolicy2/src/decision.rs index 15dc1f711d..68231358e1 100644 --- a/codex-rs/execpolicy2/src/decision.rs +++ b/codex-rs/execpolicy2/src/decision.rs @@ -25,3 +25,13 @@ impl Decision { } } } + +impl std::fmt::Display for Decision { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Allow => f.write_str("allow"), + Self::Prompt => f.write_str("prompt"), + Self::Forbidden => f.write_str("forbidden"), + } + } +} diff --git a/codex-rs/execpolicy2/src/policy.rs b/codex-rs/execpolicy2/src/policy.rs index 7896e95bb2..f2c3e6c683 100644 --- a/codex-rs/execpolicy2/src/policy.rs +++ b/codex-rs/execpolicy2/src/policy.rs @@ -64,3 +64,23 @@ impl Policy { } } } + +impl std::fmt::Display for Evaluation { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::NoMatch => f.write_str("NoMatch"), + Self::Match { + decision, + matched_rules, + } => { + writeln!(f, "Match {{")?; + writeln!(f, " decision: {decision},")?; + writeln!(f, " matched_rules: [")?; + for rule in matched_rules { + writeln!(f, " {rule},")?; + } + write!(f, " ]\n}}") + } + } + } +} diff --git a/codex-rs/execpolicy2/src/rule.rs b/codex-rs/execpolicy2/src/rule.rs index eda25beda8..100803bcca 100644 --- a/codex-rs/execpolicy2/src/rule.rs +++ b/codex-rs/execpolicy2/src/rule.rs @@ -28,6 +28,12 @@ impl PatternToken { } } +impl std::fmt::Display for PatternToken { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(render_pattern_token(self).as_str()) + } +} + /// Prefix matcher for commands with support for alternative match tokens. /// First token is fixed since we key by the first token in policy. #[derive(Clone, Debug, Eq, PartialEq)] @@ -60,11 +66,29 @@ impl PrefixPattern { } } +impl std::fmt::Display for PrefixPattern { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "[{}]", render_pattern(self)) + } +} + #[derive(Clone, Debug)] pub enum Rule { Prefix(PrefixRule), } +impl std::fmt::Display for Rule { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Prefix(rule) => write!( + f, + "prefix_rule(pattern = {}, decision = {})", + rule.pattern, rule.decision + ), + } + } +} + #[derive(Clone, Debug, Eq, PartialEq)] pub struct PrefixRule { pub pattern: PrefixPattern, @@ -87,6 +111,20 @@ impl RuleMatch { } } +impl std::fmt::Display for RuleMatch { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::PrefixRuleMatch { + matched_prefix, + decision, + } => write!( + f, + "PrefixRuleMatch {{ matched_prefix: {matched_prefix:?}, decision: {decision} }}" + ), + } + } +} + impl Rule { pub fn program(&self) -> &str { match self { diff --git a/codex-rs/execpolicy2/tests/basic.rs b/codex-rs/execpolicy2/tests/basic.rs index 3ac13b2bce..ba444e3cae 100644 --- a/codex-rs/execpolicy2/tests/basic.rs +++ b/codex-rs/execpolicy2/tests/basic.rs @@ -1,14 +1,22 @@ -use codex_execpolicy2::Decision; -use codex_execpolicy2::Evaluation; use codex_execpolicy2::PolicyParser; -use codex_execpolicy2::RuleMatch; -use codex_execpolicy2::rule::PatternToken; -use codex_execpolicy2::rule::Rule; +use codex_execpolicy2::Rule; +use expect_test::expect; fn tokens(cmd: &[&str]) -> Vec { cmd.iter().map(std::string::ToString::to_string).collect() } +fn rules_to_string(rules: &[Rule]) -> String { + format!( + "[{}]", + rules + .iter() + .map(std::string::ToString::to_string) + .collect::>() + .join(", ") + ) +} + #[test] fn basic_match() { let policy_src = r#" @@ -20,21 +28,14 @@ prefix_rule( .parse() .expect("parse policy"); let cmd = tokens(&["git", "status"]); - let Evaluation::Match { - decision, - matched_rules, - } = policy.evaluate(&cmd) - else { - panic!("expected match"); - }; - assert_eq!(decision, Decision::Allow); - assert_eq!( - matched_rules, - vec![RuleMatch::PrefixRuleMatch { - matched_prefix: tokens(&["git", "status"]), - decision: Decision::Allow, - }] - ); + let evaluation = policy.evaluate(&cmd); + expect![[r#"Match { + decision: allow, + matched_rules: [ + PrefixRuleMatch { matched_prefix: ["git", "status"], decision: allow }, + ] +}"#]] + .assert_eq(&evaluation.to_string()); } #[test] @@ -49,24 +50,28 @@ prefix_rule( let bash_rules = policy.rules().get_vec("bash").expect("bash rules"); let sh_rules = policy.rules().get_vec("sh").expect("sh rules"); - assert_eq!(bash_rules.len(), 1); - assert_eq!(sh_rules.len(), 1); + expect![[r#"[prefix_rule(pattern = [bash, [-c, -l]], decision = allow)]"#]] + .assert_eq(&rules_to_string(bash_rules)); + expect![[r#"[prefix_rule(pattern = [sh, [-c, -l]], decision = allow)]"#]] + .assert_eq(&rules_to_string(sh_rules)); - for (cmd, prefix) in [ - ( - tokens(&["bash", "-c", "echo", "hi"]), - tokens(&["bash", "-c"]), - ), - (tokens(&["sh", "-l", "echo", "hi"]), tokens(&["sh", "-l"])), - ] { - let Evaluation::Match { matched_rules, .. } = policy.evaluate(&cmd) else { - panic!("expected match"); - }; - let matched_prefix = match &matched_rules[0] { - RuleMatch::PrefixRuleMatch { matched_prefix, .. } => matched_prefix, - }; - assert_eq!(matched_prefix, &prefix); - } + let bash_eval = policy.evaluate(&tokens(&["bash", "-c", "echo", "hi"])); + expect![[r#"Match { + decision: allow, + matched_rules: [ + PrefixRuleMatch { matched_prefix: ["bash", "-c"], decision: allow }, + ] +}"#]] + .assert_eq(&bash_eval.to_string()); + + let sh_eval = policy.evaluate(&tokens(&["sh", "-l", "echo", "hi"])); + expect![[r#"Match { + decision: allow, + matched_rules: [ + PrefixRuleMatch { matched_prefix: ["sh", "-l"], decision: allow }, + ] +}"#]] + .assert_eq(&sh_eval.to_string()); } #[test] @@ -80,22 +85,26 @@ prefix_rule( let policy = parser.parse().expect("parse policy"); let rules = policy.rules().get_vec("npm").expect("npm rules"); - assert_eq!(rules.len(), 1); - let Rule::Prefix(rule) = &rules[0]; - assert_eq!( - rule.pattern.rest, - vec![ - PatternToken::Alts(tokens(&["i", "install"])), - PatternToken::Alts(tokens(&["--legacy-peer-deps", "--no-save"])), - ], - ); + expect![[r#"[prefix_rule(pattern = [npm, [i, install], [--legacy-peer-deps, --no-save]], decision = allow)]"#]] + .assert_eq(&rules_to_string(rules)); - for cmd in [ - tokens(&["npm", "i", "--legacy-peer-deps"]), - tokens(&["npm", "install", "--no-save", "leftpad"]), - ] { - assert!(matches!(policy.evaluate(&cmd), Evaluation::Match { .. })); - } + let npm_i = policy.evaluate(&tokens(&["npm", "i", "--legacy-peer-deps"])); + expect![[r#"Match { + decision: allow, + matched_rules: [ + PrefixRuleMatch { matched_prefix: ["npm", "i", "--legacy-peer-deps"], decision: allow }, + ] +}"#]] + .assert_eq(&npm_i.to_string()); + + let npm_install = policy.evaluate(&tokens(&["npm", "install", "--no-save", "leftpad"])); + expect![[r#"Match { + decision: allow, + matched_rules: [ + PrefixRuleMatch { matched_prefix: ["npm", "install", "--no-save"], decision: allow }, + ] +}"#]] + .assert_eq(&npm_install.to_string()); } #[test] @@ -109,14 +118,17 @@ prefix_rule( "#; let parser = PolicyParser::new("test.policy", policy_src); let policy = parser.parse().expect("parse policy"); - assert!(matches!( - policy.evaluate(&tokens(&["git", "status"])), - Evaluation::Match { .. } - )); - assert!(matches!( - policy.evaluate(&tokens(&["git", "reset", "--hard"])), - Evaluation::NoMatch - )); + let match_eval = policy.evaluate(&tokens(&["git", "status"])); + expect![[r#"Match { + decision: allow, + matched_rules: [ + PrefixRuleMatch { matched_prefix: ["git", "status"], decision: allow }, + ] +}"#]] + .assert_eq(&match_eval.to_string()); + + let no_match_eval = policy.evaluate(&tokens(&["git", "reset", "--hard"])); + expect!["NoMatch"].assert_eq(&no_match_eval.to_string()); } #[test] @@ -138,49 +150,23 @@ prefix_rule( let parser = PolicyParser::new("test.policy", policy_src); let policy = parser.parse().expect("parse policy"); - let status = tokens(&["git", "status"]); - let Evaluation::Match { - decision: status_decision, - matched_rules: status_matches, - } = policy.evaluate(&status) - else { - panic!("expected status to match"); - }; - assert_eq!(status_decision, Decision::Prompt); - assert_eq!( - status_matches, - vec![ - RuleMatch::PrefixRuleMatch { - matched_prefix: tokens(&["git", "status"]), - decision: Decision::Allow, - }, - RuleMatch::PrefixRuleMatch { - matched_prefix: tokens(&["git"]), - decision: Decision::Prompt, - } - ] - ); + let status = policy.evaluate(&tokens(&["git", "status"])); + expect![[r#"Match { + decision: prompt, + matched_rules: [ + PrefixRuleMatch { matched_prefix: ["git", "status"], decision: allow }, + PrefixRuleMatch { matched_prefix: ["git"], decision: prompt }, + ] +}"#]] + .assert_eq(&status.to_string()); - let commit = tokens(&["git", "commit", "-m", "hi"]); - let Evaluation::Match { - decision: commit_decision, - matched_rules: commit_matches, - } = policy.evaluate(&commit) - else { - panic!("expected commit to match"); - }; - assert_eq!(commit_decision, Decision::Forbidden); - assert_eq!( - commit_matches, - vec![ - RuleMatch::PrefixRuleMatch { - matched_prefix: tokens(&["git"]), - decision: Decision::Prompt, - }, - RuleMatch::PrefixRuleMatch { - matched_prefix: tokens(&["git", "commit"]), - decision: Decision::Forbidden, - } - ] - ); + let commit = policy.evaluate(&tokens(&["git", "commit", "-m", "hi"])); + expect![[r#"Match { + decision: forbidden, + matched_rules: [ + PrefixRuleMatch { matched_prefix: ["git"], decision: prompt }, + PrefixRuleMatch { matched_prefix: ["git", "commit"], decision: forbidden }, + ] +}"#]] + .assert_eq(&commit.to_string()); }