mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
mcp: remove codex/sandbox-state custom request support (#17957)
## Why #17763 moved sandbox-state delivery for MCP tool calls to request `_meta` via the `codex/sandbox-state-meta` experimental capability. Keeping the older `codex/sandbox-state` capability meant Codex still maintained a second transport that pushed updates with the custom `codex/sandbox-state/update` request at server startup and when the session sandbox policy changed. That duplicate MCP path is redundant with the per-tool-call metadata path and makes the sandbox-state contract larger than needed. The existing managed network proxy refresh on sandbox-policy changes is still needed, so this keeps that behavior separate from the removed MCP notification. ## What Changed - Removed the exported `MCP_SANDBOX_STATE_CAPABILITY` and `MCP_SANDBOX_STATE_METHOD` constants. - Removed detection of `codex/sandbox-state` during MCP initialization and stopped sending `codex/sandbox-state/update` at server startup. - Removed the `McpConnectionManager::notify_sandbox_state_change` plumbing while preserving the managed network proxy refresh when a user turn changes sandbox policy. - Slimmed `McpConnectionManager::new` so startup paths pass only the initial `SandboxPolicy` needed for MCP elicitation state. - Kept `codex/sandbox-state-meta` support intact; servers that opt in still receive the current `SandboxState` on tool-call request `_meta` ([remaining call path](https://github.com/openai/codex/blob/ff2d3c1e72ff08ce13743b99605d19d338edd51c/codex-rs/core/src/mcp_tool_call.rs#L487-L526)). - Added regression coverage for refreshing the live managed network proxy on a per-turn sandbox-policy change. ## Verification - `cargo test -p codex-core new_turn_refreshes_managed_network_proxy_for_sandbox_change` - `cargo test -p codex-mcp`
This commit is contained in:
committed by
GitHub
Unverified
parent
83abf67d20
commit
66533ddc61
@@ -78,7 +78,6 @@ use codex_login::CodexAuth;
|
||||
use codex_login::auth_env_telemetry::collect_auth_env_telemetry;
|
||||
use codex_login::default_client::originator;
|
||||
use codex_mcp::McpConnectionManager;
|
||||
use codex_mcp::SandboxState;
|
||||
use codex_mcp::ToolInfo;
|
||||
use codex_mcp::codex_apps_tools_cache_key;
|
||||
#[cfg(test)]
|
||||
@@ -2212,14 +2211,6 @@ impl Session {
|
||||
// Start the watcher after SessionConfigured so it cannot emit earlier events.
|
||||
sess.start_skills_watcher_listener();
|
||||
sess.start_agent_identity_registration();
|
||||
// Construct sandbox_state before MCP startup so it can be sent to each
|
||||
// MCP server immediately after it becomes ready (avoiding blocking).
|
||||
let sandbox_state = SandboxState {
|
||||
sandbox_policy: session_configuration.sandbox_policy.get().clone(),
|
||||
codex_linux_sandbox_exe: config.codex_linux_sandbox_exe.clone(),
|
||||
sandbox_cwd: session_configuration.cwd.to_path_buf(),
|
||||
use_legacy_landlock: config.features.use_legacy_landlock(),
|
||||
};
|
||||
let mut required_mcp_servers: Vec<String> = mcp_servers
|
||||
.iter()
|
||||
.filter(|(_, server)| server.enabled && server.required)
|
||||
@@ -2241,7 +2232,7 @@ impl Session {
|
||||
&session_configuration.approval_policy,
|
||||
INITIAL_SUBMIT_ID.to_owned(),
|
||||
tx_event.clone(),
|
||||
sandbox_state,
|
||||
session_configuration.sandbox_policy.get().clone(),
|
||||
config.codex_home.to_path_buf(),
|
||||
codex_apps_tools_cache_key(auth),
|
||||
tool_plugin_provenance,
|
||||
@@ -2673,12 +2664,16 @@ impl Session {
|
||||
&session_source,
|
||||
);
|
||||
|
||||
if sandbox_policy_changed {
|
||||
self.refresh_managed_network_proxy_for_current_sandbox_policy()
|
||||
.await;
|
||||
}
|
||||
|
||||
Ok(self
|
||||
.new_turn_from_configuration(
|
||||
sub_id,
|
||||
session_configuration,
|
||||
updates.final_output_json_schema,
|
||||
sandbox_policy_changed,
|
||||
)
|
||||
.await)
|
||||
}
|
||||
@@ -2688,7 +2683,6 @@ impl Session {
|
||||
sub_id: String,
|
||||
session_configuration: SessionConfiguration,
|
||||
final_output_json_schema: Option<Option<Value>>,
|
||||
sandbox_policy_changed: bool,
|
||||
) -> Arc<TurnContext> {
|
||||
let per_turn_config = Self::build_per_turn_config(&session_configuration);
|
||||
{
|
||||
@@ -2698,27 +2692,6 @@ impl Session {
|
||||
.set_sandbox_policy(per_turn_config.permissions.sandbox_policy.get());
|
||||
}
|
||||
|
||||
if sandbox_policy_changed {
|
||||
self.refresh_managed_network_proxy_for_current_sandbox_policy()
|
||||
.await;
|
||||
let sandbox_state = SandboxState {
|
||||
sandbox_policy: per_turn_config.permissions.sandbox_policy.get().clone(),
|
||||
codex_linux_sandbox_exe: per_turn_config.codex_linux_sandbox_exe.clone(),
|
||||
sandbox_cwd: per_turn_config.cwd.to_path_buf(),
|
||||
use_legacy_landlock: per_turn_config.features.use_legacy_landlock(),
|
||||
};
|
||||
if let Err(e) = self
|
||||
.services
|
||||
.mcp_connection_manager
|
||||
.read()
|
||||
.await
|
||||
.notify_sandbox_state_change(&sandbox_state)
|
||||
.await
|
||||
{
|
||||
warn!("Failed to notify sandbox state change to MCP servers: {e:#}");
|
||||
}
|
||||
}
|
||||
|
||||
let model_info = self
|
||||
.services
|
||||
.models_manager
|
||||
@@ -2867,7 +2840,6 @@ impl Session {
|
||||
sub_id,
|
||||
session_configuration,
|
||||
/*final_output_json_schema*/ None,
|
||||
/*sandbox_policy_changed*/ false,
|
||||
)
|
||||
.await
|
||||
}
|
||||
@@ -4579,12 +4551,6 @@ impl Session {
|
||||
.await;
|
||||
let mcp_servers = with_codex_apps_mcp(mcp_servers, auth.as_ref(), &mcp_config);
|
||||
let auth_statuses = compute_auth_statuses(mcp_servers.iter(), store_mode).await;
|
||||
let sandbox_state = SandboxState {
|
||||
sandbox_policy: turn_context.sandbox_policy.get().clone(),
|
||||
codex_linux_sandbox_exe: turn_context.codex_linux_sandbox_exe.clone(),
|
||||
sandbox_cwd: turn_context.cwd.to_path_buf(),
|
||||
use_legacy_landlock: turn_context.features.use_legacy_landlock(),
|
||||
};
|
||||
{
|
||||
let mut guard = self.services.mcp_startup_cancellation_token.lock().await;
|
||||
guard.cancel();
|
||||
@@ -4597,7 +4563,7 @@ impl Session {
|
||||
&turn_context.config.permissions.approval_policy,
|
||||
turn_context.sub_id.clone(),
|
||||
self.get_tx_event(),
|
||||
sandbox_state,
|
||||
turn_context.sandbox_policy.get().clone(),
|
||||
config.codex_home.to_path_buf(),
|
||||
codex_apps_tools_cache_key(auth.as_ref()),
|
||||
tool_plugin_provenance,
|
||||
|
||||
@@ -656,6 +656,89 @@ async fn managed_network_proxy_decider_survives_full_access_start() -> anyhow::R
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn new_turn_refreshes_managed_network_proxy_for_sandbox_change() -> anyhow::Result<()> {
|
||||
let (mut session, _turn_context) = make_session_and_context().await;
|
||||
let initial_policy = SandboxPolicy::new_workspace_write_policy();
|
||||
|
||||
let mut network_config = NetworkProxyConfig::default();
|
||||
network_config
|
||||
.network
|
||||
.set_allowed_domains(vec!["evil.com".to_string()]);
|
||||
let requirements = NetworkConstraints {
|
||||
domains: Some(NetworkDomainPermissionsToml {
|
||||
entries: std::collections::BTreeMap::from([(
|
||||
"*.example.com".to_string(),
|
||||
NetworkDomainPermissionToml::Allow,
|
||||
)]),
|
||||
}),
|
||||
..Default::default()
|
||||
};
|
||||
let spec = crate::config::NetworkProxySpec::from_config_and_constraints(
|
||||
network_config,
|
||||
Some(requirements),
|
||||
&initial_policy,
|
||||
)?;
|
||||
let (started_proxy, _) = Session::start_managed_network_proxy(
|
||||
&spec,
|
||||
&Policy::empty(),
|
||||
&initial_policy,
|
||||
/*network_policy_decider*/ None,
|
||||
/*blocked_request_observer*/ None,
|
||||
/*managed_network_requirements_enabled*/ false,
|
||||
crate::config::NetworkProxyAuditMetadata::default(),
|
||||
)
|
||||
.await?;
|
||||
assert_eq!(
|
||||
started_proxy
|
||||
.proxy()
|
||||
.current_cfg()
|
||||
.await?
|
||||
.network
|
||||
.allowed_domains(),
|
||||
Some(vec!["*.example.com".to_string(), "evil.com".to_string()])
|
||||
);
|
||||
|
||||
{
|
||||
let mut state = session.state.lock().await;
|
||||
let mut config = (*state.session_configuration.original_config_do_not_use).clone();
|
||||
config.permissions.network = Some(spec);
|
||||
config.permissions.sandbox_policy =
|
||||
codex_config::Constrained::allow_any(initial_policy.clone());
|
||||
state.session_configuration.original_config_do_not_use = Arc::new(config);
|
||||
state.session_configuration.sandbox_policy =
|
||||
codex_config::Constrained::allow_any(initial_policy);
|
||||
}
|
||||
session.services.network_proxy = Some(started_proxy);
|
||||
|
||||
session
|
||||
.new_turn_with_sub_id(
|
||||
"sandbox-policy-change".to_string(),
|
||||
SessionSettingsUpdate {
|
||||
sandbox_policy: Some(SandboxPolicy::DangerFullAccess),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
|
||||
let started_proxy = session
|
||||
.services
|
||||
.network_proxy
|
||||
.as_ref()
|
||||
.expect("managed network proxy should be present");
|
||||
assert_eq!(
|
||||
started_proxy
|
||||
.proxy()
|
||||
.current_cfg()
|
||||
.await?
|
||||
.network
|
||||
.allowed_domains(),
|
||||
Some(vec!["*.example.com".to_string()])
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn get_base_instructions_no_user_content() {
|
||||
let prompt_with_apply_patch_instructions =
|
||||
|
||||
@@ -1,8 +1,6 @@
|
||||
use std::collections::BTreeSet;
|
||||
use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
use std::env;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
use std::sync::LazyLock;
|
||||
use std::sync::Mutex as StdMutex;
|
||||
@@ -42,7 +40,6 @@ use codex_login::default_client::is_first_party_chat_originator;
|
||||
use codex_login::default_client::originator;
|
||||
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
|
||||
use codex_mcp::McpConnectionManager;
|
||||
use codex_mcp::SandboxState;
|
||||
use codex_mcp::ToolInfo;
|
||||
use codex_mcp::ToolPluginProvenance;
|
||||
use codex_mcp::codex_apps_tools_cache_key;
|
||||
@@ -228,13 +225,6 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_options_and_status(
|
||||
let (tx_event, rx_event) = unbounded();
|
||||
drop(rx_event);
|
||||
|
||||
let sandbox_state = SandboxState {
|
||||
sandbox_policy: SandboxPolicy::new_read_only_policy(),
|
||||
codex_linux_sandbox_exe: config.codex_linux_sandbox_exe.clone(),
|
||||
sandbox_cwd: env::current_dir().unwrap_or_else(|_| PathBuf::from("/")),
|
||||
use_legacy_landlock: config.features.use_legacy_landlock(),
|
||||
};
|
||||
|
||||
let (mcp_connection_manager, cancel_token) = McpConnectionManager::new(
|
||||
&mcp_servers,
|
||||
config.mcp_oauth_credentials_store_mode,
|
||||
@@ -242,7 +232,7 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_options_and_status(
|
||||
&config.permissions.approval_policy,
|
||||
INITIAL_SUBMIT_ID.to_owned(),
|
||||
tx_event,
|
||||
sandbox_state,
|
||||
SandboxPolicy::new_read_only_policy(),
|
||||
config.codex_home.to_path_buf(),
|
||||
codex_apps_tools_cache_key(auth.as_ref()),
|
||||
ToolPluginProvenance::default(),
|
||||
|
||||
@@ -55,8 +55,6 @@ pub use network_proxy_loader::MtimeConfigReloader;
|
||||
pub use network_proxy_loader::build_network_proxy_state;
|
||||
pub use network_proxy_loader::build_network_proxy_state_and_reloader;
|
||||
mod original_image_detail;
|
||||
pub use codex_mcp::MCP_SANDBOX_STATE_CAPABILITY;
|
||||
pub use codex_mcp::MCP_SANDBOX_STATE_METHOD;
|
||||
pub use codex_mcp::SandboxState;
|
||||
mod mcp_openai_file;
|
||||
mod mcp_tool_call;
|
||||
|
||||
Reference in New Issue
Block a user