diff --git a/rust/crates/plugins/src/lib.rs b/rust/crates/plugins/src/lib.rs index 321070a..6c7c0bc 100644 --- a/rust/crates/plugins/src/lib.rs +++ b/rust/crates/plugins/src/lib.rs @@ -655,6 +655,106 @@ pub struct PluginSummary { pub enabled: bool, } +#[derive(Debug)] +pub struct PluginLoadFailure { + pub plugin_root: PathBuf, + pub kind: PluginKind, + pub source: String, + error: Box, +} + +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, +} + +impl PluginRegistryReport { + #[must_use] + pub fn new(registry: PluginRegistry, failures: Vec) -> 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 { + self.registry.summaries() + } + + pub fn into_registry(self) -> Result { + if self.failures.is_empty() { + Ok(self.registry) + } else { + Err(PluginError::LoadFailures(self.failures)) + } + } +} + +#[derive(Debug, Default)] +struct PluginDiscovery { + plugins: Vec, + failures: Vec, +} + +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, @@ -809,6 +909,10 @@ pub enum PluginManifestValidationError { kind: &'static str, path: PathBuf, }, + PathIsDirectory { + kind: &'static str, + path: PathBuf, + }, InvalidToolInputSchema { tool_name: String, }, @@ -845,6 +949,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, @@ -867,6 +974,7 @@ pub enum PluginError { Io(std::io::Error), Json(serde_json::Error), ManifestValidation(Vec), + LoadFailures(Vec), InvalidManifest(String), NotFound(String), CommandFailed(String), @@ -886,6 +994,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}"), @@ -942,15 +1059,23 @@ impl PluginManager { } pub fn plugin_registry(&self) -> Result { - 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 { + 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, PluginError> { @@ -962,11 +1087,12 @@ impl PluginManager { } pub fn discover_plugins(&self) -> Result, 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 { @@ -1101,9 +1227,9 @@ impl PluginManager { }) } - fn discover_installed_plugins(&self) -> Result, PluginError> { + fn discover_installed_plugins_with_failures(&self) -> Result { let mut registry = self.load_registry()?; - let mut plugins = Vec::new(); + let mut discovery = PluginDiscovery::default(); let mut seen_ids = BTreeSet::::new(); let mut seen_paths = BTreeSet::::new(); let mut stale_registry_ids = Vec::new(); @@ -1118,10 +1244,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, + )); + } } } @@ -1134,15 +1271,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, + )); + } } } @@ -1153,47 +1302,51 @@ impl PluginManager { self.store_registry(®istry)?; } - Ok(plugins) + Ok(discovery) } - fn discover_external_directory_plugins( + fn discover_external_directory_plugins_with_failures( &self, existing_plugins: &[PluginDefinition], - ) -> Result, PluginError> { - let mut plugins = Vec::new(); + ) -> Result { + 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 { + pub fn installed_plugin_registry_report(&self) -> Result { 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> { @@ -1339,6 +1492,26 @@ impl PluginManager { } }) } + + fn installed_plugin_registry(&self) -> Result { + 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] @@ -1683,6 +1856,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 }); } } @@ -1800,6 +1975,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(()) } @@ -2111,6 +2292,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( @@ -2332,6 +2527,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"); @@ -2823,6 +3102,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");