From 6c469a9d8d0467ba0d01387322ea983742e728e6 Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Tue, 7 Jul 2026 06:19:44 +0000 Subject: [PATCH 1/2] perf(skills): resolve plugin namespaces per root --- codex-rs/core-skills/src/loader.rs | 40 +++--- .../core-skills/src/loader/environment.rs | 65 +++------- codex-rs/core-skills/src/loader/namespace.rs | 114 ++++++++++++++++++ 3 files changed, 151 insertions(+), 68 deletions(-) create mode 100644 codex-rs/core-skills/src/loader/namespace.rs diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index c9ace542046f..60b47bfb7913 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -1,4 +1,5 @@ mod environment; +mod namespace; pub use environment::EnvironmentSkillLoadOutcome; pub use environment::EnvironmentSkillMetadata; @@ -27,9 +28,10 @@ use codex_utils_absolute_path::AbsolutePathBufGuard; use codex_utils_path_uri::PathUri; use codex_utils_plugins::DISCOVERABLE_PLUGIN_MANIFEST_PATHS; use codex_utils_plugins::PluginSkillRoot; -use codex_utils_plugins::plugin_namespace_for_skill_path; use dirs::home_dir; use futures::future::join_all; +use namespace::ResolvedSkillNamespace; +use namespace::SkillNamespaceResolver; use serde::Deserialize; use std::collections::HashSet; use std::collections::VecDeque; @@ -680,12 +682,25 @@ async fn load_skills_under_root( }; let SkillFileDiscovery { skill_files, + plugin_roots, + namespace_roots, warnings, - .. } = discover_skills_under_root(fs, &PathUri::from_abs_path(root), symlink_policy).await; for warning in warnings { error!("{warning}"); } + let namespace_resolver = match plugin_namespace { + Some(namespace) => SkillNamespaceResolver::with_provided_namespace(namespace), + None => { + SkillNamespaceResolver::discover( + fs, + &PathUri::from_abs_path(root), + plugin_roots, + namespace_roots, + ) + .await + } + }; for path_uri in skill_files { let path = match path_uri.to_abs_path() { Ok(path) => path, @@ -699,7 +714,7 @@ async fn load_skills_under_root( &path, scope, plugin_id, - plugin_namespace, + namespace_resolver.for_skill(&path_uri), plugin_root.as_ref(), ) .await @@ -719,7 +734,7 @@ async fn parse_skill_file( path: &AbsolutePathBuf, scope: SkillScope, plugin_id: Option<&str>, - plugin_namespace: Option<&str>, + namespace: &ResolvedSkillNamespace, plugin_root: Option<&AbsolutePathBuf>, ) -> Result { let path_uri = PathUri::from_abs_path(path); @@ -732,7 +747,7 @@ async fn parse_skill_file( description, short_description, } = parse_skill_frontmatter_metadata_inner(&contents, || default_skill_name(path))?; - let name = namespaced_skill_name(fs, path, &base_name, plugin_namespace).await; + let name = namespace.qualify(&base_name); let LoadedSkillMetadata { interface, dependencies, @@ -818,21 +833,6 @@ fn default_skill_name(path: &AbsolutePathBuf) -> String { .unwrap_or_else(|| "skill".to_string()) } -async fn namespaced_skill_name( - fs: &dyn ExecutorFileSystem, - path: &AbsolutePathBuf, - base_name: &str, - plugin_namespace: Option<&str>, -) -> String { - if let Some(plugin_namespace) = plugin_namespace { - return format!("{plugin_namespace}:{base_name}"); - } - plugin_namespace_for_skill_path(fs, path) - .await - .map(|namespace| format!("{namespace}:{base_name}")) - .unwrap_or_else(|| base_name.to_string()) -} - async fn load_skill_metadata( fs: &dyn ExecutorFileSystem, skill_path: &AbsolutePathBuf, diff --git a/codex-rs/core-skills/src/loader/environment.rs b/codex-rs/core-skills/src/loader/environment.rs index 3d50bafe1fd3..ed937e4b5f81 100644 --- a/codex-rs/core-skills/src/loader/environment.rs +++ b/codex-rs/core-skills/src/loader/environment.rs @@ -1,4 +1,3 @@ -use std::collections::HashMap; use std::collections::HashSet; use std::io; @@ -8,10 +7,7 @@ use codex_exec_server::WalkOptions; use codex_protocol::protocol::Product; use codex_utils_path_uri::PathUri; use codex_utils_plugins::DISCOVERABLE_PLUGIN_MANIFEST_PATHS; -use codex_utils_plugins::plugin_namespace_for_root_uri; -use codex_utils_plugins::plugin_namespace_for_skill_uri; use futures::StreamExt; -use futures::future::join_all; use crate::model::SkillDependencies; use crate::model::SkillPolicy; @@ -24,6 +20,7 @@ use super::SKILLS_FILENAME; use super::SKILLS_METADATA_DIR; use super::SKILLS_METADATA_FILENAME; use super::SkillMetadataFile; +use super::namespace::SkillNamespaceResolver; use super::parse_skill_frontmatter_metadata_inner; use super::resolve_dependencies; use super::resolve_policy; @@ -256,36 +253,18 @@ pub async fn load_environment_skills_from_root( } } - let namespace_roots = discovery.namespace_roots; - let plugin_namespaces = async { - let namespace_lookups = join_all(namespace_roots.iter().map(|namespace_root| async { - ( - namespace_root.clone(), - plugin_namespace_for_skill_uri(file_system, namespace_root).await, - ) - })); - let plugin_lookups = join_all( - discovery - .plugin_roots - .iter() - .filter(|plugin_root| skill_ancestors.contains(*plugin_root)) - .filter(|plugin_root| !namespace_roots.contains(*plugin_root)) - .map(|plugin_root| async { - ( - plugin_root.clone(), - plugin_namespace_for_root_uri(file_system, plugin_root).await, - ) - }), - ); - let (namespace_lookups, plugin_lookups) = tokio::join!(namespace_lookups, plugin_lookups); - namespace_lookups - .into_iter() - .chain(plugin_lookups) - .filter_map(|(plugin_root, namespace)| { - namespace.map(|namespace| (plugin_root, namespace)) - }) - .collect::>() - }; + // Only pass roots above loaded skills so unused sibling manifests are never probed. + let plugin_roots = discovery + .plugin_roots + .into_iter() + .filter(|plugin_root| skill_ancestors.contains(plugin_root)) + .collect(); + let namespace_resolver = SkillNamespaceResolver::discover( + file_system, + root, + plugin_roots, + discovery.namespace_roots, + ); // Remote executors can multiplex these independent per-skill reads, so polling a bounded // number together allows the I/O for each skill and its metadata to happen concurrently. @@ -301,23 +280,13 @@ pub async fn load_environment_skills_from_root( }) .buffered(MAX_CONCURRENT_SKILL_LOADS) .collect::>(); - let (plugin_namespaces, skill_results) = tokio::join!(plugin_namespaces, skill_results); + let (namespace_resolver, skill_results) = tokio::join!(namespace_resolver, skill_results); for (path, result) in skill_results { let result = result.and_then(|skill| { - let mut ancestor = skill.path_to_skills_md.parent(); - let plugin_namespace = loop { - let Some(current) = ancestor else { - break None; - }; - if let Some(namespace) = plugin_namespaces.get(¤t) { - break Some(namespace.as_str()); - } - ancestor = current.parent(); - }; - let name = plugin_namespace - .map(|namespace| format!("{namespace}:{}", skill.base_name)) - .unwrap_or(skill.base_name); + let name = namespace_resolver + .for_skill(&skill.path_to_skills_md) + .qualify(&skill.base_name); validate_len(&name, MAX_QUALIFIED_NAME_LEN, "qualified name") .map_err(|err| err.to_string())?; diff --git a/codex-rs/core-skills/src/loader/namespace.rs b/codex-rs/core-skills/src/loader/namespace.rs new file mode 100644 index 000000000000..082af56d7812 --- /dev/null +++ b/codex-rs/core-skills/src/loader/namespace.rs @@ -0,0 +1,114 @@ +use codex_exec_server::ExecutorFileSystem; +use codex_utils_path_uri::PathUri; +use codex_utils_plugins::plugin_namespace_for_root_uri; +use codex_utils_plugins::plugin_namespace_for_skill_uri; +use futures::future::join_all; +use std::collections::HashSet; + +/// Resolves the namespace prefix applied to skill names during one skills scan. +/// +/// A plugin namespace is the plugin name from the nearest valid plugin manifest +/// above a skill path. For example, a skill named `search` beneath a plugin named +/// `sample` is exposed as `sample:search`. +/// +/// Resolving the namespace separately for every `SKILL.md` repeats the same +/// ancestor manifest probes for sibling skills. This resolver resolves relevant +/// roots once per scan, then selects the nearest matching root for each skill. +/// +/// Namespace precedence is: +/// +/// 1. an explicitly provided plugin namespace; +/// 2. the deepest matching canonical symlink root or nested plugin root; +/// 3. the namespace inherited from the scanned skills root. +pub(crate) struct SkillNamespaceResolver { + inherited_namespace: ResolvedSkillNamespace, + nested_namespaces: Vec<(PathUri, ResolvedSkillNamespace)>, +} + +impl SkillNamespaceResolver { + /// Builds a resolver whose explicit plugin-owned namespace overrides discovery. + pub(crate) fn with_provided_namespace(namespace: &str) -> Self { + Self { + inherited_namespace: ResolvedSkillNamespace::Plugin(namespace.to_string()), + nested_namespaces: Vec::new(), + } + } + + pub(crate) async fn discover( + fs: &dyn ExecutorFileSystem, + root: &PathUri, + plugin_roots: HashSet, + namespace_roots: HashSet, + ) -> Self { + // Ordinary descendants fall back to the nearest valid manifest at or above the scan root. + let inherited_namespace = plugin_namespace_for_skill_uri(fs, root) + .await + .map(ResolvedSkillNamespace::Plugin) + .unwrap_or(ResolvedSkillNamespace::Plain); + // The scan root is already the fallback above if nothing else matches, exclude from the search. + let namespace_roots = namespace_roots + .into_iter() + .filter(|namespace_root| namespace_root != root) + .collect::>(); + let namespace_root_set = namespace_roots.iter().cloned().collect::>(); + // Keep independent probes concurrent for remote executor latency. + let namespace_lookups = join_all(namespace_roots.into_iter().map(|namespace_root| async { + let namespace = plugin_namespace_for_skill_uri(fs, &namespace_root) + .await + .map(ResolvedSkillNamespace::Plugin) + .unwrap_or(ResolvedSkillNamespace::Plain); + (namespace_root, namespace) + })); + // Invalid nested manifests are omitted, so the deepest remaining match wins. + let plugin_lookups = join_all( + plugin_roots + .into_iter() + .filter(|plugin_root| { + plugin_root != root && !namespace_root_set.contains(plugin_root) + }) + .map(|plugin_root| async move { + plugin_namespace_for_root_uri(fs, &plugin_root) + .await + .map(|namespace| (plugin_root, ResolvedSkillNamespace::Plugin(namespace))) + }), + ); + let (namespace_lookups, plugin_lookups) = tokio::join!(namespace_lookups, plugin_lookups); + let nested_namespaces = namespace_lookups + .into_iter() + .chain(plugin_lookups.into_iter().flatten()) + .collect(); + + Self { + inherited_namespace, + nested_namespaces, + } + } + + pub(crate) fn for_skill(&self, path: &PathUri) -> &ResolvedSkillNamespace { + // The deepest matching path prefix is the nearest applicable namespace. + self.nested_namespaces + .iter() + .filter(|(namespace_root, _)| path.starts_with(namespace_root)) + .max_by_key(|(namespace_root, _)| namespace_root.ancestors().count()) + .map(|(_, namespace)| namespace) + .unwrap_or(&self.inherited_namespace) + } +} + +/// The completed namespace resolution for a skill root. +#[derive(Clone, Debug, Eq, PartialEq)] +pub(crate) enum ResolvedSkillNamespace { + /// No plugin namespace applies to matching skills. + Plain, + /// Qualify matching skill names with this plugin namespace. + Plugin(String), +} + +impl ResolvedSkillNamespace { + pub(crate) fn qualify(&self, base_name: &str) -> String { + match self { + Self::Plain => base_name.to_string(), + Self::Plugin(namespace) => format!("{namespace}:{base_name}"), + } + } +} From d59b7b390163bbf3ab85b98dbb40b2688b6689a9 Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Tue, 7 Jul 2026 06:43:20 +0000 Subject: [PATCH 2/2] codex: address PR review feedback (#31348) --- codex-rs/core-skills/src/loader.rs | 6 +++-- .../core-skills/src/loader/environment.rs | 25 ++++++------------- codex-rs/core-skills/src/loader/namespace.rs | 24 ++++++++++++++++-- 3 files changed, 34 insertions(+), 21 deletions(-) diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 60b47bfb7913..e74eb01a0c5a 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -689,12 +689,14 @@ async fn load_skills_under_root( for warning in warnings { error!("{warning}"); } + let root_uri = PathUri::from_abs_path(root); let namespace_resolver = match plugin_namespace { Some(namespace) => SkillNamespaceResolver::with_provided_namespace(namespace), None => { SkillNamespaceResolver::discover( fs, - &PathUri::from_abs_path(root), + &root_uri, + &skill_files, plugin_roots, namespace_roots, ) @@ -714,7 +716,7 @@ async fn load_skills_under_root( &path, scope, plugin_id, - namespace_resolver.for_skill(&path_uri), + namespace_resolver.for_skill(&root_uri, &path_uri), plugin_root.as_ref(), ) .await diff --git a/codex-rs/core-skills/src/loader/environment.rs b/codex-rs/core-skills/src/loader/environment.rs index ed937e4b5f81..da525d33620b 100644 --- a/codex-rs/core-skills/src/loader/environment.rs +++ b/codex-rs/core-skills/src/loader/environment.rs @@ -244,25 +244,16 @@ pub async fn load_environment_skills_from_root( return outcome; } - let mut skill_ancestors = HashSet::new(); - for skill in &discovery.skills { - let mut ancestor = skill.path.parent(); - while let Some(path) = ancestor { - skill_ancestors.insert(path.clone()); - ancestor = path.parent(); - } - } - - // Only pass roots above loaded skills so unused sibling manifests are never probed. - let plugin_roots = discovery - .plugin_roots - .into_iter() - .filter(|plugin_root| skill_ancestors.contains(plugin_root)) - .collect(); + let skill_paths = discovery + .skills + .iter() + .map(|skill| skill.path.clone()) + .collect::>(); let namespace_resolver = SkillNamespaceResolver::discover( file_system, root, - plugin_roots, + &skill_paths, + discovery.plugin_roots, discovery.namespace_roots, ); @@ -285,7 +276,7 @@ pub async fn load_environment_skills_from_root( for (path, result) in skill_results { let result = result.and_then(|skill| { let name = namespace_resolver - .for_skill(&skill.path_to_skills_md) + .for_skill(root, &skill.path_to_skills_md) .qualify(&skill.base_name); validate_len(&name, MAX_QUALIFIED_NAME_LEN, "qualified name") .map_err(|err| err.to_string())?; diff --git a/codex-rs/core-skills/src/loader/namespace.rs b/codex-rs/core-skills/src/loader/namespace.rs index 082af56d7812..34d8e9b7c4b1 100644 --- a/codex-rs/core-skills/src/loader/namespace.rs +++ b/codex-rs/core-skills/src/loader/namespace.rs @@ -37,9 +37,24 @@ impl SkillNamespaceResolver { pub(crate) async fn discover( fs: &dyn ExecutorFileSystem, root: &PathUri, + skill_paths: &[PathUri], plugin_roots: HashSet, namespace_roots: HashSet, ) -> Self { + // Only probe plugin roots above loaded skills; unused siblings cannot affect names. + let mut skill_ancestors = HashSet::new(); + for skill_path in skill_paths { + let mut ancestor = skill_path.parent(); + while let Some(path) = ancestor { + skill_ancestors.insert(path.clone()); + ancestor = path.parent(); + } + } + let plugin_roots = plugin_roots + .into_iter() + .filter(|plugin_root| skill_ancestors.contains(plugin_root)) + .collect::>(); + // Ordinary descendants fall back to the nearest valid manifest at or above the scan root. let inherited_namespace = plugin_namespace_for_skill_uri(fs, root) .await @@ -84,11 +99,16 @@ impl SkillNamespaceResolver { } } - pub(crate) fn for_skill(&self, path: &PathUri) -> &ResolvedSkillNamespace { + pub(crate) fn for_skill(&self, root: &PathUri, path: &PathUri) -> &ResolvedSkillNamespace { + // Ancestor symlink targets cannot override skills still owned by the scan root. + let path_is_under_root = path.starts_with(root); // The deepest matching path prefix is the nearest applicable namespace. self.nested_namespaces .iter() - .filter(|(namespace_root, _)| path.starts_with(namespace_root)) + .filter(|(namespace_root, _)| { + path.starts_with(namespace_root) + && (!path_is_under_root || !root.starts_with(namespace_root)) + }) .max_by_key(|(namespace_root, _)| namespace_root.ancestors().count()) .map(|(_, namespace)| namespace) .unwrap_or(&self.inherited_namespace)