Cache plugin namespace during executor skill discovery (#29831)

## Why Executor skill discovery runs before the remote skills catalog is available. For a remote environment, each `ExecutorFileSystem` operation becomes an exec-server RPC. Previously, every discovered `SKILL.md` independently resolved its plugin namespace by walking its ancestors and probing both supported manifest locations. In the common `plugin/skills/<skill>/SKILL.md` layout, that repeats 8 RPCs per skill even though every skill under the plugin root uses the same namespace. These lookups happen while skills are parsed, so their cost grows linearly with the skill count and adds directly to first-turn latency. A selected capability root can also contain standalone skills, multiple sibling plugins, nested plugins, or symlinked directories. The optimization therefore needs to retain the nearest-ancestor namespace for each skill rather than assuming the selected root represents exactly one plugin. ## What changed - record plugin-root candidates from directory entries already returned during skill discovery - prune candidates that are not ancestors of any discovered `SKILL.md` before reading manifests - resolve each relevant plugin root once, with one fallback lookup per canonical traversal root for symlinked directories - select the nearest cached plugin namespace for each discovered skill - avoid namespace lookup entirely when the root contains no skills No additional directory traversal is required. Namespace work now scales with the number of plugin roots that contain discovered skills, rather than the total number of skills or unrelated sibling plugins. Standalone and nested-plugin names keep their previous behavior. ## Benchmarks I used a temporary counting `ExecutorFileSystem` around the real local filesystem. Each filesystem operation was counted as one remote RPC and given 1 ms of injected latency. Each variant ran three times; times below are medians. ### One plugin with 100 skills | Operation | Before | After | Delta | | --- | ---: | ---: | ---: | | `get_metadata` | 1,002 | 303 | -699 | | `read_file` | 200 | 101 | -99 | | `read_directory` | 102 | 102 | 0 | | **Total filesystem RPCs** | **1,304** | **506** | **-798 (-61.2%)** | | **Median load time** | **2.890 s** | **0.997 s** | **2.90× faster** | The namespace-specific work drops from 800 RPCs to 2 in this layout. ### Multiple plugins under one selected root These runs compare the correct pre-optimization implementation with the final nearest-plugin-root cache. The total plugin skill count stays at 100 while the number of plugin roots changes. | Layout | Before RPCs | After RPCs | Reduction | Before | After | Speedup | | --- | ---: | ---: | ---: | ---: | ---: | ---: | | 2 plugins × 50 skills | 1,312 | 530 | 59.6% | 1,819 ms | 711 ms | 2.56× | | 10 plugins × 10 skills | 1,344 | 578 | 57.0% | 1,850 ms | 778 ms | 2.38× | | 50 plugins × 2 skills | 1,504 | 818 | 45.6% | 2,094 ms | 1,086 ms | 1.93× | | 10 plugins × 10 skills + 10 standalone skills | 1,596 | 630 | 60.5% | 2,209 ms | 860 ms | 2.57× | The remaining cost grows with the number of relevant plugin manifests. Each relevant manifest is read once instead of once per skill, while sibling plugins with no discovered skills are not read. Absolute latency savings depend on the executor's real RPC latency. ## Tests - `just test -p codex-core-skills` (109 passed across the library and integration-test binaries) - one integration test covers standalone, outer-plugin, nested-plugin, and unused sibling-plugin layouts, and asserts the exact set of manifests read
2026-07-01 00:31:56 +08:00 · 2026-06-24 17:14:34 +01:00
parent 3694b48a82
commit 390b73133b
5 changed files with 301 additions and 6 deletions
@@ -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}");
@@ -1,3 +1,5 @@
+use std::collections::HashMap;
+use std::collections::HashSet;
 use std::io;

 use codex_exec_server::ExecutorFileSystem;
@@ -5,7 +7,9 @@ use codex_exec_server::WalkEntryKind;
 use codex_exec_server::WalkOptions;
 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;
@@ -58,7 +62,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
@@ -69,8 +77,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")
@@ -152,8 +159,65 @@ pub async fn load_environment_skills_from_root(
        },
    };
    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);
            }
@@ -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);
+}
@@ -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;

@@ -24,7 +24,8 @@ struct RawPluginManifestName {
    name: String,
 }

-async fn plugin_manifest_name(
+/// Returns the plugin manifest `name` defined directly below `plugin_root`.
+pub async fn plugin_namespace_for_root_uri(
    fs: &dyn ExecutorFileSystem,
    plugin_root: &PathUri,
 ) -> Option<String> {
@@ -68,7 +69,7 @@ pub async fn plugin_namespace_for_skill_uri(
 ) -> Option<String> {
    let mut ancestor = Some(path.clone());
    while let Some(path) = ancestor {
-        if let Some(name) = plugin_manifest_name(fs, &path).await {
+        if let Some(name) = plugin_namespace_for_root_uri(fs, &path).await {
            return Some(name);
        }
        ancestor = path.parent();