WIP: allow partial MCP config layers

Snapshot shared in-progress state before reconciliation.
This commit is contained in:
Felix Xia
2026-06-24 20:25:02 +01:00
parent 5013d10824
commit 70135cc051
11 changed files with 343 additions and 43 deletions
@@ -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(),
@@ -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");
+6 -6
View File
@@ -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,
@@ -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();
+1
View File
@@ -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;
+4 -7
View File
@@ -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::<ConfigToml>(
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,
+3 -5
View File
@@ -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,
+4 -6
View File
@@ -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::<ConfigToml>(
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,
+68 -4
View File
@@ -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<String, RawMcpServerConfig>,
}
pub fn config_error_from_ignored_toml_fields<T: DeserializeOwned>(
path: impl AsRef<Path>,
contents: &str,
@@ -25,30 +36,83 @@ pub fn config_error_from_ignored_toml_fields<T: DeserializeOwned>(
}
}
pub(crate) fn config_error_from_ignored_toml_value_fields<T: DeserializeOwned>(
pub(crate) fn config_error_from_config_toml_layer(
path: impl AsRef<Path>,
contents: &str,
value: TomlValue,
) -> Option<ConfigError> {
config_error_from_ignored_toml_value_fields_for_source::<T>(
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<T: DeserializeOwned>(
pub(crate) fn config_error_from_config_toml_layer_for_source_name(
source_name: &str,
contents: &str,
value: TomlValue,
) -> Option<ConfigError> {
config_error_from_ignored_toml_value_fields_for_source::<T>(
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<ConfigError> {
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::<ConfigToml>(
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::<ConfigTomlMcpServersLayer>(
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<T: DeserializeOwned>(
source: ConfigDiagnosticSource<'_>,
contents: &str,
+55 -6
View File
@@ -13,12 +13,8 @@ model = "gpt-5"
unknown_key = true"#;
let value = toml::from_str::<TomlValue>(contents).expect("valid TOML");
let error = config_error_from_ignored_toml_value_fields_for_source_name::<ConfigToml>(
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::<TomlValue>(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::<TomlValue>(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::<TomlValue>(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
);
}
@@ -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");