diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 69539ffb7f..a3940e2593 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -866,7 +866,7 @@ impl Session { &self, prefix: &[String], ) -> Result<(), ExecPolicyUpdateError> { - let (features, codex_home) = { + let (features, codex_home, current_policy) = { let state = self.state.lock().await; ( state.session_configuration.features.clone(), @@ -875,12 +875,20 @@ impl Session { .original_config_do_not_use .codex_home .clone(), + state.session_configuration.exec_policy.clone(), ) }; - let policy = - crate::exec_policy::append_allow_prefix_rule_and_reload(&features, &codex_home, prefix) - .await?; + if !features.enabled(Feature::ExecPolicy) { + error!("attempted to append execpolicy rule while execpolicy feature is disabled"); + return Err(ExecPolicyUpdateError::FeatureDisabled); + } + + let policy = crate::exec_policy::append_allow_prefix_rule_and_update( + &codex_home, + current_policy, + prefix, + )?; let mut state = self.state.lock().await; state.session_configuration.exec_policy = policy; @@ -1668,23 +1676,19 @@ mod handlers { decision: ReviewDecision, allow_prefix: Option>, ) { - if let Some(prefix) = allow_prefix - && matches!( - decision, - ReviewDecision::Approved - | ReviewDecision::ApprovedAllowPrefix - | ReviewDecision::ApprovedForSession - ) - && let Err(err) = sess.persist_command_allow_prefix(&prefix).await - { - let message = format!("Failed to update execpolicy allow list: {err}"); - tracing::warn!("{message}"); - let warning = EventMsg::Warning(WarningEvent { message }); - sess.send_event_raw(Event { - id: id.clone(), - msg: warning, - }) - .await; + if let Some(prefix) = allow_prefix { + if matches!(decision, ReviewDecision::ApprovedAllowPrefix) + && let Err(err) = sess.persist_command_allow_prefix(&prefix).await + { + let message = format!("Failed to update execpolicy allow list: {err}"); + tracing::warn!("{message}"); + let warning = EventMsg::Warning(WarningEvent { message }); + sess.send_event_raw(Event { + id: id.clone(), + msg: warning, + }) + .await; + } } match decision { ReviewDecision::Abort => { diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index bfd531c378..8dc961d4be 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -6,6 +6,7 @@ use std::sync::Arc; use crate::command_safety::is_dangerous_command::requires_initial_appoval; use codex_execpolicy::AmendError; use codex_execpolicy::Decision; +use codex_execpolicy::Error as ExecPolicyRuleError; use codex_execpolicy::Evaluation; use codex_execpolicy::Policy; use codex_execpolicy::PolicyParser; @@ -53,8 +54,14 @@ pub enum ExecPolicyUpdateError { #[error("failed to update execpolicy file {path}: {source}")] AppendRule { path: PathBuf, source: AmendError }, - #[error("failed to reload execpolicy after updating policy: {0}")] - Reload(#[from] ExecPolicyError), + #[error("failed to update in-memory execpolicy: {source}")] + AddRule { + #[from] + source: ExecPolicyRuleError, + }, + + #[error("cannot append execpolicy rule because execpolicy feature is disabled")] + FeatureDisabled, } pub(crate) async fn exec_policy_for( @@ -100,9 +107,9 @@ pub(crate) fn default_policy_path(codex_home: &Path) -> PathBuf { codex_home.join(POLICY_DIR_NAME).join(DEFAULT_POLICY_FILE) } -pub(crate) async fn append_allow_prefix_rule_and_reload( - features: &Features, +pub(crate) fn append_allow_prefix_rule_and_update( codex_home: &Path, + mut current_policy: Arc, prefix: &[String], ) -> Result, ExecPolicyUpdateError> { let policy_path = default_policy_path(codex_home); @@ -113,9 +120,9 @@ pub(crate) async fn append_allow_prefix_rule_and_reload( } })?; - exec_policy_for(features, codex_home) - .await - .map_err(ExecPolicyUpdateError::Reload) + Arc::make_mut(&mut current_policy).add_prefix_rule(prefix, Decision::Allow)?; + + Ok(current_policy) } fn requirement_from_decision( @@ -245,6 +252,7 @@ mod tests { use codex_protocol::protocol::SandboxPolicy; use pretty_assertions::assert_eq; use std::fs; + use std::sync::Arc; use tempfile::tempdir; #[tokio::test] @@ -425,6 +433,50 @@ prefix_rule(pattern=["rm"], decision="forbidden") ); } + #[test] + fn append_allow_prefix_rule_updates_policy_and_file() { + let codex_home = tempdir().expect("create temp dir"); + let current_policy = Arc::new(Policy::empty()); + let prefix = vec!["echo".to_string(), "hello".to_string()]; + + let updated_policy = + append_allow_prefix_rule_and_update(codex_home.path(), current_policy, &prefix) + .expect("update policy"); + + let evaluation = + updated_policy.check(&["echo".to_string(), "hello".to_string(), "world".to_string()]); + assert!(matches!( + evaluation, + Evaluation::Match { + decision: Decision::Allow, + .. + } + )); + + let contents = fs::read_to_string(default_policy_path(codex_home.path())) + .expect("policy file should have been created"); + assert_eq!( + contents, + "prefix_rule(pattern=[\"echo\", \"hello\"], decision=\"allow\")\n" + ); + } + + #[test] + fn append_allow_prefix_rule_rejects_empty_prefix() { + let codex_home = tempdir().expect("create temp dir"); + let current_policy = Arc::new(Policy::empty()); + + let result = append_allow_prefix_rule_and_update(codex_home.path(), current_policy, &[]); + + assert!(matches!( + result, + Err(ExecPolicyUpdateError::AppendRule { + source: AmendError::EmptyPrefix, + .. + }) + )); + } + #[test] fn allow_prefix_is_present_for_single_command_without_policy_match() { let command = vec!["python".to_string()];