From e26eb59b5c53d76227eee29e65807e73d0229f26 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 13:52:38 -0700 Subject: [PATCH 1/4] refactor(search): move the marketplace matcher to catalog_search The tokenising, ranking matcher written for the two marketplace searches is about to serve the installed-skill search as well, which has nothing to do with the marketplace. It moves from `marketplace/search.rs` to a neutral `catalog_search.rs`; its result types drop the `Marketplace` prefix (`CatalogSearch`, `CatalogSearchHit`), and `rank`, `Weight` and the noise lists become crate-visible so a second module can call them. No behaviour changes. The matcher's own seven tests and every marketplace test pass unchanged. --- .../search.rs => catalog_search.rs} | 38 ++++++++++--------- crates/biorouter/src/lib.rs | 1 + crates/biorouter/src/marketplace.rs | 22 +++++------ 3 files changed, 30 insertions(+), 31 deletions(-) rename crates/biorouter/src/{marketplace/search.rs => catalog_search.rs} (93%) diff --git a/crates/biorouter/src/marketplace/search.rs b/crates/biorouter/src/catalog_search.rs similarity index 93% rename from crates/biorouter/src/marketplace/search.rs rename to crates/biorouter/src/catalog_search.rs index 52dacae22..288877a22 100644 --- a/crates/biorouter/src/marketplace/search.rs +++ b/crates/biorouter/src/catalog_search.rs @@ -1,6 +1,8 @@ -//! Free-text search over the trusted marketplace catalog — the matcher behind -//! both `skills__searchMarketplaceSkills` and -//! `extensionmanager__search_marketplace_extensions`. +//! Free-text search over a catalog of named entries — the ONE matcher behind +//! `skills__searchMarketplaceSkills` and +//! `extensionmanager__search_marketplace_extensions`. A new catalog search +//! should call [`rank`] with its own fields rather than grow a matcher of its +//! own: every copy of this logic so far has drifted into the failure below. //! //! ⚠ **A query is a set of words, not a substring.** The matcher this replaced //! asked whether the WHOLE lowercased query occurred inside a single field, so a @@ -36,7 +38,7 @@ /// How much a match in one field says about an entry. #[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub(super) enum Weight { +pub(crate) enum Weight { /// Free prose: a description. Prose = 1, /// Curated labels: tags, keywords, a category, an organization. @@ -97,17 +99,17 @@ const FILLER: &[&str] = &[ ]; /// Filler specific to the skills catalog: every entry in it is a skill. -pub(super) const SKILL_NOISE: &[&str] = &["skill", "skills"]; +pub(crate) const SKILL_NOISE: &[&str] = &["skill", "skills"]; /// Filler specific to the extensions catalog: every entry in it is an extension. -pub(super) const EXTENSION_NOISE: &[&str] = &["extension", "extensions"]; +pub(crate) const EXTENSION_NOISE: &[&str] = &["extension", "extensions"]; /// Below this many characters a term matches whole words only. const MIN_PARTIAL_CHARS: usize = 3; /// One entry a search returned, with the query terms it matched. #[derive(Debug)] -pub struct MarketplaceSearchHit<'a, T> { +pub struct CatalogSearchHit<'a, T> { pub entry: &'a T, /// The terms this entry matched, in query order. Empty only for an entry /// that nothing but the verbatim query found. @@ -117,14 +119,14 @@ pub struct MarketplaceSearchHit<'a, T> { /// A ranked search: the terms the query was split into, and every entry that /// matched at least one of them (or the whole query verbatim), best first. #[derive(Debug)] -pub struct MarketplaceSearch<'a, T> { +pub struct CatalogSearch<'a, T> { /// What the query was read as, after filler words were dropped. Reported /// to the model so a surprising result can be traced to its input. pub terms: Vec, - pub hits: Vec>, + pub hits: Vec>, } -impl MarketplaceSearch<'_, T> { +impl CatalogSearch<'_, T> { pub fn len(&self) -> usize { self.hits.len() } @@ -160,7 +162,7 @@ fn words(text: &str) -> impl Iterator + '_ { } /// The distinct terms of `query`, in the order written, without filler. -pub(super) fn terms(query: &str, noise: &[&str]) -> Vec { +fn terms(query: &str, noise: &[&str]) -> Vec { let mut all: Vec = Vec::new(); for word in words(query) { if !all.contains(&word) { @@ -217,19 +219,19 @@ fn term_strength(term: &str, word: &str) -> u32 { /// /// An empty (or all-whitespace) query is the browse case: every entry, in /// registry order. -pub(super) fn rank<'a, T>( +pub(crate) fn rank<'a, T>( query: &str, noise: &[&str], entries: impl IntoIterator, fields: impl Fn(&'a T) -> Vec<(&'a str, Weight)>, -) -> MarketplaceSearch<'a, T> { +) -> CatalogSearch<'a, T> { let phrase = query.trim().to_lowercase(); if phrase.is_empty() { - return MarketplaceSearch { + return CatalogSearch { terms: Vec::new(), hits: entries .into_iter() - .map(|entry| MarketplaceSearchHit { + .map(|entry| CatalogSearchHit { entry, matched_terms: Vec::new(), }) @@ -265,7 +267,7 @@ pub(super) fn rank<'a, T>( if verbatim || !matched_terms.is_empty() { ranked.push(( (verbatim, matched_terms.len(), score), - MarketplaceSearchHit { + CatalogSearchHit { entry, matched_terms, }, @@ -274,7 +276,7 @@ pub(super) fn rank<'a, T>( } // Stable, and descending on the key: equal ranks keep registry order. ranked.sort_by(|(left, _), (right, _)| right.cmp(left)); - MarketplaceSearch { + CatalogSearch { terms, hits: ranked.into_iter().map(|(_, hit)| hit).collect(), } @@ -301,7 +303,7 @@ mod tests { fields } - fn ids<'a>(search: &MarketplaceSearch<'a, Entry>) -> Vec<&'a str> { + fn ids<'a>(search: &CatalogSearch<'a, Entry>) -> Vec<&'a str> { search.hits.iter().map(|hit| hit.entry.id).collect() } diff --git a/crates/biorouter/src/lib.rs b/crates/biorouter/src/lib.rs index 4cf00d775..d8ff63092 100644 --- a/crates/biorouter/src/lib.rs +++ b/crates/biorouter/src/lib.rs @@ -18,6 +18,7 @@ compile_error!( pub mod action_required_manager; pub mod agents; pub mod catalog; +pub mod catalog_search; pub mod checkpoint; pub mod config; pub mod context_budget; diff --git a/crates/biorouter/src/marketplace.rs b/crates/biorouter/src/marketplace.rs index e038fe503..ba894b424 100644 --- a/crates/biorouter/src/marketplace.rs +++ b/crates/biorouter/src/marketplace.rs @@ -8,15 +8,11 @@ use futures::StreamExt; use serde::Deserialize; use url::Url; +use crate::catalog_search::{rank, CatalogSearch, Weight, EXTENSION_NOISE, SKILL_NOISE}; use crate::config::paths::Paths; use crate::privacy::affiliation::InstitutionId; use crate::privacy::{ExtensionAffiliation, ProviderTier}; -mod search; - -use search::Weight; -pub use search::{MarketplaceSearch, MarketplaceSearchHit}; - pub const REGISTRY_URL: &str = "https://biorouter.ucsf.edu/registry.json"; const REGISTRY_SOURCE: &str = "https://biorouter.ucsf.edu/baam"; pub const MAX_REGISTRY_BYTES: usize = 2 * 1024 * 1024; @@ -116,7 +112,7 @@ impl MarketplaceCatalog { /// Rank the extensions visible to `caller` against a free-text query. How a /// query is matched — and why a phrase is a union of its words rather than - /// one substring (finding F5) — is documented in `marketplace/search.rs`. + /// one substring (finding F5) — is documented in `catalog_search.rs`. /// /// The caller filter runs FIRST, so an extension hidden from `caller` is /// never scored and cannot move, or be counted among, what it is shown. @@ -124,10 +120,10 @@ impl MarketplaceCatalog { &self, caller: ProviderTier, query: &str, - ) -> MarketplaceSearch<'_, MarketplaceExtensionDescriptor> { - search::rank( + ) -> CatalogSearch<'_, MarketplaceExtensionDescriptor> { + rank( query, - search::EXTENSION_NOISE, + EXTENSION_NOISE, self.browse_extensions(caller), |entry| { let mut fields = vec![ @@ -148,9 +144,9 @@ impl MarketplaceCatalog { } /// Rank every skill against a free-text query, matched as documented in - /// `marketplace/search.rs`. - pub fn search_skills(&self, query: &str) -> MarketplaceSearch<'_, MarketplaceSkillDescriptor> { - search::rank(query, search::SKILL_NOISE, self.skills.values(), |entry| { + /// `catalog_search.rs`. + pub fn search_skills(&self, query: &str) -> CatalogSearch<'_, MarketplaceSkillDescriptor> { + rank(query, SKILL_NOISE, self.skills.values(), |entry| { let mut fields = vec![ (entry.registry_id.as_str(), Weight::Name), (entry.name.as_str(), Weight::Name), @@ -956,7 +952,7 @@ mod tests { .unwrap() } - fn skill_ids(search: &MarketplaceSearch<'_, MarketplaceSkillDescriptor>) -> Vec { + fn skill_ids(search: &CatalogSearch<'_, MarketplaceSkillDescriptor>) -> Vec { search .hits .iter() From cf96c5095f05de3978f7ded33eece28b68eb9290 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 13:59:08 -0700 Subject: [PATCH 2/4] fix(skills): rank an installed-skill query by its words, not all of them `skills__searchSkills` kept an installed skill only when its name, description and bundle held EVERY word of the query as a substring, so the phrase a model composes on a user's behalf found nothing unless one skill happened to say all of it. Measured with a ggplot skill ("Publication-quality ggplot2 visualization guide for R...") and an r-scripting skill installed: searchSkills {query: "R scripting ggplot visualization"} -> {"total": 0, ..., "skills": []} That is QA finding F5 again, on the installed catalog instead of the marketplace and through different code. It is fixed the same way, through `catalog_search::rank` rather than a second matcher: an any-term union, ranked by terms matched and then by where (name > bundle > description), with short terms matching whole words only and filler dropped. The same query now returns ggplot (3 terms), r-scripting (2) and python-scripting (1), and never rna-qc or variant-calling, whose text is full of the letter r but never says R. The page keeps its shape (total/offset/limit/returned/next_offset/skills and every provenance and removal field) and adds what the marketplace page carries: `terms`, a per-row `matchedTerms`, and, on zero hits, a `guidance` sentence counting the skills this conversation has enabled. It names only searchSkills, because an app agent holds searchSkills and loadSkill alone and this handler cannot see the roster. Unchanged, and now pinned on the search path too: the conversation's switches run before the ranking (a switched-off skill is neither returned nor counted), `removable` and `removalTarget` come through a ranked row, pagination walks the ranking, and an empty or wordless query is still the listing, carrying none of the new fields. One existing assertion encoded the AND and changes on purpose: `bio-bundle rna` returned 1 skill; it now returns all 5 in the bundle, with `rna-qc` first as the only one matching all three words. --- .../biorouter/src/agents/skills_extension.rs | 460 +++++++++++++++--- 1 file changed, 379 insertions(+), 81 deletions(-) diff --git a/crates/biorouter/src/agents/skills_extension.rs b/crates/biorouter/src/agents/skills_extension.rs index 2ecd74f41..1aabb8487 100644 --- a/crates/biorouter/src/agents/skills_extension.rs +++ b/crates/biorouter/src/agents/skills_extension.rs @@ -2,6 +2,7 @@ use crate::agents::extension::PlatformExtensionContext; use crate::agents::mcp_client::{Error, McpClientTrait, McpMeta}; use crate::agents::skill_catalog; use crate::catalog::{CatalogChangeReason, CatalogEntryChange, CatalogEvents, CatalogSkillChange}; +use crate::catalog_search::{self, Weight}; use crate::config::paths::Paths; use anyhow::Result; use async_trait::async_trait; @@ -33,7 +34,7 @@ pub static EXTENSION_NAME: &str = "skills"; const SKILL_OPERATION_GUIDANCE: &[(&str, &str)] = &[ ( "searchSkills", - "searchSkills lists installed skills, or filters them when you pass a query, and marks each one removable or not", + "searchSkills lists installed skills, or ranks those matching any word of a query you pass, and marks each one removable or not", ), ("loadSkill", "loadSkill reads an exact installed skill"), ( @@ -634,8 +635,10 @@ struct SessionSkillParams { #[derive(Debug, Serialize, Deserialize, JsonSchema)] struct SearchSkillsParams { - /// Filter and rank installed skills by name, description or bundle. Omit to - /// page the whole catalog alphabetically. + /// Rank installed skills by the words of this query found in their name, + /// description or bundle: a skill matching any word is returned, those + /// matching the most words first. Omit to page the whole catalog + /// alphabetically. /// /// ⚠ The doc comment is the contract: schemars emits it as the property's /// `description`, and that is the only channel through which a Gemini-bound @@ -711,6 +714,11 @@ struct SkillCatalogItem { /// `removable`. For a bundle member this is the BUNDLE, not the skill. #[serde(skip_serializing_if = "Option::is_none")] removal_target: Option, + /// The query terms this skill matched, in query order — present only on a + /// search, so a model reading a long ranked page can tell a skill that + /// matched every word from one that matched a single common one. + #[serde(skip_serializing_if = "Option::is_none")] + matched_terms: Option>, } /// Where a `SkillsClient` reads its skills from. @@ -1363,44 +1371,18 @@ impl SkillsClient { (offset, limit) } - fn normalize_search_text(value: &str) -> String { - value - .chars() - .map(|c| { - if c.is_alphanumeric() { - c.to_ascii_lowercase() - } else { - ' ' - } - }) - .collect::() - .split_whitespace() - .collect::>() - .join(" ") - } - - fn search_score(skill: &Skill, query: &str, terms: &[&str]) -> usize { - let name = Self::normalize_search_text(&skill.metadata.name); - let description = Self::normalize_search_text(&skill.metadata.description); - let bundle = Self::normalize_search_text(skill.bundle_name.as_deref().unwrap_or_default()); - - if name == query { - 100 - } else if name.contains(query) { - 90 - } else if terms.iter().all(|term| name.contains(term)) { - 80 - } else if description.contains(query) { - 70 - } else if terms.iter().all(|term| description.contains(term)) { - 60 - } else if bundle.contains(query) { - 50 - } else if terms.iter().all(|term| bundle.contains(term)) { - 40 - } else { - 0 + /// What `searchSkills` matches a query against, and how much a match in + /// each place counts: the skill's own name most, then the bundle it ships + /// in — a label its author gave a whole group — then its description. + fn search_fields(skill: &Skill) -> Vec<(&str, Weight)> { + let mut fields = vec![ + (skill.metadata.name.as_str(), Weight::Name), + (skill.metadata.description.as_str(), Weight::Prose), + ]; + if let Some(bundle) = skill.bundle_name.as_deref() { + fields.push((bundle, Weight::Label)); } + fields } /// The name `removeSkillPackage` takes for this skill, before the @@ -1451,15 +1433,18 @@ impl SkillsClient { extension: source.extension, removal_target: if removable { removal_target } else { None }, removable, + matched_terms: None, } } - fn catalog_response( + /// One page of the installed catalog — the listing's shape, which a search + /// extends rather than replaces. + fn catalog_page( total: usize, offset: usize, limit: usize, skills: Vec, - ) -> Result, String> { + ) -> serde_json::Value { let returned = skills.len(); let next_offset = if offset + returned < total { Some(offset + returned) @@ -1467,15 +1452,18 @@ impl SkillsClient { None }; - let response = serde_json::json!({ + serde_json::json!({ "total": total, "offset": offset, "limit": limit, "returned": returned, "next_offset": next_offset, "skills": skills, - }); - serde_json::to_string_pretty(&response) + }) + } + + fn catalog_response(page: &serde_json::Value) -> Result, String> { + serde_json::to_string_pretty(page) .map(|text| vec![Content::text(text)]) .map_err(|error| error.to_string()) } @@ -2057,20 +2045,65 @@ impl SkillsClient { .map(|(_, skill)| Self::catalog_item(skill, &sources)) .collect(); - Self::catalog_response(total, offset, limit, skills) + Self::catalog_response(&Self::catalog_page(total, offset, limit, skills)) } + /// What an empty installed-skill search says instead of a bare `total: 0` + /// — the installed-catalog counterpart of + /// [`Self::no_marketplace_skill_matched`]. A bare zero let a model tell + /// the user nothing installed fit the job, when the miss was more often the + /// query's wording. `enabled` is what the conversation could have matched: + /// a skill switched off here is not searched, so it is not counted. + /// + /// Only `searchSkills` is named. The caller may hold nothing else — an app + /// agent is granted `searchSkills` and `loadSkill` alone — and this handler + /// cannot see the roster, so pointing at the marketplace here could teach a + /// tool the caller does not have. + fn no_installed_skill_matched(asked: &str, enabled: usize) -> String { + match enabled { + 0 => format!( + "No installed skill matched {asked}: no skill is enabled in this conversation, \ + so there was nothing to search." + ), + 1 => format!( + "No installed skill matched {asked}. 1 skill is enabled in this conversation, so \ + this does not mean it is irrelevant: try a shorter or more general term (one \ + tool, language or topic name), or call searchSkills with no query to list it." + ), + enabled => format!( + "No installed skill matched {asked}. {enabled} skills are enabled in this \ + conversation, so this does not mean none of them is relevant: try a shorter or \ + more general term (one tool, language or topic name), or call searchSkills with \ + no query to list them all." + ), + } + } + + /// Search the skills this conversation has enabled. + /// + /// ⚠ **A query is ranked by its words, not filtered by all of them.** This + /// kept a skill only when its text held EVERY word of the query as a + /// substring, so the phrase a model composes on a user's behalf — `R + /// scripting ggplot visualization` — found nothing unless one skill said + /// all of it, and `r` matched nearly every skill there is. It is finding + /// F5's installed-skill twin, fixed the same way: through the one matcher + /// in [`crate::catalog_search`], never a copy of it. + /// + /// The conversation's switches ([`Self::enabled_skill_entries`]) run FIRST, + /// so a skill switched off here is never scored, returned or counted. async fn handle_search_skills( &self, arguments: Option, over: &crate::agents::session_skills::SessionSkillOverride, ) -> Result, String> { let params: SearchSkillsParams = Self::parse_tool_args(arguments)?; - let query = Self::normalize_search_text(params.query.as_deref().unwrap_or_default().trim()); + let query = params.query.as_deref().unwrap_or_default().trim(); // ⚠ An absent or empty query is the LIST case, not an error. This tool // absorbed `listSkills`, whose entire schema was the two pagination // fields; refusing here would refuse the call the retired tool made. - if query.is_empty() { + // A query without a single letter or digit has no word to match, and + // it has always listed too. + if !query.chars().any(char::is_alphanumeric) { return self .handle_list_skills( Some(serde_json::Map::from_iter([ @@ -2082,41 +2115,44 @@ impl SkillsClient { .await; } - let terms: Vec<&str> = query.split_whitespace().collect(); let (offset, limit) = Self::parse_pagination(params.offset, params.limit); let skills = self.skills.skills(); - let mut matches: Vec<_> = Self::enabled_skill_entries(&skills, over) - .into_iter() - .filter(|(_, skill)| { - let haystack = format!( - "{} {} {}", - Self::normalize_search_text(&skill.metadata.name), - Self::normalize_search_text(&skill.metadata.description), - Self::normalize_search_text(skill.bundle_name.as_deref().unwrap_or_default()) - ); - terms.iter().all(|term| haystack.contains(term)) - }) - .collect(); - - matches.sort_by(|(left_name, left_skill), (right_name, right_skill)| { - let left_score = Self::search_score(left_skill, &query, &terms); - let right_score = Self::search_score(right_skill, &query, &terms); - right_score - .cmp(&left_score) - .then_with(|| left_skill.metadata.name.cmp(&right_skill.metadata.name)) - .then_with(|| left_name.cmp(right_name)) - }); + let enabled = Self::enabled_skill_entries(&skills, over); + // `enabled` is sorted by name, and the ranking is stable, so skills + // that rank equally stay in alphabetical order. + let search = catalog_search::rank( + query, + catalog_search::SKILL_NOISE, + enabled.iter().map(|&(_, skill)| skill), + Self::search_fields, + ); - let total = matches.len(); + let total = search.len(); let sources = skill_catalog::root_sources(); - let skills = matches - .into_iter() + let rows = search + .hits + .iter() .skip(offset) .take(limit) - .map(|(_, skill)| Self::catalog_item(skill, &sources)) + .map(|hit| SkillCatalogItem { + matched_terms: Some(hit.matched_terms.clone()), + ..Self::catalog_item(hit.entry, &sources) + }) .collect(); - - Self::catalog_response(total, offset, limit, skills) + let mut page = Self::catalog_page(total, offset, limit, rows); + if let Some(fields) = page.as_object_mut() { + fields.insert("terms".to_owned(), serde_json::json!(&search.terms)); + if search.is_empty() { + fields.insert( + "guidance".to_owned(), + serde_json::Value::String(Self::no_installed_skill_matched( + &search.describe_query(query), + enabled.len(), + )), + ); + } + } + Self::catalog_response(&page) } async fn handle_load_skill( @@ -2800,9 +2836,11 @@ impl SkillsClient { indoc! {r#" List or search the skills installed on this machine. - Pass `query` to match a name, description or bundle; omit it to page the - whole catalog alphabetically. Use this before loadSkill when you need a - skill's exact name. Results are paginated. + Pass `query` to search names, descriptions and bundles: a skill matching any + of its words is returned, the skills matching the most words first, each with + the `matchedTerms` it matched. Omit `query` to page the whole catalog + alphabetically. Use this before loadSkill when you need a skill's exact name. + Results are paginated. Each result also says where the skill came from and whether it can be uninstalled: `builtin` marks one Biorouter ships and re-seeds on startup, @@ -3831,7 +3869,7 @@ Content } #[tokio::test] - async fn test_search_skills_filters_by_name_description_and_bundle() { + async fn test_search_skills_matches_name_description_and_bundle() { let temp_dir = TempDir::new().unwrap(); let bundle_dir = temp_dir.path().join("bio-bundle"); fs::create_dir(&bundle_dir).unwrap(); @@ -3928,8 +3966,21 @@ Content let text = &result.content[0].as_text().unwrap().text; let payload: serde_json::Value = serde_json::from_str(text).unwrap(); - assert_eq!(payload["total"], 1); + // The bundle's name is searched too. Every skill here ships in + // `bio-bundle`, so each matches two of the three words, and the one that + // also says `rna` matched all three and ranks first. This asserted + // `total == 1` while the search kept only skills holding EVERY word — + // the filter that answered F5's phrase with nothing. + assert_eq!(payload["total"], 5, "{payload:#}"); assert_eq!(payload["skills"][0]["name"], "rna-qc"); + assert_eq!( + payload["skills"][0]["matchedTerms"], + serde_json::json!(["bio", "bundle", "rna"]) + ); + assert_eq!( + payload["skills"][1]["matchedTerms"], + serde_json::json!(["bio", "bundle"]) + ); let args = serde_json::json!({ "query": "systematic review PRISMA", "limit": 10 }) .as_object() @@ -3954,6 +4005,230 @@ Content assert_eq!(payload["skills"][0]["name"], "systematic-review-prisma"); } + /// Installed skills shaped like the ones finding F5's query was after, plus + /// two whose text is full of the letter r without ever naming R. + const F5_INSTALLED: &[(&str, &str)] = &[ + ( + "ggplot", + "Publication-quality ggplot2 visualization guide for R. Use when creating new \ + ggplot figures, reviewing existing plots for publication readiness, or \ + refactoring code to improve aesthetics.", + ), + ( + "python-scripting", + "Applies Python naming, typing, error handling, and project structure \ + conventions when writing Python code.", + ), + ( + "r-scripting", + "Applies tidyverse conventions and documentation standards when writing or \ + reviewing R code.", + ), + ("rna-qc", "Quality control for transcriptomics"), + ("variant-calling", "Call variants from sequencing reads"), + ]; + + /// A client whose whole catalog is `skills`, each written as a SKILL.md and + /// read back by the real scanner, so a search sees exactly the frontmatter an + /// installed skill carries. Keep the `TempDir` alive as long as the client. + fn client_over_installed(skills: &[(&str, &str)]) -> (TempDir, SkillsClient) { + let root = TempDir::new().unwrap(); + for (name, description) in skills { + let skill_dir = root.path().join(name); + fs::create_dir(&skill_dir).unwrap(); + fs::write( + skill_dir.join("SKILL.md"), + format!("---\nname: {name}\ndescription: {description}\n---\nBody"), + ) + .unwrap(); + } + let mut client = SkillsClient::new(test_context()).unwrap(); + client.skills = + SkillsClient::discover_skills_in_directories(&[root.path().to_path_buf()]).into(); + (root, client) + } + + /// One `searchSkills` page, dispatched the way the model calls it. + async fn search_installed( + client: &SkillsClient, + arguments: serde_json::Value, + ) -> serde_json::Value { + let result = client + .call_tool( + "searchSkills", + arguments.as_object().cloned(), + McpMeta::new( + "test-session", + crate::privacy::CallCapability::for_test_restricted(), + ), + CancellationToken::new(), + ) + .await + .unwrap(); + serde_json::from_str(&tool_text(&result)).unwrap() + } + + /// [`search_installed`] under an explicit per-conversation override. It + /// calls the handler directly, because `call_tool` would read the override + /// from the session's row instead. + async fn search_installed_with( + client: &SkillsClient, + query: &str, + over: &crate::agents::session_skills::SessionSkillOverride, + ) -> serde_json::Value { + let arguments = serde_json::json!({ "query": query }).as_object().cloned(); + let content = client.handle_search_skills(arguments, over).await.unwrap(); + serde_json::from_str(&content[0].as_text().unwrap().text).unwrap() + } + + fn page_names(page: &serde_json::Value) -> Vec<&str> { + page["skills"] + .as_array() + .unwrap() + .iter() + .map(|skill| skill["name"].as_str().unwrap()) + .collect() + } + + /// Finding F5's installed-skill twin. `searchSkills` kept a skill only when + /// its text held EVERY word of the query as a substring, so the phrase a + /// model composes on a user's behalf found nothing unless one skill happened + /// to say all of it. Measured against these fixtures before the fix: the + /// phrase below returned `total: 0`, with a ggplot skill and an R-scripting + /// skill both installed. + /// + /// It now finds every skill matching any word of the phrase, ranked by how + /// many words each matched, and every row names the terms it matched — so + /// `python-scripting`, which matched only `scripting`, reads as the weak + /// hit it is. + #[tokio::test] + async fn an_installed_skill_search_ranks_every_skill_matching_a_word_of_the_phrase() { + let (_root, client) = client_over_installed(F5_INSTALLED); + let phrase = "R scripting ggplot visualization"; + + let page = search_installed(&client, serde_json::json!({ "query": phrase })).await; + assert_eq!(page["total"], 3, "measured before the fix: 0 — {page:#}"); + assert_eq!( + page_names(&page), + ["ggplot", "r-scripting", "python-scripting"], + "three of the four terms, then two, then one; `rna-qc` and \ + `variant-calling` are full of the letter r and match none" + ); + assert_eq!( + page["terms"], + serde_json::json!(["r", "scripting", "ggplot", "visualization"]) + ); + assert_eq!( + page["skills"][0]["matchedTerms"], + serde_json::json!(["r", "ggplot", "visualization"]) + ); + assert_eq!( + page["skills"][1]["matchedTerms"], + serde_json::json!(["r", "scripting"]) + ); + assert_eq!( + page["skills"][2]["matchedTerms"], + serde_json::json!(["scripting"]) + ); + assert!(page.get("guidance").is_none(), "{page:#}"); + + // A single word still finds exactly what it names. + let control = search_installed(&client, serde_json::json!({ "query": "ggplot" })).await; + assert_eq!(page_names(&control), ["ggplot"], "{control:#}"); + + // A ranked row is the listing's row plus `matchedTerms` — provenance, + // `removable` and `removalTarget` come through the ranking unchanged. + let listed = search_installed(&client, serde_json::json!({})).await; + let listed_row = listed["skills"] + .as_array() + .unwrap() + .iter() + .find(|row| row["name"] == "ggplot") + .unwrap() + .clone(); + let mut ranked_row = page["skills"][0].clone(); + ranked_row.as_object_mut().unwrap().remove("matchedTerms"); + assert_eq!(ranked_row, listed_row); + + // Pagination walks the ranking, not the alphabet. + let second = search_installed( + &client, + serde_json::json!({ "query": phrase, "offset": 1, "limit": 1 }), + ) + .await; + assert_eq!(second["total"], 3); + assert_eq!(second["returned"], 1); + assert_eq!(second["next_offset"], 2); + assert_eq!(page_names(&second), ["r-scripting"]); + } + + /// A search that matches nothing explains itself instead of returning the + /// bare `total: 0` that let a model tell the user no skill was installed + /// for the job, and says how many skills the conversation could have + /// matched. The listing — no query, or one with no word in it — is + /// untouched: no terms, no matchedTerms, no guidance. + #[tokio::test] + async fn an_installed_skill_search_that_matches_nothing_explains_itself() { + let (_root, client) = client_over_installed(F5_INSTALLED); + + let none = search_installed(&client, serde_json::json!({ "query": "zzqx" })).await; + assert_eq!(none["total"], 0); + assert_eq!(none["terms"], serde_json::json!(["zzqx"])); + let guidance = none["guidance"] + .as_str() + .unwrap_or_else(|| panic!("an empty result explains itself: {none:#}")); + assert!( + guidance.contains("`zzqx`") + && guidance.contains("5 skills are enabled in this conversation") + && guidance.contains("searchSkills with no query"), + "{guidance}" + ); + + for arguments in [ + serde_json::json!({}), + serde_json::json!({ "query": " " }), + serde_json::json!({ "query": " - " }), + ] { + let listed = search_installed(&client, arguments.clone()).await; + assert_eq!(listed["total"], 5, "{arguments}: {listed:#}"); + assert!(listed.get("terms").is_none(), "{listed:#}"); + assert!(listed.get("guidance").is_none(), "{listed:#}"); + assert!( + listed["skills"] + .as_array() + .unwrap() + .iter() + .all(|row| row.get("matchedTerms").is_none()), + "{listed:#}" + ); + } + } + + /// The conversation's own switches run BEFORE the ranking: a skill switched + /// off here is neither returned nor counted, however well it matches. + #[tokio::test] + async fn an_installed_skill_search_ranks_only_what_the_conversation_has_enabled() { + let (_root, client) = client_over_installed(F5_INSTALLED); + let over = crate::agents::session_skills::SessionSkillOverride { + add: Vec::new(), + remove: vec!["ggplot".to_string()], + }; + + let page = search_installed_with(&client, "R scripting ggplot visualization", &over).await; + assert_eq!( + page_names(&page), + ["r-scripting", "python-scripting"], + "{page:#}" + ); + + let none = search_installed_with(&client, "zzqx", &over).await; + let guidance = none["guidance"].as_str().unwrap_or_default(); + assert!( + guidance.contains("4 skills are enabled in this conversation"), + "the switched-off skill is not counted either: {none:#}" + ); + } + // BR-71: `test_context()` builds a `SessionManager`, whose lazy sqlx pool // must be constructed inside a Tokio runtime, so this is now an async test. #[tokio::test] @@ -4442,6 +4717,29 @@ Working dir biorouter content serde_json::json!(false), "removeSkillPackage only deletes under the install root: {supplied:#}" ); + + // A query changes which rows come back and in what order, never what a + // row says about removal. (`my` is filler, so the one term is `package`.) + let searched = client + .handle_search_skills( + serde_json::json!({ "query": "my-package" }) + .as_object() + .cloned(), + &crate::agents::session_skills::SessionSkillOverride::default(), + ) + .await + .unwrap(); + let searched: serde_json::Value = + serde_json::from_str(&searched[0].as_text().unwrap().text).unwrap(); + assert_eq!(searched["total"], 1, "{searched:#}"); + let ranked = &searched["skills"][0]; + assert_eq!(ranked["removable"], serde_json::json!(true), "{ranked:#}"); + assert_eq!( + ranked["removalTarget"], + serde_json::json!("my-package"), + "{ranked:#}" + ); + assert_eq!(ranked["matchedTerms"], serde_json::json!(["package"])); } /// A bundle member's removal target is the BUNDLE's directory: a package is From 4de29d1a23df05b1abd4b5a739fb00d630e27837 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 14:15:08 -0700 Subject: [PATCH 3/4] fix(search): the query as written counts only as whole words The matcher ranks an entry that holds the whole query first, and it tested that with a plain substring check. That let a query which is itself one short term back in through every word containing it, undoing the rule that a term under three characters matches whole words only. Measured once the installed-skill search moved onto the matcher: searchSkills {query: "R"} -> all 5 fixture skills; python-scripting, rna-qc and variant-calling came back with `matchedTerms: []`, found by the letter r alone In the matcher's own fixtures `rank("R")` returned complex-plots (through "Draws") and prose-only as well. The same check ranked noise first inside a phrase: for `R scripting`, an entry saying "snippets for scripting" outranked one that said both words, because "fo[r scripting]" contains the query. The whole query now counts as written only where it starts and ends at a word boundary. Term matching is untouched, so every entry a word of the query matches is still returned; what goes is an entry found by nothing but a fragment of a word, and the rank such a fragment bought. The two queries above now return r-scripting then ggplot, and put the entry matching both words first. Every marketplace and extension search test passes unchanged. `a_verbatim_occurrence_is_still_a_hit` asserted the leak as a feature (`dy` found r-scripting inside "Tidyverse"). It is replaced by a test that the query as written still ranks first, but only as whole words. --- .../biorouter/src/agents/skills_extension.rs | 17 ++ crates/biorouter/src/catalog_search.rs | 184 +++++++++++++++--- 2 files changed, 171 insertions(+), 30 deletions(-) diff --git a/crates/biorouter/src/agents/skills_extension.rs b/crates/biorouter/src/agents/skills_extension.rs index 1aabb8487..5501d24c4 100644 --- a/crates/biorouter/src/agents/skills_extension.rs +++ b/crates/biorouter/src/agents/skills_extension.rs @@ -4162,6 +4162,23 @@ Content assert_eq!(page_names(&second), ["r-scripting"]); } + /// `r` has to mean the R language, and as a substring it is in nearly every + /// word. Two measurements against these fixtures, both returning all five + /// skills: the AND-of-substrings search, and then the shared matcher + /// itself, whose whole-query check was a substring test — the three extra + /// rows came back with `matchedTerms: []`, found by the letter alone. + #[tokio::test] + async fn a_one_letter_installed_skill_query_matches_whole_words_only() { + let (_root, client) = client_over_installed(F5_INSTALLED); + + let page = search_installed(&client, serde_json::json!({ "query": "R" })).await; + assert_eq!( + page_names(&page), + ["r-scripting", "ggplot"], + "the name says R, then the description does; nothing else says R: {page:#}" + ); + } + /// A search that matches nothing explains itself instead of returning the /// bare `total: 0` that let a model tell the user no skill was installed /// for the job, and says how many skills the conversation could have diff --git a/crates/biorouter/src/catalog_search.rs b/crates/biorouter/src/catalog_search.rs index 288877a22..cfaf4ef11 100644 --- a/crates/biorouter/src/catalog_search.rs +++ b/crates/biorouter/src/catalog_search.rs @@ -1,8 +1,9 @@ //! Free-text search over a catalog of named entries — the ONE matcher behind -//! `skills__searchMarketplaceSkills` and -//! `extensionmanager__search_marketplace_extensions`. A new catalog search -//! should call [`rank`] with its own fields rather than grow a matcher of its -//! own: every copy of this logic so far has drifted into the failure below. +//! `skills__searchMarketplaceSkills`, +//! `extensionmanager__search_marketplace_extensions` and the installed-skill +//! `skills__searchSkills`. A new catalog search should call [`rank`] with its +//! own fields rather than grow a matcher of its own: every copy of this logic +//! so far has drifted into the failure below. //! //! ⚠ **A query is a set of words, not a substring.** The matcher this replaced //! asked whether the WHOLE lowercased query occurred inside a single field, so a @@ -11,27 +12,37 @@ //! `R scripting ggplot visualization` → `total: 0`, while `ggplot` → 2 and //! `r-scripting` → 1. A model composes exactly that phrase on a user's behalf, //! so the shape that failed was the common one, and the model went on to tell -//! the user the marketplace had nothing. +//! the user the marketplace had nothing. The installed-skill search failed the +//! same phrase through code of its own — it kept a skill only when EVERY word +//! was in it — and answered `total: 0` with a ggplot skill and an R-scripting +//! skill installed. //! //! So a query is split into terms and an entry is a hit when it matches ANY of //! them. The union is deliberate: no single entry has to contain every word a //! user happened to say, and an AND over a phrase is the same empty answer with //! a different cause. Precision comes from the ranking instead, best first: //! -//! 1. an entry containing the query **verbatim** — what the old matcher found, -//! so nothing it returned is lost; +//! 1. an entry holding the query **as written** — its words, in that order, as +//! whole words — which no scatter of the same words outranks; //! 2. then by **how many terms** it matched, so an entry matching every term //! precedes one matching some; //! 3. then by **where** each term matched — the id or name outweighs a tag, //! which outweighs the description — and how exactly (the whole word, the //! start of one, or inside one); -//! 4. then registry order, which is by id, so a result never reshuffles. +//! 4. then the order the entries were given in — the registry's is by id, the +//! installed skills' by name — so a result never reshuffles. //! //! Two rules keep the union from drowning the useful hits, and both were needed //! by the measured query itself: //! //! * **A term under three characters matches whole words only.** `r` has to -//! find the R language; as a substring it matched nearly every entry. +//! find the R language; as a substring it matched nearly every entry. The +//! query as written is held to the same edges — it counts only where it +//! starts and ends at a word boundary. Tested as a plain substring it let a +//! query that IS one short term back in through every word containing it: +//! `R` alone still returned all five fixture skills of the installed-skill +//! search, three of them with no matched term at all, and `R scripting` +//! ranked "for scripting" above an entry that said both words. //! * **Filler words are dropped** ("a skill about R" is `r`), because in a union //! a word like `for` or `about` inflates the term count of every entry whose //! prose happens to use it, which ranked noise above the real hit. @@ -41,9 +52,9 @@ pub(crate) enum Weight { /// Free prose: a description. Prose = 1, - /// Curated labels: tags, keywords, a category, an organization. + /// Curated labels: tags, keywords, a category, an organization, a bundle. Label = 2, - /// What the entry is called: its registry id and names. + /// What the entry is called: its id and names. Name = 3, } @@ -111,13 +122,15 @@ const MIN_PARTIAL_CHARS: usize = 3; #[derive(Debug)] pub struct CatalogSearchHit<'a, T> { pub entry: &'a T, - /// The terms this entry matched, in query order. Empty only for an entry - /// that nothing but the verbatim query found. + /// The terms this entry matched, in query order. Empty only when the query + /// holds no word at all (`++`), so that nothing but the query as written + /// could have found the entry. pub matched_terms: Vec, } /// A ranked search: the terms the query was split into, and every entry that -/// matched at least one of them (or the whole query verbatim), best first. +/// matched at least one of them (or holds the whole query as written), best +/// first. #[derive(Debug)] pub struct CatalogSearch<'a, T> { /// What the query was read as, after filler words were dropped. Reported @@ -214,11 +227,45 @@ fn term_strength(term: &str, word: &str) -> u32 { } } +/// Does `text` hold `phrase` as written — starting and ending at a word +/// boundary, not inside a longer word? `r scripting` is in "R scripting" but +/// not in "for scripting", where its `r` is the tail of `for`. Both are +/// lowercase already. +/// +/// An edge of `phrase` that is not a letter or digit needs no boundary: it is +/// one, so `++` is written in "c++". +/// +/// Every character position is tried, not only the occurrences `find` would +/// step through, because a refused occurrence can overlap an accepted one: +/// `a a` in "ba a a" is written only from the second `a`. +fn written_in(text: &str, phrase: &str) -> bool { + let starts_word = phrase.chars().next().is_some_and(char::is_alphanumeric); + let ends_word = phrase + .chars() + .next_back() + .is_some_and(char::is_alphanumeric); + let mut before = None; + for (start, current) in text.char_indices() { + if let Some(rest) = text.get(start..).filter(|rest| rest.starts_with(phrase)) { + let after = rest + .get(phrase.len()..) + .and_then(|tail| tail.chars().next()); + let opens = !starts_word || !before.is_some_and(char::is_alphanumeric); + let closes = !ends_word || !after.is_some_and(char::is_alphanumeric); + if opens && closes { + return true; + } + } + before = Some(current); + } + false +} + /// Rank `entries` against `query`. `fields` names the text of one entry that /// is searched, and how much a match there counts. /// -/// An empty (or all-whitespace) query is the browse case: every entry, in -/// registry order. +/// An empty (or all-whitespace) query is the browse case: every entry, in the +/// order given. pub(crate) fn rank<'a, T>( query: &str, noise: &[&str], @@ -243,9 +290,9 @@ pub(crate) fn rank<'a, T>( let mut ranked = Vec::new(); for entry in entries { let fields = fields(entry); - let verbatim = fields + let written = fields .iter() - .any(|(text, _)| text.to_lowercase().contains(&phrase)); + .any(|(text, _)| written_in(&text.to_lowercase(), &phrase)); let entry_words: Vec<(String, u32)> = fields .iter() .flat_map(|(text, weight)| words(text).map(move |word| (word, *weight as u32))) @@ -264,9 +311,9 @@ pub(crate) fn rank<'a, T>( score += best; } } - if verbatim || !matched_terms.is_empty() { + if written || !matched_terms.is_empty() { ranked.push(( - (verbatim, matched_terms.len(), score), + (written, matched_terms.len(), score), CatalogSearchHit { entry, matched_terms, @@ -274,7 +321,7 @@ pub(crate) fn rank<'a, T>( )); } } - // Stable, and descending on the key: equal ranks keep registry order. + // Stable, and descending on the key: equal ranks keep the order given. ranked.sort_by(|(left, _), (right, _)| right.cmp(left)); CatalogSearch { terms, @@ -377,6 +424,44 @@ mod tests { ); } + /// The same rule for a query that IS one short term. The whole-query check + /// used to be a plain substring test, so `r` alone found every entry with + /// the letter anywhere — `complex-plots` through "Draws", `prose-only` + /// through its own name — and the rule above held only inside a longer + /// phrase. + #[test] + fn a_one_letter_query_matches_whole_words_only() { + let search = rank("R", &[], ENTRIES, fields); + assert_eq!(ids(&search), ["r-scripting"]); + assert_eq!(search.hits[0].matched_terms, ["r"]); + } + + /// The query as written outranks any count of separate words, so it has to + /// be written there: `r scripting` inside "for scripting" is the tail of + /// `for` and then a word. Read as a substring it ranked an entry matching + /// one of the two words above one matching both. + #[test] + fn a_phrase_found_only_inside_other_words_is_not_the_query_as_written() { + let entries = [ + Entry { + id: "shell-snippets", + name: "Shell Snippets", + description: "Snippets for scripting the shell.", + tags: &[], + }, + Entry { + id: "tidy-style", + name: "Tidy Style", + description: "Scripting conventions for R.", + tags: &["R"], + }, + ]; + let search = rank("R scripting", &[], &entries, fields); + assert_eq!(ids(&search), ["tidy-style", "shell-snippets"]); + assert_eq!(search.hits[0].matched_terms, ["r", "scripting"]); + assert_eq!(search.hits[1].matched_terms, ["scripting"]); + } + #[test] fn a_long_term_matches_inside_a_word_and_a_plural_finds_its_singular() { assert_eq!( @@ -413,17 +498,56 @@ mod tests { ); } - /// Everything the substring matcher found is still found: a query that - /// occurs verbatim in a field is a hit even when its terms are too short - /// to match on their own. #[test] - fn a_verbatim_occurrence_is_still_a_hit() { - let search = rank("s p", &[], ENTRIES, fields); - assert!(search.is_empty(), "neither `s` nor `p` is a whole word"); + fn a_phrase_is_written_only_between_word_boundaries() { + assert!(written_in("r scripting", "r scripting")); + assert!(written_in("tidy code for r.", "r"), "the second `r`"); + assert!(!written_in("snippets for scripting", "r scripting")); + assert!(!written_in("tidyverse", "dy")); + assert!( + written_in("ba a a", "a a"), + "written at the second `a`, which overlaps the refused first occurrence" + ); + assert!( + written_in("c++ code", "++"), + "an edge that is not a letter or digit is a boundary itself" + ); + } - let search = rank("dy", &[], ENTRIES, fields); - assert_eq!(ids(&search), ["r-scripting"], "`dy` inside `Tidyverse`"); - assert!(search.hits[0].matched_terms.is_empty()); + /// The query as written — its words, in order, as words — outranks any + /// scatter of the same words, even one in a weightier field. A fragment of + /// a word is not the query as written, though: `dy` inside `Tidyverse` is + /// exactly the substring the short-term rule refuses. + #[test] + fn the_query_as_written_ranks_first_but_only_as_whole_words() { + let entries = [ + Entry { + id: "code-tidy", + name: "Code Tidy", + description: "Formatting rules.", + tags: &[], + }, + Entry { + id: "styler", + name: "Styler", + description: "Writes tidy code.", + tags: &[], + }, + ]; + assert_eq!( + ids(&rank("tidy code", &[], &entries, fields)), + ["styler", "code-tidy"], + "both hold both words, in the name or the prose; only one says `tidy code`" + ); + + assert!( + rank("dy", &[], ENTRIES, fields).is_empty(), + "`dy` is inside `Tidyverse`, not a word of it" + ); + assert!( + rank("s p", &[], ENTRIES, fields).is_empty(), + "neither `s` nor `p` is a whole word" + ); } #[test] From cf7fadb331d1770e5ebad06743a66afb5479dc06 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 14:18:52 -0700 Subject: [PATCH 4/4] docs(skills): how an installed-skill query is matched The skill-catalog reference described what a searchSkills row carries but not how a query picks the rows. It now says: ranked by any word through the shared catalog_search matcher, short words whole-word only, the conversation's switches first, and the terms/matchedTerms/guidance fields a ranked page adds, with the F5 measurement that motivated it. --- docs/extensions/skill-catalog.md | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/docs/extensions/skill-catalog.md b/docs/extensions/skill-catalog.md index ebc1d43ea..526aefcf2 100644 --- a/docs/extensions/skill-catalog.md +++ b/docs/extensions/skill-catalog.md @@ -223,6 +223,38 @@ unchanged: a batch with any bad name still removes nothing. that is the only root `removeSkillPackage` deletes under. An extension's skill is uninstalled by removing the extension. +## How a query is matched + +`skills__searchSkills` with no `query` pages the catalog alphabetically. With a +query it **ranks** the skills this conversation has enabled, through +`catalog_search::rank` — the same matcher behind `searchMarketplaceSkills` and +`search_marketplace_extensions`, whose module doc is the specification: + +- A skill is returned when it matches **any** word of the query, not all of + them. Skills matching the most words come first; ties go to where a word + matched — the skill's name, then its bundle's name, then its description — + and then to alphabetical order. A skill holding the whole query as written + outranks all of those. +- A word under three characters matches whole words only, so `r` finds the R + language rather than every word with an r in it. Filler (`for`, `about`, + `skill`) is dropped, and a plural falls back to its singular. +- The conversation's switches apply **first**: a skill turned off here is never + scored, returned or counted. + +A ranked page is the listing's page plus three fields. `terms` is what the query +was read as, each row adds `matchedTerms`, and a search that matches nothing adds +`guidance` — how many skills are enabled in this conversation, and to try a +shorter term or list them all. A query with no letter or digit in it is the +listing. + +⚠ **The search used to keep a skill only when it contained every word of the +query.** With a ggplot skill and an R-scripting skill installed, +`R scripting ggplot visualization` returned `total: 0` — QA finding F5, which the +marketplace search had too, through different code. It now returns the ggplot +skill (three of the four words), the R-scripting skill (two), then any skill +matching one. Add a catalog search by calling `rank` with that catalog's fields, +never by writing another matcher. + ## Debugging a skill that is installed but not usable 1. `biorouter skill list` — if it is absent, discovery never saw it. Check the