From 21b846bd87d5a6637b4a53f539d5a2bcf3f4313b Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 24 Jun 2025 15:14:28 -0700 Subject: [PATCH] feat: redesign sandbox config --- codex-rs/cli/src/debug_sandbox.rs | 17 +- codex-rs/cli/src/lib.rs | 7 - codex-rs/common/src/approval_mode_cli_arg.rs | 39 ---- codex-rs/common/src/lib.rs | 2 - codex-rs/core/src/config.rs | 147 +------------ codex-rs/core/src/protocol.rs | 209 ++++++++----------- codex-rs/exec/src/cli.rs | 4 - codex-rs/exec/src/lib.rs | 5 +- codex-rs/linux-sandbox/src/linux_run_main.rs | 11 +- codex-rs/linux-sandbox/tests/landlock.rs | 6 +- codex-rs/mcp-server/src/codex_tool_config.rs | 90 ++------ codex-rs/tui/src/cli.rs | 4 - codex-rs/tui/src/lib.rs | 4 +- 13 files changed, 125 insertions(+), 420 deletions(-) diff --git a/codex-rs/cli/src/debug_sandbox.rs b/codex-rs/cli/src/debug_sandbox.rs index deacca5f28..a21cd4e73e 100644 --- a/codex-rs/cli/src/debug_sandbox.rs +++ b/codex-rs/cli/src/debug_sandbox.rs @@ -1,7 +1,6 @@ use std::path::PathBuf; use codex_common::CliConfigOverrides; -use codex_common::SandboxPermissionOption; use codex_core::config::Config; use codex_core::config::ConfigOverrides; use codex_core::exec::StdioPolicy; @@ -20,13 +19,11 @@ pub async fn run_command_under_seatbelt( ) -> anyhow::Result<()> { let SeatbeltCommand { full_auto, - sandbox, config_overrides, command, } = command; run_command_under_sandbox( full_auto, - sandbox, command, config_overrides, codex_linux_sandbox_exe, @@ -41,13 +38,11 @@ pub async fn run_command_under_landlock( ) -> anyhow::Result<()> { let LandlockCommand { full_auto, - sandbox, config_overrides, command, } = command; run_command_under_sandbox( full_auto, - sandbox, command, config_overrides, codex_linux_sandbox_exe, @@ -63,13 +58,12 @@ enum SandboxType { async fn run_command_under_sandbox( full_auto: bool, - sandbox: SandboxPermissionOption, command: Vec, config_overrides: CliConfigOverrides, codex_linux_sandbox_exe: Option, sandbox_type: SandboxType, ) -> anyhow::Result<()> { - let sandbox_policy = create_sandbox_policy(full_auto, sandbox); + let sandbox_policy = create_sandbox_policy(full_auto); let cwd = std::env::current_dir()?; let config = Config::load_with_cli_overrides( config_overrides @@ -110,13 +104,10 @@ async fn run_command_under_sandbox( handle_exit_status(status); } -pub fn create_sandbox_policy(full_auto: bool, sandbox: SandboxPermissionOption) -> SandboxPolicy { +pub fn create_sandbox_policy(full_auto: bool) -> SandboxPolicy { if full_auto { - SandboxPolicy::new_full_auto_policy() + SandboxPolicy::new_workspace_write_policy() } else { - match sandbox.permissions.map(Into::into) { - Some(sandbox_policy) => sandbox_policy, - None => SandboxPolicy::new_read_only_policy(), - } + SandboxPolicy::new_read_only_policy() } } diff --git a/codex-rs/cli/src/lib.rs b/codex-rs/cli/src/lib.rs index fa78d18ab4..c6d80c0adf 100644 --- a/codex-rs/cli/src/lib.rs +++ b/codex-rs/cli/src/lib.rs @@ -5,7 +5,6 @@ pub mod proto; use clap::Parser; use codex_common::CliConfigOverrides; -use codex_common::SandboxPermissionOption; #[derive(Debug, Parser)] pub struct SeatbeltCommand { @@ -13,9 +12,6 @@ pub struct SeatbeltCommand { #[arg(long = "full-auto", default_value_t = false)] pub full_auto: bool, - #[clap(flatten)] - pub sandbox: SandboxPermissionOption, - #[clap(skip)] pub config_overrides: CliConfigOverrides, @@ -30,9 +26,6 @@ pub struct LandlockCommand { #[arg(long = "full-auto", default_value_t = false)] pub full_auto: bool, - #[clap(flatten)] - pub sandbox: SandboxPermissionOption, - #[clap(skip)] pub config_overrides: CliConfigOverrides, diff --git a/codex-rs/common/src/approval_mode_cli_arg.rs b/codex-rs/common/src/approval_mode_cli_arg.rs index 199541148a..94bd8e8927 100644 --- a/codex-rs/common/src/approval_mode_cli_arg.rs +++ b/codex-rs/common/src/approval_mode_cli_arg.rs @@ -1,13 +1,9 @@ //! Standard type to use with the `--approval-mode` CLI option. //! Available when the `cli` feature is enabled for the crate. -use clap::ArgAction; -use clap::Parser; use clap::ValueEnum; -use codex_core::config::parse_sandbox_permission_with_base_path; use codex_core::protocol::AskForApproval; -use codex_core::protocol::SandboxPermission; #[derive(Clone, Copy, Debug, ValueEnum)] #[value(rename_all = "kebab-case")] @@ -36,38 +32,3 @@ impl From for AskForApproval { } } } - -#[derive(Parser, Debug)] -pub struct SandboxPermissionOption { - /// Specify this flag multiple times to specify the full set of permissions - /// to grant to Codex. - /// - /// ```shell - /// codex -s disk-full-read-access \ - /// -s disk-write-cwd \ - /// -s disk-write-platform-user-temp-folder \ - /// -s disk-write-platform-global-temp-folder - /// ``` - /// - /// Note disk-write-folder takes a value: - /// - /// ```shell - /// -s disk-write-folder=$HOME/.pyenv/shims - /// ``` - /// - /// These permissions are quite broad and should be used with caution: - /// - /// ```shell - /// -s disk-full-write-access - /// -s network-full-access - /// ``` - #[arg(long = "sandbox-permission", short = 's', action = ArgAction::Append, value_parser = parse_sandbox_permission)] - pub permissions: Option>, -} - -/// Custom value-parser so we can keep the CLI surface small *and* -/// still handle the parameterised `disk-write-folder` case. -fn parse_sandbox_permission(raw: &str) -> std::io::Result { - let base_path = std::env::current_dir()?; - parse_sandbox_permission_with_base_path(raw, base_path) -} diff --git a/codex-rs/common/src/lib.rs b/codex-rs/common/src/lib.rs index c2283640cb..074f648fe6 100644 --- a/codex-rs/common/src/lib.rs +++ b/codex-rs/common/src/lib.rs @@ -6,8 +6,6 @@ pub mod elapsed; #[cfg(feature = "cli")] pub use approval_mode_cli_arg::ApprovalModeCliArg; -#[cfg(feature = "cli")] -pub use approval_mode_cli_arg::SandboxPermissionOption; #[cfg(any(feature = "cli", test))] mod config_override; diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index 74798129ba..a1d1208e39 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -11,7 +11,6 @@ use crate::flags::OPENAI_DEFAULT_MODEL; use crate::model_provider_info::ModelProviderInfo; use crate::model_provider_info::built_in_model_providers; use crate::protocol::AskForApproval; -use crate::protocol::SandboxPermission; use crate::protocol::SandboxPolicy; use dirs::home_dir; use serde::Deserialize; @@ -244,8 +243,10 @@ pub struct ConfigToml { // The `default` attribute ensures that the field is treated as `None` when // the key is omitted from the TOML. Without it, Serde treats the field as // required because we supply a custom deserializer. - #[serde(default, deserialize_with = "deserialize_sandbox_permissions")] - pub sandbox_permissions: Option>, + /// Optional sandbox policy for the session. If omitted, Codex defaults to + /// the restrictive `read-only` policy. + #[serde(default)] + pub sandbox: Option, /// Disable server-side response storage (sends the full conversation /// context with every request). Currently necessary for OpenAI customers @@ -296,32 +297,6 @@ pub struct ConfigToml { pub model_reasoning_summary: Option, } -fn deserialize_sandbox_permissions<'de, D>( - deserializer: D, -) -> Result>, D::Error> -where - D: serde::Deserializer<'de>, -{ - let permissions: Option> = Option::deserialize(deserializer)?; - - match permissions { - Some(raw_permissions) => { - let base_path = find_codex_home().map_err(serde::de::Error::custom)?; - - let converted = raw_permissions - .into_iter() - .map(|raw| { - parse_sandbox_permission_with_base_path(&raw, base_path.clone()) - .map_err(serde::de::Error::custom) - }) - .collect::, D::Error>>()?; - - Ok(Some(converted)) - } - None => Ok(None), - } -} - /// Optional overrides for user configuration (e.g., from CLI flags). #[derive(Default, Debug, Clone)] pub struct ConfigOverrides { @@ -369,20 +344,10 @@ impl Config { None => ConfigProfile::default(), }; - let sandbox_policy = match sandbox_policy { - Some(sandbox_policy) => sandbox_policy, - None => { - // Derive a SandboxPolicy from the permissions in the config. - match cfg.sandbox_permissions { - // Note this means the user can explicitly set permissions - // to the empty list in the config file, granting it no - // permissions whatsoever. - Some(permissions) => SandboxPolicy::from(permissions), - // Default to read only rather than completely locked down. - None => SandboxPolicy::new_read_only_policy(), - } - } - }; + let sandbox_policy = sandbox_policy.unwrap_or_else(|| { + cfg.sandbox + .unwrap_or_else(SandboxPolicy::new_read_only_policy) + }); let mut model_providers = built_in_model_providers(); // Merge user-defined providers into the built-in list. @@ -520,50 +485,6 @@ pub fn log_dir(cfg: &Config) -> std::io::Result { Ok(p) } -pub fn parse_sandbox_permission_with_base_path( - raw: &str, - base_path: PathBuf, -) -> std::io::Result { - use SandboxPermission::*; - - if let Some(path) = raw.strip_prefix("disk-write-folder=") { - return if path.is_empty() { - Err(std::io::Error::new( - std::io::ErrorKind::InvalidInput, - "--sandbox-permission disk-write-folder= requires a non-empty PATH", - )) - } else { - use path_absolutize::*; - - let file = PathBuf::from(path); - let absolute_path = if file.is_relative() { - file.absolutize_from(base_path) - } else { - file.absolutize() - } - .map(|path| path.into_owned())?; - Ok(DiskWriteFolder { - folder: absolute_path, - }) - }; - } - - match raw { - "disk-full-read-access" => Ok(DiskFullReadAccess), - "disk-write-platform-user-temp-folder" => Ok(DiskWritePlatformUserTempFolder), - "disk-write-platform-global-temp-folder" => Ok(DiskWritePlatformGlobalTempFolder), - "disk-write-cwd" => Ok(DiskWriteCwd), - "disk-full-write-access" => Ok(DiskFullWriteAccess), - "network-full-access" => Ok(NetworkFullAccess), - _ => Err(std::io::Error::new( - std::io::ErrorKind::InvalidInput, - format!( - "`{raw}` is not a recognised permission.\nRun with `--help` to see the accepted values." - ), - )), - } -} - #[cfg(test)] mod tests { #![allow(clippy::expect_used, clippy::unwrap_used)] @@ -573,42 +494,6 @@ mod tests { use pretty_assertions::assert_eq; use tempfile::TempDir; - /// Verify that the `sandbox_permissions` field on `ConfigToml` correctly - /// differentiates between a value that is completely absent in the - /// provided TOML (i.e. `None`) and one that is explicitly specified as an - /// empty array (i.e. `Some(vec![])`). This ensures that downstream logic - /// that treats these two cases differently (default read-only policy vs a - /// fully locked-down sandbox) continues to function. - #[test] - fn test_sandbox_permissions_none_vs_empty_vec() { - // Case 1: `sandbox_permissions` key is *absent* from the TOML source. - let toml_source_without_key = ""; - let cfg_without_key: ConfigToml = toml::from_str(toml_source_without_key) - .expect("TOML deserialization without key should succeed"); - assert!(cfg_without_key.sandbox_permissions.is_none()); - - // Case 2: `sandbox_permissions` is present but set to an *empty array*. - let toml_source_with_empty = "sandbox_permissions = []"; - let cfg_with_empty: ConfigToml = toml::from_str(toml_source_with_empty) - .expect("TOML deserialization with empty array should succeed"); - assert_eq!(Some(vec![]), cfg_with_empty.sandbox_permissions); - - // Case 3: `sandbox_permissions` contains a non-empty list of valid values. - let toml_source_with_values = r#" - sandbox_permissions = ["disk-full-read-access", "network-full-access"] - "#; - let cfg_with_values: ConfigToml = toml::from_str(toml_source_with_values) - .expect("TOML deserialization with valid permissions should succeed"); - - assert_eq!( - Some(vec![ - SandboxPermission::DiskFullReadAccess, - SandboxPermission::NetworkFullAccess - ]), - cfg_with_values.sandbox_permissions - ); - } - #[test] fn test_toml_parsing() { let history_with_persistence = r#" @@ -643,22 +528,6 @@ persistence = "none" ); } - /// Deserializing a TOML string containing an *invalid* permission should - /// fail with a helpful error rather than silently defaulting or - /// succeeding. - #[test] - fn test_sandbox_permissions_illegal_value() { - let toml_bad = r#"sandbox_permissions = ["not-a-real-permission"]"#; - - let err = toml::from_str::(toml_bad) - .expect_err("Deserialization should fail for invalid permission"); - - // Make sure the error message contains the invalid value so users have - // useful feedback. - let msg = err.to_string(); - assert!(msg.contains("not-a-real-permission")); - } - struct PrecedenceTestFixture { cwd: TempDir, codex_home: TempDir, diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index 737acc7732..b6ca2a602a 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -136,157 +136,126 @@ pub enum AskForApproval { Never, } -/// Determines execution restrictions for model shell commands -#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] -#[serde(rename_all = "kebab-case")] -pub struct SandboxPolicy { - permissions: Vec, +/// Additional configuration for [`SandboxPolicy::WorkspaceWrite`]. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct WorkspaceWriteConfig { + /// Additional folders (beyond cwd and possibly TMPDIR) that should be + /// writable from within the sandbox. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub writable_roots: Vec, + + /// When set to `true`, outbound network access is allowed. `false` by + /// default. + #[serde(default)] + pub network_access: bool, } -impl From> for SandboxPolicy { - fn from(permissions: Vec) -> Self { - Self { permissions } - } +/// Determines execution restrictions for model shell commands. +/// +/// Instead of the previous "bag of permissions" approach, the policy is now +/// chiefly expressed via a *mode* with a handful of per-mode configuration +/// knobs. This makes the user-facing TOML much easier to reason about while +/// still letting us derive the granular permissions required by the lower +/// layers (seccomp / Landlock, etc.). +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case")] +// The `tag = "sandbox"` ensures that a standalone string such as +// `sandbox = "read-only"` in user configuration files still deserialises into +// the enum, while tables like +// +// ```toml +// [sandbox] +// mode = "workspace-write" +// writable_roots = ["/tmp"] +// network_access = true +// ``` +// also work. +pub enum SandboxPolicy { + /// No restrictions whatsoever. Use with caution. + #[serde(rename = "danger-full-access")] + DangerFullAccess, + + /// Read-only access to the entire file-system. Network is *off* by default + /// but can be enabled. + #[serde(rename = "read-only")] + ReadOnly { network_access: bool }, + + /// Same as `ReadOnly` but additionally grants write access to the current + /// working directory ("workspace"). + #[serde(rename = "workspace-write")] + WorkspaceWrite(WorkspaceWriteConfig), } impl SandboxPolicy { + /// Returns a policy with read-only disk access and no network. pub fn new_read_only_policy() -> Self { - Self { - permissions: vec![SandboxPermission::DiskFullReadAccess], + SandboxPolicy::ReadOnly { + network_access: false, } } - pub fn new_read_only_policy_with_writable_roots(writable_roots: &[PathBuf]) -> Self { - let mut permissions = Self::new_read_only_policy().permissions; - permissions.extend(writable_roots.iter().map(|folder| { - SandboxPermission::DiskWriteFolder { - folder: folder.clone(), + /// Returns a policy that can read the entire disk, but can only write to + /// the current working directory and the per-user tmp dir on macOS. It does + /// not allow network access. + pub fn new_workspace_write_policy() -> Self { + let mut writable_roots = vec![]; + + // Also include the per-user tmp dir on macOS. + if cfg!(target_os = "macos") { + if let Some(tmpdir) = std::env::var_os("TMPDIR") { + writable_roots.push(PathBuf::from(tmpdir)); } - })); - Self { permissions } - } - - pub fn new_full_auto_policy() -> Self { - Self { - permissions: vec![ - SandboxPermission::DiskFullReadAccess, - SandboxPermission::DiskWritePlatformUserTempFolder, - SandboxPermission::DiskWriteCwd, - ], } + + SandboxPolicy::WorkspaceWrite(WorkspaceWriteConfig { + writable_roots, + network_access: false, + }) } + /// Always returns `true` for now, as we do not yet support restricting read + /// access. pub fn has_full_disk_read_access(&self) -> bool { - self.permissions - .iter() - .any(|perm| matches!(perm, SandboxPermission::DiskFullReadAccess)) + true } pub fn has_full_disk_write_access(&self) -> bool { - self.permissions - .iter() - .any(|perm| matches!(perm, SandboxPermission::DiskFullWriteAccess)) + match self { + SandboxPolicy::DangerFullAccess => true, + SandboxPolicy::ReadOnly { .. } => false, + SandboxPolicy::WorkspaceWrite(_) => false, + } } pub fn has_full_network_access(&self) -> bool { - self.permissions - .iter() - .any(|perm| matches!(perm, SandboxPermission::NetworkFullAccess)) + match self { + SandboxPolicy::DangerFullAccess => true, + SandboxPolicy::ReadOnly { network_access } => *network_access, + SandboxPolicy::WorkspaceWrite(cfg) => cfg.network_access, + } } + /// Returns the list of writable roots that should be passed down to the + /// Landlock rules installer, tailored to the current working directory. pub fn get_writable_roots_with_cwd(&self, cwd: &Path) -> Vec { - let mut writable_roots = Vec::::new(); - for perm in &self.permissions { - use SandboxPermission::*; - match perm { - DiskWritePlatformUserTempFolder => { - if cfg!(target_os = "macos") { - if let Some(tempdir) = std::env::var_os("TMPDIR") { - // Likely something that starts with /var/folders/... - let tmpdir_path = PathBuf::from(&tempdir); - if tmpdir_path.is_absolute() { - writable_roots.push(tmpdir_path.clone()); - match tmpdir_path.canonicalize() { - Ok(canonicalized) => { - // Likely something that starts with /private/var/folders/... - if canonicalized != tmpdir_path { - writable_roots.push(canonicalized); - } - } - Err(e) => { - tracing::error!("Failed to canonicalize TMPDIR: {e}"); - } - } - } else { - tracing::error!("TMPDIR is not an absolute path: {tempdir:?}"); - } - } - } - - // For Linux, should this be XDG_RUNTIME_DIR, /run/user/, or something else? - } - DiskWritePlatformGlobalTempFolder => { - if cfg!(unix) { - writable_roots.push(PathBuf::from("/tmp")); - } - } - DiskWriteCwd => { - writable_roots.push(cwd.to_path_buf()); - } - DiskWriteFolder { folder } => { - writable_roots.push(folder.clone()); - } - DiskFullReadAccess | NetworkFullAccess => {} - DiskFullWriteAccess => { - // Currently, we expect callers to only invoke this method - // after verifying has_full_disk_write_access() is false. - } + match self { + SandboxPolicy::DangerFullAccess => Vec::new(), + SandboxPolicy::ReadOnly { .. } => Vec::new(), + SandboxPolicy::WorkspaceWrite(cfg) => { + let mut roots = cfg.writable_roots.clone(); + roots.push(cwd.to_path_buf()); + roots } } - writable_roots } + // TODO(mbolin): This conflates sandbox policy and approval policy and + // should go away. pub fn is_unrestricted(&self) -> bool { - self.has_full_disk_read_access() - && self.has_full_disk_write_access() - && self.has_full_network_access() + matches!(self, SandboxPolicy::DangerFullAccess) } } -/// Permissions that should be granted to the sandbox in which the agent -/// operates. -#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] -#[serde(rename_all = "kebab-case")] -pub enum SandboxPermission { - /// Is allowed to read all files on disk. - DiskFullReadAccess, - - /// Is allowed to write to the operating system's temp dir that - /// is restricted to the user the agent is running as. For - /// example, on macOS, this is generally something under - /// `/var/folders` as opposed to `/tmp`. - DiskWritePlatformUserTempFolder, - - /// Is allowed to write to the operating system's shared temp - /// dir. On UNIX, this is generally `/tmp`. - DiskWritePlatformGlobalTempFolder, - - /// Is allowed to write to the current working directory (in practice, this - /// is the `cwd` where `codex` was spawned). - DiskWriteCwd, - - /// Is allowed to the specified folder. `PathBuf` must be an - /// absolute path, though it is up to the caller to canonicalize - /// it if the path contains symlinks. - DiskWriteFolder { folder: PathBuf }, - - /// Is allowed to write to any file on disk. - DiskFullWriteAccess, - - /// Can make arbitrary network requests. - NetworkFullAccess, -} - /// User input #[non_exhaustive] #[derive(Debug, Clone, Deserialize, Serialize, PartialEq)] diff --git a/codex-rs/exec/src/cli.rs b/codex-rs/exec/src/cli.rs index 413fd23cb7..f14b28e702 100644 --- a/codex-rs/exec/src/cli.rs +++ b/codex-rs/exec/src/cli.rs @@ -1,7 +1,6 @@ use clap::Parser; use clap::ValueEnum; use codex_common::CliConfigOverrides; -use codex_common::SandboxPermissionOption; use std::path::PathBuf; #[derive(Parser, Debug)] @@ -23,9 +22,6 @@ pub struct Cli { #[arg(long = "full-auto", default_value_t = false)] pub full_auto: bool, - #[clap(flatten)] - pub sandbox: SandboxPermissionOption, - /// Tell the agent to use the specified directory as its working root. #[clap(long = "cd", short = 'C', value_name = "DIR")] pub cwd: Option, diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index 925e25d670..bd59e117a2 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -31,7 +31,6 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any model, config_profile, full_auto, - sandbox, cwd, skip_git_repo_check, color, @@ -85,9 +84,9 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any }; let sandbox_policy = if full_auto { - Some(SandboxPolicy::new_full_auto_policy()) + Some(SandboxPolicy::new_workspace_write_policy()) } else { - sandbox.permissions.clone().map(Into::into) + None }; // Load configuration and determine approval policy diff --git a/codex-rs/linux-sandbox/src/linux_run_main.rs b/codex-rs/linux-sandbox/src/linux_run_main.rs index a8c73aa75d..7ed35ce020 100644 --- a/codex-rs/linux-sandbox/src/linux_run_main.rs +++ b/codex-rs/linux-sandbox/src/linux_run_main.rs @@ -1,26 +1,19 @@ use clap::Parser; -use codex_common::SandboxPermissionOption; use std::ffi::CString; use crate::landlock::apply_sandbox_policy_to_current_thread; #[derive(Debug, Parser)] pub struct LandlockCommand { - #[clap(flatten)] - pub sandbox: SandboxPermissionOption, - /// Full command args to run under landlock. #[arg(trailing_var_arg = true)] pub command: Vec, } pub fn run_main() -> ! { - let LandlockCommand { sandbox, command } = LandlockCommand::parse(); + let LandlockCommand { command } = LandlockCommand::parse(); - let sandbox_policy = match sandbox.permissions.map(Into::into) { - Some(sandbox_policy) => sandbox_policy, - None => codex_core::protocol::SandboxPolicy::new_read_only_policy(), - }; + let sandbox_policy = codex_core::protocol::SandboxPolicy::new_read_only_policy(); let cwd = match std::env::current_dir() { Ok(cwd) => cwd, diff --git a/codex-rs/linux-sandbox/tests/landlock.rs b/codex-rs/linux-sandbox/tests/landlock.rs index 17bdd9d801..88c6df60b1 100644 --- a/codex-rs/linux-sandbox/tests/landlock.rs +++ b/codex-rs/linux-sandbox/tests/landlock.rs @@ -9,6 +9,7 @@ use codex_core::exec::SandboxType; use codex_core::exec::process_exec_tool_call; use codex_core::exec_env::create_env; use codex_core::protocol::SandboxPolicy; +use codex_core::protocol::WorkspaceWriteConfig; use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; @@ -46,7 +47,10 @@ async fn run_cmd(cmd: &[&str], writable_roots: &[PathBuf], timeout_ms: u64) { env: create_env_from_core_vars(), }; - let sandbox_policy = SandboxPolicy::new_read_only_policy_with_writable_roots(writable_roots); + let sandbox_policy = SandboxPolicy::WorkspaceWrite(WorkspaceWriteConfig { + writable_roots: writable_roots.to_vec(), + network_access: false, + }); let sandbox_program = env!("CARGO_BIN_EXE_codex-linux-sandbox"); let codex_linux_sandbox_exe = Some(PathBuf::from(sandbox_program)); let ctrl_c = Arc::new(Notify::new()); diff --git a/codex-rs/mcp-server/src/codex_tool_config.rs b/codex-rs/mcp-server/src/codex_tool_config.rs index 03e7234449..0afefc15ca 100644 --- a/codex-rs/mcp-server/src/codex_tool_config.rs +++ b/codex-rs/mcp-server/src/codex_tool_config.rs @@ -1,7 +1,6 @@ //! Configuration object accepted by the `codex` MCP tool-call. use codex_core::protocol::AskForApproval; -use codex_core::protocol::SandboxPolicy; use mcp_types::Tool; use mcp_types::ToolInputSchema; use schemars::JsonSchema; @@ -19,7 +18,7 @@ pub(crate) struct CodexToolCallParam { /// The *initial user prompt* to start the Codex conversation. pub prompt: String, - /// Optional override for the model name (e.g. "o3", "o4-mini") + /// Optional override for the model name (e.g. "o3", "o4-mini"). #[serde(default, skip_serializing_if = "Option::is_none")] pub model: Option, @@ -37,22 +36,14 @@ pub(crate) struct CodexToolCallParam { #[serde(default, skip_serializing_if = "Option::is_none")] pub approval_policy: Option, - /// Sandbox permissions using the same string values accepted by the CLI - /// (e.g. "disk-write-cwd", "network-full-access"). - #[serde(default, skip_serializing_if = "Option::is_none")] - pub sandbox_permissions: Option>, - /// Individual config settings that will override what is in /// CODEX_HOME/config.toml. #[serde(default, skip_serializing_if = "Option::is_none")] pub config: Option>, } -// Create custom enums for use with `CodexToolCallApprovalPolicy` where we -// intentionally exclude docstrings from the generated schema because they -// introduce anyOf in the the generated JSON schema, which makes it more complex -// without adding any real value since we aspire to use self-descriptive names. - +// Custom enum mirroring `AskForApproval`, but constrained to the subset we +// expose via the tool-call schema. #[derive(Debug, Clone, Deserialize, JsonSchema)] #[serde(rename_all = "kebab-case")] pub(crate) enum CodexToolCallApprovalPolicy { @@ -73,50 +64,12 @@ impl From for AskForApproval { } } -// TODO: Support additional writable folders via a separate property on -// CodexToolCallParam. - -#[derive(Debug, Clone, Deserialize, JsonSchema)] -#[serde(rename_all = "kebab-case")] -pub(crate) enum CodexToolCallSandboxPermission { - DiskFullReadAccess, - DiskWriteCwd, - DiskWritePlatformUserTempFolder, - DiskWritePlatformGlobalTempFolder, - DiskFullWriteAccess, - NetworkFullAccess, -} - -impl From for codex_core::protocol::SandboxPermission { - fn from(value: CodexToolCallSandboxPermission) -> Self { - match value { - CodexToolCallSandboxPermission::DiskFullReadAccess => { - codex_core::protocol::SandboxPermission::DiskFullReadAccess - } - CodexToolCallSandboxPermission::DiskWriteCwd => { - codex_core::protocol::SandboxPermission::DiskWriteCwd - } - CodexToolCallSandboxPermission::DiskWritePlatformUserTempFolder => { - codex_core::protocol::SandboxPermission::DiskWritePlatformUserTempFolder - } - CodexToolCallSandboxPermission::DiskWritePlatformGlobalTempFolder => { - codex_core::protocol::SandboxPermission::DiskWritePlatformGlobalTempFolder - } - CodexToolCallSandboxPermission::DiskFullWriteAccess => { - codex_core::protocol::SandboxPermission::DiskFullWriteAccess - } - CodexToolCallSandboxPermission::NetworkFullAccess => { - codex_core::protocol::SandboxPermission::NetworkFullAccess - } - } - } -} - +/// Builds a `Tool` definition (JSON schema etc.) for the Codex tool-call. pub(crate) fn create_tool_for_codex_tool_call_param() -> Tool { let schema = SchemaSettings::draft2019_09() .with(|s| { s.inline_subschemas = true; - s.option_add_null_type = false + s.option_add_null_type = false; }) .into_generator() .into_root_schema_for::(); @@ -129,12 +82,12 @@ pub(crate) fn create_tool_for_codex_tool_call_param() -> Tool { serde_json::from_value::(schema_value).unwrap_or_else(|e| { panic!("failed to create Tool from schema: {e}"); }); + Tool { name: "codex".to_string(), input_schema: tool_input_schema, description: Some( - "Run a Codex session. Accepts configuration parameters matching the Codex Config struct." - .to_string(), + "Run a Codex session. Accepts configuration parameters matching the Codex Config struct.".to_string(), ), annotations: None, } @@ -142,7 +95,7 @@ pub(crate) fn create_tool_for_codex_tool_call_param() -> Tool { impl CodexToolCallParam { /// Returns the initial user prompt to start the Codex conversation and the - /// Config. + /// effective Config object generated from the supplied parameters. pub fn into_config( self, codex_linux_sandbox_exe: Option, @@ -153,20 +106,18 @@ impl CodexToolCallParam { profile, cwd, approval_policy, - sandbox_permissions, config: cli_overrides, } = self; - let sandbox_policy = sandbox_permissions.map(|perms| { - SandboxPolicy::from(perms.into_iter().map(Into::into).collect::>()) - }); - // Build ConfigOverrides recognised by codex-core. + // Build the `ConfigOverrides` recognised by codex-core. let overrides = codex_core::config::ConfigOverrides { model, config_profile: profile, cwd: cwd.map(PathBuf::from), approval_policy: approval_policy.map(Into::into), - sandbox_policy, + // Note we may want to expose a field on CodexToolCallParam to + // facilitate configuring the sandbox policy. + sandbox_policy: None, model_provider: None, codex_linux_sandbox_exe, }; @@ -230,7 +181,7 @@ mod tests { "type": "string" }, "model": { - "description": "Optional override for the model name (e.g. \"o3\", \"o4-mini\")", + "description": "Optional override for the model name (e.g. \"o3\", \"o4-mini\").", "type": "string" }, "profile": { @@ -241,21 +192,6 @@ mod tests { "description": "The *initial user prompt* to start the Codex conversation.", "type": "string" }, - "sandbox-permissions": { - "description": "Sandbox permissions using the same string values accepted by the CLI (e.g. \"disk-write-cwd\", \"network-full-access\").", - "items": { - "enum": [ - "disk-full-read-access", - "disk-write-cwd", - "disk-write-platform-user-temp-folder", - "disk-write-platform-global-temp-folder", - "disk-full-write-access", - "network-full-access" - ], - "type": "string" - }, - "type": "array" - } }, "required": [ "prompt" diff --git a/codex-rs/tui/src/cli.rs b/codex-rs/tui/src/cli.rs index 4abd684144..e4ee752ba9 100644 --- a/codex-rs/tui/src/cli.rs +++ b/codex-rs/tui/src/cli.rs @@ -1,7 +1,6 @@ use clap::Parser; use codex_common::ApprovalModeCliArg; use codex_common::CliConfigOverrides; -use codex_common::SandboxPermissionOption; use std::path::PathBuf; #[derive(Parser, Debug)] @@ -30,9 +29,6 @@ pub struct Cli { #[arg(long = "full-auto", default_value_t = false)] pub full_auto: bool, - #[clap(flatten)] - pub sandbox: SandboxPermissionOption, - /// Tell the agent to use the specified directory as its working root. #[clap(long = "cd", short = 'C', value_name = "DIR")] pub cwd: Option, diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 5f3e2d69b5..78fc283461 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -48,11 +48,11 @@ pub use cli::Cli; pub fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> std::io::Result<()> { let (sandbox_policy, approval_policy) = if cli.full_auto { ( - Some(SandboxPolicy::new_full_auto_policy()), + Some(SandboxPolicy::new_workspace_write_policy()), Some(AskForApproval::OnFailure), ) } else { - let sandbox_policy = cli.sandbox.permissions.clone().map(Into::into); + let sandbox_policy = None; (sandbox_policy, cli.approval_policy.map(Into::into)) };