From 70135cc051cdd2b26acd6a9873418263e1d18d87 Mon Sep 17 00:00:00 2001 From: Felix Xia Date: Wed, 24 Jun 2026 20:25:02 +0100 Subject: [PATCH] WIP: allow partial MCP config layers Snapshot shared in-progress state before reconciliation. --- .../app-server/src/config_manager_service.rs | 23 ++-- .../src/config_manager_service_tests.rs | 106 ++++++++++++++++++ codex-rs/config/src/cloud_config_layers.rs | 12 +- .../config/src/cloud_config_layers_tests.rs | 44 ++++++++ codex-rs/config/src/lib.rs | 1 + codex-rs/config/src/loader/layer_io.rs | 11 +- codex-rs/config/src/loader/macos.rs | 8 +- codex-rs/config/src/loader/mod.rs | 10 +- codex-rs/config/src/strict_config.rs | 72 +++++++++++- codex-rs/config/src/strict_config_tests.rs | 61 +++++++++- .../core/src/config/config_loader_tests.rs | 38 +++++++ 11 files changed, 343 insertions(+), 43 deletions(-) diff --git a/codex-rs/app-server/src/config_manager_service.rs b/codex-rs/app-server/src/config_manager_service.rs index 94980bf8e0..8c8ff227e2 100644 --- a/codex-rs/app-server/src/config_manager_service.rs +++ b/codex-rs/app-server/src/config_manager_service.rs @@ -20,6 +20,7 @@ use codex_config::ConfigLayerStackOrdering; use codex_config::ConfigRequirementsToml; use codex_config::config_toml::ConfigToml; use codex_config::merge_toml_values; +use codex_config::validate_config_toml_layer; use codex_core::config::deserialize_config_toml_with_base; use codex_core::config::edit::ConfigEdit; use codex_core::config::edit::ConfigEditsBuilder; @@ -293,21 +294,25 @@ impl ConfigManager { parsed_segments.push(segments); } - validate_config(&user_config).map_err(|err| { + validate_config_toml_layer(user_config.clone(), self.codex_home()).map_err(|err| { ConfigManagerError::write( ConfigWriteErrorCode::ConfigValidationError, format!("Invalid configuration: {err}"), ) })?; + + let mut user_config_without_mcp_servers = user_config.clone(); + if let Some(table) = user_config_without_mcp_servers.as_table_mut() { + table.remove("mcp_servers"); + } let user_config_toml = - deserialize_config_toml_with_base(user_config.clone(), self.codex_home()).map_err( - |err| { - ConfigManagerError::write( - ConfigWriteErrorCode::ConfigValidationError, - format!("Invalid configuration: {err}"), - ) - }, - )?; + deserialize_config_toml_with_base(user_config_without_mcp_servers, self.codex_home()) + .map_err(|err| { + ConfigManagerError::write( + ConfigWriteErrorCode::ConfigValidationError, + format!("Invalid configuration: {err}"), + ) + })?; validate_feature_requirements_for_config_toml( &user_config_toml, layers.requirements().feature_requirements.as_ref(), diff --git a/codex-rs/app-server/src/config_manager_service_tests.rs b/codex-rs/app-server/src/config_manager_service_tests.rs index 1ec79a749b..f46e9a31fd 100644 --- a/codex-rs/app-server/src/config_manager_service_tests.rs +++ b/codex-rs/app-server/src/config_manager_service_tests.rs @@ -341,6 +341,112 @@ async fn write_value_supports_custom_mcp_server_default_tool_approval_mode() -> Ok(()) } +#[tokio::test] +async fn write_value_allows_partial_mcp_server_override_over_cloud_config() -> Result<()> { + let tmp = tempdir().expect("tempdir"); + let config_path = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write(&config_path, "")?; + + let service = ConfigManager::new_for_tests( + tmp.path().to_path_buf(), + vec![], + LoaderOverrides::without_managed_config_for_tests(), + CloudConfigBundleFixture::loader_with_enterprise_config( + r#"[mcp_servers.docs] +command = "cloud-docs-server-v1" +enabled = false +"#, + ), + ); + + service + .write_value(ConfigValueWriteParams { + file_path: Some(config_path.display().to_string()), + key_path: "mcp_servers.docs.enabled".to_string(), + value: serde_json::json!(true), + merge_strategy: MergeStrategy::Replace, + expected_version: None, + }) + .await + .expect("partial MCP override should be valid after config layers merge"); + + let persisted: TomlValue = toml::from_str(&std::fs::read_to_string(&config_path)?)?; + let expected: TomlValue = toml::from_str( + r#"[mcp_servers.docs] +enabled = true +"#, + )?; + assert_eq!(persisted, expected); + + let config = service.load_latest_config(/*fallback_cwd*/ None).await?; + let server = config + .mcp_servers + .get() + .get("docs") + .expect("merged MCP server"); + assert!(server.enabled); + let codex_config::McpServerTransportConfig::Stdio { command, .. } = &server.transport else { + panic!("expected stdio MCP server"); + }; + assert_eq!(command, "cloud-docs-server-v1"); + + let updated_service = ConfigManager::new_for_tests( + tmp.path().to_path_buf(), + vec![], + LoaderOverrides::without_managed_config_for_tests(), + CloudConfigBundleFixture::loader_with_enterprise_config( + r#"[mcp_servers.docs] +command = "cloud-docs-server-v2" +enabled = false +"#, + ), + ); + let updated_config = updated_service + .load_latest_config(/*fallback_cwd*/ None) + .await?; + let updated_server = updated_config + .mcp_servers + .get() + .get("docs") + .expect("updated merged MCP server"); + assert!(updated_server.enabled); + let codex_config::McpServerTransportConfig::Stdio { command, .. } = &updated_server.transport + else { + panic!("expected stdio MCP server"); + }; + assert_eq!(command, "cloud-docs-server-v2"); + + Ok(()) +} + +#[tokio::test] +async fn write_value_rejects_partial_mcp_server_without_effective_transport() -> Result<()> { + let tmp = tempdir().expect("tempdir"); + let config_path = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write(&config_path, "")?; + let service = ConfigManager::without_managed_config_for_tests(tmp.path().to_path_buf()); + + let error = service + .write_value(ConfigValueWriteParams { + file_path: Some(config_path.display().to_string()), + key_path: "mcp_servers.docs.enabled".to_string(), + value: serde_json::json!(true), + merge_strategy: MergeStrategy::Replace, + expected_version: None, + }) + .await + .expect_err("an effective MCP server still needs a transport"); + + assert_eq!( + error.write_error_code(), + Some(ConfigWriteErrorCode::ConfigValidationError) + ); + assert!(error.to_string().contains("invalid transport"), "{error}"); + assert_eq!(std::fs::read_to_string(config_path)?, ""); + + Ok(()) +} + #[tokio::test] async fn read_includes_origins_and_layers() { let tmp = tempdir().expect("tempdir"); diff --git a/codex-rs/config/src/cloud_config_layers.rs b/codex-rs/config/src/cloud_config_layers.rs index f6fcd40b4a..1f1d3c7bff 100644 --- a/codex-rs/config/src/cloud_config_layers.rs +++ b/codex-rs/config/src/cloud_config_layers.rs @@ -7,9 +7,8 @@ use crate::ConfigLayerEntry; use crate::ConfigLayerSource; use crate::TomlValue; -use crate::config_toml::ConfigToml; use crate::loader::resolve_relative_paths_in_config_toml; -use crate::strict_config::config_error_from_ignored_toml_value_fields_for_source_name; +use crate::strict_config::config_error_from_config_toml_layer_for_source_name; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::AbsolutePathBufGuard; use serde::Deserialize; @@ -127,10 +126,11 @@ fn validate_fragment_strictly( base_dir: &AbsolutePathBuf, ) -> Result<(), CloudConfigLayerError> { let _guard = AbsolutePathBufGuard::new(base_dir.as_path()); - if let Some(config_error) = config_error_from_ignored_toml_value_fields_for_source_name::< - ConfigToml, - >(&source_ref.to_string(), raw_toml, value.clone()) - { + if let Some(config_error) = config_error_from_config_toml_layer_for_source_name( + &source_ref.to_string(), + raw_toml, + value.clone(), + ) { return Err(CloudConfigLayerError::Invalid { fragment: source_ref.clone(), message: config_error.message, diff --git a/codex-rs/config/src/cloud_config_layers_tests.rs b/codex-rs/config/src/cloud_config_layers_tests.rs index c2bc7383bc..1559042e28 100644 --- a/codex-rs/config/src/cloud_config_layers_tests.rs +++ b/codex-rs/config/src/cloud_config_layers_tests.rs @@ -79,6 +79,50 @@ fn strict_layers_reject_unknown_config_fields() { ); } +#[test] +fn strict_layers_allow_partial_mcp_server_fragments() { + let base_dir = base_dir(); + let layers = cloud_config_layers_from_fragments_strict( + vec![ + fragment( + "high", + "Enable docs", + r#"[mcp_servers.docs] +enabled = true +"#, + ), + fragment( + "low", + "Define docs", + r#"[mcp_servers.docs] +command = "cloud-docs-server" +enabled = false +"#, + ), + ], + &base_dir, + ) + .expect("partial cloud MCP fragments should be structurally valid"); + + let stack = ConfigLayerStack::new( + layers, + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("cloud config layers should compose"); + let config: ConfigToml = stack + .effective_config() + .try_into() + .expect("merged MCP server should be complete"); + let server = config.mcp_servers.get("docs").expect("merged MCP server"); + + assert!(server.enabled); + let crate::McpServerTransportConfig::Stdio { command, .. } = &server.transport else { + panic!("expected stdio MCP server"); + }; + assert_eq!(command, "cloud-docs-server"); +} + #[test] fn enterprise_layers_precede_user_and_override_system() { let base_dir = base_dir(); diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 1cbbbaf0a9..f857e1d7a0 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -147,6 +147,7 @@ pub use state::ConfigLayerStackOrdering; pub use state::ConfigLoadOptions; pub use state::LoaderOverrides; pub use strict_config::config_error_from_ignored_toml_fields; +pub use strict_config::validate_config_toml_layer; pub use thread_config::NoopThreadConfigLoader; pub use thread_config::RemoteThreadConfigLoader; pub use thread_config::SessionThreadConfig; diff --git a/codex-rs/config/src/loader/layer_io.rs b/codex-rs/config/src/loader/layer_io.rs index 947911a893..f604f1ff3c 100644 --- a/codex-rs/config/src/loader/layer_io.rs +++ b/codex-rs/config/src/loader/layer_io.rs @@ -2,11 +2,10 @@ use super::macos::ManagedAdminConfigLayer; #[cfg(target_os = "macos")] use super::macos::load_managed_admin_config_layer; -use crate::config_toml::ConfigToml; use crate::diagnostics::config_error_from_toml; use crate::diagnostics::io_error_from_config_error; use crate::state::LoaderOverrides; -use crate::strict_config::config_error_from_ignored_toml_value_fields; +use crate::strict_config::config_error_from_config_toml_layer; use codex_file_system::ExecutorFileSystem; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::AbsolutePathBufGuard; @@ -153,11 +152,9 @@ fn validate_config_toml_strictly( )); }; let _guard = AbsolutePathBufGuard::new(base_dir); - if let Some(config_error) = config_error_from_ignored_toml_value_fields::( - path.as_path(), - contents, - value.clone(), - ) { + if let Some(config_error) = + config_error_from_config_toml_layer(path.as_path(), contents, value.clone()) + { return Err(io_error_from_config_error( io::ErrorKind::InvalidData, config_error, diff --git a/codex-rs/config/src/loader/macos.rs b/codex-rs/config/src/loader/macos.rs index 974cb27604..d727abb367 100644 --- a/codex-rs/config/src/loader/macos.rs +++ b/codex-rs/config/src/loader/macos.rs @@ -1,10 +1,9 @@ use crate::RequirementsLayerEntry; use crate::config_requirements::RequirementSource; -use crate::config_toml::ConfigToml; use crate::diagnostics::ConfigDiagnosticSource; use crate::diagnostics::config_error_from_toml_for_source; use crate::diagnostics::io_error_from_config_error; -use crate::strict_config::config_error_from_ignored_toml_value_fields_for_source_name; +use crate::strict_config::config_error_from_config_toml_layer_for_source_name; use base64::Engine; use base64::prelude::BASE64_STANDARD; use codex_utils_absolute_path::AbsolutePathBufGuard; @@ -193,9 +192,8 @@ fn validate_managed_config_toml_strictly_if_requested( } let _guard = AbsolutePathBufGuard::new(base_dir); - if let Some(config_error) = config_error_from_ignored_toml_value_fields_for_source_name::< - ConfigToml, - >(source_name, raw_toml, parsed.clone()) + if let Some(config_error) = + config_error_from_config_toml_layer_for_source_name(source_name, raw_toml, parsed.clone()) { Err(io_error_from_config_error( io::ErrorKind::InvalidData, diff --git a/codex-rs/config/src/loader/mod.rs b/codex-rs/config/src/loader/mod.rs index 00110bc08b..a3a725be85 100644 --- a/codex-rs/config/src/loader/mod.rs +++ b/codex-rs/config/src/loader/mod.rs @@ -27,7 +27,7 @@ use crate::state::ConfigLayerEntry; use crate::state::ConfigLayerStack; use crate::state::ConfigLoadOptions; use crate::state::LoaderOverrides; -use crate::strict_config::config_error_from_ignored_toml_value_fields; +use crate::strict_config::config_error_from_config_toml_layer; use crate::strict_config::ignored_toml_value_field; use crate::strict_config::unknown_feature_toml_value_field; use crate::thread_config::ThreadConfigContext; @@ -525,11 +525,9 @@ fn validate_config_toml_strictly( base_dir: &Path, ) -> io::Result<()> { let _guard = AbsolutePathBufGuard::new(base_dir); - if let Some(config_error) = config_error_from_ignored_toml_value_fields::( - toml_file, - contents, - value.clone(), - ) { + if let Some(config_error) = + config_error_from_config_toml_layer(toml_file, contents, value.clone()) + { Err(io_error_from_config_error( io::ErrorKind::InvalidData, config_error, diff --git a/codex-rs/config/src/strict_config.rs b/codex-rs/config/src/strict_config.rs index c1523c706c..0ca82203b2 100644 --- a/codex-rs/config/src/strict_config.rs +++ b/codex-rs/config/src/strict_config.rs @@ -1,5 +1,7 @@ //! Strict config validation built on top of serde's ignored-field tracking. +use crate::RawMcpServerConfig; +use crate::config_toml::ConfigToml; use crate::diagnostics::ConfigDiagnosticSource; use crate::diagnostics::ConfigError; use crate::diagnostics::config_error_from_toml_for_source; @@ -8,10 +10,19 @@ use crate::diagnostics::span_for_config_path; use crate::diagnostics::span_for_toml_key_path; use crate::diagnostics::text_range_from_span; use codex_features::is_known_feature_key; +use codex_utils_absolute_path::AbsolutePathBufGuard; +use serde::Deserialize; use serde::de::DeserializeOwned; +use std::collections::HashMap; use std::path::Path; use toml::Value as TomlValue; +#[derive(Deserialize)] +struct ConfigTomlMcpServersLayer { + #[serde(default, rename = "mcp_servers")] + _mcp_servers: HashMap, +} + pub fn config_error_from_ignored_toml_fields( path: impl AsRef, contents: &str, @@ -25,30 +36,83 @@ pub fn config_error_from_ignored_toml_fields( } } -pub(crate) fn config_error_from_ignored_toml_value_fields( +pub(crate) fn config_error_from_config_toml_layer( path: impl AsRef, contents: &str, value: TomlValue, ) -> Option { - config_error_from_ignored_toml_value_fields_for_source::( + config_error_from_config_toml_layer_for_source( ConfigDiagnosticSource::Path(path.as_ref()), contents, value, ) } -pub(crate) fn config_error_from_ignored_toml_value_fields_for_source_name( +pub(crate) fn config_error_from_config_toml_layer_for_source_name( source_name: &str, contents: &str, value: TomlValue, ) -> Option { - config_error_from_ignored_toml_value_fields_for_source::( + config_error_from_config_toml_layer_for_source( ConfigDiagnosticSource::DisplayName(source_name), contents, value, ) } +/// Validate one config layer without requiring an MCP server fragment to +/// contain a transport. MCP server definitions are merged recursively across +/// config layers, so transport completeness is validated on the effective +/// config after composition. +pub fn validate_config_toml_layer( + mut value: TomlValue, + base_dir: &Path, +) -> Result<(), toml::de::Error> { + let _guard = AbsolutePathBufGuard::new(base_dir); + let mcp_servers = value + .as_table_mut() + .and_then(|table| table.remove("mcp_servers")); + + let _: ConfigToml = value.try_into()?; + if let Some(mcp_servers) = mcp_servers { + let _: ConfigTomlMcpServersLayer = mcp_servers_layer(mcp_servers).try_into()?; + } + Ok(()) +} + +/// Strictly validate one config layer while allowing MCP server fragments to +/// receive their transport from a lower-precedence layer. +fn config_error_from_config_toml_layer_for_source( + source: ConfigDiagnosticSource<'_>, + contents: &str, + mut value: TomlValue, +) -> Option { + let mcp_servers = value + .as_table_mut() + .and_then(|table| table.remove("mcp_servers")); + + if let Some(error) = config_error_from_ignored_toml_value_fields_for_source::( + source, contents, value, + ) { + return Some(error); + } + + let mcp_servers = mcp_servers?; + let mcp_servers_layer = mcp_servers_layer(mcp_servers); + config_error_from_ignored_toml_value_fields_for_source::( + source, + contents, + mcp_servers_layer, + ) +} + +fn mcp_servers_layer(mcp_servers: TomlValue) -> TomlValue { + TomlValue::Table(toml::map::Map::from_iter([( + "mcp_servers".to_string(), + mcp_servers, + )])) +} + fn config_error_from_ignored_toml_value_fields_for_source( source: ConfigDiagnosticSource<'_>, contents: &str, diff --git a/codex-rs/config/src/strict_config_tests.rs b/codex-rs/config/src/strict_config_tests.rs index 4d5a62df25..ed7084fe80 100644 --- a/codex-rs/config/src/strict_config_tests.rs +++ b/codex-rs/config/src/strict_config_tests.rs @@ -13,12 +13,8 @@ model = "gpt-5" unknown_key = true"#; let value = toml::from_str::(contents).expect("valid TOML"); - let error = config_error_from_ignored_toml_value_fields_for_source_name::( - source_name, - contents, - value, - ) - .expect("unknown field error"); + let error = config_error_from_config_toml_layer_for_source_name(source_name, contents, value) + .expect("unknown field error"); assert_eq!( error, @@ -125,3 +121,56 @@ collapsed = true"#; assert_eq!(error, None); } + +#[test] +fn strict_config_layer_accepts_partial_mcp_server() { + let path = Path::new("/tmp/config.toml"); + let contents = r#" +[mcp_servers.docs] +enabled = true + +[mcp_servers.docs.env] +DOCS_TOKEN = "local-token""#; + let value = toml::from_str::(contents).expect("valid TOML"); + + let error = config_error_from_config_toml_layer(path, contents, value); + + assert_eq!(error, None); +} + +#[test] +fn strict_config_layer_rejects_unknown_partial_mcp_server_field() { + let path = Path::new("/tmp/config.toml"); + let contents = r#" +[mcp_servers.docs] +enabled = true +unknown_key = true"#; + let value = toml::from_str::(contents).expect("valid TOML"); + + let error = config_error_from_config_toml_layer(path, contents, value) + .expect("unknown MCP field error"); + + assert_eq!( + error.message, + "unknown configuration field `mcp_servers.docs.unknown_key`" + ); +} + +#[test] +fn strict_config_layer_rejects_invalid_partial_mcp_server_field_type() { + let path = Path::new("/tmp/config.toml"); + let contents = r#" +[mcp_servers.docs] +enabled = "yes""#; + let value = toml::from_str::(contents).expect("valid TOML"); + + let error = + config_error_from_config_toml_layer(path, contents, value).expect("invalid MCP field type"); + + assert!( + error.message.contains("invalid type: string \"yes\"") + && error.message.contains("expected a boolean"), + "{}", + error.message + ); +} diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index 9857e08f16..2d3b1befae 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -381,6 +381,44 @@ unknown_key = true"#; assert_eq!(config_error, &expected_config_error); } +#[tokio::test] +async fn strict_config_accepts_partial_user_mcp_server_completed_by_cloud_config() { + let tmp = tempdir().expect("tempdir"); + std::fs::write( + tmp.path().join(CONFIG_TOML_FILE), + r#"[mcp_servers.docs] +enabled = true +"#, + ) + .expect("write partial user config"); + + let config = ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + .strict_config(/*strict_config*/ true) + .cloud_config_bundle(CloudConfigBundleFixture::loader_with_enterprise_config( + r#"[mcp_servers.docs] +command = "cloud-docs-server" +enabled = false +"#, + )) + .build() + .await + .expect("partial user MCP server should be completed by cloud config"); + + let server = config + .mcp_servers + .get() + .get("docs") + .expect("merged MCP server"); + assert!(server.enabled); + let codex_config::McpServerTransportConfig::Stdio { command, .. } = &server.transport else { + panic!("expected stdio MCP server"); + }; + assert_eq!(command, "cloud-docs-server"); +} + #[tokio::test] async fn strict_config_rejects_unknown_cli_override_key() { let tmp = tempdir().expect("tempdir");