diff --git a/CHANGELOG.md b/CHANGELOG.md index 49664377..fecdad8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -161,6 +161,15 @@ accumulate under `Unreleased` until a `v*` tag publishes them. statistics and eviction estimates count tokens without materializing full text, and the eviction frontier measures only the batches it consumes. +- Plugin pack contents take effect in sessions instead of staying + inventory-only: pack skills merge into the catalog as the new `pack` + source (below user and project, which keep their slots on a name + collision), pack MCP entries convert into configured servers for the + live connect path (a user-configured server owns its name; later + packs win among themselves), and pack hook rules join the engine + after the config-declared ones with the same matcher/timeout + semantics. Collisions and invalid entries degrade to session + warnings; `plugin list` notes that pack contents are wired. ### Fixed - Leaving the console TUI runs the SessionEnd hooks and the memory diff --git a/crates/capabilities/hooks/src/lib.rs b/crates/capabilities/hooks/src/lib.rs index 5bba4d33..e70fb79e 100644 --- a/crates/capabilities/hooks/src/lib.rs +++ b/crates/capabilities/hooks/src/lib.rs @@ -171,6 +171,28 @@ impl HookDef { self.timeout_ms }) } + + /// Reasons this entry can never do anything useful at `point`: an + /// empty command, a blank matcher, or a matcher on a point that + /// carries no tool name. Empty when the entry is usable; a zero + /// timeout is not listed because it falls back to the default + /// rather than disabling the entry. [`HookEngine::validate`] and + /// the assembly layers share this rule, so a skipped entry and the + /// static diagnostics cannot drift apart. + pub fn unusable_reasons(&self, point: HookEventPoint) -> Vec<&'static str> { + let mut reasons = Vec::new(); + if self.command.trim().is_empty() { + reasons.push("has an empty command (never executes)"); + } + if let Some(matcher) = &self.matcher { + if matcher.trim().is_empty() { + reasons.push("matcher is blank (never matches)"); + } else if !is_tool_point(point) { + reasons.push("has a matcher but this point carries no tool name (never fires)"); + } + } + reasons + } } /// An event payload for one hook firing. @@ -293,11 +315,8 @@ impl HookEngine { tables.sort_by_key(|(p, _)| p.as_str()); for (point, defs) in tables { for (idx, def) in defs.iter().enumerate() { - if def.command.trim().is_empty() { - out.push(format!( - "[{}] entry #{idx} has an empty command (never executes)", - point.as_str() - )); + for reason in def.unusable_reasons(point) { + out.push(format!("[{}] entry #{idx} {reason}", point.as_str())); } if def.timeout_ms == 0 { out.push(format!( @@ -305,19 +324,6 @@ impl HookEngine { point.as_str() )); } - if let Some(matcher) = &def.matcher { - if matcher.trim().is_empty() { - out.push(format!( - "[{}] entry #{idx} matcher is blank (never matches)", - point.as_str() - )); - } else if !is_tool_point(point) { - out.push(format!( - "[{}] entry #{idx} has a matcher but this point carries no tool name (never fires)", - point.as_str() - )); - } - } } } out diff --git a/crates/capabilities/skills/src/lib.rs b/crates/capabilities/skills/src/lib.rs index 6ac6fddf..0d170e32 100644 --- a/crates/capabilities/skills/src/lib.rs +++ b/crates/capabilities/skills/src/lib.rs @@ -62,6 +62,10 @@ const SKILL_DIR_VARIABLE: &str = "${WAVECODE_SKILL_DIR}"; pub enum SkillSource { /// Builtin skill set shipped with the binary. Builtin, + /// Installed plugin pack (`~/.wavecode/plugins//…`). Ranks + /// below user- and project-level skills: a pack is a bundle someone + /// else curated, so the user's own same-named skill wins. + Pack, /// User-level `~/.wavecode/skills`. User, /// Project-level `/.wavecode/skills`. @@ -77,6 +81,7 @@ impl SkillSource { pub fn as_str(self) -> &'static str { match self { Self::Builtin => "builtin", + Self::Pack => "pack", Self::User => "user", Self::Project => "project", Self::Mcp => "mcp", diff --git a/crates/capabilities/skills/src/plugin.rs b/crates/capabilities/skills/src/plugin.rs index 2f59fd52..cb43b7d1 100644 --- a/crates/capabilities/skills/src/plugin.rs +++ b/crates/capabilities/skills/src/plugin.rs @@ -10,10 +10,11 @@ * - Warn-and-skip invalid packs without failing discovery. * * Constraints: - * - This module is inventory only: the session assembly consumes - * builtin/user/project skill roots and never merges pack contents, so - * a pack's skills, MCP servers, and hooks are counted and surfaced - * (e.g. by `plugin list`) but do not take effect. + * - This module parses and validates but does not decide precedence: the + * session assembly merges pack skills under the standard roots via + * [`PluginLoader::apply_skills`], and the composition root converts + * the raw MCP entries and hook rules into configured servers and + * engine definitions (config-crate types this module must not import). * * This module must not depend on: workspace crates beyond its own * (manifest shapes mirror config/hook types instead of importing them). @@ -24,12 +25,12 @@ //! //! A manifest carries `{name, version, skills_dir?, hooks_file?}` plus //! optional `[mcp_servers.]` inline tables. Skills load through -//! the [`discover`] pipeline with [`SkillSource::User`]. The loader is -//! inventory only: MCP entries and hook rules are returned raw (mirrors -//! of the config/hook shapes, so this module imports neither), and -//! nothing currently converts them into configured servers or active -//! hook rules — consumers read counts and entries for listing and -//! diagnostics. Invalid packs warn-and-skip with a reason. +//! the [`discover`] pipeline with [`SkillSource::Pack`]. MCP entries and +//! hook rules are returned raw (mirrors of the config/hook shapes, so +//! this module imports neither); the session assembly merges the skills +//! below user/project sources and the composition root converts the +//! entries into configured servers and active hook rules. Invalid packs +//! warn-and-skip with a reason. use std::collections::HashMap; use std::path::{Path, PathBuf}; @@ -51,8 +52,8 @@ fn valid_name(name: &str) -> bool { /// Raw MCP server entry from a plugin manifest. /// /// Mirrors the config-crate server shape without depending on it. -/// Retained for listing and diagnostics; no session assembly consumes -/// these entries today. +/// The session assembly converts entries into configured servers for +/// the live connect path. #[derive(Debug, Clone, serde::Deserialize)] pub struct PluginMcpServer { /// Stdio executable command (stdio servers). @@ -73,8 +74,8 @@ pub struct PluginMcpServer { /// Raw hook rule from a plugin hooks file. /// /// Mirrors the config-crate hook rule without depending on it. -/// Retained for listing and diagnostics; no session assembly converts -/// these into engine definitions today. +/// The session assembly converts rules into engine definitions with +/// the same matcher/timeout semantics as config-declared rules. #[derive(Debug, Clone, serde::Deserialize)] pub struct PluginHookRule { /// Tool-name matcher (optional). @@ -123,12 +124,13 @@ pub struct LoadedPlugin { pub name: String, /// Pack version string. pub version: String, - /// Skills discovered from `skills_dir` ([`SkillSource::User`]). + /// Skills discovered from `skills_dir` ([`SkillSource::Pack`]). pub skills: SkillSet, - /// Raw MCP entries, sorted by server name (listing only; see the - /// module docs for the inventory-only contract). + /// Raw MCP entries, sorted by server name (converted into + /// configured servers by the composition root). pub mcp_servers: Vec<(String, PluginMcpServer)>, - /// Raw hook rules, keyed by event-point name (listing only). + /// Raw hook rules, keyed by event-point name (converted into + /// engine definitions by the session assembly). pub hooks: HashMap>, } @@ -203,15 +205,29 @@ impl PluginLoader { &self.warnings } - /// Merge every pack's skills into `set` (same names override, so - /// later packs win over earlier ones). + /// Merge every pack's skills into `set` as the pack source. /// - /// No production caller today — the session assembly never merges - /// pack skills; this is the hook a wiring pass would call. + /// Pack skills rank between builtin and the standard roots: they + /// displace a same-named builtin entry and yield to user and project + /// entries, which keep their slots, so the caller merges packs after + /// the standard roots. Later packs win among themselves, in + /// directory order. pub fn apply_skills(&self, set: &mut SkillSet) { for plugin in &self.plugins { for skill in plugin.skills.iter() { - set.add(skill.clone()); + // A user or project entry keeps its slot; a pack entry + // yields to the later pack, in directory order; a builtin + // entry yields to the pack (source order: builtin < pack + // < user < project). + let displaced = match set.get(&skill.name) { + Some(existing) => { + matches!(existing.source, SkillSource::Builtin | SkillSource::Pack) + } + None => true, + }; + if displaced { + set.add(skill.clone()); + } } } } @@ -239,9 +255,9 @@ impl PluginLoader { let skills = match &manifest.skills_dir { Some(rel) => { // Reuse the Discovery pipeline: same SKILL.md parsing and - // per-file warn-and-skip, attributed to the User source. + // per-file warn-and-skip, attributed to the Pack source. let roots = [SkillRoot { - source: SkillSource::User, + source: SkillSource::Pack, dir: dir.join(rel), }]; let discovery = discover(&roots); @@ -295,6 +311,7 @@ impl PluginLoader { #[cfg(test)] mod tests { + use super::super::Skill; use super::*; const SKILL_MD: &str = "---\ndescription: Review helper skill.\n---\n\nReview body here.\n"; @@ -454,7 +471,7 @@ command = "notify.sh" } #[test] - fn apply_skills_merges_as_user_source() { + fn apply_skills_merges_as_pack_source_below_existing_entries() { let home = home_with_pack( "demo", &[ @@ -467,6 +484,81 @@ command = "notify.sh" let mut set = SkillSet::default(); loader.apply_skills(&mut set); let skill = set.get("greet").expect("merged skill"); - assert_eq!(skill.source, SkillSource::User); + assert_eq!(skill.source, SkillSource::Pack); + + // An entry already in the set keeps its slot: user/project + // skills win over a pack skill of the same name. + let user = tempfile::tempdir().unwrap(); + let greet = user.path().join("greet"); + std::fs::create_dir_all(&greet).unwrap(); + std::fs::write(greet.join("SKILL.md"), SKILL_MD).unwrap(); + let mut set = SkillSet::default(); + set.add(Skill::parse(&greet, SkillSource::User).expect("hand-built skill")); + loader.apply_skills(&mut set); + assert_eq!(set.get("greet").unwrap().source, SkillSource::User); + + // A builtin entry yields to the pack: pack ranks above builtin + // in the source order (builtin < pack < user < project). + let builtin = tempfile::tempdir().unwrap(); + let greet = builtin.path().join("greet"); + std::fs::create_dir_all(&greet).unwrap(); + std::fs::write(greet.join("SKILL.md"), SKILL_MD).unwrap(); + let mut set = SkillSet::default(); + set.add(Skill::parse(&greet, SkillSource::Builtin).expect("hand-built skill")); + loader.apply_skills(&mut set); + assert_eq!(set.get("greet").unwrap().source, SkillSource::Pack); + } + + /// Two packs declaring the same skill name: the later pack (directory + /// order) wins the slot, while a user skill still beats both. + #[test] + fn later_pack_wins_the_same_named_skill() { + let manifest = |name: &str| { + format!( + "name = \"{name}\" +version = \"1\" +skills_dir = \"skills\" +" + ) + }; + let home = home_with_pack("aaa", &[("plugin.toml", &manifest("aaa"))]); + // Distinct SKILL.md fixtures: both packs carry the same skill + // name, so the merged entry's description is what tells the + // later pack's body apart from the first pack's. + let first_skill = "---\ndescription: First pack skill.\n---\n\nFirst pack body.\n"; + let later_skill = "---\ndescription: Later pack skill.\n---\n\nLater pack body.\n"; + let pack = home.path().join(".wavecode").join("plugins").join("bbb"); + std::fs::create_dir_all(pack.join("skills").join("greet")).unwrap(); + std::fs::write(pack.join("plugin.toml"), manifest("bbb")).unwrap(); + std::fs::write( + pack.join("skills").join("greet").join("SKILL.md"), + later_skill, + ) + .unwrap(); + let first = home.path().join(".wavecode").join("plugins").join("aaa"); + std::fs::create_dir_all(first.join("skills").join("greet")).unwrap(); + std::fs::write( + first.join("skills").join("greet").join("SKILL.md"), + first_skill, + ) + .unwrap(); + let loader = PluginLoader::load(Some(home.path())); + assert_eq!(loader.plugins().len(), 2); + + let mut set = SkillSet::default(); + loader.apply_skills(&mut set); + assert_eq!(set.get("greet").unwrap().source, SkillSource::Pack); + let merged = set.get("greet").unwrap(); + assert_eq!( + merged.meta.description, "Later pack skill.", + "the later pack's metadata displaced the first pack's entry" + ); + assert_eq!( + merged.body, "Later pack body.", + "the merged entry carries the later pack's body, not just its metadata" + ); + + // The collision leaves one entry, not a duplicate. + assert_eq!(set.len(), 1); } } diff --git a/crates/frontends/harness/src/serve.rs b/crates/frontends/harness/src/serve.rs index 90c4de7f..9750c187 100644 --- a/crates/frontends/harness/src/serve.rs +++ b/crates/frontends/harness/src/serve.rs @@ -62,6 +62,10 @@ pub(crate) fn run_plugin_list(home: Option) { plugin.name, plugin.version, plugin.skills, plugin.mcp_servers, plugin.hooks ); } + println!( + "pack contents take effect in new sessions; a same-named user skill or \ + configured mcp server wins over a pack entry" + ); } /// Serve live sessions over local HTTP until Ctrl-C or `POST /shutdown`. diff --git a/crates/operations/bootstrap/src/plugin_inventory.rs b/crates/operations/bootstrap/src/plugin_inventory.rs index ce5c1ede..3706ad1a 100644 --- a/crates/operations/bootstrap/src/plugin_inventory.rs +++ b/crates/operations/bootstrap/src/plugin_inventory.rs @@ -32,9 +32,13 @@ pub struct PluginSummary { /// Load every installed plugin pack under `home`. /// /// Returns the summaries plus non-fatal parse warnings: a broken pack -/// degrades to a warning, never a hard failure. The summaries are -/// inventory only — pack skills, MCP servers, and hooks are counted for -/// listing and diagnostics; none of them is wired into a session. +/// degrades to a warning, never a hard failure. The summaries mirror +/// what a session would wire: pack skills merge into the catalog under +/// user/project sources, pack MCP servers convert into configured +/// servers, and pack hook rules join the hook engine. Whether a given +/// entry survives session-time collision checks (a same-named user +/// skill or configured server wins) depends on the session's cwd and +/// config, which this listing deliberately does not load. pub fn discover_plugin_summaries(home: Option<&Path>) -> (Vec, Vec) { let loader = wavecode_skills::plugin::PluginLoader::load(home); let summaries = loader diff --git a/crates/operations/bootstrap/src/session/assembly.rs b/crates/operations/bootstrap/src/session/assembly.rs index 420717f2..659ea267 100644 --- a/crates/operations/bootstrap/src/session/assembly.rs +++ b/crates/operations/bootstrap/src/session/assembly.rs @@ -344,6 +344,7 @@ pub fn assemble_session_after_model(parts: WithModel) -> SessionHandle { skill_catalog, skill_names, mcp_servers, + pack_mcp_servers, .. } = ctx; @@ -396,8 +397,11 @@ pub fn assemble_session_after_model(parts: WithModel) -> SessionHandle { _ => client, }; + // Configured servers connect first; converted pack servers append + // after (the list is name-sorted anyway). let mut mcp_pending: Vec<(String, wavecode_config::McpServerRaw)> = config.mcp_servers.clone().into_iter().collect(); + mcp_pending.extend(pack_mcp_servers); mcp_pending.sort_by(|a, b| a.0.cmp(&b.0)); // Frontend status views share the tool-side snapshot root so /plan, @@ -477,6 +481,11 @@ struct AssembledContext { hooks: Arc, /// Configured MCP servers as one display line each. mcp_servers: Vec, + /// Pack MCP servers converted into config shape for the live + /// connect path (user config wins name collisions; later packs win + /// among themselves). The finalize phase appends them to the + /// pending list after the configured servers. + pack_mcp_servers: Vec<(String, wavecode_config::McpServerRaw)>, } /// Build [`AssembledContext`] (phases 1-4): shared stores, the policy @@ -526,12 +535,18 @@ fn build_assembled_context( let sandbox = wavecode_sandbox::Sandbox::from_rules(permission_mode, permissions.allow, permissions.deny); - // 4. Context sources with warn-and-continue degradation. + // 4. Context sources with warn-and-continue degradation. Plugin + // packs load once here: the skills merge into the catalog, the hook + // rules join the engine, and the MCP entries convert into pending + // servers; loader warnings degrade like every other source. + let plugins = wavecode_skills::plugin::PluginLoader::load(home); + warnings.extend(plugins.warnings().iter().cloned()); let (instruction_memory, memory_index, instruction_sources) = assemble_memory(home, cwd, warnings); - let (skill_catalog, skill_names, skill_set) = assemble_skills(home, cwd, warnings); - let hooks = assemble_hooks(config, warnings); - let mcp_servers = describe_mcp_servers(config); + let (skill_catalog, skill_names, skill_set) = assemble_skills(&plugins, home, cwd, warnings); + let hooks = assemble_hooks(config, &plugins, warnings); + let pack_mcp_servers = pack_mcp_servers(config, &plugins, warnings); + let mcp_servers = describe_mcp_servers(config, &pack_mcp_servers); AssembledContext { spill_store, @@ -548,6 +563,7 @@ fn build_assembled_context( skill_set, hooks, mcp_servers, + pack_mcp_servers, } } @@ -1217,13 +1233,18 @@ fn assemble_memory( /// Assemble the skill catalog text with discovery warnings preserved, /// returning catalog text, directly invokable names for UIs, and the set /// itself for the `skill` tool backend. +/// +/// Pack skills merge under the standard roots: a same-named user or +/// project skill keeps its slot and a pack only fills the gaps. fn assemble_skills( + plugins: &wavecode_skills::plugin::PluginLoader, home: Option<&Path>, cwd: &Path, warnings: &mut Vec, ) -> (String, Vec, Arc) { let roots = wavecode_skills::standard_roots(None, home, cwd); - let discovery = wavecode_skills::discover(&roots); + let mut discovery = wavecode_skills::discover(&roots); + plugins.apply_skills(&mut discovery.set); warnings.extend(discovery.warnings.iter().cloned()); let names: Vec = discovery .set @@ -1238,54 +1259,227 @@ fn assemble_skills( ) } -/// Convert raw config hooks into an engine, warning past bad entries. +/// Convert raw config hooks plus pack hook rules into an engine, warning +/// past bad entries. fn assemble_hooks( config: &wavecode_config::Config, + plugins: &wavecode_skills::plugin::PluginLoader, warnings: &mut Vec, ) -> Arc { - let mut defs = HashMap::new(); + Arc::new(wavecode_hooks::HookEngine::new(hook_defs( + config, plugins, warnings, + ))) +} + +/// Build the engine's definitions: config-declared rules come first at +/// every event point; pack rules append after (pack directory order, +/// then file order within a pack) with the same matcher/timeout +/// semantics as config rules. +pub(super) fn hook_defs( + config: &wavecode_config::Config, + plugins: &wavecode_skills::plugin::PluginLoader, + warnings: &mut Vec, +) -> HashMap> { + let mut defs: HashMap> = + HashMap::new(); + config_hook_defs(config, &mut defs, warnings); + pack_hook_defs(plugins, &mut defs, warnings); + defs +} + +/// One config `HookRule` in engine form (absent timeout/once fall back +/// to the engine defaults). +fn config_rule_to_def(rule: &wavecode_config::HookRule) -> wavecode_hooks::HookDef { + wavecode_hooks::HookDef { + matcher: rule.matcher.clone(), + command: rule.command.clone(), + timeout_ms: rule + .timeout_ms + .unwrap_or(wavecode_hooks::DEFAULT_TIMEOUT_MS), + once: rule.once.unwrap_or(false), + } +} + +/// Insert the config-declared rules; an unknown event point warns and +/// skips (config keys are free-form TOML). +fn config_hook_defs( + config: &wavecode_config::Config, + defs: &mut HashMap>, + warnings: &mut Vec, +) { for (point_name, ruleset) in &config.hooks { match wavecode_hooks::HookEventPoint::parse(point_name) { None => warnings.push(format!("unknown hook point {point_name:?}; skipped")), Some(point) => { defs.insert( point, - ruleset - .rules() - .iter() - .map(|rule| wavecode_hooks::HookDef { - matcher: rule.matcher.clone(), - command: rule.command.clone(), - timeout_ms: rule - .timeout_ms - .unwrap_or(wavecode_hooks::DEFAULT_TIMEOUT_MS), - once: rule.once.unwrap_or(false), - }) - .collect(), + ruleset.rules().iter().map(config_rule_to_def).collect(), ); } } } - Arc::new(wavecode_hooks::HookEngine::new(defs)) } -/// Describe configured MCP servers as one display line each. +/// Append the pack rules after the config rules at every event point. +/// +/// An unknown pack point warns with the pack attribution; a rule the +/// engine can never fire (an empty command, a blank matcher, or a +/// matcher on a point that carries no tool name — the engine's own +/// [`wavecode_hooks::HookDef::unusable_reasons`] rule) warns with the +/// pack name and the reason, and is skipped. +fn pack_hook_defs( + plugins: &wavecode_skills::plugin::PluginLoader, + defs: &mut HashMap>, + warnings: &mut Vec, +) { + for plugin in plugins.plugins() { + for (point_name, rules) in &plugin.hooks { + match wavecode_hooks::HookEventPoint::parse(point_name) { + None => warnings.push(format!( + "plugin {}: unknown hook point {point_name:?}; skipped", + plugin.name + )), + Some(point) => file_pack_rules( + &plugin.name, + point_name, + point, + rules, + defs.entry(point).or_default(), + warnings, + ), + } + } + } +} + +/// File one pack point's rules into `slot` in rule order. +/// +/// A rule the engine can never fire +/// ([`wavecode_hooks::HookDef::unusable_reasons`]) warns with the pack +/// attribution and is skipped instead of joining `slot`. +fn file_pack_rules( + plugin_name: &str, + point_name: &str, + point: wavecode_hooks::HookEventPoint, + rules: &[wavecode_skills::plugin::PluginHookRule], + slot: &mut Vec, + warnings: &mut Vec, +) { + for (idx, rule) in rules.iter().enumerate() { + let def = wavecode_hooks::HookDef { + matcher: rule.matcher.clone(), + command: rule.command.clone(), + timeout_ms: rule + .timeout_ms + .unwrap_or(wavecode_hooks::DEFAULT_TIMEOUT_MS), + once: rule.once.unwrap_or(false), + }; + let reasons = def.unusable_reasons(point); + if reasons.is_empty() { + slot.push(def); + } else { + warnings.push(format!( + "plugin {plugin_name}: hook {point_name} entry #{idx} skipped: {}", + reasons.join("; ") + )); + } + } +} + +/// Convert pack MCP entries into config-crate servers for the live +/// connect path. +/// +/// A user-configured server owns its name: a colliding pack entry warns +/// and drops. Among packs the later one wins a shared name (directory +/// order), matching pack skill precedence. Every converted entry goes +/// through the connect path's own conversion +/// ([`wavecode_mcp::McpServerConfig::from_raw`]) first: an entry that +/// could never connect warns with the pack name and the reason, and is +/// skipped — so it also cannot displace an earlier pack's valid entry +/// on a shared name. +pub(super) fn pack_mcp_servers( + config: &wavecode_config::Config, + plugins: &wavecode_skills::plugin::PluginLoader, + warnings: &mut Vec, +) -> Vec<(String, wavecode_config::McpServerRaw)> { + let mut servers: Vec<(String, wavecode_config::McpServerRaw)> = Vec::new(); + for plugin in plugins.plugins() { + for (name, raw) in &plugin.mcp_servers { + if config.mcp_servers.contains_key(name) { + warnings.push(format!( + "plugin {}: mcp server {name:?} ignored: a configured server already owns that name", + plugin.name + )); + continue; + } + let server = plugin_mcp_to_config(raw); + if let Err(reason) = wavecode_mcp::McpServerConfig::from_raw(name, &server) { + warnings.push(format!( + "plugin {}: mcp server {name:?} skipped: {reason}", + plugin.name + )); + continue; + } + match servers.iter_mut().find(|(known, _)| known == name) { + Some(slot) => { + warnings.push(format!( + "plugin {}: mcp server {name:?} replaces the earlier pack's entry", + plugin.name + )); + slot.1 = server; + } + None => servers.push((name.clone(), server)), + } + } + } + servers.sort_by(|a, b| a.0.cmp(&b.0)); + servers +} + +/// Lift a pack MCP entry into the config-crate shape. Packs carry no +/// OAuth fields, so those default to absent. +fn plugin_mcp_to_config( + raw: &wavecode_skills::plugin::PluginMcpServer, +) -> wavecode_config::McpServerRaw { + wavecode_config::McpServerRaw { + command: raw.command.clone(), + args: raw.args.clone(), + env: raw.env.clone(), + url: raw.url.clone(), + headers: raw.headers.clone(), + oauth_token_url: None, + oauth_client_id: None, + oauth_client_secret: None, + oauth_scope: None, + } +} + +/// Describe configured and pack MCP servers as one display line each. /// /// stdio entries show their command, HTTP entries their URL; entries /// with neither are reported, never silently dropped. -fn describe_mcp_servers(config: &wavecode_config::Config) -> Vec { +fn describe_mcp_servers( + config: &wavecode_config::Config, + pack_servers: &[(String, wavecode_config::McpServerRaw)], +) -> Vec { + let describe = |name: &str, server: &wavecode_config::McpServerRaw| { + if let Some(command) = &server.command { + format!("{name} (stdio: {command})") + } else if let Some(url) = &server.url { + format!("{name} (http: {url})") + } else { + format!("{name} (unconfigured)") + } + }; let mut lines: Vec = config .mcp_servers .iter() - .map(|(name, server)| { - if let Some(command) = &server.command { - format!("{name} (stdio: {command})") - } else if let Some(url) = &server.url { - format!("{name} (http: {url})") - } else { - format!("{name} (unconfigured)") - } - }) + .map(|(name, server)| describe(name, server)) + .chain( + pack_servers + .iter() + .map(|(name, server)| describe(name, server)), + ) .collect(); lines.sort(); lines diff --git a/crates/operations/bootstrap/src/session/tests.rs b/crates/operations/bootstrap/src/session/tests.rs index aa5b6ddd..09759af6 100644 --- a/crates/operations/bootstrap/src/session/tests.rs +++ b/crates/operations/bootstrap/src/session/tests.rs @@ -995,3 +995,359 @@ fn gapped_journal_is_rewritten_before_new_records() { assert_eq!(replayed.entries.len(), 2); assert_eq!(replayed.entries[1].text(), "after"); } + +/// Lay out a home dir holding one plugin pack with the given files +/// (the same harness the loader tests in wavecode-skills use). +fn home_with_pack(name: &str, files: &[(&str, &str)]) -> tempfile::TempDir { + let home = tempfile::tempdir().unwrap(); + let pack = home.path().join(".wavecode").join("plugins").join(name); + std::fs::create_dir_all(&pack).unwrap(); + for (rel, content) in files { + let path = pack.join(rel); + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).unwrap(); + } + std::fs::write(path, content).unwrap(); + } + home +} + +/// Write a skill dir holding one SKILL.md with the given description. +fn write_skill(root: &std::path::Path, name: &str, description: &str) { + let dir = root.join(name); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write( + dir.join("SKILL.md"), + format!("---\ndescription: {description}.\n---\n\nBody.\n"), + ) + .unwrap(); +} + +const PACK_MANIFEST: &str = r#" +name = "demo" +version = "0.1.0" +skills_dir = "skills" +hooks_file = "hooks.toml" + +[mcp_servers.lint] +command = "lint-server" +args = ["--fast"] +"#; + +const PACK_HOOKS: &str = r#" +[[PreToolUse]] +command = "pack.sh" +timeout_ms = 250 + +[[PostToolUse]] +command = "fmt.sh" + +[BogusPoint] +command = "x.sh" +"#; + +/// Pack contents reach the assembled session end to end: pack skills +/// merge into the catalog under the user and project sources, the pack +/// MCP entry converts into a pending server, and the pack hook rules +/// join the engine (including the unknown-point warning). +#[tokio::test] +async fn pack_contents_reach_the_session_under_standard_precedence() { + let dir = tempfile::tempdir().unwrap(); + let home = home_with_pack( + "demo", + &[("plugin.toml", PACK_MANIFEST), ("hooks.toml", PACK_HOOKS)], + ); + // Pack skills merge under the standard sources: the user skill + // overrides the pack skill, and the project skill overrides both, + // so only the project description may surface for `greet`. + let pack_skills = home + .path() + .join(".wavecode") + .join("plugins") + .join("demo") + .join("skills"); + write_skill(&pack_skills, "greet", "pack greet helper"); + write_skill(&pack_skills, "packonly", "packonly helper"); + write_skill( + &home.path().join(".wavecode").join("skills"), + "greet", + "user greet helper", + ); + write_skill( + &dir.path().join(".wavecode").join("skills"), + "greet", + "project greet helper", + ); + let config_path = dir.path().join("config.toml"); + std::fs::write(&config_path, CONFIG).unwrap(); + + let handle = assemble_session(AssembleOptions { + config_path: Some(config_path), + model_override: None, + provider_override: None, + permission_override: None, + thinking_override: None, + wave_denylist: None, + cwd: dir.path().to_path_buf(), + home: Some(home.path().to_path_buf()), + identity: DEFAULT_IDENTITY.to_string(), + headless: true, + initial_history: Vec::new(), + session_id: None, + }) + .unwrap(); + + // Skills: project > user > pack, and the pack-only skill lands. + assert!( + handle.system.contains("project greet helper"), + "the project skill must win the greet slot" + ); + assert!(!handle.system.contains("user greet helper")); + assert!(!handle.system.contains("pack greet helper")); + assert!(handle.system.contains("packonly helper")); + assert!(handle.skill_names.iter().any(|name| name == "packonly")); + + // MCP: the pack entry converts into a pending server with the OAuth + // fields absent, and the display line lists it. + let lint = handle + .mcp_pending + .iter() + .find(|(name, _)| name == "lint") + .expect("pack server rides the pending list"); + assert_eq!(lint.1.command.as_deref(), Some("lint-server")); + assert_eq!(lint.1.args, vec!["--fast".to_string()]); + assert!(lint.1.oauth_token_url.is_none()); + assert!( + handle + .mcp_servers + .iter() + .any(|line| line.contains("lint") && line.contains("lint-server")) + ); + + // Hooks: the unknown pack point warns with the pack attribution. + let unknown_point = handle + .warnings + .iter() + .any(|w| w.contains("plugin demo") && w.contains("unknown hook point")); + assert!(unknown_point, "unknown-point warning missing"); +} + +/// A user-configured MCP server owns its name: the colliding pack entry +/// warns and drops instead of reaching the connect path. +#[tokio::test] +async fn configured_mcp_server_wins_over_pack_entry() { + let dir = tempfile::tempdir().unwrap(); + let config_path = dir.path().join("config.toml"); + std::fs::write( + &config_path, + format!("{CONFIG}\n[mcp_servers.lint]\ncommand = \"config-server\"\n"), + ) + .unwrap(); + let home = home_with_pack("demo", &[("plugin.toml", PACK_MANIFEST)]); + + let handle = assemble_session(AssembleOptions { + config_path: Some(config_path), + model_override: None, + provider_override: None, + permission_override: None, + thinking_override: None, + wave_denylist: None, + cwd: dir.path().to_path_buf(), + home: Some(home.path().to_path_buf()), + identity: DEFAULT_IDENTITY.to_string(), + headless: true, + initial_history: Vec::new(), + session_id: None, + }) + .unwrap(); + + let lints: Vec<&(String, wavecode_config::McpServerRaw)> = handle + .mcp_pending + .iter() + .filter(|(name, _)| name == "lint") + .collect(); + assert_eq!(lints.len(), 1, "the pack entry must not ride along"); + assert_eq!(lints[0].1.command.as_deref(), Some("config-server")); + let name_owned = handle + .warnings + .iter() + .any(|w| w.contains("already owns that name")); + assert!(name_owned, "collision warning missing"); +} + +/// Hook definitions: config rules keep their point slot, pack rules +/// append after them with the same timeout/once semantics, and an +/// unknown pack point warns with the pack attribution. +#[test] +fn pack_hook_rules_join_config_rules_at_the_same_point() { + let config = config_with("\n[hooks.PreToolUse]\ncommand = \"config.sh\"\n"); + let home = home_with_pack( + "demo", + &[("plugin.toml", PACK_MANIFEST), ("hooks.toml", PACK_HOOKS)], + ); + let plugins = wavecode_skills::plugin::PluginLoader::load(Some(home.path())); + let mut warnings = Vec::new(); + let defs = super::assembly::hook_defs(&config, &plugins, &mut warnings); + + let pre = &defs[&wavecode_hooks::HookEventPoint::PreToolUse]; + assert_eq!(pre.len(), 2); + assert_eq!(pre[0].command, "config.sh"); + assert_eq!(pre[1].command, "pack.sh"); + assert_eq!(pre[1].timeout_ms, 250); + assert!(!pre[1].once); + + let post = &defs[&wavecode_hooks::HookEventPoint::PostToolUse]; + assert_eq!(post.len(), 1); + assert_eq!(post[0].command, "fmt.sh"); + // Absent timeout/once fall back to the engine defaults, same as + // config-declared rules. + assert_eq!(post[0].timeout_ms, wavecode_hooks::DEFAULT_TIMEOUT_MS); + + let unknown_point = warnings + .iter() + .any(|w| w.contains("plugin demo") && w.contains("BogusPoint")); + assert!( + unknown_point, + "unknown-point warning missing (warnings_count={})", + warnings.len() + ); +} + +/// Pack MCP entries: a configured server wins the name, later packs win +/// among themselves, and the survivors convert in sorted order. +#[test] +fn pack_mcp_entries_convert_with_config_priority_and_pack_order() { + let config = config_with( + "\n[mcp_servers.lint]\ncommand = \"config-server\"\n[mcp_servers.only]\ncommand = \"only-server\"\n", + ); + let home = tempfile::tempdir().unwrap(); + let plugins_root = home.path().join(".wavecode").join("plugins"); + for (pack, command) in [("aaa", "first-server"), ("zzz", "second-server")] { + let pack_dir = plugins_root.join(pack); + std::fs::create_dir_all(&pack_dir).unwrap(); + std::fs::write( + pack_dir.join("plugin.toml"), + format!("name = \"{pack}\"\nversion = \"1\"\n\n[mcp_servers.fetch]\ncommand = \"{command}\"\n[mcp_servers.lint]\ncommand = \"{command}\"\n"), + ) + .unwrap(); + } + let plugins = wavecode_skills::plugin::PluginLoader::load(Some(home.path())); + let mut warnings = Vec::new(); + let servers = super::assembly::pack_mcp_servers(&config, &plugins, &mut warnings); + + // `lint` collides with config (both packs drop with a warning), + // `fetch` collides across packs (the later pack wins, warned). + let names: Vec<&str> = servers.iter().map(|(name, _)| name.as_str()).collect(); + assert_eq!(names, vec!["fetch"]); + assert_eq!( + servers[0].1.command.as_deref(), + Some("second-server"), + "the later pack (zzz) wins the shared name" + ); + assert_eq!( + warnings + .iter() + .filter(|w| w.contains("already owns that name")) + .count(), + 2, + "both packs' lint entries drop against config (warnings_count={})", + warnings.len() + ); + let replaced = warnings + .iter() + .any(|w| w.contains("replaces the earlier pack's entry")); + assert!( + replaced, + "pack-vs-pack warning missing (warnings_count={})", + warnings.len() + ); +} + +/// A pack rule the engine can never fire (an empty command, a blank +/// matcher) warns with the pack attribution and is skipped, while the +/// usable rules from the same pack still load. +#[test] +fn unusable_pack_hook_rules_warn_and_skip() { + let config = config_with(""); + let pack_hooks = r#" +[[PreToolUse]] +command = "pack.sh" + +[[PreToolUse]] +command = " " + +[[PostToolUse]] +matcher = " " +command = "fmt.sh" +"#; + let home = home_with_pack( + "demo", + &[("plugin.toml", PACK_MANIFEST), ("hooks.toml", pack_hooks)], + ); + let plugins = wavecode_skills::plugin::PluginLoader::load(Some(home.path())); + let mut warnings = Vec::new(); + let defs = super::assembly::hook_defs(&config, &plugins, &mut warnings); + + // Only the usable rule reaches the engine; the unusable ones warn + // with the pack name and never join the definitions. + let pre = &defs[&wavecode_hooks::HookEventPoint::PreToolUse]; + assert_eq!(pre.len(), 1); + assert_eq!(pre[0].command, "pack.sh"); + let post = &defs[&wavecode_hooks::HookEventPoint::PostToolUse]; + assert!(post.is_empty(), "a blank matcher never fires: {post:?}"); + + let skipped = warnings + .iter() + .filter(|w| w.contains("plugin demo") && w.contains("skipped")) + .count(); + assert_eq!(skipped, 2, "both unusable rules warn: {warnings:?}"); +} + +/// An entry that could never connect (neither command nor url) warns +/// with the pack attribution and is skipped, so a later pack's invalid +/// entry cannot displace an earlier pack's valid one on a shared name. +#[test] +fn invalid_pack_mcp_entry_warns_and_skips() { + let config = config_with(""); + let home = tempfile::tempdir().unwrap(); + let plugins_root = home.path().join(".wavecode").join("plugins"); + let first = plugins_root.join("aaa"); + std::fs::create_dir_all(&first).unwrap(); + std::fs::write( + first.join("plugin.toml"), + "name = \"aaa\"\nversion = \"1\"\n\n[mcp_servers.fetch]\ncommand = \"first-server\"\n", + ) + .unwrap(); + let second = plugins_root.join("zzz"); + std::fs::create_dir_all(&second).unwrap(); + std::fs::write( + second.join("plugin.toml"), + "name = \"zzz\"\nversion = \"1\"\n\n[mcp_servers.fetch]\nargs = []\n", + ) + .unwrap(); + let plugins = wavecode_skills::plugin::PluginLoader::load(Some(home.path())); + let mut warnings = Vec::new(); + let servers = super::assembly::pack_mcp_servers(&config, &plugins, &mut warnings); + + let names: Vec<&str> = servers.iter().map(|(name, _)| name.as_str()).collect(); + assert_eq!(names, vec!["fetch"]); + assert_eq!( + servers[0].1.command.as_deref(), + Some("first-server"), + "the later pack's invalid entry cannot displace the valid one" + ); + assert!( + warnings.iter().any(|w| w.contains("plugin zzz") + && w.contains("skipped") + && w.contains("neither command nor url")), + "pack-attributed skip warning missing (warnings_count={})", + warnings.len() + ); + assert!( + !warnings + .iter() + .any(|w| w.contains("replaces the earlier pack's entry")), + "no replacement happened (warnings_count={})", + warnings.len() + ); +} diff --git a/docs/subsystems/extensibility.md b/docs/subsystems/extensibility.md index e8fa514c..ae0f0a44 100644 --- a/docs/subsystems/extensibility.md +++ b/docs/subsystems/extensibility.md @@ -34,7 +34,7 @@ Known limits, stated in the code: interactive browser/PKCE OAuth is out of scope ## Plugins (`crates/runtime/plugin/src/lib.rs`, `crates/capabilities/skills/src/plugin.rs`) -The runtime `Registry` starts plugins in manifest-dependency order and stops them in reverse; `ServiceMap` injects typed services with type-id keyed lookup. Discovery reads `runtime.toml` manifests (`{name, version, depends?}` identity) and degrades **warn-and-skip**: an invalid manifest warns with its reason and is skipped, never failing assembly. The `PluginLoader` in the skills crate loads user plugin packs (`plugin.toml`: `name`, `version`, optional `skills_dir`, hooks, MCP server entries) with the same warn-and-skip contract, feeding their skills/hooks/MCP servers into normal assembly. +The runtime `Registry` starts plugins in manifest-dependency order and stops them in reverse; `ServiceMap` injects typed services with type-id keyed lookup. Discovery reads `runtime.toml` manifests (`{name, version, depends?}` identity) and degrades **warn-and-skip**: an invalid manifest warns with its reason and is skipped, never failing assembly. The `PluginLoader` in the skills crate loads user plugin packs (`plugin.toml`: `name`, `version`, optional `skills_dir`, hooks, MCP server entries) with the same warn-and-skip contract, and their contents take effect in sessions: pack skills merge into the catalog as the `pack` source below user and project (a same-named user or project skill keeps its slot; a builtin one yields), pack MCP entries convert into configured servers (a user-configured server owns its name; later packs win among themselves, in directory order; an entry that could never connect warns with its reason and skips instead of displacing an earlier pack's valid entry), and pack hook rules join the engine after the config-declared ones with the same matcher/timeout semantics (a rule the engine can never fire — an empty command or a blank matcher — warns with its reason and skips). ## Named agent definitions (`crates/capabilities/tools/src/agent_task_tool.rs`) diff --git a/docs/subsystems/extensibility.zh.md b/docs/subsystems/extensibility.zh.md index 98107154..e2feeb33 100644 --- a/docs/subsystems/extensibility.zh.md +++ b/docs/subsystems/extensibility.zh.md @@ -34,7 +34,7 @@ Frontmatter 解析使用 `serde-saphyr` 而不是手写扫描器——一个刻 ## 插件（`crates/runtime/plugin/src/lib.rs`、`crates/capabilities/skills/src/plugin.rs`） -runtime 的 `Registry` 按清单依赖顺序启动插件，逆序停止；`ServiceMap` 以 type-id 键控查找注入类型化服务。发现读取 `runtime.toml` 清单（`{name, version, depends?}` 身份）并按**警告并跳过**降级：无效清单带原因警告并被跳过，绝不让装配失败。skills crate 中的 `PluginLoader` 加载用户插件包（`plugin.toml`：`name`、`version`、可选 `skills_dir`、hooks、MCP 服务器条目），带同样的 warn-and-skip 契约，把它们的技能/钩子/MCP 服务器喂进正常装配。 +runtime 的 `Registry` 按清单依赖顺序启动插件，逆序停止；`ServiceMap` 以 type-id 键控查找注入类型化服务。发现读取 `runtime.toml` 清单（`{name, version, depends?}` 身份）并按**警告并跳过**降级：无效清单带原因警告并被跳过，绝不让装配失败。skills crate 中的 `PluginLoader` 加载用户插件包（`plugin.toml`：`name`、`version`、可选 `skills_dir`、hooks、MCP 服务器条目），带同样的 warn-and-skip 契约，其内容会在会话中生效：包技能以 `pack` 来源并入目录，优先级低于 user 与 project（同名的 user/project 技能保有自己的槽位；builtin 槽位让位给 pack）；包 MCP 条目转换为已配置服务器（用户配置的服务器拥有其名字；多个包之间按目录顺序后者获胜；永远无法连接的条目带原因警告并被跳过，不会挤掉先包的有效条目）；包 hook 规则以与配置规则相同的 matcher/timeout 语义追加到引擎中配置规则之后（引擎永远无法触发的规则——空 command 或空 matcher——带原因警告并被跳过）。 ## 命名的 agent 定义（`crates/capabilities/tools/src/agent_task_tool.rs`）