From 80f057b7bcd448e002dbd9066f55693f5d7e6a8c Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Jun 2026 02:00:08 +0000 Subject: [PATCH 01/13] Add custom predicate extensions via [[predicate]] Plugin authors can now define custom predicate functions that extend the predicate language. When a crates expression uses an unknown function name (e.g., `battery-pack(cli>=0.3)`), Symposium looks it up in the custom predicate registry and spawns the declared command with the raw argument appended. Key additions: - Predicate::Custom variant with balanced-paren parsing - [[predicate]] section in plugin TOML manifests - CustomPredicateEvaluator with per-sync caching - Witness support (stdout JSON with selectedCrates) - Runnable::spawn helper on symposium-install - Full sync integration (predicates resolved before evaluation) - Integration tests and reference documentation Co-authored-by: Claude --- md/reference/plugin-definition.md | 64 ++ src/help_render.rs | 2 + src/hook.rs | 1 + src/plugins.rs | 160 ++++- src/predicate.rs | 575 +++++++++++++++++- src/skills.rs | 121 ++++ src/subcommand_dispatch.rs | 2 + src/sync.rs | 71 ++- symposium-install/src/lib.rs | 15 + tests/custom_predicates.rs | 93 +++ tests/fixtures/custom-predicate0/Cargo.toml | 7 + .../dot-symposium/config.toml | 5 + .../plugins/bp-plugin/SYMPOSIUM.toml | 13 + .../bp-plugin/skills/bp-skill/SKILL.md | 6 + tests/fixtures/custom-predicate0/src/lib.rs | 0 15 files changed, 1122 insertions(+), 13 deletions(-) create mode 100644 tests/custom_predicates.rs create mode 100644 tests/fixtures/custom-predicate0/Cargo.toml create mode 100644 tests/fixtures/custom-predicate0/dot-symposium/config.toml create mode 100644 tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml create mode 100644 tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/skills/bp-skill/SKILL.md create mode 100644 tests/fixtures/custom-predicate0/src/lib.rs diff --git a/md/reference/plugin-definition.md b/md/reference/plugin-definition.md index 991ced70..cde64e75 100644 --- a/md/reference/plugin-definition.md +++ b/md/reference/plugin-definition.md @@ -38,6 +38,7 @@ source.path = "skills" | `installations` | array of tables | no | Named installation declarations (`[[installations]]`). Hooks reference these by name. See [Installations](#installations). | | `skills` | array of tables | no | Skill groups (`[[skills]]`). | | `hooks` | array of tables | no | Hooks (`[[hooks]]`). | +| `predicate` | array of tables | no | Custom predicate definitions (`[[predicate]]`). See [Custom predicates](#predicate). | | `mcp_servers` | array of tables | no | MCP server registrations (`[[mcp_servers]]`). | **Note**: Every plugin must reference at least one crate somewhere — at the plugin level, in `[[skills]]` groups, or in `[[mcp_servers]]` entries — via a `crates` list or a `crate(...)` [predicate](./predicates.md). Plugins without any crate targeting will fail validation. @@ -444,6 +445,69 @@ echo '{"tool": "Bash", "input": "cargo test"}' | cargo agents hook claude pre-to You can also use `copilot`, `gemini`, `codex`, or `kiro` as the agent name. +## `[[predicate]]` + +Each `[[predicate]]` entry defines a custom predicate function that can be used in `predicates` expressions anywhere a predicate is accepted. Custom predicates extend the built-in predicate language with plugin-specific checks. + +| Field | Type | Description | +|-------|------|-------------| +| `name` | string | The predicate name. Must be a valid identifier (`[a-zA-Z][a-zA-Z0-9_]*`) and must not collide with builtins (`crate`, `shell`, `path_exists`, `env`, `not`, `any`, `all`). | +| `command` | string or table | The installation to run. Same shape as hook `command` (a string naming a `[[installations]]` entry or an inline table). | +| `args` | array of strings | Optional. Static arguments passed to the command before the dynamic argument. | + +### How custom predicates work + +When a predicate expression uses a function name that isn't a builtin, Symposium looks it up in the custom predicate registry. If found, it spawns the declared command with the static `args` followed by the raw argument text from the expression. + +```toml +[[installations]] +name = "cargo-bp-install" +source = "cargo" +crate = "cargo-bp" +executable = "cargo-bp" + +[[predicate]] +name = "battery_pack" +command = "cargo-bp-install" +args = ["bp", "status", "--check"] +``` + +Usage in a `predicates` expression: + +```toml +predicates = ["battery_pack(cli>=0.3)"] +``` + +This evaluates as: + +``` +cargo-bp bp status --check cli>=0.3 +``` + +Exit 0 means the predicate passes; non-zero means it fails. + +### Witness output (stdout JSON) + +On success (exit 0), the command may write a JSON object to stdout. If present and valid, the `selectedCrates` field drives `source = "crate"` skill resolution — the named crates are fetched for skills, just as if they'd been matched by a `crate(...)` predicate. + +```json +{ + "selectedCrates": [ + { "crate": "cli-battery-pack", "version": "0.3.1" } + ] +} +``` + +If stdout is empty or non-JSON, the predicate still passes but contributes no witness crates. Invalid JSON or bad semver in `version` fields produce a warning but don't fail the predicate. + +### Collisions + +If two plugins define the same predicate name, both definitions are skipped and a warning is emitted. Skills referencing the collided name evaluate as false. + +### Caching + +Results are cached by `(predicate_name, raw_arg)` for the duration of a single sync run. The same predicate called with the same argument is only spawned once. + ## `[[mcp_servers]]` Each `[[mcp_servers]]` entry declares an MCP server that Symposium registers into the agent's configuration during `sync --agent`. diff --git a/src/help_render.rs b/src/help_render.rs index 9b5d631c..f23223ed 100644 --- a/src/help_render.rs +++ b/src/help_render.rs @@ -212,6 +212,7 @@ mod tests { mcp_servers: vec![], subcommands, installations: vec![], + custom_predicates: vec![], }, source_name: "test".into(), source_dir: PathBuf::from("/test"), @@ -232,6 +233,7 @@ mod tests { plugins, standalone_skills: vec![], warnings: vec![], + custom_predicates: std::collections::HashMap::new(), } } diff --git a/src/hook.rs b/src/hook.rs index eb587296..ff612df2 100644 --- a/src/hook.rs +++ b/src/hook.rs @@ -968,6 +968,7 @@ mod tests { skills: vec![], mcp_servers: vec![], subcommands: BTreeMap::new(), + custom_predicates: vec![], }; crate::plugins::ParsedPlugin { path: std::path::PathBuf::from("test.toml"), diff --git a/src/plugins.rs b/src/plugins.rs index 94c745f6..54cc25e8 100644 --- a/src/plugins.rs +++ b/src/plugins.rs @@ -279,6 +279,18 @@ pub struct Installation { pub args: Vec, } +/// A validated custom predicate definition from a `[[predicate]]` entry. +#[derive(Debug, Clone, Serialize)] +pub struct CustomPredicate { + /// The predicate name (valid identifier, not a builtin). + pub name: String, + /// Name of the installation whose binary/script implements this predicate. + pub command: String, + /// Static arguments passed before the dynamic raw-arg. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub args: Vec, +} + /// A parsed plugin with its path and manifest. #[derive(Debug, Clone)] pub struct ParsedPlugin { @@ -324,6 +336,9 @@ pub struct Plugin { /// after `cargo agents`. Empty for plugins that vend no subcommands. #[serde(default, skip_serializing_if = "std::collections::BTreeMap::is_empty")] pub subcommands: std::collections::BTreeMap, + /// Custom predicate definitions vended by this plugin. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub custom_predicates: Vec, } impl Plugin { @@ -694,6 +709,20 @@ pub struct StandaloneSkill { pub origin: crate::skills::SkillOrigin, } +/// A resolved custom predicate definition in the registry. +/// +/// Stores the plugin index and predicate index within that plugin so that +/// acquisition can look up the `Installation` later. +#[derive(Debug, Clone)] +pub struct ResolvedCustomPredicate { + /// Index into `PluginRegistry.plugins` for the owning plugin. + pub plugin_index: usize, + /// The command installation name on the owning plugin. + pub command: String, + /// Static args passed before the dynamic raw-arg. + pub args: Vec, +} + /// Loaded plugin registry: plugins from TOML manifests and standalone skills /// discovered directly in plugin source directories. #[derive(Debug)] @@ -705,6 +734,9 @@ pub struct PluginRegistry { pub standalone_skills: Vec, /// Non-fatal load warnings for plugins or standalone skills that were skipped. pub warnings: Vec, + /// Global custom predicate registry. Built from all plugins' `custom_predicates`. + /// Collisions (same name from two plugins) are excluded and warned. + pub custom_predicates: std::collections::HashMap, } /// A non-fatal plugin source load failure. @@ -724,6 +756,17 @@ struct SourceDirContents { skill_files: Vec, } +/// A `[[predicate]]` entry in the raw TOML manifest. +#[derive(Debug, Deserialize)] +#[serde(deny_unknown_fields)] +struct RawCustomPredicate { + name: String, + /// Named installation or inline installation table. + command: RawInstallationRef, + #[serde(default)] + args: Vec, +} + /// Raw TOML manifest deserialized from a plugin `.toml` file. #[derive(Debug, Deserialize)] #[serde(deny_unknown_fields)] @@ -745,6 +788,8 @@ struct RawPluginManifest { /// `Plugin` is plural (`subcommands`). #[serde(default)] subcommand: std::collections::BTreeMap, + #[serde(default)] + predicate: Vec, } /// `[[installations]]` entry: a name plus the same fields as a `RawInlineInstallation`. @@ -1102,10 +1147,13 @@ pub fn load_registry(sym: &Symposium) -> PluginRegistry { "plugin registry loaded" ); + let custom_predicates = build_custom_predicate_registry(&plugins, &mut warnings); + PluginRegistry { plugins, standalone_skills, warnings, + custom_predicates, } } @@ -1483,13 +1531,28 @@ fn validate_manifest(manifest: RawPluginManifest) -> Result { subcommands.insert(name, sub); } + let mut custom_predicates = Vec::with_capacity(manifest.predicate.len()); + for raw in manifest.predicate { + custom_predicates.push(validate_custom_predicate( + raw, + &mut installations, + &mut names, + )?); + } + let predicates = crate::predicate::PredicateSet::merged(Some(manifest.crates), manifest.predicates); - // Every plugin must reference at least one crate somewhere — at the plugin, - // skill-group, hook, or MCP-server level — via `crates` or a `crate(...)` - // predicate. Otherwise it would never apply to any project. - let mentions_crate = predicates.mentions_crate() + // Every plugin must reference at least one crate (or custom predicate) + // somewhere — at the plugin, skill-group, hook, or MCP-server level — via + // `crates`, a `crate(...)` predicate, or a custom predicate. Otherwise it + // would never apply to any project. + let has_custom_predicate = predicates + .predicates + .iter() + .any(|p| matches!(p, crate::predicate::Predicate::Custom { .. })); + let mentions_crate = has_custom_predicate + || predicates.mentions_crate() || manifest .skills .iter() @@ -1517,6 +1580,7 @@ fn validate_manifest(manifest: RawPluginManifest) -> Result { skills: manifest.skills, mcp_servers: manifest.mcp_servers, subcommands, + custom_predicates, }) } @@ -1590,6 +1654,90 @@ fn validate_subcommand( }) } +fn validate_custom_predicate_name(name: &str) -> Result<()> { + if name.is_empty() { + bail!("predicate name is empty"); + } + let first = name.as_bytes()[0]; + if !first.is_ascii_alphabetic() { + bail!("predicate `{name}` must start with a letter"); + } + if !name.bytes().all(|c| c.is_ascii_alphanumeric() || c == b'_') { + bail!("predicate `{name}` has invalid characters (allow ASCII alphanumeric and `_`)"); + } + if crate::predicate::BUILTIN_PREDICATE_NAMES.contains(&name) { + bail!("predicate `{name}` collides with a builtin predicate name"); + } + Ok(()) +} + +/// Validate a `[[predicate]]` entry, promoting inline `command` if needed. +fn validate_custom_predicate( + raw: RawCustomPredicate, + installations: &mut Vec, + names: &mut std::collections::BTreeSet, +) -> Result { + validate_custom_predicate_name(&raw.name)?; + + let command = resolve_or_promote( + raw.command, + installations, + names, + &mut || format!("__pred_{}", raw.name), + &format!("predicate `{}`", raw.name), + )?; + + Ok(CustomPredicate { + name: raw.name, + command, + args: raw.args, + }) +} + +/// Collect custom predicates from all plugins, detecting collisions. +fn build_custom_predicate_registry( + plugins: &[ParsedPlugin], + warnings: &mut Vec, +) -> std::collections::HashMap { + use std::collections::HashMap; + + let mut registry: HashMap = HashMap::new(); + let mut collisions: std::collections::HashSet = std::collections::HashSet::new(); + + for (plugin_idx, parsed) in plugins.iter().enumerate() { + for cp in &parsed.plugin.custom_predicates { + if collisions.contains(&cp.name) { + continue; + } + if let Some(existing) = registry.get(&cp.name) { + // Collision: two different plugins define the same name. + // Skip both and warn. + let existing_plugin_name = &plugins[existing.plugin_index].plugin.name; + warnings.push(LoadWarning { + path: parsed.path.clone(), + message: format!( + "custom predicate `{}` defined by both `{}` and `{}` — skipping both", + cp.name, existing_plugin_name, parsed.plugin.name + ), + }); + registry.remove(&cp.name); + collisions.insert(cp.name.clone()); + } else { + registry.insert( + cp.name.clone(), + ResolvedCustomPredicate { + plugin_index: plugin_idx, + command: cp.command.clone(), + args: cp.args.clone(), + }, + ); + } + } + } + + registry +} + /// Validate skill-group source constraints that serde alone cannot express. /// /// When a group uses `source = "crate"`, a concrete crate must be named in a @@ -2196,6 +2344,7 @@ mod tests { mcp_servers: vec![], installations: Vec::new(), subcommands: BTreeMap::new(), + custom_predicates: vec![], }; assert!(plugin_wildcard.applies(&ctx(&workspace_crates))); @@ -2208,6 +2357,7 @@ mod tests { mcp_servers: vec![], installations: Vec::new(), subcommands: BTreeMap::new(), + custom_predicates: vec![], }; assert!(plugin_serde.applies(&ctx(&workspace_crates))); @@ -2220,6 +2370,7 @@ mod tests { mcp_servers: vec![], installations: Vec::new(), subcommands: BTreeMap::new(), + custom_predicates: vec![], }; assert!(!plugin_other.applies(&ctx(&workspace_crates))); @@ -2232,6 +2383,7 @@ mod tests { mcp_servers: vec![], installations: Vec::new(), subcommands: BTreeMap::new(), + custom_predicates: vec![], }; assert!(!plugin_version.applies(&ctx(&workspace_crates))); } diff --git a/src/predicate.rs b/src/predicate.rs index 3825013e..e13f31c3 100644 --- a/src/predicate.rs +++ b/src/predicate.rs @@ -30,6 +30,10 @@ use std::process::Command; use anyhow::{Context, Result, bail}; +/// Names reserved for builtin predicates. Custom predicates must not use these. +pub const BUILTIN_PREDICATE_NAMES: &[&str] = + &["crate", "shell", "path_exists", "env", "not", "any", "all"]; + /// The evaluation environment a predicate is checked against. /// /// The crate graph is passed explicitly; the OS environment (`shell`, @@ -64,6 +68,10 @@ pub enum Predicate { Any(Vec), /// `all(

, …)` — passes when every inner predicate does. All(Vec), + /// A plugin-defined predicate evaluated by spawning an external command. + /// Always returns false from `evaluate()` — requires external evaluation + /// via [`CustomPredicateEvaluator`]. + Custom { name: String, arg: String }, } impl Predicate { @@ -84,6 +92,7 @@ impl Predicate { Predicate::Not(inner) => !inner.evaluate(ctx), Predicate::Any(children) => children.iter().any(|p| p.evaluate(ctx)), Predicate::All(children) => children.iter().all(|p| p.evaluate(ctx)), + Predicate::Custom { .. } => false, } } @@ -134,6 +143,7 @@ impl Predicate { } Some(crates) } + Predicate::Custom { .. } => None, } } @@ -144,6 +154,7 @@ impl Predicate { Predicate::Crate(n, _) => n == name, Predicate::Not(p) => p.references_crate(name), Predicate::Any(v) | Predicate::All(v) => v.iter().any(|p| p.references_crate(name)), + Predicate::Custom { .. } => false, _ => false, } } @@ -154,6 +165,7 @@ impl Predicate { Predicate::Crate(..) | Predicate::CrateWildcard => true, Predicate::Not(p) => p.mentions_crate(), Predicate::Any(v) | Predicate::All(v) => v.iter().any(Predicate::mentions_crate), + Predicate::Custom { .. } => false, _ => false, } } @@ -165,6 +177,7 @@ impl Predicate { Predicate::Crate(..) => true, Predicate::Not(p) => p.has_concrete_crate(), Predicate::Any(v) | Predicate::All(v) => v.iter().any(Predicate::has_concrete_crate), + Predicate::Custom { .. } => false, _ => false, } } @@ -173,10 +186,12 @@ impl Predicate { /// appear in a [`witness`](Self::witness) — i.e. a `crate(serde)` not under /// any `not(...)`. A crate beneath a negation never contributes a crate to /// fetch from (the `Not` arm of `witness` discards its inner witness), so - /// it cannot anchor a `source = "crate"` group. + /// it cannot anchor a `source = "crate"` group. Custom predicates may + /// produce witnesses at runtime, so they count as fetchable. pub fn has_fetchable_crate(&self) -> bool { match self { Predicate::Crate(..) => true, + Predicate::Custom { .. } => true, Predicate::Not(_) => false, Predicate::Any(v) | Predicate::All(v) => v.iter().any(Predicate::has_fetchable_crate), _ => false, @@ -186,7 +201,8 @@ impl Predicate { /// Collect every crate name referenced anywhere in this predicate. /// /// Used for crates.io existence validation, so it ignores tree position - /// (a crate named under `not(...)` is still validated). + /// (a crate named under `not(...)` is still validated). Custom predicates + /// are a no-op — their crate names are dynamic. pub fn collect_crate_names(&self, out: &mut std::collections::BTreeSet) { match self { Predicate::Crate(name, _) => { @@ -198,6 +214,7 @@ impl Predicate { p.collect_crate_names(out); } } + Predicate::Custom { .. } => {} _ => {} } } @@ -377,7 +394,7 @@ impl<'de> serde::Deserialize<'de> for CrateList { // --- function-call predicate parsing --- /// Parse a single function-call predicate expression. -pub fn parse(input: &str) -> Result { +fn parse(input: &str) -> Result { let trimmed = input.trim(); let Some(open) = trimmed.find('(') else { bail!("predicate {trimmed:?} is not a function call (expected `name(arg)`)"); @@ -410,10 +427,10 @@ pub fn parse(input: &str) -> Result { } Ok(Predicate::All(preds)) } - other => bail!( - "unknown predicate `{other}` \ - (expected `crate`, `shell`, `path_exists`, `env`, `not`, `any`, or `all`)" - ), + other => Ok(Predicate::Custom { + name: other.to_string(), + arg: arg.to_string(), + }), } } @@ -536,6 +553,22 @@ impl<'a> AtomParser<'a> { ); } + // Function-call syntax: name(arg) — either `crate(...)` or a custom predicate. + if self.pos < self.input.len() && self.input.as_bytes()[self.pos] == b'(' { + self.pos += 1; // consume '(' + let arg = self.consume_balanced_parens()?; + return if name == "crate" { + // `crate(serde)` or `crate(serde>=1.0)` — parse inner as a crate atom + parse_crate_atom(&arg) + .with_context(|| format!("inside `crate(...)`: failed to parse `{arg}`")) + } else { + Ok(Predicate::Custom { + name: name.to_string(), + arg, + }) + }; + } + // Version constraint (starts with >=, <=, >, <, =, ^, ~). Bare `=` is // treated as `^` (compatible), matching Cargo's default. let version_req = if self.pos < self.input.len() { @@ -567,6 +600,32 @@ impl<'a> AtomParser<'a> { Ok(Predicate::Crate(name.to_string(), version_req)) } + + /// Consume content between balanced parentheses. The opening `(` has + /// already been consumed. Returns the content up to the matching `)`. + fn consume_balanced_parens(&mut self) -> Result { + let start = self.pos; + let mut depth: u32 = 1; + while self.pos < self.input.len() { + match self.input.as_bytes()[self.pos] { + b'(' => depth += 1, + b')' => { + depth -= 1; + if depth == 0 { + let content = self.input[start..self.pos].to_string(); + self.pos += 1; // consume closing ')' + return Ok(content); + } + } + _ => {} + } + self.pos += 1; + } + bail!( + "unmatched `(` at position {} — no closing `)` found", + start.saturating_sub(1) + ); + } } // --- environment evaluation --- @@ -657,6 +716,7 @@ impl std::fmt::Display for Predicate { Predicate::Not(inner) => write!(f, "not({inner})"), Predicate::Any(preds) => write!(f, "any({})", join(preds)), Predicate::All(preds) => write!(f, "all({})", join(preds)), + Predicate::Custom { name, arg } => write!(f, "{name}({arg})"), } } } @@ -669,6 +729,215 @@ fn join(preds: &[Predicate]) -> String { .join(", ") } +// --- custom predicate evaluation infrastructure --- + +/// A crate reported by a custom predicate's witness output. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct WitnessCrate { + pub name: String, + pub version: semver::Version, +} + +impl<'de> serde::Deserialize<'de> for WitnessCrate { + fn deserialize>( + deserializer: D, + ) -> std::result::Result { + #[derive(serde::Deserialize)] + struct Raw { + #[serde(rename = "crate")] + crate_name: String, + version: String, + } + let raw = Raw::deserialize(deserializer)?; + let version = semver::Version::parse(&raw.version).map_err(serde::de::Error::custom)?; + Ok(WitnessCrate { + name: raw.crate_name, + version, + }) + } +} + +/// Cached result of a custom predicate invocation. +#[derive(Debug, Clone)] +pub struct CustomPredicateResult { + /// Whether the predicate passed (exit 0). + pub passed: bool, + /// Parsed witness crates from stdout (empty if stdout was absent/invalid). + pub witness: Vec, +} + +/// A resolved entry ready for invocation. +#[derive(Debug)] +pub struct ResolvedPredicateEntry { + pub runnable: symposium_install::Runnable, + pub args: Vec, +} + +/// Runtime evaluator for custom predicates. Holds resolved runnables and +/// caches results by `(name, raw_arg)` for the duration of a single sync. +#[derive(Debug)] +pub struct CustomPredicateEvaluator { + entries: std::collections::HashMap, + cache: std::collections::HashMap<(String, String), CustomPredicateResult>, +} + +impl CustomPredicateEvaluator { + /// Create a new evaluator from resolved entries. + pub fn new(entries: std::collections::HashMap) -> Self { + Self { + entries, + cache: std::collections::HashMap::new(), + } + } + + /// Create an empty evaluator (no custom predicates available). + pub fn empty() -> Self { + Self::new(std::collections::HashMap::new()) + } + + /// Evaluate a custom predicate. Returns the cached result if available. + pub fn evaluate(&mut self, name: &str, arg: &str) -> &CustomPredicateResult { + let key = (name.to_string(), arg.to_string()); + if self.cache.contains_key(&key) { + return &self.cache[&key]; + } + + let result = self.run(name, arg); + self.cache.entry(key).or_insert(result) + } + + /// Get witness crates from a custom predicate. + /// + /// Returns `None` if the predicate failed (exit non-zero). + /// Returns `Some(&[])` if it passed but had no witness crates. + /// Returns `Some(&[...])` with parsed crates on valid witness JSON. + pub fn witness(&mut self, name: &str, arg: &str) -> Option<&[WitnessCrate]> { + let result = self.evaluate(name, arg); + if result.passed { + Some(&result.witness) + } else { + None + } + } + + fn run(&self, name: &str, arg: &str) -> CustomPredicateResult { + let Some(entry) = self.entries.get(name) else { + tracing::warn!(predicate = name, "custom predicate not found in registry"); + return CustomPredicateResult { + passed: false, + witness: Vec::new(), + }; + }; + + let mut full_args: Vec<&str> = entry.args.iter().map(|s| s.as_str()).collect(); + if !arg.is_empty() { + full_args.push(arg); + } + + tracing::debug!( + predicate = name, + args = ?full_args, + "spawning custom predicate" + ); + + match entry.runnable.spawn(&full_args) { + Ok(output) => { + if !output.stderr.is_empty() { + tracing::debug!( + predicate = name, + stderr = %String::from_utf8_lossy(&output.stderr), + "custom predicate stderr" + ); + } + let passed = output.status.success(); + let witness = if passed { + parse_witness_stdout(name, &output.stdout) + } else { + Vec::new() + }; + CustomPredicateResult { passed, witness } + } + Err(e) => { + tracing::warn!( + predicate = name, + error = %e, + "failed to spawn custom predicate" + ); + CustomPredicateResult { + passed: false, + witness: Vec::new(), + } + } + } + } +} + +/// Parse witness JSON from custom predicate stdout. +fn parse_witness_stdout(predicate_name: &str, stdout: &[u8]) -> Vec { + if stdout.is_empty() { + return Vec::new(); + } + + #[derive(serde::Deserialize)] + #[serde(rename_all = "camelCase")] + struct WitnessOutput { + selected_crates: Vec, + } + + let output: WitnessOutput = match serde_json::from_slice(stdout) { + Ok(o) => o, + Err(e) => { + tracing::warn!( + predicate = predicate_name, + error = %e, + "custom predicate stdout is not valid witness JSON" + ); + return Vec::new(); + } + }; + + let mut crates = Vec::new(); + for value in output.selected_crates { + match serde_json::from_value::(value) { + Ok(wc) => crates.push(wc), + Err(e) => { + tracing::warn!( + predicate = predicate_name, + error = %e, + "skipping witness crate with invalid format" + ); + } + } + } + crates +} + +/// Evaluate a predicate that may be a Custom variant, using the evaluator. +/// +/// For non-Custom predicates, delegates to [`Predicate::evaluate`]. +/// For `Custom`, uses the evaluator. +pub fn evaluate_predicate( + pred: &Predicate, + ctx: &PredicateContext, + evaluator: &mut CustomPredicateEvaluator, +) -> bool { + match pred { + Predicate::Custom { name, arg } => evaluator.evaluate(name, arg).passed, + _ => pred.evaluate(ctx), + } +} + +/// Evaluate a PredicateSet with custom predicate support (AND semantics). +pub fn evaluate_predicate_set( + set: &PredicateSet, + ctx: &PredicateContext, + evaluator: &mut CustomPredicateEvaluator, +) -> bool { + set.predicates + .iter() + .all(|p| evaluate_predicate(p, ctx, evaluator)) +} + #[cfg(test)] mod tests { use super::*; @@ -748,7 +1017,14 @@ mod tests { ]) ); assert!(parse("all()").is_err()); - assert!(parse("bogus(x)").is_err()); + // Unknown function names now parse as Custom predicates + assert_eq!( + parse("bogus(x)").unwrap(), + Predicate::Custom { + name: "bogus".into(), + arg: "x".into() + } + ); } // --- evaluation --- @@ -977,4 +1253,287 @@ mod tests { let c2: Container = toml::from_str(r#"crates = "serde""#).unwrap(); assert_eq!(c2.crates.0, vec![Predicate::Crate("serde".into(), None)]); } + + // --- Custom predicate parsing tests --- + + #[test] + fn parse_custom_predicate_expression() { + let p = parse("battery-pack(cli>=0.3)").unwrap(); + assert_eq!( + p, + Predicate::Custom { + name: "battery-pack".into(), + arg: "cli>=0.3".into() + } + ); + } + + #[test] + fn parse_custom_predicate_empty_arg() { + let p = parse("my-pred()").unwrap(); + assert_eq!( + p, + Predicate::Custom { + name: "my-pred".into(), + arg: "".into() + } + ); + } + + #[test] + fn parse_custom_predicate_arg_with_parens() { + let p = parse("foo(bar(baz))").unwrap(); + assert_eq!( + p, + Predicate::Custom { + name: "foo".into(), + arg: "bar(baz)".into() + } + ); + } + + #[test] + fn display_roundtrip_custom() { + let p = Predicate::Custom { + name: "battery-pack".into(), + arg: "cli>=0.3".into(), + }; + let displayed = p.to_string(); + assert_eq!(displayed, "battery-pack(cli>=0.3)"); + let reparsed = parse(&displayed).unwrap(); + assert_eq!(p, reparsed); + } + + #[test] + fn custom_not_confused_with_builtin() { + let p = parse("crate(serde)").unwrap(); + assert_eq!(p, Predicate::Crate("serde".into(), None)); + } + + #[test] + fn has_concrete_crate_custom_is_false() { + let p = Predicate::Custom { + name: "foo".into(), + arg: "x".into(), + }; + assert!(!p.has_concrete_crate()); + } + + #[test] + fn has_fetchable_crate_custom_is_true() { + let p = Predicate::Custom { + name: "foo".into(), + arg: "x".into(), + }; + assert!(p.has_fetchable_crate()); + } + + #[test] + fn mentions_crate_custom_is_false() { + let p = Predicate::Custom { + name: "foo".into(), + arg: "x".into(), + }; + assert!(!p.mentions_crate()); + } + + #[test] + fn references_crate_custom_is_false() { + let p = Predicate::Custom { + name: "foo".into(), + arg: "x".into(), + }; + assert!(!p.references_crate("foo")); + assert!(!p.references_crate("x")); + } + + #[test] + fn collect_crate_names_custom_is_noop() { + let p = Predicate::Custom { + name: "foo".into(), + arg: "x".into(), + }; + let mut names = std::collections::BTreeSet::new(); + p.collect_crate_names(&mut names); + assert!(names.is_empty()); + } + + // --- Custom predicate evaluation tests --- + + fn evaluator_with(entries: Vec<(&str, &str)>) -> CustomPredicateEvaluator { + let mut map = std::collections::HashMap::new(); + for (name, path) in entries { + map.insert( + name.to_string(), + ResolvedPredicateEntry { + runnable: symposium_install::Runnable::Exec(std::path::PathBuf::from(path)), + args: vec![], + }, + ); + } + CustomPredicateEvaluator::new(map) + } + + #[test] + fn evaluate_custom_predicate_pass() { + let mut eval = evaluator_with(vec![("foo", "/bin/true")]); + let result = eval.evaluate("foo", "x"); + assert!(result.passed); + } + + #[test] + fn evaluate_custom_predicate_fail() { + let mut eval = evaluator_with(vec![("foo", "/bin/false")]); + let result = eval.evaluate("foo", "x"); + assert!(!result.passed); + } + + #[test] + fn evaluate_custom_predicate_missing_from_registry() { + let mut eval = CustomPredicateEvaluator::empty(); + let result = eval.evaluate("nonexistent", "x"); + assert!(!result.passed); + } + + #[test] + fn evaluate_custom_predicate_spawn_failure() { + let mut eval = evaluator_with(vec![("foo", "/nonexistent/binary/zzz")]); + let result = eval.evaluate("foo", "x"); + assert!(!result.passed); + } + + #[test] + fn evaluate_custom_predicate_cached() { + use std::io::Write; + let tmp = tempfile::NamedTempFile::new().unwrap(); + let counter_path = tmp.path().to_path_buf(); + + let script = tempfile::Builder::new().suffix(".sh").tempfile().unwrap(); + writeln!( + script.as_file(), + "#!/bin/sh\necho x >> {}\nexit 0", + counter_path.display() + ) + .unwrap(); + + let mut entries = std::collections::HashMap::new(); + entries.insert( + "counter".to_string(), + ResolvedPredicateEntry { + runnable: symposium_install::Runnable::Script(script.path().to_path_buf()), + args: vec![], + }, + ); + let mut eval = CustomPredicateEvaluator::new(entries); + + // Evaluate twice with same (name, arg) + assert!(eval.evaluate("counter", "a").passed); + assert!(eval.evaluate("counter", "a").passed); + + // Script should have been called only once + let content = std::fs::read_to_string(&counter_path).unwrap(); + assert_eq!(content.lines().count(), 1); + } + + #[test] + fn evaluate_custom_predicate_args_appended() { + use std::io::Write; + let output_file = tempfile::NamedTempFile::new().unwrap(); + let output_path = output_file.path().to_path_buf(); + + let script = tempfile::Builder::new().suffix(".sh").tempfile().unwrap(); + writeln!( + script.as_file(), + "#!/bin/sh\necho \"$@\" > {}", + output_path.display() + ) + .unwrap(); + + let mut entries = std::collections::HashMap::new(); + entries.insert( + "checker".to_string(), + ResolvedPredicateEntry { + runnable: symposium_install::Runnable::Script(script.path().to_path_buf()), + args: vec!["--static".into(), "arg".into()], + }, + ); + let mut eval = CustomPredicateEvaluator::new(entries); + + let result = eval.evaluate("checker", "dynamic-arg"); + assert!(result.passed); + + let content = std::fs::read_to_string(&output_path).unwrap(); + assert_eq!(content.trim(), "--static arg dynamic-arg"); + } + + // --- Witness tests --- + + fn make_script_evaluator( + name: &str, + script_content: &str, + ) -> (CustomPredicateEvaluator, tempfile::NamedTempFile) { + use std::io::Write; + let script = tempfile::Builder::new().suffix(".sh").tempfile().unwrap(); + writeln!(script.as_file(), "#!/bin/sh\n{script_content}").unwrap(); + let mut entries = std::collections::HashMap::new(); + entries.insert( + name.to_string(), + ResolvedPredicateEntry { + runnable: symposium_install::Runnable::Script(script.path().to_path_buf()), + args: vec![], + }, + ); + (CustomPredicateEvaluator::new(entries), script) + } + + #[test] + fn witness_custom_with_selected_crates() { + let json = r#"{"selectedCrates":[{"crate":"cli-battery-pack","version":"0.3.1"}]}"#; + let (mut eval, _script) = make_script_evaluator("bp", &format!("printf '{json}'")); + let witness = eval.witness("bp", "cli").unwrap(); + assert_eq!(witness.len(), 1); + assert_eq!(witness[0].name, "cli-battery-pack"); + assert_eq!(witness[0].version, semver::Version::parse("0.3.1").unwrap()); + } + + #[test] + fn witness_custom_empty_stdout() { + let mut eval = evaluator_with(vec![("foo", "/bin/true")]); + let witness = eval.witness("foo", "x").unwrap(); + assert!(witness.is_empty()); + } + + #[test] + fn witness_custom_exit_nonzero() { + let mut eval = evaluator_with(vec![("foo", "/bin/false")]); + let witness = eval.witness("foo", "x"); + assert!(witness.is_none()); + } + + #[test] + fn witness_custom_invalid_json() { + let (mut eval, _script) = make_script_evaluator("bp", "printf 'not json at all'"); + let witness = eval.witness("bp", "x").unwrap(); + assert!(witness.is_empty()); + } + + #[test] + fn witness_custom_invalid_version() { + let json = r#"{"selectedCrates":[{"crate":"good","version":"1.0.0"},{"crate":"bad","version":"not-semver"}]}"#; + let (mut eval, _script) = make_script_evaluator("bp", &format!("printf '{json}'")); + let witness = eval.witness("bp", "x").unwrap(); + assert_eq!(witness.len(), 1); + assert_eq!(witness[0].name, "good"); + } + + #[test] + fn witness_custom_multiple_crates() { + let json = r#"{"selectedCrates":[{"crate":"a","version":"1.0.0"},{"crate":"b","version":"2.0.0"},{"crate":"c","version":"3.0.0"}]}"#; + let (mut eval, _script) = make_script_evaluator("bp", &format!("printf '{json}'")); + let witness = eval.witness("bp", "x").unwrap(); + assert_eq!(witness.len(), 3); + assert_eq!(witness[0].name, "a"); + assert_eq!(witness[1].name, "b"); + assert_eq!(witness[2].name, "c"); + } } diff --git a/src/skills.rs b/src/skills.rs index 5a81e363..bd1b5349 100644 --- a/src/skills.rs +++ b/src/skills.rs @@ -214,6 +214,83 @@ pub async fn skills_applicable_to( results } +/// Like [`skills_applicable_to`], but with a custom predicate evaluator. +pub async fn skills_applicable_to_with_evaluator( + sym: &Symposium, + registry: &PluginRegistry, + workspace_crates: &[crate::crate_sources::WorkspaceCrate], + evaluator: &mut predicate::CustomPredicateEvaluator, +) -> Vec { + let mut results = Vec::new(); + + let for_crates = crate::crate_sources::crate_pairs(workspace_crates); + let ctx = PredicateContext::new(&for_crates); + + // Skills from plugin manifests. We iterate these separately + // because we lazily load skill groups, so there + // is extra logic. + for parsed in ®istry.plugins { + let plugin = &parsed.plugin; + // Plugin-level predicates gate everything below. Evaluated before + // group fetching to avoid wasted work. + if !predicate::evaluate_predicate_set(&plugin.predicates, &ctx, evaluator) { + tracing::debug!( + report = %crate::report::ReportEvent::PluginConsidered { + plugin: plugin.name.clone(), + matched: false, + reason: Some("plugin-level predicates not satisfied".into()), + }, + ); + continue; + } + + tracing::debug!( + report = %crate::report::ReportEvent::PluginConsidered { + plugin: plugin.name.clone(), + matched: true, + reason: None, + }, + ); + + for group in &plugin.skills { + let skills = load_skills_for_group(sym, parsed, group, workspace_crates, &ctx).await; + for (skill, origin) in skills { + collect_skill_applicable_to_with_evaluator( + skill, + origin, + &plugin.name, + &ctx, + evaluator, + &mut results, + ); + } + } + } + + // Standalone skills already carry their own `SkillOrigin`. + if !registry.standalone_skills.is_empty() { + tracing::debug!( + report = %crate::report::ReportEvent::PluginConsidered { + plugin: "(standalone skills)".into(), + matched: true, + reason: None, + }, + ); + } + for entry in ®istry.standalone_skills { + collect_skill_applicable_to_with_evaluator( + entry.skill.clone(), + entry.origin.clone(), + "(standalone skills)", + &ctx, + evaluator, + &mut results, + ); + } + + results +} + /// Discover and load skills for a group, applying pre-fetch filtering. /// /// Checks group-level `crates` predicates against `for_crates` before @@ -802,6 +879,39 @@ fn collect_skill_applicable_to( results.push(SkillWithGroupContext { skill, origin }); } +/// Like [`collect_skill_applicable_to`] but uses the custom predicate evaluator +/// for skill-level predicate evaluation. +fn collect_skill_applicable_to_with_evaluator( + skill: Skill, + origin: SkillOrigin, + plugin_name: &str, + ctx: &PredicateContext, + evaluator: &mut predicate::CustomPredicateEvaluator, + results: &mut Vec, +) { + if !predicate::evaluate_predicate_set(&skill.predicates, ctx, evaluator) { + tracing::debug!( + report = %crate::report::ReportEvent::SkillConsidered { + skill: skill.name().to_string(), + plugin: plugin_name.to_string(), + matched: false, + reason: Some("skill-level predicates not satisfied".into()), + }, + ); + return; + } + + tracing::debug!( + report = %crate::report::ReportEvent::SkillConsidered { + skill: skill.name().to_string(), + plugin: plugin_name.to_string(), + matched: true, + reason: None, + }, + ); + results.push(SkillWithGroupContext { skill, origin }); +} + /// Raw frontmatter fields extracted from a SKILL.md file. /// `crates` is comma-separated on a single line. #[derive(Debug)] @@ -1297,6 +1407,7 @@ mod tests { mcp_servers: vec![], installations: Vec::new(), subcommands: BTreeMap::new(), + custom_predicates: vec![], }; let registry = PluginRegistry { @@ -1308,6 +1419,7 @@ mod tests { }], standalone_skills: vec![], warnings: vec![], + custom_predicates: std::collections::HashMap::new(), }; // Query for serde - should find no skills because plugin doesn't apply @@ -1344,6 +1456,7 @@ mod tests { mcp_servers: vec![], installations: Vec::new(), subcommands: BTreeMap::new(), + custom_predicates: vec![], }; let registry = PluginRegistry { @@ -1355,6 +1468,7 @@ mod tests { }], standalone_skills: vec![], warnings: vec![], + custom_predicates: std::collections::HashMap::new(), }; // Query for serde - should find no skills because group doesn't match @@ -1409,6 +1523,7 @@ mod tests { mcp_servers: vec![], installations: Vec::new(), subcommands: BTreeMap::new(), + custom_predicates: vec![], }; let registry = PluginRegistry { @@ -1420,6 +1535,7 @@ mod tests { }], standalone_skills: vec![], warnings: vec![], + custom_predicates: std::collections::HashMap::new(), }; let workspace_crates = vec![crate::crate_sources::WorkspaceCrate { @@ -1479,6 +1595,7 @@ mod tests { mcp_servers: vec![], installations: Vec::new(), subcommands: Default::default(), + custom_predicates: vec![], }; let registry = PluginRegistry { @@ -1490,6 +1607,7 @@ mod tests { }], standalone_skills: vec![], warnings: vec![], + custom_predicates: std::collections::HashMap::new(), }; let workspace = vec![crate::crate_sources::WorkspaceCrate { @@ -1550,6 +1668,7 @@ mod tests { mcp_servers: vec![], installations: Vec::new(), subcommands: Default::default(), + custom_predicates: vec![], }; let registry = PluginRegistry { @@ -1561,6 +1680,7 @@ mod tests { }], standalone_skills: vec![], warnings: vec![], + custom_predicates: std::collections::HashMap::new(), }; let workspace = vec![crate::crate_sources::WorkspaceCrate { @@ -1685,6 +1805,7 @@ mod tests { }, }], warnings: vec![], + custom_predicates: std::collections::HashMap::new(), }; let sym = crate::config::Symposium::from_dir(tmp.path()); diff --git a/src/subcommand_dispatch.rs b/src/subcommand_dispatch.rs index a6bcb581..730d4450 100644 --- a/src/subcommand_dispatch.rs +++ b/src/subcommand_dispatch.rs @@ -187,6 +187,7 @@ mod tests { skills: vec![], mcp_servers: vec![], subcommands, + custom_predicates: vec![], }, source_name: "test".into(), source_dir: PathBuf::from("/test"), @@ -207,6 +208,7 @@ mod tests { plugins, standalone_skills: vec![], warnings: vec![], + custom_predicates: std::collections::HashMap::new(), } } diff --git a/src/sync.rs b/src/sync.rs index 4cf23961..1ef109af 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -164,6 +164,70 @@ fn propagate_user_skill(source_dir: &Path, dest_dir: &Path, project_root: &Path) Ok(true) } +/// Resolve custom predicate installations from the registry into a +/// `CustomPredicateEvaluator`. Acquisition failures are logged as warnings +/// and the corresponding predicate is marked non-functional (evaluates false). +async fn resolve_custom_predicates( + sym: &Symposium, + registry: &plugins::PluginRegistry, +) -> crate::predicate::CustomPredicateEvaluator { + use crate::predicate::{CustomPredicateEvaluator, ResolvedPredicateEntry}; + + if registry.custom_predicates.is_empty() { + return CustomPredicateEvaluator::empty(); + } + + let mut entries = std::collections::HashMap::new(); + + for (name, resolved) in ®istry.custom_predicates { + let plugin = ®istry.plugins[resolved.plugin_index]; + let Some(install) = plugin.plugin.get_installation(&resolved.command) else { + tracing::warn!( + predicate = name, + command = &resolved.command, + "custom predicate references unknown installation" + ); + continue; + }; + + let acquired = + match crate::installation::acquire_installation(sym, install, None, None).await { + Ok(a) => a, + Err(e) => { + tracing::warn!( + predicate = name, + error = %e, + "failed to acquire custom predicate installation" + ); + continue; + } + }; + + let runnable = + match crate::installation::resolve_runnable(acquired, &format!("predicate `{name}`")) { + Ok(r) => r, + Err(e) => { + tracing::warn!( + predicate = name, + error = %e, + "failed to resolve custom predicate runnable" + ); + continue; + } + }; + + entries.insert( + name.clone(), + ResolvedPredicateEntry { + runnable, + args: resolved.args.clone(), + }, + ); + } + + CustomPredicateEvaluator::new(entries) +} + /// Run the full sync: discover applicable skills, install into agent dirs, /// clean up stale installations. pub async fn sync(sym: &Symposium, cwd: &Path) -> Result<()> { @@ -189,8 +253,13 @@ pub async fn sync(sym: &Symposium, cwd: &Path) -> Result<()> { }, ); + // Resolve custom predicate installations and build the evaluator. + let mut evaluator = resolve_custom_predicates(sym, ®istry).await; + // Find all applicable skills - let applicable = skills::skills_applicable_to(sym, ®istry, &workspace).await; + let applicable = + skills::skills_applicable_to_with_evaluator(sym, ®istry, &workspace, &mut evaluator) + .await; // Dedup by `(skill_name, SkillOrigin)`: two `Crate` origins with the // same (name, version) collapse (the skills are the same logical bytes diff --git a/symposium-install/src/lib.rs b/symposium-install/src/lib.rs index 3f10427f..360e9377 100644 --- a/symposium-install/src/lib.rs +++ b/symposium-install/src/lib.rs @@ -631,6 +631,21 @@ pub enum Runnable { Script(PathBuf), } +impl Runnable { + /// Spawn this runnable with the given arguments, wait for it to finish, + /// and return the captured output. + pub fn spawn( + &self, + args: &[impl AsRef], + ) -> std::io::Result { + use std::process::Command; + match self { + Runnable::Exec(path) => Command::new(path).args(args).output(), + Runnable::Script(path) => Command::new("sh").arg(path).args(args).output(), + } + } +} + /// Ensure a path is executable on Unix. No-op on other platforms. pub fn make_executable(path: &Path) -> Result<()> { #[cfg(unix)] diff --git a/tests/custom_predicates.rs b/tests/custom_predicates.rs new file mode 100644 index 00000000..14e80683 --- /dev/null +++ b/tests/custom_predicates.rs @@ -0,0 +1,93 @@ +//! Integration tests for custom predicate extensions. + +use std::path::Path; + +use symposium_testlib::{TestMode, with_fixture}; + +/// Write a shell script to the given path and make it executable. +fn write_script(path: &Path, content: &str) { + std::fs::write(path, format!("#!/bin/sh\n{content}")).unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o755)).unwrap(); + } +} + +/// `sync` installs a skill when the custom predicate passes (exit 0). +#[tokio::test] +async fn sync_custom_predicate_installs_skill() { + with_fixture( + TestMode::SimulationOnly, + &["custom-predicate0"], + async |mut ctx| { + // Write a passing predicate script + let script_path = ctx.tempdir.join("bp-checker.sh"); + write_script(&script_path, "exit 0"); + + ctx.symposium(&["init", "--add-agent", "claude"]).await?; + ctx.symposium(&["sync"]).await?; + + let skills_dir = ctx + .workspace_root + .as_ref() + .unwrap() + .join(".claude") + .join("skills"); + let skill_dir = skills_dir.join("bp-skill"); + assert!( + skill_dir.join("SKILL.md").exists(), + "skill should be installed when predicate passes; skills_dir={}, contents={:?}", + skills_dir.display(), + std::fs::read_dir(&skills_dir) + .ok() + .map(|d| d.flatten().map(|e| e.path()).collect::>()), + ); + Ok(()) + }, + ) + .await + .unwrap(); +} + +/// `sync` skips a skill when the custom predicate fails (exit non-zero). +#[tokio::test] +async fn sync_custom_predicate_fails_skips_skill() { + with_fixture( + TestMode::SimulationOnly, + &["custom-predicate0"], + async |mut ctx| { + // Write a failing predicate script + let script_path = ctx.tempdir.join("bp-checker.sh"); + write_script(&script_path, "exit 1"); + + ctx.symposium(&["init", "--add-agent", "claude"]).await?; + ctx.symposium(&["sync"]).await?; + + let skills_dir = ctx + .workspace_root + .as_ref() + .unwrap() + .join(".claude") + .join("skills"); + // The skill should NOT be installed + let entries: Vec<_> = std::fs::read_dir(&skills_dir) + .ok() + .map(|d| { + d.flatten() + .filter(|e| e.path().is_dir()) + .filter(|e| e.path().join("SKILL.md").exists()) + .collect() + }) + .unwrap_or_default(); + assert!( + entries.is_empty(), + "no skills should be installed when predicate fails; got: {:?}", + entries.iter().map(|e| e.path()).collect::>(), + ); + Ok(()) + }, + ) + .await + .unwrap(); +} diff --git a/tests/fixtures/custom-predicate0/Cargo.toml b/tests/fixtures/custom-predicate0/Cargo.toml new file mode 100644 index 00000000..43692309 --- /dev/null +++ b/tests/fixtures/custom-predicate0/Cargo.toml @@ -0,0 +1,7 @@ +[package] +name = "test-workspace" +version = "0.1.0" +edition = "2021" + +[dependencies] +serde = "1.0" diff --git a/tests/fixtures/custom-predicate0/dot-symposium/config.toml b/tests/fixtures/custom-predicate0/dot-symposium/config.toml new file mode 100644 index 00000000..777c5d87 --- /dev/null +++ b/tests/fixtures/custom-predicate0/dot-symposium/config.toml @@ -0,0 +1,5 @@ +hook-scope = "project" + +[defaults] +symposium-recommendations = false +user-plugins = true diff --git a/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml b/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml new file mode 100644 index 00000000..9f2ffbf4 --- /dev/null +++ b/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml @@ -0,0 +1,13 @@ +name = "bp-plugin" +crates = ["battery_pack(cli)"] + +[[installations]] +name = "bp-checker" +executable = "$TEST_DIR/bp-checker.sh" + +[[predicate]] +name = "battery_pack" +command = "bp-checker" + +[[skills]] +source.path = "skills" diff --git a/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/skills/bp-skill/SKILL.md b/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/skills/bp-skill/SKILL.md new file mode 100644 index 00000000..bcd0f063 --- /dev/null +++ b/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/skills/bp-skill/SKILL.md @@ -0,0 +1,6 @@ +--- +name: bp-skill +description: Battery pack skill +--- + +Use the battery pack CLI. diff --git a/tests/fixtures/custom-predicate0/src/lib.rs b/tests/fixtures/custom-predicate0/src/lib.rs new file mode 100644 index 00000000..e69de29b From 7210fb31f0b5180727ad5f5b66da1ca3b7660290 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Jun 2026 15:17:37 +0000 Subject: [PATCH 02/13] Integrate custom predicate evaluator into PredicateContext Move the CustomPredicateEvaluator's entries and cache directly into PredicateContext, making evaluate()/witness() take &mut self. This eliminates the separate _with_evaluator function variants and fixes a bug where group-level custom predicates were silently evaluated as false (because load_skills_for_group called evaluate() without the external evaluator). Co-authored-by: Claude --- src/help_render.rs | 1 + src/hook.rs | 18 +- src/plugins.rs | 12 +- src/predicate.rs | 473 +++++++++++++++++++++---------------- src/skills.rs | 180 +++++--------- src/subcommand_dispatch.rs | 41 ++-- src/sync.rs | 39 ++- 7 files changed, 372 insertions(+), 392 deletions(-) diff --git a/src/help_render.rs b/src/help_render.rs index f23223ed..bb3188eb 100644 --- a/src/help_render.rs +++ b/src/help_render.rs @@ -161,6 +161,7 @@ fn collect_section( builtins.sort(); let mut plugins = applicable_subcommands(registry, deps) + .into_iter() .filter(|(_, _, subcommand)| subcommand.audience == target) .map(|(_, name, subcommand)| (name.to_string(), subcommand.description.clone())) .collect::>(); diff --git a/src/hook.rs b/src/hook.rs index ff612df2..6f3d59f5 100644 --- a/src/hook.rs +++ b/src/hook.rs @@ -413,7 +413,7 @@ fn discovery_hint(sym: &Symposium, cwd: &Path) -> Option { let registry = load_registry(sym); let deps = crate::crate_sources::crate_pairs(&workspace_crates(cwd)); - let any_subcommand = applicable_subcommands(®istry, &deps).next().is_some(); + let any_subcommand = !applicable_subcommands(®istry, &deps).is_empty(); any_subcommand.then(|| { format!( @@ -501,8 +501,8 @@ pub async fn dispatch_plugin_hooks( } else { Vec::new() }; - let ctx = crate::predicate::PredicateContext::new(&deps); - let hooks = dispatched_hooks_for_payload(&plugins, sym_input, host_agent, &ctx); + let mut ctx = crate::predicate::PredicateContext::new(&deps); + let hooks = dispatched_hooks_for_payload(&plugins, sym_input, host_agent, &mut ctx); let mut output = prior_output; @@ -663,7 +663,7 @@ fn dispatched_hooks_for_payload( plugins: &[ParsedPlugin], input: &symposium::InputEvent, host_agent: HookAgent, - ctx: &crate::predicate::PredicateContext, + ctx: &mut crate::predicate::PredicateContext, ) -> Vec { tracing::trace!(?input, "matching hooks for payload"); @@ -994,7 +994,7 @@ mod tests { &[plugin], &pre_tool_use_input(), HookAgent::Claude, - &crate::predicate::PredicateContext::new(&[]), + &mut crate::predicate::PredicateContext::new(&[]), ); assert!(hooks.is_empty(), "plugin-level false should drop all hooks"); } @@ -1006,7 +1006,7 @@ mod tests { &[plugin], &pre_tool_use_input(), HookAgent::Claude, - &crate::predicate::PredicateContext::new(&[]), + &mut crate::predicate::PredicateContext::new(&[]), ); assert!(hooks.is_empty(), "hook-level false should drop the hook"); } @@ -1018,7 +1018,7 @@ mod tests { &[plugin], &pre_tool_use_input(), HookAgent::Claude, - &crate::predicate::PredicateContext::new(&[]), + &mut crate::predicate::PredicateContext::new(&[]), ); assert_eq!(hooks.len(), 1); } @@ -1039,7 +1039,7 @@ mod tests { &[plugin.clone()], &pre_tool_use_input(), HookAgent::Claude, - &crate::predicate::PredicateContext::new(&[]), + &mut crate::predicate::PredicateContext::new(&[]), ); assert!( empty.is_empty(), @@ -1052,7 +1052,7 @@ mod tests { &[plugin], &pre_tool_use_input(), HookAgent::Claude, - &crate::predicate::PredicateContext::new(&deps), + &mut crate::predicate::PredicateContext::new(&deps), ); assert_eq!( matched.len(), diff --git a/src/plugins.rs b/src/plugins.rs index 54cc25e8..acf17d9d 100644 --- a/src/plugins.rs +++ b/src/plugins.rs @@ -343,7 +343,7 @@ pub struct Plugin { impl Plugin { /// Check if this plugin's activation predicates hold in `ctx`. - pub fn applies(&self, ctx: &crate::predicate::PredicateContext) -> bool { + pub fn applies(&self, ctx: &mut crate::predicate::PredicateContext) -> bool { self.predicates.evaluate(ctx) } @@ -367,7 +367,7 @@ impl Plugin { /// separately. pub fn applicable_mcp_servers( &self, - ctx: &crate::predicate::PredicateContext, + ctx: &mut crate::predicate::PredicateContext, ) -> Vec { self.mcp_servers .iter() @@ -2346,7 +2346,7 @@ mod tests { subcommands: BTreeMap::new(), custom_predicates: vec![], }; - assert!(plugin_wildcard.applies(&ctx(&workspace_crates))); + assert!(plugin_wildcard.applies(&mut ctx(&workspace_crates))); // Plugin targeting serde - should apply let plugin_serde = Plugin { @@ -2359,7 +2359,7 @@ mod tests { subcommands: BTreeMap::new(), custom_predicates: vec![], }; - assert!(plugin_serde.applies(&ctx(&workspace_crates))); + assert!(plugin_serde.applies(&mut ctx(&workspace_crates))); // Plugin targeting non-existent crate - should not apply let plugin_other = Plugin { @@ -2372,7 +2372,7 @@ mod tests { subcommands: BTreeMap::new(), custom_predicates: vec![], }; - assert!(!plugin_other.applies(&ctx(&workspace_crates))); + assert!(!plugin_other.applies(&mut ctx(&workspace_crates))); // Plugin with version predicate - should reject wrong version let plugin_version = Plugin { @@ -2385,7 +2385,7 @@ mod tests { subcommands: BTreeMap::new(), custom_predicates: vec![], }; - assert!(!plugin_version.applies(&ctx(&workspace_crates))); + assert!(!plugin_version.applies(&mut ctx(&workspace_crates))); } #[test] diff --git a/src/predicate.rs b/src/predicate.rs index e13f31c3..6359000d 100644 --- a/src/predicate.rs +++ b/src/predicate.rs @@ -37,15 +37,65 @@ pub const BUILTIN_PREDICATE_NAMES: &[&str] = /// The evaluation environment a predicate is checked against. /// /// The crate graph is passed explicitly; the OS environment (`shell`, -/// `path_exists`, `env`) is read ambiently at evaluation time. -#[derive(Debug, Clone, Copy)] +/// `path_exists`, `env`) is read ambiently at evaluation time. Custom +/// (plugin-defined) predicates are resolved entries whose results are cached +/// for the lifetime of the context. +#[derive(Debug)] pub struct PredicateContext<'a> { pub crates: &'a [(String, semver::Version)], + custom_entries: std::collections::HashMap, + custom_cache: std::collections::HashMap<(String, String), CustomPredicateResult>, } impl<'a> PredicateContext<'a> { pub fn new(crates: &'a [(String, semver::Version)]) -> Self { - Self { crates } + Self { + crates, + custom_entries: std::collections::HashMap::new(), + custom_cache: std::collections::HashMap::new(), + } + } + + pub fn with_custom_predicates( + crates: &'a [(String, semver::Version)], + entries: std::collections::HashMap, + ) -> Self { + Self { + crates, + custom_entries: entries, + custom_cache: std::collections::HashMap::new(), + } + } + + /// Evaluate a custom predicate by name and argument, returning the cached + /// result if already computed. + fn evaluate_custom(&mut self, name: &str, arg: &str) -> bool { + let key = (name.to_string(), arg.to_string()); + if let Some(result) = self.custom_cache.get(&key) { + return result.passed; + } + let result = run_custom_predicate(&self.custom_entries, name, arg); + let passed = result.passed; + self.custom_cache.insert(key, result); + passed + } + + /// Get witness crates from a custom predicate's cached result. + /// + /// Returns `None` if the predicate failed or hasn't been evaluated. + /// Returns `Some(&[])` if it passed but had no witness crates. + pub fn custom_witness(&mut self, name: &str, arg: &str) -> Option<&[WitnessCrate]> { + let key = (name.to_string(), arg.to_string()); + if !self.custom_cache.contains_key(&key) { + let result = run_custom_predicate(&self.custom_entries, name, arg); + self.custom_cache.insert(key.clone(), result); + } + let result = self.custom_cache.get(&key).unwrap(); + if result.passed { + Some(&result.witness) + } else { + None + } } } @@ -69,8 +119,7 @@ pub enum Predicate { /// `all(

, …)` — passes when every inner predicate does. All(Vec), /// A plugin-defined predicate evaluated by spawning an external command. - /// Always returns false from `evaluate()` — requires external evaluation - /// via [`CustomPredicateEvaluator`]. + /// Evaluated via the custom predicate entries in [`PredicateContext`]. Custom { name: String, arg: String }, } @@ -78,9 +127,9 @@ impl Predicate { /// True if this predicate holds in `ctx`. /// /// Short-circuits (`any` stops at the first true child, `all` at the first - /// false) and allocates nothing — this is the gating hot path. Use - /// [`Predicate::witness`] when the satisfying crate set is also needed. - pub fn evaluate(&self, ctx: &PredicateContext) -> bool { + /// false). Use [`Predicate::witness`] when the satisfying crate set is also + /// needed. + pub fn evaluate(&self, ctx: &mut PredicateContext) -> bool { match self { Predicate::Crate(name, version_req) => ctx.crates.iter().any(|(dep_name, dep_ver)| { dep_name == name && version_req.as_ref().is_none_or(|req| req.matches(dep_ver)) @@ -92,7 +141,7 @@ impl Predicate { Predicate::Not(inner) => !inner.evaluate(ctx), Predicate::Any(children) => children.iter().any(|p| p.evaluate(ctx)), Predicate::All(children) => children.iter().all(|p| p.evaluate(ctx)), - Predicate::Custom { .. } => false, + Predicate::Custom { name, arg } => ctx.evaluate_custom(name, arg), } } @@ -103,7 +152,7 @@ impl Predicate { /// unions the witnesses of its *true* children, `all` unions all children's /// witnesses (when all hold), and `not` contributes nothing (negation is /// about absence). Non-crate leaves contribute an empty witness. - pub fn witness(&self, ctx: &PredicateContext) -> Option> { + pub fn witness(&self, ctx: &mut PredicateContext) -> Option> { match self { Predicate::Crate(name, version_req) => { let hits: Vec<_> = ctx @@ -143,7 +192,14 @@ impl Predicate { } Some(crates) } - Predicate::Custom { .. } => None, + Predicate::Custom { name, arg } => { + let witness = ctx.custom_witness(name, arg)?; + let pairs = witness + .iter() + .map(|wc| (wc.name.clone(), wc.version.clone())) + .collect(); + Some(pairs) + } } } @@ -258,13 +314,13 @@ impl PredicateSet { } /// True if every predicate holds (or the set is empty). - pub fn evaluate(&self, ctx: &PredicateContext) -> bool { + pub fn evaluate(&self, ctx: &mut PredicateContext) -> bool { self.predicates.iter().all(|p| p.evaluate(ctx)) } /// Witness for the whole set (treated as one big `all(...)`): `None` if any /// predicate is false, otherwise the deduplicated union of witnesses. - pub fn witness(&self, ctx: &PredicateContext) -> Option> { + pub fn witness(&self, ctx: &mut PredicateContext) -> Option> { let mut crates = Vec::new(); for p in &self.predicates { crates.extend(p.witness(ctx)?); @@ -311,7 +367,7 @@ impl PredicateSet { /// resolution: the concrete crates whose source trees to fetch skills from. pub fn union_matched_crates( sets: &[&PredicateSet], - ctx: &PredicateContext, + ctx: &mut PredicateContext, ) -> Vec<(String, semver::Version)> { let mut seen = std::collections::HashSet::new(); let mut result = Vec::new(); @@ -393,6 +449,20 @@ impl<'de> serde::Deserialize<'de> for CrateList { // --- function-call predicate parsing --- +/// Validate that a custom predicate name uses only `[a-zA-Z][a-zA-Z0-9_]*`. +/// Hyphens are intentionally rejected to keep the namespace aligned with +/// `[[predicate]]` definition validation. +fn validate_custom_predicate_name_at_parse(name: &str) -> Result<()> { + if let Some(pos) = name.find(|c: char| !c.is_ascii_alphanumeric() && c != '_') { + bail!( + "custom predicate name `{name}` contains invalid character '{}' at position {pos} \ + (only ASCII alphanumeric and `_` allowed)", + name.as_bytes()[pos] as char, + ); + } + Ok(()) +} + /// Parse a single function-call predicate expression. fn parse(input: &str) -> Result { let trimmed = input.trim(); @@ -427,10 +497,13 @@ fn parse(input: &str) -> Result { } Ok(Predicate::All(preds)) } - other => Ok(Predicate::Custom { - name: other.to_string(), - arg: arg.to_string(), - }), + other => { + validate_custom_predicate_name_at_parse(other)?; + Ok(Predicate::Custom { + name: other.to_string(), + arg: arg.to_string(), + }) + } } } @@ -562,6 +635,7 @@ impl<'a> AtomParser<'a> { parse_crate_atom(&arg) .with_context(|| format!("inside `crate(...)`: failed to parse `{arg}`")) } else { + validate_custom_predicate_name_at_parse(name)?; Ok(Predicate::Custom { name: name.to_string(), arg, @@ -766,107 +840,64 @@ pub struct CustomPredicateResult { pub witness: Vec, } -/// A resolved entry ready for invocation. +/// A resolved custom predicate entry ready for invocation. #[derive(Debug)] pub struct ResolvedPredicateEntry { pub runnable: symposium_install::Runnable, pub args: Vec, } -/// Runtime evaluator for custom predicates. Holds resolved runnables and -/// caches results by `(name, raw_arg)` for the duration of a single sync. -#[derive(Debug)] -pub struct CustomPredicateEvaluator { - entries: std::collections::HashMap, - cache: std::collections::HashMap<(String, String), CustomPredicateResult>, -} - -impl CustomPredicateEvaluator { - /// Create a new evaluator from resolved entries. - pub fn new(entries: std::collections::HashMap) -> Self { - Self { - entries, - cache: std::collections::HashMap::new(), - } - } +/// Spawn a custom predicate command and return the result. +fn run_custom_predicate( + entries: &std::collections::HashMap, + name: &str, + arg: &str, +) -> CustomPredicateResult { + let Some(entry) = entries.get(name) else { + tracing::warn!(predicate = name, "custom predicate not found in registry"); + return CustomPredicateResult { + passed: false, + witness: Vec::new(), + }; + }; - /// Create an empty evaluator (no custom predicates available). - pub fn empty() -> Self { - Self::new(std::collections::HashMap::new()) + let mut full_args: Vec<&str> = entry.args.iter().map(|s| s.as_str()).collect(); + if !arg.is_empty() { + full_args.push(arg); } - /// Evaluate a custom predicate. Returns the cached result if available. - pub fn evaluate(&mut self, name: &str, arg: &str) -> &CustomPredicateResult { - let key = (name.to_string(), arg.to_string()); - if self.cache.contains_key(&key) { - return &self.cache[&key]; - } - - let result = self.run(name, arg); - self.cache.entry(key).or_insert(result) - } + tracing::debug!( + predicate = name, + args = ?full_args, + "spawning custom predicate" + ); - /// Get witness crates from a custom predicate. - /// - /// Returns `None` if the predicate failed (exit non-zero). - /// Returns `Some(&[])` if it passed but had no witness crates. - /// Returns `Some(&[...])` with parsed crates on valid witness JSON. - pub fn witness(&mut self, name: &str, arg: &str) -> Option<&[WitnessCrate]> { - let result = self.evaluate(name, arg); - if result.passed { - Some(&result.witness) - } else { - None + match entry.runnable.spawn(&full_args) { + Ok(output) => { + if !output.stderr.is_empty() { + tracing::debug!( + predicate = name, + stderr = %String::from_utf8_lossy(&output.stderr), + "custom predicate stderr" + ); + } + let passed = output.status.success(); + let witness = if passed { + parse_witness_stdout(name, &output.stdout) + } else { + Vec::new() + }; + CustomPredicateResult { passed, witness } } - } - - fn run(&self, name: &str, arg: &str) -> CustomPredicateResult { - let Some(entry) = self.entries.get(name) else { - tracing::warn!(predicate = name, "custom predicate not found in registry"); - return CustomPredicateResult { + Err(e) => { + tracing::warn!( + predicate = name, + error = %e, + "failed to spawn custom predicate" + ); + CustomPredicateResult { passed: false, witness: Vec::new(), - }; - }; - - let mut full_args: Vec<&str> = entry.args.iter().map(|s| s.as_str()).collect(); - if !arg.is_empty() { - full_args.push(arg); - } - - tracing::debug!( - predicate = name, - args = ?full_args, - "spawning custom predicate" - ); - - match entry.runnable.spawn(&full_args) { - Ok(output) => { - if !output.stderr.is_empty() { - tracing::debug!( - predicate = name, - stderr = %String::from_utf8_lossy(&output.stderr), - "custom predicate stderr" - ); - } - let passed = output.status.success(); - let witness = if passed { - parse_witness_stdout(name, &output.stdout) - } else { - Vec::new() - }; - CustomPredicateResult { passed, witness } - } - Err(e) => { - tracing::warn!( - predicate = name, - error = %e, - "failed to spawn custom predicate" - ); - CustomPredicateResult { - passed: false, - witness: Vec::new(), - } } } } @@ -912,32 +943,6 @@ fn parse_witness_stdout(predicate_name: &str, stdout: &[u8]) -> Vec bool { - match pred { - Predicate::Custom { name, arg } => evaluator.evaluate(name, arg).passed, - _ => pred.evaluate(ctx), - } -} - -/// Evaluate a PredicateSet with custom predicate support (AND semantics). -pub fn evaluate_predicate_set( - set: &PredicateSet, - ctx: &PredicateContext, - evaluator: &mut CustomPredicateEvaluator, -) -> bool { - set.predicates - .iter() - .all(|p| evaluate_predicate(p, ctx, evaluator)) -} - #[cfg(test)] mod tests { use super::*; @@ -1032,24 +1037,24 @@ mod tests { #[test] fn evaluate_crate_and_wildcard() { let w = ws(&[("serde", "1.0.0")]); - assert!(parse("crate(serde)").unwrap().evaluate(&ctx(&w))); - assert!(!parse("crate(tokio)").unwrap().evaluate(&ctx(&w))); - assert!(parse("crate(*)").unwrap().evaluate(&ctx(&[]))); + assert!(parse("crate(serde)").unwrap().evaluate(&mut ctx(&w))); + assert!(!parse("crate(tokio)").unwrap().evaluate(&mut ctx(&w))); + assert!(parse("crate(*)").unwrap().evaluate(&mut ctx(&[]))); } #[test] fn evaluate_combinators() { let w = ws(&[("serde", "1.0.0")]); - assert!(parse("not(crate(tokio))").unwrap().evaluate(&ctx(&w))); + assert!(parse("not(crate(tokio))").unwrap().evaluate(&mut ctx(&w))); assert!( parse("any(crate(tokio), crate(serde))") .unwrap() - .evaluate(&ctx(&w)) + .evaluate(&mut ctx(&w)) ); assert!( !parse("all(crate(serde), crate(tokio))") .unwrap() - .evaluate(&ctx(&w)) + .evaluate(&mut ctx(&w)) ); } @@ -1070,8 +1075,8 @@ mod tests { ] { let p = parse(input).unwrap(); assert_eq!( - p.evaluate(&ctx(&w)), - p.witness(&ctx(&w)).is_some(), + p.evaluate(&mut ctx(&w)), + p.witness(&mut ctx(&w)).is_some(), "evaluate/witness disagree: {input}" ); } @@ -1080,7 +1085,7 @@ mod tests { #[test] fn path_exists_empty_is_false() { // `path_exists()` must not resolve to a `$PATH` dir via `dir.join("")`. - assert!(!Predicate::PathExists(String::new()).evaluate(&ctx(&[]))); + assert!(!Predicate::PathExists(String::new()).evaluate(&mut ctx(&[]))); } // --- witness: the source="crate" fetch set --- @@ -1092,7 +1097,7 @@ mod tests { let w = ws(&[("c1", "1.0.0"), ("c2", "1.0.0")]); // env unset -> all(...) is a dead branch -> only c1 let names: Vec<_> = p - .witness(&ctx(&w)) + .witness(&mut ctx(&w)) .unwrap() .into_iter() .map(|(n, _)| n) @@ -1109,7 +1114,7 @@ mod tests { let w = ws(&[("c1", "1.0.0"), ("c2", "1.0.0")]); // PATH is set -> not(env(PATH)) false -> all dead -> only c1 let names: Vec<_> = p - .witness(&ctx(&w)) + .witness(&mut ctx(&w)) .unwrap() .into_iter() .map(|(n, _)| n) @@ -1124,7 +1129,7 @@ mod tests { let p = parse("any(crate(c1), any(env(PATH), crate(c2)))").unwrap(); let w = ws(&[("c1", "1.0.0"), ("c2", "1.0.0")]); let mut names: Vec<_> = p - .witness(&ctx(&w)) + .witness(&mut ctx(&w)) .unwrap() .into_iter() .map(|(n, _)| n) @@ -1136,7 +1141,7 @@ mod tests { #[test] fn witness_false_gate_is_none() { let p = parse("crate(absent)").unwrap(); - assert!(p.witness(&ctx(&[])).is_none()); + assert!(p.witness(&mut ctx(&[])).is_none()); } #[test] @@ -1144,7 +1149,7 @@ mod tests { let plugin = PredicateSet::from_crates("serde").unwrap(); let group = PredicateSet::from_crates("serde, tokio").unwrap(); let w = ws(&[("serde", "1.0.0"), ("tokio", "1.0.0")]); - let result = union_matched_crates(&[&plugin, &group], &ctx(&w)); + let result = union_matched_crates(&[&plugin, &group], &mut ctx(&w)); let mut names: Vec<_> = result.into_iter().map(|(n, _)| n).collect(); names.sort(); assert_eq!(names, vec!["serde", "tokio"]); @@ -1258,23 +1263,29 @@ mod tests { #[test] fn parse_custom_predicate_expression() { - let p = parse("battery-pack(cli>=0.3)").unwrap(); + let p = parse("battery_pack(cli>=0.3)").unwrap(); assert_eq!( p, Predicate::Custom { - name: "battery-pack".into(), + name: "battery_pack".into(), arg: "cli>=0.3".into() } ); } + #[test] + fn parse_custom_predicate_rejects_hyphens() { + assert!(parse("battery-pack(cli>=0.3)").is_err()); + assert!(parse("my-pred()").is_err()); + } + #[test] fn parse_custom_predicate_empty_arg() { - let p = parse("my-pred()").unwrap(); + let p = parse("my_pred()").unwrap(); assert_eq!( p, Predicate::Custom { - name: "my-pred".into(), + name: "my_pred".into(), arg: "".into() } ); @@ -1295,11 +1306,11 @@ mod tests { #[test] fn display_roundtrip_custom() { let p = Predicate::Custom { - name: "battery-pack".into(), + name: "battery_pack".into(), arg: "cli>=0.3".into(), }; let displayed = p.to_string(); - assert_eq!(displayed, "battery-pack(cli>=0.3)"); + assert_eq!(displayed, "battery_pack(cli>=0.3)"); let reparsed = parse(&displayed).unwrap(); assert_eq!(p, reparsed); } @@ -1360,7 +1371,7 @@ mod tests { // --- Custom predicate evaluation tests --- - fn evaluator_with(entries: Vec<(&str, &str)>) -> CustomPredicateEvaluator { + fn ctx_with_custom(entries: Vec<(&str, &str)>) -> PredicateContext<'static> { let mut map = std::collections::HashMap::new(); for (name, path) in entries { map.insert( @@ -1371,35 +1382,68 @@ mod tests { }, ); } - CustomPredicateEvaluator::new(map) + PredicateContext::with_custom_predicates(&[], map) + } + + fn ctx_with_script_entry( + name: &str, + script_content: &str, + ) -> (PredicateContext<'static>, tempfile::NamedTempFile) { + use std::io::Write; + let script = tempfile::Builder::new().suffix(".sh").tempfile().unwrap(); + writeln!(script.as_file(), "#!/bin/sh\n{script_content}").unwrap(); + let mut entries = std::collections::HashMap::new(); + entries.insert( + name.to_string(), + ResolvedPredicateEntry { + runnable: symposium_install::Runnable::Script(script.path().to_path_buf()), + args: vec![], + }, + ); + ( + PredicateContext::with_custom_predicates(&[], entries), + script, + ) } #[test] fn evaluate_custom_predicate_pass() { - let mut eval = evaluator_with(vec![("foo", "/bin/true")]); - let result = eval.evaluate("foo", "x"); - assert!(result.passed); + let mut ctx = ctx_with_custom(vec![("foo", "/bin/true")]); + let pred = Predicate::Custom { + name: "foo".into(), + arg: "x".into(), + }; + assert!(pred.evaluate(&mut ctx)); } #[test] fn evaluate_custom_predicate_fail() { - let mut eval = evaluator_with(vec![("foo", "/bin/false")]); - let result = eval.evaluate("foo", "x"); - assert!(!result.passed); + let mut ctx = ctx_with_custom(vec![("foo", "/bin/false")]); + let pred = Predicate::Custom { + name: "foo".into(), + arg: "x".into(), + }; + assert!(!pred.evaluate(&mut ctx)); } #[test] fn evaluate_custom_predicate_missing_from_registry() { - let mut eval = CustomPredicateEvaluator::empty(); - let result = eval.evaluate("nonexistent", "x"); - assert!(!result.passed); + let mut ctx = PredicateContext::new(&[]); + let pred = Predicate::Custom { + name: "nonexistent".into(), + arg: "x".into(), + }; + assert!(!pred.evaluate(&mut ctx)); } #[test] fn evaluate_custom_predicate_spawn_failure() { - let mut eval = evaluator_with(vec![("foo", "/nonexistent/binary/zzz")]); - let result = eval.evaluate("foo", "x"); - assert!(!result.passed); + let mut ctx = ctx_with_custom(vec![("foo", "/nonexistent/binary/zzz")]); + let pred = Predicate::Custom { + name: "foo".into(), + arg: "x".into(), + }; + assert!(!pred.evaluate(&mut ctx)); } #[test] @@ -1424,11 +1468,15 @@ mod tests { args: vec![], }, ); - let mut eval = CustomPredicateEvaluator::new(entries); + let mut ctx = PredicateContext::with_custom_predicates(&[], entries); + let pred = Predicate::Custom { + name: "counter".into(), + arg: "a".into(), + }; // Evaluate twice with same (name, arg) - assert!(eval.evaluate("counter", "a").passed); - assert!(eval.evaluate("counter", "a").passed); + assert!(pred.evaluate(&mut ctx)); + assert!(pred.evaluate(&mut ctx)); // Script should have been called only once let content = std::fs::read_to_string(&counter_path).unwrap(); @@ -1457,10 +1505,13 @@ mod tests { args: vec!["--static".into(), "arg".into()], }, ); - let mut eval = CustomPredicateEvaluator::new(entries); + let mut ctx = PredicateContext::with_custom_predicates(&[], entries); + let pred = Predicate::Custom { + name: "checker".into(), + arg: "dynamic-arg".into(), + }; - let result = eval.evaluate("checker", "dynamic-arg"); - assert!(result.passed); + assert!(pred.evaluate(&mut ctx)); let content = std::fs::read_to_string(&output_path).unwrap(); assert_eq!(content.trim(), "--static arg dynamic-arg"); @@ -1468,72 +1519,78 @@ mod tests { // --- Witness tests --- - fn make_script_evaluator( - name: &str, - script_content: &str, - ) -> (CustomPredicateEvaluator, tempfile::NamedTempFile) { - use std::io::Write; - let script = tempfile::Builder::new().suffix(".sh").tempfile().unwrap(); - writeln!(script.as_file(), "#!/bin/sh\n{script_content}").unwrap(); - let mut entries = std::collections::HashMap::new(); - entries.insert( - name.to_string(), - ResolvedPredicateEntry { - runnable: symposium_install::Runnable::Script(script.path().to_path_buf()), - args: vec![], - }, - ); - (CustomPredicateEvaluator::new(entries), script) - } - #[test] fn witness_custom_with_selected_crates() { let json = r#"{"selectedCrates":[{"crate":"cli-battery-pack","version":"0.3.1"}]}"#; - let (mut eval, _script) = make_script_evaluator("bp", &format!("printf '{json}'")); - let witness = eval.witness("bp", "cli").unwrap(); + let (mut ctx, _script) = ctx_with_script_entry("bp", &format!("printf '{json}'")); + let pred = Predicate::Custom { + name: "bp".into(), + arg: "cli".into(), + }; + let witness = pred.witness(&mut ctx).unwrap(); assert_eq!(witness.len(), 1); - assert_eq!(witness[0].name, "cli-battery-pack"); - assert_eq!(witness[0].version, semver::Version::parse("0.3.1").unwrap()); + assert_eq!(witness[0].0, "cli-battery-pack"); + assert_eq!(witness[0].1, semver::Version::parse("0.3.1").unwrap()); } #[test] fn witness_custom_empty_stdout() { - let mut eval = evaluator_with(vec![("foo", "/bin/true")]); - let witness = eval.witness("foo", "x").unwrap(); + let mut ctx = ctx_with_custom(vec![("foo", "/bin/true")]); + let pred = Predicate::Custom { + name: "foo".into(), + arg: "x".into(), + }; + let witness = pred.witness(&mut ctx).unwrap(); assert!(witness.is_empty()); } #[test] fn witness_custom_exit_nonzero() { - let mut eval = evaluator_with(vec![("foo", "/bin/false")]); - let witness = eval.witness("foo", "x"); + let mut ctx = ctx_with_custom(vec![("foo", "/bin/false")]); + let pred = Predicate::Custom { + name: "foo".into(), + arg: "x".into(), + }; + let witness = pred.witness(&mut ctx); assert!(witness.is_none()); } #[test] fn witness_custom_invalid_json() { - let (mut eval, _script) = make_script_evaluator("bp", "printf 'not json at all'"); - let witness = eval.witness("bp", "x").unwrap(); + let (mut ctx, _script) = ctx_with_script_entry("bp", "printf 'not json at all'"); + let pred = Predicate::Custom { + name: "bp".into(), + arg: "x".into(), + }; + let witness = pred.witness(&mut ctx).unwrap(); assert!(witness.is_empty()); } #[test] fn witness_custom_invalid_version() { let json = r#"{"selectedCrates":[{"crate":"good","version":"1.0.0"},{"crate":"bad","version":"not-semver"}]}"#; - let (mut eval, _script) = make_script_evaluator("bp", &format!("printf '{json}'")); - let witness = eval.witness("bp", "x").unwrap(); + let (mut ctx, _script) = ctx_with_script_entry("bp", &format!("printf '{json}'")); + let pred = Predicate::Custom { + name: "bp".into(), + arg: "x".into(), + }; + let witness = pred.witness(&mut ctx).unwrap(); assert_eq!(witness.len(), 1); - assert_eq!(witness[0].name, "good"); + assert_eq!(witness[0].0, "good"); } #[test] fn witness_custom_multiple_crates() { let json = r#"{"selectedCrates":[{"crate":"a","version":"1.0.0"},{"crate":"b","version":"2.0.0"},{"crate":"c","version":"3.0.0"}]}"#; - let (mut eval, _script) = make_script_evaluator("bp", &format!("printf '{json}'")); - let witness = eval.witness("bp", "x").unwrap(); + let (mut ctx, _script) = ctx_with_script_entry("bp", &format!("printf '{json}'")); + let pred = Predicate::Custom { + name: "bp".into(), + arg: "x".into(), + }; + let witness = pred.witness(&mut ctx).unwrap(); assert_eq!(witness.len(), 3); - assert_eq!(witness[0].name, "a"); - assert_eq!(witness[1].name, "b"); - assert_eq!(witness[2].name, "c"); + assert_eq!(witness[0].0, "a"); + assert_eq!(witness[1].0, "b"); + assert_eq!(witness[2].0, "c"); } } diff --git a/src/skills.rs b/src/skills.rs index bd1b5349..b0ccbd94 100644 --- a/src/skills.rs +++ b/src/skills.rs @@ -150,11 +150,12 @@ pub async fn skills_applicable_to( sym: &Symposium, registry: &PluginRegistry, workspace_crates: &[crate::crate_sources::WorkspaceCrate], + custom_predicate_entries: std::collections::HashMap, ) -> Vec { let mut results = Vec::new(); let for_crates = crate::crate_sources::crate_pairs(workspace_crates); - let ctx = PredicateContext::new(&for_crates); + let mut ctx = PredicateContext::with_custom_predicates(&for_crates, custom_predicate_entries); // Skills from plugin manifests. We iterate these separately // because we lazily load skill groups, so there @@ -163,7 +164,7 @@ pub async fn skills_applicable_to( let plugin = &parsed.plugin; // Plugin-level predicates gate everything below. Evaluated before // group fetching to avoid wasted work. - if !plugin.applies(&ctx) { + if !plugin.predicates.evaluate(&mut ctx) { tracing::debug!( report = %crate::report::ReportEvent::PluginConsidered { plugin: plugin.name.clone(), @@ -183,9 +184,10 @@ pub async fn skills_applicable_to( ); for group in &plugin.skills { - let skills = load_skills_for_group(sym, parsed, group, workspace_crates, &ctx).await; + let skills = + load_skills_for_group(sym, parsed, group, workspace_crates, &mut ctx).await; for (skill, origin) in skills { - collect_skill_applicable_to(skill, origin, &plugin.name, &ctx, &mut results); + collect_skill_applicable_to(skill, origin, &plugin.name, &mut ctx, &mut results); } } } @@ -206,84 +208,7 @@ pub async fn skills_applicable_to( entry.skill.clone(), entry.origin.clone(), "(standalone skills)", - &ctx, - &mut results, - ); - } - - results -} - -/// Like [`skills_applicable_to`], but with a custom predicate evaluator. -pub async fn skills_applicable_to_with_evaluator( - sym: &Symposium, - registry: &PluginRegistry, - workspace_crates: &[crate::crate_sources::WorkspaceCrate], - evaluator: &mut predicate::CustomPredicateEvaluator, -) -> Vec { - let mut results = Vec::new(); - - let for_crates = crate::crate_sources::crate_pairs(workspace_crates); - let ctx = PredicateContext::new(&for_crates); - - // Skills from plugin manifests. We iterate these separately - // because we lazily load skill groups, so there - // is extra logic. - for parsed in ®istry.plugins { - let plugin = &parsed.plugin; - // Plugin-level predicates gate everything below. Evaluated before - // group fetching to avoid wasted work. - if !predicate::evaluate_predicate_set(&plugin.predicates, &ctx, evaluator) { - tracing::debug!( - report = %crate::report::ReportEvent::PluginConsidered { - plugin: plugin.name.clone(), - matched: false, - reason: Some("plugin-level predicates not satisfied".into()), - }, - ); - continue; - } - - tracing::debug!( - report = %crate::report::ReportEvent::PluginConsidered { - plugin: plugin.name.clone(), - matched: true, - reason: None, - }, - ); - - for group in &plugin.skills { - let skills = load_skills_for_group(sym, parsed, group, workspace_crates, &ctx).await; - for (skill, origin) in skills { - collect_skill_applicable_to_with_evaluator( - skill, - origin, - &plugin.name, - &ctx, - evaluator, - &mut results, - ); - } - } - } - - // Standalone skills already carry their own `SkillOrigin`. - if !registry.standalone_skills.is_empty() { - tracing::debug!( - report = %crate::report::ReportEvent::PluginConsidered { - plugin: "(standalone skills)".into(), - matched: true, - reason: None, - }, - ); - } - for entry in ®istry.standalone_skills { - collect_skill_applicable_to_with_evaluator( - entry.skill.clone(), - entry.origin.clone(), - "(standalone skills)", - &ctx, - evaluator, + &mut ctx, &mut results, ); } @@ -310,7 +235,7 @@ async fn load_skills_for_group( parsed: &ParsedPlugin, group: &SkillGroup, workspace_crates: &[crate::crate_sources::WorkspaceCrate], - ctx: &PredicateContext<'_>, + ctx: &mut PredicateContext<'_>, ) -> Vec<(Skill, SkillOrigin)> { let plugin = &parsed.plugin; let plugin_path = parsed.path.as_path(); @@ -382,7 +307,7 @@ async fn load_crate_skills( plugin: &crate::plugins::Plugin, group: &SkillGroup, workspace_crates: &[crate::crate_sources::WorkspaceCrate], - ctx: &PredicateContext<'_>, + ctx: &mut PredicateContext<'_>, ) -> Vec<(Skill, SkillOrigin)> { let matched = predicate::union_matched_crates(&[&plugin.predicates, &group.predicates], ctx); let mut skills = Vec::new(); @@ -853,7 +778,7 @@ fn collect_skill_applicable_to( skill: Skill, origin: SkillOrigin, plugin_name: &str, - ctx: &PredicateContext, + ctx: &mut PredicateContext, results: &mut Vec, ) { if !skill.predicates.evaluate(ctx) { @@ -879,39 +804,6 @@ fn collect_skill_applicable_to( results.push(SkillWithGroupContext { skill, origin }); } -/// Like [`collect_skill_applicable_to`] but uses the custom predicate evaluator -/// for skill-level predicate evaluation. -fn collect_skill_applicable_to_with_evaluator( - skill: Skill, - origin: SkillOrigin, - plugin_name: &str, - ctx: &PredicateContext, - evaluator: &mut predicate::CustomPredicateEvaluator, - results: &mut Vec, -) { - if !predicate::evaluate_predicate_set(&skill.predicates, ctx, evaluator) { - tracing::debug!( - report = %crate::report::ReportEvent::SkillConsidered { - skill: skill.name().to_string(), - plugin: plugin_name.to_string(), - matched: false, - reason: Some("skill-level predicates not satisfied".into()), - }, - ); - return; - } - - tracing::debug!( - report = %crate::report::ReportEvent::SkillConsidered { - skill: skill.name().to_string(), - plugin: plugin_name.to_string(), - matched: true, - reason: None, - }, - ); - results.push(SkillWithGroupContext { skill, origin }); -} - /// Raw frontmatter fields extracted from a SKILL.md file. /// `crates` is comma-separated on a single line. #[derive(Debug)] @@ -1428,7 +1320,13 @@ mod tests { version: semver::Version::new(1, 0, 0), path: None, }]; - let skills = skills_applicable_to(&sym, ®istry, &workspace_crates).await; + let skills = skills_applicable_to( + &sym, + ®istry, + &workspace_crates, + std::collections::HashMap::new(), + ) + .await; assert!( skills.is_empty(), @@ -1477,7 +1375,13 @@ mod tests { version: semver::Version::new(1, 0, 0), path: None, }]; - let skills = skills_applicable_to(&sym, ®istry, &workspace_crates).await; + let skills = skills_applicable_to( + &sym, + ®istry, + &workspace_crates, + std::collections::HashMap::new(), + ) + .await; assert!( skills.is_empty(), @@ -1543,7 +1447,13 @@ mod tests { version: semver::Version::new(1, 0, 0), path: None, }]; - let skills = skills_applicable_to(&sym, ®istry, &workspace_crates).await; + let skills = skills_applicable_to( + &sym, + ®istry, + &workspace_crates, + std::collections::HashMap::new(), + ) + .await; assert_eq!( skills.len(), @@ -1615,7 +1525,13 @@ mod tests { version: semver::Version::new(1, 0, 0), path: None, }]; - let skills = skills_applicable_to(&sym, ®istry, &workspace).await; + let skills = skills_applicable_to( + &sym, + ®istry, + &workspace, + std::collections::HashMap::new(), + ) + .await; assert!( skills.is_empty(), "plugin predicate=false should filter out skills" @@ -1688,7 +1604,13 @@ mod tests { version: semver::Version::new(1, 0, 0), path: None, }]; - let skills = skills_applicable_to(&sym, ®istry, &workspace).await; + let skills = skills_applicable_to( + &sym, + ®istry, + &workspace, + std::collections::HashMap::new(), + ) + .await; assert_eq!(skills.len(), 1); } @@ -1814,7 +1736,13 @@ mod tests { version: semver::Version::new(1, 0, 0), path: None, }]; - let results = skills_applicable_to(&sym, ®istry, &workspace).await; + let results = skills_applicable_to( + &sym, + ®istry, + &workspace, + std::collections::HashMap::new(), + ) + .await; assert_eq!(results.len(), 1); assert_eq!(results[0].skill.name(), "standalone-serde"); assert!(results[0].skill.predicates.references_crate("serde")); @@ -1969,7 +1897,9 @@ mod tests { /// Each level's `crates` lowers to one predicate set; the skill applies when /// every level's set holds (AND across levels). fn applies(levels: &[&str], ws: &[(String, semver::Version)]) -> bool { - levels.iter().all(|spec| pred_set(spec).evaluate(&ctx(ws))) + levels + .iter() + .all(|spec| pred_set(spec).evaluate(&mut ctx(ws))) } #[test] diff --git a/src/subcommand_dispatch.rs b/src/subcommand_dispatch.rs index 730d4450..1a25b4ff 100644 --- a/src/subcommand_dispatch.rs +++ b/src/subcommand_dispatch.rs @@ -19,27 +19,25 @@ use semver::Version; use symposium_install::Runnable; use tokio::process::Command; -/// Iterate every plugin subcommand whose plugin-level and subcommand-level crate predicate +/// Collect every plugin subcommand whose plugin-level and subcommand-level predicates /// apply to `deps`. Shared between dispatch (name lookup) and help rendering (audience grouping). -pub fn applicable_subcommands<'a, 'd>( +pub fn applicable_subcommands<'a>( registry: &'a PluginRegistry, - deps: &'d [(String, Version)], -) -> impl Iterator + 'd -where - 'a: 'd, -{ - let ctx = crate::predicate::PredicateContext::new(deps); - registry - .plugins - .iter() - .filter(move |ParsedPlugin { plugin, .. }| plugin.applies(&ctx)) - .flat_map(move |ParsedPlugin { plugin, .. }| { - plugin - .subcommands - .iter() - .filter(move |(_, sub)| sub.predicates.evaluate(&ctx)) - .map(move |(name, subcommand)| (plugin, name.as_str(), subcommand)) - }) + deps: &[(String, Version)], +) -> Vec<(&'a Plugin, &'a str, &'a Subcommand)> { + let mut ctx = crate::predicate::PredicateContext::new(deps); + let mut results = Vec::new(); + for ParsedPlugin { plugin, .. } in ®istry.plugins { + if !plugin.applies(&mut ctx) { + continue; + } + for (name, subcommand) in &plugin.subcommands { + if subcommand.predicates.evaluate(&mut ctx) { + results.push((plugin, name.as_str(), subcommand)); + } + } + } + results } /// Look up a subcommand by name across all plugins, filtered by workspace crates at a plugin @@ -55,10 +53,11 @@ pub fn find_subcommand<'a>( ) -> Result> { let deps = crate::crate_sources::crate_pairs(workspace); - let matches = applicable_subcommands(registry, &deps) + let matches: Vec<_> = applicable_subcommands(registry, &deps) + .into_iter() .filter(|(_, n, _)| *n == name) .map(|(plugin, _, subcmd)| (plugin, subcmd)) - .collect::>(); + .collect(); match matches.as_slice() { [] => Ok(None), diff --git a/src/sync.rs b/src/sync.rs index 1ef109af..7994f44d 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -164,18 +164,13 @@ fn propagate_user_skill(source_dir: &Path, dest_dir: &Path, project_root: &Path) Ok(true) } -/// Resolve custom predicate installations from the registry into a -/// `CustomPredicateEvaluator`. Acquisition failures are logged as warnings -/// and the corresponding predicate is marked non-functional (evaluates false). -async fn resolve_custom_predicates( +/// Resolve custom predicate installations from the registry into entries +/// suitable for [`PredicateContext::with_custom_predicates`]. +async fn resolve_custom_predicate_entries( sym: &Symposium, registry: &plugins::PluginRegistry, -) -> crate::predicate::CustomPredicateEvaluator { - use crate::predicate::{CustomPredicateEvaluator, ResolvedPredicateEntry}; - - if registry.custom_predicates.is_empty() { - return CustomPredicateEvaluator::empty(); - } +) -> std::collections::HashMap { + use crate::predicate::ResolvedPredicateEntry; let mut entries = std::collections::HashMap::new(); @@ -225,7 +220,7 @@ async fn resolve_custom_predicates( ); } - CustomPredicateEvaluator::new(entries) + entries } /// Run the full sync: discover applicable skills, install into agent dirs, @@ -253,13 +248,11 @@ pub async fn sync(sym: &Symposium, cwd: &Path) -> Result<()> { }, ); - // Resolve custom predicate installations and build the evaluator. - let mut evaluator = resolve_custom_predicates(sym, ®istry).await; + // Resolve custom predicate installations. + let custom_entries = resolve_custom_predicate_entries(sym, ®istry).await; // Find all applicable skills - let applicable = - skills::skills_applicable_to_with_evaluator(sym, ®istry, &workspace, &mut evaluator) - .await; + let applicable = skills::skills_applicable_to(sym, ®istry, &workspace, custom_entries).await; // Dedup by `(skill_name, SkillOrigin)`: two `Crate` origins with the // same (name, version) collapse (the skills are the same logical bytes @@ -282,13 +275,13 @@ pub async fn sync(sym: &Symposium, cwd: &Path) -> Result<()> { // Collect MCP servers from applicable plugins, filtered by workspace deps let semver_pairs = crate::crate_sources::crate_pairs(&workspace); - let ctx = crate::predicate::PredicateContext::new(&semver_pairs); - let mcp_servers: Vec = registry - .plugins - .iter() - .filter(|p| p.plugin.applies(&ctx)) - .flat_map(|p| p.plugin.applicable_mcp_servers(&ctx)) - .collect(); + let mut ctx = crate::predicate::PredicateContext::new(&semver_pairs); + let mut mcp_servers: Vec = Vec::new(); + for p in ®istry.plugins { + if p.plugin.applies(&mut ctx) { + mcp_servers.extend(p.plugin.applicable_mcp_servers(&mut ctx)); + } + } let server_names: Vec<&str> = mcp_servers .iter() From f8d45eec8eb8f953e7f35334c3c481a0356128e5 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Jun 2026 15:29:52 +0000 Subject: [PATCH 03/13] Consolidate predicate name validation and fix CrateList comma parsing - Extract `validate_custom_predicate_name` as a single public helper in predicate.rs, enforcing `[a-zA-Z][a-zA-Z0-9_]*` + no builtin collision. Used by both the expression parser and plugin manifest validation. - Fix CrateList::parse to use split_top_level (paren-aware comma split) so custom predicates like `bp(a, b)` in SKILL.md frontmatter are not incorrectly split on the inner comma. - Add collision tests for build_custom_predicate_registry (two-way and three-way). Co-authored-by: Claude --- src/plugins.rs | 101 ++++++++++++++++++++++++++++++++++++++--------- src/predicate.rs | 67 +++++++++++++++++++++++++------ 2 files changed, 139 insertions(+), 29 deletions(-) diff --git a/src/plugins.rs b/src/plugins.rs index acf17d9d..18379e99 100644 --- a/src/plugins.rs +++ b/src/plugins.rs @@ -1654,30 +1654,13 @@ fn validate_subcommand( }) } -fn validate_custom_predicate_name(name: &str) -> Result<()> { - if name.is_empty() { - bail!("predicate name is empty"); - } - let first = name.as_bytes()[0]; - if !first.is_ascii_alphabetic() { - bail!("predicate `{name}` must start with a letter"); - } - if !name.bytes().all(|c| c.is_ascii_alphanumeric() || c == b'_') { - bail!("predicate `{name}` has invalid characters (allow ASCII alphanumeric and `_`)"); - } - if crate::predicate::BUILTIN_PREDICATE_NAMES.contains(&name) { - bail!("predicate `{name}` collides with a builtin predicate name"); - } - Ok(()) -} - /// Validate a `[[predicate]]` entry, promoting inline `command` if needed. fn validate_custom_predicate( raw: RawCustomPredicate, installations: &mut Vec, names: &mut std::collections::BTreeSet, ) -> Result { - validate_custom_predicate_name(&raw.name)?; + crate::predicate::validate_custom_predicate_name(&raw.name)?; let command = resolve_or_promote( raw.command, @@ -3915,4 +3898,86 @@ mod tests { let sub = &plugin.subcommands["foo"]; assert!(sub.predicates.references_crate("serde")); } + + // --- custom predicate collision tests --- + + fn make_plugin_with_predicate(plugin_name: &str, predicate_name: &str) -> ParsedPlugin { + ParsedPlugin { + path: std::path::PathBuf::from(format!("{plugin_name}.toml")), + plugin: Plugin { + name: plugin_name.to_string(), + predicates: pred_set("*"), + installations: vec![Installation { + name: "checker".to_string(), + source: None, + executable: Some("/bin/true".to_string()), + script: None, + args: vec![], + requirements: vec![], + install_commands: vec![], + }], + hooks: vec![], + skills: vec![], + mcp_servers: vec![], + subcommands: BTreeMap::new(), + custom_predicates: vec![CustomPredicate { + name: predicate_name.to_string(), + command: "checker".to_string(), + args: vec![], + }], + }, + source_name: "test".into(), + source_dir: std::path::PathBuf::from("/test"), + } + } + + #[test] + fn custom_predicate_registry_no_collision() { + let plugins = vec![ + make_plugin_with_predicate("alpha", "foo"), + make_plugin_with_predicate("beta", "bar"), + ]; + let mut warnings = vec![]; + let registry = build_custom_predicate_registry(&plugins, &mut warnings); + assert!(warnings.is_empty()); + assert_eq!(registry.len(), 2); + assert!(registry.contains_key("foo")); + assert!(registry.contains_key("bar")); + } + + #[test] + fn custom_predicate_registry_two_way_collision() { + let plugins = vec![ + make_plugin_with_predicate("alpha", "shared"), + make_plugin_with_predicate("beta", "shared"), + ]; + let mut warnings = vec![]; + let registry = build_custom_predicate_registry(&plugins, &mut warnings); + assert_eq!(warnings.len(), 1); + assert!(warnings[0].message.contains("shared")); + assert!(warnings[0].message.contains("alpha")); + assert!(warnings[0].message.contains("beta")); + assert!( + !registry.contains_key("shared"), + "collided predicate must be removed" + ); + } + + #[test] + fn custom_predicate_registry_three_way_collision() { + let plugins = vec![ + make_plugin_with_predicate("alpha", "shared"), + make_plugin_with_predicate("beta", "shared"), + make_plugin_with_predicate("gamma", "shared"), + ]; + let mut warnings = vec![]; + let registry = build_custom_predicate_registry(&plugins, &mut warnings); + // Warning is emitted only on the second occurrence (alpha vs beta); + // the third (gamma) sees the name in the collision set and skips. + assert_eq!(warnings.len(), 1); + assert!( + !registry.contains_key("shared"), + "collided predicate must be removed" + ); + } } diff --git a/src/predicate.rs b/src/predicate.rs index 6359000d..5d343f5d 100644 --- a/src/predicate.rs +++ b/src/predicate.rs @@ -400,10 +400,13 @@ pub struct CrateList(pub Vec); impl CrateList { /// Parse comma-separated crate atoms (`serde, tokio>=1.0, *`). + /// + /// Commas inside balanced parentheses are preserved so that custom + /// predicates like `battery_pack(a, b)` are not split incorrectly. pub fn parse(input: &str) -> Result { - let atoms = input - .split(',') - .map(str::trim) + let atoms = split_top_level(input) + .iter() + .map(|s| s.trim()) .filter(|s| !s.is_empty()) .map(|s| { parse_crate_atom(s) @@ -449,17 +452,28 @@ impl<'de> serde::Deserialize<'de> for CrateList { // --- function-call predicate parsing --- -/// Validate that a custom predicate name uses only `[a-zA-Z][a-zA-Z0-9_]*`. -/// Hyphens are intentionally rejected to keep the namespace aligned with -/// `[[predicate]]` definition validation. -fn validate_custom_predicate_name_at_parse(name: &str) -> Result<()> { +/// Validate that `name` is a legal custom predicate identifier: +/// `[a-zA-Z][a-zA-Z0-9_]*`, must not collide with a builtin name. +/// +/// Shared by both the expression parser (encountering an unknown function +/// name) and the `[[predicate]]` definition validator in `plugins.rs`. +pub fn validate_custom_predicate_name(name: &str) -> Result<()> { + if name.is_empty() { + bail!("predicate name is empty"); + } + if !name.as_bytes()[0].is_ascii_alphabetic() { + bail!("predicate `{name}` must start with a letter"); + } if let Some(pos) = name.find(|c: char| !c.is_ascii_alphanumeric() && c != '_') { bail!( - "custom predicate name `{name}` contains invalid character '{}' at position {pos} \ + "predicate `{name}` contains invalid character '{}' at position {pos} \ (only ASCII alphanumeric and `_` allowed)", name.as_bytes()[pos] as char, ); } + if BUILTIN_PREDICATE_NAMES.contains(&name) { + bail!("predicate `{name}` collides with a builtin predicate name"); + } Ok(()) } @@ -498,7 +512,7 @@ fn parse(input: &str) -> Result { Ok(Predicate::All(preds)) } other => { - validate_custom_predicate_name_at_parse(other)?; + validate_custom_predicate_name(other)?; Ok(Predicate::Custom { name: other.to_string(), arg: arg.to_string(), @@ -635,7 +649,7 @@ impl<'a> AtomParser<'a> { parse_crate_atom(&arg) .with_context(|| format!("inside `crate(...)`: failed to parse `{arg}`")) } else { - validate_custom_predicate_name_at_parse(name)?; + validate_custom_predicate_name(name)?; Ok(Predicate::Custom { name: name.to_string(), arg, @@ -994,6 +1008,28 @@ mod tests { Predicate::Crate("tokio".into(), None), ])) ); + // Commas inside balanced parens are preserved (custom predicates). + assert_eq!( + CrateList::parse("bp(cli, web)").unwrap().into_predicate(), + Some(Predicate::Custom { + name: "bp".into(), + arg: "cli, web".into(), + }) + ); + // Mixed: normal atom + custom predicate with inner comma + let mixed = CrateList::parse("serde, bp(a, b)") + .unwrap() + .into_predicate(); + assert_eq!( + mixed, + Some(Predicate::Any(vec![ + Predicate::Crate("serde".into(), None), + Predicate::Custom { + name: "bp".into(), + arg: "a, b".into(), + }, + ])) + ); } // --- function-call parsing --- @@ -1274,9 +1310,18 @@ mod tests { } #[test] - fn parse_custom_predicate_rejects_hyphens() { + fn parse_custom_predicate_rejects_invalid_names() { + // Hyphens not allowed assert!(parse("battery-pack(cli>=0.3)").is_err()); assert!(parse("my-pred()").is_err()); + // Must start with a letter + assert!(parse("0foo(x)").is_err()); + assert!(parse("_foo(x)").is_err()); + // Builtin names cannot be redefined (they're matched first anyway, + // but the validator rejects them if somehow reached) + assert!(validate_custom_predicate_name("crate").is_err()); + assert!(validate_custom_predicate_name("shell").is_err()); + assert!(validate_custom_predicate_name("not").is_err()); } #[test] From 03ffdc3d438b9a7431a2c249682bfcfca2cf7ba7 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Jun 2026 15:45:07 +0000 Subject: [PATCH 04/13] Reject function-call syntax in the `crates` field The `crates` field should only accept crate-name atoms (e.g. `serde`, `serde>=1.0`, `*`). Full predicate expressions including custom predicates belong in the `predicates` field. This matches the design decision from the Zulip discussion: the fields serve distinct roles and should not accept each other's syntax. - AtomParser now errors on `(` instead of parsing function calls - Remove dead `consume_balanced_parens` from AtomParser - Fix test fixture to use `predicates` instead of `crates` - Update CrateList tests to assert rejection Co-authored-by: Claude --- src/predicate.rs | 73 +++---------------- .../plugins/bp-plugin/SYMPOSIUM.toml | 2 +- 2 files changed, 12 insertions(+), 63 deletions(-) diff --git a/src/predicate.rs b/src/predicate.rs index 5d343f5d..d198dc6f 100644 --- a/src/predicate.rs +++ b/src/predicate.rs @@ -640,21 +640,15 @@ impl<'a> AtomParser<'a> { ); } - // Function-call syntax: name(arg) — either `crate(...)` or a custom predicate. + // Function-call syntax is NOT valid in crate-atom position. The + // `crates` field accepts only bare names + optional version constraints. + // Full predicate expressions (including custom predicates) belong in the + // `predicates` field. if self.pos < self.input.len() && self.input.as_bytes()[self.pos] == b'(' { - self.pos += 1; // consume '(' - let arg = self.consume_balanced_parens()?; - return if name == "crate" { - // `crate(serde)` or `crate(serde>=1.0)` — parse inner as a crate atom - parse_crate_atom(&arg) - .with_context(|| format!("inside `crate(...)`: failed to parse `{arg}`")) - } else { - validate_custom_predicate_name(name)?; - Ok(Predicate::Custom { - name: name.to_string(), - arg, - }) - }; + bail!( + "function-call syntax `{name}(...)` is not valid in the `crates` field; \ + use the `predicates` field instead" + ); } // Version constraint (starts with >=, <=, >, <, =, ^, ~). Bare `=` is @@ -688,32 +682,6 @@ impl<'a> AtomParser<'a> { Ok(Predicate::Crate(name.to_string(), version_req)) } - - /// Consume content between balanced parentheses. The opening `(` has - /// already been consumed. Returns the content up to the matching `)`. - fn consume_balanced_parens(&mut self) -> Result { - let start = self.pos; - let mut depth: u32 = 1; - while self.pos < self.input.len() { - match self.input.as_bytes()[self.pos] { - b'(' => depth += 1, - b')' => { - depth -= 1; - if depth == 0 { - let content = self.input[start..self.pos].to_string(); - self.pos += 1; // consume closing ')' - return Ok(content); - } - } - _ => {} - } - self.pos += 1; - } - bail!( - "unmatched `(` at position {} — no closing `)` found", - start.saturating_sub(1) - ); - } } // --- environment evaluation --- @@ -1008,28 +976,9 @@ mod tests { Predicate::Crate("tokio".into(), None), ])) ); - // Commas inside balanced parens are preserved (custom predicates). - assert_eq!( - CrateList::parse("bp(cli, web)").unwrap().into_predicate(), - Some(Predicate::Custom { - name: "bp".into(), - arg: "cli, web".into(), - }) - ); - // Mixed: normal atom + custom predicate with inner comma - let mixed = CrateList::parse("serde, bp(a, b)") - .unwrap() - .into_predicate(); - assert_eq!( - mixed, - Some(Predicate::Any(vec![ - Predicate::Crate("serde".into(), None), - Predicate::Custom { - name: "bp".into(), - arg: "a, b".into(), - }, - ])) - ); + // Function-call syntax is rejected in the `crates` field. + assert!(CrateList::parse("bp(cli, web)").is_err()); + assert!(CrateList::parse("serde, bp(a, b)").is_err()); } // --- function-call parsing --- diff --git a/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml b/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml index 9f2ffbf4..5b3b9063 100644 --- a/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml +++ b/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml @@ -1,5 +1,5 @@ name = "bp-plugin" -crates = ["battery_pack(cli)"] +predicates = ["battery_pack(cli)"] [[installations]] name = "bp-checker" From aee290175988122575807379c1cf871018217ff8 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Jun 2026 16:07:19 +0000 Subject: [PATCH 05/13] Add tests for field syntax boundary enforcement Assert that the `crates` field rejects all function-call forms (all(), crate(), not(), shell()) and that the `predicates` field rejects bare crate-atom syntax (serde, tokio>=1.0, *). Co-authored-by: Claude --- src/predicate.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/predicate.rs b/src/predicate.rs index d198dc6f..ca42ef02 100644 --- a/src/predicate.rs +++ b/src/predicate.rs @@ -979,10 +979,22 @@ mod tests { // Function-call syntax is rejected in the `crates` field. assert!(CrateList::parse("bp(cli, web)").is_err()); assert!(CrateList::parse("serde, bp(a, b)").is_err()); + assert!(CrateList::parse("all()").is_err()); + assert!(CrateList::parse("crate(serde)").is_err()); + assert!(CrateList::parse("not(serde)").is_err()); + assert!(CrateList::parse("shell(true)").is_err()); } // --- function-call parsing --- + #[test] + fn predicates_field_rejects_bare_names() { + // The `predicates` field requires function-call syntax. + assert!(parse("serde").is_err()); + assert!(parse("tokio>=1.0").is_err()); + assert!(parse("*").is_err()); + } + #[test] fn parse_function_calls() { assert_eq!( From d009da9fce7087993baaa12b22323890e359c3c3 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Jun 2026 16:27:56 +0000 Subject: [PATCH 06/13] Fix CI: replace /bin/true with script-based test helpers The custom predicate tests used Runnable::Exec with hardcoded /bin/true and /bin/false paths which may not exist on all CI environments. Replace with Runnable::Script using tempfile shell scripts that explicitly `exit 0` or `exit 1`. Co-authored-by: Claude --- src/predicate.rs | 38 +++++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/src/predicate.rs b/src/predicate.rs index ca42ef02..8cab97a4 100644 --- a/src/predicate.rs +++ b/src/predicate.rs @@ -1377,18 +1377,27 @@ mod tests { // --- Custom predicate evaluation tests --- - fn ctx_with_custom(entries: Vec<(&str, &str)>) -> PredicateContext<'static> { + /// Create a context with custom predicate entries using shell scripts. + /// Each entry is `(name, exit_code)` — the script does `exit `. + fn ctx_with_exit_codes( + entries: Vec<(&str, u8)>, + ) -> (PredicateContext<'static>, Vec) { + use std::io::Write; let mut map = std::collections::HashMap::new(); - for (name, path) in entries { + let mut scripts = Vec::new(); + for (name, code) in entries { + let script = tempfile::Builder::new().suffix(".sh").tempfile().unwrap(); + writeln!(script.as_file(), "#!/bin/sh\nexit {code}").unwrap(); map.insert( name.to_string(), ResolvedPredicateEntry { - runnable: symposium_install::Runnable::Exec(std::path::PathBuf::from(path)), + runnable: symposium_install::Runnable::Script(script.path().to_path_buf()), args: vec![], }, ); + scripts.push(script); } - PredicateContext::with_custom_predicates(&[], map) + (PredicateContext::with_custom_predicates(&[], map), scripts) } fn ctx_with_script_entry( @@ -1414,7 +1423,7 @@ mod tests { #[test] fn evaluate_custom_predicate_pass() { - let mut ctx = ctx_with_custom(vec![("foo", "/bin/true")]); + let (mut ctx, _scripts) = ctx_with_exit_codes(vec![("foo", 0)]); let pred = Predicate::Custom { name: "foo".into(), arg: "x".into(), @@ -1424,7 +1433,7 @@ mod tests { #[test] fn evaluate_custom_predicate_fail() { - let mut ctx = ctx_with_custom(vec![("foo", "/bin/false")]); + let (mut ctx, _scripts) = ctx_with_exit_codes(vec![("foo", 1)]); let pred = Predicate::Custom { name: "foo".into(), arg: "x".into(), @@ -1444,7 +1453,18 @@ mod tests { #[test] fn evaluate_custom_predicate_spawn_failure() { - let mut ctx = ctx_with_custom(vec![("foo", "/nonexistent/binary/zzz")]); + use std::collections::HashMap; + let mut entries = HashMap::new(); + entries.insert( + "foo".to_string(), + ResolvedPredicateEntry { + runnable: symposium_install::Runnable::Exec(std::path::PathBuf::from( + "/nonexistent/binary/zzz", + )), + args: vec![], + }, + ); + let mut ctx = PredicateContext::with_custom_predicates(&[], entries); let pred = Predicate::Custom { name: "foo".into(), arg: "x".into(), @@ -1541,7 +1561,7 @@ mod tests { #[test] fn witness_custom_empty_stdout() { - let mut ctx = ctx_with_custom(vec![("foo", "/bin/true")]); + let (mut ctx, _scripts) = ctx_with_exit_codes(vec![("foo", 0)]); let pred = Predicate::Custom { name: "foo".into(), arg: "x".into(), @@ -1552,7 +1572,7 @@ mod tests { #[test] fn witness_custom_exit_nonzero() { - let mut ctx = ctx_with_custom(vec![("foo", "/bin/false")]); + let (mut ctx, _scripts) = ctx_with_exit_codes(vec![("foo", 1)]); let pred = Predicate::Custom { name: "foo".into(), arg: "x".into(), From 873b488bce57008b4a2a5f2ad846473612e9f4cd Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Jun 2026 17:48:07 +0000 Subject: [PATCH 07/13] Add integration tests for argument passing and witness-driven crate source Address PR review feedback: - Add comment to fixture explaining the script is test-generated - Add test that verifies the predicate argument ("cli") is passed correctly - Add test showing wrong argument causes predicate failure - Add witness integration test: custom predicate returns selectedCrates JSON, which drives `source = "crate"` skill resolution from a path dep Co-authored-by: Claude --- tests/custom_predicates.rs | 120 +++++++++++++++++- .../custom-predicate-witness0/Cargo.toml | 7 + .../bp-crate/Cargo.toml | 4 + .../bp-crate/skills/bp-guidance/SKILL.md | 6 + .../bp-crate/src/lib.rs | 0 .../dot-symposium/config.toml | 5 + .../plugins/bp-plugin/SYMPOSIUM.toml | 15 +++ .../custom-predicate-witness0/src/lib.rs | 0 .../plugins/bp-plugin/SYMPOSIUM.toml | 2 + 9 files changed, 156 insertions(+), 3 deletions(-) create mode 100644 tests/fixtures/custom-predicate-witness0/Cargo.toml create mode 100644 tests/fixtures/custom-predicate-witness0/bp-crate/Cargo.toml create mode 100644 tests/fixtures/custom-predicate-witness0/bp-crate/skills/bp-guidance/SKILL.md create mode 100644 tests/fixtures/custom-predicate-witness0/bp-crate/src/lib.rs create mode 100644 tests/fixtures/custom-predicate-witness0/dot-symposium/config.toml create mode 100644 tests/fixtures/custom-predicate-witness0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml create mode 100644 tests/fixtures/custom-predicate-witness0/src/lib.rs diff --git a/tests/custom_predicates.rs b/tests/custom_predicates.rs index 14e80683..5de798e1 100644 --- a/tests/custom_predicates.rs +++ b/tests/custom_predicates.rs @@ -21,7 +21,6 @@ async fn sync_custom_predicate_installs_skill() { TestMode::SimulationOnly, &["custom-predicate0"], async |mut ctx| { - // Write a passing predicate script let script_path = ctx.tempdir.join("bp-checker.sh"); write_script(&script_path, "exit 0"); @@ -57,7 +56,6 @@ async fn sync_custom_predicate_fails_skips_skill() { TestMode::SimulationOnly, &["custom-predicate0"], async |mut ctx| { - // Write a failing predicate script let script_path = ctx.tempdir.join("bp-checker.sh"); write_script(&script_path, "exit 1"); @@ -70,7 +68,6 @@ async fn sync_custom_predicate_fails_skips_skill() { .unwrap() .join(".claude") .join("skills"); - // The skill should NOT be installed let entries: Vec<_> = std::fs::read_dir(&skills_dir) .ok() .map(|d| { @@ -91,3 +88,120 @@ async fn sync_custom_predicate_fails_skips_skill() { .await .unwrap(); } + +/// The raw argument from the predicate expression is passed to the script. +/// `predicates = ["battery_pack(cli)"]` should pass "cli" as the last arg. +#[tokio::test] +async fn sync_custom_predicate_receives_correct_argument() { + with_fixture( + TestMode::SimulationOnly, + &["custom-predicate0"], + async |mut ctx| { + let script_path = ctx.tempdir.join("bp-checker.sh"); + // Only pass if the argument is exactly "cli" + write_script( + &script_path, + r#"if [ "$1" = "cli" ]; then exit 0; else exit 1; fi"#, + ); + + ctx.symposium(&["init", "--add-agent", "claude"]).await?; + ctx.symposium(&["sync"]).await?; + + let skills_dir = ctx + .workspace_root + .as_ref() + .unwrap() + .join(".claude") + .join("skills"); + assert!( + skills_dir.join("bp-skill").join("SKILL.md").exists(), + "skill should be installed when argument matches 'cli'" + ); + Ok(()) + }, + ) + .await + .unwrap(); +} + +/// When the argument doesn't match, the predicate fails. +#[tokio::test] +async fn sync_custom_predicate_wrong_argument_fails() { + with_fixture( + TestMode::SimulationOnly, + &["custom-predicate0"], + async |mut ctx| { + let script_path = ctx.tempdir.join("bp-checker.sh"); + // Only pass if the argument is "web" — but the fixture uses "cli" + write_script( + &script_path, + r#"if [ "$1" = "web" ]; then exit 0; else exit 1; fi"#, + ); + + ctx.symposium(&["init", "--add-agent", "claude"]).await?; + ctx.symposium(&["sync"]).await?; + + let skills_dir = ctx + .workspace_root + .as_ref() + .unwrap() + .join(".claude") + .join("skills"); + let entries: Vec<_> = std::fs::read_dir(&skills_dir) + .ok() + .map(|d| { + d.flatten() + .filter(|e| e.path().is_dir()) + .filter(|e| e.path().join("SKILL.md").exists()) + .collect() + }) + .unwrap_or_default(); + assert!( + entries.is_empty(), + "skill should NOT be installed when argument doesn't match" + ); + Ok(()) + }, + ) + .await + .unwrap(); +} + +/// A custom predicate that returns witness JSON drives `source = "crate"` resolution. +/// The predicate script outputs `{"selectedCrates": [...]}` naming bp-crate, +/// and the skill from that crate's skills/ directory gets installed. +#[tokio::test] +async fn sync_custom_predicate_witness_drives_crate_source() { + with_fixture( + TestMode::SimulationOnly, + &["custom-predicate-witness0"], + async |mut ctx| { + let script_path = ctx.tempdir.join("bp-checker.sh"); + write_script( + &script_path, + r#"printf '{"selectedCrates":[{"crate":"bp-crate","version":"0.2.0"}]}'"#, + ); + + ctx.symposium(&["init", "--add-agent", "claude"]).await?; + ctx.symposium(&["sync"]).await?; + + let skills_dir = ctx + .workspace_root + .as_ref() + .unwrap() + .join(".claude") + .join("skills"); + assert!( + skills_dir.join("bp-guidance").join("SKILL.md").exists(), + "skill from witness crate should be installed; skills_dir={}, contents={:?}", + skills_dir.display(), + std::fs::read_dir(&skills_dir) + .ok() + .map(|d| d.flatten().map(|e| e.path()).collect::>()), + ); + Ok(()) + }, + ) + .await + .unwrap(); +} diff --git a/tests/fixtures/custom-predicate-witness0/Cargo.toml b/tests/fixtures/custom-predicate-witness0/Cargo.toml new file mode 100644 index 00000000..87ab8dd1 --- /dev/null +++ b/tests/fixtures/custom-predicate-witness0/Cargo.toml @@ -0,0 +1,7 @@ +[package] +name = "test-workspace" +version = "0.1.0" +edition = "2021" + +[dependencies] +bp-crate = { path = "bp-crate" } diff --git a/tests/fixtures/custom-predicate-witness0/bp-crate/Cargo.toml b/tests/fixtures/custom-predicate-witness0/bp-crate/Cargo.toml new file mode 100644 index 00000000..7b5819c7 --- /dev/null +++ b/tests/fixtures/custom-predicate-witness0/bp-crate/Cargo.toml @@ -0,0 +1,4 @@ +[package] +name = "bp-crate" +version = "0.2.0" +edition = "2021" diff --git a/tests/fixtures/custom-predicate-witness0/bp-crate/skills/bp-guidance/SKILL.md b/tests/fixtures/custom-predicate-witness0/bp-crate/skills/bp-guidance/SKILL.md new file mode 100644 index 00000000..944c96ef --- /dev/null +++ b/tests/fixtures/custom-predicate-witness0/bp-crate/skills/bp-guidance/SKILL.md @@ -0,0 +1,6 @@ +--- +name: bp-guidance +description: Guidance from the battery pack crate +--- + +Use the battery pack. diff --git a/tests/fixtures/custom-predicate-witness0/bp-crate/src/lib.rs b/tests/fixtures/custom-predicate-witness0/bp-crate/src/lib.rs new file mode 100644 index 00000000..e69de29b diff --git a/tests/fixtures/custom-predicate-witness0/dot-symposium/config.toml b/tests/fixtures/custom-predicate-witness0/dot-symposium/config.toml new file mode 100644 index 00000000..777c5d87 --- /dev/null +++ b/tests/fixtures/custom-predicate-witness0/dot-symposium/config.toml @@ -0,0 +1,5 @@ +hook-scope = "project" + +[defaults] +symposium-recommendations = false +user-plugins = true diff --git a/tests/fixtures/custom-predicate-witness0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml b/tests/fixtures/custom-predicate-witness0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml new file mode 100644 index 00000000..df386b49 --- /dev/null +++ b/tests/fixtures/custom-predicate-witness0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml @@ -0,0 +1,15 @@ +name = "bp-witness-plugin" +predicates = ["battery_pack(cli)"] + +[[installations]] +name = "bp-checker" +# Script generated at runtime by the test — returns witness JSON +# naming bp-crate so that source = "crate" resolution picks it up. +executable = "$TEST_DIR/bp-checker.sh" + +[[predicate]] +name = "battery_pack" +command = "bp-checker" + +[[skills]] +source = "crate" diff --git a/tests/fixtures/custom-predicate-witness0/src/lib.rs b/tests/fixtures/custom-predicate-witness0/src/lib.rs new file mode 100644 index 00000000..e69de29b diff --git a/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml b/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml index 5b3b9063..8b337317 100644 --- a/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml +++ b/tests/fixtures/custom-predicate0/dot-symposium/plugins/bp-plugin/SYMPOSIUM.toml @@ -3,6 +3,8 @@ predicates = ["battery_pack(cli)"] [[installations]] name = "bp-checker" +# Script generated at runtime by the test — allows tests to vary +# behavior (exit 0 vs exit 1, witness JSON, etc). executable = "$TEST_DIR/bp-checker.sh" [[predicate]] From e5c940ced5f72bb8728833da82e0cd5df598a90b Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Jun 2026 17:55:47 +0000 Subject: [PATCH 08/13] Fail predicate when stdout is non-empty but not valid witness JSON Previously, invalid JSON on stdout was silently ignored (predicate still passed with no witness). Now non-empty invalid stdout causes the predicate to evaluate as false, since the command likely has a bug. Empty stdout remains valid (pass with no witness crates). Co-authored-by: Claude --- md/reference/plugin-definition.md | 2 +- src/predicate.rs | 43 ++++++++++++++++++++----------- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/md/reference/plugin-definition.md b/md/reference/plugin-definition.md index cde64e75..64c53066 100644 --- a/md/reference/plugin-definition.md +++ b/md/reference/plugin-definition.md @@ -498,7 +498,7 @@ On success (exit 0), the command may write a JSON object to stdout. If present a } ``` -If stdout is empty or non-JSON, the predicate still passes but contributes no witness crates. Invalid JSON or bad semver in `version` fields produce a warning but don't fail the predicate. +If stdout is empty, the predicate passes but contributes no witness crates. If stdout is non-empty but not valid witness JSON, the predicate **fails** (treated as exit non-zero) and a warning is emitted. Individual entries with bad semver in `version` fields are skipped with a warning but don't fail the whole predicate. ### Collisions diff --git a/src/predicate.rs b/src/predicate.rs index 8cab97a4..3ce9d8a5 100644 --- a/src/predicate.rs +++ b/src/predicate.rs @@ -863,13 +863,22 @@ fn run_custom_predicate( "custom predicate stderr" ); } - let passed = output.status.success(); - let witness = if passed { - parse_witness_stdout(name, &output.stdout) - } else { - Vec::new() - }; - CustomPredicateResult { passed, witness } + if !output.status.success() { + return CustomPredicateResult { + passed: false, + witness: Vec::new(), + }; + } + match parse_witness_stdout(name, &output.stdout) { + Some(witness) => CustomPredicateResult { + passed: true, + witness, + }, + None => CustomPredicateResult { + passed: false, + witness: Vec::new(), + }, + } } Err(e) => { tracing::warn!( @@ -886,9 +895,13 @@ fn run_custom_predicate( } /// Parse witness JSON from custom predicate stdout. -fn parse_witness_stdout(predicate_name: &str, stdout: &[u8]) -> Vec { +/// +/// Returns `None` if stdout is non-empty but not valid witness JSON (the +/// predicate should be treated as failed). Returns `Some(vec![])` for empty +/// stdout (pass, no witness crates). Returns `Some(crates)` on valid JSON. +fn parse_witness_stdout(predicate_name: &str, stdout: &[u8]) -> Option> { if stdout.is_empty() { - return Vec::new(); + return Some(Vec::new()); } #[derive(serde::Deserialize)] @@ -903,9 +916,9 @@ fn parse_witness_stdout(predicate_name: &str, stdout: &[u8]) -> Vec Vec Date: Tue, 9 Jun 2026 17:58:55 +0000 Subject: [PATCH 09/13] Fail predicate on any malformed witness entry Any invalid entry in selectedCrates (e.g. bad semver) now fails the whole predicate rather than silently skipping it. Consistent with the principle that non-empty invalid stdout means the command has a bug. Co-authored-by: Claude --- md/reference/plugin-definition.md | 2 +- src/predicate.rs | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/md/reference/plugin-definition.md b/md/reference/plugin-definition.md index 64c53066..2c09374c 100644 --- a/md/reference/plugin-definition.md +++ b/md/reference/plugin-definition.md @@ -498,7 +498,7 @@ On success (exit 0), the command may write a JSON object to stdout. If present a } ``` -If stdout is empty, the predicate passes but contributes no witness crates. If stdout is non-empty but not valid witness JSON, the predicate **fails** (treated as exit non-zero) and a warning is emitted. Individual entries with bad semver in `version` fields are skipped with a warning but don't fail the whole predicate. +If stdout is empty, the predicate passes but contributes no witness crates. If stdout is non-empty but not valid JSON, or any entry has an invalid `version` field, the predicate **fails** (treated as exit non-zero) and a warning is emitted. ### Collisions diff --git a/src/predicate.rs b/src/predicate.rs index 3ce9d8a5..162ae5ba 100644 --- a/src/predicate.rs +++ b/src/predicate.rs @@ -930,8 +930,9 @@ fn parse_witness_stdout(predicate_name: &str, stdout: &[u8]) -> Option Date: Tue, 9 Jun 2026 18:02:50 +0000 Subject: [PATCH 10/13] Document and test that empty/whitespace args are not passed - `foo()` and `foo( )` do not append any argument to the command - Leading/trailing whitespace is stripped from the argument - Add unit tests verifying parsing and runtime behavior Co-authored-by: Claude --- md/reference/plugin-definition.md | 2 + src/predicate.rs | 65 +++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/md/reference/plugin-definition.md b/md/reference/plugin-definition.md index 2c09374c..0d7957ce 100644 --- a/md/reference/plugin-definition.md +++ b/md/reference/plugin-definition.md @@ -486,6 +486,8 @@ cargo-bp bp status --check cli>=0.3 Exit 0 means the predicate passes; non-zero means it fails. +The argument is trimmed of leading/trailing whitespace before being passed. An empty argument — `battery_pack()` or `battery_pack( )` — does not append anything to the command (only the static `args` are passed). + ### Witness output (stdout JSON) On success (exit 0), the command may write a JSON object to stdout. If present and valid, the `selectedCrates` field drives `source = "crate"` skill resolution — the named crates are fetched for skills, just as if they'd been matched by a `crate(...)` predicate. diff --git a/src/predicate.rs b/src/predicate.rs index 162ae5ba..727e0505 100644 --- a/src/predicate.rs +++ b/src/predicate.rs @@ -1557,6 +1557,71 @@ mod tests { assert_eq!(content.trim(), "--static arg dynamic-arg"); } + #[test] + fn evaluate_custom_predicate_empty_arg_not_passed() { + use std::io::Write; + let output_file = tempfile::NamedTempFile::new().unwrap(); + let output_path = output_file.path().to_path_buf(); + + let script = tempfile::Builder::new().suffix(".sh").tempfile().unwrap(); + writeln!( + script.as_file(), + "#!/bin/sh\necho \"$@\" > {}", + output_path.display() + ) + .unwrap(); + + let mut entries = std::collections::HashMap::new(); + entries.insert( + "checker".to_string(), + ResolvedPredicateEntry { + runnable: symposium_install::Runnable::Script(script.path().to_path_buf()), + args: vec!["--static".into()], + }, + ); + let mut ctx = PredicateContext::with_custom_predicates(&[], entries); + + // Empty arg (from `foo()`) — should not be appended. + let pred = Predicate::Custom { + name: "checker".into(), + arg: "".into(), + }; + assert!(pred.evaluate(&mut ctx)); + let content = std::fs::read_to_string(&output_path).unwrap(); + assert_eq!(content.trim(), "--static"); + } + + #[test] + fn parse_custom_predicate_whitespace_arg_is_empty() { + // `foo( )` parses to empty arg after trimming. + let p = parse("foo( )").unwrap(); + assert_eq!( + p, + Predicate::Custom { + name: "foo".into(), + arg: "".into() + } + ); + // `foo( \t )` also trims to empty. + let p2 = parse("foo( \t )").unwrap(); + assert_eq!( + p2, + Predicate::Custom { + name: "foo".into(), + arg: "".into() + } + ); + // Leading/trailing whitespace is stripped from the argument. + let p3 = parse("foo( hello )").unwrap(); + assert_eq!( + p3, + Predicate::Custom { + name: "foo".into(), + arg: "hello".into() + } + ); + } + // --- Witness tests --- #[test] From a704384c92d6150006b7f9ebf2c42eeb5ee9988b Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Jun 2026 20:58:27 +0000 Subject: [PATCH 11/13] Add integration tests for argument passing and witness-driven crate source - Add cross-plugin predicate tests: one plugin defines [[predicate]], another plugin uses it in predicates = [...]. Demonstrates that custom predicates are always registered regardless of which plugin defines them and can be used by any other plugin. - Tests both pass and fail paths for cross-plugin usage. Co-authored-by: Claude --- tests/custom_predicates.rs | 76 +++++++++++++++++++ .../custom-predicate-cross0/Cargo.toml | 7 ++ .../dot-symposium/config.toml | 5 ++ .../plugins/consumer-plugin/SYMPOSIUM.toml | 5 ++ .../skills/consumer-skill/SKILL.md | 6 ++ .../plugins/provider-plugin/SYMPOSIUM.toml | 10 +++ .../custom-predicate-cross0/src/lib.rs | 0 7 files changed, 109 insertions(+) create mode 100644 tests/fixtures/custom-predicate-cross0/Cargo.toml create mode 100644 tests/fixtures/custom-predicate-cross0/dot-symposium/config.toml create mode 100644 tests/fixtures/custom-predicate-cross0/dot-symposium/plugins/consumer-plugin/SYMPOSIUM.toml create mode 100644 tests/fixtures/custom-predicate-cross0/dot-symposium/plugins/consumer-plugin/skills/consumer-skill/SKILL.md create mode 100644 tests/fixtures/custom-predicate-cross0/dot-symposium/plugins/provider-plugin/SYMPOSIUM.toml create mode 100644 tests/fixtures/custom-predicate-cross0/src/lib.rs diff --git a/tests/custom_predicates.rs b/tests/custom_predicates.rs index 5de798e1..c9714f6b 100644 --- a/tests/custom_predicates.rs +++ b/tests/custom_predicates.rs @@ -167,6 +167,82 @@ async fn sync_custom_predicate_wrong_argument_fails() { .unwrap(); } +/// A predicate defined by one plugin can be used by a different plugin. +/// The provider-plugin defines `my_check`; the consumer-plugin uses it. +#[tokio::test] +async fn sync_custom_predicate_cross_plugin() { + with_fixture( + TestMode::SimulationOnly, + &["custom-predicate-cross0"], + async |mut ctx| { + let script_path = ctx.tempdir.join("cross-checker.sh"); + write_script(&script_path, "exit 0"); + + ctx.symposium(&["init", "--add-agent", "claude"]).await?; + ctx.symposium(&["sync"]).await?; + + let skills_dir = ctx + .workspace_root + .as_ref() + .unwrap() + .join(".claude") + .join("skills"); + assert!( + skills_dir.join("consumer-skill").join("SKILL.md").exists(), + "consumer plugin skill should install when provider's predicate passes; \ + skills_dir={}, contents={:?}", + skills_dir.display(), + std::fs::read_dir(&skills_dir) + .ok() + .map(|d| d.flatten().map(|e| e.path()).collect::>()), + ); + Ok(()) + }, + ) + .await + .unwrap(); +} + +/// Cross-plugin predicate: when the provider's predicate fails, the consumer's +/// skill is not installed. +#[tokio::test] +async fn sync_custom_predicate_cross_plugin_fails() { + with_fixture( + TestMode::SimulationOnly, + &["custom-predicate-cross0"], + async |mut ctx| { + let script_path = ctx.tempdir.join("cross-checker.sh"); + write_script(&script_path, "exit 1"); + + ctx.symposium(&["init", "--add-agent", "claude"]).await?; + ctx.symposium(&["sync"]).await?; + + let skills_dir = ctx + .workspace_root + .as_ref() + .unwrap() + .join(".claude") + .join("skills"); + let entries: Vec<_> = std::fs::read_dir(&skills_dir) + .ok() + .map(|d| { + d.flatten() + .filter(|e| e.path().is_dir()) + .filter(|e| e.path().join("SKILL.md").exists()) + .collect() + }) + .unwrap_or_default(); + assert!( + entries.is_empty(), + "consumer skill should NOT install when provider's predicate fails" + ); + Ok(()) + }, + ) + .await + .unwrap(); +} + /// A custom predicate that returns witness JSON drives `source = "crate"` resolution. /// The predicate script outputs `{"selectedCrates": [...]}` naming bp-crate, /// and the skill from that crate's skills/ directory gets installed. diff --git a/tests/fixtures/custom-predicate-cross0/Cargo.toml b/tests/fixtures/custom-predicate-cross0/Cargo.toml new file mode 100644 index 00000000..43692309 --- /dev/null +++ b/tests/fixtures/custom-predicate-cross0/Cargo.toml @@ -0,0 +1,7 @@ +[package] +name = "test-workspace" +version = "0.1.0" +edition = "2021" + +[dependencies] +serde = "1.0" diff --git a/tests/fixtures/custom-predicate-cross0/dot-symposium/config.toml b/tests/fixtures/custom-predicate-cross0/dot-symposium/config.toml new file mode 100644 index 00000000..777c5d87 --- /dev/null +++ b/tests/fixtures/custom-predicate-cross0/dot-symposium/config.toml @@ -0,0 +1,5 @@ +hook-scope = "project" + +[defaults] +symposium-recommendations = false +user-plugins = true diff --git a/tests/fixtures/custom-predicate-cross0/dot-symposium/plugins/consumer-plugin/SYMPOSIUM.toml b/tests/fixtures/custom-predicate-cross0/dot-symposium/plugins/consumer-plugin/SYMPOSIUM.toml new file mode 100644 index 00000000..94a14d1a --- /dev/null +++ b/tests/fixtures/custom-predicate-cross0/dot-symposium/plugins/consumer-plugin/SYMPOSIUM.toml @@ -0,0 +1,5 @@ +name = "consumer-plugin" +predicates = ["my_check(hello)"] + +[[skills]] +source.path = "skills" diff --git a/tests/fixtures/custom-predicate-cross0/dot-symposium/plugins/consumer-plugin/skills/consumer-skill/SKILL.md b/tests/fixtures/custom-predicate-cross0/dot-symposium/plugins/consumer-plugin/skills/consumer-skill/SKILL.md new file mode 100644 index 00000000..6e34765b --- /dev/null +++ b/tests/fixtures/custom-predicate-cross0/dot-symposium/plugins/consumer-plugin/skills/consumer-skill/SKILL.md @@ -0,0 +1,6 @@ +--- +name: consumer-skill +description: Skill from the consumer plugin +--- + +This skill is gated by a predicate defined in a different plugin. diff --git a/tests/fixtures/custom-predicate-cross0/dot-symposium/plugins/provider-plugin/SYMPOSIUM.toml b/tests/fixtures/custom-predicate-cross0/dot-symposium/plugins/provider-plugin/SYMPOSIUM.toml new file mode 100644 index 00000000..a17acc7e --- /dev/null +++ b/tests/fixtures/custom-predicate-cross0/dot-symposium/plugins/provider-plugin/SYMPOSIUM.toml @@ -0,0 +1,10 @@ +name = "provider-plugin" +crates = ["*"] + +[[installations]] +name = "my-checker" +executable = "$TEST_DIR/cross-checker.sh" + +[[predicate]] +name = "my_check" +command = "my-checker" diff --git a/tests/fixtures/custom-predicate-cross0/src/lib.rs b/tests/fixtures/custom-predicate-cross0/src/lib.rs new file mode 100644 index 00000000..e69de29b From 66e50dc76d67e89e37fe4ec7654795460ba07fc2 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Jun 2026 21:01:27 +0000 Subject: [PATCH 12/13] Introduce CustomPredicateRegistry newtype Replace the raw HashMap with a dedicated CustomPredicateRegistry struct that exposes only the operations we need (get, iter, contains_key, len, is_empty). Co-authored-by: Claude --- src/help_render.rs | 2 +- src/plugins.rs | 52 +++++++++++++++++++++++++++++--------- src/skills.rs | 12 ++++----- src/subcommand_dispatch.rs | 2 +- src/sync.rs | 2 +- 5 files changed, 49 insertions(+), 21 deletions(-) diff --git a/src/help_render.rs b/src/help_render.rs index bb3188eb..6b32eb3b 100644 --- a/src/help_render.rs +++ b/src/help_render.rs @@ -234,7 +234,7 @@ mod tests { plugins, standalone_skills: vec![], warnings: vec![], - custom_predicates: std::collections::HashMap::new(), + custom_predicates: crate::plugins::CustomPredicateRegistry::default(), } } diff --git a/src/plugins.rs b/src/plugins.rs index 18379e99..f7271327 100644 --- a/src/plugins.rs +++ b/src/plugins.rs @@ -723,6 +723,38 @@ pub struct ResolvedCustomPredicate { pub args: Vec, } +/// Global registry of custom predicates collected from all plugins. +/// +/// Built unconditionally from every plugin's `[[predicate]]` entries (regardless +/// of whether the plugin is "active" in the current workspace). Collisions +/// (same name from two plugins) are excluded and warned at load time. +#[derive(Debug, Default)] +pub struct CustomPredicateRegistry { + entries: std::collections::HashMap, +} + +impl CustomPredicateRegistry { + pub fn is_empty(&self) -> bool { + self.entries.is_empty() + } + + pub fn len(&self) -> usize { + self.entries.len() + } + + pub fn get(&self, name: &str) -> Option<&ResolvedCustomPredicate> { + self.entries.get(name) + } + + pub fn contains_key(&self, name: &str) -> bool { + self.entries.contains_key(name) + } + + pub fn iter(&self) -> impl Iterator { + self.entries.iter() + } +} + /// Loaded plugin registry: plugins from TOML manifests and standalone skills /// discovered directly in plugin source directories. #[derive(Debug)] @@ -735,8 +767,7 @@ pub struct PluginRegistry { /// Non-fatal load warnings for plugins or standalone skills that were skipped. pub warnings: Vec, /// Global custom predicate registry. Built from all plugins' `custom_predicates`. - /// Collisions (same name from two plugins) are excluded and warned. - pub custom_predicates: std::collections::HashMap, + pub custom_predicates: CustomPredicateRegistry, } /// A non-fatal plugin source load failure. @@ -1681,10 +1712,8 @@ fn validate_custom_predicate( fn build_custom_predicate_registry( plugins: &[ParsedPlugin], warnings: &mut Vec, -) -> std::collections::HashMap { - use std::collections::HashMap; - - let mut registry: HashMap = HashMap::new(); +) -> CustomPredicateRegistry { + let mut entries = std::collections::HashMap::new(); let mut collisions: std::collections::HashSet = std::collections::HashSet::new(); for (plugin_idx, parsed) in plugins.iter().enumerate() { @@ -1692,9 +1721,8 @@ fn build_custom_predicate_registry( if collisions.contains(&cp.name) { continue; } - if let Some(existing) = registry.get(&cp.name) { - // Collision: two different plugins define the same name. - // Skip both and warn. + if let Some(existing) = entries.get(&cp.name) { + let existing: &ResolvedCustomPredicate = existing; let existing_plugin_name = &plugins[existing.plugin_index].plugin.name; warnings.push(LoadWarning { path: parsed.path.clone(), @@ -1703,10 +1731,10 @@ fn build_custom_predicate_registry( cp.name, existing_plugin_name, parsed.plugin.name ), }); - registry.remove(&cp.name); + entries.remove(&cp.name); collisions.insert(cp.name.clone()); } else { - registry.insert( + entries.insert( cp.name.clone(), ResolvedCustomPredicate { plugin_index: plugin_idx, @@ -1718,7 +1746,7 @@ fn build_custom_predicate_registry( } } - registry + CustomPredicateRegistry { entries } } /// Validate skill-group source constraints that serde alone cannot express. diff --git a/src/skills.rs b/src/skills.rs index b0ccbd94..0e872313 100644 --- a/src/skills.rs +++ b/src/skills.rs @@ -1311,7 +1311,7 @@ mod tests { }], standalone_skills: vec![], warnings: vec![], - custom_predicates: std::collections::HashMap::new(), + custom_predicates: crate::plugins::CustomPredicateRegistry::default(), }; // Query for serde - should find no skills because plugin doesn't apply @@ -1366,7 +1366,7 @@ mod tests { }], standalone_skills: vec![], warnings: vec![], - custom_predicates: std::collections::HashMap::new(), + custom_predicates: crate::plugins::CustomPredicateRegistry::default(), }; // Query for serde - should find no skills because group doesn't match @@ -1439,7 +1439,7 @@ mod tests { }], standalone_skills: vec![], warnings: vec![], - custom_predicates: std::collections::HashMap::new(), + custom_predicates: crate::plugins::CustomPredicateRegistry::default(), }; let workspace_crates = vec![crate::crate_sources::WorkspaceCrate { @@ -1517,7 +1517,7 @@ mod tests { }], standalone_skills: vec![], warnings: vec![], - custom_predicates: std::collections::HashMap::new(), + custom_predicates: crate::plugins::CustomPredicateRegistry::default(), }; let workspace = vec![crate::crate_sources::WorkspaceCrate { @@ -1596,7 +1596,7 @@ mod tests { }], standalone_skills: vec![], warnings: vec![], - custom_predicates: std::collections::HashMap::new(), + custom_predicates: crate::plugins::CustomPredicateRegistry::default(), }; let workspace = vec![crate::crate_sources::WorkspaceCrate { @@ -1727,7 +1727,7 @@ mod tests { }, }], warnings: vec![], - custom_predicates: std::collections::HashMap::new(), + custom_predicates: crate::plugins::CustomPredicateRegistry::default(), }; let sym = crate::config::Symposium::from_dir(tmp.path()); diff --git a/src/subcommand_dispatch.rs b/src/subcommand_dispatch.rs index 1a25b4ff..5186adc6 100644 --- a/src/subcommand_dispatch.rs +++ b/src/subcommand_dispatch.rs @@ -207,7 +207,7 @@ mod tests { plugins, standalone_skills: vec![], warnings: vec![], - custom_predicates: std::collections::HashMap::new(), + custom_predicates: crate::plugins::CustomPredicateRegistry::default(), } } diff --git a/src/sync.rs b/src/sync.rs index 7994f44d..1d3d572d 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -174,7 +174,7 @@ async fn resolve_custom_predicate_entries( let mut entries = std::collections::HashMap::new(); - for (name, resolved) in ®istry.custom_predicates { + for (name, resolved) in registry.custom_predicates.iter() { let plugin = ®istry.plugins[resolved.plugin_index]; let Some(install) = plugin.plugin.get_installation(&resolved.command) else { tracing::warn!( From cc56f8e3d6c4bed507fada49d7f4c1615b0c8e88 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Jun 2026 21:02:24 +0000 Subject: [PATCH 13/13] Document that custom predicates are globally available across plugins Co-authored-by: Claude --- md/reference/plugin-definition.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/md/reference/plugin-definition.md b/md/reference/plugin-definition.md index 0d7957ce..745dd7ac 100644 --- a/md/reference/plugin-definition.md +++ b/md/reference/plugin-definition.md @@ -457,6 +457,8 @@ Each `[[predicate]]` entry defines a custom predicate function that can be used ### How custom predicates work +Custom predicates are registered globally — a predicate defined in one plugin can be used by any other plugin's `predicates` expressions. Registration is unconditional: even if the defining plugin's own crate predicates don't match the current workspace, its `[[predicate]]` entries are still available. + When a predicate expression uses a function name that isn't a builtin, Symposium looks it up in the custom predicate registry. If found, it spawns the declared command with the static `args` followed by the raw argument text from the expression. ```toml