diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index a6c0bd2a1d..8aee923822 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1195,7 +1195,6 @@ dependencies = [ "anyhow", "serde", "serde_json", - "shlex", "starlark", "thiserror 2.0.17", ] diff --git a/codex-rs/execpolicy2/Cargo.toml b/codex-rs/execpolicy2/Cargo.toml index 8489498eab..ceae2bd989 100644 --- a/codex-rs/execpolicy2/Cargo.toml +++ b/codex-rs/execpolicy2/Cargo.toml @@ -17,6 +17,5 @@ path = "src/main.rs" anyhow = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } -shlex = { workspace = true } starlark = { workspace = true } thiserror = { workspace = true } diff --git a/codex-rs/execpolicy2/src/command.rs b/codex-rs/execpolicy2/src/command.rs deleted file mode 100644 index da7c230a63..0000000000 --- a/codex-rs/execpolicy2/src/command.rs +++ /dev/null @@ -1,9 +0,0 @@ -use crate::error::Error; -use crate::error::Result; - -pub fn tokenize_command(raw: &str) -> Result> { - shlex::split(raw).ok_or_else(|| Error::TokenizationFailed { - example: raw.to_string(), - reason: "invalid shell tokens".to_string(), - }) -} diff --git a/codex-rs/execpolicy2/src/default.policy b/codex-rs/execpolicy2/src/default.policy index 54dd006cec..306114398a 100644 --- a/codex-rs/execpolicy2/src/default.policy +++ b/codex-rs/execpolicy2/src/default.policy @@ -2,12 +2,12 @@ prefix_rule( id = "git_status", pattern = ["git", "status"], match = [ - "git status", - "git status -- path/to/file", + ["git", "status"], + ["git", "status", "--", "path/to/file"], ], not_match = [ - "git statusx", - "git reset --hard", + ["git", "statusx"], + ["git", "reset", "--hard"], ], ) @@ -16,13 +16,13 @@ prefix_rule( pattern = ["npm", ["i", "install"]], decision = "prompt", match = [ - "npm i", - "npm install", - "npm install lodash", + ["npm", "i"], + ["npm", "install"], + ["npm", "install", "lodash"], ], not_match = [ - "npmx install", - "npm outdated", + ["npmx", "install"], + ["npm", "outdated"], ], ) @@ -31,6 +31,6 @@ prefix_rule( pattern = ["git", "reset", "--hard"], decision = "forbidden", match = [ - "git reset --hard", + ["git", "reset", "--hard"], ], ) diff --git a/codex-rs/execpolicy2/src/error.rs b/codex-rs/execpolicy2/src/error.rs index e572ff29dd..0ef2fe1856 100644 --- a/codex-rs/execpolicy2/src/error.rs +++ b/codex-rs/execpolicy2/src/error.rs @@ -8,8 +8,8 @@ pub enum Error { InvalidDecision(String), #[error("invalid pattern element: {0}")] InvalidPattern(String), - #[error("failed to tokenize example `{example}`: {reason}")] - TokenizationFailed { example: String, reason: 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}")] diff --git a/codex-rs/execpolicy2/src/lib.rs b/codex-rs/execpolicy2/src/lib.rs index 476f79b7f2..adee2ea731 100644 --- a/codex-rs/execpolicy2/src/lib.rs +++ b/codex-rs/execpolicy2/src/lib.rs @@ -1,11 +1,9 @@ -pub mod command; pub mod decision; pub mod error; pub mod parser; pub mod policy; pub mod rule; -pub use command::tokenize_command; pub use decision::Decision; pub use error::Error; pub use error::Result; diff --git a/codex-rs/execpolicy2/src/main.rs b/codex-rs/execpolicy2/src/main.rs index 04030a7b36..dd7239a897 100644 --- a/codex-rs/execpolicy2/src/main.rs +++ b/codex-rs/execpolicy2/src/main.rs @@ -6,7 +6,6 @@ use anyhow::Result; use anyhow::bail; use codex_execpolicy2::PolicyParser; use codex_execpolicy2::load_default_policy; -use codex_execpolicy2::tokenize_command; fn main() -> Result<()> { let mut args = std::env::args().skip(1); @@ -45,17 +44,11 @@ fn run_subcommand( fn cmd_check(policy_path: Option, args: Vec) -> Result<()> { if args.is_empty() { - bail!("usage: codex-execpolicy2 check "); + bail!("usage: codex-execpolicy2 check "); } let policy = load_policy(policy_path)?; - let tokens = if args.len() == 1 { - tokenize_command(&args[0])? - } else { - args - }; - - match policy.evaluate(&tokens) { + match policy.evaluate(&args) { Some(eval) => { let json = serde_json::to_string_pretty(&eval)?; println!("{json}"); @@ -80,6 +73,6 @@ fn load_policy(policy_path: Option) -> Result fn print_usage() { eprintln!( "usage: - codex-execpolicy2 [--policy path] check " + codex-execpolicy2 [--policy path] check " ); } diff --git a/codex-rs/execpolicy2/src/parser.rs b/codex-rs/execpolicy2/src/parser.rs index 1823136501..acf5e4d7ba 100644 --- a/codex-rs/execpolicy2/src/parser.rs +++ b/codex-rs/execpolicy2/src/parser.rs @@ -12,7 +12,6 @@ use starlark::values::list::ListRef; use starlark::values::list::UnpackList; use starlark::values::none::NoneType; -use crate::command::tokenize_command; use crate::decision::Decision; use crate::error::Error; use crate::error::Result; @@ -136,13 +135,36 @@ fn parse_pattern<'v>(pattern: UnpackList>) -> Result>> Ok(expand_pattern(&parts)) } +fn parse_examples<'v>(examples: UnpackList>) -> Result>> { + let mut parsed = Vec::new(); + for example in examples.items { + let list = ListRef::from_value(example).ok_or_else(|| { + Error::InvalidExample("example must be a list of strings".to_string()) + })?; + let mut tokens = Vec::new(); + for value in list.content() { + let token = value.unpack_str().ok_or_else(|| { + Error::InvalidExample("example tokens must be strings".to_string()) + })?; + tokens.push(token.to_string()); + } + if tokens.is_empty() { + return Err(Error::InvalidExample( + "example cannot be an empty list".to_string(), + )); + } + parsed.push(tokens); + } + Ok(parsed) +} + #[starlark_module] fn policy_builtins(builder: &mut GlobalsBuilder) { fn prefix_rule<'v>( pattern: UnpackList>, decision: Option<&'v str>, - r#match: Option>, - not_match: Option>, + r#match: Option>>, + not_match: Option>>, id: Option<&'v str>, eval: &mut Evaluator<'v, '_, '_>, ) -> anyhow::Result { @@ -153,24 +175,10 @@ fn policy_builtins(builder: &mut GlobalsBuilder) { let prefixes = parse_pattern(pattern)?; - let positive_examples: Vec> = r#match - .map(|examples| { - examples - .items - .into_iter() - .map(tokenize_command) - .collect::>>() - }) - .transpose()? - .unwrap_or_default(); + let positive_examples: Vec> = + r#match.map(parse_examples).transpose()?.unwrap_or_default(); let negative_examples: Vec> = not_match - .map(|examples| { - examples - .items - .into_iter() - .map(tokenize_command) - .collect::>>() - }) + .map(parse_examples) .transpose()? .unwrap_or_default(); diff --git a/codex-rs/execpolicy2/tests/basic.rs b/codex-rs/execpolicy2/tests/basic.rs index 7b69a93d15..5273409e76 100644 --- a/codex-rs/execpolicy2/tests/basic.rs +++ b/codex-rs/execpolicy2/tests/basic.rs @@ -1,12 +1,15 @@ use codex_execpolicy2::Decision; use codex_execpolicy2::PolicyParser; use codex_execpolicy2::RuleMatch; -use codex_execpolicy2::tokenize_command; + +fn tokens(cmd: &[&str]) -> Vec { + cmd.iter().map(|token| token.to_string()).collect() +} #[test] fn matches_default_git_status() { let policy = codex_execpolicy2::load_default_policy().expect("parse"); - let cmd = tokenize_command("git status").expect("tokenize"); + let cmd = tokens(&["git", "status"]); let eval = policy.evaluate(&cmd).expect("match"); assert_eq!(eval.decision, Decision::Allow); assert_eq!( @@ -30,14 +33,11 @@ prefix_rule( let parser = PolicyParser::new("test.policy", policy_src); let policy = parser.parse().expect("parse policy"); - for cmd in ["npm i", "npm install"] { - let tokens = tokenize_command(cmd).expect("tokenize"); - let eval = policy.evaluate(&tokens).expect("match"); - let matched_prefix = if cmd.ends_with(" i") { - vec!["npm".to_string(), "i".to_string()] - } else { - vec!["npm".to_string(), "install".to_string()] - }; + for (cmd, matched_prefix) in [ + (tokens(&["npm", "i"]), tokens(&["npm", "i"])), + (tokens(&["npm", "install"]), tokens(&["npm", "install"])), + ] { + let eval = policy.evaluate(&cmd).expect("match"); assert_eq!( eval.matched_rules, vec![RuleMatch { @@ -48,7 +48,7 @@ prefix_rule( ); } - let no_match = tokenize_command("npmx install").expect("tokenize"); + let no_match = tokens(&["npmx", "install"]); assert!(policy.evaluate(&no_match).is_none()); } @@ -58,20 +58,16 @@ fn match_and_not_match_examples_are_enforced() { prefix_rule( id = "git_status", pattern = ["git", "status"], - match = ["git status"], - not_match = ["git reset --hard"], + match = [["git", "status"]], + not_match = [["git", "reset", "--hard"]], ) "#; let parser = PolicyParser::new("test.policy", policy_src); let policy = parser.parse().expect("parse policy"); + assert!(policy.evaluate(&tokens(&["git", "status"])).is_some()); assert!( policy - .evaluate(&tokenize_command("git status").expect("tokenize")) - .is_some() - ); - assert!( - policy - .evaluate(&tokenize_command("git reset --hard").expect("tokenize")) + .evaluate(&tokens(&["git", "reset", "--hard"])) .is_none() ); } @@ -98,7 +94,7 @@ prefix_rule( let parser = PolicyParser::new("test.policy", policy_src); let policy = parser.parse().expect("parse policy"); - let status = tokenize_command("git status").expect("tokenize"); + let status = tokens(&["git", "status"]); let status_eval = policy.evaluate(&status).expect("match"); assert_eq!(status_eval.decision, Decision::Prompt); assert_eq!( @@ -117,7 +113,7 @@ prefix_rule( ] ); - let commit = tokenize_command("git commit -m hi").expect("tokenize"); + let commit = tokens(&["git", "commit", "-m", "hi"]); let commit_eval = policy.evaluate(&commit).expect("match"); assert_eq!(commit_eval.decision, Decision::Forbidden); assert_eq!( @@ -147,9 +143,7 @@ prefix_rule( "#; let parser = PolicyParser::new("test.policy", policy_src); let policy = parser.parse().expect("parse policy"); - let eval = policy - .evaluate(&tokenize_command("echo hi").expect("tokenize")) - .expect("match"); + let eval = policy.evaluate(&tokens(&["echo", "hi"])).expect("match"); assert_eq!( eval.matched_rules, vec![RuleMatch {