diff --git a/crates/biorouter/src/agents/skills_extension.rs b/crates/biorouter/src/agents/skills_extension.rs index 2ecd74f41..5501d24c4 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,247 @@ 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"]); + } + + /// `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 + /// 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 +4734,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 diff --git a/crates/biorouter/src/marketplace/search.rs b/crates/biorouter/src/catalog_search.rs similarity index 61% rename from crates/biorouter/src/marketplace/search.rs rename to crates/biorouter/src/catalog_search.rs index 52dacae22..cfaf4ef11 100644 --- a/crates/biorouter/src/marketplace/search.rs +++ b/crates/biorouter/src/catalog_search.rs @@ -1,6 +1,9 @@ -//! 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`, +//! `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 @@ -9,39 +12,49 @@ //! `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. /// 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. + /// 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, } @@ -97,34 +110,36 @@ 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. + /// 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 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 +175,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) { @@ -212,24 +227,58 @@ 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. -pub(super) fn rank<'a, T>( +/// 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], 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(), }) @@ -241,9 +290,9 @@ pub(super) 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))) @@ -262,19 +311,19 @@ pub(super) 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), - MarketplaceSearchHit { + (written, matched_terms.len(), score), + CatalogSearchHit { entry, matched_terms, }, )); } } - // 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)); - MarketplaceSearch { + CatalogSearch { terms, hits: ranked.into_iter().map(|(_, hit)| hit).collect(), } @@ -301,7 +350,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() } @@ -375,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!( @@ -411,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] 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() 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