From 43f28cc9f39cd0f21ff51e71e8bf0a2c5c372a6b Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Thu, 20 Nov 2025 17:15:51 -0500 Subject: [PATCH 01/15] Add allow-prefix amendment helper to execpolicy --- codex-rs/Cargo.lock | 1 + codex-rs/execpolicy/Cargo.toml | 1 + codex-rs/execpolicy/src/amend.rs | 143 +++++++++++++++++++++++++++++++ codex-rs/execpolicy/src/lib.rs | 3 + 4 files changed, 148 insertions(+) create mode 100644 codex-rs/execpolicy/src/amend.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 2320afeecb..8e27a1655b 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1281,6 +1281,7 @@ dependencies = [ "serde_json", "shlex", "starlark", + "tempfile", "thiserror 2.0.17", ] diff --git a/codex-rs/execpolicy/Cargo.toml b/codex-rs/execpolicy/Cargo.toml index 77278bb118..890c23faa7 100644 --- a/codex-rs/execpolicy/Cargo.toml +++ b/codex-rs/execpolicy/Cargo.toml @@ -28,3 +28,4 @@ thiserror = { workspace = true } [dev-dependencies] pretty_assertions = { workspace = true } +tempfile = { workspace = true } diff --git a/codex-rs/execpolicy/src/amend.rs b/codex-rs/execpolicy/src/amend.rs new file mode 100644 index 0000000000..614e13d076 --- /dev/null +++ b/codex-rs/execpolicy/src/amend.rs @@ -0,0 +1,143 @@ +use std::fs::OpenOptions; +use std::io::Write; +use std::path::Path; +use std::path::PathBuf; + +use serde_json; +use thiserror::Error; + +#[derive(Debug, Error)] +pub enum AmendError { + #[error("prefix rule requires at least one token")] + EmptyPrefix, + #[error("policy path has no parent: {path}")] + MissingParent { path: PathBuf }, + #[error("failed to create policy directory {dir}: {source}")] + CreatePolicyDir { + dir: PathBuf, + source: std::io::Error, + }, + #[error("failed to format prefix token {token}: {source}")] + SerializeToken { + token: String, + source: serde_json::Error, + }, + #[error("failed to open policy file {path}: {source}")] + OpenPolicyFile { + path: PathBuf, + source: std::io::Error, + }, + #[error("failed to write to policy file {path}: {source}")] + WritePolicyFile { + path: PathBuf, + source: std::io::Error, + }, + #[error("failed to read metadata for policy file {path}: {source}")] + PolicyMetadata { + path: PathBuf, + source: std::io::Error, + }, +} + +pub fn append_allow_prefix_rule(policy_path: &Path, prefix: &[String]) -> Result<(), AmendError> { + if prefix.is_empty() { + return Err(AmendError::EmptyPrefix); + } + + let tokens: Vec = prefix + .iter() + .map(|token| { + serde_json::to_string(token).map_err(|source| AmendError::SerializeToken { + token: token.clone(), + source, + }) + }) + .collect::>()?; + let pattern = tokens.join(", "); + let rule = format!("prefix_rule(pattern=[{pattern}], decision=\"allow\")\n"); + + let dir = policy_path + .parent() + .ok_or_else(|| AmendError::MissingParent { + path: policy_path.to_path_buf(), + })?; + match std::fs::create_dir(dir) { + Ok(()) => {} + Err(ref source) if source.kind() == std::io::ErrorKind::AlreadyExists => {} + Err(source) => { + return Err(AmendError::CreatePolicyDir { + dir: dir.to_path_buf(), + source, + }); + } + } + let mut file = OpenOptions::new() + .create(true) + .append(true) + .open(policy_path) + .map_err(|source| AmendError::OpenPolicyFile { + path: policy_path.to_path_buf(), + source, + })?; + let needs_newline = file + .metadata() + .map(|metadata| metadata.len() > 0) + .map_err(|source| AmendError::PolicyMetadata { + path: policy_path.to_path_buf(), + source, + })?; + let final_rule = if needs_newline { + format!("\n{rule}") + } else { + rule + }; + + file.write_all(final_rule.as_bytes()) + .map_err(|source| AmendError::WritePolicyFile { + path: policy_path.to_path_buf(), + source, + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + use tempfile::tempdir; + + #[test] + fn appends_rule_and_creates_directories() { + let tmp = tempdir().expect("create temp dir"); + let policy_path = tmp.path().join("policy").join("default.codexpolicy"); + + append_allow_prefix_rule(&policy_path, &[String::from("echo"), String::from("Hello, world!")]) + .expect("append rule"); + + let contents = + std::fs::read_to_string(&policy_path).expect("default.codexpolicy should exist"); + assert_eq!( + contents, + "prefix_rule(pattern=[\"echo\", \"Hello, world!\"], decision=\"allow\")\n" + ); + } + + #[test] + fn separates_rules_with_newlines_when_appending() { + let tmp = tempdir().expect("create temp dir"); + let policy_path = tmp.path().join("policy").join("default.codexpolicy"); + std::fs::create_dir_all(policy_path.parent().unwrap()).expect("create policy dir"); + std::fs::write( + &policy_path, + "prefix_rule(pattern=[\"ls\"], decision=\"allow\")\n", + ) + .expect("write seed rule"); + + append_allow_prefix_rule(&policy_path, &[String::from("echo"), String::from("Hello, world!")]).expect("append rule"); + + let contents = std::fs::read_to_string(&policy_path).expect("read policy"); + assert_eq!( + contents, + "prefix_rule(pattern=[\"ls\"], decision=\"allow\")\n\nprefix_rule(pattern=[\"echo\", \"Hello, world!\"], decision=\"allow\")\n" + ); + } +} diff --git a/codex-rs/execpolicy/src/lib.rs b/codex-rs/execpolicy/src/lib.rs index b459caea16..f75fd8fa5e 100644 --- a/codex-rs/execpolicy/src/lib.rs +++ b/codex-rs/execpolicy/src/lib.rs @@ -1,3 +1,4 @@ +pub mod amend; pub mod decision; pub mod error; pub mod execpolicycheck; @@ -5,6 +6,8 @@ pub mod parser; pub mod policy; pub mod rule; +pub use amend::AmendError; +pub use amend::append_allow_prefix_rule; pub use decision::Decision; pub use error::Error; pub use error::Result; From 73ecd7cd360a755bc8d803adf1ca2e77ae51632d Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Thu, 20 Nov 2025 17:32:48 -0500 Subject: [PATCH 02/15] fmt --- codex-rs/execpolicy/src/amend.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/codex-rs/execpolicy/src/amend.rs b/codex-rs/execpolicy/src/amend.rs index 614e13d076..8c1a999561 100644 --- a/codex-rs/execpolicy/src/amend.rs +++ b/codex-rs/execpolicy/src/amend.rs @@ -110,8 +110,11 @@ mod tests { let tmp = tempdir().expect("create temp dir"); let policy_path = tmp.path().join("policy").join("default.codexpolicy"); - append_allow_prefix_rule(&policy_path, &[String::from("echo"), String::from("Hello, world!")]) - .expect("append rule"); + append_allow_prefix_rule( + &policy_path, + &[String::from("echo"), String::from("Hello, world!")], + ) + .expect("append rule"); let contents = std::fs::read_to_string(&policy_path).expect("default.codexpolicy should exist"); @@ -132,7 +135,11 @@ mod tests { ) .expect("write seed rule"); - append_allow_prefix_rule(&policy_path, &[String::from("echo"), String::from("Hello, world!")]).expect("append rule"); + append_allow_prefix_rule( + &policy_path, + &[String::from("echo"), String::from("Hello, world!")], + ) + .expect("append rule"); let contents = std::fs::read_to_string(&policy_path).expect("read policy"); assert_eq!( From 154ab8be07620dc66c1b2a5f5cc7f01f192a4517 Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Thu, 20 Nov 2025 19:25:34 -0500 Subject: [PATCH 03/15] feat: adding prefix rules to existing policy --- codex-rs/execpolicy/src/policy.rs | 27 ++++++++++++++++++ codex-rs/execpolicy/tests/basic.rs | 45 ++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/codex-rs/execpolicy/src/policy.rs b/codex-rs/execpolicy/src/policy.rs index e048fce1ff..10858c9fad 100644 --- a/codex-rs/execpolicy/src/policy.rs +++ b/codex-rs/execpolicy/src/policy.rs @@ -1,9 +1,15 @@ use crate::decision::Decision; +use crate::error::Error; +use crate::error::Result; +use crate::rule::PatternToken; +use crate::rule::PrefixPattern; +use crate::rule::PrefixRule; use crate::rule::RuleMatch; use crate::rule::RuleRef; use multimap::MultiMap; use serde::Deserialize; use serde::Serialize; +use std::sync::Arc; #[derive(Clone, Debug)] pub struct Policy { @@ -23,6 +29,27 @@ impl Policy { &self.rules_by_program } + pub fn add_prefix_rule(&mut self, prefix: &[String], decision: Decision) -> Result<()> { + let (first_token, rest) = prefix + .split_first() + .ok_or_else(|| Error::InvalidPattern("prefix cannot be empty".to_string()))?; + + let rule: RuleRef = Arc::new(PrefixRule { + pattern: PrefixPattern { + first: Arc::from(first_token.as_str()), + rest: rest + .iter() + .map(|token| PatternToken::Single(token.clone())) + .collect::>() + .into(), + }, + decision, + }); + + self.rules_by_program.insert(first_token.clone(), rule); + Ok(()) + } + pub fn check(&self, cmd: &[String]) -> Evaluation { let rules = match cmd.first() { Some(first) => match self.rules_by_program.get_vec(first) { diff --git a/codex-rs/execpolicy/tests/basic.rs b/codex-rs/execpolicy/tests/basic.rs index e4189caa21..9cac9fb701 100644 --- a/codex-rs/execpolicy/tests/basic.rs +++ b/codex-rs/execpolicy/tests/basic.rs @@ -2,7 +2,9 @@ use std::any::Any; use std::sync::Arc; use codex_execpolicy::Decision; +use codex_execpolicy::Error; use codex_execpolicy::Evaluation; +use codex_execpolicy::Policy; use codex_execpolicy::PolicyParser; use codex_execpolicy::RuleMatch; use codex_execpolicy::RuleRef; @@ -60,6 +62,49 @@ prefix_rule( ); } +#[test] +fn add_prefix_rule_extends_policy() { + let mut policy = Policy::empty(); + policy + .add_prefix_rule(&tokens(&["ls", "-l"]), Decision::Prompt) + .expect("add prefix rule"); + + let rules = rule_snapshots(policy.rules().get_vec("ls").expect("ls rules")); + assert_eq!( + vec![RuleSnapshot::Prefix(PrefixRule { + pattern: PrefixPattern { + first: Arc::from("ls"), + rest: vec![PatternToken::Single(String::from("-l"))].into(), + }, + decision: Decision::Prompt, + })], + rules + ); + + let evaluation = policy.check(&tokens(&["ls", "-l", "/tmp"])); + assert_eq!( + Evaluation::Match { + decision: Decision::Prompt, + matched_rules: vec![RuleMatch::PrefixRuleMatch { + matched_prefix: tokens(&["ls", "-l"]), + decision: Decision::Prompt, + }], + }, + evaluation + ); +} + +#[test] +fn add_prefix_rule_rejects_empty_prefix() { + let mut policy = Policy::empty(); + let result = policy.add_prefix_rule(&[], Decision::Allow); + + assert!(matches!( + result, + Err(Error::InvalidPattern(message)) if message == "prefix cannot be empty" + )); +} + #[test] fn parses_multiple_policy_files() { let first_policy = r#" From b544a5b5d1968ace8735b5cee29e89fbded1ef44 Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Thu, 20 Nov 2025 20:30:59 -0500 Subject: [PATCH 04/15] using json-serialize --- codex-rs/execpolicy/src/amend.rs | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/codex-rs/execpolicy/src/amend.rs b/codex-rs/execpolicy/src/amend.rs index 8c1a999561..91bb67d7c4 100644 --- a/codex-rs/execpolicy/src/amend.rs +++ b/codex-rs/execpolicy/src/amend.rs @@ -17,11 +17,8 @@ pub enum AmendError { dir: PathBuf, source: std::io::Error, }, - #[error("failed to format prefix token {token}: {source}")] - SerializeToken { - token: String, - source: serde_json::Error, - }, + #[error("failed to format prefix tokens: {source}")] + SerializePrefix { source: serde_json::Error }, #[error("failed to open policy file {path}: {source}")] OpenPolicyFile { path: PathBuf, @@ -44,17 +41,9 @@ pub fn append_allow_prefix_rule(policy_path: &Path, prefix: &[String]) -> Result return Err(AmendError::EmptyPrefix); } - let tokens: Vec = prefix - .iter() - .map(|token| { - serde_json::to_string(token).map_err(|source| AmendError::SerializeToken { - token: token.clone(), - source, - }) - }) - .collect::>()?; - let pattern = tokens.join(", "); - let rule = format!("prefix_rule(pattern=[{pattern}], decision=\"allow\")\n"); + let pattern = + serde_json::to_string(prefix).map_err(|source| AmendError::SerializePrefix { source })?; + let rule = format!("prefix_rule(pattern={pattern}, decision=\"allow\")\n"); let dir = policy_path .parent() @@ -120,7 +109,7 @@ mod tests { std::fs::read_to_string(&policy_path).expect("default.codexpolicy should exist"); assert_eq!( contents, - "prefix_rule(pattern=[\"echo\", \"Hello, world!\"], decision=\"allow\")\n" + "prefix_rule(pattern=[\"echo\",\"Hello, world!\"], decision=\"allow\")\n" ); } @@ -144,7 +133,7 @@ mod tests { let contents = std::fs::read_to_string(&policy_path).expect("read policy"); assert_eq!( contents, - "prefix_rule(pattern=[\"ls\"], decision=\"allow\")\n\nprefix_rule(pattern=[\"echo\", \"Hello, world!\"], decision=\"allow\")\n" + "prefix_rule(pattern=[\"ls\"], decision=\"allow\")\n\nprefix_rule(pattern=[\"echo\",\"Hello, world!\"], decision=\"allow\")\n" ); } } From e896cf547902b82fdb642736dd1ac927e3052f16 Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Fri, 21 Nov 2025 12:13:40 -0500 Subject: [PATCH 05/15] feat: advisory locks in amend --- codex-rs/execpolicy/src/amend.rs | 115 ++++++++++++++++++++++++++----- 1 file changed, 99 insertions(+), 16 deletions(-) diff --git a/codex-rs/execpolicy/src/amend.rs b/codex-rs/execpolicy/src/amend.rs index 91bb67d7c4..faaf75aa72 100644 --- a/codex-rs/execpolicy/src/amend.rs +++ b/codex-rs/execpolicy/src/amend.rs @@ -1,4 +1,7 @@ use std::fs::OpenOptions; +use std::io::Read; +use std::io::Seek; +use std::io::SeekFrom; use std::io::Write; use std::path::Path; use std::path::PathBuf; @@ -29,6 +32,26 @@ pub enum AmendError { path: PathBuf, source: std::io::Error, }, + #[error("failed to lock policy file {path}: {source}")] + LockPolicyFile { + path: PathBuf, + source: std::io::Error, + }, + #[error("failed to unlock policy file {path}: {source}")] + UnlockPolicyFile { + path: PathBuf, + source: std::io::Error, + }, + #[error("failed to seek policy file {path}: {source}")] + SeekPolicyFile { + path: PathBuf, + source: std::io::Error, + }, + #[error("failed to read policy file {path}: {source}")] + ReadPolicyFile { + path: PathBuf, + source: std::io::Error, + }, #[error("failed to read metadata for policy file {path}: {source}")] PolicyMetadata { path: PathBuf, @@ -43,7 +66,7 @@ pub fn append_allow_prefix_rule(policy_path: &Path, prefix: &[String]) -> Result let pattern = serde_json::to_string(prefix).map_err(|source| AmendError::SerializePrefix { source })?; - let rule = format!("prefix_rule(pattern={pattern}, decision=\"allow\")\n"); + let rule = format!("prefix_rule(pattern={pattern}, decision=\"allow\")"); let dir = policy_path .parent() @@ -60,30 +83,66 @@ pub fn append_allow_prefix_rule(policy_path: &Path, prefix: &[String]) -> Result }); } } + append_locked_line(policy_path, &rule) +} + +fn append_locked_line(policy_path: &Path, line: &str) -> Result<(), AmendError> { + let policy_path = policy_path.to_path_buf(); let mut file = OpenOptions::new() .create(true) + .read(true) .append(true) - .open(policy_path) + .open(&policy_path) .map_err(|source| AmendError::OpenPolicyFile { - path: policy_path.to_path_buf(), + path: policy_path.clone(), + source, + })?; + file.lock() + .map_err(|source| AmendError::LockPolicyFile { + path: policy_path.clone(), source, })?; - let needs_newline = file + + let len = file .metadata() - .map(|metadata| metadata.len() > 0) .map_err(|source| AmendError::PolicyMetadata { - path: policy_path.to_path_buf(), + path: policy_path.clone(), source, - })?; - let final_rule = if needs_newline { - format!("\n{rule}") - } else { - rule - }; + })? + .len(); + + if len > 0 { + file.seek(SeekFrom::End(-1)) + .map_err(|source| AmendError::SeekPolicyFile { + path: policy_path.clone(), + source, + })?; + let mut last = [0; 1]; + file.read_exact(&mut last) + .map_err(|source| AmendError::ReadPolicyFile { + path: policy_path.clone(), + source, + })?; + + if last[0] != b'\n' { + file.write_all(b"\n") + .map_err(|source| AmendError::WritePolicyFile { + path: policy_path.clone(), + source, + })?; + } + } - file.write_all(final_rule.as_bytes()) + file.write_all(line.as_bytes()) + .and_then(|()| file.write_all(b"\n")) .map_err(|source| AmendError::WritePolicyFile { - path: policy_path.to_path_buf(), + path: policy_path.clone(), + source, + })?; + + file.unlock() + .map_err(|source| AmendError::UnlockPolicyFile { + path: policy_path, source, }) } @@ -114,7 +173,7 @@ mod tests { } #[test] - fn separates_rules_with_newlines_when_appending() { + fn appends_rule_without_duplicate_newline() { let tmp = tempdir().expect("create temp dir"); let policy_path = tmp.path().join("policy").join("default.codexpolicy"); std::fs::create_dir_all(policy_path.parent().unwrap()).expect("create policy dir"); @@ -133,7 +192,31 @@ mod tests { let contents = std::fs::read_to_string(&policy_path).expect("read policy"); assert_eq!( contents, - "prefix_rule(pattern=[\"ls\"], decision=\"allow\")\n\nprefix_rule(pattern=[\"echo\",\"Hello, world!\"], decision=\"allow\")\n" + "prefix_rule(pattern=[\"ls\"], decision=\"allow\")\nprefix_rule(pattern=[\"echo\",\"Hello, world!\"], decision=\"allow\")\n" + ); + } + + #[test] + fn inserts_newline_when_missing_before_append() { + let tmp = tempdir().expect("create temp dir"); + let policy_path = tmp.path().join("policy").join("default.codexpolicy"); + std::fs::create_dir_all(policy_path.parent().unwrap()).expect("create policy dir"); + std::fs::write( + &policy_path, + "prefix_rule(pattern=[\"ls\"], decision=\"allow\")", + ) + .expect("write seed rule without newline"); + + append_allow_prefix_rule( + &policy_path, + &[String::from("echo"), String::from("Hello, world!")], + ) + .expect("append rule"); + + let contents = std::fs::read_to_string(&policy_path).expect("read policy"); + assert_eq!( + contents, + "prefix_rule(pattern=[\"ls\"], decision=\"allow\")\nprefix_rule(pattern=[\"echo\",\"Hello, world!\"], decision=\"allow\")\n" ); } } From 71b53b2aa1f1a3539150d19964d970c139e7ce99 Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Fri, 21 Nov 2025 12:14:12 -0500 Subject: [PATCH 06/15] fmt --- codex-rs/execpolicy/src/amend.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/codex-rs/execpolicy/src/amend.rs b/codex-rs/execpolicy/src/amend.rs index faaf75aa72..29a7e3daa3 100644 --- a/codex-rs/execpolicy/src/amend.rs +++ b/codex-rs/execpolicy/src/amend.rs @@ -97,11 +97,10 @@ fn append_locked_line(policy_path: &Path, line: &str) -> Result<(), AmendError> path: policy_path.clone(), source, })?; - file.lock() - .map_err(|source| AmendError::LockPolicyFile { - path: policy_path.clone(), - source, - })?; + file.lock().map_err(|source| AmendError::LockPolicyFile { + path: policy_path.clone(), + source, + })?; let len = file .metadata() From ac15fcbe2a5b9a26903cbcabe92ece20d71134a0 Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Tue, 2 Dec 2025 11:39:09 -0500 Subject: [PATCH 07/15] one write_all --- codex-rs/execpolicy/src/amend.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/codex-rs/execpolicy/src/amend.rs b/codex-rs/execpolicy/src/amend.rs index 29a7e3daa3..c489145084 100644 --- a/codex-rs/execpolicy/src/amend.rs +++ b/codex-rs/execpolicy/src/amend.rs @@ -132,8 +132,7 @@ fn append_locked_line(policy_path: &Path, line: &str) -> Result<(), AmendError> } } - file.write_all(line.as_bytes()) - .and_then(|()| file.write_all(b"\n")) + file.write_all(format!("{line}\n").as_bytes()) .map_err(|source| AmendError::WritePolicyFile { path: policy_path.clone(), source, From ade87ae7461410cba2b2e84cd83b0c9411acb37c Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Tue, 2 Dec 2025 11:44:24 -0500 Subject: [PATCH 08/15] using r# --- codex-rs/execpolicy/src/amend.rs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/codex-rs/execpolicy/src/amend.rs b/codex-rs/execpolicy/src/amend.rs index c489145084..7d8517f198 100644 --- a/codex-rs/execpolicy/src/amend.rs +++ b/codex-rs/execpolicy/src/amend.rs @@ -66,7 +66,7 @@ pub fn append_allow_prefix_rule(policy_path: &Path, prefix: &[String]) -> Result let pattern = serde_json::to_string(prefix).map_err(|source| AmendError::SerializePrefix { source })?; - let rule = format!("prefix_rule(pattern={pattern}, decision=\"allow\")"); + let rule = format!(r#"prefix_rule(pattern={pattern}, decision="allow")"#); let dir = policy_path .parent() @@ -166,7 +166,8 @@ mod tests { std::fs::read_to_string(&policy_path).expect("default.codexpolicy should exist"); assert_eq!( contents, - "prefix_rule(pattern=[\"echo\",\"Hello, world!\"], decision=\"allow\")\n" + r#"prefix_rule(pattern=["echo","Hello, world!"], decision="allow") +"# ); } @@ -177,7 +178,8 @@ mod tests { std::fs::create_dir_all(policy_path.parent().unwrap()).expect("create policy dir"); std::fs::write( &policy_path, - "prefix_rule(pattern=[\"ls\"], decision=\"allow\")\n", + r#"prefix_rule(pattern=["ls"], decision="allow") +"#, ) .expect("write seed rule"); @@ -190,7 +192,9 @@ mod tests { let contents = std::fs::read_to_string(&policy_path).expect("read policy"); assert_eq!( contents, - "prefix_rule(pattern=[\"ls\"], decision=\"allow\")\nprefix_rule(pattern=[\"echo\",\"Hello, world!\"], decision=\"allow\")\n" + r#"prefix_rule(pattern=["ls"], decision="allow") +prefix_rule(pattern=["echo","Hello, world!"], decision="allow") +"# ); } @@ -201,7 +205,7 @@ mod tests { std::fs::create_dir_all(policy_path.parent().unwrap()).expect("create policy dir"); std::fs::write( &policy_path, - "prefix_rule(pattern=[\"ls\"], decision=\"allow\")", + r#"prefix_rule(pattern=["ls"], decision="allow")"#, ) .expect("write seed rule without newline"); @@ -214,7 +218,9 @@ mod tests { let contents = std::fs::read_to_string(&policy_path).expect("read policy"); assert_eq!( contents, - "prefix_rule(pattern=[\"ls\"], decision=\"allow\")\nprefix_rule(pattern=[\"echo\",\"Hello, world!\"], decision=\"allow\")\n" + r#"prefix_rule(pattern=["ls"], decision="allow") +prefix_rule(pattern=["echo","Hello, world!"], decision="allow") +"# ); } } From a3c3db1c92ab61b152473e42e35f44955e4b0d6d Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Tue, 2 Dec 2025 11:46:52 -0500 Subject: [PATCH 09/15] removing explicit file.unlock() --- codex-rs/execpolicy/src/amend.rs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/codex-rs/execpolicy/src/amend.rs b/codex-rs/execpolicy/src/amend.rs index 7d8517f198..ec905acb25 100644 --- a/codex-rs/execpolicy/src/amend.rs +++ b/codex-rs/execpolicy/src/amend.rs @@ -37,11 +37,6 @@ pub enum AmendError { path: PathBuf, source: std::io::Error, }, - #[error("failed to unlock policy file {path}: {source}")] - UnlockPolicyFile { - path: PathBuf, - source: std::io::Error, - }, #[error("failed to seek policy file {path}: {source}")] SeekPolicyFile { path: PathBuf, @@ -138,11 +133,7 @@ fn append_locked_line(policy_path: &Path, line: &str) -> Result<(), AmendError> source, })?; - file.unlock() - .map_err(|source| AmendError::UnlockPolicyFile { - path: policy_path, - source, - }) + Ok(()) } #[cfg(test)] From 22b6e38430a88a9b69224c6b638d25de2c0b66c0 Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Tue, 2 Dec 2025 11:49:55 -0500 Subject: [PATCH 10/15] deferring to_path_buf() construction --- codex-rs/execpolicy/src/amend.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/codex-rs/execpolicy/src/amend.rs b/codex-rs/execpolicy/src/amend.rs index ec905acb25..27f2ab0d7c 100644 --- a/codex-rs/execpolicy/src/amend.rs +++ b/codex-rs/execpolicy/src/amend.rs @@ -82,25 +82,24 @@ pub fn append_allow_prefix_rule(policy_path: &Path, prefix: &[String]) -> Result } fn append_locked_line(policy_path: &Path, line: &str) -> Result<(), AmendError> { - let policy_path = policy_path.to_path_buf(); let mut file = OpenOptions::new() .create(true) .read(true) .append(true) - .open(&policy_path) + .open(policy_path) .map_err(|source| AmendError::OpenPolicyFile { - path: policy_path.clone(), + path: policy_path.to_path_buf(), source, })?; file.lock().map_err(|source| AmendError::LockPolicyFile { - path: policy_path.clone(), + path: policy_path.to_path_buf(), source, })?; let len = file .metadata() .map_err(|source| AmendError::PolicyMetadata { - path: policy_path.clone(), + path: policy_path.to_path_buf(), source, })? .len(); @@ -108,20 +107,20 @@ fn append_locked_line(policy_path: &Path, line: &str) -> Result<(), AmendError> if len > 0 { file.seek(SeekFrom::End(-1)) .map_err(|source| AmendError::SeekPolicyFile { - path: policy_path.clone(), + path: policy_path.to_path_buf(), source, })?; let mut last = [0; 1]; file.read_exact(&mut last) .map_err(|source| AmendError::ReadPolicyFile { - path: policy_path.clone(), + path: policy_path.to_path_buf(), source, })?; if last[0] != b'\n' { file.write_all(b"\n") .map_err(|source| AmendError::WritePolicyFile { - path: policy_path.clone(), + path: policy_path.to_path_buf(), source, })?; } @@ -129,7 +128,7 @@ fn append_locked_line(policy_path: &Path, line: &str) -> Result<(), AmendError> file.write_all(format!("{line}\n").as_bytes()) .map_err(|source| AmendError::WritePolicyFile { - path: policy_path.clone(), + path: policy_path.to_path_buf(), source, })?; From dfb4cb0416525d72f8e173b6a72fd1722445005e Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Tue, 2 Dec 2025 11:52:22 -0500 Subject: [PATCH 11/15] adding comment --- codex-rs/execpolicy/src/amend.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/codex-rs/execpolicy/src/amend.rs b/codex-rs/execpolicy/src/amend.rs index 27f2ab0d7c..d790e5b8c6 100644 --- a/codex-rs/execpolicy/src/amend.rs +++ b/codex-rs/execpolicy/src/amend.rs @@ -104,6 +104,7 @@ fn append_locked_line(policy_path: &Path, line: &str) -> Result<(), AmendError> })? .len(); + // Ensure file ends in a newline before appending. if len > 0 { file.seek(SeekFrom::End(-1)) .map_err(|source| AmendError::SeekPolicyFile { From 8a9a47519878c0c4d0692e96a95a24ce41d98289 Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Tue, 2 Dec 2025 11:55:03 -0500 Subject: [PATCH 12/15] function rename + docstring --- codex-rs/execpolicy/src/amend.rs | 13 +++++++++---- codex-rs/execpolicy/src/lib.rs | 2 +- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/codex-rs/execpolicy/src/amend.rs b/codex-rs/execpolicy/src/amend.rs index d790e5b8c6..1613a057dd 100644 --- a/codex-rs/execpolicy/src/amend.rs +++ b/codex-rs/execpolicy/src/amend.rs @@ -54,7 +54,12 @@ pub enum AmendError { }, } -pub fn append_allow_prefix_rule(policy_path: &Path, prefix: &[String]) -> Result<(), AmendError> { +/// 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( + policy_path: &Path, + prefix: &[String], +) -> Result<(), AmendError> { if prefix.is_empty() { return Err(AmendError::EmptyPrefix); } @@ -147,7 +152,7 @@ mod tests { let tmp = tempdir().expect("create temp dir"); let policy_path = tmp.path().join("policy").join("default.codexpolicy"); - append_allow_prefix_rule( + blocking_append_allow_prefix_rule( &policy_path, &[String::from("echo"), String::from("Hello, world!")], ) @@ -174,7 +179,7 @@ mod tests { ) .expect("write seed rule"); - append_allow_prefix_rule( + blocking_append_allow_prefix_rule( &policy_path, &[String::from("echo"), String::from("Hello, world!")], ) @@ -200,7 +205,7 @@ prefix_rule(pattern=["echo","Hello, world!"], decision="allow") ) .expect("write seed rule without newline"); - append_allow_prefix_rule( + blocking_append_allow_prefix_rule( &policy_path, &[String::from("echo"), String::from("Hello, world!")], ) diff --git a/codex-rs/execpolicy/src/lib.rs b/codex-rs/execpolicy/src/lib.rs index f75fd8fa5e..31062f1cb6 100644 --- a/codex-rs/execpolicy/src/lib.rs +++ b/codex-rs/execpolicy/src/lib.rs @@ -7,7 +7,7 @@ pub mod policy; pub mod rule; pub use amend::AmendError; -pub use amend::append_allow_prefix_rule; +pub use amend::blocking_append_allow_prefix_rule; pub use decision::Decision; pub use error::Error; pub use error::Result; From 6e4a66b8ef1ed0fc212595dd3c76be7cfbb9ecae Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Tue, 2 Dec 2025 13:03:59 -0500 Subject: [PATCH 13/15] tests now output -> Result<()> --- codex-rs/execpolicy/tests/basic.rs | 80 +++++++++++++++--------------- 1 file changed, 39 insertions(+), 41 deletions(-) diff --git a/codex-rs/execpolicy/tests/basic.rs b/codex-rs/execpolicy/tests/basic.rs index 9cac9fb701..41328c8252 100644 --- a/codex-rs/execpolicy/tests/basic.rs +++ b/codex-rs/execpolicy/tests/basic.rs @@ -1,6 +1,8 @@ use std::any::Any; use std::sync::Arc; +use anyhow::Context; +use anyhow::Result; use codex_execpolicy::Decision; use codex_execpolicy::Error; use codex_execpolicy::Evaluation; @@ -37,16 +39,14 @@ fn rule_snapshots(rules: &[RuleRef]) -> Vec { } #[test] -fn basic_match() { +fn basic_match() -> Result<()> { let policy_src = r#" prefix_rule( pattern = ["git", "status"], ) "#; let mut parser = PolicyParser::new(); - parser - .parse("test.codexpolicy", policy_src) - .expect("parse policy"); + parser.parse("test.codexpolicy", policy_src)?; let policy = parser.build(); let cmd = tokens(&["git", "status"]); let evaluation = policy.check(&cmd); @@ -60,16 +60,15 @@ prefix_rule( }, evaluation ); + Ok(()) } #[test] -fn add_prefix_rule_extends_policy() { +fn add_prefix_rule_extends_policy() -> Result<()> { let mut policy = Policy::empty(); - policy - .add_prefix_rule(&tokens(&["ls", "-l"]), Decision::Prompt) - .expect("add prefix rule"); + policy.add_prefix_rule(&tokens(&["ls", "-l"]), Decision::Prompt)?; - let rules = rule_snapshots(policy.rules().get_vec("ls").expect("ls rules")); + let rules = rule_snapshots(policy.rules().get_vec("ls").context("missing ls rules")?); assert_eq!( vec![RuleSnapshot::Prefix(PrefixRule { pattern: PrefixPattern { @@ -92,10 +91,11 @@ fn add_prefix_rule_extends_policy() { }, evaluation ); + Ok(()) } #[test] -fn add_prefix_rule_rejects_empty_prefix() { +fn add_prefix_rule_rejects_empty_prefix() -> Result<()> { let mut policy = Policy::empty(); let result = policy.add_prefix_rule(&[], Decision::Allow); @@ -103,10 +103,11 @@ fn add_prefix_rule_rejects_empty_prefix() { result, Err(Error::InvalidPattern(message)) if message == "prefix cannot be empty" )); + Ok(()) } #[test] -fn parses_multiple_policy_files() { +fn parses_multiple_policy_files() -> Result<()> { let first_policy = r#" prefix_rule( pattern = ["git"], @@ -120,15 +121,11 @@ prefix_rule( ) "#; let mut parser = PolicyParser::new(); - parser - .parse("first.codexpolicy", first_policy) - .expect("parse policy"); - parser - .parse("second.codexpolicy", second_policy) - .expect("parse policy"); + parser.parse("first.codexpolicy", first_policy)?; + parser.parse("second.codexpolicy", second_policy)?; let policy = parser.build(); - let git_rules = rule_snapshots(policy.rules().get_vec("git").expect("git rules")); + let git_rules = rule_snapshots(policy.rules().get_vec("git").context("missing git rules")?); assert_eq!( vec![ RuleSnapshot::Prefix(PrefixRule { @@ -178,23 +175,27 @@ prefix_rule( }, commit_eval ); + Ok(()) } #[test] -fn only_first_token_alias_expands_to_multiple_rules() { +fn only_first_token_alias_expands_to_multiple_rules() -> Result<()> { let policy_src = r#" prefix_rule( pattern = [["bash", "sh"], ["-c", "-l"]], ) "#; let mut parser = PolicyParser::new(); - parser - .parse("test.codexpolicy", policy_src) - .expect("parse policy"); + parser.parse("test.codexpolicy", policy_src)?; let policy = parser.build(); - let bash_rules = rule_snapshots(policy.rules().get_vec("bash").expect("bash rules")); - let sh_rules = rule_snapshots(policy.rules().get_vec("sh").expect("sh rules")); + let bash_rules = rule_snapshots( + policy + .rules() + .get_vec("bash") + .context("missing bash rules")?, + ); + let sh_rules = rule_snapshots(policy.rules().get_vec("sh").context("missing sh rules")?); assert_eq!( vec![RuleSnapshot::Prefix(PrefixRule { pattern: PrefixPattern { @@ -239,22 +240,21 @@ prefix_rule( }, sh_eval ); + Ok(()) } #[test] -fn tail_aliases_are_not_cartesian_expanded() { +fn tail_aliases_are_not_cartesian_expanded() -> Result<()> { let policy_src = r#" prefix_rule( pattern = ["npm", ["i", "install"], ["--legacy-peer-deps", "--no-save"]], ) "#; let mut parser = PolicyParser::new(); - parser - .parse("test.codexpolicy", policy_src) - .expect("parse policy"); + parser.parse("test.codexpolicy", policy_src)?; let policy = parser.build(); - let rules = rule_snapshots(policy.rules().get_vec("npm").expect("npm rules")); + let rules = rule_snapshots(policy.rules().get_vec("npm").context("missing npm rules")?); assert_eq!( vec![RuleSnapshot::Prefix(PrefixRule { pattern: PrefixPattern { @@ -296,10 +296,11 @@ prefix_rule( }, npm_install ); + Ok(()) } #[test] -fn match_and_not_match_examples_are_enforced() { +fn match_and_not_match_examples_are_enforced() -> Result<()> { let policy_src = r#" prefix_rule( pattern = ["git", "status"], @@ -311,9 +312,7 @@ prefix_rule( ) "#; let mut parser = PolicyParser::new(); - parser - .parse("test.codexpolicy", policy_src) - .expect("parse policy"); + parser.parse("test.codexpolicy", policy_src)?; let policy = parser.build(); let match_eval = policy.check(&tokens(&["git", "status"])); assert_eq!( @@ -334,10 +333,11 @@ prefix_rule( "status", ])); assert_eq!(Evaluation::NoMatch {}, no_match_eval); + Ok(()) } #[test] -fn strictest_decision_wins_across_matches() { +fn strictest_decision_wins_across_matches() -> Result<()> { let policy_src = r#" prefix_rule( pattern = ["git"], @@ -349,9 +349,7 @@ prefix_rule( ) "#; let mut parser = PolicyParser::new(); - parser - .parse("test.codexpolicy", policy_src) - .expect("parse policy"); + parser.parse("test.codexpolicy", policy_src)?; let policy = parser.build(); let commit = policy.check(&tokens(&["git", "commit", "-m", "hi"])); @@ -371,10 +369,11 @@ prefix_rule( }, commit ); + Ok(()) } #[test] -fn strictest_decision_across_multiple_commands() { +fn strictest_decision_across_multiple_commands() -> Result<()> { let policy_src = r#" prefix_rule( pattern = ["git"], @@ -386,9 +385,7 @@ prefix_rule( ) "#; let mut parser = PolicyParser::new(); - parser - .parse("test.codexpolicy", policy_src) - .expect("parse policy"); + parser.parse("test.codexpolicy", policy_src)?; let policy = parser.build(); let commands = vec![ @@ -417,4 +414,5 @@ prefix_rule( }, evaluation ); + Ok(()) } From 96e3f802ce4ec55d73acab28dbbbd0bd25e52a93 Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Tue, 2 Dec 2025 13:10:17 -0500 Subject: [PATCH 14/15] fix test error assertion --- codex-rs/execpolicy/tests/basic.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/codex-rs/execpolicy/tests/basic.rs b/codex-rs/execpolicy/tests/basic.rs index 41328c8252..9a7ec58b1e 100644 --- a/codex-rs/execpolicy/tests/basic.rs +++ b/codex-rs/execpolicy/tests/basic.rs @@ -99,10 +99,10 @@ fn add_prefix_rule_rejects_empty_prefix() -> Result<()> { let mut policy = Policy::empty(); let result = policy.add_prefix_rule(&[], Decision::Allow); - assert!(matches!( - result, - Err(Error::InvalidPattern(message)) if message == "prefix cannot be empty" - )); + match result.unwrap_err() { + Error::InvalidPattern(message) => assert_eq!(message, "prefix cannot be empty"), + other => panic!("expected InvalidPattern(..), got {other:?}"), + } Ok(()) } From 96a8e4721fefd9dfdb70b60c56a25d5a04b609e1 Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Tue, 2 Dec 2025 13:18:13 -0500 Subject: [PATCH 15/15] serializing commands nicely --- codex-rs/execpolicy/src/amend.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/codex-rs/execpolicy/src/amend.rs b/codex-rs/execpolicy/src/amend.rs index 1613a057dd..36a02cebf9 100644 --- a/codex-rs/execpolicy/src/amend.rs +++ b/codex-rs/execpolicy/src/amend.rs @@ -64,8 +64,12 @@ pub fn blocking_append_allow_prefix_rule( return Err(AmendError::EmptyPrefix); } - let pattern = - serde_json::to_string(prefix).map_err(|source| AmendError::SerializePrefix { source })?; + let tokens = prefix + .iter() + .map(serde_json::to_string) + .collect::, _>>() + .map_err(|source| AmendError::SerializePrefix { source })?; + let pattern = format!("[{}]", tokens.join(", ")); let rule = format!(r#"prefix_rule(pattern={pattern}, decision="allow")"#); let dir = policy_path @@ -162,7 +166,7 @@ mod tests { std::fs::read_to_string(&policy_path).expect("default.codexpolicy should exist"); assert_eq!( contents, - r#"prefix_rule(pattern=["echo","Hello, world!"], decision="allow") + r#"prefix_rule(pattern=["echo", "Hello, world!"], decision="allow") "# ); } @@ -189,7 +193,7 @@ mod tests { assert_eq!( contents, r#"prefix_rule(pattern=["ls"], decision="allow") -prefix_rule(pattern=["echo","Hello, world!"], decision="allow") +prefix_rule(pattern=["echo", "Hello, world!"], decision="allow") "# ); } @@ -215,7 +219,7 @@ prefix_rule(pattern=["echo","Hello, world!"], decision="allow") assert_eq!( contents, r#"prefix_rule(pattern=["ls"], decision="allow") -prefix_rule(pattern=["echo","Hello, world!"], decision="allow") +prefix_rule(pattern=["echo", "Hello, world!"], decision="allow") "# ); }