fix writable roots

Author: aibrahim-oai

codex-rs/app-server/tests/suite/send_message.rs

@@ -21,6 +21,7 @@ use codex_protocol::protocol::SandboxPolicy;
 use core_test_support::responses;
 use pretty_assertions::assert_eq;
 use std::path::Path;
+use std::path::PathBuf;
 use tempfile::TempDir;
 use tokio::time::timeout;
 
@@ -354,6 +355,7 @@ fn assert_permissions_message(item: &ResponseItem) {
             let expected = DeveloperInstructions::from_policy(
                 &SandboxPolicy::DangerFullAccess,
                 AskForApproval::Never,
+                &PathBuf::from("/tmp"),
             )
             .into_text();
             assert_eq!(

codex-rs/core/src/codex.rs

@@ -1021,7 +1021,14 @@ impl Session {
             return None;
         }
 
-        Some(DeveloperInstructions::from_policy(&next.sandbox_policy, next.approval_policy).into())
+        Some(
+            DeveloperInstructions::from_policy(
+                &next.sandbox_policy,
+                next.approval_policy,
+                &next.cwd,
+            )
+            .into(),
+        )
     }
 
     /// Persist the event to rollout and send it to clients.
@@ -1361,6 +1368,7 @@ impl Session {
             DeveloperInstructions::from_policy(
                 &turn_context.sandbox_policy,
                 turn_context.approval_policy,
+                &turn_context.cwd,
             )
             .into(),
         );

codex-rs/core/tests/suite/permissions_messages.rs

@@ -3,7 +3,10 @@ use codex_core::config::Constrained;
 use codex_core::protocol::AskForApproval;
 use codex_core::protocol::EventMsg;
 use codex_core::protocol::Op;
+use codex_core::protocol::SandboxPolicy;
+use codex_protocol::models::DeveloperInstructions;
 use codex_protocol::user_input::UserInput;
+use codex_utils_absolute_path::AbsolutePathBuf;
 use core_test_support::responses::ev_completed;
 use core_test_support::responses::ev_response_created;
 use core_test_support::responses::mount_sse_once;
@@ -14,6 +17,8 @@ use core_test_support::test_codex::test_codex;
 use core_test_support::wait_for_event;
 use pretty_assertions::assert_eq;
 use std::collections::HashSet;
+use std::path::PathBuf;
+use tempfile::TempDir;
 
 fn permissions_texts(input: &[serde_json::Value]) -> Vec<String> {
     input
@@ -42,15 +47,35 @@ fn sse_completed(id: &str) -> String {
     sse(vec![ev_response_created(id), ev_completed(id)])
 }
 
+fn workspace_write_policy(root: AbsolutePathBuf) -> SandboxPolicy {
+    SandboxPolicy::WorkspaceWrite {
+        writable_roots: vec![root],
+        network_access: false,
+        exclude_tmpdir_env_var: true,
+        exclude_slash_tmp: true,
+    }
+}
+
+fn expected_permissions_message(
+    sandbox_policy: &SandboxPolicy,
+    approval_policy: AskForApproval,
+    cwd: &PathBuf,
+) -> String {
+    DeveloperInstructions::from_policy(sandbox_policy, approval_policy, cwd).into_text()
+}
+
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn permissions_message_sent_once_on_start() -> Result<()> {
     skip_if_no_network!(Ok(()));
 
     let server = start_mock_server().await;
     let req = mount_sse_once(&server, sse_completed("resp-1")).await;
 
+    let writable = TempDir::new()?;
+    let sandbox_policy = workspace_write_policy(AbsolutePathBuf::try_from(writable.path())?);
     let mut builder = test_codex().with_config(|config| {
         config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
+        config.sandbox_policy = Constrained::allow_any(sandbox_policy.clone());
     });
     let test = builder.build(&server).await?;
 
@@ -68,7 +93,9 @@ async fn permissions_message_sent_once_on_start() -> Result<()> {
     let body = request.body_json();
     let input = body["input"].as_array().expect("input array");
     let permissions = permissions_texts(input);
-    assert_eq!(permissions.len(), 1);
+    let expected =
+        expected_permissions_message(&sandbox_policy, AskForApproval::OnRequest, &test.config.cwd);
+    assert_eq!(permissions, vec![expected]);
 
     Ok(())
 }
@@ -81,10 +108,17 @@ async fn permissions_message_added_on_override_change() -> Result<()> {
     let req1 = mount_sse_once(&server, sse_completed("resp-1")).await;
     let req2 = mount_sse_once(&server, sse_completed("resp-2")).await;
 
+    let writable = TempDir::new()?;
+    let sandbox_policy = workspace_write_policy(AbsolutePathBuf::try_from(writable.path())?);
     let mut builder = test_codex().with_config(|config| {
         config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
+        config.sandbox_policy = Constrained::allow_any(sandbox_policy.clone());
     });
     let test = builder.build(&server).await?;
+    let expected_on_request =
+        expected_permissions_message(&sandbox_policy, AskForApproval::OnRequest, &test.config.cwd);
+    let expected_never =
+        expected_permissions_message(&sandbox_policy, AskForApproval::Never, &test.config.cwd);
 
     test.codex
         .submit(Op::UserInput {
@@ -124,8 +158,11 @@ async fn permissions_message_added_on_override_change() -> Result<()> {
     let permissions_1 = permissions_texts(input1);
     let permissions_2 = permissions_texts(input2);
 
-    assert_eq!(permissions_1.len(), 1);
-    assert_eq!(permissions_2.len(), 2);
+    assert_eq!(permissions_1, vec![expected_on_request.clone()]);
+    assert_eq!(
+        permissions_2,
+        vec![expected_on_request, expected_never.clone()]
+    );
     let unique = permissions_2.into_iter().collect::<HashSet<String>>();
     assert_eq!(unique.len(), 2);
 
@@ -140,10 +177,15 @@ async fn permissions_message_not_added_when_no_change() -> Result<()> {
     let req1 = mount_sse_once(&server, sse_completed("resp-1")).await;
     let req2 = mount_sse_once(&server, sse_completed("resp-2")).await;
 
+    let writable = TempDir::new()?;
+    let sandbox_policy = workspace_write_policy(AbsolutePathBuf::try_from(writable.path())?);
     let mut builder = test_codex().with_config(|config| {
         config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
+        config.sandbox_policy = Constrained::allow_any(sandbox_policy.clone());
     });
     let test = builder.build(&server).await?;
+    let expected =
+        expected_permissions_message(&sandbox_policy, AskForApproval::OnRequest, &test.config.cwd);
 
     test.codex
         .submit(Op::UserInput {
@@ -172,9 +214,8 @@ async fn permissions_message_not_added_when_no_change() -> Result<()> {
     let permissions_1 = permissions_texts(input1);
     let permissions_2 = permissions_texts(input2);
 
-    assert_eq!(permissions_1.len(), 1);
-    assert_eq!(permissions_2.len(), 1);
-    assert_eq!(permissions_1, permissions_2);
+    assert_eq!(permissions_1, vec![expected.clone()]);
+    assert_eq!(permissions_2, vec![expected]);
 
     Ok(())
 }
@@ -188,12 +229,22 @@ async fn resume_replays_permissions_messages() -> Result<()> {
     let _req2 = mount_sse_once(&server, sse_completed("resp-2")).await;
     let req3 = mount_sse_once(&server, sse_completed("resp-3")).await;
 
+    let writable = TempDir::new()?;
+    let sandbox_policy = workspace_write_policy(AbsolutePathBuf::try_from(writable.path())?);
     let mut builder = test_codex().with_config(|config| {
         config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
+        config.sandbox_policy = Constrained::allow_any(sandbox_policy.clone());
     });
     let initial = builder.build(&server).await?;
     let rollout_path = initial.session_configured.rollout_path.clone();
     let home = initial.home.clone();
+    let expected_on_request = expected_permissions_message(
+        &sandbox_policy,
+        AskForApproval::OnRequest,
+        &initial.config.cwd,
+    );
+    let expected_never =
+        expected_permissions_message(&sandbox_policy, AskForApproval::Never, &initial.config.cwd);
 
     initial
         .codex
@@ -244,7 +295,14 @@ async fn resume_replays_permissions_messages() -> Result<()> {
     let body3 = req3.single_request().body_json();
     let input = body3["input"].as_array().expect("input array");
     let permissions = permissions_texts(input);
-    assert_eq!(permissions.len(), 3);
+    assert_eq!(
+        permissions,
+        vec![
+            expected_on_request.clone(),
+            expected_never,
+            expected_on_request
+        ]
+    );
     let unique = permissions.into_iter().collect::<HashSet<String>>();
     assert_eq!(unique.len(), 2);
 
@@ -261,12 +319,27 @@ async fn resume_and_fork_append_permissions_messages() -> Result<()> {
     let req3 = mount_sse_once(&server, sse_completed("resp-3")).await;
     let req4 = mount_sse_once(&server, sse_completed("resp-4")).await;
 
+    let writable = TempDir::new()?;
+    let sandbox_policy = workspace_write_policy(AbsolutePathBuf::try_from(writable.path())?);
     let mut builder = test_codex().with_config(|config| {
         config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
+        config.sandbox_policy = Constrained::allow_any(sandbox_policy.clone());
     });
     let initial = builder.build(&server).await?;
     let rollout_path = initial.session_configured.rollout_path.clone();
     let home = initial.home.clone();
+    let expected_on_request = expected_permissions_message(
+        &sandbox_policy,
+        AskForApproval::OnRequest,
+        &initial.config.cwd,
+    );
+    let expected_never =
+        expected_permissions_message(&sandbox_policy, AskForApproval::Never, &initial.config.cwd);
+    let expected_unless_trusted = expected_permissions_message(
+        &sandbox_policy,
+        AskForApproval::UnlessTrusted,
+        &initial.config.cwd,
+    );
 
     initial
         .codex
@@ -305,7 +378,10 @@ async fn resume_and_fork_append_permissions_messages() -> Result<()> {
     let body2 = req2.single_request().body_json();
     let input2 = body2["input"].as_array().expect("input array");
     let permissions_base = permissions_texts(input2);
-    assert_eq!(permissions_base.len(), 2);
+    assert_eq!(
+        permissions_base,
+        vec![expected_on_request.clone(), expected_never.clone()]
+    );
 
     builder = builder.with_config(|config| {
         config.approval_policy = Constrained::allow_any(AskForApproval::UnlessTrusted);
@@ -325,12 +401,14 @@ async fn resume_and_fork_append_permissions_messages() -> Result<()> {
     let body3 = req3.single_request().body_json();
     let input3 = body3["input"].as_array().expect("input array");
     let permissions_resume = permissions_texts(input3);
-    assert_eq!(permissions_resume.len(), permissions_base.len() + 1);
     assert_eq!(
-        &permissions_resume[..permissions_base.len()],
-        permissions_base.as_slice()
+        permissions_resume,
+        vec![
+            expected_on_request.clone(),
+            expected_never.clone(),
+            expected_unless_trusted.clone()
+        ]
     );
-    assert!(!permissions_base.contains(permissions_resume.last().expect("new permissions")));
 
     let mut fork_config = initial.config.clone();
     fork_config.approval_policy = Constrained::allow_any(AskForApproval::UnlessTrusted);
@@ -352,15 +430,15 @@ async fn resume_and_fork_append_permissions_messages() -> Result<()> {
     let body4 = req4.single_request().body_json();
     let input4 = body4["input"].as_array().expect("input array");
     let permissions_fork = permissions_texts(input4);
-    assert_eq!(permissions_fork.len(), permissions_base.len() + 2);
     assert_eq!(
-        &permissions_fork[..permissions_base.len()],
-        permissions_base.as_slice()
+        permissions_fork,
+        vec![
+            expected_on_request,
+            expected_never,
+            expected_unless_trusted.clone(),
+            expected_unless_trusted
+        ]
     );
-    let new_permissions = &permissions_fork[permissions_base.len()..];
-    assert_eq!(new_permissions.len(), 2);
-    assert_eq!(new_permissions[0], new_permissions[1]);
-    assert!(!permissions_base.contains(&new_permissions[0]));
 
     Ok(())
 }

codex-rs/protocol/src/models.rs

@@ -1,4 +1,5 @@
 use std::collections::HashMap;
+use std::path::PathBuf;
 
 use codex_utils_image::load_and_resize_to_fit;
 use mcp_types::CallToolResult;
@@ -13,9 +14,9 @@ use crate::config_types::SandboxMode;
 use crate::protocol::AskForApproval;
 use crate::protocol::NetworkAccess;
 use crate::protocol::SandboxPolicy;
+use crate::protocol::WritableRoot;
 use crate::user_input::UserInput;
 use codex_git::GhostCommit;
-use codex_utils_absolute_path::AbsolutePathBuf;
 use codex_utils_image::error::ImageProcessingError;
 use schemars::JsonSchema;
 
@@ -200,7 +201,11 @@ impl DeveloperInstructions {
         Self { text }
     }
 
-    pub fn from_policy(sandbox_policy: &SandboxPolicy, approval_policy: AskForApproval) -> Self {
+    pub fn from_policy(
+        sandbox_policy: &SandboxPolicy,
+        approval_policy: AskForApproval,
+        cwd: &PathBuf,
+    ) -> Self {
         let network_access = if sandbox_policy.has_full_network_access() {
             NetworkAccess::Enabled
         } else {
@@ -211,13 +216,9 @@ impl DeveloperInstructions {
             SandboxPolicy::DangerFullAccess => (SandboxMode::DangerFullAccess, None),
             SandboxPolicy::ReadOnly => (SandboxMode::ReadOnly, None),
             SandboxPolicy::ExternalSandbox { .. } => (SandboxMode::DangerFullAccess, None),
-            SandboxPolicy::WorkspaceWrite { writable_roots, .. } => {
-                let roots = if writable_roots.is_empty() {
-                    None
-                } else {
-                    Some(writable_roots.clone())
-                };
-                (SandboxMode::WorkspaceWrite, roots)
+            SandboxPolicy::WorkspaceWrite { .. } => {
+                let roots = sandbox_policy.get_writable_roots_with_cwd(cwd);
+                (SandboxMode::WorkspaceWrite, Some(roots))
             }
         };
 
@@ -233,7 +234,7 @@ impl DeveloperInstructions {
         sandbox_mode: SandboxMode,
         network_access: NetworkAccess,
         approval_policy: AskForApproval,
-        writable_roots: Option<Vec<AbsolutePathBuf>>,
+        writable_roots: Option<Vec<WritableRoot>>,
     ) -> Self {
         let start_tag = DeveloperInstructions::new("<permissions instructions>");
         let end_tag = DeveloperInstructions::new("</permissions instructions>");
@@ -247,7 +248,7 @@ impl DeveloperInstructions {
             .concat(end_tag)
     }
 
-    fn from_writable_roots(writable_roots: Option<Vec<AbsolutePathBuf>>) -> Self {
+    fn from_writable_roots(writable_roots: Option<Vec<WritableRoot>>) -> Self {
         let Some(roots) = writable_roots else {
             return DeveloperInstructions::new("");
         };
@@ -258,7 +259,7 @@ impl DeveloperInstructions {
 
         let roots_list: Vec<String> = roots
             .iter()
-            .map(|r| format!("`{}`", r.to_string_lossy()))
+            .map(|r| format!("`{}`", r.root.to_string_lossy()))
             .collect();
         let text = if roots_list.len() == 1 {
             format!(" The writable root is {}.", roots_list[0])
@@ -762,8 +763,11 @@ mod tests {
             exclude_slash_tmp: false,
         };
 
-        let instructions =
-            DeveloperInstructions::from_policy(&policy, AskForApproval::UnlessTrusted);
+        let instructions = DeveloperInstructions::from_policy(
+            &policy,
+            AskForApproval::UnlessTrusted,
+            &PathBuf::from("/tmp"),
+        );
         let text = instructions.into_text();
         assert!(text.contains("Network access is enabled."));
         assert!(text.contains("`approval_policy` is `unless-trusted`"));