From ded3b254a115067e3d0dbd3c69414fa5ee4eca71 Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Sat, 31 Jan 2026 14:55:49 -0700 Subject: [PATCH] Deduplicate execpolicy prefix append --- codex-rs/execpolicy/src/amend.rs | 39 ++++++++++++------------------ codex-rs/execpolicy/tests/basic.rs | 21 ++++++++++++++++ 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/codex-rs/execpolicy/src/amend.rs b/codex-rs/execpolicy/src/amend.rs index 9114d3a64f..9809a46fe2 100644 --- a/codex-rs/execpolicy/src/amend.rs +++ b/codex-rs/execpolicy/src/amend.rs @@ -105,35 +105,28 @@ fn append_locked_line(policy_path: &Path, line: &str) -> Result<(), AmendError> source, })?; - let len = file - .metadata() - .map_err(|source| AmendError::PolicyMetadata { + file.seek(SeekFrom::Start(0)) + .map_err(|source| AmendError::SeekPolicyFile { path: policy_path.to_path_buf(), source, - })? - .len(); + })?; + let mut contents = String::new(); + file.read_to_string(&mut contents) + .map_err(|source| AmendError::ReadPolicyFile { + path: policy_path.to_path_buf(), + source, + })?; - // Ensure file ends in a newline before appending. - if len > 0 { - file.seek(SeekFrom::End(-1)) - .map_err(|source| AmendError::SeekPolicyFile { + if contents.lines().any(|existing| existing == line) { + return Ok(()); + } + + if !contents.is_empty() && !contents.ends_with('\n') { + file.write_all(b"\n") + .map_err(|source| AmendError::WritePolicyFile { 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.to_path_buf(), - source, - })?; - - if last[0] != b'\n' { - file.write_all(b"\n") - .map_err(|source| AmendError::WritePolicyFile { - path: policy_path.to_path_buf(), - source, - })?; - } } file.write_all(format!("{line}\n").as_bytes()) diff --git a/codex-rs/execpolicy/tests/basic.rs b/codex-rs/execpolicy/tests/basic.rs index ed6cf3185e..040509f115 100644 --- a/codex-rs/execpolicy/tests/basic.rs +++ b/codex-rs/execpolicy/tests/basic.rs @@ -1,4 +1,5 @@ use std::any::Any; +use std::fs; use std::sync::Arc; use anyhow::Context; @@ -10,10 +11,12 @@ use codex_execpolicy::Policy; use codex_execpolicy::PolicyParser; use codex_execpolicy::RuleMatch; use codex_execpolicy::RuleRef; +use codex_execpolicy::blocking_append_allow_prefix_rule; use codex_execpolicy::rule::PatternToken; use codex_execpolicy::rule::PrefixPattern; use codex_execpolicy::rule::PrefixRule; use pretty_assertions::assert_eq; +use tempfile::tempdir; fn tokens(cmd: &[&str]) -> Vec { cmd.iter().map(std::string::ToString::to_string).collect() @@ -46,6 +49,24 @@ fn rule_snapshots(rules: &[RuleRef]) -> Vec { .collect() } +#[test] +fn append_allow_prefix_rule_dedupes_existing_rule() -> 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(&policy_path, &prefix)?; + blocking_append_allow_prefix_rule(&policy_path, &prefix)?; + + let contents = fs::read_to_string(&policy_path).context("read policy")?; + assert_eq!( + contents, + r#"prefix_rule(pattern=["python3"], decision="allow") +"# + ); + Ok(()) +} + #[test] fn basic_match() -> Result<()> { let policy_src = r#"