Cache plugin namespace during executor skill discovery

codex-rs/core-skills/src/loader.rs

@@ -25,6 +25,7 @@ use codex_protocol::protocol::SkillScope;
 use codex_utils_absolute_path::AbsolutePathBuf;
 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;
@@ -145,6 +146,8 @@ enum SymlinkPolicy {
 
 struct SkillFileDiscovery {
     skill_files: Vec<PathUri>,
+    plugin_roots: HashSet<PathUri>,
+    namespace_roots: HashSet<PathUri>,
     warnings: Vec<String>,
 }
 
@@ -500,6 +503,8 @@ async fn discover_skills_under_root(
     let root = root.clone();
     let mut discovery = SkillFileDiscovery {
         skill_files: Vec::new(),
+        plugin_roots: HashSet::new(),
+        namespace_roots: HashSet::from([root.clone()]),
         warnings: Vec::new(),
     };
     match fs.get_metadata(&root, /*sandbox*/ None).await {
@@ -553,6 +558,12 @@ async fn discover_skills_under_root(
             .into_iter()
             .filter_map(|entry| {
                 let file_name = entry.file_name;
+                if DISCOVERABLE_PLUGIN_MANIFEST_PATHS
+                    .iter()
+                    .any(|path| path.split('/').next() == Some(file_name.as_str()))
+                {
+                    discovery.plugin_roots.insert(dir.clone());
+                }
                 if file_name.starts_with('.') {
                     return None;
                 }
@@ -592,6 +603,7 @@ async fn discover_skills_under_root(
                 match fs.read_directory(&path, /*sandbox*/ None).await {
                     Ok(_) => {
                         let resolved_dir = canonicalize_uri_for_skill_identity(fs, &path).await;
+                        discovery.namespace_roots.insert(resolved_dir.clone());
                         enqueue_dir(
                             &mut queue,
                             &mut visited_dirs,
@@ -669,6 +681,7 @@ async fn load_skills_under_root(
     let SkillFileDiscovery {
         skill_files,
         warnings,
+        ..
     } = discover_skills_under_root(fs, &PathUri::from_abs_path(root), symlink_policy).await;
     for warning in warnings {
         error!("{warning}");

codex-rs/core-skills/src/loader/environment.rs

@@ -1,9 +1,13 @@
+use std::collections::HashMap;
+use std::collections::HashSet;
 use std::io;
 
 use codex_exec_server::ExecutorFileSystem;
 use codex_protocol::protocol::Product;
 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 crate::model::SkillDependencies;
 use crate::model::SkillPolicy;
@@ -52,7 +56,11 @@ impl EnvironmentSkillMetadata {
         }
     }
 
-    async fn parse(file_system: &dyn ExecutorFileSystem, path: &PathUri) -> Result<Self, String> {
+    async fn parse(
+        file_system: &dyn ExecutorFileSystem,
+        path: &PathUri,
+        plugin_namespace: Option<&str>,
+    ) -> Result<Self, String> {
         let contents = file_system
             .read_file_text(path, /*sandbox*/ None)
             .await
@@ -63,8 +71,7 @@ impl EnvironmentSkillMetadata {
             short_description,
         } = parse_skill_frontmatter_metadata_inner(&contents, || default_skill_name(path))
             .map_err(|err| err.to_string())?;
-        let name = plugin_namespace_for_skill_uri(file_system, path)
-            .await
+        let name = plugin_namespace
             .map(|namespace| format!("{namespace}:{base_name}"))
             .unwrap_or(base_name);
         validate_len(&name, MAX_QUALIFIED_NAME_LEN, "qualified name")
@@ -98,8 +105,65 @@ pub async fn load_environment_skills_from_root(
     let discovery =
         discover_skills_under_root(file_system, root, SymlinkPolicy::FollowDirectories).await;
     outcome.warnings.extend(discovery.warnings);
+    if discovery.skill_files.is_empty() {
+        return outcome;
+    }
+
+    let mut skill_ancestors = HashSet::new();
+    for skill_path in &discovery.skill_files {
+        let mut ancestor = skill_path.parent();
+        while let Some(path) = ancestor {
+            skill_ancestors.insert(path.clone());
+            ancestor = path.parent();
+        }
+    }
+
+    let namespace_roots = discovery.namespace_roots;
+    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,
+        )
+    }))
+    .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,
+                )
+            }),
+    )
+    .await;
+    let plugin_namespaces = namespace_lookups
+        .into_iter()
+        .chain(plugin_lookups)
+        .filter_map(|(plugin_root, namespace)| namespace.map(|namespace| (plugin_root, namespace)))
+        .collect::<HashMap<_, _>>();
+
     for path in discovery.skill_files {
-        match EnvironmentSkillMetadata::parse(file_system, &path).await {
+        let mut ancestor = path.parent();
+        let plugin_namespace = loop {
+            let Some(current) = ancestor else {
+                break None;
+            };
+            if let Some(namespace) = plugin_namespaces.get(&current) {
+                break Some(namespace.as_str());
+            }
+            ancestor = current.parent();
+        };
+        match EnvironmentSkillMetadata::parse(
+            file_system,
+            &path,
+            /*plugin_namespace*/ plugin_namespace,
+        )
+        .await
+        {
             Ok(skill) if skill.matches_product_restriction(restriction_product) => {
                 outcome.skills.push(skill);
             }

codex-rs/core-skills/tests/environment_loader.rs

@@ -0,0 +1,216 @@
+use std::fs;
+use std::sync::Mutex;
+
+use codex_core_skills::loader::EnvironmentSkillMetadata;
+use codex_core_skills::loader::load_environment_skills_from_root;
+use codex_exec_server::CopyOptions;
+use codex_exec_server::CreateDirectoryOptions;
+use codex_exec_server::ExecutorFileSystem;
+use codex_exec_server::ExecutorFileSystemFuture;
+use codex_exec_server::FileMetadata;
+use codex_exec_server::FileSystemReadStream;
+use codex_exec_server::FileSystemSandboxContext;
+use codex_exec_server::LOCAL_FS;
+use codex_exec_server::ReadDirectoryEntry;
+use codex_exec_server::RemoveOptions;
+use codex_utils_path_uri::PathUri;
+use pretty_assertions::assert_eq;
+use tempfile::tempdir;
+
+struct RecordingFileSystem<'a> {
+    inner: &'a dyn ExecutorFileSystem,
+    read_files: Mutex<Vec<PathUri>>,
+}
+
+impl<'a> RecordingFileSystem<'a> {
+    fn new(inner: &'a dyn ExecutorFileSystem) -> Self {
+        Self {
+            inner,
+            read_files: Mutex::new(Vec::new()),
+        }
+    }
+
+    fn read_files(&self) -> Vec<PathUri> {
+        self.read_files
+            .lock()
+            .unwrap_or_else(std::sync::PoisonError::into_inner)
+            .clone()
+    }
+}
+
+impl ExecutorFileSystem for RecordingFileSystem<'_> {
+    fn canonicalize<'a>(
+        &'a self,
+        path: &'a PathUri,
+        sandbox: Option<&'a FileSystemSandboxContext>,
+    ) -> ExecutorFileSystemFuture<'a, PathUri> {
+        self.inner.canonicalize(path, sandbox)
+    }
+
+    fn read_file<'a>(
+        &'a self,
+        path: &'a PathUri,
+        sandbox: Option<&'a FileSystemSandboxContext>,
+    ) -> ExecutorFileSystemFuture<'a, Vec<u8>> {
+        self.read_files
+            .lock()
+            .unwrap_or_else(std::sync::PoisonError::into_inner)
+            .push(path.clone());
+        self.inner.read_file(path, sandbox)
+    }
+
+    fn read_file_stream<'a>(
+        &'a self,
+        path: &'a PathUri,
+        sandbox: Option<&'a FileSystemSandboxContext>,
+    ) -> ExecutorFileSystemFuture<'a, FileSystemReadStream> {
+        self.inner.read_file_stream(path, sandbox)
+    }
+
+    fn write_file<'a>(
+        &'a self,
+        path: &'a PathUri,
+        contents: Vec<u8>,
+        sandbox: Option<&'a FileSystemSandboxContext>,
+    ) -> ExecutorFileSystemFuture<'a, ()> {
+        self.inner.write_file(path, contents, sandbox)
+    }
+
+    fn create_directory<'a>(
+        &'a self,
+        path: &'a PathUri,
+        options: CreateDirectoryOptions,
+        sandbox: Option<&'a FileSystemSandboxContext>,
+    ) -> ExecutorFileSystemFuture<'a, ()> {
+        self.inner.create_directory(path, options, sandbox)
+    }
+
+    fn get_metadata<'a>(
+        &'a self,
+        path: &'a PathUri,
+        sandbox: Option<&'a FileSystemSandboxContext>,
+    ) -> ExecutorFileSystemFuture<'a, FileMetadata> {
+        self.inner.get_metadata(path, sandbox)
+    }
+
+    fn read_directory<'a>(
+        &'a self,
+        path: &'a PathUri,
+        sandbox: Option<&'a FileSystemSandboxContext>,
+    ) -> ExecutorFileSystemFuture<'a, Vec<ReadDirectoryEntry>> {
+        self.inner.read_directory(path, sandbox)
+    }
+
+    fn remove<'a>(
+        &'a self,
+        path: &'a PathUri,
+        options: RemoveOptions,
+        sandbox: Option<&'a FileSystemSandboxContext>,
+    ) -> ExecutorFileSystemFuture<'a, ()> {
+        self.inner.remove(path, options, sandbox)
+    }
+
+    fn copy<'a>(
+        &'a self,
+        source_path: &'a PathUri,
+        destination_path: &'a PathUri,
+        options: CopyOptions,
+        sandbox: Option<&'a FileSystemSandboxContext>,
+    ) -> ExecutorFileSystemFuture<'a, ()> {
+        self.inner
+            .copy(source_path, destination_path, options, sandbox)
+    }
+}
+
+#[tokio::test]
+async fn loads_nearest_plugin_namespaces_without_reading_unused_sibling_manifests() {
+    let root = tempdir().expect("tempdir");
+    let standalone_skill = root.path().join("standalone/SKILL.md");
+    let outer_root = root.path().join("plugins/outer");
+    let outer_skill = outer_root.join("skills/deploy/SKILL.md");
+    let inner_root = outer_root.join("nested/inner");
+    let inner_skill = inner_root.join("skills/audit/SKILL.md");
+    let unused_root = root.path().join("plugins/unused");
+
+    for path in [&standalone_skill, &outer_skill, &inner_skill] {
+        fs::create_dir_all(path.parent().expect("skill parent")).expect("skill dir");
+    }
+    for (plugin_root, name) in [
+        (&outer_root, "outer"),
+        (&inner_root, "inner"),
+        (&unused_root, "unused"),
+    ] {
+        fs::create_dir_all(plugin_root.join(".codex-plugin")).expect("manifest dir");
+        fs::write(
+            plugin_root.join(".codex-plugin/plugin.json"),
+            format!(r#"{{"name":"{name}"}}"#),
+        )
+        .expect("manifest");
+    }
+    for (path, name) in [
+        (&standalone_skill, "standalone"),
+        (&outer_skill, "deploy"),
+        (&inner_skill, "audit"),
+    ] {
+        fs::write(
+            path,
+            format!("---\nname: {name}\ndescription: {name} skill.\n---\n"),
+        )
+        .expect("skill");
+    }
+
+    let file_system = RecordingFileSystem::new(LOCAL_FS.as_ref());
+    let root_uri = PathUri::from_host_native_path(root.path()).expect("root URI");
+    let outcome = load_environment_skills_from_root(
+        &file_system,
+        &root_uri,
+        /*restriction_product*/ None,
+    )
+    .await;
+
+    assert_eq!(outcome.warnings, Vec::<String>::new());
+    assert_eq!(
+        outcome.skills,
+        vec![
+            EnvironmentSkillMetadata {
+                path_to_skills_md: PathUri::from_host_native_path(&inner_skill).unwrap(),
+                name: "inner:audit".to_string(),
+                description: "audit skill.".to_string(),
+                short_description: None,
+                dependencies: None,
+                policy: None,
+            },
+            EnvironmentSkillMetadata {
+                path_to_skills_md: PathUri::from_host_native_path(&outer_skill).unwrap(),
+                name: "outer:deploy".to_string(),
+                description: "deploy skill.".to_string(),
+                short_description: None,
+                dependencies: None,
+                policy: None,
+            },
+            EnvironmentSkillMetadata {
+                path_to_skills_md: PathUri::from_host_native_path(&standalone_skill).unwrap(),
+                name: "standalone".to_string(),
+                description: "standalone skill.".to_string(),
+                short_description: None,
+                dependencies: None,
+                policy: None,
+            },
+        ]
+    );
+
+    let mut manifest_reads = file_system
+        .read_files()
+        .into_iter()
+        .filter(|path| path.basename().as_deref() == Some("plugin.json"))
+        .collect::<Vec<_>>();
+    manifest_reads.sort_by_key(ToString::to_string);
+    let mut expected_manifest_reads = [&outer_root, &inner_root]
+        .into_iter()
+        .map(|plugin_root| {
+            PathUri::from_host_native_path(plugin_root.join(".codex-plugin/plugin.json")).unwrap()
+        })
+        .collect::<Vec<_>>();
+    expected_manifest_reads.sort_by_key(ToString::to_string);
+    assert_eq!(manifest_reads, expected_manifest_reads);
+}

codex-rs/utils/plugins/src/lib.rs

@@ -9,6 +9,7 @@ pub mod plugin_namespace;
 
 pub use plugin_namespace::DISCOVERABLE_PLUGIN_MANIFEST_PATHS;
 pub use plugin_namespace::find_plugin_manifest_path;
+pub use plugin_namespace::plugin_namespace_for_root_uri;
 pub use plugin_namespace::plugin_namespace_for_skill_path;
 pub use plugin_namespace::plugin_namespace_for_skill_uri;