From fea020b82c5ea94352bfaf8822c87fd4e5dc6151 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 15 Dec 2025 21:18:24 -0800 Subject: [PATCH] feat: change ConfigLayerName into a disjoint union rather than a simple enum --- .../app-server-protocol/src/protocol/v2.rs | 25 ++++-- .../app-server/tests/suite/v2/config_rpc.rs | 69 +++++++++++---- codex-rs/core/src/config/service.rs | 83 ++++++++++++++----- codex-rs/core/src/config_loader/README.md | 3 +- codex-rs/core/src/config_loader/mod.rs | 48 +++++++---- codex-rs/core/src/config_loader/state.rs | 12 +-- 6 files changed, 167 insertions(+), 73 deletions(-) diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 05362b204e..4f6cfaa8ed 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -209,13 +209,26 @@ v2_enum_from_core!( ); #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] -#[serde(rename_all = "camelCase")] +#[serde(tag = "type", rename_all = "camelCase")] +#[ts(tag = "type")] #[ts(export_to = "v2/")] pub enum ConfigLayerName { - Mdm, - System, - SessionFlags, - User, + /// Managed preferences layer delivered by MDM (macOS only). + #[serde(rename_all = "camelCase")] + #[ts(rename_all = "camelCase")] + Mdm { domain: String, key: String }, + /// Managed config layer from a file (usually `managed_config.toml`). + #[serde(rename_all = "camelCase")] + #[ts(rename_all = "camelCase")] + System { file: AbsolutePathBuf }, + /// Session-layer overrides supplied via `-c`/`--config`. + #[serde(rename_all = "camelCase")] + #[ts(rename_all = "camelCase")] + SessionFlags { override_keys: Vec }, + /// User config layer from a file (usually `config.toml`). + #[serde(rename_all = "camelCase")] + #[ts(rename_all = "camelCase")] + User { file: AbsolutePathBuf }, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, JsonSchema, TS)] @@ -289,7 +302,6 @@ pub struct Config { #[ts(export_to = "v2/")] pub struct ConfigLayerMetadata { pub name: ConfigLayerName, - pub source: String, pub version: String, } @@ -298,7 +310,6 @@ pub struct ConfigLayerMetadata { #[ts(export_to = "v2/")] pub struct ConfigLayer { pub name: ConfigLayerName, - pub source: String, pub version: String, pub config: JsonValue, } diff --git a/codex-rs/app-server/tests/suite/v2/config_rpc.rs b/codex-rs/app-server/tests/suite/v2/config_rpc.rs index 199d2f88e6..d0cd78a99b 100644 --- a/codex-rs/app-server/tests/suite/v2/config_rpc.rs +++ b/codex-rs/app-server/tests/suite/v2/config_rpc.rs @@ -18,6 +18,7 @@ use codex_app_server_protocol::RequestId; use codex_app_server_protocol::SandboxMode; use codex_app_server_protocol::ToolsV2; use codex_app_server_protocol::WriteStatus; +use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use serde_json::json; use tempfile::TempDir; @@ -42,6 +43,7 @@ model = "gpt-user" sandbox_mode = "workspace-write" "#, )?; + let user_file = AbsolutePathBuf::try_from(codex_home.path().join("config.toml"))?; let mut mcp = McpProcess::new(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; @@ -65,12 +67,19 @@ sandbox_mode = "workspace-write" assert_eq!(config.model.as_deref(), Some("gpt-user")); assert_eq!( origins.get("model").expect("origin").name, - ConfigLayerName::User + ConfigLayerName::User { + file: user_file.clone(), + } ); let layers = layers.expect("layers present"); assert_eq!(layers.len(), 2); - assert_eq!(layers[0].name, ConfigLayerName::SessionFlags); - assert_eq!(layers[1].name, ConfigLayerName::User); + assert_eq!( + layers[0].name, + ConfigLayerName::SessionFlags { + override_keys: Vec::new(), + } + ); + assert_eq!(layers[1].name, ConfigLayerName::User { file: user_file }); Ok(()) } @@ -88,6 +97,7 @@ web_search = true view_image = false "#, )?; + let user_file = AbsolutePathBuf::try_from(codex_home.path().join("config.toml"))?; let mut mcp = McpProcess::new(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; @@ -118,17 +128,26 @@ view_image = false ); assert_eq!( origins.get("tools.web_search").expect("origin").name, - ConfigLayerName::User + ConfigLayerName::User { + file: user_file.clone(), + } ); assert_eq!( origins.get("tools.view_image").expect("origin").name, - ConfigLayerName::User + ConfigLayerName::User { + file: user_file.clone(), + } ); let layers = layers.expect("layers present"); assert_eq!(layers.len(), 2); - assert_eq!(layers[0].name, ConfigLayerName::SessionFlags); - assert_eq!(layers[1].name, ConfigLayerName::User); + assert_eq!( + layers[0].name, + ConfigLayerName::SessionFlags { + override_keys: Vec::new(), + } + ); + assert_eq!(layers[1].name, ConfigLayerName::User { file: user_file }); Ok(()) } @@ -153,8 +172,10 @@ network_access = true serde_json::json!(user_dir) ), )?; + let user_file = AbsolutePathBuf::try_from(codex_home.path().join("config.toml"))?; let managed_path = codex_home.path().join("managed_config.toml"); + let managed_file = AbsolutePathBuf::try_from(managed_path.clone())?; std::fs::write( &managed_path, format!( @@ -197,19 +218,25 @@ writable_roots = [{}] assert_eq!(config.model.as_deref(), Some("gpt-system")); assert_eq!( origins.get("model").expect("origin").name, - ConfigLayerName::System + ConfigLayerName::System { + file: managed_file.clone(), + } ); assert_eq!(config.approval_policy, Some(AskForApproval::Never)); assert_eq!( origins.get("approval_policy").expect("origin").name, - ConfigLayerName::System + ConfigLayerName::System { + file: managed_file.clone(), + } ); assert_eq!(config.sandbox_mode, Some(SandboxMode::WorkspaceWrite)); assert_eq!( origins.get("sandbox_mode").expect("origin").name, - ConfigLayerName::User + ConfigLayerName::User { + file: user_file.clone(), + } ); let sandbox = config @@ -222,7 +249,9 @@ writable_roots = [{}] .get("sandbox_workspace_write.writable_roots.0") .expect("origin") .name, - ConfigLayerName::System + ConfigLayerName::System { + file: managed_file.clone(), + } ); assert!(sandbox.network_access); @@ -231,14 +260,24 @@ writable_roots = [{}] .get("sandbox_workspace_write.network_access") .expect("origin") .name, - ConfigLayerName::User + ConfigLayerName::User { + file: user_file.clone(), + } ); let layers = layers.expect("layers present"); assert_eq!(layers.len(), 3); - assert_eq!(layers[0].name, ConfigLayerName::System); - assert_eq!(layers[1].name, ConfigLayerName::SessionFlags); - assert_eq!(layers[2].name, ConfigLayerName::User); + assert_eq!( + layers[0].name, + ConfigLayerName::System { file: managed_file } + ); + assert_eq!( + layers[1].name, + ConfigLayerName::SessionFlags { + override_keys: Vec::new(), + } + ); + assert_eq!(layers[2].name, ConfigLayerName::User { file: user_file }); Ok(()) } diff --git a/codex-rs/core/src/config/service.rs b/codex-rs/core/src/config/service.rs index 393d637f31..ed561af61e 100644 --- a/codex-rs/core/src/config/service.rs +++ b/codex-rs/core/src/config/service.rs @@ -499,10 +499,10 @@ fn value_at_path<'a>(root: &'a TomlValue, segments: &[String]) -> Option<&'a Tom fn override_message(layer: &ConfigLayerName) -> String { match layer { - ConfigLayerName::Mdm => "Overridden by managed policy (mdm)".to_string(), - ConfigLayerName::System => "Overridden by managed config (system)".to_string(), - ConfigLayerName::SessionFlags => "Overridden by session flags".to_string(), - ConfigLayerName::User => "Overridden by user config".to_string(), + ConfigLayerName::Mdm { .. } => "Overridden by managed policy (mdm)".to_string(), + ConfigLayerName::System { .. } => "Overridden by managed config (system)".to_string(), + ConfigLayerName::SessionFlags { .. } => "Overridden by session flags".to_string(), + ConfigLayerName::User { .. } => "Overridden by user config".to_string(), } } @@ -576,6 +576,7 @@ mod tests { use super::*; use anyhow::Result; use codex_app_server_protocol::AskForApproval; + use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use tempfile::tempdir; @@ -677,16 +678,19 @@ remote_compaction = true #[tokio::test] async fn read_includes_origins_and_layers() { let tmp = tempdir().expect("tempdir"); - std::fs::write(tmp.path().join(CONFIG_TOML_FILE), "model = \"user\"").unwrap(); + let user_path = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write(&user_path, "model = \"user\"").unwrap(); + let user_file = AbsolutePathBuf::try_from(user_path.clone()).expect("user file"); let managed_path = tmp.path().join("managed_config.toml"); std::fs::write(&managed_path, "approval_policy = \"never\"").unwrap(); + let managed_file = AbsolutePathBuf::try_from(managed_path.clone()).expect("managed file"); let service = ConfigService::with_overrides( tmp.path().to_path_buf(), vec![], LoaderOverrides { - managed_config_path: Some(managed_path), + managed_config_path: Some(managed_path.clone()), #[cfg(target_os = "macos")] managed_preferences_base64: None, }, @@ -707,12 +711,25 @@ remote_compaction = true .get("approval_policy") .expect("origin") .name, - ConfigLayerName::System + ConfigLayerName::System { + file: managed_file.clone(), + } ); let layers = response.layers.expect("layers present"); - assert_eq!(layers.first().unwrap().name, ConfigLayerName::System); - assert_eq!(layers.get(1).unwrap().name, ConfigLayerName::SessionFlags); - assert_eq!(layers.last().unwrap().name, ConfigLayerName::User); + assert_eq!( + layers.first().unwrap().name, + ConfigLayerName::System { file: managed_file } + ); + assert_eq!( + layers.get(1).unwrap().name, + ConfigLayerName::SessionFlags { + override_keys: Vec::new(), + } + ); + assert_eq!( + layers.last().unwrap().name, + ConfigLayerName::User { file: user_file } + ); } #[tokio::test] @@ -726,12 +743,13 @@ remote_compaction = true let managed_path = tmp.path().join("managed_config.toml"); std::fs::write(&managed_path, "approval_policy = \"never\"").unwrap(); + let managed_file = AbsolutePathBuf::try_from(managed_path.clone()).expect("managed file"); let service = ConfigService::with_overrides( tmp.path().to_path_buf(), vec![], LoaderOverrides { - managed_config_path: Some(managed_path), + managed_config_path: Some(managed_path.clone()), #[cfg(target_os = "macos")] managed_preferences_base64: None, }, @@ -764,7 +782,7 @@ remote_compaction = true .get("approval_policy") .expect("origin") .name, - ConfigLayerName::System + ConfigLayerName::System { file: managed_file } ); assert_eq!(result.status, WriteStatus::Ok); assert!(result.overridden_metadata.is_none()); @@ -773,7 +791,8 @@ remote_compaction = true #[tokio::test] async fn version_conflict_rejected() { let tmp = tempdir().expect("tempdir"); - std::fs::write(tmp.path().join(CONFIG_TOML_FILE), "model = \"user\"").unwrap(); + let user_path = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write(&user_path, "model = \"user\"").unwrap(); let service = ConfigService::new(tmp.path().to_path_buf(), vec![]); let error = service @@ -830,7 +849,7 @@ remote_compaction = true tmp.path().to_path_buf(), vec![], LoaderOverrides { - managed_config_path: Some(managed_path), + managed_config_path: Some(managed_path.clone()), #[cfg(target_os = "macos")] managed_preferences_base64: None, }, @@ -860,10 +879,13 @@ remote_compaction = true #[tokio::test] async fn read_reports_managed_overrides_user_and_session_flags() { let tmp = tempdir().expect("tempdir"); - std::fs::write(tmp.path().join(CONFIG_TOML_FILE), "model = \"user\"").unwrap(); + let user_path = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write(&user_path, "model = \"user\"").unwrap(); + let user_file = AbsolutePathBuf::try_from(user_path.clone()).expect("user file"); let managed_path = tmp.path().join("managed_config.toml"); std::fs::write(&managed_path, "model = \"system\"").unwrap(); + let managed_file = AbsolutePathBuf::try_from(managed_path.clone()).expect("managed file"); let cli_overrides = vec![( "model".to_string(), @@ -874,7 +896,7 @@ remote_compaction = true tmp.path().to_path_buf(), cli_overrides, LoaderOverrides { - managed_config_path: Some(managed_path), + managed_config_path: Some(managed_path.clone()), #[cfg(target_os = "macos")] managed_preferences_base64: None, }, @@ -890,12 +912,25 @@ remote_compaction = true assert_eq!(response.config.model.as_deref(), Some("system")); assert_eq!( response.origins.get("model").expect("origin").name, - ConfigLayerName::System + ConfigLayerName::System { + file: managed_file.clone(), + } ); let layers = response.layers.expect("layers"); - assert_eq!(layers.first().unwrap().name, ConfigLayerName::System); - assert_eq!(layers.get(1).unwrap().name, ConfigLayerName::SessionFlags); - assert_eq!(layers.get(2).unwrap().name, ConfigLayerName::User); + assert_eq!( + layers.first().unwrap().name, + ConfigLayerName::System { file: managed_file } + ); + assert_eq!( + layers.get(1).unwrap().name, + ConfigLayerName::SessionFlags { + override_keys: vec!["model".to_string()], + } + ); + assert_eq!( + layers.get(2).unwrap().name, + ConfigLayerName::User { file: user_file } + ); } #[tokio::test] @@ -905,12 +940,13 @@ remote_compaction = true let managed_path = tmp.path().join("managed_config.toml"); std::fs::write(&managed_path, "approval_policy = \"never\"").unwrap(); + let managed_file = AbsolutePathBuf::try_from(managed_path.clone()).expect("managed file"); let service = ConfigService::with_overrides( tmp.path().to_path_buf(), vec![], LoaderOverrides { - managed_config_path: Some(managed_path), + managed_config_path: Some(managed_path.clone()), #[cfg(target_os = "macos")] managed_preferences_base64: None, }, @@ -929,7 +965,10 @@ remote_compaction = true assert_eq!(result.status, WriteStatus::OkOverridden); let overridden = result.overridden_metadata.expect("overridden metadata"); - assert_eq!(overridden.overriding_layer.name, ConfigLayerName::System); + assert_eq!( + overridden.overriding_layer.name, + ConfigLayerName::System { file: managed_file } + ); assert_eq!(overridden.effective_value, serde_json::json!("never")); } diff --git a/codex-rs/core/src/config_loader/README.md b/codex-rs/core/src/config_loader/README.md index da427c4544..9df656951c 100644 --- a/codex-rs/core/src/config_loader/README.md +++ b/codex-rs/core/src/config_loader/README.md @@ -16,7 +16,7 @@ Exported from `codex_core::config_loader`: - `origins() -> HashMap` - `layers_high_to_low() -> Vec` - `with_user_config(user_config) -> ConfigLayerStack` -- `ConfigLayerEntry` (one layer’s `{name, source, config, version}`) +- `ConfigLayerEntry` (one layer’s `{name, config, version}`; `name` carries source metadata) - `LoaderOverrides` (test/override hooks for managed config sources) - `merge_toml_values(base, overlay)` (public helper used elsewhere) @@ -61,4 +61,3 @@ Implementation is split by concern: - `merge.rs`: recursive TOML merge. - `fingerprint.rs`: stable per-layer hashing and per-key origins traversal. - `macos.rs`: managed preferences integration (macOS only). - diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index c7c522008d..89b225d6e6 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -10,9 +10,9 @@ mod tests; use crate::config::CONFIG_TOML_FILE; use codex_app_server_protocol::ConfigLayerName; +use codex_utils_absolute_path::AbsolutePathBuf; use std::io; use std::path::Path; -use std::path::PathBuf; use toml::Value as TomlValue; pub use merge::merge_toml_values; @@ -20,8 +20,8 @@ pub use state::ConfigLayerEntry; pub use state::ConfigLayerStack; pub use state::LoaderOverrides; -const SESSION_FLAGS_SOURCE: &str = "--config"; -const MDM_SOURCE: &str = "com.openai.codex/config_toml_base64"; +const MDM_PREFERENCES_DOMAIN: &str = "com.openai.codex"; +const MDM_PREFERENCES_KEY: &str = "config_toml_base64"; /// Configuration layering pipeline (top overrides bottom): /// @@ -51,24 +51,38 @@ pub async fn load_config_layers_state( .unwrap_or_else(|| layer_io::managed_config_default_path(codex_home)); let layers = layer_io::load_config_layers_internal(codex_home, overrides).await?; - let cli_overrides = overrides::build_cli_overrides_layer(cli_overrides); + let session_override_keys = cli_overrides.iter().map(|(key, _)| key.clone()).collect(); + let cli_overrides_layer = overrides::build_cli_overrides_layer(cli_overrides); + let user_file = AbsolutePathBuf::from_absolute_path(codex_home.join(CONFIG_TOML_FILE))?; + + let system = match layers.managed_config { + Some(cfg) => { + let system_file = AbsolutePathBuf::from_absolute_path(managed_config_path.clone())?; + Some(ConfigLayerEntry::new( + ConfigLayerName::System { file: system_file }, + cfg, + )) + } + None => None, + }; Ok(ConfigLayerStack { - user: ConfigLayerEntry::new( - ConfigLayerName::User, - codex_home.join(CONFIG_TOML_FILE), - layers.base, - ), + user: ConfigLayerEntry::new(ConfigLayerName::User { file: user_file }, layers.base), session_flags: ConfigLayerEntry::new( - ConfigLayerName::SessionFlags, - PathBuf::from(SESSION_FLAGS_SOURCE), - cli_overrides, + ConfigLayerName::SessionFlags { + override_keys: session_override_keys, + }, + cli_overrides_layer, ), - system: layers.managed_config.map(|cfg| { - ConfigLayerEntry::new(ConfigLayerName::System, managed_config_path.clone(), cfg) + system, + mdm: layers.managed_preferences.map(|cfg| { + ConfigLayerEntry::new( + ConfigLayerName::Mdm { + domain: MDM_PREFERENCES_DOMAIN.to_string(), + key: MDM_PREFERENCES_KEY.to_string(), + }, + cfg, + ) }), - mdm: layers - .managed_preferences - .map(|cfg| ConfigLayerEntry::new(ConfigLayerName::Mdm, PathBuf::from(MDM_SOURCE), cfg)), }) } diff --git a/codex-rs/core/src/config_loader/state.rs b/codex-rs/core/src/config_loader/state.rs index e23cc243ce..b0c56af2e5 100644 --- a/codex-rs/core/src/config_loader/state.rs +++ b/codex-rs/core/src/config_loader/state.rs @@ -19,17 +19,15 @@ pub struct LoaderOverrides { #[derive(Debug, Clone)] pub struct ConfigLayerEntry { pub name: ConfigLayerName, - pub source: PathBuf, pub config: TomlValue, pub version: String, } impl ConfigLayerEntry { - pub fn new(name: ConfigLayerName, source: PathBuf, config: TomlValue) -> Self { + pub fn new(name: ConfigLayerName, config: TomlValue) -> Self { let version = version_for_toml(&config); Self { name, - source, config, version, } @@ -38,7 +36,6 @@ impl ConfigLayerEntry { pub fn metadata(&self) -> ConfigLayerMetadata { ConfigLayerMetadata { name: self.name.clone(), - source: self.source.display().to_string(), version: self.version.clone(), } } @@ -46,7 +43,6 @@ impl ConfigLayerEntry { pub fn as_layer(&self) -> ConfigLayer { ConfigLayer { name: self.name.clone(), - source: self.source.display().to_string(), version: self.version.clone(), config: serde_json::to_value(&self.config).unwrap_or(JsonValue::Null), } @@ -64,11 +60,7 @@ pub struct ConfigLayerStack { impl ConfigLayerStack { pub fn with_user_config(&self, user_config: TomlValue) -> Self { Self { - user: ConfigLayerEntry::new( - self.user.name.clone(), - self.user.source.clone(), - user_config, - ), + user: ConfigLayerEntry::new(self.user.name.clone(), user_config), session_flags: self.session_flags.clone(), system: self.system.clone(), mdm: self.mdm.clone(),