chore: refactor to move Arc<RwLock> concern outside exec_policy_for (#7615)

The caller should decide whether wrapping the policy in `Arc<RwLock>` is
necessary. This should make #7609 a
bit smoother.

- `exec_policy_for()` -> `load_exec_policy_for_features()`
- introduce `load_exec_policy()` that does not take `Features` as an arg
- both return `Result<Policy, ExecPolicyError>` instead of
Result<Arc<RwLock<Policy>>, ExecPolicyError>`

This simplifies the tests as they have no need for `Arc<RwLock>`.

Author: bolinfest

codex-rs/core/src/codex.rs

@@ -11,6 +11,7 @@ use crate::compact;
 use crate::compact::run_inline_auto_compact_task;
 use crate::compact::should_use_remote_compact_task;
 use crate::compact_remote::run_inline_remote_auto_compact_task;
+use crate::exec_policy::load_exec_policy_for_features;
 use crate::features::Feature;
 use crate::features::Features;
 use crate::openai_models::models_manager::ModelsManager;
@@ -174,9 +175,10 @@ impl Codex {
 
         let user_instructions = get_user_instructions(&config).await;
 
-        let exec_policy = crate::exec_policy::exec_policy_for(&config.features, &config.codex_home)
+        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 config = Arc::new(config);
 

codex-rs/core/src/exec_policy.rs

@@ -73,14 +73,18 @@ pub enum ExecPolicyUpdateError {
     FeatureDisabled,
 }
 
-pub(crate) async fn exec_policy_for(
+pub(crate) async fn load_exec_policy_for_features(
     features: &Features,
     codex_home: &Path,
-) -> Result<Arc<RwLock<Policy>>, ExecPolicyError> {
+) -> Result<Policy, ExecPolicyError> {
     if !features.enabled(Feature::ExecPolicy) {
-        return Ok(Arc::new(RwLock::new(Policy::empty())));
+        Ok(Policy::empty())
+    } else {
+        load_exec_policy(codex_home).await
     }
+}
 
+pub async fn load_exec_policy(codex_home: &Path) -> Result<Policy, ExecPolicyError> {
     let policy_dir = codex_home.join(POLICY_DIR_NAME);
     let policy_paths = collect_policy_files(&policy_dir).await?;
 
@@ -102,7 +106,7 @@ pub(crate) async fn exec_policy_for(
             })?;
     }
 
-    let policy = Arc::new(RwLock::new(parser.build()));
+    let policy = parser.build();
     tracing::debug!(
         "loaded execpolicy from {} files in {}",
         policy_paths.len(),
@@ -306,7 +310,7 @@ mod tests {
         features.disable(Feature::ExecPolicy);
         let temp_dir = tempdir().expect("create temp dir");
 
-        let policy = exec_policy_for(&features, temp_dir.path())
+        let policy = load_exec_policy_for_features(&features, temp_dir.path())
             .await
             .expect("policy result");
 
@@ -319,10 +323,7 @@ mod tests {
                     decision: Decision::Allow
                 }],
             },
-            policy
-                .read()
-                .await
-                .check_multiple(commands.iter(), &|_| Decision::Allow)
+            policy.check_multiple(commands.iter(), &|_| Decision::Allow)
         );
         assert!(!temp_dir.path().join(POLICY_DIR_NAME).exists());
     }
@@ -350,7 +351,7 @@ mod tests {
         )
         .expect("write policy file");
 
-        let policy = exec_policy_for(&Features::with_defaults(), temp_dir.path())
+        let policy = load_exec_policy(temp_dir.path())
             .await
             .expect("policy result");
         let command = [vec!["rm".to_string()]];
@@ -362,10 +363,7 @@ mod tests {
                     decision: Decision::Forbidden
                 }],
             },
-            policy
-                .read()
-                .await
-                .check_multiple(command.iter(), &|_| Decision::Allow)
+            policy.check_multiple(command.iter(), &|_| Decision::Allow)
         );
     }
 
@@ -378,7 +376,7 @@ mod tests {
         )
         .expect("write policy file");
 
-        let policy = exec_policy_for(&Features::with_defaults(), temp_dir.path())
+        let policy = load_exec_policy(temp_dir.path())
             .await
             .expect("policy result");
         let command = [vec!["ls".to_string()]];
@@ -390,10 +388,7 @@ mod tests {
                     decision: Decision::Allow
                 }],
             },
-            policy
-                .read()
-                .await
-                .check_multiple(command.iter(), &|_| Decision::Allow)
+            policy.check_multiple(command.iter(), &|_| Decision::Allow)
         );
     }