From 6f0d002c2010f9bb7fba9d096bfd371f41f5a6f8 Mon Sep 17 00:00:00 2001 From: Felix Xia Date: Tue, 23 Jun 2026 20:53:06 +0100 Subject: [PATCH 1/8] Represent MCP server requirements as variants --- codex-rs/config/src/config_requirements.rs | 13 +++++----- codex-rs/core/src/config/config_tests.rs | 22 ++++++++--------- codex-rs/core/src/config/mod.rs | 28 ++++++++++++---------- codex-rs/tui/src/debug_config.rs | 4 ++-- 4 files changed, 35 insertions(+), 32 deletions(-) diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index e757c2149c9f..cf35f3038f0d 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -225,8 +225,9 @@ pub enum McpServerIdentity { } #[derive(Deserialize, Debug, Clone, PartialEq, Eq)] -pub struct McpServerRequirement { - pub identity: McpServerIdentity, +#[serde(untagged)] +pub enum McpServerRequirement { + Identity { identity: McpServerIdentity }, } #[derive(Deserialize, Debug, Clone, Default, PartialEq, Eq)] @@ -3469,7 +3470,7 @@ command = "python3 /enterprise/hooks/pre.py" BTreeMap::from([ ( "docs".to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Command { command: "codex-mcp".to_string(), }, @@ -3477,7 +3478,7 @@ command = "python3 /enterprise/hooks/pre.py" ), ( "remote".to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Url { url: "https://example.com/mcp".to_string(), }, @@ -3511,7 +3512,7 @@ command = "python3 /enterprise/hooks/pre.py" PluginRequirementsToml { mcp_servers: Some(BTreeMap::from([( "remote".to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Url { url: "https://example.com/mcp".to_string(), }, @@ -3524,7 +3525,7 @@ command = "python3 /enterprise/hooks/pre.py" PluginRequirementsToml { mcp_servers: Some(BTreeMap::from([( "sample".to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Command { command: "sample-mcp".to_string(), }, diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 62d9f792ef36..b098ccbf84ae 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -4140,7 +4140,7 @@ fn filter_mcp_servers_by_allowlist_enforces_identity_rules() { BTreeMap::from([ ( MISMATCHED_URL_SERVER.to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Url { url: "https://example.com/other".to_string(), }, @@ -4148,7 +4148,7 @@ fn filter_mcp_servers_by_allowlist_enforces_identity_rules() { ), ( MISMATCHED_COMMAND_SERVER.to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Command { command: "other-cmd".to_string(), }, @@ -4156,7 +4156,7 @@ fn filter_mcp_servers_by_allowlist_enforces_identity_rules() { ), ( MATCHED_URL_SERVER.to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Url { url: GOOD_URL.to_string(), }, @@ -4164,7 +4164,7 @@ fn filter_mcp_servers_by_allowlist_enforces_identity_rules() { ), ( MATCHED_COMMAND_SERVER.to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Command { command: GOOD_CMD.to_string(), }, @@ -4271,7 +4271,7 @@ fn filter_plugin_mcp_servers_by_allowlist_enforces_plugin_and_identity_rules() { mcp_servers: Some(BTreeMap::from([ ( MATCHED_SERVER.to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Command { command: GOOD_CMD.to_string(), }, @@ -4279,7 +4279,7 @@ fn filter_plugin_mcp_servers_by_allowlist_enforces_plugin_and_identity_rules() { ), ( MISMATCHED_SERVER.to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Command { command: GOOD_CMD.to_string(), }, @@ -4320,7 +4320,7 @@ fn filter_plugin_mcp_servers_by_allowlist_blocks_unlisted_plugin() { codex_config::PluginRequirementsToml { mcp_servers: Some(BTreeMap::from([( "server-a".to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Command { command: "cmd-a".to_string(), }, @@ -4360,7 +4360,7 @@ async fn rebuild_preserving_session_layers_refreshes_requirements() -> std::io:: let mcp_requirements = BTreeMap::from([ ( "session_overrides_user".to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Command { command: "session-command".to_string(), }, @@ -4368,7 +4368,7 @@ async fn rebuild_preserving_session_layers_refreshes_requirements() -> std::io:: ), ( "managed_overrides_session".to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Command { command: "managed-command".to_string(), }, @@ -4376,7 +4376,7 @@ async fn rebuild_preserving_session_layers_refreshes_requirements() -> std::io:: ), ( "fresh_global".to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Command { command: "fresh-global-command".to_string(), }, @@ -4384,7 +4384,7 @@ async fn rebuild_preserving_session_layers_refreshes_requirements() -> std::io:: ), ( "fresh_project".to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Command { command: "fresh-project-command".to_string(), }, diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 96ba987b9a3b..86d020763c52 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1974,19 +1974,21 @@ fn mcp_server_matches_requirement( requirement: &McpServerRequirement, server: &McpServerConfig, ) -> bool { - match &requirement.identity { - McpServerIdentity::Command { - command: want_command, - } => matches!( - &server.transport, - McpServerTransportConfig::Stdio { command: got_command, .. } - if got_command == want_command - ), - McpServerIdentity::Url { url: want_url } => matches!( - &server.transport, - McpServerTransportConfig::StreamableHttp { url: got_url, .. } - if got_url == want_url - ), + match requirement { + McpServerRequirement::Identity { identity } => match identity { + McpServerIdentity::Command { + command: want_command, + } => matches!( + &server.transport, + McpServerTransportConfig::Stdio { command: got_command, .. } + if got_command == want_command + ), + McpServerIdentity::Url { url: want_url } => matches!( + &server.transport, + McpServerTransportConfig::StreamableHttp { url: got_url, .. } + if got_url == want_url + ), + }, } } diff --git a/codex-rs/tui/src/debug_config.rs b/codex-rs/tui/src/debug_config.rs index ed9aeffddb97..7a35cc450652 100644 --- a/codex-rs/tui/src/debug_config.rs +++ b/codex-rs/tui/src/debug_config.rs @@ -699,7 +699,7 @@ mod tests { mcp_servers: Some(Sourced::new( BTreeMap::from([( "docs".to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Command { command: "codex-mcp".to_string(), }, @@ -778,7 +778,7 @@ mod tests { hooks: None, mcp_servers: Some(BTreeMap::from([( "docs".to_string(), - McpServerRequirement { + McpServerRequirement::Identity { identity: McpServerIdentity::Command { command: "codex-mcp".to_string(), }, From a469b94cb63f7993abdc08f1fc725b29f2d8db72 Mon Sep 17 00:00:00 2001 From: Felix Xia Date: Tue, 23 Jun 2026 20:53:27 +0100 Subject: [PATCH 2/8] Add matcher forms to MCP server requirements --- codex-rs/Cargo.lock | 1 + codex-rs/config/Cargo.toml | 1 + codex-rs/config/src/config_requirements.rs | 186 ++++++++++++++++++ codex-rs/config/src/constraint.rs | 9 + codex-rs/config/src/lib.rs | 4 + codex-rs/config/src/mcp_requirements.rs | 109 ++++++++++ codex-rs/config/src/mcp_requirements_tests.rs | 142 +++++++++++++ codex-rs/core/src/config/mod.rs | 28 +-- 8 files changed, 454 insertions(+), 26 deletions(-) create mode 100644 codex-rs/config/src/mcp_requirements.rs create mode 100644 codex-rs/config/src/mcp_requirements_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 3324fbd3275d..3e21bcce42ca 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2557,6 +2557,7 @@ dependencies = [ "multimap", "pretty_assertions", "prost 0.14.3", + "regex-lite", "schemars 0.8.22", "serde", "serde_ignored", diff --git a/codex-rs/config/Cargo.toml b/codex-rs/config/Cargo.toml index 509ffd189add..4fe529d74380 100644 --- a/codex-rs/config/Cargo.toml +++ b/codex-rs/config/Cargo.toml @@ -30,6 +30,7 @@ gethostname = { workspace = true } indexmap = { workspace = true, features = ["serde"] } multimap = { workspace = true } prost = "0.14.3" +regex-lite = { workspace = true } schemars = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_ignored = { workspace = true } diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index cf35f3038f0d..e06996077d84 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -19,6 +19,8 @@ use super::requirements_exec_policy::RequirementsExecPolicyToml; use crate::Constrained; use crate::ConstraintError; use crate::ManagedHooksRequirementsToml; +use crate::McpServerCommandMatcher; +use crate::McpServerUrlMatcher; use crate::mcp_types::AppToolApproval; use crate::permissions_toml::PermissionProfileToml; use crate::types::WindowsSandboxModeToml; @@ -224,10 +226,17 @@ pub enum McpServerIdentity { Url { url: String }, } +/// A requirement for one named MCP server. +/// +/// The `Identity` variant preserves the released exact-match contract. The +/// command and URL variants add matcher-based requirements under the same +/// `mcp_servers` namespace. #[derive(Deserialize, Debug, Clone, PartialEq, Eq)] #[serde(untagged)] pub enum McpServerRequirement { Identity { identity: McpServerIdentity }, + Command(McpServerCommandMatcher), + Url(McpServerUrlMatcher), } #[derive(Deserialize, Debug, Clone, Default, PartialEq, Eq)] @@ -1202,6 +1211,25 @@ impl ConfigRequirementsToml { } } +fn validate_mcp_server_requirements( + requirements: &BTreeMap, + source: &RequirementSource, + plugin_name: Option<&str>, +) -> Result<(), ConstraintError> { + for (server_name, requirement) in requirements { + requirement + .validate() + .map_err(|reason| ConstraintError::McpServerRequirementParse { + server_name: plugin_name + .map(|plugin_name| format!("{plugin_name}/{server_name}")) + .unwrap_or_else(|| server_name.clone()), + requirement_source: source.clone(), + reason, + })?; + } + Ok(()) +} + impl TryFrom for ConfigRequirements { type Error = ConstraintError; @@ -1234,6 +1262,25 @@ impl TryFrom for ConfigRequirements { guardian_policy_config, } = toml; + if let Some(requirements) = &mcp_servers { + validate_mcp_server_requirements( + &requirements.value, + &requirements.source, + /*plugin_name*/ None, + )?; + } + if let Some(plugin_requirements) = &plugins { + for (plugin_name, plugin) in &plugin_requirements.value { + if let Some(requirements) = &plugin.mcp_servers { + validate_mcp_server_requirements( + requirements, + &plugin_requirements.source, + Some(plugin_name), + )?; + } + } + } + let approval_policy = match allowed_approval_policies { Some(Sourced { value: policies, @@ -1543,6 +1590,8 @@ pub fn sandbox_mode_requirement_for_permission_profile( mod tests { use super::*; use crate::HookEventsToml; + use crate::McpServerCommandMatcher; + use crate::McpServerValueMatcher; use anyhow::Result; use codex_execpolicy::Decision; use codex_execpolicy::Evaluation; @@ -3455,6 +3504,9 @@ command = "python3 /enterprise/hooks/pre.py" #[test] fn deserialize_mcp_server_requirements() -> Result<()> { let toml_str = r#" + [mcp_servers.docs] + description = "ignored legacy field" + [mcp_servers.docs.identity] command = "codex-mcp" @@ -3491,6 +3543,75 @@ command = "python3 /enterprise/hooks/pre.py" Ok(()) } + #[test] + fn deserialize_mcp_server_matcher_requirements() -> Result<()> { + let toml_str = r#" + [mcp_servers.internal_mcp_proxy] + command = "company-cli" + args = [ + { match = "exact", value = "mcp" }, + { match = "exact", value = "proxy" }, + { match = "exact", value = "--server" }, + { match = "regex", expression = '^https://[A-Za-z0-9-]+\.mcp\.internal\.example\.com(?::443)?(?:/.*)?$' }, + ] + "#; + let requirements: ConfigRequirements = + with_unknown_source(from_str(toml_str)?).try_into()?; + + assert_eq!( + requirements.mcp_servers, + Some(Sourced::new( + BTreeMap::from([( + "internal_mcp_proxy".to_string(), + McpServerRequirement::Command(McpServerCommandMatcher { + command: "company-cli".to_string(), + args: vec![ + McpServerValueMatcher::Exact { + value: "mcp".to_string(), + }, + McpServerValueMatcher::Exact { + value: "proxy".to_string(), + }, + McpServerValueMatcher::Exact { + value: "--server".to_string(), + }, + McpServerValueMatcher::Regex { + expression: r"^https://[A-Za-z0-9-]+\.mcp\.internal\.example\.com(?::443)?(?:/.*)?$" + .to_string(), + }, + ], + }), + )]), + RequirementSource::Unknown, + )) + ); + Ok(()) + } + + #[test] + fn invalid_mcp_server_requirement_regex_reports_the_server_name_and_source() -> Result<()> { + let toml_str = r#" + [mcp_servers.broken_rule] + url = { match = "regex", expression = "[" } + "#; + + let err = ConfigRequirements::try_from(with_unknown_source(from_str(toml_str)?)) + .expect_err("invalid matcher regex should fail requirements normalization"); + let ConstraintError::McpServerRequirementParse { + server_name, + requirement_source, + reason, + } = err + else { + panic!("unexpected error: {err:?}"); + }; + + assert_eq!(server_name, "broken_rule"); + assert_eq!(requirement_source, RequirementSource::Unknown); + assert!(reason.contains("invalid regex `[`"), "{reason}"); + Ok(()) + } + #[test] fn deserialize_plugin_mcp_server_requirements() -> Result<()> { let toml_str = r#" @@ -3540,6 +3661,71 @@ command = "python3 /enterprise/hooks/pre.py" Ok(()) } + #[test] + fn deserialize_plugin_mcp_server_matcher_requirement() -> Result<()> { + let toml_str = r#" + [plugins."sample@test".mcp_servers.internal_proxy] + command = "company-cli" + args = [ + { match = "exact", value = "mcp" }, + { match = "regex", expression = '^https://[a-z]+\.example\.com$' }, + ] + "#; + let requirements: ConfigRequirements = + with_unknown_source(from_str(toml_str)?).try_into()?; + + assert_eq!( + requirements.plugins, + Some(Sourced::new( + BTreeMap::from([( + "sample@test".to_string(), + PluginRequirementsToml { + mcp_servers: Some(BTreeMap::from([( + "internal_proxy".to_string(), + McpServerRequirement::Command(McpServerCommandMatcher { + command: "company-cli".to_string(), + args: vec![ + McpServerValueMatcher::Exact { + value: "mcp".to_string(), + }, + McpServerValueMatcher::Regex { + expression: r"^https://[a-z]+\.example\.com$".to_string(), + }, + ], + }), + )])), + }, + )]), + RequirementSource::Unknown, + )) + ); + Ok(()) + } + + #[test] + fn invalid_plugin_mcp_server_regex_reports_plugin_and_server_name() -> Result<()> { + let toml_str = r#" + [plugins."sample@test".mcp_servers.broken_rule] + url = { match = "regex", expression = "[" } + "#; + + let err = ConfigRequirements::try_from(with_unknown_source(from_str(toml_str)?)) + .expect_err("invalid plugin MCP regex should fail requirements normalization"); + let ConstraintError::McpServerRequirementParse { + server_name, + requirement_source, + reason, + } = err + else { + panic!("unexpected error: {err:?}"); + }; + + assert_eq!(server_name, "sample@test/broken_rule"); + assert_eq!(requirement_source, RequirementSource::Unknown); + assert!(reason.contains("invalid regex `[`"), "{reason}"); + Ok(()) + } + #[test] fn deserialize_exec_policy_requirements() -> Result<()> { let toml_str = r#" diff --git a/codex-rs/config/src/constraint.rs b/codex-rs/config/src/constraint.rs index 64b604cbc2d1..8628f3909c1d 100644 --- a/codex-rs/config/src/constraint.rs +++ b/codex-rs/config/src/constraint.rs @@ -24,6 +24,15 @@ pub enum ConstraintError { requirement_source: RequirementSource, reason: String, }, + + #[error( + "invalid requirement for MCP server `{server_name}` (set by {requirement_source}): {reason}" + )] + McpServerRequirementParse { + server_name: String, + requirement_source: RequirementSource, + reason: String, + }, } impl ConstraintError { diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 1cbbbaf0a9d1..918a48d05ed4 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -12,6 +12,7 @@ mod key_aliases; pub mod loader; mod marketplace_edit; mod mcp_edit; +mod mcp_requirements; mod mcp_types; mod merge; mod overrides; @@ -113,6 +114,9 @@ pub use marketplace_edit::remove_user_marketplace; pub use marketplace_edit::remove_user_marketplace_config; pub use mcp_edit::ConfigEditsBuilder; pub use mcp_edit::load_global_mcp_servers; +pub use mcp_requirements::McpServerCommandMatcher; +pub use mcp_requirements::McpServerUrlMatcher; +pub use mcp_requirements::McpServerValueMatcher; pub use mcp_types::AppToolApproval; pub use mcp_types::DEFAULT_MCP_SERVER_ENVIRONMENT_ID; pub use mcp_types::McpServerConfig; diff --git a/codex-rs/config/src/mcp_requirements.rs b/codex-rs/config/src/mcp_requirements.rs new file mode 100644 index 000000000000..be49fd15ecdb --- /dev/null +++ b/codex-rs/config/src/mcp_requirements.rs @@ -0,0 +1,109 @@ +use crate::McpServerIdentity; +use crate::McpServerRequirement; +use crate::mcp_types::McpServerConfig; +use crate::mcp_types::McpServerTransportConfig; +use regex_lite::Regex; +use serde::Deserialize; + +/// String matching operations available to managed MCP server matchers. +#[derive(Deserialize, Debug, Clone, PartialEq, Eq)] +#[serde(tag = "match", rename_all = "snake_case", deny_unknown_fields)] +pub enum McpServerValueMatcher { + Exact { value: String }, + Prefix { value: String }, + Regex { expression: String }, +} + +impl McpServerValueMatcher { + fn compile_full_regex(expression: &str) -> Result { + Regex::new(&format!(r"\A(?:{expression})\z")) + .map_err(|err| format!("invalid regex `{expression}`: {err}")) + } + + fn validate(&self) -> Result<(), String> { + let Self::Regex { expression } = self else { + return Ok(()); + }; + Self::compile_full_regex(expression).map(|_| ()) + } + + fn matches(&self, candidate: &str) -> bool { + match self { + Self::Exact { value } => candidate == value, + Self::Prefix { value } => candidate.starts_with(value), + Self::Regex { expression } => Self::compile_full_regex(expression) + .ok() + .is_some_and(|regex| regex.is_match(candidate)), + } + } +} + +#[derive(Deserialize, Debug, Clone, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub struct McpServerCommandMatcher { + pub command: String, + pub args: Vec, +} + +#[derive(Deserialize, Debug, Clone, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub struct McpServerUrlMatcher { + pub url: McpServerValueMatcher, +} + +impl McpServerRequirement { + pub(crate) fn validate(&self) -> Result<(), String> { + match self { + Self::Identity { .. } => Ok(()), + Self::Command(matcher) => { + for (index, arg) in matcher.args.iter().enumerate() { + arg.validate().map_err(|err| { + format!("invalid argument matcher at index {index}: {err}") + })?; + } + Ok(()) + } + Self::Url(matcher) => matcher.url.validate(), + } + } + + pub fn matches(&self, server: &McpServerConfig) -> bool { + match (self, &server.transport) { + ( + Self::Identity { + identity: + McpServerIdentity::Command { + command: want_command, + }, + }, + McpServerTransportConfig::Stdio { + command: got_command, + .. + }, + ) => got_command == want_command, + ( + Self::Identity { + identity: McpServerIdentity::Url { url: want_url }, + }, + McpServerTransportConfig::StreamableHttp { url: got_url, .. }, + ) => got_url == want_url, + (Self::Command(matcher), McpServerTransportConfig::Stdio { command, args, .. }) => { + matcher.command == *command + && matcher.args.len() == args.len() + && matcher + .args + .iter() + .zip(args) + .all(|(matcher, arg)| matcher.matches(arg)) + } + (Self::Url(matcher), McpServerTransportConfig::StreamableHttp { url, .. }) => { + matcher.url.matches(url) + } + _ => false, + } + } +} + +#[cfg(test)] +#[path = "mcp_requirements_tests.rs"] +mod tests; diff --git a/codex-rs/config/src/mcp_requirements_tests.rs b/codex-rs/config/src/mcp_requirements_tests.rs new file mode 100644 index 000000000000..f0b4b7b268bf --- /dev/null +++ b/codex-rs/config/src/mcp_requirements_tests.rs @@ -0,0 +1,142 @@ +use super::*; +use crate::mcp_types::McpServerConfig; +use pretty_assertions::assert_eq; +use std::collections::HashMap; + +fn stdio_server(command: &str, args: &[&str]) -> McpServerConfig { + McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command: command.to_string(), + args: args.iter().map(ToString::to_string).collect(), + env: None, + env_vars: Vec::new(), + cwd: None, + }, + environment_id: crate::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), + enabled: true, + required: false, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: None, + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth: None, + oauth_resource: None, + tools: HashMap::new(), + } +} + +#[test] +fn command_matcher_matches_exact_positional_arguments() { + let requirement = McpServerRequirement::Command(McpServerCommandMatcher { + command: "company-cli".to_string(), + args: vec![ + McpServerValueMatcher::Exact { + value: "mcp".to_string(), + }, + McpServerValueMatcher::Regex { + expression: r"https://[a-z]+\.example\.com".to_string(), + }, + ], + }); + + assert!(requirement.matches(&stdio_server( + "company-cli", + &["mcp", "https://pricing.example.com"] + ))); + assert!(!requirement.matches(&stdio_server( + "company-cli", + &["https://pricing.example.com", "mcp"] + ))); + assert!(!requirement.matches(&stdio_server( + "company-cli", + &["mcp", "https://pricing.example.com", "--verbose"] + ))); + assert!(!requirement.matches(&stdio_server( + "/usr/local/bin/company-cli", + &["mcp", "https://pricing.example.com"] + ))); +} + +#[test] +fn regex_matcher_requires_a_full_value_match() { + let matcher = McpServerValueMatcher::Regex { + expression: "mcp".to_string(), + }; + + assert!(matcher.matches("mcp")); + assert!(!matcher.matches("mcp-proxy")); + assert!(!matcher.matches("prefix-mcp")); +} + +#[test] +fn regex_matcher_allows_a_later_alternative_to_match_the_full_value() { + let matcher = McpServerValueMatcher::Regex { + expression: r"https://api\.example\.com|https://api\.example\.com/mcp".to_string(), + }; + + assert!(matcher.matches("https://api.example.com/mcp")); +} + +#[test] +fn legacy_command_identity_keeps_ignoring_arguments() { + let requirement: McpServerRequirement = toml::from_str( + r#" +[identity] +command = "company-cli" +"#, + ) + .expect("legacy command identity"); + + assert!(requirement.matches(&stdio_server( + "company-cli", + &["any", "arguments", "remain", "allowed"] + ))); + assert!(!requirement.matches(&stdio_server("different-cli", &[]))); +} + +#[test] +fn requirement_deserializes_command_and_url_matcher_shapes() { + let command: McpServerRequirement = toml::from_str( + r#" +command = "company-cli" +args = [ + { match = "exact", value = "mcp" }, + { match = "regex", expression = '^https://[a-z]+\.example\.com$' }, +] +"#, + ) + .expect("command matcher"); + let url: McpServerRequirement = toml::from_str( + r#" +url = { match = "prefix", value = "https://mcp.example.com/" } +"#, + ) + .expect("URL matcher"); + + assert_eq!( + command, + McpServerRequirement::Command(McpServerCommandMatcher { + command: "company-cli".to_string(), + args: vec![ + McpServerValueMatcher::Exact { + value: "mcp".to_string(), + }, + McpServerValueMatcher::Regex { + expression: r"^https://[a-z]+\.example\.com$".to_string(), + }, + ], + }) + ); + assert_eq!( + url, + McpServerRequirement::Url(McpServerUrlMatcher { + url: McpServerValueMatcher::Prefix { + value: "https://mcp.example.com/".to_string(), + }, + }) + ); +} diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 86d020763c52..c0cbd453f741 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -14,7 +14,6 @@ use codex_config::ConfigRequirements; use codex_config::ConfigRequirementsToml; use codex_config::ConstrainedWithSource; use codex_config::FeatureRequirementsToml; -use codex_config::McpServerIdentity; use codex_config::McpServerRequirement; use codex_config::PluginRequirementsToml; use codex_config::ProfileV2Name; @@ -40,7 +39,6 @@ use codex_config::types::AuthKeyringBackendKind; use codex_config::types::History; use codex_config::types::McpServerConfig; use codex_config::types::McpServerDisabledReason; -use codex_config::types::McpServerTransportConfig; use codex_config::types::MemoriesConfig; use codex_config::types::ModelAvailabilityNuxConfig; use codex_config::types::Notice; @@ -1878,7 +1876,7 @@ fn filter_mcp_servers_by_requirements( let allowed = allowlist .value .get(name) - .is_some_and(|requirement| mcp_server_matches_requirement(requirement, server)); + .is_some_and(|requirement| requirement.matches(server)); if allowed { server.disabled_reason = None; } else { @@ -1907,7 +1905,7 @@ fn filter_plugin_mcp_servers_by_requirements( for (name, server) in mcp_servers.iter_mut() { let allowed = plugin_mcp_requirements .and_then(|mcp_requirements| mcp_requirements.get(name)) - .is_some_and(|requirement| mcp_server_matches_requirement(requirement, server)); + .is_some_and(|requirement| requirement.matches(server)); if allowed { server.disabled_reason = None; } else { @@ -1970,28 +1968,6 @@ where Ok(false) } -fn mcp_server_matches_requirement( - requirement: &McpServerRequirement, - server: &McpServerConfig, -) -> bool { - match requirement { - McpServerRequirement::Identity { identity } => match identity { - McpServerIdentity::Command { - command: want_command, - } => matches!( - &server.transport, - McpServerTransportConfig::Stdio { command: got_command, .. } - if got_command == want_command - ), - McpServerIdentity::Url { url: want_url } => matches!( - &server.transport, - McpServerTransportConfig::StreamableHttp { url: got_url, .. } - if got_url == want_url - ), - }, - } -} - pub async fn load_global_mcp_servers( codex_home: &Path, ) -> std::io::Result> { From 99febf800d65f32cd4c71161e9d428cf34446c47 Mon Sep 17 00:00:00 2001 From: Felix Xia Date: Tue, 23 Jun 2026 20:54:34 +0100 Subject: [PATCH 3/8] Test MCP matcher enforcement in config filtering --- codex-rs/core/src/config/config_tests.rs | 180 ++++++++++++++++++++++- 1 file changed, 179 insertions(+), 1 deletion(-) diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index b098ccbf84ae..fcdc3a071d3b 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -6,6 +6,10 @@ use codex_config::CONFIG_TOML_FILE; use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerSource; use codex_config::ConfigLayerStack; +use codex_config::McpServerCommandMatcher; +use codex_config::McpServerIdentity; +use codex_config::McpServerRequirement; +use codex_config::McpServerValueMatcher; use codex_config::ProfileV2Name; use codex_config::RequirementSource; use codex_config::Sourced; @@ -113,10 +117,14 @@ use std::time::Duration; use tempfile::TempDir; fn stdio_mcp(command: &str) -> McpServerConfig { + stdio_mcp_with_args(command, &[]) +} + +fn stdio_mcp_with_args(command: &str, args: &[&str]) -> McpServerConfig { McpServerConfig { transport: McpServerTransportConfig::Stdio { command: command.to_string(), - args: Vec::new(), + args: args.iter().map(ToString::to_string).collect(), env: None, env_vars: Vec::new(), cwd: None, @@ -4221,6 +4229,117 @@ fn filter_mcp_servers_by_allowlist_allows_all_when_unset() { ); } +#[test] +fn filter_mcp_servers_by_matchers_enforces_command_and_positional_args() { + let mut servers = HashMap::from([ + ( + "internal_mcp_proxy".to_string(), + stdio_mcp_with_args( + "company-cli", + &[ + "mcp", + "proxy", + "--server", + "https://pricing.mcp.internal.example.com", + ], + ), + ), + ( + "unlisted".to_string(), + stdio_mcp_with_args( + "company-cli", + &[ + "mcp", + "proxy", + "--server", + "https://pricing.mcp.internal.example.com", + ], + ), + ), + ( + "wrong-order".to_string(), + stdio_mcp_with_args( + "company-cli", + &[ + "proxy", + "mcp", + "--server", + "https://pricing.mcp.internal.example.com", + ], + ), + ), + ( + "trailing-arg".to_string(), + stdio_mcp_with_args( + "company-cli", + &[ + "mcp", + "proxy", + "--server", + "https://pricing.mcp.internal.example.com", + "--verbose", + ], + ), + ), + ( + "wrong-host".to_string(), + stdio_mcp_with_args( + "company-cli", + &["mcp", "proxy", "--server", "https://mcp.example.com"], + ), + ), + ]); + let source = RequirementSource::LegacyManagedConfigTomlFromMdm; + let requirement = McpServerRequirement::Command(McpServerCommandMatcher { + command: "company-cli".to_string(), + args: vec![ + McpServerValueMatcher::Exact { + value: "mcp".to_string(), + }, + McpServerValueMatcher::Exact { + value: "proxy".to_string(), + }, + McpServerValueMatcher::Exact { + value: "--server".to_string(), + }, + McpServerValueMatcher::Regex { + expression: + r"^https://[A-Za-z0-9-]+\.mcp\.internal\.example\.com(?::443)?(?:/.*)?$" + .to_string(), + }, + ], + }); + let requirements = Sourced::new( + BTreeMap::from([ + ("internal_mcp_proxy".to_string(), requirement.clone()), + ("wrong-order".to_string(), requirement.clone()), + ("trailing-arg".to_string(), requirement.clone()), + ("wrong-host".to_string(), requirement), + ]), + source.clone(), + ); + + filter_mcp_servers_by_requirements(&mut servers, Some(&requirements)); + + let reason = Some(McpServerDisabledReason::Requirements { source }); + assert_eq!( + servers + .iter() + .map(|(name, server)| ( + name.clone(), + (server.enabled, server.disabled_reason.clone()) + )) + .collect::)>>(), + HashMap::from([ + ("internal_mcp_proxy".to_string(), (true, None)), + ("unlisted".to_string(), (false, reason.clone())), + ("wrong-order".to_string(), (false, reason.clone())), + ("trailing-arg".to_string(), (false, reason.clone())), + ("wrong-host".to_string(), (false, reason)), + ]) + ); +} + #[test] fn filter_mcp_servers_by_allowlist_blocks_all_when_empty() { let mut servers = HashMap::from([ @@ -4351,6 +4470,65 @@ fn filter_plugin_mcp_servers_by_allowlist_blocks_unlisted_plugin() { ); } +#[test] +fn filter_plugin_mcp_servers_by_matchers_enforces_name_and_invocation() { + const MATCHED_SERVER: &str = "matched"; + const MISMATCHED_SERVER: &str = "mismatched"; + const UNLISTED_SERVER: &str = "unlisted"; + + let mut servers = HashMap::from([ + ( + MATCHED_SERVER.to_string(), + stdio_mcp_with_args("company-cli", &["approved"]), + ), + ( + MISMATCHED_SERVER.to_string(), + stdio_mcp_with_args("company-cli", &["rejected"]), + ), + ( + UNLISTED_SERVER.to_string(), + stdio_mcp_with_args("company-cli", &["approved"]), + ), + ]); + let source = RequirementSource::LegacyManagedConfigTomlFromMdm; + let requirement = McpServerRequirement::Command(McpServerCommandMatcher { + command: "company-cli".to_string(), + args: vec![McpServerValueMatcher::Exact { + value: "approved".to_string(), + }], + }); + let requirements = Sourced::new( + BTreeMap::from([( + "sample@test".to_string(), + codex_config::PluginRequirementsToml { + mcp_servers: Some(BTreeMap::from([ + (MATCHED_SERVER.to_string(), requirement.clone()), + (MISMATCHED_SERVER.to_string(), requirement), + ])), + }, + )]), + source.clone(), + ); + + filter_plugin_mcp_servers_by_requirements("sample@test", &mut servers, Some(&requirements)); + + let reason = Some(McpServerDisabledReason::Requirements { source }); + assert_eq!( + servers + .iter() + .map(|(name, server)| ( + name.clone(), + (server.enabled, server.disabled_reason.clone()) + )) + .collect::)>>(), + HashMap::from([ + (MATCHED_SERVER.to_string(), (true, None)), + (MISMATCHED_SERVER.to_string(), (false, reason.clone())), + (UNLISTED_SERVER.to_string(), (false, reason)), + ]) + ); +} + #[tokio::test] async fn rebuild_preserving_session_layers_refreshes_requirements() -> std::io::Result<()> { let codex_home = TempDir::new()?; From bfd179419fe70b2f1fd30a401beb71db17992f62 Mon Sep 17 00:00:00 2001 From: Felix Xia Date: Tue, 23 Jun 2026 20:54:46 +0100 Subject: [PATCH 4/8] Make MCP requirements atomic across layers --- codex-rs/config/src/merge.rs | 49 +++++++++++ codex-rs/config/src/merge_tests.rs | 43 ++++++++++ .../config/src/requirements_layers/stack.rs | 6 +- .../src/requirements_layers/stack_tests.rs | 82 ++++++++++++++++++- 4 files changed, 177 insertions(+), 3 deletions(-) diff --git a/codex-rs/config/src/merge.rs b/codex-rs/config/src/merge.rs index 94d670154663..b890ea4f6114 100644 --- a/codex-rs/config/src/merge.rs +++ b/codex-rs/config/src/merge.rs @@ -3,11 +3,28 @@ use crate::key_aliases::normalized_with_key_aliases; use codex_network_proxy::normalize_host; use toml::Value as TomlValue; +const ATOMIC_REQUIREMENT_PATHS: &[&[&str]] = + &[&["mcp_servers", "*"], &["plugins", "*", "mcp_servers", "*"]]; + /// Merge config `overlay` into `base`, giving `overlay` precedence. pub fn merge_toml_values(base: &mut TomlValue, overlay: &TomlValue) { merge_toml_values_at_path(base, overlay, &mut Vec::new()); } +/// Merge a requirements layer while treating selected requirement values as +/// atomic. +/// +/// The regular TOML merge recursively combines tables. Each named MCP server +/// requirement instead represents one complete requirement form, so combining +/// its internal fields across layers could retain parts of both definitions. +/// After the regular merge, reapply higher-priority values at the configured +/// atomic paths so each same-name requirement is replaced as a whole. A `*` +/// path segment matches every key at that level. +pub(crate) fn merge_requirements_toml_values(base: &mut TomlValue, overlay: &TomlValue) { + merge_toml_values(base, overlay); + apply_atomic_overrides(base, overlay, ATOMIC_REQUIREMENT_PATHS); +} + fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &mut Vec) { if let TomlValue::Table(overlay_table) = overlay && let TomlValue::Table(base_table) = base @@ -34,6 +51,38 @@ fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &m } } +fn apply_atomic_overrides(base: &mut TomlValue, overlay: &TomlValue, atomic_paths: &[&[&str]]) { + for path in atomic_paths { + apply_atomic_override_at_path(base, overlay, path); + } +} + +fn apply_atomic_override_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &[&str]) { + let Some((segment, remaining)) = path.split_first() else { + *base = overlay.clone(); + return; + }; + let Some(base_table) = base.as_table_mut() else { + return; + }; + let Some(overlay_table) = overlay.as_table() else { + return; + }; + + if *segment == "*" { + for (key, overlay_value) in overlay_table { + let Some(base_value) = base_table.get_mut(key) else { + continue; + }; + apply_atomic_override_at_path(base_value, overlay_value, remaining); + } + } else if let Some(base_value) = base_table.get_mut(*segment) + && let Some(overlay_value) = overlay_table.get(*segment) + { + apply_atomic_override_at_path(base_value, overlay_value, remaining); + } +} + fn is_permission_network_domains_path(path: &[String]) -> bool { matches!( path, diff --git a/codex-rs/config/src/merge_tests.rs b/codex-rs/config/src/merge_tests.rs index f9da6e7ebc14..2d2714b23f2b 100644 --- a/codex-rs/config/src/merge_tests.rs +++ b/codex-rs/config/src/merge_tests.rs @@ -124,3 +124,46 @@ fn merge_toml_values_normalizes_permission_network_domains_before_overlaying() { ); assert_eq!(base, expected); } + +#[test] +fn merge_requirements_toml_values_replaces_named_mcp_requirements_atomically() { + let mut base = parse_toml( + r#" +[mcp_servers.proxy.identity] +command = "legacy-proxy" + +[mcp_servers.docs.identity] +command = "docs-server" + +[plugins."sample@test".mcp_servers.proxy.identity] +command = "legacy-plugin-proxy" +"#, + ); + let overlay = parse_toml( + r#" +[mcp_servers.proxy] +command = "company-cli" +args = [{ match = "exact", value = "mcp" }] + +[plugins."sample@test".mcp_servers.proxy] +url = { match = "prefix", value = "https://mcp.example.com/" } +"#, + ); + + merge_requirements_toml_values(&mut base, &overlay); + + let expected = parse_toml( + r#" +[mcp_servers.proxy] +command = "company-cli" +args = [{ match = "exact", value = "mcp" }] + +[mcp_servers.docs.identity] +command = "docs-server" + +[plugins."sample@test".mcp_servers.proxy] +url = { match = "prefix", value = "https://mcp.example.com/" } +"#, + ); + assert_eq!(base, expected); +} diff --git a/codex-rs/config/src/requirements_layers/stack.rs b/codex-rs/config/src/requirements_layers/stack.rs index 3be1d11b5cf3..73cbaa8afbdf 100644 --- a/codex-rs/config/src/requirements_layers/stack.rs +++ b/codex-rs/config/src/requirements_layers/stack.rs @@ -10,6 +10,8 @@ //! - `rules.prefix_rules` append high-priority rules first. //! - `hooks` append high-priority event groups first while failing closed on //! active managed-dir conflicts. +//! - Higher-priority layers replace each named MCP server requirement as an +//! atomic value. //! - `permissions.filesystem.deny_read` is a high-priority-first union across //! layers. @@ -17,7 +19,7 @@ use crate::ConfigRequirementsToml; use crate::ConfigRequirementsWithSources; use crate::RequirementSource; use crate::Sourced; -use crate::merge::merge_toml_values; +use crate::merge::merge_requirements_toml_values; use std::cell::OnceCell; use std::io; use thiserror::Error; @@ -150,7 +152,7 @@ impl RequirementsLayerStack { let mut merged_toml = TomlValue::Table(toml::map::Map::new()); for layer in &layers { - merge_toml_values(&mut merged_toml, &layer.regular_toml); + merge_requirements_toml_values(&mut merged_toml, &layer.regular_toml); } let requirements: ConfigRequirementsToml = diff --git a/codex-rs/config/src/requirements_layers/stack_tests.rs b/codex-rs/config/src/requirements_layers/stack_tests.rs index a775f68a8341..5207c2a6d0d4 100644 --- a/codex-rs/config/src/requirements_layers/stack_tests.rs +++ b/codex-rs/config/src/requirements_layers/stack_tests.rs @@ -351,7 +351,7 @@ alpha = true } #[test] -fn mcp_requirements_use_regular_toml_merge() { +fn higher_priority_mcp_requirements_replace_named_rules() { let composed = compose(vec![ layer( "req_low", @@ -360,6 +360,9 @@ fn mcp_requirements_use_regular_toml_merge() { [mcp_servers.shared.identity] command = "low-mcp" +[mcp_servers.changed_form.identity] +command = "legacy-proxy" + [mcp_servers.low.identity] url = "https://low.example.com/mcp" "#, @@ -370,6 +373,10 @@ url = "https://low.example.com/mcp" r#" [mcp_servers.shared.identity] command = "high-mcp" + +[mcp_servers.changed_form] +command = "company-cli" +args = [{ match = "exact", value = "mcp" }] "#, ), ]) @@ -380,6 +387,10 @@ command = "high-mcp" composed, expected_requirements( r#" +[mcp_servers.changed_form] +command = "company-cli" +args = [{ match = "exact", value = "mcp" }] + [mcp_servers.low.identity] url = "https://low.example.com/mcp" @@ -390,6 +401,75 @@ command = "high-mcp" ); } +#[test] +fn higher_priority_identity_replaces_mcp_matcher() { + let composed = compose(vec![ + layer( + "req_low", + "Low", + r#" +[mcp_servers.shared] +command = "company-cli" +args = [{ match = "exact", value = "mcp" }] +"#, + ), + layer( + "req_high", + "High", + r#" +[mcp_servers.shared.identity] +command = "approved-mcp" +"#, + ), + ]) + .expect("compose requirements") + .expect("requirements present"); + + assert_eq!( + composed, + expected_requirements( + r#" +[mcp_servers.shared.identity] +command = "approved-mcp" +"# + ) + ); +} + +#[test] +fn higher_priority_plugin_mcp_requirements_replace_named_rules() { + let composed = compose(vec![ + layer( + "req_low", + "Low", + r#" +[plugins."sample@test".mcp_servers.shared.identity] +command = "low-plugin-mcp" +"#, + ), + layer( + "req_high", + "High", + r#" +[plugins."sample@test".mcp_servers.shared] +url = { match = "regex", expression = '^https://high\.example\.com/mcp$' } +"#, + ), + ]) + .expect("compose requirements") + .expect("requirements present"); + + assert_eq!( + composed, + expected_requirements( + r#" +[plugins."sample@test".mcp_servers.shared] +url = { match = "regex", expression = '^https://high\.example\.com/mcp$' } +"# + ) + ); +} + #[test] fn network_maps_use_regular_toml_merge() { let composed = compose(vec![ From f42f6263de69569c8b0b3566c4f07b3238d96080 Mon Sep 17 00:00:00 2001 From: Felix Xia Date: Wed, 24 Jun 2026 19:47:36 +0100 Subject: [PATCH 5/8] Address MCP matcher review feedback --- codex-rs/config/src/config_requirements.rs | 26 +------ codex-rs/config/src/lib.rs | 4 +- codex-rs/config/src/mcp_requirements.rs | 67 +++++++++++++++++-- codex-rs/config/src/mcp_requirements_tests.rs | 66 ++++++++++++++++++ codex-rs/config/src/merge.rs | 23 ++++--- .../src/requirements_layers/stack_tests.rs | 53 +++++++++++++++ 6 files changed, 199 insertions(+), 40 deletions(-) diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index e06996077d84..73c94f35ed4b 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -19,8 +19,7 @@ use super::requirements_exec_policy::RequirementsExecPolicyToml; use crate::Constrained; use crate::ConstraintError; use crate::ManagedHooksRequirementsToml; -use crate::McpServerCommandMatcher; -use crate::McpServerUrlMatcher; +use crate::mcp_requirements::McpServerRequirement; use crate::mcp_types::AppToolApproval; use crate::permissions_toml::PermissionProfileToml; use crate::types::WindowsSandboxModeToml; @@ -219,26 +218,6 @@ impl ConfigRequirements { } } -#[derive(Deserialize, Debug, Clone, PartialEq, Eq)] -#[serde(untagged)] -pub enum McpServerIdentity { - Command { command: String }, - Url { url: String }, -} - -/// A requirement for one named MCP server. -/// -/// The `Identity` variant preserves the released exact-match contract. The -/// command and URL variants add matcher-based requirements under the same -/// `mcp_servers` namespace. -#[derive(Deserialize, Debug, Clone, PartialEq, Eq)] -#[serde(untagged)] -pub enum McpServerRequirement { - Identity { identity: McpServerIdentity }, - Command(McpServerCommandMatcher), - Url(McpServerUrlMatcher), -} - #[derive(Deserialize, Debug, Clone, Default, PartialEq, Eq)] pub struct PluginRequirementsToml { pub mcp_servers: Option>, @@ -281,7 +260,7 @@ pub enum MarketplaceAllowedSourceKind { impl PluginRequirementsToml { pub fn is_empty(&self) -> bool { - self.mcp_servers.as_ref().is_none_or(BTreeMap::is_empty) + self.mcp_servers.is_none() } } @@ -1591,6 +1570,7 @@ mod tests { use super::*; use crate::HookEventsToml; use crate::McpServerCommandMatcher; + use crate::McpServerIdentity; use crate::McpServerValueMatcher; use anyhow::Result; use codex_execpolicy::Decision; diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 918a48d05ed4..8b0989f84288 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -67,8 +67,6 @@ pub use config_requirements::FilesystemDenyReadPattern; pub use config_requirements::MarketplaceAllowedSourceKind; pub use config_requirements::MarketplaceAllowedSourceToml; pub use config_requirements::MarketplaceRequirementsToml; -pub use config_requirements::McpServerIdentity; -pub use config_requirements::McpServerRequirement; pub use config_requirements::NetworkConstraints; pub use config_requirements::NetworkDomainPermissionToml; pub use config_requirements::NetworkDomainPermissionsToml; @@ -115,6 +113,8 @@ pub use marketplace_edit::remove_user_marketplace_config; pub use mcp_edit::ConfigEditsBuilder; pub use mcp_edit::load_global_mcp_servers; pub use mcp_requirements::McpServerCommandMatcher; +pub use mcp_requirements::McpServerIdentity; +pub use mcp_requirements::McpServerRequirement; pub use mcp_requirements::McpServerUrlMatcher; pub use mcp_requirements::McpServerValueMatcher; pub use mcp_types::AppToolApproval; diff --git a/codex-rs/config/src/mcp_requirements.rs b/codex-rs/config/src/mcp_requirements.rs index be49fd15ecdb..67d489d2b6d3 100644 --- a/codex-rs/config/src/mcp_requirements.rs +++ b/codex-rs/config/src/mcp_requirements.rs @@ -1,9 +1,15 @@ -use crate::McpServerIdentity; -use crate::McpServerRequirement; use crate::mcp_types::McpServerConfig; use crate::mcp_types::McpServerTransportConfig; use regex_lite::Regex; use serde::Deserialize; +use serde::de::Error as _; + +#[derive(Deserialize, Debug, Clone, PartialEq, Eq)] +#[serde(untagged)] +pub enum McpServerIdentity { + Command { command: String }, + Url { url: String }, +} /// String matching operations available to managed MCP server matchers. #[derive(Deserialize, Debug, Clone, PartialEq, Eq)] @@ -16,14 +22,17 @@ pub enum McpServerValueMatcher { impl McpServerValueMatcher { fn compile_full_regex(expression: &str) -> Result { - Regex::new(&format!(r"\A(?:{expression})\z")) - .map_err(|err| format!("invalid regex `{expression}`: {err}")) + Regex::new(&format!(r"\A(?:{expression})\z")).map_err(|err| { + format!("regex `{expression}` cannot be used for full-value matching: {err}") + }) } fn validate(&self) -> Result<(), String> { let Self::Regex { expression } = self else { return Ok(()); }; + + Regex::new(expression).map_err(|err| format!("invalid regex `{expression}`: {err}"))?; Self::compile_full_regex(expression).map(|_| ()) } @@ -51,6 +60,56 @@ pub struct McpServerUrlMatcher { pub url: McpServerValueMatcher, } +/// A requirement for one named MCP server. +/// +/// The `Identity` variant preserves the released exact-match contract. The +/// command and URL variants add matcher-based requirements under the same +/// `mcp_servers` namespace. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum McpServerRequirement { + Identity { identity: McpServerIdentity }, + Command(McpServerCommandMatcher), + Url(McpServerUrlMatcher), +} + +#[derive(Deserialize)] +#[serde(untagged)] +enum RawMcpServerRequirement { + Identity { + identity: McpServerIdentity, + command: Option, + args: Option, + url: Option, + }, + Command(McpServerCommandMatcher), + Url(McpServerUrlMatcher), +} + +impl<'de> Deserialize<'de> for McpServerRequirement { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + match RawMcpServerRequirement::deserialize(deserializer)? { + RawMcpServerRequirement::Identity { + identity, + command, + args, + url, + } => { + if command.is_some() || args.is_some() || url.is_some() { + return Err(D::Error::custom( + "`identity` cannot be combined with matcher keys `command`, `args`, or `url`", + )); + } + Ok(Self::Identity { identity }) + } + RawMcpServerRequirement::Command(matcher) => Ok(Self::Command(matcher)), + RawMcpServerRequirement::Url(matcher) => Ok(Self::Url(matcher)), + } + } +} + impl McpServerRequirement { pub(crate) fn validate(&self) -> Result<(), String> { match self { diff --git a/codex-rs/config/src/mcp_requirements_tests.rs b/codex-rs/config/src/mcp_requirements_tests.rs index f0b4b7b268bf..ac453f6e1309 100644 --- a/codex-rs/config/src/mcp_requirements_tests.rs +++ b/codex-rs/config/src/mcp_requirements_tests.rs @@ -81,6 +81,21 @@ fn regex_matcher_allows_a_later_alternative_to_match_the_full_value() { assert!(matcher.matches("https://api.example.com/mcp")); } +#[test] +fn regex_matcher_validation_rejects_expression_that_cannot_be_wrapped() { + let matcher = McpServerValueMatcher::Regex { + expression: "(?x)mcp # trailing comment".to_string(), + }; + + let err = matcher + .validate() + .expect_err("expression should not be valid for full-value matching"); + assert!( + err.contains("cannot be used for full-value matching"), + "{err}" + ); +} + #[test] fn legacy_command_identity_keeps_ignoring_arguments() { let requirement: McpServerRequirement = toml::from_str( @@ -140,3 +155,54 @@ url = { match = "prefix", value = "https://mcp.example.com/" } }) ); } + +#[test] +fn requirement_rejects_identity_combined_with_matcher_keys() { + for contents in [ + r#" +command = "company-cli" +[identity] +command = "company-cli" +"#, + r#" +args = [] +[identity] +command = "company-cli" +"#, + r#" +url = { match = "prefix", value = "https://mcp.example.com/" } +[identity] +url = "https://mcp.example.com" +"#, + ] { + let err = toml::from_str::(contents) + .expect_err("identity and matcher keys should be mutually exclusive"); + assert!( + err.to_string().contains( + "`identity` cannot be combined with matcher keys `command`, `args`, or `url`" + ), + "{err}" + ); + } +} + +#[test] +fn identity_requirement_keeps_ignoring_unrelated_sibling_fields() { + let requirement: McpServerRequirement = toml::from_str( + r#" +unrelated = "ignored" +[identity] +command = "company-cli" +"#, + ) + .expect("legacy identity with unrelated sibling field"); + + assert_eq!( + requirement, + McpServerRequirement::Identity { + identity: McpServerIdentity::Command { + command: "company-cli".to_string(), + }, + } + ); +} diff --git a/codex-rs/config/src/merge.rs b/codex-rs/config/src/merge.rs index b890ea4f6114..0708d8e38ec8 100644 --- a/codex-rs/config/src/merge.rs +++ b/codex-rs/config/src/merge.rs @@ -18,11 +18,14 @@ pub fn merge_toml_values(base: &mut TomlValue, overlay: &TomlValue) { /// requirement instead represents one complete requirement form, so combining /// its internal fields across layers could retain parts of both definitions. /// After the regular merge, reapply higher-priority values at the configured -/// atomic paths so each same-name requirement is replaced as a whole. A `*` -/// path segment matches every key at that level. +/// atomic paths so each same-name requirement is replaced as a whole. An +/// explicitly empty atomic map clears lower-priority entries. A `*` path +/// segment matches every key at that level. pub(crate) fn merge_requirements_toml_values(base: &mut TomlValue, overlay: &TomlValue) { merge_toml_values(base, overlay); - apply_atomic_overrides(base, overlay, ATOMIC_REQUIREMENT_PATHS); + for path in ATOMIC_REQUIREMENT_PATHS { + apply_atomic_override_at_path(base, overlay, path); + } } fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &mut Vec) { @@ -51,21 +54,19 @@ fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &m } } -fn apply_atomic_overrides(base: &mut TomlValue, overlay: &TomlValue, atomic_paths: &[&[&str]]) { - for path in atomic_paths { - apply_atomic_override_at_path(base, overlay, path); - } -} - fn apply_atomic_override_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &[&str]) { let Some((segment, remaining)) = path.split_first() else { *base = overlay.clone(); return; }; - let Some(base_table) = base.as_table_mut() else { + let Some(overlay_table) = overlay.as_table() else { return; }; - let Some(overlay_table) = overlay.as_table() else { + if *segment == "*" && remaining.is_empty() && overlay_table.is_empty() { + *base = overlay.clone(); + return; + } + let Some(base_table) = base.as_table_mut() else { return; }; diff --git a/codex-rs/config/src/requirements_layers/stack_tests.rs b/codex-rs/config/src/requirements_layers/stack_tests.rs index 5207c2a6d0d4..f31e56cdf2c0 100644 --- a/codex-rs/config/src/requirements_layers/stack_tests.rs +++ b/codex-rs/config/src/requirements_layers/stack_tests.rs @@ -436,6 +436,25 @@ command = "approved-mcp" ); } +#[test] +fn higher_priority_empty_mcp_allowlist_clears_lower_rules() { + let composed = compose(vec![ + layer( + "req_low", + "Low", + r#" +[mcp_servers.server.identity] +command = "low-mcp" +"#, + ), + layer("req_high", "High", "mcp_servers = {}"), + ]) + .expect("compose requirements") + .expect("requirements present"); + + assert_eq!(composed, expected_requirements("mcp_servers = {}")); +} + #[test] fn higher_priority_plugin_mcp_requirements_replace_named_rules() { let composed = compose(vec![ @@ -470,6 +489,40 @@ url = { match = "regex", expression = '^https://high\.example\.com/mcp$' } ); } +#[test] +fn higher_priority_empty_plugin_mcp_allowlist_clears_lower_rules() { + let composed = compose(vec![ + layer( + "req_low", + "Low", + r#" +[plugins."sample@test".mcp_servers.server.identity] +command = "low-plugin-mcp" +"#, + ), + layer( + "req_high", + "High", + r#" +[plugins."sample@test"] +mcp_servers = {} +"#, + ), + ]) + .expect("compose requirements") + .expect("requirements present"); + + assert_eq!( + composed, + expected_requirements( + r#" +[plugins."sample@test"] +mcp_servers = {} +"# + ) + ); +} + #[test] fn network_maps_use_regular_toml_merge() { let composed = compose(vec![ From 81840be9b66ab74c574fa655d7afee098acfeef9 Mon Sep 17 00:00:00 2001 From: Felix Xia Date: Thu, 25 Jun 2026 21:39:33 +0100 Subject: [PATCH 6/8] Nest MCP matchers under identity --- codex-rs/config/src/config_requirements.rs | 22 +++---- codex-rs/config/src/lib.rs | 1 - codex-rs/config/src/mcp_requirements.rs | 65 +++++++++---------- codex-rs/config/src/mcp_requirements_tests.rs | 51 +++++++++------ codex-rs/core/src/config/config_tests.rs | 4 +- 5 files changed, 72 insertions(+), 71 deletions(-) diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index 73c94f35ed4b..60f0023d7b7f 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -3526,14 +3526,13 @@ command = "python3 /enterprise/hooks/pre.py" #[test] fn deserialize_mcp_server_matcher_requirements() -> Result<()> { let toml_str = r#" - [mcp_servers.internal_mcp_proxy] - command = "company-cli" - args = [ + [mcp_servers.internal_mcp_proxy.identity] + command = { executable = "company-cli", args = [ { match = "exact", value = "mcp" }, { match = "exact", value = "proxy" }, { match = "exact", value = "--server" }, { match = "regex", expression = '^https://[A-Za-z0-9-]+\.mcp\.internal\.example\.com(?::443)?(?:/.*)?$' }, - ] + ] } "#; let requirements: ConfigRequirements = with_unknown_source(from_str(toml_str)?).try_into()?; @@ -3544,7 +3543,7 @@ command = "python3 /enterprise/hooks/pre.py" BTreeMap::from([( "internal_mcp_proxy".to_string(), McpServerRequirement::Command(McpServerCommandMatcher { - command: "company-cli".to_string(), + executable: "company-cli".to_string(), args: vec![ McpServerValueMatcher::Exact { value: "mcp".to_string(), @@ -3571,7 +3570,7 @@ command = "python3 /enterprise/hooks/pre.py" #[test] fn invalid_mcp_server_requirement_regex_reports_the_server_name_and_source() -> Result<()> { let toml_str = r#" - [mcp_servers.broken_rule] + [mcp_servers.broken_rule.identity] url = { match = "regex", expression = "[" } "#; @@ -3644,12 +3643,11 @@ command = "python3 /enterprise/hooks/pre.py" #[test] fn deserialize_plugin_mcp_server_matcher_requirement() -> Result<()> { let toml_str = r#" - [plugins."sample@test".mcp_servers.internal_proxy] - command = "company-cli" - args = [ + [plugins."sample@test".mcp_servers.internal_proxy.identity] + command = { executable = "company-cli", args = [ { match = "exact", value = "mcp" }, { match = "regex", expression = '^https://[a-z]+\.example\.com$' }, - ] + ] } "#; let requirements: ConfigRequirements = with_unknown_source(from_str(toml_str)?).try_into()?; @@ -3663,7 +3661,7 @@ command = "python3 /enterprise/hooks/pre.py" mcp_servers: Some(BTreeMap::from([( "internal_proxy".to_string(), McpServerRequirement::Command(McpServerCommandMatcher { - command: "company-cli".to_string(), + executable: "company-cli".to_string(), args: vec![ McpServerValueMatcher::Exact { value: "mcp".to_string(), @@ -3685,7 +3683,7 @@ command = "python3 /enterprise/hooks/pre.py" #[test] fn invalid_plugin_mcp_server_regex_reports_plugin_and_server_name() -> Result<()> { let toml_str = r#" - [plugins."sample@test".mcp_servers.broken_rule] + [plugins."sample@test".mcp_servers.broken_rule.identity] url = { match = "regex", expression = "[" } "#; diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 8b0989f84288..2840dfc74d89 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -115,7 +115,6 @@ pub use mcp_edit::load_global_mcp_servers; pub use mcp_requirements::McpServerCommandMatcher; pub use mcp_requirements::McpServerIdentity; pub use mcp_requirements::McpServerRequirement; -pub use mcp_requirements::McpServerUrlMatcher; pub use mcp_requirements::McpServerValueMatcher; pub use mcp_types::AppToolApproval; pub use mcp_types::DEFAULT_MCP_SERVER_ENVIRONMENT_ID; diff --git a/codex-rs/config/src/mcp_requirements.rs b/codex-rs/config/src/mcp_requirements.rs index 67d489d2b6d3..b8f9a12d69fe 100644 --- a/codex-rs/config/src/mcp_requirements.rs +++ b/codex-rs/config/src/mcp_requirements.rs @@ -2,7 +2,6 @@ use crate::mcp_types::McpServerConfig; use crate::mcp_types::McpServerTransportConfig; use regex_lite::Regex; use serde::Deserialize; -use serde::de::Error as _; #[derive(Deserialize, Debug, Clone, PartialEq, Eq)] #[serde(untagged)] @@ -50,39 +49,45 @@ impl McpServerValueMatcher { #[derive(Deserialize, Debug, Clone, PartialEq, Eq)] #[serde(deny_unknown_fields)] pub struct McpServerCommandMatcher { - pub command: String, + pub executable: String, pub args: Vec, } #[derive(Deserialize, Debug, Clone, PartialEq, Eq)] #[serde(deny_unknown_fields)] -pub struct McpServerUrlMatcher { - pub url: McpServerValueMatcher, +struct RawMcpServerCommandIdentity { + command: McpServerCommandMatcher, +} + +#[derive(Deserialize, Debug, Clone, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +struct RawMcpServerUrlIdentity { + url: McpServerValueMatcher, } /// A requirement for one named MCP server. /// /// The `Identity` variant preserves the released exact-match contract. The -/// command and URL variants add matcher-based requirements under the same -/// `mcp_servers` namespace. +/// command and URL variants are the normalized matcher-based forms accepted +/// under the `identity` key. #[derive(Debug, Clone, PartialEq, Eq)] pub enum McpServerRequirement { Identity { identity: McpServerIdentity }, Command(McpServerCommandMatcher), - Url(McpServerUrlMatcher), + Url(McpServerValueMatcher), +} + +#[derive(Deserialize)] +struct RawMcpServerRequirement { + identity: RawMcpServerIdentity, } #[derive(Deserialize)] #[serde(untagged)] -enum RawMcpServerRequirement { - Identity { - identity: McpServerIdentity, - command: Option, - args: Option, - url: Option, - }, - Command(McpServerCommandMatcher), - Url(McpServerUrlMatcher), +enum RawMcpServerIdentity { + Exact(McpServerIdentity), + Command(RawMcpServerCommandIdentity), + Url(RawMcpServerUrlIdentity), } impl<'de> Deserialize<'de> for McpServerRequirement { @@ -90,22 +95,12 @@ impl<'de> Deserialize<'de> for McpServerRequirement { where D: serde::Deserializer<'de>, { - match RawMcpServerRequirement::deserialize(deserializer)? { - RawMcpServerRequirement::Identity { - identity, - command, - args, - url, - } => { - if command.is_some() || args.is_some() || url.is_some() { - return Err(D::Error::custom( - "`identity` cannot be combined with matcher keys `command`, `args`, or `url`", - )); - } - Ok(Self::Identity { identity }) - } - RawMcpServerRequirement::Command(matcher) => Ok(Self::Command(matcher)), - RawMcpServerRequirement::Url(matcher) => Ok(Self::Url(matcher)), + let RawMcpServerRequirement { identity } = + RawMcpServerRequirement::deserialize(deserializer)?; + match identity { + RawMcpServerIdentity::Exact(identity) => Ok(Self::Identity { identity }), + RawMcpServerIdentity::Command(matcher) => Ok(Self::Command(matcher.command)), + RawMcpServerIdentity::Url(matcher) => Ok(Self::Url(matcher.url)), } } } @@ -122,7 +117,7 @@ impl McpServerRequirement { } Ok(()) } - Self::Url(matcher) => matcher.url.validate(), + Self::Url(matcher) => matcher.validate(), } } @@ -147,7 +142,7 @@ impl McpServerRequirement { McpServerTransportConfig::StreamableHttp { url: got_url, .. }, ) => got_url == want_url, (Self::Command(matcher), McpServerTransportConfig::Stdio { command, args, .. }) => { - matcher.command == *command + matcher.executable == *command && matcher.args.len() == args.len() && matcher .args @@ -156,7 +151,7 @@ impl McpServerRequirement { .all(|(matcher, arg)| matcher.matches(arg)) } (Self::Url(matcher), McpServerTransportConfig::StreamableHttp { url, .. }) => { - matcher.url.matches(url) + matcher.matches(url) } _ => false, } diff --git a/codex-rs/config/src/mcp_requirements_tests.rs b/codex-rs/config/src/mcp_requirements_tests.rs index ac453f6e1309..b854873568ba 100644 --- a/codex-rs/config/src/mcp_requirements_tests.rs +++ b/codex-rs/config/src/mcp_requirements_tests.rs @@ -32,7 +32,7 @@ fn stdio_server(command: &str, args: &[&str]) -> McpServerConfig { #[test] fn command_matcher_matches_exact_positional_arguments() { let requirement = McpServerRequirement::Command(McpServerCommandMatcher { - command: "company-cli".to_string(), + executable: "company-cli".to_string(), args: vec![ McpServerValueMatcher::Exact { value: "mcp".to_string(), @@ -117,16 +117,17 @@ command = "company-cli" fn requirement_deserializes_command_and_url_matcher_shapes() { let command: McpServerRequirement = toml::from_str( r#" -command = "company-cli" -args = [ +[identity] +command = { executable = "company-cli", args = [ { match = "exact", value = "mcp" }, { match = "regex", expression = '^https://[a-z]+\.example\.com$' }, -] +] } "#, ) .expect("command matcher"); let url: McpServerRequirement = toml::from_str( r#" +[identity] url = { match = "prefix", value = "https://mcp.example.com/" } "#, ) @@ -135,7 +136,7 @@ url = { match = "prefix", value = "https://mcp.example.com/" } assert_eq!( command, McpServerRequirement::Command(McpServerCommandMatcher { - command: "company-cli".to_string(), + executable: "company-cli".to_string(), args: vec![ McpServerValueMatcher::Exact { value: "mcp".to_string(), @@ -148,44 +149,52 @@ url = { match = "prefix", value = "https://mcp.example.com/" } ); assert_eq!( url, - McpServerRequirement::Url(McpServerUrlMatcher { - url: McpServerValueMatcher::Prefix { - value: "https://mcp.example.com/".to_string(), - }, + McpServerRequirement::Url(McpServerValueMatcher::Prefix { + value: "https://mcp.example.com/".to_string(), }) ); } #[test] -fn requirement_rejects_identity_combined_with_matcher_keys() { +fn requirement_rejects_matchers_outside_identity() { for contents in [ r#" command = "company-cli" -[identity] -command = "company-cli" "#, r#" -args = [] -[identity] -command = "company-cli" +command = { executable = "company-cli", args = [] } "#, r#" url = { match = "prefix", value = "https://mcp.example.com/" } -[identity] -url = "https://mcp.example.com" "#, ] { let err = toml::from_str::(contents) - .expect_err("identity and matcher keys should be mutually exclusive"); + .expect_err("MCP server requirements should use the identity key"); assert!( - err.to_string().contains( - "`identity` cannot be combined with matcher keys `command`, `args`, or `url`" - ), + err.to_string().contains("missing field `identity`"), "{err}" ); } } +#[test] +fn matcher_identity_rejects_unknown_fields() { + for contents in [ + r#" +[identity] +unknown = "value" +command = { executable = "company-cli", args = [] } +"#, + r#" +[identity] +command = { executable = "company-cli", args = [], unknown = "value" } +"#, + ] { + toml::from_str::(contents) + .expect_err("matcher identities should reject unknown fields"); + } +} + #[test] fn identity_requirement_keeps_ignoring_unrelated_sibling_fields() { let requirement: McpServerRequirement = toml::from_str( diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index fcdc3a071d3b..8b1926a88215 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -4291,7 +4291,7 @@ fn filter_mcp_servers_by_matchers_enforces_command_and_positional_args() { ]); let source = RequirementSource::LegacyManagedConfigTomlFromMdm; let requirement = McpServerRequirement::Command(McpServerCommandMatcher { - command: "company-cli".to_string(), + executable: "company-cli".to_string(), args: vec![ McpServerValueMatcher::Exact { value: "mcp".to_string(), @@ -4492,7 +4492,7 @@ fn filter_plugin_mcp_servers_by_matchers_enforces_name_and_invocation() { ]); let source = RequirementSource::LegacyManagedConfigTomlFromMdm; let requirement = McpServerRequirement::Command(McpServerCommandMatcher { - command: "company-cli".to_string(), + executable: "company-cli".to_string(), args: vec![McpServerValueMatcher::Exact { value: "approved".to_string(), }], From 12272973f8e304681b230e5846f98f14bb0b708c Mon Sep 17 00:00:00 2001 From: Felix Xia Date: Thu, 25 Jun 2026 21:39:36 +0100 Subject: [PATCH 7/8] Restore regular requirements merge behavior --- codex-rs/config/src/merge.rs | 50 ------- codex-rs/config/src/merge_tests.rs | 43 ------ .../config/src/requirements_layers/stack.rs | 6 +- .../src/requirements_layers/stack_tests.rs | 135 +----------------- 4 files changed, 3 insertions(+), 231 deletions(-) diff --git a/codex-rs/config/src/merge.rs b/codex-rs/config/src/merge.rs index 0708d8e38ec8..94d670154663 100644 --- a/codex-rs/config/src/merge.rs +++ b/codex-rs/config/src/merge.rs @@ -3,31 +3,11 @@ use crate::key_aliases::normalized_with_key_aliases; use codex_network_proxy::normalize_host; use toml::Value as TomlValue; -const ATOMIC_REQUIREMENT_PATHS: &[&[&str]] = - &[&["mcp_servers", "*"], &["plugins", "*", "mcp_servers", "*"]]; - /// Merge config `overlay` into `base`, giving `overlay` precedence. pub fn merge_toml_values(base: &mut TomlValue, overlay: &TomlValue) { merge_toml_values_at_path(base, overlay, &mut Vec::new()); } -/// Merge a requirements layer while treating selected requirement values as -/// atomic. -/// -/// The regular TOML merge recursively combines tables. Each named MCP server -/// requirement instead represents one complete requirement form, so combining -/// its internal fields across layers could retain parts of both definitions. -/// After the regular merge, reapply higher-priority values at the configured -/// atomic paths so each same-name requirement is replaced as a whole. An -/// explicitly empty atomic map clears lower-priority entries. A `*` path -/// segment matches every key at that level. -pub(crate) fn merge_requirements_toml_values(base: &mut TomlValue, overlay: &TomlValue) { - merge_toml_values(base, overlay); - for path in ATOMIC_REQUIREMENT_PATHS { - apply_atomic_override_at_path(base, overlay, path); - } -} - fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &mut Vec) { if let TomlValue::Table(overlay_table) = overlay && let TomlValue::Table(base_table) = base @@ -54,36 +34,6 @@ fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &m } } -fn apply_atomic_override_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &[&str]) { - let Some((segment, remaining)) = path.split_first() else { - *base = overlay.clone(); - return; - }; - let Some(overlay_table) = overlay.as_table() else { - return; - }; - if *segment == "*" && remaining.is_empty() && overlay_table.is_empty() { - *base = overlay.clone(); - return; - } - let Some(base_table) = base.as_table_mut() else { - return; - }; - - if *segment == "*" { - for (key, overlay_value) in overlay_table { - let Some(base_value) = base_table.get_mut(key) else { - continue; - }; - apply_atomic_override_at_path(base_value, overlay_value, remaining); - } - } else if let Some(base_value) = base_table.get_mut(*segment) - && let Some(overlay_value) = overlay_table.get(*segment) - { - apply_atomic_override_at_path(base_value, overlay_value, remaining); - } -} - fn is_permission_network_domains_path(path: &[String]) -> bool { matches!( path, diff --git a/codex-rs/config/src/merge_tests.rs b/codex-rs/config/src/merge_tests.rs index 2d2714b23f2b..f9da6e7ebc14 100644 --- a/codex-rs/config/src/merge_tests.rs +++ b/codex-rs/config/src/merge_tests.rs @@ -124,46 +124,3 @@ fn merge_toml_values_normalizes_permission_network_domains_before_overlaying() { ); assert_eq!(base, expected); } - -#[test] -fn merge_requirements_toml_values_replaces_named_mcp_requirements_atomically() { - let mut base = parse_toml( - r#" -[mcp_servers.proxy.identity] -command = "legacy-proxy" - -[mcp_servers.docs.identity] -command = "docs-server" - -[plugins."sample@test".mcp_servers.proxy.identity] -command = "legacy-plugin-proxy" -"#, - ); - let overlay = parse_toml( - r#" -[mcp_servers.proxy] -command = "company-cli" -args = [{ match = "exact", value = "mcp" }] - -[plugins."sample@test".mcp_servers.proxy] -url = { match = "prefix", value = "https://mcp.example.com/" } -"#, - ); - - merge_requirements_toml_values(&mut base, &overlay); - - let expected = parse_toml( - r#" -[mcp_servers.proxy] -command = "company-cli" -args = [{ match = "exact", value = "mcp" }] - -[mcp_servers.docs.identity] -command = "docs-server" - -[plugins."sample@test".mcp_servers.proxy] -url = { match = "prefix", value = "https://mcp.example.com/" } -"#, - ); - assert_eq!(base, expected); -} diff --git a/codex-rs/config/src/requirements_layers/stack.rs b/codex-rs/config/src/requirements_layers/stack.rs index 73cbaa8afbdf..3be1d11b5cf3 100644 --- a/codex-rs/config/src/requirements_layers/stack.rs +++ b/codex-rs/config/src/requirements_layers/stack.rs @@ -10,8 +10,6 @@ //! - `rules.prefix_rules` append high-priority rules first. //! - `hooks` append high-priority event groups first while failing closed on //! active managed-dir conflicts. -//! - Higher-priority layers replace each named MCP server requirement as an -//! atomic value. //! - `permissions.filesystem.deny_read` is a high-priority-first union across //! layers. @@ -19,7 +17,7 @@ use crate::ConfigRequirementsToml; use crate::ConfigRequirementsWithSources; use crate::RequirementSource; use crate::Sourced; -use crate::merge::merge_requirements_toml_values; +use crate::merge::merge_toml_values; use std::cell::OnceCell; use std::io; use thiserror::Error; @@ -152,7 +150,7 @@ impl RequirementsLayerStack { let mut merged_toml = TomlValue::Table(toml::map::Map::new()); for layer in &layers { - merge_requirements_toml_values(&mut merged_toml, &layer.regular_toml); + merge_toml_values(&mut merged_toml, &layer.regular_toml); } let requirements: ConfigRequirementsToml = diff --git a/codex-rs/config/src/requirements_layers/stack_tests.rs b/codex-rs/config/src/requirements_layers/stack_tests.rs index f31e56cdf2c0..a775f68a8341 100644 --- a/codex-rs/config/src/requirements_layers/stack_tests.rs +++ b/codex-rs/config/src/requirements_layers/stack_tests.rs @@ -351,7 +351,7 @@ alpha = true } #[test] -fn higher_priority_mcp_requirements_replace_named_rules() { +fn mcp_requirements_use_regular_toml_merge() { let composed = compose(vec![ layer( "req_low", @@ -360,9 +360,6 @@ fn higher_priority_mcp_requirements_replace_named_rules() { [mcp_servers.shared.identity] command = "low-mcp" -[mcp_servers.changed_form.identity] -command = "legacy-proxy" - [mcp_servers.low.identity] url = "https://low.example.com/mcp" "#, @@ -373,10 +370,6 @@ url = "https://low.example.com/mcp" r#" [mcp_servers.shared.identity] command = "high-mcp" - -[mcp_servers.changed_form] -command = "company-cli" -args = [{ match = "exact", value = "mcp" }] "#, ), ]) @@ -387,10 +380,6 @@ args = [{ match = "exact", value = "mcp" }] composed, expected_requirements( r#" -[mcp_servers.changed_form] -command = "company-cli" -args = [{ match = "exact", value = "mcp" }] - [mcp_servers.low.identity] url = "https://low.example.com/mcp" @@ -401,128 +390,6 @@ command = "high-mcp" ); } -#[test] -fn higher_priority_identity_replaces_mcp_matcher() { - let composed = compose(vec![ - layer( - "req_low", - "Low", - r#" -[mcp_servers.shared] -command = "company-cli" -args = [{ match = "exact", value = "mcp" }] -"#, - ), - layer( - "req_high", - "High", - r#" -[mcp_servers.shared.identity] -command = "approved-mcp" -"#, - ), - ]) - .expect("compose requirements") - .expect("requirements present"); - - assert_eq!( - composed, - expected_requirements( - r#" -[mcp_servers.shared.identity] -command = "approved-mcp" -"# - ) - ); -} - -#[test] -fn higher_priority_empty_mcp_allowlist_clears_lower_rules() { - let composed = compose(vec![ - layer( - "req_low", - "Low", - r#" -[mcp_servers.server.identity] -command = "low-mcp" -"#, - ), - layer("req_high", "High", "mcp_servers = {}"), - ]) - .expect("compose requirements") - .expect("requirements present"); - - assert_eq!(composed, expected_requirements("mcp_servers = {}")); -} - -#[test] -fn higher_priority_plugin_mcp_requirements_replace_named_rules() { - let composed = compose(vec![ - layer( - "req_low", - "Low", - r#" -[plugins."sample@test".mcp_servers.shared.identity] -command = "low-plugin-mcp" -"#, - ), - layer( - "req_high", - "High", - r#" -[plugins."sample@test".mcp_servers.shared] -url = { match = "regex", expression = '^https://high\.example\.com/mcp$' } -"#, - ), - ]) - .expect("compose requirements") - .expect("requirements present"); - - assert_eq!( - composed, - expected_requirements( - r#" -[plugins."sample@test".mcp_servers.shared] -url = { match = "regex", expression = '^https://high\.example\.com/mcp$' } -"# - ) - ); -} - -#[test] -fn higher_priority_empty_plugin_mcp_allowlist_clears_lower_rules() { - let composed = compose(vec![ - layer( - "req_low", - "Low", - r#" -[plugins."sample@test".mcp_servers.server.identity] -command = "low-plugin-mcp" -"#, - ), - layer( - "req_high", - "High", - r#" -[plugins."sample@test"] -mcp_servers = {} -"#, - ), - ]) - .expect("compose requirements") - .expect("requirements present"); - - assert_eq!( - composed, - expected_requirements( - r#" -[plugins."sample@test"] -mcp_servers = {} -"# - ) - ); -} - #[test] fn network_maps_use_regular_toml_merge() { let composed = compose(vec![ From 3279e99ec1c48560d033413888d15c1b0fbde3f8 Mon Sep 17 00:00:00 2001 From: Felix Xia Date: Thu, 25 Jun 2026 21:45:23 +0100 Subject: [PATCH 8/8] Preserve existing plugin requirement emptiness --- codex-rs/config/src/config_requirements.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index 60f0023d7b7f..199ca4eafa21 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -260,7 +260,7 @@ pub enum MarketplaceAllowedSourceKind { impl PluginRequirementsToml { pub fn is_empty(&self) -> bool { - self.mcp_servers.is_none() + self.mcp_servers.as_ref().is_none_or(BTreeMap::is_empty) } }