mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] Pass plugin namespace into skill loading (#28608)
## What changed - retain the parsed plugin manifest namespace on loaded plugins - carry that namespace through `PluginSkillRoot` and `SkillRoot` - use the provided namespace when qualifying plugin skill names - include the namespace in the skills cache key ## Why Plugin loading has already parsed `plugin.json`, but skill parsing currently walks every `SKILL.md` ancestor and probes/reads the manifest again to reconstruct the same namespace. Passing the parsed namespace removes those repeated filesystem calls, which are particularly costly on remote filesystems. Context: https://openai.slack.com/archives/C0ARA9GF5D4/p1781639496496439?thread_ts=1781202444.891669&cid=C0ARA9GF5D4 ## Impact Plugin skill names remain unchanged. A regression test uses a deliberately different on-disk manifest name to verify that plugin roots use the provided parsed namespace. ## Validation - `just test -p codex-core-skills -p codex-core-plugins -p codex-plugin -p codex-utils-plugins` (352 passed) - `just fix -p codex-core-skills -p codex-core-plugins -p codex-plugin -p codex-utils-plugins` - `just fmt`
This commit is contained in:
committed by
GitHub
Unverified
parent
2c7802e7cf
commit
c73296a0f0
@@ -1,6 +1,7 @@
|
||||
use crate::app_mcp_routing::apply_app_mcp_routing_policy;
|
||||
use crate::app_mcp_routing::apps_route_available;
|
||||
use crate::is_openai_curated_marketplace_name;
|
||||
use crate::manifest::PluginManifest;
|
||||
use crate::manifest::PluginManifestHooks;
|
||||
use crate::manifest::PluginManifestMcpServers;
|
||||
use crate::manifest::PluginManifestPaths;
|
||||
@@ -664,6 +665,7 @@ async fn load_plugin(
|
||||
let mut loaded_plugin = LoadedPlugin {
|
||||
config_name,
|
||||
manifest_name: None,
|
||||
plugin_namespace: None,
|
||||
manifest_description: None,
|
||||
root,
|
||||
enabled: plugin.enabled,
|
||||
@@ -706,6 +708,7 @@ async fn load_plugin(
|
||||
};
|
||||
|
||||
let manifest_paths = &manifest.paths;
|
||||
loaded_plugin.plugin_namespace = Some(manifest.name.clone());
|
||||
match scope {
|
||||
PluginLoadScope::AllCapabilities {
|
||||
restriction_product,
|
||||
@@ -717,7 +720,7 @@ async fn load_plugin(
|
||||
let resolved_skills = load_plugin_skills(
|
||||
&plugin_root,
|
||||
&loaded_plugin_id,
|
||||
manifest_paths,
|
||||
&manifest,
|
||||
*restriction_product,
|
||||
skill_config_rules,
|
||||
)
|
||||
@@ -785,17 +788,18 @@ impl ResolvedPluginSkills {
|
||||
pub async fn load_plugin_skills(
|
||||
plugin_root: &AbsolutePathBuf,
|
||||
plugin_id: &PluginId,
|
||||
manifest_paths: &PluginManifestPaths,
|
||||
manifest: &PluginManifest,
|
||||
restriction_product: Option<Product>,
|
||||
skill_config_rules: &SkillConfigRules,
|
||||
) -> ResolvedPluginSkills {
|
||||
let roots = plugin_skill_roots(plugin_root, manifest_paths)
|
||||
let roots = plugin_skill_roots(plugin_root, &manifest.paths)
|
||||
.into_iter()
|
||||
.map(|path| SkillRoot {
|
||||
path,
|
||||
scope: SkillScope::User,
|
||||
file_system: Arc::clone(&LOCAL_FS),
|
||||
plugin_id: Some(plugin_id.as_key()),
|
||||
plugin_namespace: Some(manifest.name.clone()),
|
||||
plugin_root: Some(plugin_root.clone()),
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
@@ -1591,7 +1591,7 @@ impl PluginsManager {
|
||||
let resolved_skills = load_plugin_skills(
|
||||
&source_path,
|
||||
&plugin_id,
|
||||
&manifest.paths,
|
||||
&manifest,
|
||||
self.restriction_product,
|
||||
&codex_core_skills::config_rules::skill_config_rules_from_stack(
|
||||
&config.config_layer_stack,
|
||||
|
||||
@@ -623,6 +623,7 @@ async fn load_plugins_loads_default_skills_and_mcp_servers() {
|
||||
vec![LoadedPlugin {
|
||||
config_name: "sample@test".to_string(),
|
||||
manifest_name: Some("sample".to_string()),
|
||||
plugin_namespace: Some("sample".to_string()),
|
||||
manifest_description: Some(
|
||||
"Plugin that includes the sample MCP server and Skills".to_string(),
|
||||
),
|
||||
@@ -1446,23 +1447,30 @@ async fn load_plugin_skills_dedupes_overlapping_manifest_roots() {
|
||||
&plugin_root.join("skills/edk/SKILL.md"),
|
||||
"---\nname: edk\ndescription: edk skill\n---\n",
|
||||
);
|
||||
let manifest_paths = crate::manifest::PluginManifestPaths {
|
||||
skills: vec![
|
||||
plugin_root.join("skills"),
|
||||
plugin_root.join("skills/abc"),
|
||||
plugin_root.join("skills/edk"),
|
||||
plugin_root.join("skills/abc"),
|
||||
],
|
||||
mcp_servers: None,
|
||||
apps: None,
|
||||
hooks: None,
|
||||
let manifest = crate::manifest::PluginManifest {
|
||||
name: "sample".to_string(),
|
||||
version: None,
|
||||
description: None,
|
||||
keywords: Vec::new(),
|
||||
paths: crate::manifest::PluginManifestPaths {
|
||||
skills: vec![
|
||||
plugin_root.join("skills"),
|
||||
plugin_root.join("skills/abc"),
|
||||
plugin_root.join("skills/edk"),
|
||||
plugin_root.join("skills/abc"),
|
||||
],
|
||||
mcp_servers: None,
|
||||
apps: None,
|
||||
hooks: None,
|
||||
},
|
||||
interface: None,
|
||||
};
|
||||
let plugin_id = PluginId::parse("sample@test").expect("plugin id should parse");
|
||||
|
||||
let resolved = load_plugin_skills(
|
||||
&plugin_root,
|
||||
&plugin_id,
|
||||
&manifest_paths,
|
||||
&manifest,
|
||||
/*restriction_product*/ None,
|
||||
&SkillConfigRules::default(),
|
||||
)
|
||||
@@ -1677,6 +1685,7 @@ async fn load_plugins_preserves_disabled_plugins_without_effective_contributions
|
||||
vec![LoadedPlugin {
|
||||
config_name: "sample@test".to_string(),
|
||||
manifest_name: None,
|
||||
plugin_namespace: None,
|
||||
manifest_description: None,
|
||||
root: AbsolutePathBuf::try_from(plugin_root).unwrap(),
|
||||
enabled: false,
|
||||
@@ -1845,6 +1854,12 @@ fn capability_index_filters_inactive_and_zero_capability_plugins() {
|
||||
let plugin = |config_name: &str, dir_name: &str, manifest_name: &str| LoadedPlugin {
|
||||
config_name: config_name.to_string(),
|
||||
manifest_name: Some(manifest_name.to_string()),
|
||||
plugin_namespace: Some(
|
||||
config_name
|
||||
.split_once('@')
|
||||
.map_or(config_name, |(name, _)| name)
|
||||
.to_string(),
|
||||
),
|
||||
manifest_description: None,
|
||||
root: AbsolutePathBuf::try_from(codex_home.path().join(dir_name)).unwrap(),
|
||||
enabled: true,
|
||||
|
||||
Reference in New Issue
Block a user