fix: sub-agent role when using profiles

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<TomlValue> {
     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<ConfigLayerEntry> = 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);
-
-    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())?;
-
-    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,
+mod reload {
+    use super::*;
+
+    pub(super) fn build_next_config(
+        config: &Config,
+        role_layer_toml: TomlValue,
+        preserve_current_profile: bool,
+        preserve_current_provider: bool,
+    ) -> anyhow::Result<Config> {
+        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 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<ConfigLayerStack> {
+        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<Option<ConfigLayerEntry>> {
+        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<crate::config::ConfigToml> {
+        Ok(deserialize_config_toml_with_base(
+            config_layer_stack.effective_config(),
+            &config.codex_home,
+        )?)
+    }
+
+    fn existing_layers(config: &Config) -> Vec<ConfigLayerEntry> {
+        config
+            .config_layer_stack
+            .get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst, true)
+            .into_iter()
+            .cloned()
+            .collect()
+    }
+
+    fn insert_layer(layers: &mut Vec<ConfigLayerEntry>, 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 {

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");