From 33acc1e65faec89172b80a0a8a4faafe9b65c8c5 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 16 Mar 2026 16:08:16 +0000 Subject: [PATCH] fix: sub-agent role when using profiles (#14807) Fix the layering conflict when a project profile is used with agents. This PR clean the config layering and make sure the agent config > project profile Fix https://github.com/openai/codex/issues/13849, https://github.com/openai/codex/issues/14671 --- codex-rs/core/src/agent/role.rs | 245 ++++++++++++++++++-------- codex-rs/core/src/agent/role_tests.rs | 61 +++++++ 2 files changed, 237 insertions(+), 69 deletions(-) diff --git a/codex-rs/core/src/agent/role.rs b/codex-rs/core/src/agent/role.rs index 8d607c543..afb833902 100644 --- a/codex-rs/core/src/agent/role.rs +++ b/codex-rs/core/src/agent/role.rs @@ -15,6 +15,7 @@ use crate::config_loader::ConfigLayerEntry; use crate::config_loader::ConfigLayerStack; use crate::config_loader::ConfigLayerStackOrdering; use crate::config_loader::resolve_relative_paths_in_config_toml; +use anyhow::anyhow; use codex_app_server_protocol::ConfigLayerSource; use std::collections::BTreeMap; use std::collections::BTreeSet; @@ -39,46 +40,86 @@ pub(crate) async fn apply_role_to_config( role_name: Option<&str>, ) -> Result<(), String> { let role_name = role_name.unwrap_or(DEFAULT_ROLE_NAME); - let is_built_in = !config.agent_roles.contains_key(role_name); - let (config_file, is_built_in) = resolve_role_config(config, role_name) - .map(|role| (&role.config_file, is_built_in)) + + let role = resolve_role_config(config, role_name) + .cloned() .ok_or_else(|| format!("unknown agent_type '{role_name}'"))?; - let Some(config_file) = config_file.as_ref() else { + + apply_role_to_config_inner(config, role_name, &role) + .await + .map_err(|err| { + tracing::warn!("failed to apply role to config: {err}"); + AGENT_TYPE_UNAVAILABLE_ERROR.to_string() + }) +} + +async fn apply_role_to_config_inner( + config: &mut Config, + role_name: &str, + role: &AgentRoleConfig, +) -> anyhow::Result<()> { + let is_built_in = !config.agent_roles.contains_key(role_name); + let Some(config_file) = role.config_file.as_ref() else { return Ok(()); }; + let role_layer_toml = load_role_layer_toml(config, config_file, is_built_in, role_name).await?; + let (preserve_current_profile, preserve_current_provider) = + preservation_policy(config, &role_layer_toml); + *config = reload::build_next_config( + config, + role_layer_toml, + preserve_current_profile, + preserve_current_provider, + )?; + Ok(()) +} + +async fn load_role_layer_toml( + config: &Config, + config_file: &Path, + is_built_in: bool, + role_name: &str, +) -> anyhow::Result { let (role_config_toml, role_config_base) = if is_built_in { let role_config_contents = built_in::config_file_contents(config_file) .map(str::to_owned) - .ok_or_else(|| AGENT_TYPE_UNAVAILABLE_ERROR.to_string())?; - let role_config_toml: TomlValue = toml::from_str(&role_config_contents) - .map_err(|_| AGENT_TYPE_UNAVAILABLE_ERROR.to_string())?; + .ok_or(anyhow!("No corresponding config content"))?; + let role_config_toml: TomlValue = toml::from_str(&role_config_contents)?; (role_config_toml, config.codex_home.as_path()) } else { - let role_config_contents = tokio::fs::read_to_string(config_file) - .await - .map_err(|_| AGENT_TYPE_UNAVAILABLE_ERROR.to_string())?; + let role_config_contents = tokio::fs::read_to_string(config_file).await?; + let role_config_base = config_file + .parent() + .ok_or(anyhow!("No corresponding config content"))?; let role_config_toml = parse_agent_role_file_contents( &role_config_contents, config_file, - config_file - .parent() - .ok_or_else(|| AGENT_TYPE_UNAVAILABLE_ERROR.to_string())?, + role_config_base, Some(role_name), - ) - .map_err(|_| AGENT_TYPE_UNAVAILABLE_ERROR.to_string())? + )? .config; - ( - role_config_toml, - config_file - .parent() - .ok_or_else(|| AGENT_TYPE_UNAVAILABLE_ERROR.to_string())?, - ) + (role_config_toml, role_config_base) }; - deserialize_config_toml_with_base(role_config_toml.clone(), role_config_base) - .map_err(|_| AGENT_TYPE_UNAVAILABLE_ERROR.to_string())?; - let role_layer_toml = resolve_relative_paths_in_config_toml(role_config_toml, role_config_base) - .map_err(|_| AGENT_TYPE_UNAVAILABLE_ERROR.to_string())?; + + deserialize_config_toml_with_base(role_config_toml.clone(), role_config_base)?; + Ok(resolve_relative_paths_in_config_toml( + role_config_toml, + role_config_base, + )?) +} + +pub(crate) fn resolve_role_config<'a>( + config: &'a Config, + role_name: &str, +) -> Option<&'a AgentRoleConfig> { + config + .agent_roles + .get(role_name) + .or_else(|| built_in::configs().get(role_name)) +} + +fn preservation_policy(config: &Config, role_layer_toml: &TomlValue) -> (bool, bool) { let role_selects_provider = role_layer_toml.get("model_provider").is_some(); let role_selects_profile = role_layer_toml.get("profile").is_some(); let role_updates_active_profile_provider = config @@ -93,63 +134,129 @@ pub(crate) async fn apply_role_to_config( .map(|profile| profile.contains_key("model_provider")) }) .unwrap_or(false); - // A role that does not explicitly take ownership of model selection should inherit the - // caller's current profile/provider choices across the config reload. let preserve_current_profile = !role_selects_provider && !role_selects_profile; let preserve_current_provider = preserve_current_profile && !role_updates_active_profile_provider; + (preserve_current_profile, preserve_current_provider) +} - let mut layers: Vec = config - .config_layer_stack - .get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst, true) - .into_iter() - .cloned() - .collect(); - let layer = ConfigLayerEntry::new(ConfigLayerSource::SessionFlags, role_layer_toml); - let insertion_index = - layers.partition_point(|existing_layer| existing_layer.name <= layer.name); - layers.insert(insertion_index, layer); +mod reload { + use super::*; - let config_layer_stack = ConfigLayerStack::new( - layers, - config.config_layer_stack.requirements().clone(), - config.config_layer_stack.requirements_toml().clone(), - ) - .map_err(|_| AGENT_TYPE_UNAVAILABLE_ERROR.to_string())?; + pub(super) fn build_next_config( + config: &Config, + role_layer_toml: TomlValue, + preserve_current_profile: bool, + preserve_current_provider: bool, + ) -> anyhow::Result { + let active_profile_name = preserve_current_profile + .then_some(config.active_profile.as_deref()) + .flatten(); + let config_layer_stack = + build_config_layer_stack(config, &role_layer_toml, active_profile_name)?; + let mut merged_config = deserialize_effective_config(config, &config_layer_stack)?; + if preserve_current_profile { + merged_config.profile = None; + } - let merged_toml = config_layer_stack.effective_config(); - let merged_config = deserialize_config_toml_with_base(merged_toml, &config.codex_home) - .map_err(|_| AGENT_TYPE_UNAVAILABLE_ERROR.to_string())?; - let next_config = Config::load_config_with_layer_stack( - merged_config, + let mut next_config = Config::load_config_with_layer_stack( + merged_config, + reload_overrides(config, preserve_current_provider), + config.codex_home.clone(), + config_layer_stack, + )?; + if preserve_current_profile { + next_config.active_profile = config.active_profile.clone(); + } + Ok(next_config) + } + + fn build_config_layer_stack( + config: &Config, + role_layer_toml: &TomlValue, + active_profile_name: Option<&str>, + ) -> anyhow::Result { + let mut layers = existing_layers(config); + if let Some(resolved_profile_layer) = + resolved_profile_layer(config, &layers, role_layer_toml, active_profile_name)? + { + insert_layer(&mut layers, resolved_profile_layer); + } + insert_layer(&mut layers, role_layer(role_layer_toml.clone())); + Ok(ConfigLayerStack::new( + layers, + config.config_layer_stack.requirements().clone(), + config.config_layer_stack.requirements_toml().clone(), + )?) + } + + fn resolved_profile_layer( + config: &Config, + existing_layers: &[ConfigLayerEntry], + role_layer_toml: &TomlValue, + active_profile_name: Option<&str>, + ) -> anyhow::Result> { + let Some(active_profile_name) = active_profile_name else { + return Ok(None); + }; + + let mut layers = existing_layers.to_vec(); + insert_layer(&mut layers, role_layer(role_layer_toml.clone())); + let merged_config = deserialize_effective_config( + config, + &ConfigLayerStack::new( + layers, + config.config_layer_stack.requirements().clone(), + config.config_layer_stack.requirements_toml().clone(), + )?, + )?; + let resolved_profile = + merged_config.get_config_profile(Some(active_profile_name.to_string()))?; + Ok(Some(ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + TomlValue::try_from(resolved_profile)?, + ))) + } + + fn deserialize_effective_config( + config: &Config, + config_layer_stack: &ConfigLayerStack, + ) -> anyhow::Result { + Ok(deserialize_config_toml_with_base( + config_layer_stack.effective_config(), + &config.codex_home, + )?) + } + + fn existing_layers(config: &Config) -> Vec { + config + .config_layer_stack + .get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst, true) + .into_iter() + .cloned() + .collect() + } + + fn insert_layer(layers: &mut Vec, layer: ConfigLayerEntry) { + let insertion_index = + layers.partition_point(|existing_layer| existing_layer.name <= layer.name); + layers.insert(insertion_index, layer); + } + + fn role_layer(role_layer_toml: TomlValue) -> ConfigLayerEntry { + ConfigLayerEntry::new(ConfigLayerSource::SessionFlags, role_layer_toml) + } + + fn reload_overrides(config: &Config, preserve_current_provider: bool) -> ConfigOverrides { ConfigOverrides { cwd: Some(config.cwd.clone()), model_provider: preserve_current_provider.then(|| config.model_provider_id.clone()), - config_profile: preserve_current_profile - .then(|| config.active_profile.clone()) - .flatten(), codex_linux_sandbox_exe: config.codex_linux_sandbox_exe.clone(), main_execve_wrapper_exe: config.main_execve_wrapper_exe.clone(), js_repl_node_path: config.js_repl_node_path.clone(), ..Default::default() - }, - config.codex_home.clone(), - config_layer_stack, - ) - .map_err(|_| AGENT_TYPE_UNAVAILABLE_ERROR.to_string())?; - *config = next_config; - - Ok(()) -} - -pub(crate) fn resolve_role_config<'a>( - config: &'a Config, - role_name: &str, -) -> Option<&'a AgentRoleConfig> { - config - .agent_roles - .get(role_name) - .or_else(|| built_in::configs().get(role_name)) + } + } } pub(crate) mod spawn_tool_spec { diff --git a/codex-rs/core/src/agent/role_tests.rs b/codex-rs/core/src/agent/role_tests.rs index cb04aa4e8..7726ce795 100644 --- a/codex-rs/core/src/agent/role_tests.rs +++ b/codex-rs/core/src/agent/role_tests.rs @@ -4,6 +4,8 @@ use crate::config::ConfigBuilder; use crate::config_loader::ConfigLayerStackOrdering; use crate::plugins::PluginsManager; use crate::skills::SkillsManager; +use codex_protocol::config_types::ReasoningSummary; +use codex_protocol::config_types::Verbosity; use codex_protocol::openai_models::ReasoningEffort; use pretty_assertions::assert_eq; use std::fs; @@ -243,6 +245,65 @@ model_provider = "test-provider" assert_eq!(config.model_provider.name, "Test Provider"); } +#[tokio::test] +async fn apply_role_top_level_profile_settings_override_preserved_profile() { + let home = TempDir::new().expect("create temp dir"); + tokio::fs::write( + home.path().join(CONFIG_TOML_FILE), + r#" +[profiles.base-profile] +model = "profile-model" +model_reasoning_effort = "low" +model_reasoning_summary = "concise" +model_verbosity = "low" +"#, + ) + .await + .expect("write config.toml"); + let mut config = ConfigBuilder::default() + .codex_home(home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + config_profile: Some("base-profile".to_string()), + ..Default::default() + }) + .fallback_cwd(Some(home.path().to_path_buf())) + .build() + .await + .expect("load config"); + let role_path = write_role_config( + &home, + "top-level-profile-settings-role.toml", + r#"developer_instructions = "Stay focused" +model = "role-model" +model_reasoning_effort = "high" +model_reasoning_summary = "detailed" +model_verbosity = "high" +"#, + ) + .await; + config.agent_roles.insert( + "custom".to_string(), + AgentRoleConfig { + description: None, + config_file: Some(role_path), + nickname_candidates: None, + }, + ); + + apply_role_to_config(&mut config, Some("custom")) + .await + .expect("custom role should apply"); + + assert_eq!(config.active_profile.as_deref(), Some("base-profile")); + assert_eq!(config.model.as_deref(), Some("role-model")); + assert_eq!(config.model_reasoning_effort, Some(ReasoningEffort::High)); + assert_eq!( + config.model_reasoning_summary, + Some(ReasoningSummary::Detailed) + ); + assert_eq!(config.model_verbosity, Some(Verbosity::High)); +} + #[tokio::test] async fn apply_role_uses_role_profile_instead_of_current_profile() { let home = TempDir::new().expect("create temp dir");