diff --git a/codex-rs/cli/tests/execpolicy.rs b/codex-rs/cli/tests/execpolicy.rs index 7d8a2b1c4..30b5999c0 100644 --- a/codex-rs/cli/tests/execpolicy.rs +++ b/codex-rs/cli/tests/execpolicy.rs @@ -59,3 +59,61 @@ prefix_rule( Ok(()) } + +#[test] +fn execpolicy_check_includes_justification_when_present() -> Result<(), Box> +{ + let codex_home = TempDir::new()?; + let policy_path = codex_home.path().join("rules").join("policy.rules"); + fs::create_dir_all( + policy_path + .parent() + .expect("policy path should have a parent"), + )?; + fs::write( + &policy_path, + r#" +prefix_rule( + pattern = ["git", "push"], + decision = "forbidden", + justification = "pushing is blocked in this repo", +) +"#, + )?; + + let output = Command::new(codex_utils_cargo_bin::cargo_bin("codex")?) + .env("CODEX_HOME", codex_home.path()) + .args([ + "execpolicy", + "check", + "--rules", + policy_path + .to_str() + .expect("policy path should be valid UTF-8"), + "git", + "push", + "origin", + "main", + ]) + .output()?; + + assert!(output.status.success()); + let result: serde_json::Value = serde_json::from_slice(&output.stdout)?; + assert_eq!( + result, + json!({ + "decision": "forbidden", + "matchedRules": [ + { + "prefixRuleMatch": { + "matchedPrefix": ["git", "push"], + "decision": "forbidden", + "justification": "pushing is blocked in this repo" + } + } + ] + }) + ); + + Ok(()) +} diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 5caf7d9b8..4c8ad0687 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -28,11 +28,10 @@ use crate::features::Feature; use crate::features::Features; use crate::sandboxing::SandboxPermissions; use crate::tools::sandboxing::ExecApprovalRequirement; +use shlex::try_join as shlex_try_join; -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"; + "approval required by policy, but AskForApproval is set to Never"; const RULES_DIR_NAME: &str = "rules"; const RULE_EXTENSION: &str = "rules"; const DEFAULT_POLICY_FILE: &str = "default.rules"; @@ -128,7 +127,7 @@ impl ExecPolicyManager { match evaluation.decision { Decision::Forbidden => ExecApprovalRequirement::Forbidden { - reason: FORBIDDEN_REASON.to_string(), + reason: derive_forbidden_reason(command, &evaluation), }, Decision::Prompt => { if matches!(approval_policy, AskForApproval::Never) { @@ -137,7 +136,7 @@ impl ExecPolicyManager { } } else { ExecApprovalRequirement::NeedsApproval { - reason: derive_prompt_reason(&evaluation), + reason: derive_prompt_reason(command, &evaluation), proposed_execpolicy_amendment: if features.enabled(Feature::ExecPolicy) { try_derive_execpolicy_amendment_for_prompt_rules( &evaluation.matched_rules, @@ -299,15 +298,69 @@ fn try_derive_execpolicy_amendment_for_allow_rules( }) } -/// 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 is_policy_match(rule_match) && rule_match.decision() == Decision::Prompt { - Some(PROMPT_REASON.to_string()) - } else { - None +/// Only return a reason when a policy rule drove the prompt decision. +fn derive_prompt_reason(command_args: &[String], evaluation: &Evaluation) -> Option { + let command = render_shlex_command(command_args); + + let most_specific_prompt = evaluation + .matched_rules + .iter() + .filter_map(|rule_match| match rule_match { + RuleMatch::PrefixRuleMatch { + matched_prefix, + decision: Decision::Prompt, + justification, + .. + } => Some((matched_prefix.len(), justification.as_deref())), + _ => None, + }) + .max_by_key(|(matched_prefix_len, _)| *matched_prefix_len); + + match most_specific_prompt { + Some((_matched_prefix_len, Some(justification))) => { + Some(format!("`{command}` requires approval: {justification}")) } - }) + Some((_matched_prefix_len, None)) => { + Some(format!("`{command}` requires approval by policy")) + } + None => None, + } +} + +fn render_shlex_command(args: &[String]) -> String { + shlex_try_join(args.iter().map(String::as_str)).unwrap_or_else(|_| args.join(" ")) +} + +/// Derive a string explaining why the command was forbidden. If `justification` +/// is set by the user, this can contain instructions with recommended +/// alternatives, for example. +fn derive_forbidden_reason(command_args: &[String], evaluation: &Evaluation) -> String { + let command = render_shlex_command(command_args); + + let most_specific_forbidden = evaluation + .matched_rules + .iter() + .filter_map(|rule_match| match rule_match { + RuleMatch::PrefixRuleMatch { + matched_prefix, + decision: Decision::Forbidden, + justification, + .. + } => Some((matched_prefix, justification.as_deref())), + _ => None, + }) + .max_by_key(|(matched_prefix, _)| matched_prefix.len()); + + match most_specific_forbidden { + Some((_matched_prefix, Some(justification))) => { + format!("`{command}` rejected: {justification}") + } + Some((matched_prefix, None)) => { + let prefix = render_shlex_command(matched_prefix); + format!("`{command}` rejected: policy forbids commands starting with `{prefix}`") + } + None => format!("`{command}` rejected: blocked by policy"), + } } async fn collect_policy_files(dir: impl AsRef) -> Result, ExecPolicyError> { @@ -450,7 +503,8 @@ mod tests { decision: Decision::Forbidden, matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: vec!["rm".to_string()], - decision: Decision::Forbidden + decision: Decision::Forbidden, + justification: None, }], }, policy.check_multiple(command.iter(), &|_| Decision::Allow) @@ -528,7 +582,8 @@ mod tests { decision: Decision::Forbidden, matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: vec!["rm".to_string()], - decision: Decision::Forbidden + decision: Decision::Forbidden, + justification: None, }], }, policy.check_multiple([vec!["rm".to_string()]].iter(), &|_| Decision::Allow) @@ -538,7 +593,8 @@ mod tests { decision: Decision::Prompt, matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: vec!["ls".to_string()], - decision: Decision::Prompt + decision: Decision::Prompt, + justification: None, }], }, policy.check_multiple([vec!["ls".to_string()]].iter(), &|_| Decision::Allow) @@ -560,7 +616,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") let forbidden_script = vec![ "bash".to_string(), "-lc".to_string(), - "rm -rf /tmp".to_string(), + "rm -rf /some/important/folder".to_string(), ]; let manager = ExecPolicyManager::new(policy); @@ -577,7 +633,45 @@ prefix_rule(pattern=["rm"], decision="forbidden") assert_eq!( requirement, ExecApprovalRequirement::Forbidden { - reason: FORBIDDEN_REASON.to_string() + reason: "`bash -lc 'rm -rf /some/important/folder'` rejected: policy forbids commands starting with `rm`".to_string() + } + ); + } + + #[tokio::test] + async fn justification_is_included_in_forbidden_exec_approval_requirement() { + let policy_src = r#" +prefix_rule( + pattern=["rm"], + decision="forbidden", + justification="destructive command", +) +"#; + let mut parser = PolicyParser::new(); + parser + .parse("test.rules", policy_src) + .expect("parse policy"); + let policy = Arc::new(parser.build()); + + let manager = ExecPolicyManager::new(policy); + let requirement = manager + .create_exec_approval_requirement_for_command( + &Features::with_defaults(), + &[ + "rm".to_string(), + "-rf".to_string(), + "/some/important/folder".to_string(), + ], + AskForApproval::OnRequest, + &SandboxPolicy::DangerFullAccess, + SandboxPermissions::UseDefault, + ) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::Forbidden { + reason: "`rm -rf /some/important/folder` rejected: destructive command".to_string() } ); } @@ -606,7 +700,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") assert_eq!( requirement, ExecApprovalRequirement::NeedsApproval { - reason: Some(PROMPT_REASON.to_string()), + reason: Some("`rm` requires approval by policy".to_string()), proposed_execpolicy_amendment: None, } ); @@ -824,7 +918,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") assert_eq!( requirement, ExecApprovalRequirement::NeedsApproval { - reason: Some(PROMPT_REASON.to_string()), + reason: Some("`rm` requires approval by policy".to_string()), proposed_execpolicy_amendment: None, } ); diff --git a/codex-rs/core/tests/suite/exec_policy.rs b/codex-rs/core/tests/suite/exec_policy.rs index 470478ad7..c31df5036 100644 --- a/codex-rs/core/tests/suite/exec_policy.rs +++ b/codex-rs/core/tests/suite/exec_policy.rs @@ -97,7 +97,7 @@ async fn execpolicy_blocks_shell_invocation() -> Result<()> { assert!( end.aggregated_output - .contains("execpolicy forbids this command"), + .contains("policy forbids commands starting with `echo`"), "unexpected output: {}", end.aggregated_output ); diff --git a/codex-rs/execpolicy/README.md b/codex-rs/execpolicy/README.md index 288a46dcb..30fc57184 100644 --- a/codex-rs/execpolicy/README.md +++ b/codex-rs/execpolicy/README.md @@ -1,52 +1,65 @@ # codex-execpolicy ## Overview -- Policy engine and CLI built around `prefix_rule(pattern=[...], decision?, match?, not_match?)`. + +- Policy engine and CLI built around `prefix_rule(pattern=[...], decision?, justification?, match?, not_match?)`. - This release covers the prefix-rule subset of the execpolicy 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`. +- `justification` is an optional human-readable rationale for why a rule exists. It can be provided for any `decision` and may be surfaced in different contexts (for example, in approval prompts or rejection messages). When `decision = "forbidden"` is used, include a recommended alternative in the `justification`, when appropriate (e.g., ``"Use `jj` instead of `git`."``). - `match` / `not_match` supply example invocations that are validated at load time (think of them as unit tests); examples can be token arrays or strings (strings are tokenized with `shlex`). - The CLI always prints the JSON serialization of the evaluation result. - The legacy rule matcher lives in `codex-execpolicy-legacy`. ## Policy shapes + - Prefix rules use Starlark syntax: + ```starlark prefix_rule( pattern = ["cmd", ["alt1", "alt2"]], # ordered tokens; list entries denote alternatives decision = "prompt", # allow | prompt | forbidden; defaults to allow + justification = "explain why this rule exists", match = [["cmd", "alt1"], "cmd alt2"], # examples that must match this rule not_match = [["cmd", "oops"], "cmd alt3"], # examples that must not match this rule ) ``` ## CLI + - From the Codex CLI, run `codex execpolicy check` subcommand with one or more policy files (for example `src/default.rules`) to check a command: + ```bash codex execpolicy check --rules path/to/policy.rules git status ``` + - Pass multiple `--rules` flags to merge rules, evaluated in the order provided, and use `--pretty` for formatted JSON. - You can also run the standalone dev binary directly during development: + ```bash cargo run -p codex-execpolicy -- check --rules path/to/policy.rules git status ``` + - Example outcomes: - Match: `{"matchedRules":[{...}],"decision":"allow"}` - No match: `{"matchedRules":[]}` ## Response shape + ```json { "matchedRules": [ { "prefixRuleMatch": { "matchedPrefix": ["", "..."], - "decision": "allow|prompt|forbidden" + "decision": "allow|prompt|forbidden", + "justification": "..." } } ], "decision": "allow|prompt|forbidden" } ``` + - 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/examples/example.codexpolicy b/codex-rs/execpolicy/examples/example.codexpolicy index 5bb691b6f..4040f6120 100644 --- a/codex-rs/execpolicy/examples/example.codexpolicy +++ b/codex-rs/execpolicy/examples/example.codexpolicy @@ -4,6 +4,7 @@ prefix_rule( pattern = ["git", "reset", "--hard"], decision = "forbidden", + justification = "destructive operation", match = [ ["git", "reset", "--hard"], ], diff --git a/codex-rs/execpolicy/src/error.rs b/codex-rs/execpolicy/src/error.rs index 2f168a027..9664e71a5 100644 --- a/codex-rs/execpolicy/src/error.rs +++ b/codex-rs/execpolicy/src/error.rs @@ -11,6 +11,8 @@ pub enum Error { InvalidPattern(String), #[error("invalid example: {0}")] InvalidExample(String), + #[error("invalid rule: {0}")] + InvalidRule(String), #[error( "expected every example to match at least one rule. rules: {rules:?}; unmatched examples: \ {examples:?}" diff --git a/codex-rs/execpolicy/src/parser.rs b/codex-rs/execpolicy/src/parser.rs index d50549055..0ff0f4b34 100644 --- a/codex-rs/execpolicy/src/parser.rs +++ b/codex-rs/execpolicy/src/parser.rs @@ -212,6 +212,7 @@ fn policy_builtins(builder: &mut GlobalsBuilder) { decision: Option<&'v str>, r#match: Option>>, not_match: Option>>, + justification: Option<&'v str>, eval: &mut Evaluator<'v, '_, '_>, ) -> anyhow::Result { let decision = match decision { @@ -219,6 +220,14 @@ fn policy_builtins(builder: &mut GlobalsBuilder) { None => Decision::Allow, }; + let justification = match justification { + Some(raw) if raw.trim().is_empty() => { + return Err(Error::InvalidRule("justification cannot be empty".to_string()).into()); + } + Some(raw) => Some(raw.to_string()), + None => None, + }; + let pattern_tokens = parse_pattern(pattern)?; let matches: Vec> = @@ -246,6 +255,7 @@ fn policy_builtins(builder: &mut GlobalsBuilder) { rest: rest.clone(), }, decision, + justification: justification.clone(), }) as RuleRef }) .collect(); diff --git a/codex-rs/execpolicy/src/policy.rs b/codex-rs/execpolicy/src/policy.rs index 991e904ae..0c06d572e 100644 --- a/codex-rs/execpolicy/src/policy.rs +++ b/codex-rs/execpolicy/src/policy.rs @@ -46,6 +46,7 @@ impl Policy { .into(), }, decision, + justification: None, }); self.rules_by_program.insert(first_token.clone(), rule); diff --git a/codex-rs/execpolicy/src/rule.rs b/codex-rs/execpolicy/src/rule.rs index cd0756bbb..de78a5fda 100644 --- a/codex-rs/execpolicy/src/rule.rs +++ b/codex-rs/execpolicy/src/rule.rs @@ -63,6 +63,12 @@ pub enum RuleMatch { #[serde(rename = "matchedPrefix")] matched_prefix: Vec, decision: Decision, + /// Optional rationale for why this rule exists. + /// + /// This can be supplied for any decision and may be surfaced in different contexts + /// (e.g., prompt reasons or rejection messages). + #[serde(skip_serializing_if = "Option::is_none")] + justification: Option, }, HeuristicsRuleMatch { command: Vec, @@ -83,6 +89,7 @@ impl RuleMatch { pub struct PrefixRule { pub pattern: PrefixPattern, pub decision: Decision, + pub justification: Option, } pub trait Rule: Any + Debug + Send + Sync { @@ -104,6 +111,7 @@ impl Rule for PrefixRule { .map(|matched_prefix| RuleMatch::PrefixRuleMatch { matched_prefix, decision: self.decision, + justification: self.justification.clone(), }) } } diff --git a/codex-rs/execpolicy/tests/basic.rs b/codex-rs/execpolicy/tests/basic.rs index 7ae5e6e21..ed6cf3185 100644 --- a/codex-rs/execpolicy/tests/basic.rs +++ b/codex-rs/execpolicy/tests/basic.rs @@ -64,6 +64,7 @@ prefix_rule( matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git", "status"]), decision: Decision::Allow, + justification: None, }], }, evaluation @@ -71,6 +72,84 @@ prefix_rule( Ok(()) } +#[test] +fn justification_is_attached_to_forbidden_matches() -> Result<()> { + let policy_src = r#" +prefix_rule( + pattern = ["rm"], + decision = "forbidden", + justification = "destructive command", +) + "#; + let mut parser = PolicyParser::new(); + parser.parse("test.rules", policy_src)?; + let policy = parser.build(); + + let evaluation = policy.check( + &tokens(&["rm", "-rf", "/some/important/folder"]), + &allow_all, + ); + assert_eq!( + Evaluation { + decision: Decision::Forbidden, + matched_rules: vec![RuleMatch::PrefixRuleMatch { + matched_prefix: tokens(&["rm"]), + decision: Decision::Forbidden, + justification: Some("destructive command".to_string()), + }], + }, + evaluation + ); + Ok(()) +} + +#[test] +fn justification_can_be_used_with_allow_decision() -> Result<()> { + let policy_src = r#" +prefix_rule( + pattern = ["ls"], + decision = "allow", + justification = "safe and commonly used", +) + "#; + let mut parser = PolicyParser::new(); + parser.parse("test.rules", policy_src)?; + let policy = parser.build(); + + let evaluation = policy.check(&tokens(&["ls", "-l"]), &prompt_all); + assert_eq!( + Evaluation { + decision: Decision::Allow, + matched_rules: vec![RuleMatch::PrefixRuleMatch { + matched_prefix: tokens(&["ls"]), + decision: Decision::Allow, + justification: Some("safe and commonly used".to_string()), + }], + }, + evaluation + ); + Ok(()) +} + +#[test] +fn justification_cannot_be_empty() { + let policy_src = r#" +prefix_rule( + pattern = ["ls"], + decision = "prompt", + justification = " ", +) + "#; + let mut parser = PolicyParser::new(); + let err = parser + .parse("test.rules", policy_src) + .expect_err("expected parse error"); + assert!( + err.to_string() + .contains("invalid rule: justification cannot be empty") + ); +} + #[test] fn add_prefix_rule_extends_policy() -> Result<()> { let mut policy = Policy::empty(); @@ -84,17 +163,19 @@ fn add_prefix_rule_extends_policy() -> Result<()> { rest: vec![PatternToken::Single(String::from("-l"))].into(), }, decision: Decision::Prompt, + justification: None, })], rules ); - let evaluation = policy.check(&tokens(&["ls", "-l", "/tmp"]), &allow_all); + let evaluation = policy.check(&tokens(&["ls", "-l", "/some/important/folder"]), &allow_all); assert_eq!( Evaluation { decision: Decision::Prompt, matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["ls", "-l"]), decision: Decision::Prompt, + justification: None, }], }, evaluation @@ -142,6 +223,7 @@ prefix_rule( rest: Vec::::new().into(), }, decision: Decision::Prompt, + justification: None, }), RuleSnapshot::Prefix(PrefixRule { pattern: PrefixPattern { @@ -149,6 +231,7 @@ prefix_rule( rest: vec![PatternToken::Single("commit".to_string())].into(), }, decision: Decision::Forbidden, + justification: None, }), ], git_rules @@ -161,6 +244,7 @@ prefix_rule( matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git"]), decision: Decision::Prompt, + justification: None, }], }, status_eval @@ -174,10 +258,12 @@ prefix_rule( RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git"]), decision: Decision::Prompt, + justification: None, }, RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git", "commit"]), decision: Decision::Forbidden, + justification: None, }, ], }, @@ -211,6 +297,7 @@ prefix_rule( rest: vec![PatternToken::Alts(vec!["-c".to_string(), "-l".to_string()])].into(), }, decision: Decision::Allow, + justification: None, })], bash_rules ); @@ -221,6 +308,7 @@ prefix_rule( rest: vec![PatternToken::Alts(vec!["-c".to_string(), "-l".to_string()])].into(), }, decision: Decision::Allow, + justification: None, })], sh_rules ); @@ -232,6 +320,7 @@ prefix_rule( matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["bash", "-c"]), decision: Decision::Allow, + justification: None, }], }, bash_eval @@ -244,6 +333,7 @@ prefix_rule( matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["sh", "-l"]), decision: Decision::Allow, + justification: None, }], }, sh_eval @@ -277,6 +367,7 @@ prefix_rule( .into(), }, decision: Decision::Allow, + justification: None, })], rules ); @@ -288,6 +379,7 @@ prefix_rule( matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["npm", "i", "--legacy-peer-deps"]), decision: Decision::Allow, + justification: None, }], }, npm_i @@ -303,6 +395,7 @@ prefix_rule( matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["npm", "install", "--no-save"]), decision: Decision::Allow, + justification: None, }], }, npm_install @@ -332,6 +425,7 @@ prefix_rule( matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git", "status"]), decision: Decision::Allow, + justification: None, }], }, match_eval @@ -378,10 +472,12 @@ prefix_rule( RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git"]), decision: Decision::Prompt, + justification: None, }, RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git", "commit"]), decision: Decision::Forbidden, + justification: None, }, ], }, @@ -419,14 +515,17 @@ prefix_rule( RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git"]), decision: Decision::Prompt, + justification: None, }, RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git"]), decision: Decision::Prompt, + justification: None, }, RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git", "commit"]), decision: Decision::Forbidden, + justification: None, }, ], },