From 4fa7d6f444b919afb6ccec25e49c036aa0180971 Mon Sep 17 00:00:00 2001 From: gabec-openai Date: Thu, 12 Mar 2026 11:20:31 -0700 Subject: [PATCH] Handle malformed agent role definitions nonfatally (#14488) ## Summary - make malformed agent role definitions nonfatal during config loading - drop invalid agent roles and record warnings in `startup_warnings` - forward startup warnings through app-server `configWarning` notifications ## Testing - `cargo test -p codex-core agent_role_ -- --nocapture` - `just fix -p codex-core` - `just fmt` - `cargo test -p codex-app-server config_warning -- --nocapture` Co-authored-by: Codex --- codex-rs/app-server/src/lib.rs | 8 ++ codex-rs/core/src/config/agent_roles.rs | 106 +++++++++++++++++------ codex-rs/core/src/config/config_tests.rs | 91 ++++++++++++++----- codex-rs/core/src/config/mod.rs | 3 +- 4 files changed, 159 insertions(+), 49 deletions(-) diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index ffd8eecbf..3fc6116ba 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -468,6 +468,14 @@ pub async fn run_main_with_transport( if let Some(warning) = project_config_warning(&config) { config_warnings.push(warning); } + for warning in &config.startup_warnings { + config_warnings.push(ConfigWarningNotification { + summary: warning.clone(), + details: None, + path: None, + range: None, + }); + } let feedback = CodexFeedback::new(); diff --git a/codex-rs/core/src/config/agent_roles.rs b/codex-rs/core/src/config/agent_roles.rs index f1ca4c43c..2004f6440 100644 --- a/codex-rs/core/src/config/agent_roles.rs +++ b/codex-rs/core/src/config/agent_roles.rs @@ -17,6 +17,7 @@ use toml::Value as TomlValue; pub(crate) fn load_agent_roles( cfg: &ConfigToml, config_layer_stack: &ConfigLayerStack, + startup_warnings: &mut Vec, ) -> std::io::Result> { let layers = config_layer_stack.get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst, false); @@ -28,20 +29,38 @@ pub(crate) fn load_agent_roles( 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)? { + let agents_toml = match agents_toml_from_layer(&layer.config) { + Ok(agents_toml) => agents_toml, + Err(err) => { + push_agent_role_warning(startup_warnings, err); + None + } + }; + if let Some(agents_toml) = agents_toml { for (declared_role_name, role_toml) in &agents_toml.roles { - let (role_name, role) = read_declared_role(declared_role_name, role_toml)?; + let (role_name, role) = match read_declared_role(declared_role_name, role_toml) { + Ok(role) => role, + Err(err) => { + push_agent_role_warning(startup_warnings, err); + continue; + } + }; 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 layer_roles.contains_key(&role_name) { + push_agent_role_warning( + startup_warnings, + std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "duplicate agent role name `{role_name}` declared in the same config layer" + ), ), - )); + ); + continue; } + layer_roles.insert(role_name, role); } } @@ -49,7 +68,20 @@ pub(crate) fn load_agent_roles( for (role_name, role) in discover_agent_roles_in_dir( config_folder.as_path().join("agents").as_path(), &declared_role_files, + startup_warnings, )? { + if layer_roles.contains_key(&role_name) { + push_agent_role_warning( + startup_warnings, + std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "duplicate agent role name `{role_name}` declared in the same config layer" + ), + ), + ); + continue; + } layer_roles.insert(role_name, role); } } @@ -59,10 +91,13 @@ pub(crate) fn load_agent_roles( if let Some(existing_role) = roles.get(&role_name) { merge_missing_role_fields(&mut merged_role, existing_role); } - validate_required_agent_role_description( + if let Err(err) = validate_required_agent_role_description( &role_name, merged_role.description.as_deref(), - )?; + ) { + push_agent_role_warning(startup_warnings, err); + continue; + } roles.insert(role_name, merged_role); } } @@ -70,6 +105,12 @@ pub(crate) fn load_agent_roles( Ok(roles) } +fn push_agent_role_warning(startup_warnings: &mut Vec, err: std::io::Error) { + let message = format!("Ignoring malformed agent role definition: {err}"); + tracing::warn!("{message}"); + startup_warnings.push(message); +} + fn load_agent_roles_without_layers( cfg: &ConfigToml, ) -> std::io::Result> { @@ -401,6 +442,7 @@ fn normalize_agent_role_nickname_candidates( fn discover_agent_roles_in_dir( agents_dir: &Path, declared_role_files: &BTreeSet, + startup_warnings: &mut Vec, ) -> std::io::Result> { let mut roles = BTreeMap::new(); @@ -408,27 +450,35 @@ fn discover_agent_roles_in_dir( if declared_role_files.contains(&agent_file) { continue; } - let parsed_file = read_resolved_agent_role_file(&agent_file, None)?; + let parsed_file = match read_resolved_agent_role_file(&agent_file, None) { + Ok(parsed_file) => parsed_file, + Err(err) => { + push_agent_role_warning(startup_warnings, err); + continue; + } + }; 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() + if roles.contains_key(&role_name) { + push_agent_role_warning( + startup_warnings, + std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "duplicate agent role name `{role_name}` discovered in {}", + agents_dir.display() + ), ), - )); + ); + continue; } + roles.insert( + role_name, + AgentRoleConfig { + description: parsed_file.description, + config_file: Some(agent_file), + nickname_candidates: parsed_file.nickname_candidates, + }, + ); } Ok(roles) diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index d203ab02c..00e5a1756 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -3010,7 +3010,8 @@ nickname_candidates = ["Noether"] } #[tokio::test] -async fn agent_role_file_requires_developer_instructions() -> std::io::Result<()> { +async fn agent_role_file_without_developer_instructions_is_dropped_with_warning() +-> std::io::Result<()> { let codex_home = TempDir::new()?; let repo_root = TempDir::new()?; let nested_cwd = repo_root.path().join("packages").join("app"); @@ -3036,23 +3037,41 @@ trust_level = "trusted" name = "researcher" description = "Role metadata from file" model = "gpt-5" +"#, + ) + .await?; + tokio::fs::write( + standalone_agents_dir.join("reviewer.toml"), + r#" +name = "reviewer" +description = "Review role" +developer_instructions = "Review carefully" +model = "gpt-5" "#, ) .await?; - let err = ConfigBuilder::default() + let config = 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); + .await?; + assert!(!config.agent_roles.contains_key("researcher")); + assert_eq!( + config + .agent_roles + .get("reviewer") + .and_then(|role| role.description.as_deref()), + Some("Review role") + ); assert!( - err.to_string() - .contains("must define `developer_instructions`") + config + .startup_warnings + .iter() + .any(|warning| warning.contains("must define `developer_instructions`")) ); Ok(()) @@ -3110,7 +3129,8 @@ config_file = "./agents/researcher.toml" } #[tokio::test] -async fn agent_role_requires_description_after_merge() -> std::io::Result<()> { +async fn agent_role_without_description_after_merge_is_dropped_with_warning() -> 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( @@ -3131,27 +3151,38 @@ model = "gpt-5" codex_home.path().join(CONFIG_TOML_FILE), r#"[agents.researcher] config_file = "./agents/researcher.toml" + +[agents.reviewer] +description = "Review role" "#, ) .await?; - let err = ConfigBuilder::default() + let config = 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); + .await?; + assert!(!config.agent_roles.contains_key("researcher")); + assert_eq!( + config + .agent_roles + .get("reviewer") + .and_then(|role| role.description.as_deref()), + Some("Review role") + ); assert!( - err.to_string() - .contains("agent role `researcher` must define a description") + config + .startup_warnings + .iter() + .any(|warning| warning.contains("agent role `researcher` must define a description")) ); Ok(()) } #[tokio::test] -async fn discovered_agent_role_file_requires_name() -> std::io::Result<()> { +async fn discovered_agent_role_file_without_name_is_dropped_with_warning() -> std::io::Result<()> { let codex_home = TempDir::new()?; let repo_root = TempDir::new()?; let nested_cwd = repo_root.path().join("packages").join("app"); @@ -3176,21 +3207,41 @@ trust_level = "trusted" r#" description = "Role metadata from file" developer_instructions = "Research carefully" +"#, + ) + .await?; + tokio::fs::write( + standalone_agents_dir.join("reviewer.toml"), + r#" +name = "reviewer" +description = "Review role" +developer_instructions = "Review carefully" "#, ) .await?; - let err = ConfigBuilder::default() + let config = 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`")); + .await?; + assert!(!config.agent_roles.contains_key("researcher")); + assert_eq!( + config + .agent_roles + .get("reviewer") + .and_then(|role| role.description.as_deref()), + Some("Review role") + ); + assert!( + config + .startup_warnings + .iter() + .any(|warning| warning.contains("must define a non-empty `name`")) + ); Ok(()) } diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index d2b37000a..e4e90fca4 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -2085,7 +2085,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 agent_roles = + agent_roles::load_agent_roles(&cfg, &config_layer_stack, &mut startup_warnings)?; let mut model_providers = built_in_model_providers(); // Merge user-defined providers into the built-in list.