mirror of
https://github.com/openai/codex.git
synced 2026-05-13 15:52:40 +00:00
Revert "feat: support template interpolation in multi-agent usage hints" (#21337)
Reverts openai/codex#20973
This commit is contained in:
@@ -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()?;
|
||||
|
||||
@@ -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<Self> {
|
||||
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<Self> {
|
||||
// Keep the large config-construction future off small test thread stacks.
|
||||
Box::pin(async move {
|
||||
|
||||
@@ -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<ConfigToml> {
|
||||
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<bool> {
|
||||
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::<MemoriesToml>(
|
||||
&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<String> {
|
||||
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::<anyhow::Result<Vec<_>>>()?;
|
||||
|
||||
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<String> {
|
||||
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<Toml>(
|
||||
value: &impl serde::Serialize,
|
||||
label: &'static str,
|
||||
) -> anyhow::Result<Toml>
|
||||
where
|
||||
Toml: serde::de::DeserializeOwned + serde::Serialize,
|
||||
{
|
||||
crate::config_lock::toml_round_trip(value, label).map_err(anyhow::Error::from)
|
||||
}
|
||||
@@ -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<ConfigToml> {
|
||||
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::<MemoriesToml>(
|
||||
&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<Toml>(
|
||||
value: &impl serde::Serialize,
|
||||
label: &'static str,
|
||||
) -> anyhow::Result<Toml>
|
||||
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;
|
||||
|
||||
|
||||
Reference in New Issue
Block a user