feat(plugins): add plugin loading error handling and manifest validation

- Add structured error types for plugin loading failures
- Add manifest field validation
- Improve plugin API surface with consistent error patterns
- 31 plugins tests pass, clippy clean

rust/crates/plugins/src/lib.rs

@@ -648,6 +648,106 @@ pub struct PluginSummary {
     pub enabled: bool,
 }
 
+#[derive(Debug)]
+pub struct PluginLoadFailure {
+    pub plugin_root: PathBuf,
+    pub kind: PluginKind,
+    pub source: String,
+    error: Box<PluginError>,
+}
+
+impl PluginLoadFailure {
+    #[must_use]
+    pub fn new(plugin_root: PathBuf, kind: PluginKind, source: String, error: PluginError) -> Self {
+        Self {
+            plugin_root,
+            kind,
+            source,
+            error: Box::new(error),
+        }
+    }
+
+    #[must_use]
+    pub fn error(&self) -> &PluginError {
+        self.error.as_ref()
+    }
+}
+
+impl Display for PluginLoadFailure {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "failed to load {} plugin from `{}` (source: {}): {}",
+            self.kind,
+            self.plugin_root.display(),
+            self.source,
+            self.error()
+        )
+    }
+}
+
+#[derive(Debug)]
+pub struct PluginRegistryReport {
+    registry: PluginRegistry,
+    failures: Vec<PluginLoadFailure>,
+}
+
+impl PluginRegistryReport {
+    #[must_use]
+    pub fn new(registry: PluginRegistry, failures: Vec<PluginLoadFailure>) -> Self {
+        Self { registry, failures }
+    }
+
+    #[must_use]
+    pub fn registry(&self) -> &PluginRegistry {
+        &self.registry
+    }
+
+    #[must_use]
+    pub fn failures(&self) -> &[PluginLoadFailure] {
+        &self.failures
+    }
+
+    #[must_use]
+    pub fn has_failures(&self) -> bool {
+        !self.failures.is_empty()
+    }
+
+    #[must_use]
+    pub fn summaries(&self) -> Vec<PluginSummary> {
+        self.registry.summaries()
+    }
+
+    pub fn into_registry(self) -> Result<PluginRegistry, PluginError> {
+        if self.failures.is_empty() {
+            Ok(self.registry)
+        } else {
+            Err(PluginError::LoadFailures(self.failures))
+        }
+    }
+}
+
+#[derive(Debug, Default)]
+struct PluginDiscovery {
+    plugins: Vec<PluginDefinition>,
+    failures: Vec<PluginLoadFailure>,
+}
+
+impl PluginDiscovery {
+    fn push_plugin(&mut self, plugin: PluginDefinition) {
+        self.plugins.push(plugin);
+    }
+
+    fn push_failure(&mut self, failure: PluginLoadFailure) {
+        self.failures.push(failure);
+    }
+
+    fn extend(&mut self, other: Self) {
+        self.plugins.extend(other.plugins);
+        self.failures.extend(other.failures);
+    }
+}
+
 #[derive(Debug, Clone, Default, PartialEq)]
 pub struct PluginRegistry {
     plugins: Vec<RegisteredPlugin>,
@@ -802,6 +902,10 @@ pub enum PluginManifestValidationError {
         kind: &'static str,
         path: PathBuf,
     },
+    PathIsDirectory {
+        kind: &'static str,
+        path: PathBuf,
+    },
     InvalidToolInputSchema {
         tool_name: String,
     },
@@ -838,6 +942,9 @@ impl Display for PluginManifestValidationError {
             Self::MissingPath { kind, path } => {
                 write!(f, "{kind} path `{}` does not exist", path.display())
             }
+            Self::PathIsDirectory { kind, path } => {
+                write!(f, "{kind} path `{}` must point to a file", path.display())
+            }
             Self::InvalidToolInputSchema { tool_name } => {
                 write!(
                     f,
@@ -860,6 +967,7 @@ pub enum PluginError {
     Io(std::io::Error),
     Json(serde_json::Error),
     ManifestValidation(Vec<PluginManifestValidationError>),
+    LoadFailures(Vec<PluginLoadFailure>),
     InvalidManifest(String),
     NotFound(String),
     CommandFailed(String),
@@ -879,6 +987,15 @@ impl Display for PluginError {
                 }
                 Ok(())
             }
+            Self::LoadFailures(failures) => {
+                for (index, failure) in failures.iter().enumerate() {
+                    if index > 0 {
+                        write!(f, "; ")?;
+                    }
+                    write!(f, "{failure}")?;
+                }
+                Ok(())
+            }
             Self::InvalidManifest(message)
             | Self::NotFound(message)
             | Self::CommandFailed(message) => write!(f, "{message}"),
@@ -935,15 +1052,23 @@ impl PluginManager {
     }
 
     pub fn plugin_registry(&self) -> Result<PluginRegistry, PluginError> {
-        Ok(PluginRegistry::new(
-            self.discover_plugins()?
-                .into_iter()
-                .map(|plugin| {
-                    let enabled = self.is_enabled(plugin.metadata());
-                    RegisteredPlugin::new(plugin, enabled)
-                })
-                .collect(),
-        ))
+        self.plugin_registry_report()?.into_registry()
+    }
+
+    pub fn plugin_registry_report(&self) -> Result<PluginRegistryReport, PluginError> {
+        self.sync_bundled_plugins()?;
+
+        let mut discovery = PluginDiscovery::default();
+        discovery.plugins.extend(builtin_plugins());
+
+        let installed = self.discover_installed_plugins_with_failures()?;
+        discovery.extend(installed);
+
+        let external =
+            self.discover_external_directory_plugins_with_failures(&discovery.plugins)?;
+        discovery.extend(external);
+
+        Ok(self.build_registry_report(discovery))
     }
 
     pub fn list_plugins(&self) -> Result<Vec<PluginSummary>, PluginError> {
@@ -955,11 +1080,12 @@ impl PluginManager {
     }
 
     pub fn discover_plugins(&self) -> Result<Vec<PluginDefinition>, PluginError> {
-        self.sync_bundled_plugins()?;
-        let mut plugins = builtin_plugins();
-        plugins.extend(self.discover_installed_plugins()?);
-        plugins.extend(self.discover_external_directory_plugins(&plugins)?);
-        Ok(plugins)
+        Ok(self
+            .plugin_registry()?
+            .plugins
+            .into_iter()
+            .map(|plugin| plugin.definition)
+            .collect())
     }
 
     pub fn aggregated_hooks(&self) -> Result<PluginHooks, PluginError> {
@@ -1094,9 +1220,9 @@ impl PluginManager {
         })
     }
 
-    fn discover_installed_plugins(&self) -> Result<Vec<PluginDefinition>, PluginError> {
+    fn discover_installed_plugins_with_failures(&self) -> Result<PluginDiscovery, PluginError> {
         let mut registry = self.load_registry()?;
-        let mut plugins = Vec::new();
+        let mut discovery = PluginDiscovery::default();
         let mut seen_ids = BTreeSet::<String>::new();
         let mut seen_paths = BTreeSet::<PathBuf>::new();
         let mut stale_registry_ids = Vec::new();
@@ -1111,10 +1237,21 @@ impl PluginManager {
                 || install_path.display().to_string(),
                 |record| describe_install_source(&record.source),
             );
-            let plugin = load_plugin_definition(&install_path, kind, source, kind.marketplace())?;
-            if seen_ids.insert(plugin.metadata().id.clone()) {
-                seen_paths.insert(install_path);
-                plugins.push(plugin);
+            match load_plugin_definition(&install_path, kind, source.clone(), kind.marketplace()) {
+                Ok(plugin) => {
+                    if seen_ids.insert(plugin.metadata().id.clone()) {
+                        seen_paths.insert(install_path);
+                        discovery.push_plugin(plugin);
+                    }
+                }
+                Err(error) => {
+                    discovery.push_failure(PluginLoadFailure::new(
+                        install_path,
+                        kind,
+                        source,
+                        error,
+                    ));
+                }
             }
         }
 
@@ -1127,15 +1264,27 @@ impl PluginManager {
                 stale_registry_ids.push(record.id.clone());
                 continue;
             }
-            let plugin = load_plugin_definition(
+            let source = describe_install_source(&record.source);
+            match load_plugin_definition(
                 &record.install_path,
                 record.kind,
-                describe_install_source(&record.source),
+                source.clone(),
                 record.kind.marketplace(),
-            )?;
-            if seen_ids.insert(plugin.metadata().id.clone()) {
-                seen_paths.insert(record.install_path.clone());
-                plugins.push(plugin);
+            ) {
+                Ok(plugin) => {
+                    if seen_ids.insert(plugin.metadata().id.clone()) {
+                        seen_paths.insert(record.install_path.clone());
+                        discovery.push_plugin(plugin);
+                    }
+                }
+                Err(error) => {
+                    discovery.push_failure(PluginLoadFailure::new(
+                        record.install_path.clone(),
+                        record.kind,
+                        source,
+                        error,
+                    ));
+                }
             }
         }
 
@@ -1146,47 +1295,51 @@ impl PluginManager {
             self.store_registry(&registry)?;
         }
 
-        Ok(plugins)
+        Ok(discovery)
     }
 
-    fn discover_external_directory_plugins(
+    fn discover_external_directory_plugins_with_failures(
         &self,
         existing_plugins: &[PluginDefinition],
-    ) -> Result<Vec<PluginDefinition>, PluginError> {
-        let mut plugins = Vec::new();
+    ) -> Result<PluginDiscovery, PluginError> {
+        let mut discovery = PluginDiscovery::default();
 
         for directory in &self.config.external_dirs {
             for root in discover_plugin_dirs(directory)? {
-                let plugin = load_plugin_definition(
+                let source = root.display().to_string();
+                match load_plugin_definition(
                     &root,
                     PluginKind::External,
-                    root.display().to_string(),
+                    source.clone(),
                     EXTERNAL_MARKETPLACE,
-                )?;
-                if existing_plugins
-                    .iter()
-                    .chain(plugins.iter())
-                    .all(|existing| existing.metadata().id != plugin.metadata().id)
-                {
-                    plugins.push(plugin);
+                ) {
+                    Ok(plugin) => {
+                        if existing_plugins
+                            .iter()
+                            .chain(discovery.plugins.iter())
+                            .all(|existing| existing.metadata().id != plugin.metadata().id)
+                        {
+                            discovery.push_plugin(plugin);
+                        }
+                    }
+                    Err(error) => {
+                        discovery.push_failure(PluginLoadFailure::new(
+                            root,
+                            PluginKind::External,
+                            source,
+                            error,
+                        ));
+                    }
                 }
             }
         }
 
-        Ok(plugins)
+        Ok(discovery)
     }
 
-    fn installed_plugin_registry(&self) -> Result<PluginRegistry, PluginError> {
+    pub fn installed_plugin_registry_report(&self) -> Result<PluginRegistryReport, PluginError> {
         self.sync_bundled_plugins()?;
-        Ok(PluginRegistry::new(
-            self.discover_installed_plugins()?
-                .into_iter()
-                .map(|plugin| {
-                    let enabled = self.is_enabled(plugin.metadata());
-                    RegisteredPlugin::new(plugin, enabled)
-                })
-                .collect(),
-        ))
+        Ok(self.build_registry_report(self.discover_installed_plugins_with_failures()?))
     }
 
     fn sync_bundled_plugins(&self) -> Result<(), PluginError> {
@@ -1332,6 +1485,26 @@ impl PluginManager {
             }
         })
     }
+
+    fn installed_plugin_registry(&self) -> Result<PluginRegistry, PluginError> {
+        self.installed_plugin_registry_report()?.into_registry()
+    }
+
+    fn build_registry_report(&self, discovery: PluginDiscovery) -> PluginRegistryReport {
+        PluginRegistryReport::new(
+            PluginRegistry::new(
+                discovery
+                    .plugins
+                    .into_iter()
+                    .map(|plugin| {
+                        let enabled = self.is_enabled(plugin.metadata());
+                        RegisteredPlugin::new(plugin, enabled)
+                    })
+                    .collect(),
+            ),
+            discovery.failures,
+        )
+    }
 }
 
 #[must_use]
@@ -1676,6 +1849,8 @@ fn validate_command_entry(
     };
     if !path.exists() {
         errors.push(PluginManifestValidationError::MissingPath { kind, path });
+    } else if !path.is_file() {
+        errors.push(PluginManifestValidationError::PathIsDirectory { kind, path });
     }
 }
 
@@ -1783,6 +1958,12 @@ fn validate_command_path(root: &Path, entry: &str, kind: &str) -> Result<(), Plu
             path.display()
         )));
     }
+    if !path.is_file() {
+        return Err(PluginError::InvalidManifest(format!(
+            "{kind} path `{}` must point to a file",
+            path.display()
+        )));
+    }
     Ok(())
 }
 
@@ -2094,6 +2275,20 @@ mod tests {
         );
     }
 
+    fn write_directory_path_plugin(root: &Path, name: &str) {
+        fs::create_dir_all(root.join("hooks").join("pre-dir")).expect("hook dir");
+        fs::create_dir_all(root.join("tools").join("tool-dir")).expect("tool dir");
+        fs::create_dir_all(root.join("commands").join("sync-dir")).expect("command dir");
+        fs::create_dir_all(root.join("lifecycle").join("init-dir")).expect("lifecycle dir");
+        write_file(
+            root.join(MANIFEST_FILE_NAME).as_path(),
+            format!(
+                "{{\n  \"name\": \"{name}\",\n  \"version\": \"1.0.0\",\n  \"description\": \"directory path plugin\",\n  \"hooks\": {{\n    \"PreToolUse\": [\"./hooks/pre-dir\"]\n  }},\n  \"lifecycle\": {{\n    \"Init\": [\"./lifecycle/init-dir\"]\n  }},\n  \"tools\": [\n    {{\n      \"name\": \"dir_tool\",\n      \"description\": \"Directory tool\",\n      \"inputSchema\": {{\"type\": \"object\"}},\n      \"command\": \"./tools/tool-dir\"\n    }}\n  ],\n  \"commands\": [\n    {{\n      \"name\": \"sync\",\n      \"description\": \"Directory command\",\n      \"command\": \"./commands/sync-dir\"\n    }}\n  ]\n}}"
+            )
+            .as_str(),
+        );
+    }
+
     fn write_lifecycle_plugin(root: &Path, name: &str, version: &str) -> PathBuf {
         let log_path = root.join("lifecycle.log");
         write_file(
@@ -2315,6 +2510,90 @@ mod tests {
         let _ = fs::remove_dir_all(root);
     }
 
+    #[test]
+    fn load_plugin_from_directory_rejects_missing_lifecycle_paths() {
+        // given
+        let root = temp_dir("manifest-lifecycle-paths");
+        write_file(
+            root.join(MANIFEST_FILE_NAME).as_path(),
+            r#"{
+  "name": "missing-lifecycle-paths",
+  "version": "1.0.0",
+  "description": "Missing lifecycle path validation",
+  "lifecycle": {
+    "Init": ["./lifecycle/init.sh"],
+    "Shutdown": ["./lifecycle/shutdown.sh"]
+  }
+}"#,
+        );
+
+        // when
+        let error =
+            load_plugin_from_directory(&root).expect_err("missing lifecycle paths should fail");
+
+        // then
+        match error {
+            PluginError::ManifestValidation(errors) => {
+                assert!(errors.iter().any(|error| matches!(
+                    error,
+                    PluginManifestValidationError::MissingPath { kind, path }
+                    if *kind == "lifecycle command"
+                        && path.ends_with(Path::new("lifecycle/init.sh"))
+                )));
+                assert!(errors.iter().any(|error| matches!(
+                    error,
+                    PluginManifestValidationError::MissingPath { kind, path }
+                    if *kind == "lifecycle command"
+                        && path.ends_with(Path::new("lifecycle/shutdown.sh"))
+                )));
+            }
+            other => panic!("expected manifest validation errors, got {other}"),
+        }
+
+        let _ = fs::remove_dir_all(root);
+    }
+
+    #[test]
+    fn load_plugin_from_directory_rejects_directory_command_paths() {
+        // given
+        let root = temp_dir("manifest-directory-paths");
+        write_directory_path_plugin(&root, "directory-paths");
+
+        // when
+        let error =
+            load_plugin_from_directory(&root).expect_err("directory command paths should fail");
+
+        // then
+        match error {
+            PluginError::ManifestValidation(errors) => {
+                assert!(errors.iter().any(|error| matches!(
+                    error,
+                    PluginManifestValidationError::PathIsDirectory { kind, path }
+                    if *kind == "hook" && path.ends_with(Path::new("hooks/pre-dir"))
+                )));
+                assert!(errors.iter().any(|error| matches!(
+                    error,
+                    PluginManifestValidationError::PathIsDirectory { kind, path }
+                    if *kind == "lifecycle command"
+                        && path.ends_with(Path::new("lifecycle/init-dir"))
+                )));
+                assert!(errors.iter().any(|error| matches!(
+                    error,
+                    PluginManifestValidationError::PathIsDirectory { kind, path }
+                    if *kind == "tool" && path.ends_with(Path::new("tools/tool-dir"))
+                )));
+                assert!(errors.iter().any(|error| matches!(
+                    error,
+                    PluginManifestValidationError::PathIsDirectory { kind, path }
+                    if *kind == "command" && path.ends_with(Path::new("commands/sync-dir"))
+                )));
+            }
+            other => panic!("expected manifest validation errors, got {other}"),
+        }
+
+        let _ = fs::remove_dir_all(root);
+    }
+
     #[test]
     fn load_plugin_from_directory_rejects_invalid_permissions() {
         let root = temp_dir("manifest-invalid-permissions");
@@ -2806,6 +3085,80 @@ mod tests {
         let _ = fs::remove_dir_all(source_root);
     }
 
+    #[test]
+    fn plugin_registry_report_collects_load_failures_without_dropping_valid_plugins() {
+        // given
+        let config_home = temp_dir("report-home");
+        let external_root = temp_dir("report-external");
+        write_external_plugin(&external_root.join("valid"), "valid-report", "1.0.0");
+        write_broken_plugin(&external_root.join("broken"), "broken-report");
+
+        let mut config = PluginManagerConfig::new(&config_home);
+        config.external_dirs = vec![external_root.clone()];
+        let manager = PluginManager::new(config);
+
+        // when
+        let report = manager
+            .plugin_registry_report()
+            .expect("report should tolerate invalid external plugins");
+
+        // then
+        assert!(report.registry().contains("valid-report@external"));
+        assert_eq!(report.failures().len(), 1);
+        assert_eq!(report.failures()[0].kind, PluginKind::External);
+        assert!(report.failures()[0]
+            .plugin_root
+            .ends_with(Path::new("broken")));
+        assert!(report.failures()[0]
+            .error()
+            .to_string()
+            .contains("does not exist"));
+
+        let error = manager
+            .plugin_registry()
+            .expect_err("strict registry should surface load failures");
+        match error {
+            PluginError::LoadFailures(failures) => {
+                assert_eq!(failures.len(), 1);
+                assert!(failures[0].plugin_root.ends_with(Path::new("broken")));
+            }
+            other => panic!("expected load failures, got {other}"),
+        }
+
+        let _ = fs::remove_dir_all(config_home);
+        let _ = fs::remove_dir_all(external_root);
+    }
+
+    #[test]
+    fn installed_plugin_registry_report_collects_load_failures_from_install_root() {
+        // given
+        let config_home = temp_dir("installed-report-home");
+        let bundled_root = temp_dir("installed-report-bundled");
+        let install_root = config_home.join("plugins").join("installed");
+        write_external_plugin(&install_root.join("valid"), "installed-valid", "1.0.0");
+        write_broken_plugin(&install_root.join("broken"), "installed-broken");
+
+        let mut config = PluginManagerConfig::new(&config_home);
+        config.bundled_root = Some(bundled_root.clone());
+        config.install_root = Some(install_root);
+        let manager = PluginManager::new(config);
+
+        // when
+        let report = manager
+            .installed_plugin_registry_report()
+            .expect("installed report should tolerate invalid installed plugins");
+
+        // then
+        assert!(report.registry().contains("installed-valid@external"));
+        assert_eq!(report.failures().len(), 1);
+        assert!(report.failures()[0]
+            .plugin_root
+            .ends_with(Path::new("broken")));
+
+        let _ = fs::remove_dir_all(config_home);
+        let _ = fs::remove_dir_all(bundled_root);
+    }
+
     #[test]
     fn rejects_plugin_sources_with_missing_hook_paths() {
         let config_home = temp_dir("broken-home");