From 2e764743a651907fa3c4a4ad76c796cac4572bb8 Mon Sep 17 00:00:00 2001 From: Anthony Tafoya Date: Thu, 29 Oct 2026 03:04:25 +0000 Subject: [PATCH] Apply session plugin overrides during MCP config refresh (#57974) ## What changed Allow `refresh_mcp_config` to replace explicit session `plugins` overrides while preserving other session overrides and user settings. Omitting session plugins retains the current overrides; supplying an empty map clears them and restores inheritance from lower configuration layers. ## Testing Add coverage for replacing and clearing plugin overrides, preserving unrelated settings, rejecting stale refreshes, revoking and restoring plugin tools and hooks, and retaining refreshed plugin policy when a deferred executor becomes ready. GitOrigin-RevId: 26913fac0f8ae9590b39bdc037a78c9be86abb67 --- codex-rs/config/src/state.rs | 38 ++++++ codex-rs/core/src/codex_thread.rs | 4 +- codex-rs/core/src/config/runtime_refresh.rs | 19 ++- .../core/src/config/runtime_refresh_tests.rs | 51 +++++++ codex-rs/core/src/session/tests.rs | 4 + .../suite/hooks_plugin_policy_refresh.rs | 128 ++++++++++++++---- .../tests/suite/mcp_executor_context_tests.rs | 63 +++++++-- 7 files changed, 273 insertions(+), 34 deletions(-) diff --git a/codex-rs/config/src/state.rs b/codex-rs/config/src/state.rs index 20df8a2785..39bf96a852 100644 --- a/codex-rs/config/src/state.rs +++ b/codex-rs/config/src/state.rs @@ -553,6 +553,44 @@ impl ConfigLayerStack { }) } + pub fn with_session_plugins(mut self, plugins: TomlValue) -> std::io::Result { + let session_index = self.layers.iter().rposition(|layer| { + layer.name == ConfigLayerSource::SessionFlags && !layer.is_disabled() + }); + if let Some(session_index) = session_index { + for (index, layer) in self.layers.iter_mut().enumerate() { + if layer.name != ConfigLayerSource::SessionFlags || layer.is_disabled() { + continue; + } + let mut config = layer.config.clone(); + let table = config.as_table_mut().ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + "session config must be a table", + ) + })?; + if index == session_index { + table.insert("plugins".to_owned(), plugins.clone()); + } else if table.remove("plugins").is_none() { + continue; + } + *layer = ConfigLayerEntry::new(ConfigLayerSource::SessionFlags, config); + } + } else { + let index = self.layers.partition_point(|layer| { + layer.name.precedence() <= ConfigLayerSource::SessionFlags.precedence() + }); + self.layers.insert( + index, + ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + TomlValue::Table([("plugins".to_owned(), plugins)].into_iter().collect()), + ), + ); + } + Ok(self) + } + /// Returns a new stack with the user layer copied from `other`, preserving /// every non-user layer already present in this stack. pub fn with_user_layer_from(&self, other: &Self) -> Self { diff --git a/codex-rs/core/src/codex_thread.rs b/codex-rs/core/src/codex_thread.rs index 968d331a5e..27275bf5a6 100644 --- a/codex-rs/core/src/codex_thread.rs +++ b/codex-rs/core/src/codex_thread.rs @@ -1188,7 +1188,9 @@ impl CodexThread { self.session.disable_mcp_enterprise_auth().await; } - /// Refresh MCP configuration and managed requirements without reloading unrelated settings. + /// Refresh MCP configuration, managed requirements, and explicit session plugin overrides. + /// Omitted session plugins preserve current overrides; an empty map restores inheritance. + /// Other session overrides and user settings remain unchanged. /// A stale owner requires reloading from the current config before retrying. pub async fn refresh_mcp_config( &self, diff --git a/codex-rs/core/src/config/runtime_refresh.rs b/codex-rs/core/src/config/runtime_refresh.rs index a8b61d3598..07a7cf81d0 100644 --- a/codex-rs/core/src/config/runtime_refresh.rs +++ b/codex-rs/core/src/config/runtime_refresh.rs @@ -40,7 +40,7 @@ impl Config { incoming: &Self, scope: RuntimeConfigRefresh, ) -> std::io::Result { - let layers = match scope { + let mut layers = match scope { RuntimeConfigRefresh::User | RuntimeConfigRefresh::UserFiles => self .config_layer_stack .with_user_layer_from(&incoming.config_layer_stack), @@ -50,6 +50,23 @@ impl Config { )? .with_user_layer_from(&self.config_layer_stack), }; + if matches!(scope, RuntimeConfigRefresh::Mcp) { + let plugins = incoming + .config_layer_stack + .all_layers_low_to_high() + .filter(|layer| { + layer.name == codex_config::ConfigLayerSource::SessionFlags + && !layer.is_disabled() + }) + .filter_map(|layer| layer.config.get("plugins").cloned()) + .reduce(|mut plugins, overlay| { + codex_config::merge_toml_values(&mut plugins, &overlay); + plugins + }); + if let Some(plugins) = plugins { + layers = layers.with_session_plugins(plugins)?; + } + } // User refreshes retain the session's managed configuration. layers.ensure_config_server_applied()?; let cfg: McpRefreshToml = layers diff --git a/codex-rs/core/src/config/runtime_refresh_tests.rs b/codex-rs/core/src/config/runtime_refresh_tests.rs index 698d794867..390374f5d0 100644 --- a/codex-rs/core/src/config/runtime_refresh_tests.rs +++ b/codex-rs/core/src/config/runtime_refresh_tests.rs @@ -290,3 +290,54 @@ async fn mcp_refresh_preserves_apps_until_new_session() { assert!(reloaded.features.enabled(Feature::Apps)); assert!(!next.features.enabled(Feature::Apps)); } + +/// Explicit plugin updates replace only their session override, retaining other thread settings. +#[tokio::test] +async fn plugin_override_refresh_preserves_session_and_restores_inheritance() { + let (_home, base) = base_config().await; + let mut current = layered_config( + &base, + "[plugins.browser]\nenabled = false", + "", + "", + "[apps.calendar]\nenabled = false\n[plugins.browser]\nenabled = true", + ) + .await; + let incoming = layered_config( + &base, + "[plugins.browser]\nenabled = false", + "", + "", + "[apps.calendar]\nenabled = true", + ) + .await; + let original_apps = current.config_layer_stack.effective_config()["apps"].clone(); + let layer_count = current.config_layer_stack.all_layers_low_to_high().count(); + let unchanged = current + .resolve_runtime_refresh(&incoming, RuntimeConfigRefresh::Mcp) + .unwrap(); + assert_eq!(unchanged.plugins, current.plugins); + for (override_text, expected) in [ + ("[browser]\nenabled = false", false), + ("[browser]\nenabled = true", true), + ("", false), + ] { + let mut incoming = incoming.clone(); + incoming.config_layer_stack = incoming + .config_layer_stack + .with_session_plugins(toml::from_str(override_text).unwrap()) + .unwrap(); + current = current + .resolve_runtime_refresh(&incoming, RuntimeConfigRefresh::Mcp) + .unwrap(); + assert_eq!(current.plugins.plugins["browser"].enabled, Some(expected)); + assert_eq!( + current.config_layer_stack.effective_config()["apps"], + original_apps + ); + assert_eq!( + current.config_layer_stack.all_layers_low_to_high().count(), + layer_count + ); + } +} diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 68022ae9d2..d875ed01fe 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -2387,6 +2387,10 @@ async fn refresh_config_rejects_stale_owner_without_effects(scope: RuntimeConfig let (session, _turn_context) = make_session_and_context().await; let stale_owner = session.get_config().await; let mut stale_config = stale_owner.as_ref().clone(); + stale_config.config_layer_stack = stale_config + .config_layer_stack + .with_session_plugins(toml::from_str("[browser]\nenabled = false").unwrap()) + .unwrap(); stale_config .features .set_enabled( diff --git a/codex-rs/core/tests/suite/hooks_plugin_policy_refresh.rs b/codex-rs/core/tests/suite/hooks_plugin_policy_refresh.rs index 4021e96409..6cb93812ce 100644 --- a/codex-rs/core/tests/suite/hooks_plugin_policy_refresh.rs +++ b/codex-rs/core/tests/suite/hooks_plugin_policy_refresh.rs @@ -21,6 +21,7 @@ use core_test_support::apps_test_server::recorded_apps_tool_calls; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_function_call; +use core_test_support::responses::mount_sse_once; use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; @@ -41,8 +42,18 @@ use wiremock::matchers::path; use super::rmcp_client::remote_aware_environment_id; +enum PluginPolicyRefresh { + System, + ClearSession, +} + +/// Policy refreshes revoke plugin tools and hooks; clearing session overrides restores inheritance. +#[test_case::test_case(PluginPolicyRefresh::System; "system policy")] +#[test_case::test_case(PluginPolicyRefresh::ClearSession; "clear session override")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn native_stop_hook_uses_plugin_policy_refreshed_during_turn() -> Result<()> { +async fn native_stop_hook_uses_plugin_policy_refreshed_during_turn( + refresh: PluginPolicyRefresh, +) -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; @@ -76,6 +87,16 @@ async fn native_stop_hook_uses_plugin_policy_refreshed_during_turn() -> Result<( }] }] } }, }))?, )?; + fs::write( + plugin_root.join(".mcp.json"), + serde_json::to_vec(&json!({ + "mcpServers": { + "plugin_tools": { + "url": format!("{}/api/codex/ps/mcp", observer.chatgpt_base_url), + }, + }, + }))?, + )?; fs::write( home.path().join("config.toml"), "[features]\nplugins = true\nhooks = true\n\n[plugins.\"example@test\"]\n", @@ -128,6 +149,15 @@ async fn native_stop_hook_uses_plugin_policy_refreshed_during_turn() -> Result<( test.submit_text_turn("finish the enabled baseline turn") .await?; assert_eq!(recorded_apps_tool_calls(&server).await.len(), 1); + test.codex + .call_mcp_tool( + "plugin_tools", + "calendar_list_events", + /*arguments*/ None, + /*meta*/ None, + ) + .await?; + assert_eq!(recorded_apps_tool_calls(&server).await.len(), 2); test.codex .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { @@ -142,27 +172,33 @@ async fn native_stop_hook_uses_plugin_policy_refreshed_during_turn() -> Result<( .await; let current_config = test.codex.config().await; let mut next_config = (*current_config).clone(); - let mut layers = current_config - .config_layer_stack - .all_layers_low_to_high() - .cloned() - .collect::>(); - layers.push(ConfigLayerEntry::new( - ConfigLayerSource::System { - file: current_config.codex_home.join("system-config.toml"), - }, - toml::from_str("[plugins._default]\nenabled = false")?, - )); - layers.sort_by_key(|layer| layer.name.precedence()); - next_config.config_layer_stack = ConfigLayerStack::new( - layers, - current_config.config_layer_stack.requirements().clone(), - current_config + if matches!(refresh, PluginPolicyRefresh::ClearSession) { + next_config.config_layer_stack = next_config .config_layer_stack - .requirements_toml() - .clone(), - ) - .expect("updated system layer"); + .with_session_plugins(toml::from_str("[_default]\nenabled = false")?)?; + } else { + let mut layers = current_config + .config_layer_stack + .all_layers_low_to_high() + .cloned() + .collect::>(); + layers.push(ConfigLayerEntry::new( + ConfigLayerSource::System { + file: current_config.codex_home.join("system-config.toml"), + }, + toml::from_str("[plugins._default]\nenabled = false")?, + )); + layers.sort_by_key(|layer| layer.name.precedence()); + next_config.config_layer_stack = ConfigLayerStack::new( + layers, + current_config.config_layer_stack.requirements().clone(), + current_config + .config_layer_stack + .requirements_toml() + .clone(), + ) + .expect("updated system layer"); + } assert_eq!( test.codex .refresh_mcp_config(current_config, next_config) @@ -187,7 +223,18 @@ async fn native_stop_hook_uses_plugin_policy_refreshed_during_turn() -> Result<( }) .await; assert_eq!(responses.requests().len(), 3); - assert_eq!(recorded_apps_tool_calls(&server).await.len(), 1); + assert!( + test.codex + .call_mcp_tool( + "plugin_tools", + "calendar_list_events", + /*arguments*/ None, + /*meta*/ None, + ) + .await + .is_err() + ); + assert_eq!(recorded_apps_tool_calls(&server).await.len(), 2); test.codex .call_mcp_tool( @@ -197,6 +244,41 @@ async fn native_stop_hook_uses_plugin_policy_refreshed_during_turn() -> Result<( /*meta*/ None, ) .await?; - assert_eq!(recorded_apps_tool_calls(&server).await.len(), 2); + assert_eq!(recorded_apps_tool_calls(&server).await.len(), 3); + + if matches!(refresh, PluginPolicyRefresh::ClearSession) { + let current = test.codex.config().await; + let mut next = (*current).clone(); + next.config_layer_stack = next + .config_layer_stack + .with_session_plugins(toml::Value::Table(Default::default()))?; + assert_eq!( + test.codex.refresh_mcp_config(current, next).await, + ConfigRefreshOutcome::Published + ); + assert_eq!(test.codex.config().await.plugins, test.config.plugins); + + let restored_response = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("restored-turn", "done"), + ev_completed("restored-turn"), + ]), + ) + .await; + test.submit_text_turn("finish with inherited plugin policy restored") + .await?; + assert_eq!(restored_response.requests().len(), 1); + assert_eq!(recorded_apps_tool_calls(&server).await.len(), 4); + test.codex + .call_mcp_tool( + "plugin_tools", + "calendar_list_events", + /*arguments*/ None, + /*meta*/ None, + ) + .await?; + assert_eq!(recorded_apps_tool_calls(&server).await.len(), 5); + } Ok(()) } diff --git a/codex-rs/core/tests/suite/mcp_executor_context_tests.rs b/codex-rs/core/tests/suite/mcp_executor_context_tests.rs index 00440a4716..4177271551 100644 --- a/codex-rs/core/tests/suite/mcp_executor_context_tests.rs +++ b/codex-rs/core/tests/suite/mcp_executor_context_tests.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use std::sync::Mutex; use anyhow::Result; +use codex_config::types::PluginsConfigToml; use codex_core::StartThreadOptions; use codex_core::config::Config; use codex_extension_api::ExtensionFuture; @@ -16,8 +17,11 @@ use codex_features::Feature; use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::capabilities::SelectedCapabilityRoot; use codex_protocol::protocol::EnvironmentConfigState; +use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::TurnEnvironmentRequest; use codex_protocol::protocol::TurnEnvironmentSelection; +use codex_protocol::turn_input::TurnInputRequest; +use codex_protocol::user_input::UserInput; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; use core_test_support::test_codex::environment_config_for_selection; @@ -27,7 +31,7 @@ use pretty_assertions::assert_eq; #[derive(Default)] struct ExecutorSelectionRecorder { observed: Mutex>>, - plugin_contexts: Mutex>>>, + plugin_contexts: Mutex>, PluginsConfigToml)>>, } impl McpServerContributor for ExecutorSelectionRecorder { @@ -38,13 +42,16 @@ impl McpServerContributor for ExecutorSelectionRecorder { fn selected_plugins<'a>( &'a self, context: McpServerContributionContext<'a, Config>, - _plugins_config: &'a codex_config::types::PluginsConfigToml, + plugins_config: &'a codex_config::types::PluginsConfigToml, ) -> ExtensionFuture<'a, Vec>> { Box::pin(async move { self.plugin_contexts .lock() .expect("plugin context recorder lock") - .push(context.selected_environments().map(<[_]>::to_vec)); + .push(( + context.selected_environments().map(<[_]>::to_vec), + plugins_config.clone(), + )); Vec::new() }) } @@ -140,6 +147,7 @@ async fn thread_projection_preserves_executor_order_and_unavailable_selections() Ok(()) } +/// An executor becoming ready must use the plugin policy published while it was pending. #[tokio::test] async fn model_step_plugin_projection_preserves_selected_environments() -> Result<()> { skip_if_no_network!(Ok(())); @@ -149,9 +157,37 @@ async fn model_step_plugin_projection_preserves_selected_environments() -> Resul extensions.mcp_server_contributor(recorder.clone()); let test = test_codex() .with_extensions(Arc::new(extensions.build())) + .with_config(|config| { + config + .features + .enable(Feature::DeferredExecutor) + .expect("enable deferred executor"); + }) .build_with_auto_env(&server) .await?; - let selection = test.codex.environment_selections().await.remove(0); + let mut selection = test.codex.environment_selections().await.remove(0); + selection.config = EnvironmentConfigState::Pending; + let thread = test + .thread_manager + .start_thread(StartThreadOptions { + environments: Some(vec![selection.clone().into_request()]), + ..StartThreadOptions::new(test.config.clone()) + }) + .await? + .thread; + let current = thread.config().await; + let mut next = (*current).clone(); + let plugins = toml::from_str("[\"unified-computer-use@openai-bundled\"]\nenabled = false")?; + next.config_layer_stack = next.config_layer_stack.with_session_plugins(plugins)?; + assert_eq!( + thread.refresh_mcp_config(current, next).await, + codex_core::ConfigRefreshOutcome::Published + ); + let expected_plugins = thread.config().await.plugins.clone(); + assert_eq!( + expected_plugins.plugins["unified-computer-use@openai-bundled"].enabled, + Some(false) + ); let mut environment_config = environment_config_for_selection(&test.config, &selection); // Step-local plugin projection returns early without a ready selected root. environment_config.selected_capability_roots = vec![SelectedCapabilityRoot { @@ -161,10 +197,11 @@ async fn model_step_plugin_projection_preserves_selected_environments() -> Resul path: selection.cwd.clone(), }, }]; - test.codex + thread .environment_ready(&selection, environment_config) .await?; - let selections = test.codex.environment_selections().await; + thread.current_mcp_config_and_runtime_context().await; + let selections = thread.environment_selections().await; recorder .plugin_contexts .lock() @@ -176,7 +213,14 @@ async fn model_step_plugin_projection_preserves_selected_environments() -> Resul ) .await; - test.submit_turn("Respond without using tools.").await?; + thread + .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { + text: "Respond without using tools.".to_owned(), + text_elements: Vec::new(), + }])) + .await?; + core_test_support::wait_for_event(&thread, |event| matches!(event, EventMsg::TurnComplete(_))) + .await; let contexts = recorder .plugin_contexts @@ -189,8 +233,9 @@ async fn model_step_plugin_projection_preserves_selected_environments() -> Resul assert!( contexts .iter() - .all(|context| context.as_ref() == Some(&selections)), - "every step-local plugin projection must receive its selected environments: {contexts:?}" + .all(|(context, plugins)| context.as_ref() == Some(&selections) + && plugins == &expected_plugins), + "step projection must retain selected environments and the pre-arrival plugin policy: {contexts:?}" ); Ok(()) }