codex: fix CI failure on PR #12641

Co-authored-by: Codex <noreply@openai.com>

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,27 @@ impl ExecPolicyManager {
         })?;
 
         let mut updated_policy = self.current().as_ref().clone();
-        updated_policy.add_prefix_rule(&prefix, Decision::Allow)?;
+        let has_existing_exact_allow_prefix = updated_policy
+            .check(&prefix, &|_| Decision::Allow)
+            .matched_rules
+            .iter()
+            .any(|rule_match| {
+                matches!(
+                    rule_match,
+                    RuleMatch::PrefixRuleMatch {
+                        matched_prefix,
+                        decision: Decision::Allow,
+                        ..
+                    } if matched_prefix == &prefix
+                )
+            });
+        if !has_existing_exact_allow_prefix {
+            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,23 +1888,75 @@ 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,
+                    resolved_program: None,
+                    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")
 "#
         );
     }
 
+    #[tokio::test]
+    async fn append_execpolicy_amendment_preserves_existing_justification_when_deduped() {
+        let codex_home = tempdir().expect("create temp dir");
+        let prefix = vec!["echo".to_string(), "hello".to_string()];
+        let policy_src = r#"prefix_rule(pattern=["echo", "hello"], decision="allow")"#;
+        let mut parser = PolicyParser::new();
+        parser
+            .parse("test.rules", policy_src)
+            .expect("parse policy");
+        let manager = ExecPolicyManager::new(Arc::new(parser.build()));
+
+        let policy_path = default_policy_path(codex_home.path());
+        fs::create_dir_all(
+            policy_path
+                .parent()
+                .expect("policy path should have parent"),
+        )
+        .expect("create policy dir");
+        fs::write(&policy_path, format!("{policy_src}\n")).expect("seed policy file");
+
+        manager
+            .append_amendment_and_update(codex_home.path(), &ExecPolicyAmendment::from(prefix))
+            .await
+            .expect("update policy");
+        let updated_policy = manager.current();
+
+        let evaluation = updated_policy.check(
+            &["echo".to_string(), "hello".to_string(), "world".to_string()],
+            &|_| Decision::Allow,
+        );
+        assert_eq!(
+            evaluation,
+            Evaluation {
+                decision: Decision::Allow,
+                matched_rules: vec![RuleMatch::PrefixRuleMatch {
+                    matched_prefix: vec!["echo".to_string(), "hello".to_string()],
+                    decision: Decision::Allow,
+                    resolved_program: None,
+                    justification: None,
+                }],
+            }
+        );
+
+        let contents = fs::read_to_string(policy_path).expect("policy file should exist");
+        assert_eq!(contents, format!("{policy_src}\n"));
+    }
+
     #[tokio::test]
     async fn append_execpolicy_amendment_rejects_empty_prefix() {
         let codex_home = tempdir().expect("create temp dir");

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}"
     );
 

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::<Result<Vec<_>, _>>()
         .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")
+"#
+        );
+    }
 }

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;

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<String>,
+    ) -> 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);

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,39 @@ 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,
+                resolved_program: None,
+                justification: Some("persisted during thread".to_string()),
+            }],
+        }
+    );
+    Ok(())
+}
+
 #[test]
 fn basic_match() -> Result<()> {
     let policy_src = r#"