From 6531158d9f48079bc3574213d54f061343e4b59d Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 19 Sep 2026 17:25:20 -0700 Subject: [PATCH] fix(agent): query expansion panicked on non-ASCII and rewrote text inside words MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Probing what QueryExpander::expand actually produces for LongMemEval questions turned up two defects in the same helper. `replace_word_case_insensitive` did a plain substring replace despite its name, so acronym expansion fired inside ordinary words: "training" became "trArtificial Intelligencening" ("ai") and "programming" became "Pull Requestogramming" ("pr"). Nearly every acronym expansion of prose was corrupt. Matching now requires word boundaries at both ends; real acronyms (API, database) still expand in both directions. The same helper searched `text.to_lowercase()` and then sliced `text` with the offsets it found. That holds only while lowercasing preserves byte length, and it does not — Turkish 'İ' is 2 bytes and lowercases to 3. Offsets after such a character drifted, so output was silently corrupted ("İstanbul AI trip" lost a character) or the slice landed inside a character or past the end and panicked: `expand("İ AI")` was enough, from a plain query string. Matching now walks the original string, comparing case-insensitively char by char, so offsets are always valid. Regression tests cover both, plus whole-word matching at string edges. The morphological rules remain crude ("during" -> "dured"); that is a quality limit, not a correctness bug, and is now documented as a reason to measure before enabling expansion on a retrieval path. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 13 ++ crates/clawhdf5-agent/src/query_expand.rs | 170 +++++++++++++++++++++- 2 files changed, 175 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 002bd94..5076f3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,19 @@ ## Unreleased ### Retrieval quality +- `clawhdf5-agent`: **`QueryExpander::expand` panicked on ordinary non-ASCII + input** — `"İ AI"` was enough. It searched a lowercased copy of the query and + then sliced the *original* with those offsets, which only works while + lowercasing preserves byte length (Turkish `İ` is 2 bytes and lowercases to + 3). Depending on where the offsets drifted it either corrupted the output + ("İstanbul AI trip" lost a character) or panicked. Matching now walks the + original string. +- `clawhdf5-agent`: query expansion no longer rewrites text inside words. + `replace_word_case_insensitive` did a plain substring replace despite its + name, so "training" became "trArtificial Intelligencening" and "programming" + became "Pull Requestogramming" — every acronym expansion of ordinary prose + was corrupt. Matches now require word boundaries; genuine acronyms + (`API`, `database`) still expand. - `clawhdf5-agent`: **the default fusion weights are now the measured ones.** A sweep of every 0.1 step over the full LongMemEval haystack (500 questions, real MiniLM embeddings) shows the long-standing `0.7/0.3` default is diff --git a/crates/clawhdf5-agent/src/query_expand.rs b/crates/clawhdf5-agent/src/query_expand.rs index 64e5489..eb5c739 100644 --- a/crates/clawhdf5-agent/src/query_expand.rs +++ b/crates/clawhdf5-agent/src/query_expand.rs @@ -6,6 +6,11 @@ //! - Temporal expansion (time-related rewrites) //! - Morphological variants (stemming-like transforms) //! - Knowledge graph expansion (entity aliases and neighbors) +//! +//! The morphological rules are crude suffix swaps, so some variants are not +//! words ("during" -> "dured"). That is tolerable for a BM25 stage, which +//! simply finds no postings for a nonsense term, but it means expansion is not +//! free: measure before enabling it on a retrieval path. use crate::knowledge::KnowledgeCache; @@ -340,20 +345,87 @@ fn contains_phrase(text: &str, phrase: &str) -> bool { /// Replace a phrase in `text` case-insensitively, preserving surrounding case. fn replace_word_case_insensitive(text: &str, from: &str, to: &str) -> String { - case_insensitive_replace(text, from, to) + replace_first(text, from, to, MatchKind::WholeWord) } fn case_insensitive_replace(text: &str, from: &str, to: &str) -> String { - let lower = text.to_lowercase(); - let lower_from = from.to_lowercase(); - if let Some(pos) = lower.find(&lower_from) { - let end = pos + from.len(); - format!("{}{}{}", &text[..pos], to, &text[end..]) - } else { - text.to_string() + replace_first(text, from, to, MatchKind::Substring) +} + +/// Whether a match may fall inside a larger word. +#[derive(Clone, Copy, PartialEq)] +enum MatchKind { + /// Match anywhere, including inside another word. + Substring, + /// Match only when both ends sit on a word boundary. + WholeWord, +} + +/// Replace the first case-insensitive match of `from` in `text` with `to`. +/// +/// Matching walks the *original* string rather than a lowercased copy. The +/// previous implementation searched `text.to_lowercase()` and then sliced +/// `text` with the offsets it found, which only holds while lowercasing +/// preserves byte length. It does not: Turkish `İ` (2 bytes) lowercases to +/// `i` + U+0307 (3 bytes), so every later offset was wrong — silently +/// corrupting the output, or panicking when an offset landed inside a +/// character or past the end. `"İ AI"` was enough to panic. +fn replace_first(text: &str, from: &str, to: &str, kind: MatchKind) -> String { + match find_case_insensitive(text, from, kind) { + Some((start, end)) => { + let mut out = String::with_capacity(text.len() - (end - start) + to.len()); + out.push_str(&text[..start]); + out.push_str(to); + out.push_str(&text[end..]); + out + } + None => text.to_string(), } } +/// Byte range of the first case-insensitive match of `needle` in `haystack`. +fn find_case_insensitive(haystack: &str, needle: &str, kind: MatchKind) -> Option<(usize, usize)> { + if needle.is_empty() { + return None; + } + let lowered: Vec = needle.chars().flat_map(char::to_lowercase).collect(); + let is_word = |c: char| c.is_alphanumeric() || c == '_'; + + for (start, _) in haystack.char_indices() { + if kind == MatchKind::WholeWord + && haystack[..start].chars().next_back().is_some_and(is_word) + { + continue; // mid-word: "ai" inside "training" + } + let mut matched = 0usize; + let mut end = start; + for (offset, ch) in haystack[start..].char_indices() { + if matched == lowered.len() { + break; + } + let mut consumed_all = true; + for lc in ch.to_lowercase() { + if lowered.get(matched) != Some(&lc) { + consumed_all = false; + break; + } + matched += 1; + } + if !consumed_all { + break; + } + end = start + offset + ch.len_utf8(); + } + if matched == lowered.len() + && !(kind == MatchKind::WholeWord + && haystack[end..].chars().next().is_some_and(is_word)) + { + return Some((start, end)); + } + } + None +} + /// Simple whitespace/punctuation tokenizer. fn tokenize(text: &str) -> Vec { text.split(|c: char| !c.is_alphanumeric()) @@ -637,4 +709,86 @@ mod tests { expanded.iter().map(|x| &x.text).collect::>() ); } + #[test] + fn acronyms_only_match_whole_words() { + let ex = QueryExpander::new(QueryExpansionConfig::default()); + // "training" contains "ai", "programming" contains "pr". These used to + // be rewritten to "trArtificial Intelligencening" and + // "Pull Requestogramming". + for query in [ + "How many miles during my marathon training?", + "Which programming language did I pick?", + "I updated the maintainer list", + ] { + for expansion in ex.expand(query) { + assert!( + expansion.expansion_type != "acronym", + "{query:?} produced {expansion:?}" + ); + } + } + // A real acronym still expands, in both directions. + let texts: Vec = ex + .expand("What about the API and the database?") + .into_iter() + .filter(|e| e.expansion_type == "acronym") + .map(|e| e.text) + .collect(); + assert!( + texts + .iter() + .any(|t| t.contains("Application Programming Interface")), + "{texts:?}" + ); + assert!(texts.iter().any(|t| t.contains("DB")), "{texts:?}"); + } + + #[test] + fn non_ascii_queries_do_not_panic_or_corrupt() { + let ex = QueryExpander::new(QueryExpansionConfig::default()); + // Turkish 'İ' is 2 bytes but lowercases to 3, so offsets taken from a + // lowercased copy no longer line up with the original. `"İ AI"` used + // to panic; `"İstanbul AI trip"` used to silently eat a character. + for query in ["İ AI", "İé AI", "İİ ML", "İstanbul AI trip", "ǰ ML notes"] { + for expansion in ex.expand(query) { + assert!( + expansion.text.contains('İ') || expansion.text.contains('ǰ'), + "{query:?} lost its leading character: {expansion:?}" + ); + } + } + let expanded = ex.expand("İstanbul AI trip"); + assert!( + expanded + .iter() + .any(|e| e.text == "İstanbul Artificial Intelligence trip"), + "{expanded:?}" + ); + } + + #[test] + fn whole_word_matching_handles_string_edges_and_case() { + assert_eq!( + replace_word_case_insensitive("ai tools", "AI", "Artificial Intelligence"), + "Artificial Intelligence tools" + ); + assert_eq!( + replace_word_case_insensitive("tools for ai", "AI", "Artificial Intelligence"), + "tools for Artificial Intelligence" + ); + assert_eq!( + replace_word_case_insensitive("the aim", "AI", "Artificial Intelligence"), + "the aim", + "must not match inside a word" + ); + assert_eq!( + replace_word_case_insensitive("no match here", "xyz", "abc"), + "no match here" + ); + // Only the first occurrence is replaced, as before. + assert_eq!( + replace_word_case_insensitive("ai and ai", "ai", "ML"), + "ML and ai" + ); + } }