From 939ea70a5158d497e0e2f4a5496a4c77903a3d68 Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Mon, 23 Feb 2026 20:34:10 -0800 Subject: [PATCH] feat(core) add source for thread-derived rules --- codex-rs/core/src/exec_policy.rs | 29 ++++-- codex-rs/core/tests/suite/approvals.rs | 4 +- codex-rs/execpolicy/src/amend.rs | 124 +++++++++++++++++++++++-- codex-rs/execpolicy/src/lib.rs | 1 + codex-rs/execpolicy/src/policy.rs | 11 ++- codex-rs/execpolicy/tests/basic.rs | 33 +++++++ 6 files changed, 185 insertions(+), 17 deletions(-) diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 69a7cb933d..0268002db9 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -18,7 +18,7 @@ use codex_execpolicy::NetworkRuleProtocol; use codex_execpolicy::Policy; use codex_execpolicy::PolicyParser; use codex_execpolicy::RuleMatch; -use codex_execpolicy::blocking_append_allow_prefix_rule; +use codex_execpolicy::blocking_append_allow_prefix_rule_with_justification; use codex_execpolicy::blocking_append_network_rule; use codex_protocol::approvals::ExecPolicyAmendment; use codex_protocol::protocol::AskForApproval; @@ -42,6 +42,7 @@ const REJECT_RULES_APPROVAL_REASON: &str = const RULES_DIR_NAME: &str = "rules"; const RULE_EXTENSION: &str = "rules"; const DEFAULT_POLICY_FILE: &str = "default.rules"; +const THREAD_PERSISTED_JUSTIFICATION: &str = "persisted during thread"; static BANNED_PREFIX_SUGGESTIONS: &[&[&str]] = &[ &["python3"], &["python3", "-"], @@ -290,7 +291,13 @@ impl ExecPolicyManager { spawn_blocking({ let policy_path = policy_path.clone(); let prefix = prefix.clone(); - move || blocking_append_allow_prefix_rule(&policy_path, &prefix) + move || { + blocking_append_allow_prefix_rule_with_justification( + &policy_path, + &prefix, + Some(THREAD_PERSISTED_JUSTIFICATION), + ) + } }) .await .map_err(|source| ExecPolicyUpdateError::JoinBlockingTask { source })? @@ -300,7 +307,11 @@ impl ExecPolicyManager { })?; let mut updated_policy = self.current().as_ref().clone(); - updated_policy.add_prefix_rule(&prefix, Decision::Allow)?; + updated_policy.add_prefix_rule_with_justification( + &prefix, + Decision::Allow, + Some(THREAD_PERSISTED_JUSTIFICATION.to_string()), + )?; self.policy.store(Arc::new(updated_policy)); Ok(()) } @@ -1861,19 +1872,23 @@ prefix_rule(pattern=["git"], decision="prompt") &["echo".to_string(), "hello".to_string(), "world".to_string()], &|_| Decision::Allow, ); - assert!(matches!( + assert_eq!( evaluation, Evaluation { decision: Decision::Allow, - .. + matched_rules: vec![RuleMatch::PrefixRuleMatch { + matched_prefix: vec!["echo".to_string(), "hello".to_string()], + decision: Decision::Allow, + justification: Some(THREAD_PERSISTED_JUSTIFICATION.to_string()), + }], } - )); + ); let contents = fs::read_to_string(default_policy_path(codex_home.path())) .expect("policy file should have been created"); assert_eq!( contents, - r#"prefix_rule(pattern=["echo", "hello"], decision="allow") + r#"prefix_rule(pattern=["echo", "hello"], decision="allow", justification="persisted during thread") "# ); } diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 9ca159965f..5c03c4e85b 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -1903,7 +1903,9 @@ async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts let policy_contents = fs::read_to_string(&policy_path)?; assert!( policy_contents - .contains(r#"prefix_rule(pattern=["touch", "allow-prefix.txt"], decision="allow")"#), + .contains( + r#"prefix_rule(pattern=["touch", "allow-prefix.txt"], decision="allow", justification="persisted during thread")"# + ), "unexpected policy contents: {policy_contents}" ); diff --git a/codex-rs/execpolicy/src/amend.rs b/codex-rs/execpolicy/src/amend.rs index 2a6115c2fa..8bef75b868 100644 --- a/codex-rs/execpolicy/src/amend.rs +++ b/codex-rs/execpolicy/src/amend.rs @@ -66,6 +66,16 @@ pub enum AmendError { pub fn blocking_append_allow_prefix_rule( policy_path: &Path, prefix: &[String], +) -> Result<(), AmendError> { + blocking_append_allow_prefix_rule_with_justification(policy_path, prefix, None) +} + +/// Note this thread uses advisory file locking and performs blocking I/O, so it should be used with +/// [`tokio::task::spawn_blocking`] when called from an async context. +pub fn blocking_append_allow_prefix_rule_with_justification( + policy_path: &Path, + prefix: &[String], + justification: Option<&str>, ) -> Result<(), AmendError> { if prefix.is_empty() { return Err(AmendError::EmptyPrefix); @@ -77,8 +87,18 @@ pub fn blocking_append_allow_prefix_rule( .collect::, _>>() .map_err(|source| AmendError::SerializePrefix { source })?; let pattern = format!("[{}]", tokens.join(", ")); - let rule = format!(r#"prefix_rule(pattern={pattern}, decision="allow")"#); - append_rule_line(policy_path, &rule) + let rule = match justification { + Some(justification) => { + let justification = serde_json::to_string(justification) + .map_err(|source| AmendError::SerializePrefix { source })?; + format!( + r#"prefix_rule(pattern={pattern}, decision="allow", justification={justification})"# + ) + } + None => format!(r#"prefix_rule(pattern={pattern}, decision="allow")"#), + }; + let base_rule = format!(r#"prefix_rule(pattern={pattern}, decision="allow")"#); + append_rule_line(policy_path, &rule, Some(&base_rule)) } /// Note this function uses advisory file locking and performs blocking I/O, so it should be used @@ -122,10 +142,14 @@ pub fn blocking_append_network_rule( args.push(format!("justification={justification}")); } let rule = format!("network_rule({})", args.join(", ")); - append_rule_line(policy_path, &rule) + append_rule_line(policy_path, &rule, None) } -fn append_rule_line(policy_path: &Path, rule: &str) -> Result<(), AmendError> { +fn append_rule_line( + policy_path: &Path, + rule: &str, + base_rule: Option<&str>, +) -> Result<(), AmendError> { let dir = policy_path .parent() .ok_or_else(|| AmendError::MissingParent { @@ -141,11 +165,14 @@ fn append_rule_line(policy_path: &Path, rule: &str) -> Result<(), AmendError> { }); } } - - append_locked_line(policy_path, rule) + append_locked_line(policy_path, rule, base_rule) } -fn append_locked_line(policy_path: &Path, line: &str) -> Result<(), AmendError> { +fn append_locked_line( + policy_path: &Path, + line: &str, + base_rule: Option<&str>, +) -> Result<(), AmendError> { let mut file = OpenOptions::new() .create(true) .read(true) @@ -172,7 +199,18 @@ fn append_locked_line(policy_path: &Path, line: &str) -> Result<(), AmendError> source, })?; - if contents.lines().any(|existing| existing == line) { + if contents.lines().any(|existing| { + if existing == line { + return true; + } + let Some(base_rule) = base_rule else { + return false; + }; + existing == base_rule + || existing + .strip_prefix(base_rule.strip_suffix(')').unwrap_or(base_rule)) + .is_some_and(|suffix| suffix.starts_with(", justification=")) + }) { return Ok(()); } @@ -293,6 +331,26 @@ prefix_rule(pattern=["echo", "Hello, world!"], decision="allow") ); } + #[test] + fn appends_rule_with_justification() { + let tmp = tempdir().expect("create temp dir"); + let policy_path = tmp.path().join("rules").join("default.rules"); + + blocking_append_allow_prefix_rule_with_justification( + &policy_path, + &[String::from("echo"), String::from("Hello, world!")], + Some("persisted during thread"), + ) + .expect("append rule"); + + let contents = std::fs::read_to_string(&policy_path).expect("default.rules should exist"); + assert_eq!( + contents, + r#"prefix_rule(pattern=["echo", "Hello, world!"], decision="allow", justification="persisted during thread") +"# + ); + } + #[test] fn appends_prefix_and_network_rules() { let tmp = tempdir().expect("create temp dir"); @@ -318,6 +376,33 @@ network_rule(host="api.github.com", protocol="https", decision="allow", justific ); } + #[test] + fn dedupes_justified_rule_against_unjustified_existing_rule() { + let tmp = tempdir().expect("create temp dir"); + let policy_path = tmp.path().join("rules").join("default.rules"); + std::fs::create_dir_all(policy_path.parent().expect("policy parent")).expect("mkdir"); + std::fs::write( + &policy_path, + r#"prefix_rule(pattern=["python3"], decision="allow") +"#, + ) + .expect("write seed rule"); + + blocking_append_allow_prefix_rule_with_justification( + &policy_path, + &[String::from("python3")], + Some("persisted during thread"), + ) + .expect("append rule"); + + let contents = std::fs::read_to_string(&policy_path).expect("read policy"); + assert_eq!( + contents, + r#"prefix_rule(pattern=["python3"], decision="allow") +"# + ); + } + #[test] fn rejects_wildcard_network_rule_host() { let tmp = tempdir().expect("create temp dir"); @@ -335,4 +420,27 @@ network_rule(host="api.github.com", protocol="https", decision="allow", justific "invalid network rule: invalid rule: network_rule host must be a specific host; wildcards are not allowed" ); } + + #[test] + fn dedupes_unjustified_rule_against_justified_existing_rule() { + let tmp = tempdir().expect("create temp dir"); + let policy_path = tmp.path().join("rules").join("default.rules"); + std::fs::create_dir_all(policy_path.parent().expect("policy parent")).expect("mkdir"); + std::fs::write( + &policy_path, + r#"prefix_rule(pattern=["python3"], decision="allow", justification="persisted during thread") +"#, + ) + .expect("write seed rule"); + + blocking_append_allow_prefix_rule(&policy_path, &[String::from("python3")]) + .expect("append rule"); + + let contents = std::fs::read_to_string(&policy_path).expect("read policy"); + assert_eq!( + contents, + r#"prefix_rule(pattern=["python3"], decision="allow", justification="persisted during thread") +"# + ); + } } diff --git a/codex-rs/execpolicy/src/lib.rs b/codex-rs/execpolicy/src/lib.rs index 8cb89c36e4..ac6bb33d2c 100644 --- a/codex-rs/execpolicy/src/lib.rs +++ b/codex-rs/execpolicy/src/lib.rs @@ -9,6 +9,7 @@ pub mod rule; pub use amend::AmendError; pub use amend::blocking_append_allow_prefix_rule; +pub use amend::blocking_append_allow_prefix_rule_with_justification; pub use amend::blocking_append_network_rule; pub use decision::Decision; pub use error::Error; diff --git a/codex-rs/execpolicy/src/policy.rs b/codex-rs/execpolicy/src/policy.rs index 3102264e39..df6761471b 100644 --- a/codex-rs/execpolicy/src/policy.rs +++ b/codex-rs/execpolicy/src/policy.rs @@ -89,6 +89,15 @@ impl Policy { } pub fn add_prefix_rule(&mut self, prefix: &[String], decision: Decision) -> Result<()> { + self.add_prefix_rule_with_justification(prefix, decision, None) + } + + pub fn add_prefix_rule_with_justification( + &mut self, + prefix: &[String], + decision: Decision, + justification: Option, + ) -> Result<()> { let (first_token, rest) = prefix .split_first() .ok_or_else(|| Error::InvalidPattern("prefix cannot be empty".to_string()))?; @@ -103,7 +112,7 @@ impl Policy { .into(), }, decision, - justification: None, + justification, }); self.rules_by_program.insert(first_token.clone(), rule); diff --git a/codex-rs/execpolicy/tests/basic.rs b/codex-rs/execpolicy/tests/basic.rs index bef845da67..f306b4bac9 100644 --- a/codex-rs/execpolicy/tests/basic.rs +++ b/codex-rs/execpolicy/tests/basic.rs @@ -15,6 +15,7 @@ use codex_execpolicy::PolicyParser; use codex_execpolicy::RuleMatch; use codex_execpolicy::RuleRef; use codex_execpolicy::blocking_append_allow_prefix_rule; +use codex_execpolicy::blocking_append_allow_prefix_rule_with_justification; use codex_execpolicy::rule::PatternToken; use codex_execpolicy::rule::PrefixPattern; use codex_execpolicy::rule::PrefixRule; @@ -139,6 +140,38 @@ fn network_rule_rejects_wildcard_hosts() { assert!(err.to_string().contains("wildcards are not allowed")); } +#[test] +fn append_allow_prefix_rule_with_justification_round_trips() -> Result<()> { + let tmp = tempdir().context("create temp dir")?; + let policy_path = tmp.path().join("rules").join("default.rules"); + let prefix = tokens(&["python3"]); + + blocking_append_allow_prefix_rule_with_justification( + &policy_path, + &prefix, + Some("persisted during thread"), + )?; + + let policy_src = fs::read_to_string(&policy_path).context("read policy")?; + let mut parser = PolicyParser::new(); + parser.parse("default.rules", &policy_src)?; + let policy = parser.build(); + let evaluation = policy.check(&tokens(&["python3", "-V"]), &allow_all); + + assert_eq!( + evaluation, + Evaluation { + decision: Decision::Allow, + matched_rules: vec![RuleMatch::PrefixRuleMatch { + matched_prefix: tokens(&["python3"]), + decision: Decision::Allow, + justification: Some("persisted during thread".to_string()), + }], + } + ); + Ok(()) +} + #[test] fn basic_match() -> Result<()> { let policy_src = r#"