From c5d94319715d2598a2e8b2b7d0e21a3b1e83aec6 Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Fri, 7 Aug 2026 14:29:14 +0000 Subject: [PATCH] Unify plugin skill loading through the shared loader (#37452) ## What changed - Route plugin inventory loading and capability summaries through the injected `SkillRootLoader`. - Preserve recursive discovery for legacy plugins while restricting agent-plugin skill discovery to direct children of the skills root. ## Testing - Verify `plugin/read` excludes nested agent-plugin skills. - Verify installed agent-plugin telemetry reports skills only for portable, direct-child layouts. GitOrigin-RevId: 9e78dd0c70a632d510c780963a6ceb3ad4bc2522 --- .../app-server/tests/suite/v2/plugin_read.rs | 61 +++++++++ codex-rs/core-plugins/src/loader.rs | 120 +----------------- codex-rs/core-plugins/src/manager.rs | 23 +++- codex-rs/core-plugins/src/manager_tests.rs | 88 +++++++++++-- 4 files changed, 158 insertions(+), 134 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/plugin_read.rs b/codex-rs/app-server/tests/suite/v2/plugin_read.rs index bd1e85e50b..faf88e4daa 100644 --- a/codex-rs/app-server/tests/suite/v2/plugin_read.rs +++ b/codex-rs/app-server/tests/suite/v2/plugin_read.rs @@ -1368,6 +1368,67 @@ async fn plugin_read_fails_on_malformed_share_mapping() -> Result<()> { Ok(()) } +#[tokio::test] +async fn plugin_read_agent_plugin_excludes_nested_skills() -> Result<()> { + let codex_home = TempDir::new()?; + let repo_root = TempDir::new()?; + let plugin_root = repo_root.path().join("plugins/demo-plugin"); + write_plugins_enabled_config(&codex_home)?; + write_plugin_marketplace( + repo_root.path(), + "codex-curated", + "demo-plugin", + "./plugins/demo-plugin", + )?; + std::fs::create_dir_all(plugin_root.join("skills/direct"))?; + std::fs::create_dir_all(plugin_root.join("skills/group/nested"))?; + std::fs::write( + plugin_root.join("plugin.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/plugin.schema.json","name":"demo-plugin"}"#, + )?; + let direct_skill_path = plugin_root.join("skills/direct/SKILL.md"); + std::fs::write( + &direct_skill_path, + "---\nname: direct\ndescription: Direct skill\n---\n", + )?; + std::fs::write( + plugin_root.join("skills/group/nested/SKILL.md"), + "---\nname: nested\ndescription: Nested skill\n---\n", + )?; + + let mut mcp = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .without_auto_env() + .build_initialized_with_timeout(DEFAULT_TIMEOUT) + .await?; + let request_id = mcp + .send_plugin_read_request(PluginReadParams { + marketplace_path: Some(AbsolutePathBuf::try_from( + repo_root.path().join(".agents/plugins/marketplace.json"), + )?), + remote_marketplace_name: None, + plugin_name: "demo-plugin".to_string(), + }) + .await?; + let response: PluginReadResponse = + timeout(DEFAULT_TIMEOUT, mcp.read_response(request_id)).await??; + + assert_eq!( + response.plugin.skills, + vec![codex_app_server_protocol::SkillSummary { + name: "demo-plugin:direct".to_string(), + description: "Direct skill".to_string(), + short_description: None, + interface: None, + path: Some(AbsolutePathBuf::try_from(std::fs::canonicalize( + direct_skill_path + )?)?), + enabled: true, + }] + ); + Ok(()) +} + #[tokio::test] async fn plugin_read_returns_plugin_details_with_bundle_contents() -> Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index 414c0791b6..4c399c32f6 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -28,12 +28,8 @@ use codex_config::types::PluginConfig; use codex_config::types::PluginMcpServerConfig; use codex_connectors::parse_plugin_app_config; use codex_connectors::parse_plugin_app_config_value; -use codex_core_skills::PluginSkillSnapshots as LegacyPluginSkillSnapshots; use codex_core_skills::config_rules::resolve_disabled_skill_paths; use codex_core_skills::config_rules::skill_config_rules_from_stack; -use codex_core_skills::loader::SkillRoot; -use codex_core_skills::loader::load_skills_from_roots; -use codex_exec_server::LOCAL_FS; use codex_mcp::parse_agent_plugin_mcp_config; use codex_mcp::parse_plugin_mcp_config; use codex_plugin::AppDeclaration; @@ -45,7 +41,6 @@ use codex_plugin::PluginIdError; use codex_plugin::app_connector_ids_from_declarations; use codex_protocol::auth::AuthMode; use codex_protocol::protocol::Product; -use codex_protocol::protocol::SkillScope; use codex_skills::SkillConfigRules; use codex_skills::SkillMetadata; use codex_skills::SkillRootLoadRequest; @@ -63,9 +58,7 @@ use std::collections::HashSet; use std::fs; use std::path::Path; use std::process::Command; -use std::sync::Arc; use tempfile::TempDir; -use tokio::sync::Semaphore; use tracing::instrument; use tracing::warn; @@ -911,7 +904,7 @@ async fn load_plugin( plugin_id: loaded_plugin_id.as_key(), remote_plugin_id: loaded_plugin.remote_plugin_id.clone(), }; - let resolved_skills = load_plugin_skill_inventory_with_loader( + let resolved_skills = load_plugin_skill_inventory( &plugin_root, &plugin_identity, &manifest, @@ -1019,115 +1012,7 @@ fn contains_enabled_skill( .any(|skill| !disabled_skill_paths.contains(&skill.path_to_skills_md)) } -pub async fn load_plugin_skills( - plugin_root: &AbsolutePathBuf, - plugin_id: &PluginId, - manifest: &PluginManifest, - restriction_product: Option, - skill_config_rules: &SkillConfigRules, - plugin_skill_snapshots: Option<&LegacyPluginSkillSnapshots>, - root_scan_slots: Arc, -) -> ResolvedPluginSkills { - let plugin_identity = PluginIdentity { - plugin_id: plugin_id.as_key(), - remote_plugin_id: None, - }; - load_plugin_skills_with_identity( - plugin_root, - &plugin_identity, - manifest, - restriction_product, - skill_config_rules, - plugin_skill_snapshots, - root_scan_slots, - ) - .await -} - -pub(crate) async fn load_plugin_skills_with_identity( - plugin_root: &AbsolutePathBuf, - plugin_identity: &PluginIdentity, - manifest: &PluginManifest, - restriction_product: Option, - skill_config_rules: &SkillConfigRules, - plugin_skill_snapshots: Option<&LegacyPluginSkillSnapshots>, - root_scan_slots: Arc, -) -> ResolvedPluginSkills { - load_plugin_skill_inventory( - plugin_root, - plugin_identity, - manifest, - PluginManifestFormat::Legacy, - restriction_product, - plugin_skill_snapshots, - root_scan_slots, - ) - .await - .resolve(skill_config_rules) -} - pub(crate) async fn load_plugin_skill_inventory( - plugin_root: &AbsolutePathBuf, - plugin_identity: &PluginIdentity, - manifest: &PluginManifest, - manifest_format: PluginManifestFormat, - restriction_product: Option, - plugin_skill_snapshots: Option<&LegacyPluginSkillSnapshots>, - root_scan_slots: Arc, -) -> PluginSkillInventory { - let discovery_mode = match manifest_format { - PluginManifestFormat::Legacy => SkillDiscoveryMode::Recursive, - PluginManifestFormat::AgentPlugin => SkillDiscoveryMode::DirectChildren, - }; - let roots = plugin_skill_roots(plugin_root, &manifest.paths, manifest_format) - .into_iter() - .map(|path| SkillRoot { - path, - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: Some(plugin_identity.clone()), - plugin_namespace: Some(manifest.name.clone()), - plugin_root: Some(plugin_root.clone()), - discovery_mode, - }) - .collect::>(); - let outcome = load_skills_from_roots(roots, plugin_skill_snapshots, root_scan_slots).await; - let had_errors = !outcome.errors.is_empty(); - let migrated_command_skills = migrated_command_skills_root(plugin_root); - let migrated_command_skills = fs::canonicalize(migrated_command_skills.as_path()) - .ok() - .and_then(|path| AbsolutePathBuf::from_absolute_path_checked(path).ok()) - .unwrap_or(migrated_command_skills); - let skills = outcome - .skills - .into_iter() - .filter(|skill| skill.matches_product_restriction_for_product(restriction_product)) - .collect::>(); - let native_skill_names = skills - .iter() - .filter(|skill| { - !skill - .path_to_skills_md - .as_path() - .starts_with(migrated_command_skills.as_path()) - }) - .map(|skill| skill.name.clone()) - .collect::>(); - let skills = skills - .into_iter() - .filter(|skill| { - !skill - .path_to_skills_md - .as_path() - .starts_with(migrated_command_skills.as_path()) - || !native_skill_names.contains(&skill.name) - }) - .collect::>(); - - PluginSkillInventory { skills, had_errors } -} - -pub(crate) async fn load_plugin_skill_inventory_with_loader( plugin_root: &AbsolutePathBuf, plugin_identity: &PluginIdentity, manifest: &PluginManifest, @@ -1419,6 +1304,7 @@ async fn load_apps_from_paths( pub async fn plugin_capability_summary_from_root( plugin_id: &PluginId, plugin_root: &AbsolutePathBuf, + skill_root_loader: &dyn SkillRootLoader, ) -> Option { let loaded_manifest = load_plugin_manifest_with_format(plugin_root.as_path())?; let manifest_format = loaded_manifest.format; @@ -1441,7 +1327,7 @@ pub async fn plugin_capability_summary_from_root( manifest_format, /*restriction_product*/ None, /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(1)), + skill_root_loader, ) .await .skills diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index 079514cc79..087b846e90 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -76,7 +76,6 @@ use codex_config::types::PluginConfig; use codex_config::types::ToolSuggestDisabledTool; use codex_config::types::ToolSuggestDiscoverableType; use codex_core_skills::config_rules::skill_config_rules_from_stack; -use codex_core_skills::loader::MAX_CONCURRENT_ROOT_SCANS; use codex_hooks::plugin_hook_declarations; use codex_http_client::HttpClientFactory; use codex_login::AuthManager; @@ -443,7 +442,6 @@ pub struct PluginsManager { // Keep the cache auth-independent so auth changes only need to resolve capabilities again. loaded_plugins_cache: RwLock, loaded_plugins_load_semaphore: Semaphore, - skill_root_scan_slots: Arc, skill_root_loader: Arc>, tool_suggest_metadata_cache: ToolSuggestMetadataCache, remote_installed_plugins_cache: RwLock>>, @@ -555,7 +553,6 @@ impl PluginsManager { .0, loaded_plugins_cache: RwLock::new(LoadedPluginsCache::default()), loaded_plugins_load_semaphore: Semaphore::new(/*permits*/ 1), - skill_root_scan_slots: Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), skill_root_loader, tool_suggest_metadata_cache: ToolSuggestMetadataCache::new(), remote_installed_plugins_cache: RwLock::new(None), @@ -931,7 +928,14 @@ impl PluginsManager { ) -> PluginTelemetryMetadata { let mut metadata = self.telemetry_metadata_for_plugin_id(plugin_id); metadata.capability_summary = match self.store.active_plugin_root(plugin_id) { - Some(plugin_root) => plugin_capability_summary_from_root(plugin_id, &plugin_root).await, + Some(plugin_root) => { + plugin_capability_summary_from_root( + plugin_id, + &plugin_root, + self.skill_root_loader.as_ref(), + ) + .await + } None => None, }; metadata @@ -945,7 +949,14 @@ impl PluginsManager { let mut metadata = self.telemetry_metadata_for_plugin_id_with_remote_id(plugin_id, remote_plugin_id); metadata.capability_summary = match self.store.active_plugin_root(plugin_id) { - Some(plugin_root) => plugin_capability_summary_from_root(plugin_id, &plugin_root).await, + Some(plugin_root) => { + plugin_capability_summary_from_root( + plugin_id, + &plugin_root, + self.skill_root_loader.as_ref(), + ) + .await + } None => None, }; metadata @@ -2054,7 +2065,7 @@ impl PluginsManager { manifest_format, self.restriction_product, /*plugin_skill_snapshots*/ None, - Arc::clone(&self.skill_root_scan_slots), + self.skill_root_loader.as_ref(), ) .await .resolve(&skill_config_rules); diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index 4776447665..0c28a217a7 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -6,7 +6,7 @@ use crate::PluginLoadOutcome; use crate::ToolSuggestDiscoverablePlugin; use crate::ToolSuggestPluginDiscoveryInput; use crate::installed_marketplaces::marketplace_install_root; -use crate::loader::load_plugin_skills; +use crate::loader::load_plugin_skill_inventory; use crate::loader::load_plugins_from_layer_stack; use crate::loader::refresh_non_curated_plugin_cache; use crate::loader::refresh_non_curated_plugin_cache_force_reinstall; @@ -1253,6 +1253,62 @@ async fn installed_plugin_telemetry_metadata_collects_capabilities() { ); } +#[tokio::test] +async fn installed_agent_plugin_telemetry_metadata_uses_portable_capabilities() { + for (skill_path, has_skills) in [ + ("skills/direct/SKILL.md", true), + ("skills/group/nested/SKILL.md", false), + ] { + let codex_home = TempDir::new().unwrap(); + let plugin_root = codex_home + .path() + .join("plugins/cache/test/agent-plugin/local"); + write_file( + &plugin_root.join("plugin.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/plugin.schema.json","name":"agent.tools"}"#, + ); + write_file( + &plugin_root.join(skill_path), + "---\nname: portable\ndescription: Portable skill\n---\n", + ); + write_file( + &plugin_root.join("mcp.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/mcp.schema.json","mcpServers":{"portable":{"type":"stdio","command":"echo"}}}"#, + ); + write_file( + &plugin_root.join(".codex-plugin/plugin.json"), + r#"{"apps":"./.app.json"}"#, + ); + write_file( + &plugin_root.join(".app.json"), + r#"{"apps":{"legacy":{"id":"connector_legacy"}}}"#, + ); + let manager = test_plugins_manager(codex_home.path().to_path_buf()); + let plugin_id = PluginId::parse("agent-plugin@test").expect("plugin id should parse"); + + let metadata = manager + .telemetry_metadata_for_installed_plugin(&plugin_id) + .await; + + assert_eq!( + metadata, + PluginTelemetryMetadata { + plugin_id: Some(plugin_id), + remote_plugin_id: None, + capability_summary: Some(PluginCapabilitySummary { + config_name: "agent-plugin@test".to_string(), + display_name: "agent-plugin".to_string(), + plugin_namespace: Some("agent.tools".to_string()), + description: None, + has_skills, + mcp_server_names: vec!["portable".to_string()], + app_connector_ids: Vec::new(), + }), + } + ); + } +} + #[tokio::test] async fn installed_plugin_telemetry_metadata_resolves_persisted_remote_identity() { let codex_home = TempDir::new().unwrap(); @@ -1826,6 +1882,7 @@ async fn plugin_telemetry_metadata_uses_default_mcp_config_path() { let summary = plugin_capability_summary_from_root( &PluginId::parse("sample@test").expect("plugin id should parse"), &plugin_root.abs(), + test_skill_root_loader().as_ref(), ) .await; @@ -1868,6 +1925,7 @@ async fn plugin_capability_summary_uses_manifest_mcp_server_objects() { let summary = plugin_capability_summary_from_root( &PluginId::parse("counter-sample@test").expect("plugin id should parse"), &plugin_root.abs(), + test_skill_root_loader().as_ref(), ) .await; @@ -2171,16 +2229,20 @@ async fn install_plugin_materializes_default_command_skills() { ); let manifest = crate::manifest::load_plugin_manifest(&result.installed_path).unwrap(); - let resolved = load_plugin_skills( + let resolved = load_plugin_skill_inventory( &result.installed_path, - &result.plugin_id, + &PluginIdentity { + plugin_id: result.plugin_id.as_key(), + remote_plugin_id: None, + }, &manifest, + PluginManifestFormat::Legacy, /*restriction_product*/ None, - &SkillConfigRules::default(), /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), + test_skill_root_loader().as_ref(), ) - .await; + .await + .resolve(&SkillConfigRules::default()); assert_eq!( resolved .skills @@ -2292,16 +2354,20 @@ async fn load_plugin_skills_dedupes_overlapping_manifest_roots() { interface: None, }; let plugin_id = PluginId::parse("sample@test").expect("plugin id should parse"); - let resolved = load_plugin_skills( + let resolved = load_plugin_skill_inventory( &plugin_root, - &plugin_id, + &PluginIdentity { + plugin_id: plugin_id.as_key(), + remote_plugin_id: None, + }, &manifest, + PluginManifestFormat::Legacy, /*restriction_product*/ None, - &SkillConfigRules::default(), /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), + test_skill_root_loader().as_ref(), ) - .await; + .await + .resolve(&SkillConfigRules::default()); let skill_paths = resolved .skills