mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
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`
This commit is contained in:
9 files changed
+83
-19
No files matched your search
Generated
+1
@@ -3289,6 +3289,7 @@ dependencies = [
|
|||||||
"codex-plugin",
|
"codex-plugin",
|
||||||
"codex-protocol",
|
"codex-protocol",
|
||||||
"codex-rmcp-client",
|
"codex-rmcp-client",
|
||||||
|
"codex-utils-path-uri",
|
||||||
"codex-utils-plugins",
|
"codex-utils-plugins",
|
||||||
"futures",
|
"futures",
|
||||||
"pretty_assertions",
|
"pretty_assertions",
|
||||||
|
|||||||
@@ -26,6 +26,7 @@ codex-otel = { workspace = true }
|
|||||||
codex-plugin = { workspace = true }
|
codex-plugin = { workspace = true }
|
||||||
codex-protocol = { workspace = true }
|
codex-protocol = { workspace = true }
|
||||||
codex-rmcp-client = { workspace = true }
|
codex-rmcp-client = { workspace = true }
|
||||||
|
codex-utils-path-uri = { workspace = true }
|
||||||
codex-utils-plugins = { workspace = true }
|
codex-utils-plugins = { workspace = true }
|
||||||
futures = { workspace = true }
|
futures = { workspace = true }
|
||||||
regex-lite = { workspace = true }
|
regex-lite = { workspace = true }
|
||||||
|
|||||||
@@ -370,6 +370,12 @@ impl McpConnectionManager {
|
|||||||
.map(super::server::McpServerOrigin::as_str)
|
.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 {
|
pub fn server_pollutes_memory(&self, server_name: &str) -> bool {
|
||||||
self.server_metadata
|
self.server_metadata
|
||||||
.get(server_name)
|
.get(server_name)
|
||||||
|
|||||||
@@ -1127,6 +1127,7 @@ async fn list_all_tools_adds_server_metadata_to_cached_tools() {
|
|||||||
manager.server_metadata.insert(
|
manager.server_metadata.insert(
|
||||||
server_name.to_string(),
|
server_name.to_string(),
|
||||||
McpServerMetadata {
|
McpServerMetadata {
|
||||||
|
environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(),
|
||||||
pollutes_memory: true,
|
pollutes_memory: true,
|
||||||
origin: Some(McpServerOrigin::StreamableHttp(
|
origin: Some(McpServerOrigin::StreamableHttp(
|
||||||
"https://docs.example".to_string(),
|
"https://docs.example".to_string(),
|
||||||
@@ -1162,6 +1163,7 @@ fn server_metadata_preserves_tool_approval_policy() {
|
|||||||
"https://docs.example",
|
"https://docs.example",
|
||||||
/*apps_mcp_product_sku*/ None,
|
/*apps_mcp_product_sku*/ None,
|
||||||
);
|
);
|
||||||
|
config.environment_id = "remote".to_string();
|
||||||
config.default_tools_approval_mode = Some(AppToolApproval::Prompt);
|
config.default_tools_approval_mode = Some(AppToolApproval::Prompt);
|
||||||
config.tools.insert(
|
config.tools.insert(
|
||||||
"search".to_string(),
|
"search".to_string(),
|
||||||
@@ -1171,6 +1173,7 @@ fn server_metadata_preserves_tool_approval_policy() {
|
|||||||
);
|
);
|
||||||
let metadata = McpServerMetadata::from(&EffectiveMcpServer::configured(config));
|
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("read"), AppToolApproval::Prompt);
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
metadata.tool_approval_mode("search"),
|
metadata.tool_approval_mode("search"),
|
||||||
|
|||||||
@@ -12,7 +12,7 @@ use std::time::Duration;
|
|||||||
use codex_exec_server::Environment;
|
use codex_exec_server::Environment;
|
||||||
use codex_exec_server::EnvironmentManager;
|
use codex_exec_server::EnvironmentManager;
|
||||||
use codex_protocol::models::PermissionProfile;
|
use codex_protocol::models::PermissionProfile;
|
||||||
use codex_protocol::protocol::SandboxPolicy;
|
use codex_utils_path_uri::PathUri;
|
||||||
|
|
||||||
use serde::Deserialize;
|
use serde::Deserialize;
|
||||||
use serde::Serialize;
|
use serde::Serialize;
|
||||||
@@ -22,9 +22,8 @@ use serde::Serialize;
|
|||||||
pub struct SandboxState {
|
pub struct SandboxState {
|
||||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||||
pub permission_profile: Option<PermissionProfile>,
|
pub permission_profile: Option<PermissionProfile>,
|
||||||
pub sandbox_policy: SandboxPolicy,
|
|
||||||
pub codex_linux_sandbox_exe: Option<PathBuf>,
|
pub codex_linux_sandbox_exe: Option<PathBuf>,
|
||||||
pub sandbox_cwd: PathBuf,
|
pub sandbox_cwd: PathUri,
|
||||||
#[serde(default)]
|
#[serde(default)]
|
||||||
pub use_legacy_landlock: bool,
|
pub use_legacy_landlock: bool,
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -75,6 +75,7 @@ impl McpServerOrigin {
|
|||||||
/// Semantic metadata that must survive after the server is launched.
|
/// Semantic metadata that must survive after the server is launched.
|
||||||
#[derive(Debug, Clone)]
|
#[derive(Debug, Clone)]
|
||||||
pub(crate) struct McpServerMetadata {
|
pub(crate) struct McpServerMetadata {
|
||||||
|
pub environment_id: String,
|
||||||
pub pollutes_memory: bool,
|
pub pollutes_memory: bool,
|
||||||
pub origin: Option<McpServerOrigin>,
|
pub origin: Option<McpServerOrigin>,
|
||||||
pub supports_parallel_tool_calls: bool,
|
pub supports_parallel_tool_calls: bool,
|
||||||
@@ -96,6 +97,7 @@ impl From<&EffectiveMcpServer> for McpServerMetadata {
|
|||||||
fn from(server: &EffectiveMcpServer) -> Self {
|
fn from(server: &EffectiveMcpServer) -> Self {
|
||||||
match server.launch() {
|
match server.launch() {
|
||||||
McpServerLaunch::Configured(config) => Self {
|
McpServerLaunch::Configured(config) => Self {
|
||||||
|
environment_id: config.environment_id.clone(),
|
||||||
pollutes_memory: true,
|
pollutes_memory: true,
|
||||||
origin: McpServerOrigin::from_transport(&config.transport),
|
origin: McpServerOrigin::from_transport(&config.transport),
|
||||||
supports_parallel_tool_calls: config.supports_parallel_tool_calls,
|
supports_parallel_tool_calls: config.supports_parallel_tool_calls,
|
||||||
|
|||||||
@@ -81,6 +81,7 @@ use codex_rollout::state_db;
|
|||||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||||
use codex_utils_output_truncation::TruncationPolicy;
|
use codex_utils_output_truncation::TruncationPolicy;
|
||||||
use codex_utils_output_truncation::truncate_text;
|
use codex_utils_output_truncation::truncate_text;
|
||||||
|
use codex_utils_path_uri::PathUri;
|
||||||
use codex_utils_pty::DEFAULT_OUTPUT_BYTES_CAP;
|
use codex_utils_pty::DEFAULT_OUTPUT_BYTES_CAP;
|
||||||
use rmcp::model::ToolAnnotations;
|
use rmcp::model::ToolAnnotations;
|
||||||
use serde::Deserialize;
|
use serde::Deserialize;
|
||||||
@@ -713,10 +714,8 @@ async fn augment_mcp_tool_request_meta_with_sandbox_state(
|
|||||||
server: &str,
|
server: &str,
|
||||||
mut meta: Option<serde_json::Value>,
|
mut meta: Option<serde_json::Value>,
|
||||||
) -> anyhow::Result<Option<serde_json::Value>> {
|
) -> anyhow::Result<Option<serde_json::Value>> {
|
||||||
let supports_sandbox_state_meta = sess
|
let mcp_connection_manager = sess.services.mcp_connection_manager.load_full();
|
||||||
.services
|
let supports_sandbox_state_meta = mcp_connection_manager
|
||||||
.mcp_connection_manager
|
|
||||||
.load_full()
|
|
||||||
.server_supports_sandbox_state_meta_capability(server)
|
.server_supports_sandbox_state_meta_capability(server)
|
||||||
.await
|
.await
|
||||||
.unwrap_or(false);
|
.unwrap_or(false);
|
||||||
@@ -724,12 +723,17 @@ async fn augment_mcp_tool_request_meta_with_sandbox_state(
|
|||||||
return Ok(meta);
|
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 {
|
let sandbox_state = serde_json::to_value(SandboxState {
|
||||||
permission_profile: Some(turn_context.permission_profile()),
|
permission_profile: Some(permission_profile),
|
||||||
sandbox_policy: turn_context.sandbox_policy(),
|
|
||||||
codex_linux_sandbox_exe: turn_context.config.codex_linux_sandbox_exe.clone(),
|
codex_linux_sandbox_exe: turn_context.config.codex_linux_sandbox_exe.clone(),
|
||||||
#[allow(deprecated)]
|
sandbox_cwd,
|
||||||
sandbox_cwd: turn_context.cwd.to_path_buf(),
|
|
||||||
use_legacy_landlock: turn_context.config.features.use_legacy_landlock(),
|
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)
|
Ok(meta)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn sandbox_cwd_for_mcp_server(turn_context: &TurnContext, environment_id: &str) -> Option<PathUri> {
|
||||||
|
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(
|
async fn maybe_mark_thread_memory_mode_polluted(
|
||||||
sess: &Session,
|
sess: &Session,
|
||||||
turn_context: &TurnContext,
|
turn_context: &TurnContext,
|
||||||
|
|||||||
@@ -3,6 +3,7 @@ use crate::config::ConfigBuilder;
|
|||||||
use crate::config::ManagedFeatures;
|
use crate::config::ManagedFeatures;
|
||||||
use crate::session::tests::make_session_and_context;
|
use crate::session::tests::make_session_and_context;
|
||||||
use crate::session::tests::make_session_and_context_with_rx;
|
use crate::session::tests::make_session_and_context_with_rx;
|
||||||
|
use crate::session::turn_context::TurnEnvironment;
|
||||||
use crate::state::ActiveTurn;
|
use crate::state::ActiveTurn;
|
||||||
use crate::test_support::models_manager_with_provider;
|
use crate::test_support::models_manager_with_provider;
|
||||||
use crate::tools::hook_names::HookToolName;
|
use crate::tools::hook_names::HookToolName;
|
||||||
@@ -31,6 +32,7 @@ use codex_rollout_trace::ToolDispatchInvocation;
|
|||||||
use codex_rollout_trace::ToolDispatchPayload;
|
use codex_rollout_trace::ToolDispatchPayload;
|
||||||
use codex_rollout_trace::ToolDispatchRequester;
|
use codex_rollout_trace::ToolDispatchRequester;
|
||||||
use codex_rollout_trace::replay_bundle;
|
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::hooks::trusted_config_layer_stack;
|
||||||
use core_test_support::responses::ev_assistant_message;
|
use core_test_support::responses::ev_assistant_message;
|
||||||
use core_test_support::responses::ev_completed;
|
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]
|
#[tokio::test]
|
||||||
async fn plugin_mcp_tool_call_request_meta_includes_plugin_id() {
|
async fn plugin_mcp_tool_call_request_meta_includes_plugin_id() {
|
||||||
let (_, turn_context) = make_session_and_context().await;
|
let (_, turn_context) = make_session_and_context().await;
|
||||||
|
|||||||
@@ -815,16 +815,11 @@ async fn stdio_mcp_tool_call_includes_sandbox_state_meta() -> anyhow::Result<()>
|
|||||||
let sandbox_meta = meta
|
let sandbox_meta = meta
|
||||||
.get(MCP_SANDBOX_STATE_META_CAPABILITY)
|
.get(MCP_SANDBOX_STATE_META_CAPABILITY)
|
||||||
.expect("sandbox state metadata should be present");
|
.expect("sandbox state metadata should be present");
|
||||||
let (sandbox_policy, _) =
|
assert_eq!(sandbox_meta.get("sandboxPolicy"), None);
|
||||||
turn_permission_fields(PermissionProfile::read_only(), fixture.config.cwd.as_path());
|
let expected_sandbox_cwd = PathUri::from_abs_path(&fixture.config.cwd).to_string();
|
||||||
let expected_sandbox_policy = serde_json::to_value(&sandbox_policy)?;
|
|
||||||
assert_eq!(
|
|
||||||
sandbox_meta.get("sandboxPolicy"),
|
|
||||||
Some(&expected_sandbox_policy)
|
|
||||||
);
|
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
sandbox_meta.get("sandboxCwd").and_then(Value::as_str),
|
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)));
|
assert_eq!(sandbox_meta.get("useLegacyLandlock"), Some(&json!(false)));
|
||||||
|
|
||||||
|
|||||||
Reference in new issue
Block a user