From a67660da2d274282c9c8dee7101787bf023e6f94 Mon Sep 17 00:00:00 2001 From: gabec-openai Date: Tue, 10 Mar 2026 16:21:48 -0700 Subject: [PATCH] Load agent metadata from role files (#14177) --- codex-rs/core/config.schema.json | 2 +- codex-rs/core/src/agent/role.rs | 104 +++- codex-rs/core/src/config/agent_roles.rs | 469 +++++++++++++++ codex-rs/core/src/config/config_tests.rs | 737 ++++++++++++++++++++++- codex-rs/core/src/config/mod.rs | 116 +--- 5 files changed, 1293 insertions(+), 135 deletions(-) create mode 100644 codex-rs/core/src/config/agent_roles.rs diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 573b35218..067c60585 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -18,7 +18,7 @@ "description": "Path to a role-specific config layer. Relative paths are resolved relative to the `config.toml` that defines them." }, "description": { - "description": "Human-facing role documentation used in spawn tool guidance.", + "description": "Human-facing role documentation used in spawn tool guidance. Required unless supplied by the referenced agent role file.", "type": "string" }, "nickname_candidates": { diff --git a/codex-rs/core/src/agent/role.rs b/codex-rs/core/src/agent/role.rs index 3a635a7e1..878f8fc84 100644 --- a/codex-rs/core/src/agent/role.rs +++ b/codex-rs/core/src/agent/role.rs @@ -9,6 +9,7 @@ use crate::config::AgentRoleConfig; use crate::config::Config; use crate::config::ConfigOverrides; +use crate::config::agent_roles::parse_agent_role_file_contents; use crate::config::deserialize_config_toml_with_base; use crate::config_loader::ConfigLayerEntry; use crate::config_loader::ConfigLayerStack; @@ -46,26 +47,34 @@ pub(crate) async fn apply_role_to_config( return Ok(()); }; - let (role_config_contents, role_config_base) = if is_built_in { - ( - built_in::config_file_contents(config_file) - .map(str::to_owned) - .ok_or_else(|| AGENT_TYPE_UNAVAILABLE_ERROR.to_string())?, - config.codex_home.as_path(), - ) + 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())?; + (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_toml = parse_agent_role_file_contents( + &role_config_contents, + config_file, + config_file + .parent() + .ok_or_else(|| AGENT_TYPE_UNAVAILABLE_ERROR.to_string())?, + Some(role_name), + ) + .map_err(|_| AGENT_TYPE_UNAVAILABLE_ERROR.to_string())? + .config; ( - tokio::fs::read_to_string(config_file) - .await - .map_err(|_| AGENT_TYPE_UNAVAILABLE_ERROR.to_string())?, + role_config_toml, config_file .parent() .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())?; 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) @@ -391,6 +400,37 @@ mod tests { assert_eq!(err, AGENT_TYPE_UNAVAILABLE_ERROR); } + #[tokio::test] + async fn apply_role_ignores_agent_metadata_fields_in_user_role_file() { + let (home, mut config) = test_config_with_cli_overrides(Vec::new()).await; + let role_path = write_role_config( + &home, + "metadata-role.toml", + r#" +name = "archivist" +description = "Role metadata" +nickname_candidates = ["Hypatia"] +developer_instructions = "Stay focused" +model = "role-model" +"#, + ) + .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.model.as_deref(), Some("role-model")); + } + #[tokio::test] async fn apply_role_preserves_unspecified_keys() { let (home, mut config) = test_config_with_cli_overrides(vec![( @@ -403,7 +443,7 @@ mod tests { let role_path = write_role_config( &home, "effort-only.toml", - "model_reasoning_effort = \"high\"", + "developer_instructions = \"Stay focused\"\nmodel_reasoning_effort = \"high\"", ) .await; config.agent_roles.insert( @@ -459,7 +499,12 @@ model_provider = "test-provider" .build() .await .expect("load config"); - let role_path = write_role_config(&home, "empty-role.toml", "").await; + let role_path = write_role_config( + &home, + "empty-role.toml", + "developer_instructions = \"Stay focused\"", + ) + .await; config.agent_roles.insert( "custom".to_string(), AgentRoleConfig { @@ -515,8 +560,12 @@ model_provider = "role-provider" .build() .await .expect("load config"); - let role_path = - write_role_config(&home, "profile-role.toml", "profile = \"role-profile\"").await; + let role_path = write_role_config( + &home, + "profile-role.toml", + "developer_instructions = \"Stay focused\"\nprofile = \"role-profile\"", + ) + .await; config.agent_roles.insert( "custom".to_string(), AgentRoleConfig { @@ -572,7 +621,7 @@ model_provider = "base-provider" let role_path = write_role_config( &home, "provider-role.toml", - "model_provider = \"role-provider\"", + "developer_instructions = \"Stay focused\"\nmodel_provider = \"role-provider\"", ) .await; config.agent_roles.insert( @@ -631,7 +680,9 @@ model_reasoning_effort = "low" let role_path = write_role_config( &home, "profile-edit-role.toml", - r#"[profiles.base-profile] + r#"developer_instructions = "Stay focused" + +[profiles.base-profile] model_provider = "role-provider" model_reasoning_effort = "high" "#, @@ -674,7 +725,9 @@ model_reasoning_effort = "high" let role_path = write_role_config( &home, "sandbox-role.toml", - r#"[sandbox_workspace_write] + r#"developer_instructions = "Stay focused" + +[sandbox_workspace_write] writable_roots = ["./sandbox-root"] "#, ) @@ -732,7 +785,12 @@ writable_roots = ["./sandbox-root"] )]) .await; let before_layers = session_flags_layer_count(&config); - let role_path = write_role_config(&home, "model-role.toml", "model = \"role-model\"").await; + let role_path = write_role_config( + &home, + "model-role.toml", + "developer_instructions = \"Stay focused\"\nmodel = \"role-model\"", + ) + .await; config.agent_roles.insert( "custom".to_string(), AgentRoleConfig { @@ -766,7 +824,9 @@ writable_roots = ["./sandbox-root"] &home, "skills-role.toml", &format!( - r#"[[skills.config]] + r#"developer_instructions = "Stay focused" + +[[skills.config]] path = "{}" enabled = false "#, diff --git a/codex-rs/core/src/config/agent_roles.rs b/codex-rs/core/src/config/agent_roles.rs new file mode 100644 index 000000000..f1ca4c43c --- /dev/null +++ b/codex-rs/core/src/config/agent_roles.rs @@ -0,0 +1,469 @@ +use super::AgentRoleConfig; +use super::AgentRoleToml; +use super::AgentsToml; +use super::ConfigToml; +use crate::config_loader::ConfigLayerStack; +use crate::config_loader::ConfigLayerStackOrdering; +use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_absolute_path::AbsolutePathBufGuard; +use serde::Deserialize; +use std::collections::BTreeMap; +use std::collections::BTreeSet; +use std::io::ErrorKind; +use std::path::Path; +use std::path::PathBuf; +use toml::Value as TomlValue; + +pub(crate) fn load_agent_roles( + cfg: &ConfigToml, + config_layer_stack: &ConfigLayerStack, +) -> std::io::Result> { + let layers = + config_layer_stack.get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst, false); + if layers.is_empty() { + return load_agent_roles_without_layers(cfg); + } + + let mut roles: BTreeMap = BTreeMap::new(); + for layer in layers { + let mut layer_roles: BTreeMap = BTreeMap::new(); + let mut declared_role_files = BTreeSet::new(); + if let Some(agents_toml) = agents_toml_from_layer(&layer.config)? { + for (declared_role_name, role_toml) in &agents_toml.roles { + let (role_name, role) = read_declared_role(declared_role_name, role_toml)?; + if let Some(config_file) = role.config_file.clone() { + declared_role_files.insert(config_file); + } + if layer_roles.insert(role_name.clone(), role).is_some() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "duplicate agent role name `{role_name}` declared in the same config layer" + ), + )); + } + } + } + + if let Some(config_folder) = layer.config_folder() { + for (role_name, role) in discover_agent_roles_in_dir( + config_folder.as_path().join("agents").as_path(), + &declared_role_files, + )? { + layer_roles.insert(role_name, role); + } + } + + for (role_name, role) in layer_roles { + let mut merged_role = role; + if let Some(existing_role) = roles.get(&role_name) { + merge_missing_role_fields(&mut merged_role, existing_role); + } + validate_required_agent_role_description( + &role_name, + merged_role.description.as_deref(), + )?; + roles.insert(role_name, merged_role); + } + } + + Ok(roles) +} + +fn load_agent_roles_without_layers( + cfg: &ConfigToml, +) -> std::io::Result> { + let mut roles = BTreeMap::new(); + if let Some(agents_toml) = cfg.agents.as_ref() { + for (declared_role_name, role_toml) in &agents_toml.roles { + let (role_name, role) = read_declared_role(declared_role_name, role_toml)?; + validate_required_agent_role_description(&role_name, role.description.as_deref())?; + + if roles.insert(role_name.clone(), role).is_some() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!("duplicate agent role name `{role_name}` declared in config"), + )); + } + } + } + + Ok(roles) +} + +fn read_declared_role( + declared_role_name: &str, + role_toml: &AgentRoleToml, +) -> std::io::Result<(String, AgentRoleConfig)> { + let mut role = agent_role_config_from_toml(declared_role_name, role_toml)?; + let mut role_name = declared_role_name.to_string(); + if let Some(config_file) = role.config_file.as_deref() { + let parsed_file = read_resolved_agent_role_file(config_file, Some(declared_role_name))?; + role_name = parsed_file.role_name; + role.description = parsed_file.description.or(role.description); + role.nickname_candidates = parsed_file.nickname_candidates.or(role.nickname_candidates); + } + + Ok((role_name, role)) +} + +fn merge_missing_role_fields(role: &mut AgentRoleConfig, fallback: &AgentRoleConfig) { + role.description = role.description.clone().or(fallback.description.clone()); + role.config_file = role.config_file.clone().or(fallback.config_file.clone()); + role.nickname_candidates = role + .nickname_candidates + .clone() + .or(fallback.nickname_candidates.clone()); +} + +fn agents_toml_from_layer(layer_toml: &TomlValue) -> std::io::Result> { + let Some(agents_toml) = layer_toml.get("agents") else { + return Ok(None); + }; + + agents_toml + .clone() + .try_into() + .map(Some) + .map_err(|err| std::io::Error::new(std::io::ErrorKind::InvalidData, err)) +} + +fn agent_role_config_from_toml( + role_name: &str, + role: &AgentRoleToml, +) -> std::io::Result { + let config_file = role.config_file.as_ref().map(AbsolutePathBuf::to_path_buf); + validate_agent_role_config_file(role_name, config_file.as_deref())?; + let description = normalize_agent_role_description( + &format!("agents.{role_name}.description"), + role.description.as_deref(), + )?; + let nickname_candidates = normalize_agent_role_nickname_candidates( + &format!("agents.{role_name}.nickname_candidates"), + role.nickname_candidates.as_deref(), + )?; + + Ok(AgentRoleConfig { + description, + config_file, + nickname_candidates, + }) +} + +#[derive(Deserialize, Debug, Clone, Default, PartialEq)] +#[serde(deny_unknown_fields)] +struct RawAgentRoleFileToml { + name: Option, + description: Option, + nickname_candidates: Option>, + #[serde(flatten)] + config: ConfigToml, +} + +#[derive(Debug, Clone, PartialEq)] +pub(crate) struct ResolvedAgentRoleFile { + pub(crate) role_name: String, + pub(crate) description: Option, + pub(crate) nickname_candidates: Option>, + pub(crate) config: TomlValue, +} + +pub(crate) fn parse_agent_role_file_contents( + contents: &str, + role_file_label: &Path, + config_base_dir: &Path, + role_name_hint: Option<&str>, +) -> std::io::Result { + let role_file_toml: TomlValue = toml::from_str(contents).map_err(|err| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!( + "failed to parse agent role file at {}: {err}", + role_file_label.display() + ), + ) + })?; + let _guard = AbsolutePathBufGuard::new(config_base_dir); + let parsed: RawAgentRoleFileToml = role_file_toml.clone().try_into().map_err(|err| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!( + "failed to deserialize agent role file at {}: {err}", + role_file_label.display() + ), + ) + })?; + let description = normalize_agent_role_description( + &format!("agent role file {}.description", role_file_label.display()), + parsed.description.as_deref(), + )?; + validate_agent_role_file_developer_instructions( + role_file_label, + parsed.config.developer_instructions.as_deref(), + role_name_hint.is_none(), + )?; + + let role_name = parsed + .name + .as_deref() + .map(str::trim) + .filter(|name| !name.is_empty()) + .map(ToOwned::to_owned) + .or_else(|| role_name_hint.map(ToOwned::to_owned)) + .ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "agent role file at {} must define a non-empty `name`", + role_file_label.display() + ), + ) + })?; + + let nickname_candidates = normalize_agent_role_nickname_candidates( + &format!( + "agent role file {}.nickname_candidates", + role_file_label.display() + ), + parsed.nickname_candidates.as_deref(), + )?; + + let mut config = role_file_toml; + let Some(config_table) = config.as_table_mut() else { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!( + "agent role file at {} must contain a TOML table", + role_file_label.display() + ), + )); + }; + config_table.remove("name"); + config_table.remove("description"); + config_table.remove("nickname_candidates"); + + Ok(ResolvedAgentRoleFile { + role_name, + description, + nickname_candidates, + config, + }) +} + +fn read_resolved_agent_role_file( + path: &Path, + role_name_hint: Option<&str>, +) -> std::io::Result { + let contents = std::fs::read_to_string(path)?; + parse_agent_role_file_contents( + &contents, + path, + path.parent().unwrap_or(path), + role_name_hint, + ) +} + +fn normalize_agent_role_description( + field_label: &str, + description: Option<&str>, +) -> std::io::Result> { + match description.map(str::trim) { + Some("") => Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!("{field_label} cannot be blank"), + )), + Some(description) => Ok(Some(description.to_string())), + None => Ok(None), + } +} + +fn validate_required_agent_role_description( + role_name: &str, + description: Option<&str>, +) -> std::io::Result<()> { + if description.is_some() { + Ok(()) + } else { + Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!("agent role `{role_name}` must define a description"), + )) + } +} + +fn validate_agent_role_file_developer_instructions( + role_file_label: &Path, + developer_instructions: Option<&str>, + require_present: bool, +) -> std::io::Result<()> { + match developer_instructions.map(str::trim) { + Some("") => Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "agent role file at {}.developer_instructions cannot be blank", + role_file_label.display() + ), + )), + Some(_) => Ok(()), + None if require_present => Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "agent role file at {} must define `developer_instructions`", + role_file_label.display() + ), + )), + None => Ok(()), + } +} + +fn validate_agent_role_config_file( + role_name: &str, + config_file: Option<&Path>, +) -> std::io::Result<()> { + let Some(config_file) = config_file else { + return Ok(()); + }; + + let metadata = std::fs::metadata(config_file).map_err(|e| { + std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "agents.{role_name}.config_file must point to an existing file at {}: {e}", + config_file.display() + ), + ) + })?; + if metadata.is_file() { + Ok(()) + } else { + Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "agents.{role_name}.config_file must point to a file: {}", + config_file.display() + ), + )) + } +} + +fn normalize_agent_role_nickname_candidates( + field_label: &str, + nickname_candidates: Option<&[String]>, +) -> std::io::Result>> { + let Some(nickname_candidates) = nickname_candidates else { + return Ok(None); + }; + + if nickname_candidates.is_empty() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!("{field_label} must contain at least one name"), + )); + } + + let mut normalized_candidates = Vec::with_capacity(nickname_candidates.len()); + let mut seen_candidates = BTreeSet::new(); + + for nickname in nickname_candidates { + let normalized_nickname = nickname.trim(); + if normalized_nickname.is_empty() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!("{field_label} cannot contain blank names"), + )); + } + + if !seen_candidates.insert(normalized_nickname.to_owned()) { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!("{field_label} cannot contain duplicates"), + )); + } + + if !normalized_nickname + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, ' ' | '-' | '_')) + { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "{field_label} may only contain ASCII letters, digits, spaces, hyphens, and underscores" + ), + )); + } + + normalized_candidates.push(normalized_nickname.to_owned()); + } + + Ok(Some(normalized_candidates)) +} + +fn discover_agent_roles_in_dir( + agents_dir: &Path, + declared_role_files: &BTreeSet, +) -> std::io::Result> { + let mut roles = BTreeMap::new(); + + for agent_file in collect_agent_role_files(agents_dir)? { + if declared_role_files.contains(&agent_file) { + continue; + } + let parsed_file = read_resolved_agent_role_file(&agent_file, None)?; + let role_name = parsed_file.role_name; + if roles + .insert( + role_name.clone(), + AgentRoleConfig { + description: parsed_file.description, + config_file: Some(agent_file), + nickname_candidates: parsed_file.nickname_candidates, + }, + ) + .is_some() + { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "duplicate agent role name `{role_name}` discovered in {}", + agents_dir.display() + ), + )); + } + } + + Ok(roles) +} + +fn collect_agent_role_files(dir: &Path) -> std::io::Result> { + let mut files = Vec::new(); + collect_agent_role_files_recursive(dir, &mut files)?; + files.sort(); + Ok(files) +} + +fn collect_agent_role_files_recursive(dir: &Path, files: &mut Vec) -> std::io::Result<()> { + let read_dir = match std::fs::read_dir(dir) { + Ok(read_dir) => read_dir, + Err(err) if err.kind() == ErrorKind::NotFound => return Ok(()), + Err(err) => return Err(err), + }; + + for entry in read_dir { + let entry = entry?; + let path = entry.path(); + let file_type = entry.file_type()?; + if file_type.is_dir() { + collect_agent_role_files_recursive(&path, files)?; + continue; + } + if file_type.is_file() + && path + .extension() + .is_some_and(|extension| extension == "toml") + { + files.push(path); + } + } + + Ok(()) +} diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 27126923a..f15ec4dd5 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -2809,7 +2809,11 @@ async fn agent_role_relative_config_file_resolves_against_config_toml() -> std:: .expect("role config should have a parent directory"), ) .await?; - tokio::fs::write(&role_config_path, "model = \"gpt-5\"").await?; + tokio::fs::write( + &role_config_path, + "developer_instructions = \"Research carefully\"\nmodel = \"gpt-5\"", + ) + .await?; tokio::fs::write( codex_home.path().join(CONFIG_TOML_FILE), r#"[agents.researcher] @@ -2844,6 +2848,737 @@ nickname_candidates = ["Hypatia", "Noether"] Ok(()) } +#[tokio::test] +async fn agent_role_file_metadata_overrides_config_toml_metadata() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let role_config_path = codex_home.path().join("agents").join("researcher.toml"); + tokio::fs::create_dir_all( + role_config_path + .parent() + .expect("role config should have a parent directory"), + ) + .await?; + tokio::fs::write( + &role_config_path, + r#" +description = "Role metadata from file" +nickname_candidates = ["Hypatia"] +developer_instructions = "Research carefully" +model = "gpt-5" +"#, + ) + .await?; + tokio::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#"[agents.researcher] +description = "Research role from config" +config_file = "./agents/researcher.toml" +nickname_candidates = ["Noether"] +"#, + ) + .await?; + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .build() + .await?; + let role = config + .agent_roles + .get("researcher") + .expect("researcher role should load"); + assert_eq!(role.description.as_deref(), Some("Role metadata from file")); + assert_eq!(role.config_file.as_ref(), Some(&role_config_path)); + assert_eq!( + role.nickname_candidates + .as_ref() + .map(|candidates| candidates.iter().map(String::as_str).collect::>()), + Some(vec!["Hypatia"]) + ); + + Ok(()) +} + +#[tokio::test] +async fn agent_role_file_requires_developer_instructions() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let repo_root = TempDir::new()?; + let nested_cwd = repo_root.path().join("packages").join("app"); + std::fs::create_dir_all(repo_root.path().join(".git"))?; + std::fs::create_dir_all(&nested_cwd)?; + + let workspace_key = repo_root.path().to_string_lossy().replace('\\', "\\\\"); + tokio::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + format!( + r#"[projects."{workspace_key}"] +trust_level = "trusted" +"# + ), + ) + .await?; + + let standalone_agents_dir = repo_root.path().join(".codex").join("agents"); + tokio::fs::create_dir_all(&standalone_agents_dir).await?; + tokio::fs::write( + standalone_agents_dir.join("researcher.toml"), + r#" +name = "researcher" +description = "Role metadata from file" +model = "gpt-5" +"#, + ) + .await?; + + let err = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + cwd: Some(nested_cwd), + ..Default::default() + }) + .build() + .await + .expect_err("agent role file without developer instructions should fail"); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput); + assert!( + err.to_string() + .contains("must define `developer_instructions`") + ); + + Ok(()) +} + +#[tokio::test] +async fn legacy_agent_role_config_file_allows_missing_developer_instructions() -> std::io::Result<()> +{ + let codex_home = TempDir::new()?; + let role_config_path = codex_home.path().join("agents").join("researcher.toml"); + tokio::fs::create_dir_all( + role_config_path + .parent() + .expect("role config should have a parent directory"), + ) + .await?; + tokio::fs::write( + &role_config_path, + r#" +model = "gpt-5" +model_reasoning_effort = "high" +"#, + ) + .await?; + tokio::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#"[agents.researcher] +description = "Research role from config" +config_file = "./agents/researcher.toml" +"#, + ) + .await?; + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .build() + .await?; + assert_eq!( + config + .agent_roles + .get("researcher") + .and_then(|role| role.description.as_deref()), + Some("Research role from config") + ); + assert_eq!( + config + .agent_roles + .get("researcher") + .and_then(|role| role.config_file.as_ref()), + Some(&role_config_path) + ); + + Ok(()) +} + +#[tokio::test] +async fn agent_role_requires_description_after_merge() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let role_config_path = codex_home.path().join("agents").join("researcher.toml"); + tokio::fs::create_dir_all( + role_config_path + .parent() + .expect("role config should have a parent directory"), + ) + .await?; + tokio::fs::write( + &role_config_path, + r#" +developer_instructions = "Research carefully" +model = "gpt-5" +"#, + ) + .await?; + tokio::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#"[agents.researcher] +config_file = "./agents/researcher.toml" +"#, + ) + .await?; + + let err = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .build() + .await + .expect_err("agent role without description should fail"); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput); + assert!( + err.to_string() + .contains("agent role `researcher` must define a description") + ); + + Ok(()) +} + +#[tokio::test] +async fn discovered_agent_role_file_requires_name() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let repo_root = TempDir::new()?; + let nested_cwd = repo_root.path().join("packages").join("app"); + std::fs::create_dir_all(repo_root.path().join(".git"))?; + std::fs::create_dir_all(&nested_cwd)?; + + let workspace_key = repo_root.path().to_string_lossy().replace('\\', "\\\\"); + tokio::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + format!( + r#"[projects."{workspace_key}"] +trust_level = "trusted" +"# + ), + ) + .await?; + + let standalone_agents_dir = repo_root.path().join(".codex").join("agents"); + tokio::fs::create_dir_all(&standalone_agents_dir).await?; + tokio::fs::write( + standalone_agents_dir.join("researcher.toml"), + r#" +description = "Role metadata from file" +developer_instructions = "Research carefully" +"#, + ) + .await?; + + let err = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + cwd: Some(nested_cwd), + ..Default::default() + }) + .build() + .await + .expect_err("discovered agent role file without name should fail"); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput); + assert!(err.to_string().contains("must define a non-empty `name`")); + + Ok(()) +} + +#[tokio::test] +async fn agent_role_file_name_takes_precedence_over_config_key() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let role_config_path = codex_home.path().join("agents").join("researcher.toml"); + tokio::fs::create_dir_all( + role_config_path + .parent() + .expect("role config should have a parent directory"), + ) + .await?; + tokio::fs::write( + &role_config_path, + r#" +name = "archivist" +description = "Role metadata from file" +developer_instructions = "Research carefully" +model = "gpt-5" +"#, + ) + .await?; + tokio::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#"[agents.researcher] +description = "Research role from config" +config_file = "./agents/researcher.toml" +"#, + ) + .await?; + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .build() + .await?; + assert_eq!(config.agent_roles.contains_key("researcher"), false); + let role = config + .agent_roles + .get("archivist") + .expect("role should use file-provided name"); + assert_eq!(role.description.as_deref(), Some("Role metadata from file")); + assert_eq!(role.config_file.as_ref(), Some(&role_config_path)); + + Ok(()) +} + +#[tokio::test] +async fn loads_legacy_split_agent_roles_from_config_toml() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let researcher_path = codex_home.path().join("agents").join("researcher.toml"); + let reviewer_path = codex_home.path().join("agents").join("reviewer.toml"); + tokio::fs::create_dir_all( + researcher_path + .parent() + .expect("role config should have a parent directory"), + ) + .await?; + tokio::fs::write( + &researcher_path, + "developer_instructions = \"Research carefully\"\nmodel = \"gpt-5\"", + ) + .await?; + tokio::fs::write( + &reviewer_path, + "developer_instructions = \"Review carefully\"\nmodel = \"gpt-4.1\"", + ) + .await?; + tokio::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#"[agents.researcher] +description = "Research role" +config_file = "./agents/researcher.toml" +nickname_candidates = ["Hypatia", "Noether"] + +[agents.reviewer] +description = "Review role" +config_file = "./agents/reviewer.toml" +nickname_candidates = ["Atlas"] +"#, + ) + .await?; + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .build() + .await?; + + assert_eq!( + config + .agent_roles + .get("researcher") + .and_then(|role| role.description.as_deref()), + Some("Research role") + ); + assert_eq!( + config + .agent_roles + .get("researcher") + .and_then(|role| role.config_file.as_ref()), + Some(&researcher_path) + ); + assert_eq!( + config + .agent_roles + .get("researcher") + .and_then(|role| role.nickname_candidates.as_ref()) + .map(|candidates| candidates.iter().map(String::as_str).collect::>()), + Some(vec!["Hypatia", "Noether"]) + ); + assert_eq!( + config + .agent_roles + .get("reviewer") + .and_then(|role| role.description.as_deref()), + Some("Review role") + ); + assert_eq!( + config + .agent_roles + .get("reviewer") + .and_then(|role| role.config_file.as_ref()), + Some(&reviewer_path) + ); + assert_eq!( + config + .agent_roles + .get("reviewer") + .and_then(|role| role.nickname_candidates.as_ref()) + .map(|candidates| candidates.iter().map(String::as_str).collect::>()), + Some(vec!["Atlas"]) + ); + + Ok(()) +} + +#[tokio::test] +async fn discovers_multiple_standalone_agent_role_files() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let repo_root = TempDir::new()?; + let nested_cwd = repo_root.path().join("packages").join("app"); + std::fs::create_dir_all(repo_root.path().join(".git"))?; + std::fs::create_dir_all(&nested_cwd)?; + + let workspace_key = repo_root.path().to_string_lossy().replace('\\', "\\\\"); + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + format!( + r#"[projects."{workspace_key}"] +trust_level = "trusted" +"# + ), + )?; + + let root_agent = repo_root + .path() + .join(".codex") + .join("agents") + .join("root.toml"); + std::fs::create_dir_all( + root_agent + .parent() + .expect("root agent should have a parent directory"), + )?; + std::fs::write( + &root_agent, + r#" +name = "researcher" +description = "from root" +developer_instructions = "Research carefully" +"#, + )?; + + let nested_agent = repo_root + .path() + .join("packages") + .join(".codex") + .join("agents") + .join("review") + .join("nested.toml"); + std::fs::create_dir_all( + nested_agent + .parent() + .expect("nested agent should have a parent directory"), + )?; + std::fs::write( + &nested_agent, + r#" +name = "reviewer" +description = "from nested" +nickname_candidates = ["Atlas"] +developer_instructions = "Review carefully" +"#, + )?; + + let sibling_agent = repo_root + .path() + .join("packages") + .join(".codex") + .join("agents") + .join("writer.toml"); + std::fs::create_dir_all( + sibling_agent + .parent() + .expect("sibling agent should have a parent directory"), + )?; + std::fs::write( + &sibling_agent, + r#" +name = "writer" +description = "from sibling" +nickname_candidates = ["Sagan"] +developer_instructions = "Write carefully" +"#, + )?; + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + cwd: Some(nested_cwd), + ..Default::default() + }) + .build() + .await?; + + assert_eq!( + config + .agent_roles + .get("researcher") + .and_then(|role| role.description.as_deref()), + Some("from root") + ); + assert_eq!( + config + .agent_roles + .get("reviewer") + .and_then(|role| role.description.as_deref()), + Some("from nested") + ); + assert_eq!( + config + .agent_roles + .get("reviewer") + .and_then(|role| role.nickname_candidates.as_ref()) + .map(|candidates| candidates.iter().map(String::as_str).collect::>()), + Some(vec!["Atlas"]) + ); + assert_eq!( + config + .agent_roles + .get("writer") + .and_then(|role| role.description.as_deref()), + Some("from sibling") + ); + assert_eq!( + config + .agent_roles + .get("writer") + .and_then(|role| role.nickname_candidates.as_ref()) + .map(|candidates| candidates.iter().map(String::as_str).collect::>()), + Some(vec!["Sagan"]) + ); + + Ok(()) +} + +#[tokio::test] +async fn mixed_legacy_and_standalone_agent_role_sources_merge_with_precedence() +-> std::io::Result<()> { + let codex_home = TempDir::new()?; + let repo_root = TempDir::new()?; + let nested_cwd = repo_root.path().join("packages").join("app"); + std::fs::create_dir_all(repo_root.path().join(".git"))?; + std::fs::create_dir_all(&nested_cwd)?; + + let workspace_key = repo_root.path().to_string_lossy().replace('\\', "\\\\"); + tokio::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + format!( + r#"[projects."{workspace_key}"] +trust_level = "trusted" + +[agents.researcher] +description = "Research role from config" +config_file = "./agents/researcher.toml" +nickname_candidates = ["Noether"] + +[agents.critic] +description = "Critic role from config" +config_file = "./agents/critic.toml" +nickname_candidates = ["Ada"] +"# + ), + ) + .await?; + + let home_agents_dir = codex_home.path().join("agents"); + tokio::fs::create_dir_all(&home_agents_dir).await?; + tokio::fs::write( + home_agents_dir.join("researcher.toml"), + r#" +developer_instructions = "Research carefully" +model = "gpt-5" +"#, + ) + .await?; + tokio::fs::write( + home_agents_dir.join("critic.toml"), + r#" +developer_instructions = "Critique carefully" +model = "gpt-4.1" +"#, + ) + .await?; + + let standalone_agents_dir = repo_root.path().join(".codex").join("agents"); + tokio::fs::create_dir_all(&standalone_agents_dir).await?; + tokio::fs::write( + standalone_agents_dir.join("researcher.toml"), + r#" +name = "researcher" +description = "Research role from file" +nickname_candidates = ["Hypatia"] +developer_instructions = "Research from file" +model = "gpt-5-mini" +"#, + ) + .await?; + tokio::fs::write( + standalone_agents_dir.join("writer.toml"), + r#" +name = "writer" +description = "Writer role from file" +nickname_candidates = ["Sagan"] +developer_instructions = "Write carefully" +model = "gpt-5" +"#, + ) + .await?; + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + cwd: Some(nested_cwd), + ..Default::default() + }) + .build() + .await?; + + assert_eq!( + config + .agent_roles + .get("researcher") + .and_then(|role| role.description.as_deref()), + Some("Research role from file") + ); + assert_eq!( + config + .agent_roles + .get("researcher") + .and_then(|role| role.config_file.as_ref()), + Some(&standalone_agents_dir.join("researcher.toml")) + ); + assert_eq!( + config + .agent_roles + .get("researcher") + .and_then(|role| role.nickname_candidates.as_ref()) + .map(|candidates| candidates.iter().map(String::as_str).collect::>()), + Some(vec!["Hypatia"]) + ); + assert_eq!( + config + .agent_roles + .get("critic") + .and_then(|role| role.description.as_deref()), + Some("Critic role from config") + ); + assert_eq!( + config + .agent_roles + .get("critic") + .and_then(|role| role.config_file.as_ref()), + Some(&home_agents_dir.join("critic.toml")) + ); + assert_eq!( + config + .agent_roles + .get("critic") + .and_then(|role| role.nickname_candidates.as_ref()) + .map(|candidates| candidates.iter().map(String::as_str).collect::>()), + Some(vec!["Ada"]) + ); + assert_eq!( + config + .agent_roles + .get("writer") + .and_then(|role| role.description.as_deref()), + Some("Writer role from file") + ); + assert_eq!( + config + .agent_roles + .get("writer") + .and_then(|role| role.nickname_candidates.as_ref()) + .map(|candidates| candidates.iter().map(String::as_str).collect::>()), + Some(vec!["Sagan"]) + ); + + Ok(()) +} + +#[tokio::test] +async fn higher_precedence_agent_role_can_inherit_description_from_lower_layer() +-> std::io::Result<()> { + let codex_home = TempDir::new()?; + let repo_root = TempDir::new()?; + let nested_cwd = repo_root.path().join("packages").join("app"); + std::fs::create_dir_all(repo_root.path().join(".git"))?; + std::fs::create_dir_all(&nested_cwd)?; + + let workspace_key = repo_root.path().to_string_lossy().replace('\\', "\\\\"); + tokio::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + format!( + r#"[projects."{workspace_key}"] +trust_level = "trusted" + +[agents.researcher] +description = "Research role from config" +config_file = "./agents/researcher.toml" +"# + ), + ) + .await?; + + let home_agents_dir = codex_home.path().join("agents"); + tokio::fs::create_dir_all(&home_agents_dir).await?; + tokio::fs::write( + home_agents_dir.join("researcher.toml"), + r#" +developer_instructions = "Research carefully" +model = "gpt-5" +"#, + ) + .await?; + + let standalone_agents_dir = repo_root.path().join(".codex").join("agents"); + tokio::fs::create_dir_all(&standalone_agents_dir).await?; + tokio::fs::write( + standalone_agents_dir.join("researcher.toml"), + r#" +name = "researcher" +nickname_candidates = ["Hypatia"] +developer_instructions = "Research from file" +model = "gpt-5-mini" +"#, + ) + .await?; + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + cwd: Some(nested_cwd), + ..Default::default() + }) + .build() + .await?; + + assert_eq!( + config + .agent_roles + .get("researcher") + .and_then(|role| role.description.as_deref()), + Some("Research role from config") + ); + assert_eq!( + config + .agent_roles + .get("researcher") + .and_then(|role| role.config_file.as_ref()), + Some(&standalone_agents_dir.join("researcher.toml")) + ); + assert_eq!( + config + .agent_roles + .get("researcher") + .and_then(|role| role.nickname_candidates.as_ref()) + .map(|candidates| candidates.iter().map(String::as_str).collect::>()), + Some(vec!["Hypatia"]) + ); + + Ok(()) +} + #[test] fn load_config_normalizes_agent_role_nickname_candidates() -> std::io::Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 624fef72d..697f50d7c 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -85,7 +85,6 @@ use serde::Deserialize; use serde::Serialize; use similar::DiffableStr; use std::collections::BTreeMap; -use std::collections::BTreeSet; use std::collections::HashMap; use std::io::ErrorKind; use std::path::Path; @@ -98,6 +97,7 @@ use codex_network_proxy::NetworkProxyConfig; use toml::Value as TomlValue; use toml_edit::DocumentMut; +pub(crate) mod agent_roles; pub mod edit; mod managed_features; mod network_proxy_spec; @@ -1423,6 +1423,7 @@ pub struct AgentsToml { #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct AgentRoleConfig { /// Human-facing role documentation used in spawn tool guidance. + /// Required for loaded user-defined roles after deprecated/new metadata precedence resolves. pub description: Option, /// Path to a role-specific config layer. pub config_file: Option, @@ -1434,6 +1435,7 @@ pub struct AgentRoleConfig { #[schemars(deny_unknown_fields)] pub struct AgentRoleToml { /// Human-facing role documentation used in spawn tool guidance. + /// Required unless supplied by the referenced agent role file. pub description: Option, /// Path to a role-specific config layer. @@ -2046,6 +2048,8 @@ impl Config { .unwrap_or(WebSearchMode::Cached); let web_search_config = resolve_web_search_config(&cfg, &config_profile); + let agent_roles = agent_roles::load_agent_roles(&cfg, &config_layer_stack)?; + let mut model_providers = built_in_model_providers(); // Merge user-defined providers into the built-in list. for (key, provider) in cfg.model_providers.into_iter() { @@ -2095,34 +2099,6 @@ impl Config { "agents.max_depth must be at least 1", )); } - let agent_roles = cfg - .agents - .as_ref() - .map(|agents| { - agents - .roles - .iter() - .map(|(name, role)| { - let config_file = - role.config_file.as_ref().map(AbsolutePathBuf::to_path_buf); - Self::validate_agent_role_config_file(name, config_file.as_deref())?; - let nickname_candidates = Self::normalize_agent_role_nickname_candidates( - name, - role.nickname_candidates.as_deref(), - )?; - Ok(( - name.clone(), - AgentRoleConfig { - description: role.description.clone(), - config_file, - nickname_candidates, - }, - )) - }) - .collect::>>() - }) - .transpose()? - .unwrap_or_default(); let agent_job_max_runtime_seconds = cfg .agents .as_ref() @@ -2567,88 +2543,6 @@ impl Config { } } - fn validate_agent_role_config_file( - role_name: &str, - config_file: Option<&Path>, - ) -> std::io::Result<()> { - let Some(config_file) = config_file else { - return Ok(()); - }; - - let metadata = std::fs::metadata(config_file).map_err(|e| { - std::io::Error::new( - std::io::ErrorKind::InvalidInput, - format!( - "agents.{role_name}.config_file must point to an existing file at {}: {e}", - config_file.display() - ), - ) - })?; - if metadata.is_file() { - Ok(()) - } else { - Err(std::io::Error::new( - std::io::ErrorKind::InvalidInput, - format!( - "agents.{role_name}.config_file must point to a file: {}", - config_file.display() - ), - )) - } - } - - fn normalize_agent_role_nickname_candidates( - role_name: &str, - nickname_candidates: Option<&[String]>, - ) -> std::io::Result>> { - let Some(nickname_candidates) = nickname_candidates else { - return Ok(None); - }; - - if nickname_candidates.is_empty() { - return Err(std::io::Error::new( - std::io::ErrorKind::InvalidInput, - format!("agents.{role_name}.nickname_candidates must contain at least one name"), - )); - } - - let mut normalized_candidates = Vec::with_capacity(nickname_candidates.len()); - let mut seen_candidates = BTreeSet::new(); - - for nickname in nickname_candidates { - let normalized_nickname = nickname.trim(); - if normalized_nickname.is_empty() { - return Err(std::io::Error::new( - std::io::ErrorKind::InvalidInput, - format!("agents.{role_name}.nickname_candidates cannot contain blank names"), - )); - } - - if !seen_candidates.insert(normalized_nickname.to_owned()) { - return Err(std::io::Error::new( - std::io::ErrorKind::InvalidInput, - format!("agents.{role_name}.nickname_candidates cannot contain duplicates"), - )); - } - - if !normalized_nickname - .chars() - .all(|c| c.is_ascii_alphanumeric() || matches!(c, ' ' | '-' | '_')) - { - return Err(std::io::Error::new( - std::io::ErrorKind::InvalidInput, - format!( - "agents.{role_name}.nickname_candidates may only contain ASCII letters, digits, spaces, hyphens, and underscores" - ), - )); - } - - normalized_candidates.push(normalized_nickname.to_owned()); - } - - Ok(Some(normalized_candidates)) - } - pub fn set_windows_sandbox_enabled(&mut self, value: bool) { self.permissions.windows_sandbox_mode = if value { Some(WindowsSandboxModeToml::Unelevated)