From d291d891304d7bbe4bce7bf5f9cc640965a3e232 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 19 Dec 2025 09:39:26 -0800 Subject: [PATCH] Remove RwLock from exec_policy --- codex-rs/core/src/codex.rs | 20 ++++--- codex-rs/core/src/exec_policy.rs | 60 +++++++++---------- codex-rs/core/src/tools/handlers/shell.rs | 2 +- .../core/src/unified_exec/session_manager.rs | 2 +- 4 files changed, 42 insertions(+), 42 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index f0d2056587..2078a63316 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -245,7 +245,7 @@ impl Codex { let exec_policy = load_exec_policy_for_features(&config.features, &config.codex_home) .await .map_err(|err| CodexErr::Fatal(format!("failed to load execpolicy: {err}")))?; - let exec_policy = Arc::new(RwLock::new(exec_policy)); + let exec_policy = Arc::new(exec_policy); let config = Arc::new(config); if config.features.enabled(Feature::RemoteModels) @@ -372,7 +372,7 @@ pub(crate) struct TurnContext { pub(crate) final_output_json_schema: Option, pub(crate) codex_linux_sandbox_exe: Option, pub(crate) tool_call_gate: Arc, - pub(crate) exec_policy: Arc>, + pub(crate) exec_policy: Arc, pub(crate) truncation_policy: TruncationPolicy, } @@ -428,7 +428,7 @@ pub(crate) struct SessionConfiguration { cwd: PathBuf, /// Execpolicy policy, applied only when enabled by feature flag. - exec_policy: Arc>, + exec_policy: Arc, // TODO(pakrym): Remove config from here original_config_do_not_use: Arc, @@ -1049,12 +1049,14 @@ impl Session { return Err(ExecPolicyUpdateError::FeatureDisabled); } - crate::exec_policy::append_execpolicy_amendment_and_update( + let updated_policy = crate::exec_policy::append_execpolicy_amendment_and_update( &codex_home, - ¤t_policy, + current_policy.as_ref(), &amendment.command, ) .await?; + let mut state = self.state.lock().await; + state.session_configuration.exec_policy = Arc::new(updated_policy); Ok(()) } @@ -2849,7 +2851,7 @@ mod tests { sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), - exec_policy: Arc::new(RwLock::new(ExecPolicy::empty())), + exec_policy: Arc::new(ExecPolicy::empty()), session_source: SessionSource::Exec, }; @@ -2916,7 +2918,7 @@ mod tests { sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), - exec_policy: Arc::new(RwLock::new(ExecPolicy::empty())), + exec_policy: Arc::new(ExecPolicy::empty()), session_source: SessionSource::Exec, }; @@ -3123,7 +3125,7 @@ mod tests { sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), - exec_policy: Arc::new(RwLock::new(ExecPolicy::empty())), + exec_policy: Arc::new(ExecPolicy::empty()), session_source: SessionSource::Exec, }; let per_turn_config = Session::build_per_turn_config(&session_configuration); @@ -3209,7 +3211,7 @@ mod tests { sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), - exec_policy: Arc::new(RwLock::new(ExecPolicy::empty())), + exec_policy: Arc::new(ExecPolicy::empty()), session_source: SessionSource::Exec, }; let per_turn_config = Session::build_per_turn_config(&session_configuration); diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 8917610ffc..89fcc06301 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -1,7 +1,6 @@ use std::io::ErrorKind; use std::path::Path; use std::path::PathBuf; -use std::sync::Arc; use crate::command_safety::is_dangerous_command::requires_initial_appoval; use codex_execpolicy::AmendError; @@ -17,7 +16,6 @@ use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::SandboxPolicy; use thiserror::Error; use tokio::fs; -use tokio::sync::RwLock; use tokio::task::spawn_blocking; use crate::bash::parse_shell_lc_plain_commands; @@ -129,9 +127,9 @@ pub(crate) fn default_policy_path(codex_home: &Path) -> PathBuf { pub(crate) async fn append_execpolicy_amendment_and_update( codex_home: &Path, - current_policy: &Arc>, + current_policy: &Policy, prefix: &[String], -) -> Result<(), ExecPolicyUpdateError> { +) -> Result { let policy_path = default_policy_path(codex_home); let prefix = prefix.to_vec(); spawn_blocking({ @@ -146,12 +144,10 @@ pub(crate) async fn append_execpolicy_amendment_and_update( source, })?; - current_policy - .write() - .await - .add_prefix_rule(&prefix, Decision::Allow)?; + let mut next_policy = current_policy.clone(); + next_policy.add_prefix_rule(&prefix, Decision::Allow)?; - Ok(()) + Ok(next_policy) } /// Derive a proposed execpolicy amendment when a command requires user approval @@ -218,7 +214,7 @@ fn derive_prompt_reason(evaluation: &Evaluation) -> Option { } pub(crate) async fn create_exec_approval_requirement_for_command( - exec_policy: &Arc>, + exec_policy: &Policy, features: &Features, command: &[String], approval_policy: AskForApproval, @@ -233,8 +229,7 @@ pub(crate) async fn create_exec_approval_requirement_for_command( Decision::Allow } }; - let policy = exec_policy.read().await; - let evaluation = policy.check_multiple(commands.iter(), &heuristics_fallback); + let evaluation = exec_policy.check_multiple(commands.iter(), &heuristics_fallback); match evaluation.decision { Decision::Forbidden => ExecApprovalRequirement::Forbidden { @@ -325,7 +320,6 @@ 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,7 +419,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") parser .parse("test.rules", policy_src) .expect("parse policy"); - let policy = Arc::new(RwLock::new(parser.build())); + let policy = parser.build(); let forbidden_script = vec![ "bash".to_string(), @@ -458,7 +452,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") parser .parse("test.rules", policy_src) .expect("parse policy"); - let policy = Arc::new(RwLock::new(parser.build())); + let policy = parser.build(); let command = vec!["rm".to_string()]; let requirement = create_exec_approval_requirement_for_command( @@ -487,7 +481,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") parser .parse("test.rules", policy_src) .expect("parse policy"); - let policy = Arc::new(RwLock::new(parser.build())); + let policy = parser.build(); let command = vec!["rm".to_string()]; let requirement = create_exec_approval_requirement_for_command( @@ -512,7 +506,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") async fn exec_approval_requirement_falls_back_to_heuristics() { let command = vec!["cargo".to_string(), "build".to_string()]; - let empty_policy = Arc::new(RwLock::new(Policy::empty())); + let empty_policy = Policy::empty(); let requirement = create_exec_approval_requirement_for_command( &empty_policy, &Features::with_defaults(), @@ -539,7 +533,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") parser .parse("test.rules", policy_src) .expect("parse policy"); - let policy = Arc::new(RwLock::new(parser.build())); + let policy = parser.build(); let command = vec![ "bash".to_string(), "-lc".to_string(), @@ -568,14 +562,15 @@ prefix_rule(pattern=["rm"], decision="forbidden") #[tokio::test] async fn append_execpolicy_amendment_updates_policy_and_file() { let codex_home = tempdir().expect("create temp dir"); - let current_policy = Arc::new(RwLock::new(Policy::empty())); + let current_policy = Policy::empty(); let prefix = vec!["echo".to_string(), "hello".to_string()]; - append_execpolicy_amendment_and_update(codex_home.path(), ¤t_policy, &prefix) - .await - .expect("update policy"); + let updated_policy = + append_execpolicy_amendment_and_update(codex_home.path(), ¤t_policy, &prefix) + .await + .expect("update policy"); - let evaluation = current_policy.read().await.check( + let evaluation = updated_policy.check( &["echo".to_string(), "hello".to_string(), "world".to_string()], &|_| Decision::Allow, ); @@ -599,7 +594,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") #[tokio::test] async fn append_execpolicy_amendment_rejects_empty_prefix() { let codex_home = tempdir().expect("create temp dir"); - let current_policy = Arc::new(RwLock::new(Policy::empty())); + let current_policy = Policy::empty(); let result = append_execpolicy_amendment_and_update(codex_home.path(), ¤t_policy, &[]).await; @@ -617,7 +612,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") async fn proposed_execpolicy_amendment_is_present_for_single_command_without_policy_match() { let command = vec!["cargo".to_string(), "build".to_string()]; - let empty_policy = Arc::new(RwLock::new(Policy::empty())); + let empty_policy = Policy::empty(); let requirement = create_exec_approval_requirement_for_command( &empty_policy, &Features::with_defaults(), @@ -643,9 +638,10 @@ prefix_rule(pattern=["rm"], decision="forbidden") let mut features = Features::with_defaults(); features.disable(Feature::ExecPolicy); + let empty_policy = Policy::empty(); let requirement = create_exec_approval_requirement_for_command( - &Arc::new(RwLock::new(Policy::empty())), + &empty_policy, &features, &command, AskForApproval::UnlessTrusted, @@ -670,7 +666,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") parser .parse("test.rules", policy_src) .expect("parse policy"); - let policy = Arc::new(RwLock::new(parser.build())); + let policy = parser.build(); let command = vec!["rm".to_string()]; let requirement = create_exec_approval_requirement_for_command( @@ -699,8 +695,9 @@ prefix_rule(pattern=["rm"], decision="forbidden") "-lc".to_string(), "cargo build && echo ok".to_string(), ]; + let empty_policy = Policy::empty(); let requirement = create_exec_approval_requirement_for_command( - &Arc::new(RwLock::new(Policy::empty())), + &empty_policy, &Features::with_defaults(), &command, AskForApproval::UnlessTrusted, @@ -728,7 +725,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") parser .parse("test.rules", policy_src) .expect("parse policy"); - let policy = Arc::new(RwLock::new(parser.build())); + let policy = parser.build(); let command = vec![ "bash".to_string(), @@ -758,9 +755,10 @@ prefix_rule(pattern=["rm"], decision="forbidden") #[tokio::test] async fn proposed_execpolicy_amendment_is_present_when_heuristics_allow() { let command = vec!["echo".to_string(), "safe".to_string()]; + let empty_policy = Policy::empty(); let requirement = create_exec_approval_requirement_for_command( - &Arc::new(RwLock::new(Policy::empty())), + &empty_policy, &Features::with_defaults(), &command, AskForApproval::OnRequest, @@ -785,7 +783,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") parser .parse("test.rules", policy_src) .expect("parse policy"); - let policy = Arc::new(RwLock::new(parser.build())); + let policy = parser.build(); let command = vec!["echo".to_string(), "safe".to_string()]; let requirement = create_exec_approval_requirement_for_command( diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 624094a5ad..007c96dfe7 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -253,7 +253,7 @@ impl ShellHandler { let features = session.features(); let exec_approval_requirement = create_exec_approval_requirement_for_command( - &turn.exec_policy, + turn.exec_policy.as_ref(), &features, &exec_params.command, turn.approval_policy, diff --git a/codex-rs/core/src/unified_exec/session_manager.rs b/codex-rs/core/src/unified_exec/session_manager.rs index 17d5ef671a..fbbd0c0fd6 100644 --- a/codex-rs/core/src/unified_exec/session_manager.rs +++ b/codex-rs/core/src/unified_exec/session_manager.rs @@ -485,7 +485,7 @@ impl UnifiedExecSessionManager { let mut orchestrator = ToolOrchestrator::new(); let mut runtime = UnifiedExecRuntime::new(self); let exec_approval_requirement = create_exec_approval_requirement_for_command( - &context.turn.exec_policy, + context.turn.exec_policy.as_ref(), &features, command, context.turn.approval_policy,