From 003a4cd5b8fa0c198506445c93519279d9064f35 Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Wed, 3 Dec 2025 18:37:55 +0000 Subject: [PATCH] feat: better execpolicy ask again behavior --- codex-rs/cli/tests/execpolicy.rs | 18 +- codex-rs/core/src/exec_policy.rs | 241 +++++++++++++----- codex-rs/core/src/tools/handlers/shell.rs | 1 - codex-rs/execpolicy/README.md | 32 +-- codex-rs/execpolicy/src/execpolicycheck.rs | 28 +- codex-rs/execpolicy/src/main.rs | 8 +- codex-rs/execpolicy/src/policy.rs | 96 ++++--- codex-rs/execpolicy/src/rule.rs | 5 + codex-rs/execpolicy/tests/basic.rs | 94 ++++--- .../tui/src/bottom_pane/approval_overlay.rs | 40 ++- ...hatwidget__tests__approval_modal_exec.snap | 8 +- ..._tests__approval_modal_exec_no_reason.snap | 5 +- ...dget__tests__exec_approval_modal_exec.snap | 13 +- ...sts__status_widget_and_approval_modal.snap | 28 +- codex-rs/tui/src/chatwidget/tests.rs | 20 +- docs/execpolicy.md | 24 ++ 16 files changed, 422 insertions(+), 239 deletions(-) diff --git a/codex-rs/cli/tests/execpolicy.rs b/codex-rs/cli/tests/execpolicy.rs index c6bca85bc6..4610b95874 100644 --- a/codex-rs/cli/tests/execpolicy.rs +++ b/codex-rs/cli/tests/execpolicy.rs @@ -40,17 +40,15 @@ prefix_rule( assert_eq!( result, json!({ - "match": { - "decision": "forbidden", - "matchedRules": [ - { - "prefixRuleMatch": { - "matchedPrefix": ["git", "push"], - "decision": "forbidden" - } + "decision": "forbidden", + "matchedRules": [ + { + "prefixRuleMatch": { + "matchedPrefix": ["git", "push"], + "decision": "forbidden" } - ] - } + } + ] }) ); diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 44328de14a..80d5aeae34 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -10,6 +10,7 @@ use codex_execpolicy::Error as ExecPolicyRuleError; use codex_execpolicy::Evaluation; use codex_execpolicy::Policy; use codex_execpolicy::PolicyParser; +use codex_execpolicy::RuleMatch; use codex_execpolicy::blocking_append_allow_prefix_rule; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::SandboxPolicy; @@ -24,6 +25,8 @@ use crate::sandboxing::SandboxPermissions; use crate::tools::sandboxing::ApprovalRequirement; const FORBIDDEN_REASON: &str = "execpolicy forbids this command"; +const PROMPT_CONFLICT_REASON: &str = + "execpolicy requires approval for this command, but AskForApproval is set to Never"; const PROMPT_REASON: &str = "execpolicy requires approval for this command"; const POLICY_DIR_NAME: &str = "policy"; const POLICY_EXTENSION: &str = "codexpolicy"; @@ -129,45 +132,32 @@ pub(crate) async fn append_allow_prefix_rule_and_update( Ok(()) } -fn requirement_from_decision( - decision: Decision, - approval_policy: AskForApproval, -) -> ApprovalRequirement { - match decision { - Decision::Forbidden => ApprovalRequirement::Forbidden { - reason: FORBIDDEN_REASON.to_string(), - }, - Decision::Prompt => { - let reason = PROMPT_REASON.to_string(); - if matches!(approval_policy, AskForApproval::Never) { - ApprovalRequirement::Forbidden { reason } - } else { - ApprovalRequirement::NeedsApproval { - reason: Some(reason), - allow_prefix: None, +/// Return an allow-prefix option when a command needs approval and execpolicy did not drive the decision +fn allow_prefix_if_applicable(evaluation: &Evaluation, features: &Features) -> Option> { + if !features.enabled(Feature::ExecPolicy) { + return None; + } + + if evaluation.decision != Decision::Prompt { + return None; + } + + let mut first_prompt_from_heuristics: Option> = None; + for rule_match in &evaluation.matched_rules { + match rule_match { + RuleMatch::HeuristicsRuleMatch { command, decision } => { + if *decision == Decision::Prompt && first_prompt_from_heuristics.is_none() { + first_prompt_from_heuristics = Some(command.clone()); } } + _ if rule_match.decision() == Decision::Prompt => { + return None; + } + _ => {} } - Decision::Allow => ApprovalRequirement::Skip { - bypass_sandbox: true, - }, } -} -/// Return an allow-prefix option when a single plain command needs approval without -/// any matching policy rule. We only surface the prefix opt-in when execpolicy did -/// not already drive the decision (NoMatch) and when the command is a single -/// unrolled command (multi-part scripts shouldn’t be whitelisted via prefix) and -/// when execpolicy feature is enabled. -fn allow_prefix_if_applicable( - commands: &[Vec], - features: &Features, -) -> Option> { - if features.enabled(Feature::ExecPolicy) && commands.len() == 1 { - Some(commands[0].clone()) - } else { - None - } + first_prompt_from_heuristics } pub(crate) async fn create_approval_requirement_for_command( @@ -179,31 +169,56 @@ pub(crate) async fn create_approval_requirement_for_command( sandbox_permissions: SandboxPermissions, ) -> ApprovalRequirement { let commands = parse_shell_lc_plain_commands(command).unwrap_or_else(|| vec![command.to_vec()]); + let heuristics_fallback = |cmd: &[String]| { + if requires_initial_appoval(approval_policy, sandbox_policy, cmd, sandbox_permissions) { + Decision::Prompt + } else { + Decision::Allow + } + }; let policy = exec_policy.read().await; - let evaluation = policy.check_multiple(commands.iter()); + let evaluation = policy.check_multiple(commands.iter(), &heuristics_fallback); + let has_policy_allow = evaluation.matched_rules.iter().any(|rule_match| { + !matches!(rule_match, RuleMatch::HeuristicsRuleMatch { .. }) + && rule_match.decision() == Decision::Allow + }); - match evaluation { - Evaluation::Match { decision, .. } => requirement_from_decision(decision, approval_policy), - Evaluation::NoMatch { .. } => { - if requires_initial_appoval( - approval_policy, - sandbox_policy, - command, - sandbox_permissions, - ) { - ApprovalRequirement::NeedsApproval { - reason: None, - allow_prefix: allow_prefix_if_applicable(&commands, features), + match evaluation.decision { + Decision::Forbidden => ApprovalRequirement::Forbidden { + reason: FORBIDDEN_REASON.to_string(), + }, + Decision::Prompt => { + let prompt_reason = derive_prompt_reason(&evaluation); + if matches!(approval_policy, AskForApproval::Never) { + ApprovalRequirement::Forbidden { + reason: PROMPT_CONFLICT_REASON.to_string(), } } else { - ApprovalRequirement::Skip { - bypass_sandbox: false, + ApprovalRequirement::NeedsApproval { + reason: prompt_reason, + allow_prefix: allow_prefix_if_applicable(&evaluation, features), } } } + Decision::Allow => ApprovalRequirement::Skip { + bypass_sandbox: has_policy_allow, + }, } } +/// Only return PROMPT_REASON when an execpolicy rule drove the prompt decision +fn derive_prompt_reason(evaluation: &Evaluation) -> Option { + evaluation.matched_rules.iter().find_map(|rule_match| { + if !matches!(rule_match, RuleMatch::HeuristicsRuleMatch { .. }) + && rule_match.decision() == Decision::Prompt + { + Some(PROMPT_REASON.to_string()) + } else { + None + } + }) +} + async fn collect_policy_files(dir: &Path) -> Result, ExecPolicyError> { let mut read_dir = match fs::read_dir(dir).await { Ok(read_dir) => read_dir, @@ -273,10 +288,19 @@ mod tests { .expect("policy result"); let commands = [vec!["rm".to_string()]]; - assert!(matches!( - policy.read().await.check_multiple(commands.iter()), - Evaluation::NoMatch { .. } - )); + assert_eq!( + Evaluation { + decision: Decision::Allow, + matched_rules: vec![RuleMatch::HeuristicsRuleMatch { + command: vec!["rm".to_string()], + decision: Decision::Allow + }], + }, + policy + .read() + .await + .check_multiple(commands.iter(), &|_| Decision::Allow) + ); assert!(!temp_dir.path().join(POLICY_DIR_NAME).exists()); } @@ -307,10 +331,19 @@ mod tests { .await .expect("policy result"); let command = [vec!["rm".to_string()]]; - assert!(matches!( - policy.read().await.check_multiple(command.iter()), - Evaluation::Match { .. } - )); + assert_eq!( + Evaluation { + decision: Decision::Forbidden, + matched_rules: vec![RuleMatch::PrefixRuleMatch { + matched_prefix: vec!["rm".to_string()], + decision: Decision::Forbidden + }], + }, + policy + .read() + .await + .check_multiple(command.iter(), &|_| Decision::Allow) + ); } #[tokio::test] @@ -326,10 +359,19 @@ mod tests { .await .expect("policy result"); let command = [vec!["ls".to_string()]]; - assert!(matches!( - policy.read().await.check_multiple(command.iter()), - Evaluation::NoMatch { .. } - )); + assert_eq!( + Evaluation { + decision: Decision::Allow, + matched_rules: vec![RuleMatch::HeuristicsRuleMatch { + command: vec!["ls".to_string()], + decision: Decision::Allow + }], + }, + policy + .read() + .await + .check_multiple(command.iter(), &|_| Decision::Allow) + ); } #[tokio::test] @@ -419,7 +461,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") assert_eq!( requirement, ApprovalRequirement::Forbidden { - reason: PROMPT_REASON.to_string() + reason: PROMPT_CONFLICT_REASON.to_string() } ); } @@ -448,6 +490,37 @@ prefix_rule(pattern=["rm"], decision="forbidden") ); } + #[tokio::test] + async fn heuristics_apply_when_other_commands_match_policy() { + let policy_src = r#"prefix_rule(pattern=["apple"], decision="allow")"#; + let mut parser = PolicyParser::new(); + parser + .parse("test.codexpolicy", policy_src) + .expect("parse policy"); + let policy = Arc::new(RwLock::new(parser.build())); + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "apple | orange".to_string(), + ]; + + assert_eq!( + create_approval_requirement_for_command( + &policy, + &Features::with_defaults(), + &command, + AskForApproval::UnlessTrusted, + &SandboxPolicy::DangerFullAccess, + SandboxPermissions::UseDefault, + ) + .await, + ApprovalRequirement::NeedsApproval { + reason: None, + allow_prefix: Some(vec!["orange".to_string()]) + } + ); + } + #[tokio::test] async fn append_allow_prefix_rule_updates_policy_and_file() { let codex_home = tempdir().expect("create temp dir"); @@ -458,14 +531,13 @@ prefix_rule(pattern=["rm"], decision="forbidden") .await .expect("update policy"); - let evaluation = current_policy.read().await.check(&[ - "echo".to_string(), - "hello".to_string(), - "world".to_string(), - ]); + let evaluation = current_policy.read().await.check( + &["echo".to_string(), "hello".to_string(), "world".to_string()], + &|_| Decision::Allow, + ); assert!(matches!( evaluation, - Evaluation::Match { + Evaluation { decision: Decision::Allow, .. } @@ -597,7 +669,38 @@ prefix_rule(pattern=["rm"], decision="forbidden") requirement, ApprovalRequirement::NeedsApproval { reason: None, - allow_prefix: None, + allow_prefix: Some(vec!["python".to_string()]), + } + ); + } + + #[tokio::test] + async fn allow_prefix_uses_first_no_match_in_multi_command_scripts() { + let policy_src = r#"prefix_rule(pattern=["python"], decision="allow")"#; + let mut parser = PolicyParser::new(); + parser + .parse("test.codexpolicy", policy_src) + .expect("parse policy"); + let policy = Arc::new(RwLock::new(parser.build())); + + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "python && echo ok".to_string(), + ]; + + assert_eq!( + create_approval_requirement_for_command( + &policy, + &Features::with_defaults(), + &command, + AskForApproval::UnlessTrusted, + &SandboxPolicy::ReadOnly, + SandboxPermissions::UseDefault, + ) + .await, + ApprovalRequirement::Skip { + bypass_sandbox: true } ); } diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 3928ee6b47..3be04b6c82 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -241,7 +241,6 @@ impl ShellHandler { SandboxPermissions::from(exec_params.with_escalated_permissions.unwrap_or(false)), ) .await; - let req = ShellRequest { command: exec_params.command.clone(), cwd: exec_params.cwd.clone(), diff --git a/codex-rs/execpolicy/README.md b/codex-rs/execpolicy/README.md index 9fd9c63306..1ddb67252f 100644 --- a/codex-rs/execpolicy/README.md +++ b/codex-rs/execpolicy/README.md @@ -30,32 +30,24 @@ codex execpolicy check --policy path/to/policy.codexpolicy git status cargo run -p codex-execpolicy -- check --policy path/to/policy.codexpolicy git status ``` - Example outcomes: - - Match: `{"match": { ... "decision": "allow" ... }}` - - No match: `{"noMatch": {}}` + - Match: `{"matchedRules":[{...}],"decision":"allow"}` + - No match: `{"matchedRules":[]}` -## Response shapes -- Match: +## Response shape ```json { - "match": { - "decision": "allow|prompt|forbidden", - "matchedRules": [ - { - "prefixRuleMatch": { - "matchedPrefix": ["", "..."], - "decision": "allow|prompt|forbidden" - } + "matchedRules": [ + { + "prefixRuleMatch": { + "matchedPrefix": ["", "..."], + "decision": "allow|prompt|forbidden" } - ] - } + } + ], + "decision": "allow|prompt|forbidden" } ``` - -- No match: -```json -{"noMatch": {}} -``` - +- When no rules match, `matchedRules` is an empty array and `decision` is omitted. - `matchedRules` lists every rule whose prefix matched the command; `matchedPrefix` is the exact prefix that matched. - The effective `decision` is the strictest severity across all matches (`forbidden` > `prompt` > `allow`). diff --git a/codex-rs/execpolicy/src/execpolicycheck.rs b/codex-rs/execpolicy/src/execpolicycheck.rs index 0b5e0dcafc..939ed8590b 100644 --- a/codex-rs/execpolicy/src/execpolicycheck.rs +++ b/codex-rs/execpolicy/src/execpolicycheck.rs @@ -4,10 +4,12 @@ use std::path::PathBuf; use anyhow::Context; use anyhow::Result; use clap::Parser; +use serde::Serialize; -use crate::Evaluation; +use crate::Decision; use crate::Policy; use crate::PolicyParser; +use crate::RuleMatch; /// Arguments for evaluating a command against one or more execpolicy files. #[derive(Debug, Parser, Clone)] @@ -34,20 +36,25 @@ impl ExecPolicyCheckCommand { /// Load the policies for this command, evaluate the command, and render JSON output. pub fn run(&self) -> Result<()> { let policy = load_policies(&self.policies)?; - let evaluation = policy.check(&self.command); + let matched_rules = policy.matches_for_command(&self.command, None); - let json = format_evaluation_json(&evaluation, self.pretty)?; + let json = format_matches_json(&matched_rules, self.pretty)?; println!("{json}"); Ok(()) } } -pub fn format_evaluation_json(evaluation: &Evaluation, pretty: bool) -> Result { +pub fn format_matches_json(matched_rules: &[RuleMatch], pretty: bool) -> Result { + let output = ExecPolicyCheckOutput { + matched_rules, + decision: matched_rules.iter().map(RuleMatch::decision).max(), + }; + if pretty { - serde_json::to_string_pretty(evaluation).map_err(Into::into) + serde_json::to_string_pretty(&output).map_err(Into::into) } else { - serde_json::to_string(evaluation).map_err(Into::into) + serde_json::to_string(&output).map_err(Into::into) } } @@ -65,3 +72,12 @@ pub fn load_policies(policy_paths: &[PathBuf]) -> Result { Ok(parser.build()) } + +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] +struct ExecPolicyCheckOutput<'a> { + #[serde(rename = "matchedRules")] + matched_rules: &'a [RuleMatch], + #[serde(skip_serializing_if = "Option::is_none")] + decision: Option, +} diff --git a/codex-rs/execpolicy/src/main.rs b/codex-rs/execpolicy/src/main.rs index e1373b6d16..d3b34a3307 100644 --- a/codex-rs/execpolicy/src/main.rs +++ b/codex-rs/execpolicy/src/main.rs @@ -1,6 +1,6 @@ use anyhow::Result; use clap::Parser; -use codex_execpolicy::ExecPolicyCheckCommand; +use codex_execpolicy::execpolicycheck::ExecPolicyCheckCommand; /// CLI for evaluating exec policies #[derive(Parser)] @@ -13,10 +13,6 @@ enum Cli { fn main() -> Result<()> { let cli = Cli::parse(); match cli { - Cli::Check(cmd) => cmd_check(cmd), + Cli::Check(cmd) => cmd.run(), } } - -fn cmd_check(cmd: ExecPolicyCheckCommand) -> Result<()> { - cmd.run() -} diff --git a/codex-rs/execpolicy/src/policy.rs b/codex-rs/execpolicy/src/policy.rs index 10858c9fad..991e904ae9 100644 --- a/codex-rs/execpolicy/src/policy.rs +++ b/codex-rs/execpolicy/src/policy.rs @@ -11,6 +11,8 @@ use serde::Deserialize; use serde::Serialize; use std::sync::Arc; +type HeuristicsFallback<'a> = Option<&'a dyn Fn(&[String]) -> Decision>; + #[derive(Clone, Debug)] pub struct Policy { rules_by_program: MultiMap, @@ -50,62 +52,84 @@ impl Policy { Ok(()) } - pub fn check(&self, cmd: &[String]) -> Evaluation { - let rules = match cmd.first() { - Some(first) => match self.rules_by_program.get_vec(first) { - Some(rules) => rules, - None => return Evaluation::NoMatch {}, - }, - None => return Evaluation::NoMatch {}, - }; - - let matched_rules: Vec = - rules.iter().filter_map(|rule| rule.matches(cmd)).collect(); - match matched_rules.iter().map(RuleMatch::decision).max() { - Some(decision) => Evaluation::Match { - decision, - matched_rules, - }, - None => Evaluation::NoMatch {}, - } + pub fn check(&self, cmd: &[String], heuristics_fallback: &F) -> Evaluation + where + F: Fn(&[String]) -> Decision, + { + let matched_rules = self.matches_for_command(cmd, Some(heuristics_fallback)); + Evaluation::from_matches(matched_rules) } - pub fn check_multiple(&self, commands: Commands) -> Evaluation + pub fn check_multiple( + &self, + commands: Commands, + heuristics_fallback: &F, + ) -> Evaluation where Commands: IntoIterator, Commands::Item: AsRef<[String]>, + F: Fn(&[String]) -> Decision, { let matched_rules: Vec = commands .into_iter() - .flat_map(|command| match self.check(command.as_ref()) { - Evaluation::Match { matched_rules, .. } => matched_rules, - Evaluation::NoMatch { .. } => Vec::new(), + .flat_map(|command| { + self.matches_for_command(command.as_ref(), Some(heuristics_fallback)) }) .collect(); - match matched_rules.iter().map(RuleMatch::decision).max() { - Some(decision) => Evaluation::Match { - decision, - matched_rules, - }, - None => Evaluation::NoMatch {}, + Evaluation::from_matches(matched_rules) + } + + pub fn matches_for_command( + &self, + cmd: &[String], + heuristics_fallback: HeuristicsFallback<'_>, + ) -> Vec { + let mut matched_rules: Vec = match cmd.first() { + Some(first) => self + .rules_by_program + .get_vec(first) + .map(|rules| rules.iter().filter_map(|rule| rule.matches(cmd)).collect()) + .unwrap_or_default(), + None => Vec::new(), + }; + + if let (true, Some(heuristics_fallback)) = (matched_rules.is_empty(), heuristics_fallback) { + matched_rules.push(RuleMatch::HeuristicsRuleMatch { + command: cmd.to_vec(), + decision: heuristics_fallback(cmd), + }); } + + matched_rules } } #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] -pub enum Evaluation { - NoMatch {}, - Match { - decision: Decision, - #[serde(rename = "matchedRules")] - matched_rules: Vec, - }, +pub struct Evaluation { + pub decision: Decision, + #[serde(rename = "matchedRules")] + pub matched_rules: Vec, } impl Evaluation { pub fn is_match(&self) -> bool { - matches!(self, Self::Match { .. }) + self.matched_rules + .iter() + .any(|rule_match| !matches!(rule_match, RuleMatch::HeuristicsRuleMatch { .. })) + } + + fn from_matches(matched_rules: Vec) -> Self { + let decision = matched_rules + .iter() + .map(RuleMatch::decision) + .max() + .unwrap_or(Decision::Allow); + + Self { + decision, + matched_rules, + } } } diff --git a/codex-rs/execpolicy/src/rule.rs b/codex-rs/execpolicy/src/rule.rs index 20e23fe6a2..cd0756bbb3 100644 --- a/codex-rs/execpolicy/src/rule.rs +++ b/codex-rs/execpolicy/src/rule.rs @@ -64,12 +64,17 @@ pub enum RuleMatch { matched_prefix: Vec, decision: Decision, }, + HeuristicsRuleMatch { + command: Vec, + decision: Decision, + }, } impl RuleMatch { pub fn decision(&self) -> Decision { match self { Self::PrefixRuleMatch { decision, .. } => *decision, + Self::HeuristicsRuleMatch { decision, .. } => *decision, } } } diff --git a/codex-rs/execpolicy/tests/basic.rs b/codex-rs/execpolicy/tests/basic.rs index 9a7ec58b1e..3ea33604ae 100644 --- a/codex-rs/execpolicy/tests/basic.rs +++ b/codex-rs/execpolicy/tests/basic.rs @@ -19,6 +19,14 @@ fn tokens(cmd: &[&str]) -> Vec { cmd.iter().map(std::string::ToString::to_string).collect() } +fn allow_all(_: &[String]) -> Decision { + Decision::Allow +} + +fn prompt_all(_: &[String]) -> Decision { + Decision::Prompt +} + #[derive(Clone, Debug, Eq, PartialEq)] enum RuleSnapshot { Prefix(PrefixRule), @@ -49,9 +57,9 @@ prefix_rule( parser.parse("test.codexpolicy", policy_src)?; let policy = parser.build(); let cmd = tokens(&["git", "status"]); - let evaluation = policy.check(&cmd); + let evaluation = policy.check(&cmd, &allow_all); assert_eq!( - Evaluation::Match { + Evaluation { decision: Decision::Allow, matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git", "status"]), @@ -80,9 +88,9 @@ fn add_prefix_rule_extends_policy() -> Result<()> { rules ); - let evaluation = policy.check(&tokens(&["ls", "-l", "/tmp"])); + let evaluation = policy.check(&tokens(&["ls", "-l", "/tmp"]), &allow_all); assert_eq!( - Evaluation::Match { + Evaluation { decision: Decision::Prompt, matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["ls", "-l"]), @@ -146,9 +154,9 @@ prefix_rule( git_rules ); - let status_eval = policy.check(&tokens(&["git", "status"])); + let status_eval = policy.check(&tokens(&["git", "status"]), &allow_all); assert_eq!( - Evaluation::Match { + Evaluation { decision: Decision::Prompt, matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git"]), @@ -158,9 +166,9 @@ prefix_rule( status_eval ); - let commit_eval = policy.check(&tokens(&["git", "commit", "-m", "hi"])); + let commit_eval = policy.check(&tokens(&["git", "commit", "-m", "hi"]), &allow_all); assert_eq!( - Evaluation::Match { + Evaluation { decision: Decision::Forbidden, matched_rules: vec![ RuleMatch::PrefixRuleMatch { @@ -217,9 +225,9 @@ prefix_rule( sh_rules ); - let bash_eval = policy.check(&tokens(&["bash", "-c", "echo", "hi"])); + let bash_eval = policy.check(&tokens(&["bash", "-c", "echo", "hi"]), &allow_all); assert_eq!( - Evaluation::Match { + Evaluation { decision: Decision::Allow, matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["bash", "-c"]), @@ -229,9 +237,9 @@ prefix_rule( bash_eval ); - let sh_eval = policy.check(&tokens(&["sh", "-l", "echo", "hi"])); + let sh_eval = policy.check(&tokens(&["sh", "-l", "echo", "hi"]), &allow_all); assert_eq!( - Evaluation::Match { + Evaluation { decision: Decision::Allow, matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["sh", "-l"]), @@ -273,9 +281,9 @@ prefix_rule( rules ); - let npm_i = policy.check(&tokens(&["npm", "i", "--legacy-peer-deps"])); + let npm_i = policy.check(&tokens(&["npm", "i", "--legacy-peer-deps"]), &allow_all); assert_eq!( - Evaluation::Match { + Evaluation { decision: Decision::Allow, matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["npm", "i", "--legacy-peer-deps"]), @@ -285,9 +293,12 @@ prefix_rule( npm_i ); - let npm_install = policy.check(&tokens(&["npm", "install", "--no-save", "leftpad"])); + let npm_install = policy.check( + &tokens(&["npm", "install", "--no-save", "leftpad"]), + &allow_all, + ); assert_eq!( - Evaluation::Match { + Evaluation { decision: Decision::Allow, matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["npm", "install", "--no-save"]), @@ -314,9 +325,9 @@ prefix_rule( let mut parser = PolicyParser::new(); parser.parse("test.codexpolicy", policy_src)?; let policy = parser.build(); - let match_eval = policy.check(&tokens(&["git", "status"])); + let match_eval = policy.check(&tokens(&["git", "status"]), &allow_all); assert_eq!( - Evaluation::Match { + Evaluation { decision: Decision::Allow, matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git", "status"]), @@ -326,13 +337,20 @@ prefix_rule( match_eval ); - let no_match_eval = policy.check(&tokens(&[ - "git", - "--config", - "color.status=always", - "status", - ])); - assert_eq!(Evaluation::NoMatch {}, no_match_eval); + let no_match_eval = policy.check( + &tokens(&["git", "--config", "color.status=always", "status"]), + &allow_all, + ); + assert_eq!( + Evaluation { + decision: Decision::Allow, + matched_rules: vec![RuleMatch::HeuristicsRuleMatch { + command: tokens(&["git", "--config", "color.status=always", "status",]), + decision: Decision::Allow, + }], + }, + no_match_eval + ); Ok(()) } @@ -352,9 +370,9 @@ prefix_rule( parser.parse("test.codexpolicy", policy_src)?; let policy = parser.build(); - let commit = policy.check(&tokens(&["git", "commit", "-m", "hi"])); + let commit = policy.check(&tokens(&["git", "commit", "-m", "hi"]), &allow_all); assert_eq!( - Evaluation::Match { + Evaluation { decision: Decision::Forbidden, matched_rules: vec![ RuleMatch::PrefixRuleMatch { @@ -393,9 +411,9 @@ prefix_rule( tokens(&["git", "commit", "-m", "hi"]), ]; - let evaluation = policy.check_multiple(&commands); + let evaluation = policy.check_multiple(&commands, &allow_all); assert_eq!( - Evaluation::Match { + Evaluation { decision: Decision::Forbidden, matched_rules: vec![ RuleMatch::PrefixRuleMatch { @@ -416,3 +434,21 @@ prefix_rule( ); Ok(()) } + +#[test] +fn heuristics_match_is_returned_when_no_policy_matches() { + let policy = Policy::empty(); + let command = tokens(&["python"]); + + let evaluation = policy.check(&command, &prompt_all); + assert_eq!( + Evaluation { + decision: Decision::Prompt, + matched_rules: vec![RuleMatch::HeuristicsRuleMatch { + command, + decision: Decision::Prompt, + }], + }, + evaluation + ); +} diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 3defdbf9e3..82e8123f34 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -471,28 +471,23 @@ impl ApprovalOption { } fn exec_options(allow_prefix: Option>) -> Vec { - vec![ - ApprovalOption { - label: "Yes, proceed".to_string(), - decision: ApprovalDecision::Review(ReviewDecision::Approved), - display_shortcut: None, - additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))], - }, - ApprovalOption { - label: "Yes, and don't ask again this session".to_string(), - decision: ApprovalDecision::Review(ReviewDecision::ApprovedForSession), - display_shortcut: None, - additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))], - }, - ] - .into_iter() - .chain(allow_prefix.map(|prefix| ApprovalOption { - label: "Yes, and don't ask again for commands with this prefix".to_string(), - decision: ApprovalDecision::Review(ReviewDecision::ApprovedAllowPrefix { - allow_prefix: prefix, - }), + vec![ApprovalOption { + label: "Yes, proceed".to_string(), + decision: ApprovalDecision::Review(ReviewDecision::Approved), display_shortcut: None, - additional_shortcuts: vec![key_hint::plain(KeyCode::Char('p'))], + additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))], + }] + .into_iter() + .chain(allow_prefix.map(|prefix| { + let rendered_prefix = strip_bash_lc_and_escape(&prefix); + ApprovalOption { + label: format!("Yes, and don't ask again for `{rendered_prefix}` commands"), + decision: ApprovalDecision::Review(ReviewDecision::ApprovedAllowPrefix { + allow_prefix: prefix, + }), + display_shortcut: None, + additional_shortcuts: vec![key_hint::plain(KeyCode::Char('p'))], + } })) .chain([ApprovalOption { label: "No, and tell Codex what to do differently".to_string(), @@ -687,7 +682,6 @@ mod tests { let (tx_raw, mut rx) = unbounded_channel::(); let tx = AppEventSender::new(tx_raw); let mut view = ApprovalOverlay::new(make_exec_request(), tx); - view.handle_key_event(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE)); view.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); assert!( @@ -702,6 +696,6 @@ mod tests { break; } } - assert_eq!(decision, Some(ReviewDecision::ApprovedForSession)); + assert_eq!(decision, Some(ReviewDecision::Approved)); } } diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap index eaf0fed3a1..748537beee 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap @@ -4,14 +4,12 @@ expression: terminal.backend().vt100().screen().contents() --- Would you like to run the following command? - Reason: this is a test reason such as one that would be produced by the - model + Reason: this is a test reason such as one that would be produced by the model $ echo hello world › 1. Yes, proceed (y) - 2. Yes, and don't ask again this session (a) - 3. Yes, and don't ask again for commands with this prefix (p) - 4. No, and tell Codex what to do differently (esc) + 2. Yes, and don't ask again for `echo hello world` commands (p) + 3. No, and tell Codex what to do differently (esc) Press enter to confirm or esc to cancel diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap index ce2277ce62..8f3d27fe7c 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap @@ -7,8 +7,7 @@ expression: terminal.backend().vt100().screen().contents() $ echo hello world › 1. Yes, proceed (y) - 2. Yes, and don't ask again this session (a) - 3. Yes, and don't ask again for commands with this prefix (p) - 4. No, and tell Codex what to do differently (esc) + 2. Yes, and don't ask again for `echo hello world` commands (p) + 3. No, and tell Codex what to do differently (esc) Press enter to confirm or esc to cancel diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap index ff70d7d492..1c6a3ef136 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap @@ -3,7 +3,7 @@ source: tui/src/chatwidget/tests.rs expression: "format!(\"{buf:?}\")" --- Buffer { - area: Rect { x: 0, y: 0, width: 80, height: 14 }, + area: Rect { x: 0, y: 0, width: 80, height: 13 }, content: [ " ", " ", @@ -15,8 +15,7 @@ Buffer { " $ echo hello world ", " ", "› 1. Yes, proceed (y) ", - " 2. Yes, and don't ask again this session (a) ", - " 3. No, and tell Codex what to do differently (esc) ", + " 2. No, and tell Codex what to do differently (esc) ", " ", " Press enter to confirm or esc to cancel ", ], @@ -30,10 +29,8 @@ Buffer { x: 7, y: 5, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, x: 0, y: 9, fg: Cyan, bg: Reset, underline: Reset, modifier: BOLD, x: 21, y: 9, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, - x: 44, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, - x: 45, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, - x: 48, y: 11, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, - x: 51, y: 11, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, - x: 2, y: 13, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 48, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 51, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 2, y: 12, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, ] } diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap index 5ce388c87b..459ad2c2d9 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap @@ -2,18 +2,16 @@ source: tui/src/chatwidget/tests.rs expression: terminal.backend() --- -" " -" " -" Would you like to run the following command? " -" " -" Reason: this is a test reason such as one that would be produced by the " -" model " -" " -" $ echo 'hello world' " -" " -"› 1. Yes, proceed (y) " -" 2. Yes, and don't ask again this session (a) " -" 3. Yes, and don't ask again for commands with this prefix (p) " -" 4. No, and tell Codex what to do differently (esc) " -" " -" Press enter to confirm or esc to cancel " +" " +" " +" Would you like to run the following command? " +" " +" Reason: this is a test reason such as one that would be produced by the model " +" " +" $ echo 'hello world' " +" " +"› 1. Yes, proceed (y) " +" 2. Yes, and don't ask again for `echo 'hello world'` commands (p) " +" 3. No, and tell Codex what to do differently (esc) " +" " +" Press enter to confirm or esc to cancel " diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index c25355a759..5202908ade 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -1981,11 +1981,12 @@ fn approval_modal_exec_snapshot() { }); // Render to a fixed-size test terminal and snapshot. // Call desired_height first and use that exact height for rendering. - let height = chat.desired_height(80); + let width = 100; + let height = chat.desired_height(width); let mut terminal = - crate::custom_terminal::Terminal::with_options(VT100Backend::new(80, height)) + crate::custom_terminal::Terminal::with_options(VT100Backend::new(width, height)) .expect("create terminal"); - let viewport = Rect::new(0, 0, 80, height); + let viewport = Rect::new(0, 0, width, height); terminal.set_viewport_area(viewport); terminal @@ -2027,10 +2028,11 @@ fn approval_modal_exec_without_reason_snapshot() { msg: EventMsg::ExecApprovalRequest(ev), }); - let height = chat.desired_height(80); + let width = 100; + let height = chat.desired_height(width); let mut terminal = - ratatui::Terminal::new(VT100Backend::new(80, height)).expect("create terminal"); - terminal.set_viewport_area(Rect::new(0, 0, 80, height)); + ratatui::Terminal::new(VT100Backend::new(width, height)).expect("create terminal"); + terminal.set_viewport_area(Rect::new(0, 0, width, height)); terminal .draw(|f| chat.render(f.area(), f.buffer_mut())) .expect("draw approval modal (no reason)"); @@ -2242,9 +2244,11 @@ fn status_widget_and_approval_modal_snapshot() { }); // Render at the widget's desired height and snapshot. - let height = chat.desired_height(80); - let mut terminal = ratatui::Terminal::new(ratatui::backend::TestBackend::new(80, height)) + let width: u16 = 100; + let height = chat.desired_height(width); + let mut terminal = ratatui::Terminal::new(ratatui::backend::TestBackend::new(width, height)) .expect("create terminal"); + terminal.set_viewport_area(Rect::new(0, 0, width, height)); terminal .draw(|f| chat.render(f.area(), f.buffer_mut())) .expect("draw status + approval modal"); diff --git a/docs/execpolicy.md b/docs/execpolicy.md index a5b77e402e..77d8ed0e7b 100644 --- a/docs/execpolicy.md +++ b/docs/execpolicy.md @@ -33,6 +33,30 @@ codex execpolicy check --policy ~/.codex/policy/default.codexpolicy git push ori Pass multiple `--policy` flags to test how several files combine, and use `--pretty` for formatted JSON output. See the [`codex-rs/execpolicy` README](../codex-rs/execpolicy/README.md) for a more detailed walkthrough of the available syntax. +Example output when a rule matches: + +```json +{ + "matchedRules": [ + { + "prefixRuleMatch": { + "matchedPrefix": ["git", "push"], + "decision": "prompt" + } + } + ], + "decision": "prompt" +} +``` + +When no rules match, `matchedRules` is an empty array and `decision` is omitted. + +```json +{ + "matchedRules": [], +} + ## Status `execpolicy` commands are still in preview. The API may have breaking changes in the future. +```