From 383bb8e0e9b68780e6ae53470a2c1bdb2a19569a Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 15 Oct 2026 12:53:09 +0100 Subject: [PATCH] fix: deduplicate MCP autocomplete before limits (Refs #69) --- crates/terraphim_mcp_server/src/lib.rs | 84 +++++----- .../tests/test_mcp_role_contract.rs | 144 ++++++++++++++++++ 2 files changed, 194 insertions(+), 34 deletions(-) diff --git a/crates/terraphim_mcp_server/src/lib.rs b/crates/terraphim_mcp_server/src/lib.rs index 3a526ee..c1fc308 100644 --- a/crates/terraphim_mcp_server/src/lib.rs +++ b/crates/terraphim_mcp_server/src/lib.rs @@ -47,6 +47,43 @@ pub mod resource_mapper; use crate::resource_mapper::TerraphimResourceMapper; +/// Merge prefix and fuzzy suggestions without letting repeated terms consume slots. +/// +/// The fuzzy search already includes prefix matches. Keep distinct authored aliases +/// of one concept, but accept each normalised displayed term/concept pair only once. +fn prefix_fuzzy_suggestions( + index: &AutocompleteIndex, + query: &str, + limit: usize, +) -> Vec { + let mut results = Vec::new(); + let mut seen = std::collections::HashSet::new(); + if limit == 0 { + return results; + } + if let Ok(prefix) = terraphim_automata::autocomplete_search(index, query, Some(limit)) { + for result in prefix { + if seen.insert((NormalizedTermValue::from(result.term.as_str()), result.id)) { + results.push(result); + } + } + } + if results.len() < limit + && let Ok(fuzzy) = + terraphim_automata::fuzzy_autocomplete_search(index, query, 0.6, Some(limit)) + { + for result in fuzzy { + if seen.insert((NormalizedTermValue::from(result.term.as_str()), result.id)) { + results.push(result); + if results.len() == limit { + break; + } + } + } + } + results +} + /// Find the terraphim_rlm binary in common locations. fn find_terraphim_rlm_binary() -> std::path::PathBuf { // Check ~/.cargo/bin first (typical cargo install location) @@ -870,31 +907,23 @@ impl McpService { Err(result) => return Ok(result), }; let max_results = limit.unwrap_or(10); - let mut combined: Vec = Vec::new(); - // Prefer exact/prefix first - if let Ok(mut exact) = - terraphim_automata::autocomplete_search(&index, &query, Some(max_results)) - { - combined.append(&mut exact); - } - // Fill remaining with fuzzy - if combined.len() < max_results { - let remaining = max_results - combined.len(); - if let Ok(mut fuzzy) = - terraphim_automata::fuzzy_autocomplete_search(&index, &query, 0.6, Some(remaining)) - { - combined.append(&mut fuzzy); - } - } + let mut combined = prefix_fuzzy_suggestions(&index, &query, max_results); // Graph embeddings-style expansion: include additional synonyms sharing the same concept id if combined.len() < max_results { use std::collections::HashSet; - let mut seen_terms: HashSet = combined.iter().map(|r| r.term.clone()).collect(); + let mut seen_terms: HashSet<_> = combined + .iter() + .map(|r| (NormalizedTermValue::from(r.term.as_str()), r.id)) + .collect(); let concept_ids: HashSet = combined.iter().map(|r| r.id).collect(); let mut candidates: Vec = Vec::new(); // Iterate over all metadata and pick terms with same concept id - for (term, meta) in terraphim_automata::autocomplete_helpers::iter_metadata(&index) { - if concept_ids.contains(&meta.id) && !seen_terms.contains(term) { + for (_, meta) in terraphim_automata::autocomplete_helpers::iter_metadata(&index) { + let identity = ( + NormalizedTermValue::from(meta.original_term.as_str()), + meta.id, + ); + if concept_ids.contains(&meta.id) && !seen_terms.contains(&identity) { candidates.push(AutocompleteResult { term: meta.original_term.clone(), normalized_term: meta.normalized_term.clone(), @@ -915,7 +944,7 @@ impl McpService { if combined.len() >= max_results { break; } - if seen_terms.insert(cand.term.clone()) { + if seen_terms.insert((NormalizedTermValue::from(cand.term.as_str()), cand.id)) { combined.push(cand); } } @@ -944,20 +973,7 @@ impl McpService { Err(result) => return Ok(result), }; let max_results = limit.unwrap_or(10); - let mut results: Vec = Vec::new(); - if let Ok(mut prefix) = - terraphim_automata::autocomplete_search(&index, &query, Some(max_results)) - { - results.append(&mut prefix); - } - if results.len() < max_results { - let remaining = max_results - results.len(); - if let Ok(mut fuzzy) = - terraphim_automata::fuzzy_autocomplete_search(&index, &query, 0.6, Some(remaining)) - { - results.append(&mut fuzzy); - } - } + let results = prefix_fuzzy_suggestions(&index, &query, max_results); // Build snippets by searching for each term to fetch a related document and use its stub/description/body excerpt let mut service = self diff --git a/crates/terraphim_mcp_server/tests/test_mcp_role_contract.rs b/crates/terraphim_mcp_server/tests/test_mcp_role_contract.rs index e84c247..1039770 100644 --- a/crates/terraphim_mcp_server/tests/test_mcp_role_contract.rs +++ b/crates/terraphim_mcp_server/tests/test_mcp_role_contract.rs @@ -17,6 +17,9 @@ struct Fixture { } impl Fixture { async fn start() -> Result { + Self::with_fuzzy_candidate(false).await + } + async fn with_fuzzy_candidate(include_icon: bool) -> Result { let root = tempfile::tempdir()?; let project = root.path().join("project with spaces"); let settings = root.path().join("settings"); @@ -48,6 +51,13 @@ impl Fixture { format!("{canonical} {suffix}\n"), )?; } + if include_icon && name == "project" { + fs::write(concepts.join("Icon.md"), "# Icon\nsynonyms:: Pictogram\n")?; + fs::write( + corpus.join("icon.md"), + "Icon identifies the review action.\n", + )?; + } roles.insert(name.into(), json!({"name":name,"relevance_function":"terraphim-graph","terraphim_it":false,"theme":"lumen","kg":{"automata_path":null,"knowledge_graph_local":{"input_type":"markdown","path":concepts},"public":false,"publish":false},"haystacks":[{"location":corpus,"service":"Ripgrep","read_only":true,"extra_parameters":{}}],"extra":{}})); } // Replace the profile's remote-backed defaults with local, empty roles. @@ -552,3 +562,137 @@ async fn fuzzy_autocomplete_returns_authored_terms_from_the_real_index() -> Resu f.client.cancel().await?; Ok(()) } + +async fn check_unique_aliases_after_rebuild(tool: &str) -> Result<()> { + let f = Fixture::start().await?; + for (cycle, role) in ["project", "garden", "project"].iter().enumerate() { + f.call( + "build_autocomplete_index", + json!({"role":role}), + &format!("dedup-build-{tool}-{cycle}"), + ) + .await?; + let query = if *role == "project" { "Con" } else { "Flo" }; + let alias = if *role == "project" { + "concern" + } else { + "flower" + }; + let canonical = if *role == "project" { "risk" } else { "orchid" }; + for limit in [0, 1, 2, 5] { + let result = f + .call( + tool, + json!({"query":query,"role":role,"limit":limit}), + &format!("dedup-{cycle}-{tool}-{limit}"), + ) + .await?; + assert_ne!(result["isError"], true, "{result}"); + let items: Vec<_> = result["content"] + .as_array() + .unwrap() + .iter() + .skip(1) + .map(|x| x["text"].as_str().unwrap()) + .collect(); + let unique: std::collections::HashSet<_> = items.iter().copied().collect(); + assert_eq!(unique.len(), items.len(), "duplicate suggestions: {result}"); + let count = if tool == "autocomplete_terms" { + limit.min(2) + } else { + limit.min(1) + }; + assert_eq!( + items.len(), + count, + "distinct aliases must fill their available slots: {result}" + ); + assert_eq!( + result["content"][0]["text"], + format!("Found {count} suggestions") + ); + if limit > 0 { + assert!(items[0].starts_with(alias), "{result}"); + } + if tool == "autocomplete_terms" && limit >= 2 { + assert!( + items.contains(&canonical), + "same-concept alias must remain distinct: {result}" + ); + } + } + } + f.client.cancel().await?; + Ok(()) +} + +async fn check_distinct_fuzzy_slot(tool: &str) -> Result<()> { + let f = Fixture::with_fuzzy_candidate(true).await?; + f.call( + "build_autocomplete_index", + json!({"role":"project"}), + &format!("dedup-fuzzy-build-{tool}"), + ) + .await?; + // The real fuzzy index must contain the distinct candidate before checking + // that the combined autocomplete endpoints preserve its available slot. + let fuzzy = f + .call( + "fuzzy_autocomplete_search", + json!({"query":"con","similarity":0.6,"limit":2}), + &format!("dedup-fuzzy-available-{tool}"), + ) + .await?; + assert!(texts(&fuzzy).contains("icon"), "{fuzzy}"); + let result = f + .call( + tool, + json!({"query":"con","role":"project","limit":2}), + &format!("dedup-fuzzy-{tool}"), + ) + .await?; + assert_ne!(result["isError"], true, "{result}"); + let items: Vec<_> = result["content"] + .as_array() + .unwrap() + .iter() + .skip(1) + .map(|x| x["text"].as_str().unwrap()) + .collect(); + assert_eq!(items.len(), 2, "{result}"); + assert!( + items[0].starts_with("concern"), + "prefix remains first: {result}" + ); + assert!( + items[1].starts_with("icon"), + "duplicate prefix must not consume the fuzzy slot: {result}" + ); + if tool == "autocomplete_with_snippets" { + assert!(items[0].contains("Risk requires"), "{result}"); + assert!( + items[1].contains("Icon identifies the review action."), + "{result}" + ); + } + assert_eq!(result["content"][0]["text"], "Found 2 suggestions"); + f.client.cancel().await?; + Ok(()) +} + +#[tokio::test] +async fn autocomplete_terms_deduplicates_authored_aliases_after_rebuild() -> Result<()> { + check_unique_aliases_after_rebuild("autocomplete_terms").await +} +#[tokio::test] +async fn autocomplete_snippets_deduplicates_authored_aliases_after_rebuild() -> Result<()> { + check_unique_aliases_after_rebuild("autocomplete_with_snippets").await +} +#[tokio::test] +async fn autocomplete_terms_deduplicates_before_limit() -> Result<()> { + check_distinct_fuzzy_slot("autocomplete_terms").await +} +#[tokio::test] +async fn autocomplete_snippets_deduplicates_before_limit() -> Result<()> { + check_distinct_fuzzy_slot("autocomplete_with_snippets").await +}