From 790213ded0588d824e99f830f41f04f3d98196df Mon Sep 17 00:00:00 2001 From: jif Date: Thu, 18 Jun 2026 18:31:07 +0100 Subject: [PATCH] Scope MCP sandbox metadata to server environment (#28914) Scope MCP sandbox metadata to the MCP server's owning environment. Previously, `codex/sandbox-state-meta` always used the turn's primary cwd and rebuilt a legacy sandbox policy from that cwd. That can be wrong for MCP servers owned by a different execution environment. This now sends the owning environment cwd as a `file:` URI in `sandboxCwd`, keeps `permissionProfile` as the permission source of truth, and omits sandbox-state metadata when a non-default server environment is not selected for the turn. Local/default MCP servers keep the existing fallback cwd behavior. Tests: - `just fmt` - `just bazel-lock-update` - `just bazel-lock-check` - `just test -p codex-mcp` - `just test -p codex-core mcp_sandbox_cwd` - `cargo build -p codex-rmcp-client --bin test_stdio_server` - `just test -p codex-core stdio_mcp_tool_call_includes_sandbox_state_meta` --- codex-rs/Cargo.lock | 1 + codex-rs/codex-mcp/Cargo.toml | 1 + codex-rs/codex-mcp/src/connection_manager.rs | 6 +++ .../codex-mcp/src/connection_manager_tests.rs | 3 ++ codex-rs/codex-mcp/src/runtime.rs | 5 +-- codex-rs/codex-mcp/src/server.rs | 2 + codex-rs/core/src/mcp_tool_call.rs | 38 +++++++++++++++---- codex-rs/core/src/mcp_tool_call_tests.rs | 35 +++++++++++++++++ codex-rs/core/tests/suite/rmcp_client.rs | 11 ++---- 9 files changed, 83 insertions(+), 19 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 844a84ef7..1cc32ea68 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3289,6 +3289,7 @@ dependencies = [ "codex-plugin", "codex-protocol", "codex-rmcp-client", + "codex-utils-path-uri", "codex-utils-plugins", "futures", "pretty_assertions", diff --git a/codex-rs/codex-mcp/Cargo.toml b/codex-rs/codex-mcp/Cargo.toml index 433cbde8b..15866e49e 100644 --- a/codex-rs/codex-mcp/Cargo.toml +++ b/codex-rs/codex-mcp/Cargo.toml @@ -26,6 +26,7 @@ codex-otel = { workspace = true } codex-plugin = { workspace = true } codex-protocol = { workspace = true } codex-rmcp-client = { workspace = true } +codex-utils-path-uri = { workspace = true } codex-utils-plugins = { workspace = true } futures = { workspace = true } regex-lite = { workspace = true } diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index 359648656..e50532714 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -370,6 +370,12 @@ impl McpConnectionManager { .map(super::server::McpServerOrigin::as_str) } + pub fn server_environment_id(&self, server_name: &str) -> Option<&str> { + self.server_metadata + .get(server_name) + .map(|metadata| metadata.environment_id.as_str()) + } + pub fn server_pollutes_memory(&self, server_name: &str) -> bool { self.server_metadata .get(server_name) diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index bfbc09c47..20bb8d43c 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -1127,6 +1127,7 @@ async fn list_all_tools_adds_server_metadata_to_cached_tools() { manager.server_metadata.insert( server_name.to_string(), McpServerMetadata { + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), pollutes_memory: true, origin: Some(McpServerOrigin::StreamableHttp( "https://docs.example".to_string(), @@ -1162,6 +1163,7 @@ fn server_metadata_preserves_tool_approval_policy() { "https://docs.example", /*apps_mcp_product_sku*/ None, ); + config.environment_id = "remote".to_string(); config.default_tools_approval_mode = Some(AppToolApproval::Prompt); config.tools.insert( "search".to_string(), @@ -1171,6 +1173,7 @@ fn server_metadata_preserves_tool_approval_policy() { ); let metadata = McpServerMetadata::from(&EffectiveMcpServer::configured(config)); + assert_eq!(metadata.environment_id, "remote"); assert_eq!(metadata.tool_approval_mode("read"), AppToolApproval::Prompt); assert_eq!( metadata.tool_approval_mode("search"), diff --git a/codex-rs/codex-mcp/src/runtime.rs b/codex-rs/codex-mcp/src/runtime.rs index 5404bf1eb..ff5c65db0 100644 --- a/codex-rs/codex-mcp/src/runtime.rs +++ b/codex-rs/codex-mcp/src/runtime.rs @@ -12,7 +12,7 @@ use std::time::Duration; use codex_exec_server::Environment; use codex_exec_server::EnvironmentManager; use codex_protocol::models::PermissionProfile; -use codex_protocol::protocol::SandboxPolicy; +use codex_utils_path_uri::PathUri; use serde::Deserialize; use serde::Serialize; @@ -22,9 +22,8 @@ use serde::Serialize; pub struct SandboxState { #[serde(default, skip_serializing_if = "Option::is_none")] pub permission_profile: Option, - pub sandbox_policy: SandboxPolicy, pub codex_linux_sandbox_exe: Option, - pub sandbox_cwd: PathBuf, + pub sandbox_cwd: PathUri, #[serde(default)] pub use_legacy_landlock: bool, } diff --git a/codex-rs/codex-mcp/src/server.rs b/codex-rs/codex-mcp/src/server.rs index 90952cbfa..39287b213 100644 --- a/codex-rs/codex-mcp/src/server.rs +++ b/codex-rs/codex-mcp/src/server.rs @@ -75,6 +75,7 @@ impl McpServerOrigin { /// Semantic metadata that must survive after the server is launched. #[derive(Debug, Clone)] pub(crate) struct McpServerMetadata { + pub environment_id: String, pub pollutes_memory: bool, pub origin: Option, pub supports_parallel_tool_calls: bool, @@ -96,6 +97,7 @@ impl From<&EffectiveMcpServer> for McpServerMetadata { fn from(server: &EffectiveMcpServer) -> Self { match server.launch() { McpServerLaunch::Configured(config) => Self { + environment_id: config.environment_id.clone(), pollutes_memory: true, origin: McpServerOrigin::from_transport(&config.transport), supports_parallel_tool_calls: config.supports_parallel_tool_calls, diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index e8cbb56c5..a89951542 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -81,6 +81,7 @@ use codex_rollout::state_db; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_output_truncation::TruncationPolicy; use codex_utils_output_truncation::truncate_text; +use codex_utils_path_uri::PathUri; use codex_utils_pty::DEFAULT_OUTPUT_BYTES_CAP; use rmcp::model::ToolAnnotations; use serde::Deserialize; @@ -713,10 +714,8 @@ async fn augment_mcp_tool_request_meta_with_sandbox_state( server: &str, mut meta: Option, ) -> anyhow::Result> { - let supports_sandbox_state_meta = sess - .services - .mcp_connection_manager - .load_full() + let mcp_connection_manager = sess.services.mcp_connection_manager.load_full(); + let supports_sandbox_state_meta = mcp_connection_manager .server_supports_sandbox_state_meta_capability(server) .await .unwrap_or(false); @@ -724,12 +723,17 @@ async fn augment_mcp_tool_request_meta_with_sandbox_state( return Ok(meta); } + let server_environment_id = mcp_connection_manager + .server_environment_id(server) + .unwrap_or(codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID); + let Some(sandbox_cwd) = sandbox_cwd_for_mcp_server(turn_context, server_environment_id) else { + return Ok(meta); + }; + let permission_profile = turn_context.permission_profile(); let sandbox_state = serde_json::to_value(SandboxState { - permission_profile: Some(turn_context.permission_profile()), - sandbox_policy: turn_context.sandbox_policy(), + permission_profile: Some(permission_profile), codex_linux_sandbox_exe: turn_context.config.codex_linux_sandbox_exe.clone(), - #[allow(deprecated)] - sandbox_cwd: turn_context.cwd.to_path_buf(), + sandbox_cwd, use_legacy_landlock: turn_context.config.features.use_legacy_landlock(), })?; @@ -754,6 +758,24 @@ async fn augment_mcp_tool_request_meta_with_sandbox_state( Ok(meta) } +fn sandbox_cwd_for_mcp_server(turn_context: &TurnContext, environment_id: &str) -> Option { + if let Some(environment) = turn_context + .environments + .turn_environments + .iter() + .find(|environment| environment.environment_id == environment_id) + { + return Some(environment.cwd().clone()); + } + + if environment_id == codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID { + #[allow(deprecated)] + return Some(PathUri::from_abs_path(&turn_context.cwd)); + } + + None +} + async fn maybe_mark_thread_memory_mode_polluted( sess: &Session, turn_context: &TurnContext, diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 56d1ff9b6..4249c0d4b 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -3,6 +3,7 @@ use crate::config::ConfigBuilder; use crate::config::ManagedFeatures; use crate::session::tests::make_session_and_context; use crate::session::tests::make_session_and_context_with_rx; +use crate::session::turn_context::TurnEnvironment; use crate::state::ActiveTurn; use crate::test_support::models_manager_with_provider; use crate::tools::hook_names::HookToolName; @@ -31,6 +32,7 @@ use codex_rollout_trace::ToolDispatchInvocation; use codex_rollout_trace::ToolDispatchPayload; use codex_rollout_trace::ToolDispatchRequester; use codex_rollout_trace::replay_bundle; +use codex_utils_path_uri::PathUri; use core_test_support::hooks::trusted_config_layer_stack; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; @@ -1091,6 +1093,39 @@ async fn mcp_tool_call_request_meta_includes_turn_started_at_unix_ms() { ); } +#[tokio::test] +async fn mcp_sandbox_cwd_uses_matching_server_environment_uri() -> anyhow::Result<()> { + let (_, mut turn_context) = make_session_and_context().await; + let secondary_cwd = PathUri::parse("file:///C:/remote/project")?; + let environment = turn_context.environments.turn_environments[0] + .environment + .clone(); + turn_context + .environments + .turn_environments + .push(TurnEnvironment::new( + "remote".to_string(), + environment, + secondary_cwd.clone(), + /*shell*/ None, + )); + + let sandbox_cwd = sandbox_cwd_for_mcp_server(&turn_context, "remote"); + + assert_eq!(sandbox_cwd, Some(secondary_cwd)); + Ok(()) +} + +#[tokio::test] +async fn mcp_sandbox_cwd_is_none_for_unselected_server_environment() -> anyhow::Result<()> { + let (_, turn_context) = make_session_and_context().await; + + let sandbox_cwd = sandbox_cwd_for_mcp_server(&turn_context, "remote"); + + assert_eq!(sandbox_cwd, None); + Ok(()) +} + #[tokio::test] async fn plugin_mcp_tool_call_request_meta_includes_plugin_id() { let (_, turn_context) = make_session_and_context().await; diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 2be014b3f..656ca6931 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -815,16 +815,11 @@ async fn stdio_mcp_tool_call_includes_sandbox_state_meta() -> anyhow::Result<()> let sandbox_meta = meta .get(MCP_SANDBOX_STATE_META_CAPABILITY) .expect("sandbox state metadata should be present"); - let (sandbox_policy, _) = - turn_permission_fields(PermissionProfile::read_only(), fixture.config.cwd.as_path()); - let expected_sandbox_policy = serde_json::to_value(&sandbox_policy)?; - assert_eq!( - sandbox_meta.get("sandboxPolicy"), - Some(&expected_sandbox_policy) - ); + assert_eq!(sandbox_meta.get("sandboxPolicy"), None); + let expected_sandbox_cwd = PathUri::from_abs_path(&fixture.config.cwd).to_string(); assert_eq!( sandbox_meta.get("sandboxCwd").and_then(Value::as_str), - fixture.config.cwd.as_path().to_str() + Some(expected_sandbox_cwd.as_str()) ); assert_eq!(sandbox_meta.get("useLegacyLandlock"), Some(&json!(false)));