From 13feef9cee125ebd8d29b1ff69cde7e640878a46 Mon Sep 17 00:00:00 2001 From: xj Date: Sun, 1 Mar 2026 02:34:04 -0800 Subject: [PATCH] fix(plugins): address copilot review follow-ups --- src/agent/loop_.rs | 5 +- src/channels/mod.rs | 2 +- src/gateway/mod.rs | 2 +- src/hooks/mod.rs | 6 +++ src/plugins/manifest.rs | 102 ++++++++++++++++++++++++++++++++++++++++ src/plugins/registry.rs | 6 +++ src/plugins/runtime.rs | 2 +- 7 files changed, 119 insertions(+), 6 deletions(-) diff --git a/src/agent/loop_.rs b/src/agent/loop_.rs index 345506361..58125c253 100644 --- a/src/agent/loop_.rs +++ b/src/agent/loop_.rs @@ -2452,9 +2452,8 @@ pub async fn run( } system_prompt.push_str(&build_shell_policy_instructions(&config.autonomy)); - let configured_hooks = - crate::hooks::HookRunner::from_config(&config.hooks).map(std::sync::Arc::new); - let effective_hooks = hooks.or_else(|| configured_hooks.as_deref()); + let configured_hooks = crate::hooks::create_runner_from_config(&config.hooks); + let effective_hooks = hooks.or(configured_hooks.as_deref()); // ── Approval manager (supervised mode) ─────────────────────── let approval_manager = if interactive { diff --git a/src/channels/mod.rs b/src/channels/mod.rs index 0cc10585f..442550825 100644 --- a/src/channels/mod.rs +++ b/src/channels/mod.rs @@ -5731,7 +5731,7 @@ pub async fn start_channels(config: Config) -> Result<()> { message_timeout_secs, interrupt_on_new_message, multimodal: config.multimodal.clone(), - hooks: crate::hooks::HookRunner::from_config(&config.hooks).map(Arc::new), + hooks: crate::hooks::create_runner_from_config(&config.hooks), non_cli_excluded_tools: Arc::new(Mutex::new( config.autonomy.non_cli_excluded_tools.clone(), )), diff --git a/src/gateway/mod.rs b/src/gateway/mod.rs index 3098d8813..b6fa6472c 100644 --- a/src/gateway/mod.rs +++ b/src/gateway/mod.rs @@ -392,7 +392,7 @@ pub async fn run_gateway(host: &str, port: u16, config: Config) -> Result<()> { let config_state = Arc::new(Mutex::new(config.clone())); // ── Hooks ────────────────────────────────────────────────────── - let hooks = crate::hooks::HookRunner::from_config(&config.hooks).map(std::sync::Arc::new); + let hooks = crate::hooks::create_runner_from_config(&config.hooks); let addr: SocketAddr = format!("{host}:{port}").parse()?; let listener = tokio::net::TcpListener::bind(addr).await?; diff --git a/src/hooks/mod.rs b/src/hooks/mod.rs index e7f7c5817..bdfeb1a2e 100644 --- a/src/hooks/mod.rs +++ b/src/hooks/mod.rs @@ -8,3 +8,9 @@ pub use runner::HookRunner; // external integrations and future plugin authors. #[allow(unused_imports)] pub use traits::{HookHandler, HookResult}; + +pub fn create_runner_from_config( + config: &crate::config::HooksConfig, +) -> Option> { + HookRunner::from_config(config).map(std::sync::Arc::new) +} diff --git a/src/plugins/manifest.rs b/src/plugins/manifest.rs index fe6ebb8a4..8a8c14612 100644 --- a/src/plugins/manifest.rs +++ b/src/plugins/manifest.rs @@ -5,6 +5,7 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; +use std::collections::HashSet; use std::fs; use std::path::Path; @@ -129,6 +130,14 @@ fn parse_wit_package_version(input: &str) -> anyhow::Result<(&str, u64)> { Ok((package, major)) } +fn required_wit_package_for_capability(capability: &PluginCapability) -> &'static str { + match capability { + PluginCapability::Hooks | PluginCapability::ModifyToolResults => "zeroclaw:hooks", + PluginCapability::Tools => "zeroclaw:tools", + PluginCapability::Providers => "zeroclaw:providers", + } +} + pub fn validate_manifest(manifest: &PluginManifest) -> anyhow::Result<()> { if manifest.id.trim().is_empty() { anyhow::bail!("plugin id cannot be empty"); @@ -141,6 +150,10 @@ pub fn validate_manifest(manifest: &PluginManifest) -> anyhow::Result<()> { if manifest.module_path.trim().is_empty() { anyhow::bail!("plugin module_path cannot be empty"); } + if manifest.module_path.trim().is_empty() { + anyhow::bail!("plugin module_path cannot be empty"); + } + let mut declared_wit_packages = HashSet::new(); for wit_pkg in &manifest.wit_packages { let (package, major) = parse_wit_package_version(wit_pkg)?; if !SUPPORTED_WIT_PACKAGES.contains(&package) { @@ -151,6 +164,32 @@ pub fn validate_manifest(manifest: &PluginManifest) -> anyhow::Result<()> { "incompatible wit major version for '{package}': expected {SUPPORTED_WIT_MAJOR}, got {major}" ); } + declared_wit_packages.insert(package.to_string()); + } + if manifest + .capabilities + .contains(&PluginCapability::ModifyToolResults) + && !manifest.capabilities.contains(&PluginCapability::Hooks) + { + anyhow::bail!( + "plugin capability 'ModifyToolResults' requires declaring 'Hooks' capability" + ); + } + for capability in &manifest.capabilities { + let required_package = required_wit_package_for_capability(capability); + if !declared_wit_packages.contains(required_package) { + anyhow::bail!( + "plugin capability '{capability:?}' requires wit package '{required_package}@{SUPPORTED_WIT_MAJOR}.x'" + ); + } + } + if !manifest.tools.is_empty() && !declared_wit_packages.contains("zeroclaw:tools") { + anyhow::bail!("plugin tools require wit package 'zeroclaw:tools@{SUPPORTED_WIT_MAJOR}.x'"); + } + if !manifest.providers.is_empty() && !declared_wit_packages.contains("zeroclaw:providers") { + anyhow::bail!( + "plugin providers require wit package 'zeroclaw:providers@{SUPPORTED_WIT_MAJOR}.x'" + ); } for tool in &manifest.tools { if tool.name.trim().is_empty() { @@ -289,4 +328,67 @@ id = " " }; assert!(validate_manifest(&manifest).is_err()); } + + #[test] + fn manifest_rejects_empty_module_path() { + let manifest = PluginManifest { + id: "demo".into(), + version: "1.0.0".into(), + capabilities: vec![], + module_path: " ".into(), + wit_packages: vec!["zeroclaw:hooks@1.0.0".into()], + tools: vec![], + providers: vec![], + }; + assert!(validate_manifest(&manifest).is_err()); + } + + #[test] + fn manifest_rejects_capability_without_matching_wit_package() { + let manifest = PluginManifest { + id: "demo".into(), + version: "1.0.0".into(), + capabilities: vec![PluginCapability::Tools], + module_path: "plugins/demo.wasm".into(), + wit_packages: vec!["zeroclaw:hooks@1.0.0".into()], + tools: vec![], + providers: vec![], + }; + assert!(validate_manifest(&manifest).is_err()); + } + + #[test] + fn manifest_rejects_modify_tool_results_without_hooks_capability() { + let manifest = PluginManifest { + id: "demo".into(), + version: "1.0.0".into(), + capabilities: vec![PluginCapability::ModifyToolResults], + module_path: "plugins/demo.wasm".into(), + wit_packages: vec!["zeroclaw:hooks@1.0.0".into()], + tools: vec![], + providers: vec![], + }; + assert!(validate_manifest(&manifest).is_err()); + } + + #[test] + fn manifest_rejects_tools_without_tools_wit_package() { + let manifest = PluginManifest { + id: "demo".into(), + version: "1.0.0".into(), + capabilities: vec![], + module_path: "plugins/demo.wasm".into(), + wit_packages: vec!["zeroclaw:hooks@1.0.0".into()], + tools: vec![PluginToolManifest { + name: "demo_tool".into(), + description: "demo tool".into(), + parameters: serde_json::json!({ + "type": "object", + "properties": {} + }), + }], + providers: vec![], + }; + assert!(validate_manifest(&manifest).is_err()); + } } diff --git a/src/plugins/registry.rs b/src/plugins/registry.rs index a56cf6862..ec5e50f77 100644 --- a/src/plugins/registry.rs +++ b/src/plugins/registry.rs @@ -133,6 +133,10 @@ impl PluginRegistry { self.manifests.len() } + pub fn is_empty(&self) -> bool { + self.manifests.is_empty() + } + pub fn tools(&self) -> &[PluginToolManifest] { &self.manifest_tools } @@ -228,6 +232,7 @@ mod tests { fn empty_registry() { let reg = PluginRegistry::new(); assert_eq!(reg.active_count(), 0); + assert!(reg.is_empty()); assert!(reg.plugins.is_empty()); assert!(reg.tools.is_empty()); assert!(reg.tools().is_empty()); @@ -285,6 +290,7 @@ mod tests { "provider_v2_for_replace_test", )); + assert!(!reg.is_empty()); assert_eq!(reg.len(), 1); assert_eq!(reg.tools().len(), 1); assert_eq!(reg.tools()[0].name, "tool_v2"); diff --git a/src/plugins/runtime.rs b/src/plugins/runtime.rs index 2d44d1877..ce55d937b 100644 --- a/src/plugins/runtime.rs +++ b/src/plugins/runtime.rs @@ -479,7 +479,7 @@ mod tests { id = "{id}" version = "1.0.0" module_path = "plugins/{id}.wasm" -wit_packages = ["zeroclaw:tools@1.0.0"] +wit_packages = ["zeroclaw:tools@1.0.0", "zeroclaw:providers@1.0.0"] providers = ["{provider}"] [[tools]]