openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-19 17:28:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

Unify plugin skill loading through the shared loader (#37452)

Browse Source 
## 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
This commit is contained in:
felixxia-oai
2026-08-07 14:35:42 +00:00
committed by copyberry
parent a9d59d8b8e
commit c5d9431971
4 changed files with 158 additions and 134 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/app-server/tests/suite/v2/plugin_read.rs
+61
View File
@@ -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()?;
codex-rs/core-plugins/src/loader.rs
+3 -117
View File
@@ -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<Product>,
skill_config_rules: &SkillConfigRules,
plugin_skill_snapshots: Option<&LegacyPluginSkillSnapshots>,
root_scan_slots: Arc<Semaphore>,
) -> 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<Product>,
skill_config_rules: &SkillConfigRules,
plugin_skill_snapshots: Option<&LegacyPluginSkillSnapshots>,
root_scan_slots: Arc<Semaphore>,
) -> 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<Product>,
plugin_skill_snapshots: Option<&LegacyPluginSkillSnapshots>,
root_scan_slots: Arc<Semaphore>,
) -> 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::<Vec<_>>();
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::<Vec<_>>();
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::<HashSet<_>>();
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::<Vec<_>>();
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<PluginSkillRoot>,
) -> Option<PluginCapabilitySummary> {
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
codex-rs/core-plugins/src/manager.rs
+17 -6
View File
@@ -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<LoadedPluginsCache>,
loaded_plugins_load_semaphore: Semaphore,
skill_root_scan_slots: Arc<Semaphore>,
skill_root_loader: Arc<dyn SkillRootLoader<PluginSkillRoot>>,
tool_suggest_metadata_cache: ToolSuggestMetadataCache,
remote_installed_plugins_cache: RwLock<Option<Vec<RemoteInstalledPlugin>>>,
@@ -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);
codex-rs/core-plugins/src/manager_tests.rs
+77 -11
View File
@@ -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