feat: plugin subsystem progress

rust/crates/plugins/src/hooks.rs

@@ -0,0 +1,387 @@
+use std::ffi::OsStr;
+use std::path::Path;
+use std::process::Command;
+
+use serde_json::json;
+
+use crate::{PluginError, PluginHooks, PluginRegistry};
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum HookEvent {
+    PreToolUse,
+    PostToolUse,
+}
+
+impl HookEvent {
+    fn as_str(self) -> &'static str {
+        match self {
+            Self::PreToolUse => "PreToolUse",
+            Self::PostToolUse => "PostToolUse",
+        }
+    }
+}
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct HookRunResult {
+    denied: bool,
+    messages: Vec<String>,
+}
+
+impl HookRunResult {
+    #[must_use]
+    pub fn allow(messages: Vec<String>) -> Self {
+        Self {
+            denied: false,
+            messages,
+        }
+    }
+
+    #[must_use]
+    pub fn is_denied(&self) -> bool {
+        self.denied
+    }
+
+    #[must_use]
+    pub fn messages(&self) -> &[String] {
+        &self.messages
+    }
+}
+
+#[derive(Debug, Clone, PartialEq, Eq, Default)]
+pub struct HookRunner {
+    hooks: PluginHooks,
+}
+
+impl HookRunner {
+    #[must_use]
+    pub fn new(hooks: PluginHooks) -> Self {
+        Self { hooks }
+    }
+
+    pub fn from_registry(plugin_registry: &PluginRegistry) -> Result<Self, PluginError> {
+        Ok(Self::new(plugin_registry.aggregated_hooks()?))
+    }
+
+    #[must_use]
+    pub fn run_pre_tool_use(&self, tool_name: &str, tool_input: &str) -> HookRunResult {
+        self.run_commands(
+            HookEvent::PreToolUse,
+            &self.hooks.pre_tool_use,
+            tool_name,
+            tool_input,
+            None,
+            false,
+        )
+    }
+
+    #[must_use]
+    pub fn run_post_tool_use(
+        &self,
+        tool_name: &str,
+        tool_input: &str,
+        tool_output: &str,
+        is_error: bool,
+    ) -> HookRunResult {
+        self.run_commands(
+            HookEvent::PostToolUse,
+            &self.hooks.post_tool_use,
+            tool_name,
+            tool_input,
+            Some(tool_output),
+            is_error,
+        )
+    }
+
+    fn run_commands(
+        &self,
+        event: HookEvent,
+        commands: &[String],
+        tool_name: &str,
+        tool_input: &str,
+        tool_output: Option<&str>,
+        is_error: bool,
+    ) -> HookRunResult {
+        if commands.is_empty() {
+            return HookRunResult::allow(Vec::new());
+        }
+
+        let payload = json!({
+            "hook_event_name": event.as_str(),
+            "tool_name": tool_name,
+            "tool_input": parse_tool_input(tool_input),
+            "tool_input_json": tool_input,
+            "tool_output": tool_output,
+            "tool_result_is_error": is_error,
+        })
+        .to_string();
+
+        let mut messages = Vec::new();
+
+        for command in commands {
+            match self.run_command(
+                command,
+                event,
+                tool_name,
+                tool_input,
+                tool_output,
+                is_error,
+                &payload,
+            ) {
+                HookCommandOutcome::Allow { message } => {
+                    if let Some(message) = message {
+                        messages.push(message);
+                    }
+                }
+                HookCommandOutcome::Deny { message } => {
+                    messages.push(
+                        message.unwrap_or_else(|| {
+                            format!("{} hook denied tool `{tool_name}`", event.as_str())
+                        }),
+                    );
+                    return HookRunResult {
+                        denied: true,
+                        messages,
+                    };
+                }
+                HookCommandOutcome::Warn { message } => messages.push(message),
+            }
+        }
+
+        HookRunResult::allow(messages)
+    }
+
+    #[allow(clippy::too_many_arguments)]
+    fn run_command(
+        &self,
+        command: &str,
+        event: HookEvent,
+        tool_name: &str,
+        tool_input: &str,
+        tool_output: Option<&str>,
+        is_error: bool,
+        payload: &str,
+    ) -> HookCommandOutcome {
+        let mut child = shell_command(command);
+        child.stdin(std::process::Stdio::piped());
+        child.stdout(std::process::Stdio::piped());
+        child.stderr(std::process::Stdio::piped());
+        child.env("HOOK_EVENT", event.as_str());
+        child.env("HOOK_TOOL_NAME", tool_name);
+        child.env("HOOK_TOOL_INPUT", tool_input);
+        child.env("HOOK_TOOL_IS_ERROR", if is_error { "1" } else { "0" });
+        if let Some(tool_output) = tool_output {
+            child.env("HOOK_TOOL_OUTPUT", tool_output);
+        }
+
+        match child.output_with_stdin(payload.as_bytes()) {
+            Ok(output) => {
+                let stdout = String::from_utf8_lossy(&output.stdout).trim().to_string();
+                let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string();
+                let message = (!stdout.is_empty()).then_some(stdout);
+                match output.status.code() {
+                    Some(0) => HookCommandOutcome::Allow { message },
+                    Some(2) => HookCommandOutcome::Deny { message },
+                    Some(code) => HookCommandOutcome::Warn {
+                        message: format_hook_warning(
+                            command,
+                            code,
+                            message.as_deref(),
+                            stderr.as_str(),
+                        ),
+                    },
+                    None => HookCommandOutcome::Warn {
+                        message: format!(
+                            "{} hook `{command}` terminated by signal while handling `{tool_name}`",
+                            event.as_str()
+                        ),
+                    },
+                }
+            }
+            Err(error) => HookCommandOutcome::Warn {
+                message: format!(
+                    "{} hook `{command}` failed to start for `{tool_name}`: {error}",
+                    event.as_str()
+                ),
+            },
+        }
+    }
+}
+
+enum HookCommandOutcome {
+    Allow { message: Option<String> },
+    Deny { message: Option<String> },
+    Warn { message: String },
+}
+
+fn parse_tool_input(tool_input: &str) -> serde_json::Value {
+    serde_json::from_str(tool_input).unwrap_or_else(|_| json!({ "raw": tool_input }))
+}
+
+fn format_hook_warning(command: &str, code: i32, stdout: Option<&str>, stderr: &str) -> String {
+    let mut message =
+        format!("Hook `{command}` exited with status {code}; allowing tool execution to continue");
+    if let Some(stdout) = stdout.filter(|stdout| !stdout.is_empty()) {
+        message.push_str(": ");
+        message.push_str(stdout);
+    } else if !stderr.is_empty() {
+        message.push_str(": ");
+        message.push_str(stderr);
+    }
+    message
+}
+
+fn shell_command(command: &str) -> CommandWithStdin {
+    #[cfg(windows)]
+    let command_builder = {
+        let mut command_builder = Command::new("cmd");
+        command_builder.arg("/C").arg(command);
+        CommandWithStdin::new(command_builder)
+    };
+
+    #[cfg(not(windows))]
+    let command_builder = if Path::new(command).exists() {
+        let mut command_builder = Command::new("sh");
+        command_builder.arg(command);
+        CommandWithStdin::new(command_builder)
+    } else {
+        let mut command_builder = Command::new("sh");
+        command_builder.arg("-lc").arg(command);
+        CommandWithStdin::new(command_builder)
+    };
+
+    command_builder
+}
+
+struct CommandWithStdin {
+    command: Command,
+}
+
+impl CommandWithStdin {
+    fn new(command: Command) -> Self {
+        Self { command }
+    }
+
+    fn stdin(&mut self, cfg: std::process::Stdio) -> &mut Self {
+        self.command.stdin(cfg);
+        self
+    }
+
+    fn stdout(&mut self, cfg: std::process::Stdio) -> &mut Self {
+        self.command.stdout(cfg);
+        self
+    }
+
+    fn stderr(&mut self, cfg: std::process::Stdio) -> &mut Self {
+        self.command.stderr(cfg);
+        self
+    }
+
+    fn env<K, V>(&mut self, key: K, value: V) -> &mut Self
+    where
+        K: AsRef<OsStr>,
+        V: AsRef<OsStr>,
+    {
+        self.command.env(key, value);
+        self
+    }
+
+    fn output_with_stdin(&mut self, stdin: &[u8]) -> std::io::Result<std::process::Output> {
+        let mut child = self.command.spawn()?;
+        if let Some(mut child_stdin) = child.stdin.take() {
+            use std::io::Write as _;
+            child_stdin.write_all(stdin)?;
+        }
+        child.wait_with_output()
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::{HookRunResult, HookRunner};
+    use crate::{PluginManager, PluginManagerConfig};
+    use std::fs;
+    use std::path::{Path, PathBuf};
+    use std::time::{SystemTime, UNIX_EPOCH};
+
+    fn temp_dir(label: &str) -> PathBuf {
+        let nanos = SystemTime::now()
+            .duration_since(UNIX_EPOCH)
+            .expect("time should be after epoch")
+            .as_nanos();
+        std::env::temp_dir().join(format!("plugins-hook-runner-{label}-{nanos}"))
+    }
+
+    fn write_hook_plugin(root: &Path, name: &str, pre_message: &str, post_message: &str) {
+        fs::create_dir_all(root.join(".claude-plugin")).expect("manifest dir");
+        fs::create_dir_all(root.join("hooks")).expect("hooks dir");
+        fs::write(
+            root.join("hooks").join("pre.sh"),
+            format!("#!/bin/sh\nprintf '%s\\n' '{pre_message}'\n"),
+        )
+        .expect("write pre hook");
+        fs::write(
+            root.join("hooks").join("post.sh"),
+            format!("#!/bin/sh\nprintf '%s\\n' '{post_message}'\n"),
+        )
+        .expect("write post hook");
+        fs::write(
+            root.join(".claude-plugin").join("plugin.json"),
+            format!(
+                "{{\n  \"name\": \"{name}\",\n  \"version\": \"1.0.0\",\n  \"description\": \"hook plugin\",\n  \"hooks\": {{\n    \"PreToolUse\": [\"./hooks/pre.sh\"],\n    \"PostToolUse\": [\"./hooks/post.sh\"]\n  }}\n}}"
+            ),
+        )
+        .expect("write plugin manifest");
+    }
+
+    #[test]
+    fn collects_and_runs_hooks_from_enabled_plugins() {
+        let config_home = temp_dir("config");
+        let first_source_root = temp_dir("source-a");
+        let second_source_root = temp_dir("source-b");
+        write_hook_plugin(&first_source_root, "first", "plugin pre one", "plugin post one");
+        write_hook_plugin(&second_source_root, "second", "plugin pre two", "plugin post two");
+
+        let mut manager = PluginManager::new(PluginManagerConfig::new(&config_home));
+        manager
+            .install(first_source_root.to_str().expect("utf8 path"))
+            .expect("first plugin install should succeed");
+        manager
+            .install(second_source_root.to_str().expect("utf8 path"))
+            .expect("second plugin install should succeed");
+        let registry = manager.plugin_registry().expect("registry should build");
+
+        let runner = HookRunner::from_registry(&registry).expect("plugin hooks should load");
+
+        assert_eq!(
+            runner.run_pre_tool_use("Read", r#"{"path":"README.md"}"#),
+            HookRunResult::allow(vec![
+                "plugin pre one".to_string(),
+                "plugin pre two".to_string(),
+            ])
+        );
+        assert_eq!(
+            runner.run_post_tool_use("Read", r#"{"path":"README.md"}"#, "ok", false),
+            HookRunResult::allow(vec![
+                "plugin post one".to_string(),
+                "plugin post two".to_string(),
+            ])
+        );
+
+        let _ = fs::remove_dir_all(config_home);
+        let _ = fs::remove_dir_all(first_source_root);
+        let _ = fs::remove_dir_all(second_source_root);
+    }
+
+    #[test]
+    fn pre_tool_use_denies_when_plugin_hook_exits_two() {
+        let runner = HookRunner::new(crate::PluginHooks {
+            pre_tool_use: vec!["printf 'blocked by plugin'; exit 2".to_string()],
+            post_tool_use: Vec::new(),
+        });
+
+        let result = runner.run_pre_tool_use("Bash", r#"{"command":"pwd"}"#);
+
+        assert!(result.is_denied());
+        assert_eq!(result.messages(), &["blocked by plugin".to_string()]);
+    }
+}

rust/crates/plugins/src/lib.rs

@@ -1,3 +1,5 @@
+mod hooks;
+
 use std::collections::BTreeMap;
 use std::fmt::{Display, Formatter};
 use std::fs;
@@ -8,6 +10,8 @@ use std::time::{SystemTime, UNIX_EPOCH};
 use serde::{Deserialize, Serialize};
 use serde_json::{Map, Value};
 
+pub use hooks::{HookEvent, HookRunResult, HookRunner};
+
 const EXTERNAL_MARKETPLACE: &str = "external";
 const BUILTIN_MARKETPLACE: &str = "builtin";
 const BUNDLED_MARKETPLACE: &str = "bundled";
@@ -15,7 +19,6 @@ const SETTINGS_FILE_NAME: &str = "settings.json";
 const REGISTRY_FILE_NAME: &str = "installed.json";
 const MANIFEST_FILE_NAME: &str = "plugin.json";
 const MANIFEST_RELATIVE_PATH: &str = ".claude-plugin/plugin.json";
-const PACKAGE_MANIFEST_RELATIVE_PATH: &str = MANIFEST_RELATIVE_PATH;
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
 #[serde(rename_all = "lowercase")]
@@ -35,6 +38,18 @@ impl Display for PluginKind {
     }
 }
 
+
+impl PluginKind {
+    #[must_use]
+    fn marketplace(self) -> &'static str {
+        match self {
+            Self::Builtin => BUILTIN_MARKETPLACE,
+            Self::Bundled => BUNDLED_MARKETPLACE,
+            Self::External => EXTERNAL_MARKETPLACE,
+        }
+    }
+}
+
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct PluginMetadata {
     pub id: String,
@@ -244,6 +259,8 @@ pub enum PluginInstallSource {
 
 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
 pub struct InstalledPluginRecord {
+    #[serde(default = "default_plugin_kind")]
+    pub kind: PluginKind,
     pub id: String,
     pub name: String,
     pub version: String,
@@ -260,6 +277,10 @@ pub struct InstalledPluginRegistry {
     pub plugins: BTreeMap<String, InstalledPluginRecord>,
 }
 
+fn default_plugin_kind() -> PluginKind {
+    PluginKind::External
+}
+
 #[derive(Debug, Clone, PartialEq)]
 pub struct BuiltinPlugin {
     metadata: PluginMetadata,
@@ -750,10 +771,15 @@ impl PluginManager {
         Ok(self.plugin_registry()?.summaries())
     }
 
+    pub fn list_installed_plugins(&self) -> Result<Vec<PluginSummary>, PluginError> {
+        Ok(self.installed_plugin_registry()?.summaries())
+    }
+
     pub fn discover_plugins(&self) -> Result<Vec<PluginDefinition>, PluginError> {
+        self.sync_bundled_plugins()?;
         let mut plugins = builtin_plugins();
-        plugins.extend(self.discover_bundled_plugins()?);
-        plugins.extend(self.discover_external_plugins()?);
+        plugins.extend(self.discover_installed_plugins()?);
+        plugins.extend(self.discover_external_directory_plugins(&plugins)?);
         Ok(plugins)
     }
 
@@ -761,6 +787,10 @@ impl PluginManager {
         self.plugin_registry()?.aggregated_hooks()
     }
 
+    pub fn aggregated_tools(&self) -> Result<Vec<PluginTool>, PluginError> {
+        self.plugin_registry()?.aggregated_tools()
+    }
+
     pub fn validate_plugin_source(&self, source: &str) -> Result<PluginManifest, PluginError> {
         let path = resolve_local_source(source)?;
         load_plugin_from_directory(&path)
@@ -785,6 +815,7 @@ impl PluginManager {
 
         let now = unix_time_ms();
         let record = InstalledPluginRecord {
+            kind: PluginKind::External,
             id: plugin_id.clone(),
             name: manifest.name,
             version: manifest.version.clone(),
@@ -831,6 +862,12 @@ impl PluginManager {
         let record = registry.plugins.remove(plugin_id).ok_or_else(|| {
             PluginError::NotFound(format!("plugin `{plugin_id}` is not installed"))
         })?;
+        if record.kind == PluginKind::Bundled {
+            registry.plugins.insert(plugin_id.to_string(), record);
+            return Err(PluginError::CommandFailed(format!(
+                "plugin `{plugin_id}` is bundled and managed automatically; disable it instead"
+            )));
+        }
         if record.install_path.exists() {
             fs::remove_dir_all(&record.install_path)?;
         }
@@ -878,40 +915,27 @@ impl PluginManager {
         })
     }
 
-    fn discover_bundled_plugins(&self) -> Result<Vec<PluginDefinition>, PluginError> {
-        discover_plugin_dirs(
-            &self
-                .config
-                .bundled_root
-                .clone()
-                .unwrap_or_else(Self::bundled_root),
-        )?
-        .into_iter()
-        .map(|root| {
-            load_plugin_definition(
-                &root,
-                PluginKind::Bundled,
-                format!("{BUNDLED_MARKETPLACE}:{}", root.display()),
-                BUNDLED_MARKETPLACE,
-            )
-        })
-        .collect()
-    }
-
-    fn discover_external_plugins(&self) -> Result<Vec<PluginDefinition>, PluginError> {
+    fn discover_installed_plugins(&self) -> Result<Vec<PluginDefinition>, PluginError> {
         let registry = self.load_registry()?;
-        let mut plugins = registry
+        registry
             .plugins
             .values()
             .map(|record| {
                 load_plugin_definition(
                     &record.install_path,
-                    PluginKind::External,
+                    record.kind,
                     describe_install_source(&record.source),
-                    EXTERNAL_MARKETPLACE,
+                    record.kind.marketplace(),
                 )
             })
-            .collect::<Result<Vec<_>, _>>()?;
+            .collect()
+    }
+
+    fn discover_external_directory_plugins(
+        &self,
+        existing_plugins: &[PluginDefinition],
+    ) -> Result<Vec<PluginDefinition>, PluginError> {
+        let mut plugins = Vec::new();
 
         for directory in &self.config.external_dirs {
             for root in discover_plugin_dirs(directory)? {
@@ -921,8 +945,9 @@ impl PluginManager {
                     root.display().to_string(),
                     EXTERNAL_MARKETPLACE,
                 )?;
-                if plugins
+                if existing_plugins
                     .iter()
+                    .chain(plugins.iter())
                     .all(|existing| existing.metadata().id != plugin.metadata().id)
                 {
                     plugins.push(plugin);
@@ -933,6 +958,83 @@ impl PluginManager {
         Ok(plugins)
     }
 
+    fn installed_plugin_registry(&self) -> Result<PluginRegistry, 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(),
+        ))
+    }
+
+    fn sync_bundled_plugins(&self) -> Result<(), PluginError> {
+        let bundled_root = self
+            .config
+            .bundled_root
+            .clone()
+            .unwrap_or_else(Self::bundled_root);
+        let bundled_plugins = discover_plugin_dirs(&bundled_root)?;
+        if bundled_plugins.is_empty() {
+            return Ok(());
+        }
+
+        let mut registry = self.load_registry()?;
+        let mut changed = false;
+        let install_root = self.install_root();
+
+        for source_root in bundled_plugins {
+            let manifest = load_validated_package_manifest_from_root(&source_root)?;
+            let plugin_id = plugin_id(&manifest.name, BUNDLED_MARKETPLACE);
+            let install_path = install_root.join(sanitize_plugin_id(&plugin_id));
+            let now = unix_time_ms();
+            let existing_record = registry.plugins.get(&plugin_id);
+            let needs_sync = existing_record.map_or(true, |record| {
+                record.kind != PluginKind::Bundled
+                    || record.version != manifest.version
+                    || record.name != manifest.name
+                    || record.description != manifest.description
+                    || record.install_path != install_path
+                    || !record.install_path.exists()
+            });
+
+            if !needs_sync {
+                continue;
+            }
+
+            if install_path.exists() {
+                fs::remove_dir_all(&install_path)?;
+            }
+            copy_dir_all(&source_root, &install_path)?;
+
+            let installed_at_unix_ms = existing_record.map_or(now, |record| record.installed_at_unix_ms);
+            registry.plugins.insert(
+                plugin_id.clone(),
+                InstalledPluginRecord {
+                    kind: PluginKind::Bundled,
+                    id: plugin_id,
+                    name: manifest.name,
+                    version: manifest.version,
+                    description: manifest.description,
+                    install_path,
+                    source: PluginInstallSource::LocalPath { path: source_root },
+                    installed_at_unix_ms,
+                    updated_at_unix_ms: now,
+                },
+            );
+            changed = true;
+        }
+
+        if changed {
+            self.store_registry(&registry)?;
+        }
+
+        Ok(())
+    }
+
     fn is_enabled(&self, metadata: &PluginMetadata) -> bool {
         self.config
             .enabled_plugins
@@ -1089,11 +1191,15 @@ fn validate_plugin_manifest(root: &Path, manifest: &PluginManifest) -> Result<()
     validate_named_strings(&manifest.permissions, "permission")?;
     validate_hook_paths(Some(root), &manifest.hooks)?;
     validate_named_commands(root, &manifest.tools, "tool")?;
+    validate_tool_manifest_entries(&manifest.tools)?;
     validate_named_commands(root, &manifest.commands, "command")?;
     Ok(())
 }
 
-fn validate_package_manifest(root: &Path, manifest: &PluginPackageManifest) -> Result<(), PluginError> {
+fn validate_package_manifest(
+    root: &Path,
+    manifest: &PluginPackageManifest,
+) -> Result<(), PluginError> {
     if manifest.name.trim().is_empty() {
         return Err(PluginError::InvalidManifest(
             "plugin manifest name cannot be empty".to_string(),
@@ -1110,6 +1216,7 @@ fn validate_package_manifest(root: &Path, manifest: &PluginPackageManifest) -> R
         ));
     }
     validate_named_commands(root, &manifest.tools, "tool")?;
+    validate_tool_manifest_entries(&manifest.tools)?;
     Ok(())
 }
 
@@ -1204,6 +1311,27 @@ fn validate_named_commands(
     Ok(())
 }
 
+fn validate_tool_manifest_entries(entries: &[PluginToolManifest]) -> Result<(), PluginError> {
+    for entry in entries {
+        if !entry.input_schema.is_object() {
+            return Err(PluginError::InvalidManifest(format!(
+                "plugin tool `{}` inputSchema must be a JSON object",
+                entry.name
+            )));
+        }
+        if !matches!(
+            entry.required_permission.as_str(),
+            "read-only" | "workspace-write" | "danger-full-access"
+        ) {
+            return Err(PluginError::InvalidManifest(format!(
+                "plugin tool `{}` requiredPermission must be read-only, workspace-write, or danger-full-access",
+                entry.name
+            )));
+        }
+    }
+    Ok(())
+}
+
 trait NamedCommand {
     fn name(&self) -> &str;
     fn description(&self) -> &str;
@@ -1568,75 +1696,225 @@ mod tests {
         std::env::temp_dir().join(format!("plugins-{label}-{}", unix_time_ms()))
     }
 
-    fn write_external_plugin(root: &Path, name: &str, version: &str) {
-        fs::create_dir_all(root.join(".claude-plugin")).expect("manifest dir");
-        fs::create_dir_all(root.join("hooks")).expect("hooks dir");
-        fs::write(
-            root.join("hooks").join("pre.sh"),
+    fn write_file(path: &Path, contents: &str) {
+        if let Some(parent) = path.parent() {
+            fs::create_dir_all(parent).expect("parent dir");
+        }
+        fs::write(path, contents).expect("write file");
+    }
+
+    fn write_loader_plugin(root: &Path) {
+        write_file(
+            root.join("hooks").join("pre.sh").as_path(),
             "#!/bin/sh\nprintf 'pre'\n",
-        )
-        .expect("write pre hook");
-        fs::write(
-            root.join("hooks").join("post.sh"),
+        );
+        write_file(
+            root.join("tools").join("echo-tool.sh").as_path(),
+            "#!/bin/sh\ncat\n",
+        );
+        write_file(
+            root.join("commands").join("sync.sh").as_path(),
+            "#!/bin/sh\nprintf 'sync'\n",
+        );
+        write_file(
+            root.join(MANIFEST_FILE_NAME).as_path(),
+            r#"{
+  "name": "loader-demo",
+  "version": "1.2.3",
+  "description": "Manifest loader test plugin",
+  "permissions": ["read", "write"],
+  "hooks": {
+    "PreToolUse": ["./hooks/pre.sh"]
+  },
+  "tools": [
+    {
+      "name": "echo_tool",
+      "description": "Echoes JSON input",
+      "inputSchema": {
+        "type": "object"
+      },
+      "command": "./tools/echo-tool.sh",
+      "requiredPermission": "workspace-write"
+    }
+  ],
+  "commands": [
+    {
+      "name": "sync",
+      "description": "Sync command",
+      "command": "./commands/sync.sh"
+    }
+  ]
+}"#,
+        );
+    }
+
+    fn write_external_plugin(root: &Path, name: &str, version: &str) {
+        write_file(
+            root.join("hooks").join("pre.sh").as_path(),
+            "#!/bin/sh\nprintf 'pre'\n",
+        );
+        write_file(
+            root.join("hooks").join("post.sh").as_path(),
             "#!/bin/sh\nprintf 'post'\n",
-        )
-        .expect("write post hook");
-        fs::write(
-            root.join(MANIFEST_RELATIVE_PATH),
+        );
+        write_file(
+            root.join(MANIFEST_RELATIVE_PATH).as_path(),
             format!(
                 "{{\n  \"name\": \"{name}\",\n  \"version\": \"{version}\",\n  \"description\": \"test plugin\",\n  \"hooks\": {{\n    \"PreToolUse\": [\"./hooks/pre.sh\"],\n    \"PostToolUse\": [\"./hooks/post.sh\"]\n  }}\n}}"
-            ),
-        )
-        .expect("write manifest");
+            )
+            .as_str(),
+        );
     }
 
     fn write_broken_plugin(root: &Path, name: &str) {
-        fs::create_dir_all(root.join(".claude-plugin")).expect("manifest dir");
-        fs::write(
-            root.join(MANIFEST_RELATIVE_PATH),
+        write_file(
+            root.join(MANIFEST_RELATIVE_PATH).as_path(),
             format!(
                 "{{\n  \"name\": \"{name}\",\n  \"version\": \"1.0.0\",\n  \"description\": \"broken plugin\",\n  \"hooks\": {{\n    \"PreToolUse\": [\"./hooks/missing.sh\"]\n  }}\n}}"
-            ),
-        )
-        .expect("write broken manifest");
+            )
+            .as_str(),
+        );
     }
 
     fn write_lifecycle_plugin(root: &Path, name: &str, version: &str) -> PathBuf {
-        fs::create_dir_all(root.join(".claude-plugin")).expect("manifest dir");
-        fs::create_dir_all(root.join("lifecycle")).expect("lifecycle dir");
         let log_path = root.join("lifecycle.log");
-        fs::write(
-            root.join("lifecycle").join("init.sh"),
+        write_file(
+            root.join("lifecycle").join("init.sh").as_path(),
             "#!/bin/sh\nprintf 'init\\n' >> lifecycle.log\n",
-        )
-        .expect("write init hook");
-        fs::write(
-            root.join("lifecycle").join("shutdown.sh"),
+        );
+        write_file(
+            root.join("lifecycle").join("shutdown.sh").as_path(),
             "#!/bin/sh\nprintf 'shutdown\\n' >> lifecycle.log\n",
-        )
-        .expect("write shutdown hook");
-        fs::write(
-            root.join(MANIFEST_RELATIVE_PATH),
+        );
+        write_file(
+            root.join(MANIFEST_RELATIVE_PATH).as_path(),
             format!(
                 "{{\n  \"name\": \"{name}\",\n  \"version\": \"{version}\",\n  \"description\": \"lifecycle plugin\",\n  \"lifecycle\": {{\n    \"Init\": [\"./lifecycle/init.sh\"],\n    \"Shutdown\": [\"./lifecycle/shutdown.sh\"]\n  }}\n}}"
-            ),
-        )
-        .expect("write manifest");
+            )
+            .as_str(),
+        );
         log_path
     }
 
     #[test]
-    fn validates_manifest_shape() {
-        let error = validate_manifest(&PluginManifest {
-            name: String::new(),
-            version: "1.0.0".to_string(),
-            description: "desc".to_string(),
-            default_enabled: false,
-            hooks: PluginHooks::default(),
-            lifecycle: PluginLifecycle::default(),
-        })
-        .expect_err("empty name should fail");
+    fn load_plugin_from_directory_validates_required_fields() {
+        let root = temp_dir("manifest-required");
+        write_file(
+            root.join(MANIFEST_FILE_NAME).as_path(),
+            r#"{"name":"","version":"1.0.0","description":"desc"}"#,
+        );
+
+        let error = load_plugin_from_directory(&root).expect_err("empty name should fail");
         assert!(error.to_string().contains("name cannot be empty"));
+
+        let _ = fs::remove_dir_all(root);
+    }
+
+    #[test]
+    fn load_plugin_from_directory_reads_root_manifest_and_validates_entries() {
+        let root = temp_dir("manifest-root");
+        write_loader_plugin(&root);
+
+        let manifest = load_plugin_from_directory(&root).expect("manifest should load");
+        assert_eq!(manifest.name, "loader-demo");
+        assert_eq!(manifest.version, "1.2.3");
+        assert_eq!(manifest.permissions, vec!["read", "write"]);
+        assert_eq!(manifest.hooks.pre_tool_use, vec!["./hooks/pre.sh"]);
+        assert_eq!(manifest.tools.len(), 1);
+        assert_eq!(manifest.tools[0].name, "echo_tool");
+        assert_eq!(manifest.commands.len(), 1);
+        assert_eq!(manifest.commands[0].name, "sync");
+
+        let _ = fs::remove_dir_all(root);
+    }
+
+    #[test]
+    fn load_plugin_from_directory_supports_packaged_manifest_path() {
+        let root = temp_dir("manifest-packaged");
+        write_external_plugin(&root, "packaged-demo", "1.0.0");
+
+        let manifest = load_plugin_from_directory(&root).expect("packaged manifest should load");
+        assert_eq!(manifest.name, "packaged-demo");
+        assert!(manifest.tools.is_empty());
+        assert!(manifest.commands.is_empty());
+
+        let _ = fs::remove_dir_all(root);
+    }
+
+    #[test]
+    fn load_plugin_from_directory_defaults_optional_fields() {
+        let root = temp_dir("manifest-defaults");
+        write_file(
+            root.join(MANIFEST_FILE_NAME).as_path(),
+            r#"{
+  "name": "minimal",
+  "version": "0.1.0",
+  "description": "Minimal manifest"
+}"#,
+        );
+
+        let manifest = load_plugin_from_directory(&root).expect("minimal manifest should load");
+        assert!(manifest.permissions.is_empty());
+        assert!(manifest.hooks.is_empty());
+        assert!(manifest.tools.is_empty());
+        assert!(manifest.commands.is_empty());
+
+        let _ = fs::remove_dir_all(root);
+    }
+
+    #[test]
+    fn load_plugin_from_directory_rejects_duplicate_permissions_and_commands() {
+        let root = temp_dir("manifest-duplicates");
+        write_file(
+            root.join("commands").join("sync.sh").as_path(),
+            "#!/bin/sh\nprintf 'sync'\n",
+        );
+        write_file(
+            root.join(MANIFEST_FILE_NAME).as_path(),
+            r#"{
+  "name": "duplicate-manifest",
+  "version": "1.0.0",
+  "description": "Duplicate validation",
+  "permissions": ["read", "read"],
+  "commands": [
+    {"name": "sync", "description": "Sync one", "command": "./commands/sync.sh"},
+    {"name": "sync", "description": "Sync two", "command": "./commands/sync.sh"}
+  ]
+}"#,
+        );
+
+        let error = load_plugin_from_directory(&root).expect_err("duplicates should fail");
+        assert!(error
+            .to_string()
+            .contains("permission `read` is duplicated"));
+
+        let _ = fs::remove_dir_all(root);
+    }
+
+    #[test]
+    fn load_plugin_from_directory_rejects_missing_tool_or_command_paths() {
+        let root = temp_dir("manifest-paths");
+        write_file(
+            root.join(MANIFEST_FILE_NAME).as_path(),
+            r#"{
+  "name": "missing-paths",
+  "version": "1.0.0",
+  "description": "Missing path validation",
+  "tools": [
+    {
+      "name": "tool_one",
+      "description": "Missing tool script",
+      "inputSchema": {"type": "object"},
+      "command": "./tools/missing.sh"
+    }
+  ]
+}"#,
+        );
+
+        let error = load_plugin_from_directory(&root).expect_err("missing paths should fail");
+        assert!(error.to_string().contains("does not exist"));
+
+        let _ = fs::remove_dir_all(root);
     }
 
     #[test]