Preserve plugin manifest precedence

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

@@ -41,56 +41,71 @@ fn plugin_manifest_name(plugin_root: &AbsolutePathBuf, contents: &[u8]) -> Optio
 }
 
 /// Resolves plugin namespaces while caching and batching ancestor manifest probes.
-#[derive(Default)]
 pub struct PluginNamespaceResolver {
-    manifests_by_root: HashMap<AbsolutePathBuf, Option<String>>,
+    manifests_by_root: HashMap<AbsolutePathBuf, String>,
 }
 
 impl PluginNamespaceResolver {
-    pub async fn prime(&mut self, fs: &dyn ExecutorFileSystem, paths: &[AbsolutePathBuf]) {
+    /// Loads plugin manifest names for every ancestor of `paths`.
+    pub async fn load(fs: &dyn ExecutorFileSystem, paths: &[AbsolutePathBuf]) -> Self {
         let mut seen = HashSet::new();
-        let unresolved_roots = paths
+        let roots = paths
             .iter()
             .flat_map(AbsolutePathBuf::ancestors)
-            .filter(|ancestor| !self.manifests_by_root.contains_key(ancestor))
             .filter(|ancestor| seen.insert(ancestor.clone()))
             .collect::<Vec<_>>();
-        let operations = unresolved_roots
+        let operations = roots
             .iter()
             .flat_map(|root| {
                 DISCOVERABLE_PLUGIN_MANIFEST_PATHS
                     .iter()
-                    .map(|relative_path| FileSystemOperation::ReadFile {
-                        path: PathUri::from_abs_path(&root.join(relative_path)),
+                    .flat_map(|relative_path| {
+                        let path = PathUri::from_abs_path(&root.join(relative_path));
+                        [
+                            FileSystemOperation::GetMetadata { path: path.clone() },
+                            FileSystemOperation::ReadFile { path },
+                        ]
                     })
             })
             .collect();
-        let mut results = match fs.execute_batch(operations, /*sandbox*/ None).await {
-            Ok(results) => results.into_iter(),
-            Err(_) => Vec::new().into_iter(),
-        };
-        for root in unresolved_roots {
-            let mut name = None;
-            for _ in DISCOVERABLE_PLUGIN_MANIFEST_PATHS {
-                if let Some(Ok(FileSystemOperationOutput::ReadFile(contents))) = results.next()
-                    && name.is_none()
-                {
-                    name = plugin_manifest_name(&root, &contents);
+        let mut results = fs
+            .execute_batch(operations, /*sandbox*/ None)
+            .await
+            .unwrap_or_default()
+            .into_iter();
+        let manifests_by_root = roots
+            .into_iter()
+            .filter_map(|root| {
+                let mut name = None;
+                let mut selected = false;
+                for _ in DISCOVERABLE_PLUGIN_MANIFEST_PATHS {
+                    let metadata_result = results.next();
+                    let contents_result = results.next();
+                    let is_file = matches!(
+                        metadata_result,
+                        Some(Ok(FileSystemOperationOutput::GetMetadata(metadata)))
+                            if metadata.is_file
+                    );
+                    if !selected && is_file {
+                        selected = true;
+                        if let Some(Ok(FileSystemOperationOutput::ReadFile(contents))) =
+                            contents_result
+                        {
+                            name = plugin_manifest_name(&root, &contents);
+                        }
+                    }
                 }
-            }
-            self.manifests_by_root.insert(root, name);
-        }
+                name.map(|name| (root, name))
+            })
+            .collect();
+
+        Self { manifests_by_root }
     }
 
-    pub async fn resolve(
-        &mut self,
-        fs: &dyn ExecutorFileSystem,
-        path: &AbsolutePathBuf,
-    ) -> Option<String> {
-        self.prime(fs, std::slice::from_ref(path)).await;
-
+    /// Returns the nearest preloaded plugin namespace for `path`.
+    pub fn resolve(&self, path: &AbsolutePathBuf) -> Option<&str> {
         path.ancestors()
-            .find_map(|ancestor| self.manifests_by_root.get(&ancestor).cloned().flatten())
+            .find_map(|ancestor| self.manifests_by_root.get(&ancestor).map(String::as_str))
     }
 }
 
@@ -100,7 +115,10 @@ pub async fn plugin_namespace_for_skill_path(
     fs: &dyn ExecutorFileSystem,
     path: &AbsolutePathBuf,
 ) -> Option<String> {
-    PluginNamespaceResolver::default().resolve(fs, path).await
+    PluginNamespaceResolver::load(fs, std::slice::from_ref(path))
+        .await
+        .resolve(path)
+        .map(str::to_string)
 }
 
 #[cfg(test)]
@@ -112,6 +130,7 @@ mod tests {
     use std::fs;
     use tempfile::tempdir;
 
+    const PRIMARY_PLUGIN_MANIFEST_RELATIVE_PATH: &str = ".codex-plugin/plugin.json";
     const ALTERNATE_PLUGIN_MANIFEST_RELATIVE_PATH: &str = ".claude-plugin/plugin.json";
 
     #[tokio::test]
@@ -123,7 +142,7 @@ mod tests {
         fs::create_dir_all(skill_path.parent().expect("parent")).expect("mkdir");
         fs::create_dir_all(plugin_root.join(".codex-plugin")).expect("mkdir manifest");
         fs::write(
-            plugin_root.join(".codex-plugin/plugin.json"),
+            plugin_root.join(PRIMARY_PLUGIN_MANIFEST_RELATIVE_PATH),
             r#"{"name":"sample"}"#,
         )
         .expect("write manifest");
@@ -154,4 +173,32 @@ mod tests {
         );
         assert_eq!(find_plugin_manifest_path(&plugin_root), Some(manifest_path));
     }
+
+    #[tokio::test]
+    async fn invalid_primary_manifest_does_not_fall_back_to_alternate() {
+        let tmp = tempdir().expect("tempdir");
+        let plugin_root = tmp.path().join("plugins/sample");
+        let skill_path = plugin_root.join("skills/search/SKILL.md");
+        let primary_manifest_path = plugin_root.join(PRIMARY_PLUGIN_MANIFEST_RELATIVE_PATH);
+        let alternate_manifest_path = plugin_root.join(ALTERNATE_PLUGIN_MANIFEST_RELATIVE_PATH);
+
+        fs::create_dir_all(skill_path.parent().expect("parent")).expect("mkdir");
+        fs::create_dir_all(primary_manifest_path.parent().expect("manifest parent"))
+            .expect("mkdir primary manifest");
+        fs::create_dir_all(alternate_manifest_path.parent().expect("manifest parent"))
+            .expect("mkdir alternate manifest");
+        fs::write(&primary_manifest_path, "invalid json").expect("write primary manifest");
+        fs::write(&alternate_manifest_path, r#"{"name":"sample"}"#)
+            .expect("write alternate manifest");
+        fs::write(&skill_path, "---\ndescription: search\n---\n").expect("write skill");
+
+        assert_eq!(
+            plugin_namespace_for_skill_path(LOCAL_FS.as_ref(), &skill_path.abs()).await,
+            None
+        );
+        assert_eq!(
+            find_plugin_manifest_path(&plugin_root),
+            Some(primary_manifest_path)
+        );
+    }
 }