From cacefb52284a9e0f4e780104d66007f877ad0dc0 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Tue, 3 Mar 2026 09:20:25 +0000 Subject: [PATCH] fix: agent when profile (#13235) Co-authored-by: Josh McKinney Co-authored-by: Codex --- codex-rs/core/src/agent/role.rs | 263 +++++++++++++++++- .../core/src/tools/handlers/multi_agents.rs | 24 ++ 2 files changed, 286 insertions(+), 1 deletion(-) diff --git a/codex-rs/core/src/agent/role.rs b/codex-rs/core/src/agent/role.rs index 0ff5a7d56..3ca496d6e 100644 --- a/codex-rs/core/src/agent/role.rs +++ b/codex-rs/core/src/agent/role.rs @@ -1,3 +1,11 @@ +//! Applies agent-role configuration layers on top of an existing session config. +//! +//! Roles are selected at spawn time and are loaded with the same config machinery as +//! `config.toml`. This module resolves built-in and user-defined role files, inserts the role as a +//! high-precedence layer, and preserves the caller's current profile/provider unless the role +//! explicitly takes ownership of model selection. It does not decide when to spawn a sub-agent or +//! which role to use; the multi-agent tool handler owns that orchestration. + use crate::config::AgentRoleConfig; use crate::config::Config; use crate::config::ConfigOverrides; @@ -13,10 +21,18 @@ use std::path::Path; use std::sync::LazyLock; use toml::Value as TomlValue; +/// The role name used when a caller omits `agent_type`. pub const DEFAULT_ROLE_NAME: &str = "default"; const AGENT_TYPE_UNAVAILABLE_ERROR: &str = "agent type is currently not available"; -/// Applies a role config layer to a mutable config and preserves unspecified keys. +/// Applies a named role layer to `config` while preserving caller-owned model selection. +/// +/// The role layer is inserted at session-flag precedence so it can override persisted config, but +/// the caller's current `profile` and `model_provider` remain sticky runtime choices unless the +/// role explicitly sets `profile`, explicitly sets `model_provider`, or rewrites the active +/// profile's `model_provider` in place. Rebuilding the config without those overrides would make a +/// spawned agent silently fall back to the default provider, which is the bug this preservation +/// logic avoids. pub(crate) async fn apply_role_to_config( config: &mut Config, role_name: Option<&str>, @@ -60,6 +76,25 @@ pub(crate) async fn apply_role_to_config( .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())?; + 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 + .active_profile + .as_ref() + .and_then(|active_profile| { + role_layer_toml + .get("profiles") + .and_then(TomlValue::as_table) + .and_then(|profiles| profiles.get(active_profile)) + .and_then(TomlValue::as_table) + .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; let mut layers: Vec = config .config_layer_stack @@ -86,6 +121,10 @@ pub(crate) async fn apply_role_to_config( merged_config, 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(), @@ -225,6 +264,7 @@ Rules: #[cfg(test)] mod tests { use super::*; + use crate::config::CONFIG_TOML_FILE; use crate::config::ConfigBuilder; use crate::config_loader::ConfigLayerStackOrdering; use crate::plugins::PluginsManager; @@ -382,6 +422,227 @@ mod tests { ); } + #[tokio::test] + async fn apply_role_preserves_active_profile_and_model_provider() { + let home = TempDir::new().expect("create temp dir"); + tokio::fs::write( + home.path().join(CONFIG_TOML_FILE), + r#" +[model_providers.test-provider] +name = "Test Provider" +base_url = "https://example.com/v1" +env_key = "TEST_PROVIDER_API_KEY" +wire_api = "responses" + +[profiles.test-profile] +model_provider = "test-provider" +"#, + ) + .await + .expect("write config.toml"); + let mut config = ConfigBuilder::default() + .codex_home(home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + config_profile: Some("test-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, "empty-role.toml", "").await; + config.agent_roles.insert( + "custom".to_string(), + AgentRoleConfig { + description: None, + config_file: Some(role_path), + }, + ); + + apply_role_to_config(&mut config, Some("custom")) + .await + .expect("custom role should apply"); + + assert_eq!(config.active_profile.as_deref(), Some("test-profile")); + assert_eq!(config.model_provider_id, "test-provider"); + assert_eq!(config.model_provider.name, "Test Provider"); + } + + #[tokio::test] + async fn apply_role_uses_role_profile_instead_of_current_profile() { + let home = TempDir::new().expect("create temp dir"); + tokio::fs::write( + home.path().join(CONFIG_TOML_FILE), + r#" +[model_providers.base-provider] +name = "Base Provider" +base_url = "https://base.example.com/v1" +env_key = "BASE_PROVIDER_API_KEY" +wire_api = "responses" + +[model_providers.role-provider] +name = "Role Provider" +base_url = "https://role.example.com/v1" +env_key = "ROLE_PROVIDER_API_KEY" +wire_api = "responses" + +[profiles.base-profile] +model_provider = "base-provider" + +[profiles.role-profile] +model_provider = "role-provider" +"#, + ) + .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, "profile-role.toml", "profile = \"role-profile\"").await; + config.agent_roles.insert( + "custom".to_string(), + AgentRoleConfig { + description: None, + config_file: Some(role_path), + }, + ); + + apply_role_to_config(&mut config, Some("custom")) + .await + .expect("custom role should apply"); + + assert_eq!(config.active_profile.as_deref(), Some("role-profile")); + assert_eq!(config.model_provider_id, "role-provider"); + assert_eq!(config.model_provider.name, "Role Provider"); + } + + #[tokio::test] + async fn apply_role_uses_role_model_provider_instead_of_current_profile_provider() { + let home = TempDir::new().expect("create temp dir"); + tokio::fs::write( + home.path().join(CONFIG_TOML_FILE), + r#" +[model_providers.base-provider] +name = "Base Provider" +base_url = "https://base.example.com/v1" +env_key = "BASE_PROVIDER_API_KEY" +wire_api = "responses" + +[model_providers.role-provider] +name = "Role Provider" +base_url = "https://role.example.com/v1" +env_key = "ROLE_PROVIDER_API_KEY" +wire_api = "responses" + +[profiles.base-profile] +model_provider = "base-provider" +"#, + ) + .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, + "provider-role.toml", + "model_provider = \"role-provider\"", + ) + .await; + config.agent_roles.insert( + "custom".to_string(), + AgentRoleConfig { + description: None, + config_file: Some(role_path), + }, + ); + + apply_role_to_config(&mut config, Some("custom")) + .await + .expect("custom role should apply"); + + assert_eq!(config.active_profile, None); + assert_eq!(config.model_provider_id, "role-provider"); + assert_eq!(config.model_provider.name, "Role Provider"); + } + + #[tokio::test] + async fn apply_role_uses_active_profile_model_provider_update() { + let home = TempDir::new().expect("create temp dir"); + tokio::fs::write( + home.path().join(CONFIG_TOML_FILE), + r#" +[model_providers.base-provider] +name = "Base Provider" +base_url = "https://base.example.com/v1" +env_key = "BASE_PROVIDER_API_KEY" +wire_api = "responses" + +[model_providers.role-provider] +name = "Role Provider" +base_url = "https://role.example.com/v1" +env_key = "ROLE_PROVIDER_API_KEY" +wire_api = "responses" + +[profiles.base-profile] +model_provider = "base-provider" +model_reasoning_effort = "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, + "profile-edit-role.toml", + r#"[profiles.base-profile] +model_provider = "role-provider" +model_reasoning_effort = "high" +"#, + ) + .await; + config.agent_roles.insert( + "custom".to_string(), + AgentRoleConfig { + description: None, + config_file: Some(role_path), + }, + ); + + 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_provider_id, "role-provider"); + assert_eq!(config.model_provider.name, "Role Provider"); + assert_eq!(config.model_reasoning_effort, Some(ReasoningEffort::High)); + } + #[tokio::test] #[cfg(not(windows))] async fn apply_role_does_not_materialize_default_sandbox_workspace_write_fields() { diff --git a/codex-rs/core/src/tools/handlers/multi_agents.rs b/codex-rs/core/src/tools/handlers/multi_agents.rs index 076a496a4..c179fcef7 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents.rs @@ -1,3 +1,10 @@ +//! Implements the collaboration tool surface for spawning and managing sub-agents. +//! +//! This handler translates model tool calls into `AgentControl` operations and keeps spawned +//! agents aligned with the live turn that created them. Sub-agents start from the turn's effective +//! config, inherit runtime-only state such as provider, approval policy, sandbox, and cwd, and +//! then optionally layer role-specific config on top. + use crate::agent::AgentStatus; use crate::agent::exceeds_thread_spawn_depth_limit; use crate::codex::Session; @@ -35,6 +42,7 @@ use serde::Deserialize; use serde::Serialize; use std::collections::HashMap; +/// Function-tool handler for the multi-agent collaboration API. pub struct MultiAgentHandler; /// Minimum wait timeout to prevent tight polling loops from burning CPU. @@ -894,6 +902,13 @@ fn input_preview(items: &[UserInput]) -> String { parts.join("\n") } +/// Builds the base config snapshot for a newly spawned sub-agent. +/// +/// The returned config starts from the parent's effective config and then refreshes the +/// runtime-owned fields carried on `turn`, including model selection, reasoning settings, +/// approval policy, sandbox, and cwd. Role-specific overrides are layered after this step; +/// skipping this helper and cloning stale config state directly can send the child agent out with +/// the wrong provider or runtime policy. pub(crate) fn build_agent_spawn_config( base_instructions: &BaseInstructions, turn: &TurnContext, @@ -928,6 +943,10 @@ fn build_agent_shared_config(turn: &TurnContext) -> Result