From 39937b074fbc8eac04f3468657417419d2c1663f Mon Sep 17 00:00:00 2001 From: jif-oai Date: Wed, 6 May 2026 12:33:37 +0200 Subject: [PATCH] Revert "feat: support template interpolation in multi-agent usage hints" (#21337) Reverts openai/codex#20973 --- codex-rs/core/src/config/config_tests.rs | 62 ------ codex-rs/core/src/config/mod.rs | 58 ------ .../core/src/config/template_interpolation.rs | 189 ------------------ codex-rs/core/src/session/config_lock.rs | 89 ++++++++- 4 files changed, 85 insertions(+), 313 deletions(-) delete mode 100644 codex-rs/core/src/config/template_interpolation.rs diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index aed305552d..7001f9015c 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -8927,68 +8927,6 @@ hide_spawn_agent_metadata = true Ok(()) } -#[tokio::test] -async fn multi_agent_v2_usage_hint_templates_use_materialized_config_values() -> std::io::Result<()> -{ - let codex_home = TempDir::new()?; - std::fs::write( - codex_home.path().join(CONFIG_TOML_FILE), - r#"[features.multi_agent_v2] -enabled = true -usage_hint_text = "Limit {{ features.multi_agent_v2.max_concurrent_threads_per_session }}" -root_agent_usage_hint_text = "Root {{ features.multi_agent_v2.max_concurrent_threads_per_session }}" -subagent_usage_hint_text = "Subagent {{ features.multi_agent_v2.max_concurrent_threads_per_session }}" -"#, - )?; - - 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())) - .build() - .await?; - - assert_eq!( - config.multi_agent_v2.usage_hint_text.as_deref(), - Some("Limit 4") - ); - assert_eq!( - config.multi_agent_v2.root_agent_usage_hint_text.as_deref(), - Some("Root 4") - ); - assert_eq!( - config.multi_agent_v2.subagent_usage_hint_text.as_deref(), - Some("Subagent 4") - ); - - Ok(()) -} - -#[tokio::test] -async fn multi_agent_v2_usage_hint_templates_fail_when_placeholder_is_missing() { - let codex_home = TempDir::new().expect("create codex home"); - std::fs::write( - codex_home.path().join(CONFIG_TOML_FILE), - r#"[features.multi_agent_v2] -enabled = true -usage_hint_text = "{{ features.multi_agent_v2.does_not_exist }}" -"#, - ) - .expect("write config"); - - let err = ConfigBuilder::without_managed_config_for_tests() - .codex_home(codex_home.path().to_path_buf()) - .fallback_cwd(Some(codex_home.path().to_path_buf())) - .build() - .await - .expect_err("config load should fail"); - - assert!( - err.to_string() - .contains("features.multi_agent_v2.does_not_exist"), - "unexpected error: {err}", - ); -} - #[tokio::test] async fn profile_multi_agent_v2_config_overrides_base() -> 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 657a0413c9..43d88bcdd2 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -133,7 +133,6 @@ mod network_proxy_spec; mod permissions; #[cfg(test)] mod schema; -pub(crate) mod template_interpolation; pub use codex_config::Constrained; pub use codex_config::ConstraintError; pub use codex_config::ConstraintResult; @@ -2083,63 +2082,6 @@ impl Config { overrides: ConfigOverrides, codex_home: AbsolutePathBuf, config_layer_stack: ConfigLayerStack, - ) -> std::io::Result { - let config = Self::build_config_with_layer_stack( - fs, - cfg.clone(), - overrides.clone(), - codex_home.clone(), - config_layer_stack.clone(), - ) - .await?; - let mut interpolation_source_cfg = cfg.clone(); - template_interpolation::apply_resolved_config_fields( - &config, - &mut interpolation_source_cfg, - ) - .map_err(|err| { - std::io::Error::new( - std::io::ErrorKind::InvalidData, - format!("failed to materialize config for interpolation: {err}"), - ) - })?; - let interpolation_source = - toml::Value::try_from(interpolation_source_cfg).map_err(|err| { - std::io::Error::new( - std::io::ErrorKind::InvalidData, - format!("failed to serialize config for interpolation: {err}"), - ) - })?; - let mut interpolated_cfg = cfg; - let interpolated = template_interpolation::interpolate_config_string_fields( - &mut interpolated_cfg, - &interpolation_source, - ) - .map_err(|err| { - std::io::Error::new( - std::io::ErrorKind::InvalidData, - format!("failed to interpolate config template fields: {err}"), - ) - })?; - if interpolated { - return Self::build_config_with_layer_stack( - fs, - interpolated_cfg, - overrides, - codex_home, - config_layer_stack, - ) - .await; - } - Ok(config) - } - - async fn build_config_with_layer_stack( - fs: &dyn ExecutorFileSystem, - cfg: ConfigToml, - overrides: ConfigOverrides, - codex_home: AbsolutePathBuf, - config_layer_stack: ConfigLayerStack, ) -> std::io::Result { // Keep the large config-construction future off small test thread stacks. Box::pin(async move { diff --git a/codex-rs/core/src/config/template_interpolation.rs b/codex-rs/core/src/config/template_interpolation.rs deleted file mode 100644 index 7e38fed86f..0000000000 --- a/codex-rs/core/src/config/template_interpolation.rs +++ /dev/null @@ -1,189 +0,0 @@ -use anyhow::Context; -use anyhow::bail; -use codex_config::config_toml::ConfigToml; -use codex_config::types::MemoriesToml; -use codex_features::AppsMcpPathOverrideConfigToml; -use codex_features::Feature; -use codex_features::FeatureToml; -use codex_features::FeaturesToml; -use codex_features::MultiAgentV2ConfigToml; -use codex_utils_template::Template; -use toml::Value as TomlValue; - -use super::Config; - -const INTERPOLATED_CONFIG_STRING_FIELDS: &[&str] = &[ - "features.multi_agent_v2.usage_hint_text", - "features.multi_agent_v2.root_agent_usage_hint_text", - "features.multi_agent_v2.subagent_usage_hint_text", -]; - -pub(crate) fn materialized_config_toml(config: &Config) -> anyhow::Result { - let mut materialized: ConfigToml = config - .config_layer_stack - .effective_config() - .try_into() - .context("failed to deserialize effective config for config interpolation")?; - apply_resolved_config_fields(config, &mut materialized)?; - Ok(materialized) -} - -pub(crate) fn interpolate_config_string_fields( - config_toml: &mut ConfigToml, - interpolation_source: &TomlValue, -) -> anyhow::Result { - let mut target_value = TomlValue::try_from(config_toml.clone()) - .context("failed to serialize config for interpolation")?; - let mut changed = false; - - for field_path in INTERPOLATED_CONFIG_STRING_FIELDS { - let Some(value) = value_mut_at_path(&mut target_value, field_path) else { - continue; - }; - let Some(template_source) = value.as_str() else { - bail!("interpolated config field `{field_path}` must be a string"); - }; - let template = Template::parse(template_source) - .with_context(|| format!("failed to parse template in config field `{field_path}`"))?; - let rendered = render_template(&template, interpolation_source, field_path)?; - if rendered != template_source { - *value = TomlValue::String(rendered); - changed = true; - } - } - - if changed { - *config_toml = target_value - .try_into() - .context("failed to deserialize interpolated config")?; - } - - Ok(changed) -} - -pub(crate) fn apply_resolved_config_fields( - config: &Config, - config_toml: &mut ConfigToml, -) -> anyhow::Result<()> { - config_toml.web_search = Some(config.web_search_mode.value()); - config_toml.model_provider = Some(config.model_provider_id.clone()); - config_toml.plan_mode_reasoning_effort = config.plan_mode_reasoning_effort; - config_toml.model_verbosity = config.model_verbosity; - config_toml.include_permissions_instructions = Some(config.include_permissions_instructions); - config_toml.include_apps_instructions = Some(config.include_apps_instructions); - config_toml.include_environment_context = Some(config.include_environment_context); - config_toml.background_terminal_max_timeout = Some(config.background_terminal_max_timeout); - - // Feature aliases and feature configs need to be written in their resolved - // form; otherwise replay can drift when a legacy key maps to the same - // runtime feature. - let features = config_toml - .features - .get_or_insert_with(FeaturesToml::default); - features.materialize_resolved_enabled(config.features.get()); - let mut multi_agent_v2: MultiAgentV2ConfigToml = - resolved_config_to_toml(&config.multi_agent_v2, "features.multi_agent_v2")?; - multi_agent_v2.enabled = Some(config.features.enabled(Feature::MultiAgentV2)); - features.multi_agent_v2 = Some(FeatureToml::Config(multi_agent_v2)); - features.apps_mcp_path_override = Some(FeatureToml::Config(AppsMcpPathOverrideConfigToml { - enabled: Some(config.features.enabled(Feature::AppsMcpPathOverride)), - path: config.apps_mcp_path_override.clone(), - })); - - config_toml.memories = Some(resolved_config_to_toml::( - &config.memories, - "memories", - )?); - - let agents = config_toml.agents.get_or_insert_with(Default::default); - // Multi-agent v2 owns thread fanout through its feature config. Preserve - // the legacy agents.max_threads setting only when v2 is disabled. - agents.max_threads = if config.features.enabled(Feature::MultiAgentV2) { - None - } else { - config.agent_max_threads - }; - agents.max_depth = Some(config.agent_max_depth); - agents.job_max_runtime_seconds = config.agent_job_max_runtime_seconds; - agents.interrupt_message = Some(config.agent_interrupt_message_enabled); - - config_toml - .skills - .get_or_insert_with(Default::default) - .include_instructions = Some(config.include_skill_instructions); - - Ok(()) -} - -fn render_template( - template: &Template, - interpolation_source: &TomlValue, - field_path: &str, -) -> anyhow::Result { - let variables = - template - .placeholders() - .map(|placeholder| { - let value = lookup_scalar_path(interpolation_source, placeholder).with_context(|| { - format!("failed to render config field `{field_path}` placeholder `{placeholder}`") - })?; - Ok((placeholder.to_string(), value)) - }) - .collect::>>()?; - - template - .render( - variables - .iter() - .map(|(name, value)| (name.as_str(), value.as_str())), - ) - .with_context(|| format!("failed to render config field `{field_path}`")) -} - -fn lookup_scalar_path(value: &TomlValue, path: &str) -> anyhow::Result { - let resolved = value_at_path(value, path) - .with_context(|| format!("template placeholder `{path}` does not exist"))?; - - match resolved { - TomlValue::String(value) => Ok(value.clone()), - TomlValue::Integer(value) => Ok(value.to_string()), - TomlValue::Float(value) => Ok(value.to_string()), - TomlValue::Boolean(value) => Ok(value.to_string()), - _ => bail!( - "template placeholder `{path}` must resolve to a scalar string, integer, float, or boolean" - ), - } -} - -fn value_at_path<'a>(value: &'a TomlValue, path: &str) -> Option<&'a TomlValue> { - let mut current = value; - for segment in path.split('.') { - current = current.as_table()?.get(segment)?; - } - Some(current) -} - -fn value_mut_at_path<'a>(value: &'a mut TomlValue, path: &str) -> Option<&'a mut TomlValue> { - let mut current = value; - let mut segments = path.split('.').peekable(); - - while let Some(segment) = segments.next() { - let table = current.as_table_mut()?; - if segments.peek().is_none() { - return table.get_mut(segment); - } - current = table.get_mut(segment)?; - } - - Some(current) -} - -fn resolved_config_to_toml( - value: &impl serde::Serialize, - label: &'static str, -) -> anyhow::Result -where - Toml: serde::de::DeserializeOwned + serde::Serialize, -{ - crate::config_lock::toml_round_trip(value, label).map_err(anyhow::Error::from) -} diff --git a/codex-rs/core/src/session/config_lock.rs b/codex-rs/core/src/session/config_lock.rs index f0a8d209d3..d1f190510a 100644 --- a/codex-rs/core/src/session/config_lock.rs +++ b/codex-rs/core/src/session/config_lock.rs @@ -1,12 +1,19 @@ use anyhow::Context; use codex_config::config_toml::ConfigLockfileToml; use codex_config::config_toml::ConfigToml; +use codex_config::types::MemoriesToml; +use codex_features::AppsMcpPathOverrideConfigToml; +use codex_features::Feature; +use codex_features::FeatureToml; +use codex_features::FeaturesToml; +use codex_features::MultiAgentV2ConfigToml; use codex_protocol::ThreadId; -use crate::config::template_interpolation::materialized_config_toml; +use crate::config::Config; use crate::config_lock::ConfigLockReplayOptions; use crate::config_lock::clear_config_lock_debug_controls; use crate::config_lock::config_lockfile; +use crate::config_lock::toml_round_trip; use crate::config_lock::validate_config_lock_replay; use super::SessionConfiguration; @@ -74,12 +81,20 @@ fn session_configuration_to_lock_config_toml( sc: &SessionConfiguration, ) -> anyhow::Result { let config = sc.original_config_do_not_use.as_ref(); - let mut lock_config = materialized_config_toml(config)?; + // Start from the resolved layer stack, then patch in values that are only + // known after session setup. Export and replay validation both use this + // path, so every field here is part of the lockfile contract. + let mut lock_config: ConfigToml = config + .config_layer_stack + .effective_config() + .try_into() + .context("failed to deserialize effective config for config lock")?; if config.config_lock_save_fields_resolved_from_model_catalog { save_session_resolved_fields(sc, &mut lock_config); } + save_config_resolved_fields(config, &mut lock_config)?; drop_lockfile_inputs(&mut lock_config); Ok(lock_config) @@ -103,6 +118,64 @@ fn save_session_resolved_fields(sc: &SessionConfiguration, lock_config: &mut Con lock_config.approvals_reviewer = Some(sc.approvals_reviewer); } +/// Saves values stored on `Config` after higher-level resolution, +/// normalization, defaulting, or feature materialization. +/// +/// Persist the resolved representation so replay compares against the behavior +/// Codex actually ran with, not only the user-authored TOML inputs. +fn save_config_resolved_fields( + config: &Config, + lock_config: &mut ConfigToml, +) -> anyhow::Result<()> { + lock_config.web_search = Some(config.web_search_mode.value()); + lock_config.model_provider = Some(config.model_provider_id.clone()); + lock_config.plan_mode_reasoning_effort = config.plan_mode_reasoning_effort; + lock_config.model_verbosity = config.model_verbosity; + lock_config.include_permissions_instructions = Some(config.include_permissions_instructions); + lock_config.include_apps_instructions = Some(config.include_apps_instructions); + lock_config.include_environment_context = Some(config.include_environment_context); + lock_config.background_terminal_max_timeout = Some(config.background_terminal_max_timeout); + + // Feature aliases and feature configs need to be written in their resolved + // form; otherwise replay can drift when a legacy key maps to the same + // runtime feature. + let features = lock_config + .features + .get_or_insert_with(FeaturesToml::default); + features.materialize_resolved_enabled(config.features.get()); + let mut multi_agent_v2: MultiAgentV2ConfigToml = + resolved_config_to_toml(&config.multi_agent_v2, "features.multi_agent_v2")?; + multi_agent_v2.enabled = Some(config.features.enabled(Feature::MultiAgentV2)); + features.multi_agent_v2 = Some(FeatureToml::Config(multi_agent_v2)); + features.apps_mcp_path_override = Some(FeatureToml::Config(AppsMcpPathOverrideConfigToml { + enabled: Some(config.features.enabled(Feature::AppsMcpPathOverride)), + path: config.apps_mcp_path_override.clone(), + })); + lock_config.memories = Some(resolved_config_to_toml::( + &config.memories, + "memories", + )?); + + let agents = lock_config.agents.get_or_insert_with(Default::default); + // Multi-agent v2 owns thread fanout through its feature config. Preserve + // the legacy agents.max_threads setting only when v2 is disabled. + agents.max_threads = if config.features.enabled(Feature::MultiAgentV2) { + None + } else { + config.agent_max_threads + }; + agents.max_depth = Some(config.agent_max_depth); + agents.job_max_runtime_seconds = config.agent_job_max_runtime_seconds; + agents.interrupt_message = Some(config.agent_interrupt_message_enabled); + + lock_config + .skills + .get_or_insert_with(Default::default) + .include_instructions = Some(config.include_skill_instructions); + + Ok(()) +} + fn drop_lockfile_inputs(lock_config: &mut ConfigToml) { // The lockfile should contain replayable values, not the profile, // debug-control, file-include, and environment-specific inputs that @@ -122,11 +195,19 @@ fn drop_lockfile_inputs(lock_config: &mut ConfigToml) { lock_config.experimental_use_freeform_apply_patch = None; } +fn resolved_config_to_toml( + value: &impl serde::Serialize, + label: &'static str, +) -> anyhow::Result +where + Toml: serde::de::DeserializeOwned + serde::Serialize, +{ + toml_round_trip(value, label).map_err(anyhow::Error::from) +} + #[cfg(test)] mod tests { use super::*; - use codex_features::FeatureToml; - use codex_features::MultiAgentV2ConfigToml; use pretty_assertions::assert_eq; use std::sync::Arc;