mirror of
https://github.com/openai/codex.git
synced 2026-10-29 19:08:43 +00:00
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
This commit is contained in:
committed by
copyberry
parent
797bab4746
commit
2e764743a6
@@ -553,6 +553,44 @@ impl ConfigLayerStack {
|
||||
})
|
||||
}
|
||||
|
||||
pub fn with_session_plugins(mut self, plugins: TomlValue) -> std::io::Result<Self> {
|
||||
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 {
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -40,7 +40,7 @@ impl Config {
|
||||
incoming: &Self,
|
||||
scope: RuntimeConfigRefresh,
|
||||
) -> std::io::Result<Self> {
|
||||
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
|
||||
|
||||
@@ -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
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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::<Vec<_>>();
|
||||
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::<Vec<_>>();
|
||||
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(())
|
||||
}
|
||||
|
||||
@@ -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<Vec<Vec<TurnEnvironmentSelection>>>,
|
||||
plugin_contexts: Mutex<Vec<Option<Vec<TurnEnvironmentSelection>>>>,
|
||||
plugin_contexts: Mutex<Vec<(Option<Vec<TurnEnvironmentSelection>>, PluginsConfigToml)>>,
|
||||
}
|
||||
|
||||
impl McpServerContributor<Config> for ExecutorSelectionRecorder {
|
||||
@@ -38,13 +42,16 @@ impl McpServerContributor<Config> 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<SelectedPlugin<'a>>> {
|
||||
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(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user