From 0332a6dd4c1f8134a7906363eee040d4abc31cfd Mon Sep 17 00:00:00 2001 From: charlesgong-openai Date: Wed, 17 Jun 2026 13:53:18 -0700 Subject: [PATCH] Validate external agent skills before import --- .../src/config/external_agent_config.rs | 57 ++++++++++---- .../src/config/external_agent_config_tests.rs | 75 +++++++++++++++++-- 2 files changed, 111 insertions(+), 21 deletions(-) diff --git a/codex-rs/app-server/src/config/external_agent_config.rs b/codex-rs/app-server/src/config/external_agent_config.rs index ce10d6e81c..8b47f661b1 100644 --- a/codex-rs/app-server/src/config/external_agent_config.rs +++ b/codex-rs/app-server/src/config/external_agent_config.rs @@ -1,6 +1,8 @@ use codex_config::types::PluginConfig; use codex_core::config::Config; use codex_core::config::ConfigBuilder; +use codex_core::skills::loader::SkillRoot; +use codex_core::skills::loader::load_skills_from_roots; use codex_core_plugins::PluginInstallRequest; use codex_core_plugins::PluginsManager; use codex_core_plugins::marketplace::MarketplacePluginInstallPolicy; @@ -8,6 +10,7 @@ use codex_core_plugins::marketplace::find_marketplace_manifest_path; use codex_core_plugins::marketplace_add::MarketplaceAddRequest; use codex_core_plugins::marketplace_add::add_marketplace; use codex_core_plugins::marketplace_add::is_local_marketplace_source; +use codex_exec_server::LOCAL_FS; use codex_external_agent_migration::build_mcp_config_from_external; use codex_external_agent_migration::count_missing_commands; use codex_external_agent_migration::count_missing_subagents; @@ -21,6 +24,8 @@ use codex_external_agent_sessions::ExternalAgentSessionMigration; use codex_external_agent_sessions::detect_recent_sessions; use codex_plugin::PluginId; use codex_protocol::protocol::Product; +use codex_protocol::protocol::SkillScope; +use codex_utils_absolute_path::AbsolutePathBuf; use serde_json::Value as JsonValue; use std::collections::BTreeMap; use std::collections::HashMap; @@ -30,6 +35,7 @@ use std::fs; use std::io; use std::path::Path; use std::path::PathBuf; +use std::sync::Arc; use toml::Value as TomlValue; const EXTERNAL_AGENT_CONFIG_DETECT_METRIC: &str = "codex.external_agent_config.detect"; @@ -262,18 +268,19 @@ impl ExternalAgentConfigService { ); Ok(()) })(), - ExternalAgentConfigMigrationItemType::Skills => (|| { - let imported_skills = self.import_skills(migration_item.cwd.as_deref())?; - emit_migration_metric( - EXTERNAL_AGENT_CONFIG_IMPORT_METRIC, - ExternalAgentConfigMigrationItemType::Skills, - Some(imported_skills.len()), - ); - for skill_name in imported_skills { - item_result.record_success(Some(skill_name.clone()), Some(skill_name)); - } - Ok(()) - })(), + ExternalAgentConfigMigrationItemType::Skills => self + .import_skills(migration_item.cwd.as_deref()) + .await + .map(|imported_skills| { + emit_migration_metric( + EXTERNAL_AGENT_CONFIG_IMPORT_METRIC, + ExternalAgentConfigMigrationItemType::Skills, + Some(imported_skills.len()), + ); + for skill_name in imported_skills { + item_result.record_success(Some(skill_name.clone()), Some(skill_name)); + } + }), ExternalAgentConfigMigrationItemType::AgentsMd => (|| { if let Some((source, target)) = self.import_agents_md(migration_item.cwd.as_deref())? @@ -1149,7 +1156,7 @@ impl ExternalAgentConfigService { import_commands(&source_commands, &target_skills) } - fn import_skills(&self, cwd: Option<&Path>) -> io::Result> { + async fn import_skills(&self, cwd: Option<&Path>) -> io::Result> { let (source_skills, target_skills) = if let Some(repo_root) = find_repo_root(cwd)? { ( repo_root.join(EXTERNAL_AGENT_DIR).join("skills"), @@ -1166,6 +1173,30 @@ impl ExternalAgentConfigService { if !source_skills.is_dir() { return Ok(Vec::new()); } + let source_skills_root = AbsolutePathBuf::from_absolute_path_checked(&source_skills) + .map_err(|err| { + invalid_data_error(format!( + "invalid skills root {}: {err}", + source_skills.display() + )) + })?; + let skill_load_outcome = load_skills_from_roots([SkillRoot { + path: source_skills_root, + scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), + plugin_id: None, + plugin_root: None, + }]) + .await; + if !skill_load_outcome.errors.is_empty() { + let details = skill_load_outcome + .errors + .iter() + .map(|error| format!("{}: {}", error.path.display(), error.message)) + .collect::>() + .join("; "); + return Err(invalid_data_error(format!("invalid skills: {details}"))); + } fs::create_dir_all(&target_skills)?; let mut copied_names = Vec::new(); diff --git a/codex-rs/app-server/src/config/external_agent_config_tests.rs b/codex-rs/app-server/src/config/external_agent_config_tests.rs index 447741d14a..c7690e9358 100644 --- a/codex-rs/app-server/src/config/external_agent_config_tests.rs +++ b/codex-rs/app-server/src/config/external_agent_config_tests.rs @@ -805,7 +805,7 @@ async fn import_home_migrates_supported_config_fields_skills_and_agents_md() { .join("skill-a") .join("SKILL.md"), format!( - "Use {SOURCE_EXTERNAL_AGENT_PRODUCT_NAME} and {SOURCE_EXTERNAL_AGENT_UPPER_NAME} utilities." + "---\nname: skill-a\ndescription: Source skill\n---\nUse {SOURCE_EXTERNAL_AGENT_PRODUCT_NAME} and {SOURCE_EXTERNAL_AGENT_UPPER_NAME} utilities.\n" ), ) .expect("write skill"); @@ -865,7 +865,7 @@ MY_TEAM = "codex" assert_eq!( fs::read_to_string(agents_skills.join("skill-a").join("SKILL.md")) .expect("read copied skill"), - "Use Codex and Codex utilities." + "---\nname: skill-a\ndescription: Source skill\n---\nUse Codex and Codex utilities.\n" ); } @@ -2769,22 +2769,81 @@ async fn import_plugins_supports_project_relative_external_agent_plugin_marketpl assert!(config.contains("enabled = true")); } -#[test] -fn import_skills_returns_only_new_skill_directory_names() { +#[tokio::test] +async fn import_skills_returns_only_new_skill_directory_names() { let (_root, external_agent_home, codex_home) = fixture_paths(); let agents_skills = codex_home .parent() .map(|parent| parent.join(".agents").join("skills")) .unwrap_or_else(|| PathBuf::from(".agents").join("skills")); - fs::create_dir_all(external_agent_home.join("skills").join("skill-a")) - .expect("create source a"); - fs::create_dir_all(external_agent_home.join("skills").join("skill-b")) - .expect("create source b"); + let source_skill_a = external_agent_home.join("skills").join("skill-a"); + let source_skill_b = external_agent_home.join("skills").join("skill-b"); + fs::create_dir_all(&source_skill_a).expect("create source a"); + fs::create_dir_all(&source_skill_b).expect("create source b"); + fs::write( + source_skill_a.join("SKILL.md"), + "---\nname: skill-a\ndescription: source a\n---\n", + ) + .expect("write source a skill"); + fs::write( + source_skill_b.join("SKILL.md"), + "---\nname: skill-b\ndescription: source b\n---\n", + ) + .expect("write source b skill"); fs::create_dir_all(agents_skills.join("skill-a")).expect("create existing target"); let copied_names = service_for_paths(external_agent_home, codex_home) .import_skills(/*cwd*/ None) + .await .expect("import skills"); assert_eq!(copied_names, vec!["skill-b".to_string()]); } + +#[tokio::test] +async fn import_skills_rejects_invalid_skill_without_copying() { + let (_root, external_agent_home, codex_home) = fixture_paths(); + let source_skill = external_agent_home.join("skills").join("broken-skill"); + let agents_skills = codex_home + .parent() + .map(|parent| parent.join(".agents").join("skills")) + .unwrap_or_else(|| PathBuf::from(".agents").join("skills")); + fs::create_dir_all(&source_skill).expect("create source skill"); + fs::write(source_skill.join("SKILL.md"), "missing frontmatter\n").expect("write invalid skill"); + + let outcome = service_for_paths(external_agent_home, codex_home) + .import(vec![ExternalAgentConfigMigrationItem { + item_type: ExternalAgentConfigMigrationItemType::Skills, + description: "Import skills".to_string(), + cwd: None, + details: None, + }]) + .await; + + assert_eq!(outcome.pending_plugin_imports, Vec::new()); + let item_result = outcome + .item_results + .into_iter() + .next() + .expect("skills import result"); + assert_eq!( + item_result.item_type, + ExternalAgentConfigMigrationItemType::Skills + ); + assert_eq!(item_result.success_count, 0); + assert_eq!(item_result.error_count, 1); + assert_eq!(item_result.successes, Vec::new()); + assert_eq!(item_result.raw_errors.len(), 1); + let raw_error = &item_result.raw_errors[0]; + assert_eq!(raw_error.failure_stage, "import_request_failed"); + assert_eq!( + raw_error.error_type.as_deref(), + Some("external_agent_config_import_error") + ); + assert!( + raw_error + .message + .contains("missing YAML frontmatter delimited by ---") + ); + assert!(!agents_skills.join("broken-skill").exists()); +}