diff --git a/codex-rs/config/src/requirements_layers/layer.rs b/codex-rs/config/src/requirements_layers/layer.rs index d92024e117..6f16cf1c13 100644 --- a/codex-rs/config/src/requirements_layers/layer.rs +++ b/codex-rs/config/src/requirements_layers/layer.rs @@ -2,6 +2,7 @@ use crate::ConfigRequirementsToml; use crate::ManagedHooksRequirementsToml; use crate::RequirementSource; use crate::RequirementsExecPolicyToml; +use crate::types::OtelConfigToml; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::AbsolutePathBufGuard; use toml::Value as TomlValue; @@ -80,6 +81,7 @@ impl ComposableRequirementsLayer { domain_fields: DomainMergedRequirementsFields { rules: requirements.rules, hooks: requirements.hooks, + otel: requirements.otel, permissions: requirements.permissions, }, }) @@ -90,6 +92,7 @@ impl ComposableRequirementsLayer { pub(super) struct DomainMergedRequirementsFields { pub(super) rules: Option, pub(super) hooks: Option, + pub(super) otel: Option, pub(super) permissions: Option, } @@ -163,6 +166,7 @@ fn toml_value_from_serializable( fn strip_special_fields(layer_toml: &mut TomlValue) { remove_top_level_field(layer_toml, "rules"); remove_top_level_field(layer_toml, "hooks"); + remove_top_level_field(layer_toml, "otel"); remove_nested_field_and_prune_empty(layer_toml, &["permissions", "filesystem", "deny_read"]); } diff --git a/codex-rs/config/src/requirements_layers/mod.rs b/codex-rs/config/src/requirements_layers/mod.rs index 52a23a29db..5452cf3120 100644 --- a/codex-rs/config/src/requirements_layers/mod.rs +++ b/codex-rs/config/src/requirements_layers/mod.rs @@ -1,5 +1,6 @@ mod hooks; mod layer; +mod otel; mod permissions; mod rules; mod stack; diff --git a/codex-rs/config/src/requirements_layers/otel.rs b/codex-rs/config/src/requirements_layers/otel.rs new file mode 100644 index 0000000000..3c37248e6d --- /dev/null +++ b/codex-rs/config/src/requirements_layers/otel.rs @@ -0,0 +1,89 @@ +use crate::RequirementSource; +use crate::Sourced; +use crate::types::OtelConfigToml; +use std::collections::BTreeMap; + +/// Merges one OTEL requirements layer into an output assembled high-to-low. +/// +/// Exporters and scalar settings are exact leaves, while trace metadata is +/// recursively merged by key. Keeping this policy typed prevents exporter +/// variants, credentials, and TLS settings from being combined field by field. +pub(super) fn merge( + output: &mut Option>, + incoming: Option, + source: &RequirementSource, +) { + let Some(incoming) = incoming.filter(|value| !is_empty(value)) else { + return; + }; + let Some(output) = output.as_mut() else { + *output = Some(Sourced::new(incoming, source.clone())); + return; + }; + + let OtelConfigToml { + log_user_prompt, + environment, + exporter, + trace_exporter, + metrics_exporter, + span_attributes, + tracestate, + } = incoming; + + macro_rules! fill_exact_leaf { + ($field:ident) => { + if output.value.$field.is_none() { + output.value.$field = $field; + } + }; + } + + fill_exact_leaf!(log_user_prompt); + fill_exact_leaf!(environment); + fill_exact_leaf!(exporter); + fill_exact_leaf!(trace_exporter); + fill_exact_leaf!(metrics_exporter); + merge_map(&mut output.value.span_attributes, span_attributes); + merge_nested_map(&mut output.value.tracestate, tracestate); + super::stack::merge_output_source(&mut output.source, source); +} + +fn merge_map( + output: &mut Option>, + incoming: Option>, +) { + let Some(incoming) = incoming else { + return; + }; + let output = output.get_or_insert_default(); + for (key, value) in incoming { + output.entry(key).or_insert(value); + } +} + +fn merge_nested_map( + output: &mut Option>>, + incoming: Option>>, +) { + let Some(incoming) = incoming else { + return; + }; + let output = output.get_or_insert_default(); + for (member, fields) in incoming { + let output_fields = output.entry(member).or_default(); + for (key, value) in fields { + output_fields.entry(key).or_insert(value); + } + } +} + +fn is_empty(value: &OtelConfigToml) -> bool { + value.log_user_prompt.is_none() + && value.environment.is_none() + && value.exporter.is_none() + && value.trace_exporter.is_none() + && value.metrics_exporter.is_none() + && value.span_attributes.is_none() + && value.tracestate.is_none() +} diff --git a/codex-rs/config/src/requirements_layers/stack.rs b/codex-rs/config/src/requirements_layers/stack.rs index b730028363..1dfe5f18b6 100644 --- a/codex-rs/config/src/requirements_layers/stack.rs +++ b/codex-rs/config/src/requirements_layers/stack.rs @@ -6,6 +6,7 @@ //! //! A few fields carry domain-specific meaning that raw TOML replacement would //! break: +//! - `otel` combines recursively merged metadata with atomic exporter leaves. //! - `remote_sandbox_config` is evaluated within each layer before merging. //! - `rules.prefix_rules` append high-priority rules first. //! - `hooks` append high-priority event groups first while failing closed on @@ -132,11 +133,11 @@ impl RequirementsLayerStack { let mut hooks = HookMergeState::new(hook_directory_field); let mut hooks_output = None; let mut deny_read = DenyReadMergeState::default(); - // Regular TOML fields are folded low-to-high like config. These custom - // fields append or union values, so process them high-to-low to keep - // priority order visible in the output. + // Domain-specific fields are processed high-to-low so exact leaves and + // keyed values naturally retain the highest-priority contribution. for layer in layers.iter().rev() { let domain_fields = &layer.domain_fields; + super::otel::merge(&mut output.otel, domain_fields.otel.clone(), &layer.source); super::rules::merge(&mut rules, domain_fields.rules.clone(), &layer.source); hooks.merge( &mut hooks_output, @@ -181,7 +182,7 @@ fn populate_merged_regular_fields_with_sources( log_dir, model_catalog_json, check_for_update_on_startup, - otel, + otel: _, allow_login_shell, shell_environment_policy, feedback, @@ -219,7 +220,6 @@ fn populate_merged_regular_fields_with_sources( check_for_update_on_startup, &["check_for_update_on_startup"] ); - set_sourced!(otel, &["otel"]); set_sourced!(allow_login_shell, &["allow_login_shell"]); set_sourced!(shell_environment_policy, &["shell_environment_policy"]); set_sourced!(feedback, &["feedback"]); diff --git a/codex-rs/config/src/requirements_layers/stack_tests.rs b/codex-rs/config/src/requirements_layers/stack_tests.rs index bc23ed26b7..75a3d15e86 100644 --- a/codex-rs/config/src/requirements_layers/stack_tests.rs +++ b/codex-rs/config/src/requirements_layers/stack_tests.rs @@ -392,6 +392,89 @@ sandbox_private_desktop = true ); } +#[test] +fn otel_exporters_replace_atomically_across_layers() { + let high_source = RequirementSource::EnterpriseManaged { + id: "req_high".to_string(), + name: "High".to_string(), + }; + let low_source = RequirementSource::EnterpriseManaged { + id: "req_low".to_string(), + name: "Low".to_string(), + }; + let composed = compose_requirements_for_hostname( + vec![ + RequirementsLayerEntry::from_toml( + low_source.clone(), + r#" +[otel] +log_user_prompt = true +environment = "low" +exporter = { otlp-http = { endpoint = "https://logs.low.example", protocol = "binary", headers = { Authorization = "old-secret" } } } +trace_exporter = { otlp-http = { endpoint = "https://traces.low.example", protocol = "json", headers = { Trace = "old-trace" } } } +metrics_exporter = { otlp-grpc = { endpoint = "https://metrics.low.example", headers = { Metrics = "old-metrics" } } } + +[otel.span_attributes] +region = "us" +team = "low" + +[otel.tracestate.vendor] +low = "one" +shared = "low" +"#, + ), + RequirementsLayerEntry::from_toml( + high_source.clone(), + r#" +[otel] +environment = "high" +exporter = { otlp-grpc = { endpoint = "https://logs.high.example" } } +trace_exporter = { otlp-http = { endpoint = "https://traces.high.example", protocol = "binary" } } +metrics_exporter = "none" + +[otel.span_attributes] +team = "high" + +[otel.tracestate.vendor] +high = "two" +shared = "high" +"#, + ), + ], + /*hostname*/ None, + ) + .expect("compose requirements") + .expect("requirements present"); + + assert_eq!( + composed.otel, + Some(Sourced::new( + expected_requirements( + r#" +[otel] +log_user_prompt = true +environment = "high" +exporter = { otlp-grpc = { endpoint = "https://logs.high.example" } } +trace_exporter = { otlp-http = { endpoint = "https://traces.high.example", protocol = "binary" } } +metrics_exporter = "none" + +[otel.span_attributes] +region = "us" +team = "high" + +[otel.tracestate.vendor] +high = "two" +low = "one" +shared = "high" +"#, + ) + .otel + .expect("expected OTEL requirements"), + RequirementSource::composite([high_source, low_source]), + )) + ); +} + #[test] fn merged_table_source_is_composite_in_priority_order() { let high_source = RequirementSource::EnterpriseManaged { diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 748bb0e77e..77d980bfcd 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -9456,6 +9456,50 @@ async fn invalid_required_otel_trace_metadata_fails_closed() -> std::io::Result< Ok(()) } +#[tokio::test] +async fn required_otel_tracestate_survives_invalid_user_merge() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let configured_overflow = "u".repeat(/*n*/ 120); + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + format!( + r#" +[otel.tracestate.vendor] +required = "configured" +keep = "kept" +zz_overflow = {configured_overflow:?} +"# + ), + )?; + let required_value = "r".repeat(/*n*/ 120); + let requirements = format!("[otel.tracestate.vendor]\nrequired = {required_value:?}\n"); + + let config = ConfigBuilder::without_managed_config_for_tests() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .cloud_config_bundle( + CloudConfigBundleFixture::loader_with_enterprise_requirement(requirements), + ) + .build() + .await?; + + assert_eq!( + config.otel.tracestate, + BTreeMap::from([( + "vendor".to_string(), + BTreeMap::from([ + ("keep".to_string(), "kept".to_string()), + ("required".to_string(), required_value), + ]), + )]) + ); + assert!(config.startup_warnings.iter().any(|warning| { + warning.contains("otel.tracestate.vendor.zz_overflow") + && warning.contains("would invalidate tracestate required by") + })); + Ok(()) +} + #[tokio::test] async fn conflicting_login_method_requirements_fail_closed() -> std::io::Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 8b158c33a1..8041478521 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1999,16 +1999,24 @@ fn apply_otel_requirement( configured.extend(required); } if let Some(required) = required.tracestate { - let configured = configured.tracestate.get_or_insert_default(); - for (member, required_fields) in required { - let configured_fields = configured.entry(member).or_default(); - conflict |= required_fields.iter().any(|(key, value)| { - configured_fields - .get(key) - .is_some_and(|current| current != value) - }); - configured_fields.extend(required_fields); - } + let configured_tracestate = configured.tracestate.take().unwrap_or_default(); + conflict |= required.iter().any(|(member, required_fields)| { + configured_tracestate + .get(member) + .is_some_and(|configured_fields| { + required_fields.iter().any(|(key, value)| { + configured_fields + .get(key) + .is_some_and(|current| current != value) + }) + }) + }); + configured.tracestate = Some(otel::merge_required_tracestate( + configured_tracestate, + required, + &source, + startup_warnings, + )); } if conflict { diff --git a/codex-rs/core/src/config/otel.rs b/codex-rs/core/src/config/otel.rs index cb65d304d1..7d96005648 100644 --- a/codex-rs/core/src/config/otel.rs +++ b/codex-rs/core/src/config/otel.rs @@ -36,6 +36,65 @@ pub(crate) fn resolve_config( } } +/// Merges user-configured tracestate into a validated required base. +/// +/// Required member fields always win. Other configured fields are admitted one +/// at a time only when the complete candidate remains W3C-valid; rejected +/// fields produce startup warnings rather than erasing required metadata. +pub(crate) fn merge_required_tracestate( + configured: BTreeMap>, + required: BTreeMap>, + source: &impl Display, + startup_warnings: &mut Vec, +) -> BTreeMap> { + let mut effective = required.clone(); + for (member_key, configured_fields) in configured { + for (field_key, value) in configured_fields { + if required + .get(&member_key) + .is_some_and(|required_fields| required_fields.contains_key(&field_key)) + { + continue; + } + + let mut candidate = effective.clone(); + candidate + .entry(member_key.clone()) + .or_default() + .insert(field_key.clone(), value); + retain_valid_configured_tracestate( + candidate, + &format!("otel.tracestate.{member_key}.{field_key}"), + source, + startup_warnings, + &mut effective, + ); + } + } + effective +} + +fn retain_valid_configured_tracestate( + candidate: BTreeMap>, + config_key: &str, + source: &impl Display, + startup_warnings: &mut Vec, + effective: &mut BTreeMap>, +) { + match codex_otel::validate_tracestate_entries(&candidate) { + Ok(()) => { + *effective = candidate; + } + Err(err) => { + let message = format!( + "Ignoring configured `{config_key}` because it would invalidate tracestate required by {source}: {err}" + ); + tracing::warn!("{message}"); + startup_warnings.push(message); + } + } +} + fn resolve_span_attributes( span_attributes: Option>, startup_warnings: &mut Vec,