fix: agent when profile (#13235)
Co-authored-by: Josh McKinney <joshka@openai.com> Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
parent
3166a5ba82
commit
cacefb5228
2 changed files with 286 additions and 1 deletions
|
|
@ -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<ConfigLayerEntry> = 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() {
|
||||
|
|
|
|||
|
|
@ -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<Config, FunctionCallE
|
|||
Ok(config)
|
||||
}
|
||||
|
||||
/// Copies runtime-only turn state onto a child config before it is handed to `AgentControl`.
|
||||
///
|
||||
/// These values are chosen by the live turn rather than persisted config, so leaving them stale
|
||||
/// can make a child agent disagree with its parent about approval policy, cwd, or sandboxing.
|
||||
fn apply_spawn_agent_runtime_overrides(
|
||||
config: &mut Config,
|
||||
turn: &TurnContext,
|
||||
|
|
@ -1114,6 +1133,9 @@ mod tests {
|
|||
let manager = thread_manager();
|
||||
session.services.agent_control = manager.agent_control();
|
||||
let mut config = (*turn.config).clone();
|
||||
let provider = built_in_model_providers()["ollama"].clone();
|
||||
config.model_provider_id = "ollama".to_string();
|
||||
config.model_provider = provider.clone();
|
||||
config
|
||||
.permissions
|
||||
.approval_policy
|
||||
|
|
@ -1122,6 +1144,7 @@ mod tests {
|
|||
turn.approval_policy
|
||||
.set(AskForApproval::OnRequest)
|
||||
.expect("approval policy should be set");
|
||||
turn.provider = provider;
|
||||
turn.config = Arc::new(config);
|
||||
|
||||
let invocation = invocation(
|
||||
|
|
@ -1160,6 +1183,7 @@ mod tests {
|
|||
.config_snapshot()
|
||||
.await;
|
||||
assert_eq!(snapshot.approval_policy, AskForApproval::OnRequest);
|
||||
assert_eq!(snapshot.model_provider_id, "ollama");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue