Skip to content

fix(search): keep natural-language queries on the FTS5 path instead of crashing to the LIKE fallback - #159

Closed
100yenadmin wants to merge 1 commit into
mainfrom
upstream/pr-354
Closed

100yenadmin wants to merge 1 commit into
mainfrom
upstream/pr-354

Conversation

@100yenadmin

Copy link
Copy Markdown
Member

Important

This executable LCM-X PR was recreated by the migration operator from the exact upstream commit head. GitHub did not transfer the original PR actor, dates, review objects, or approval state.

Source and attribution

@coderabbitai ignore

Original commit authorship and history remain in the commits. Historical discussion and review text are imported below as attributed ordinary comments; they are not new approvals or change requests.

Original upstream PR description

Problem

Any conversational search query containing an apostrophe, comma, question mark, or most other punctuation (for example don't) is a syntax error to FTS5. store.search catches the error and silently degrades to the LIKE fallback, so in practice nearly every natural-language query runs on the slow, unranked fallback path instead of the FTS index.

Mechanism

sanitize_fts5_query (search_query.py:47) only replaces the fixed operator set _FTS5_SPECIAL_CHARS (search_query.py:36), leaving apostrophes, commas, and question marks in the unquoted query text. store.search (store.py:931) passes the result to messages_fts MATCH, FTS5 raises fts5: syntax error near "'", and the handler at store.py:997 logs "FTS message search failed, falling back to LIKE" and degrades. dag.py:427 has the same path for summary search.

Fix

Outside quoted phrases, keep only bareword-safe characters (alphanumerics, underscore, whitespace) and replace everything else with spaces. That is exactly the token alphabet the unicode61 tokenizer indexed, so the sanitized query matches the index instead of erroring; don't becomes don t, which matches rows containing don't. Quoted phrases pass through verbatim, as before.

The LIKE fallback must not use that strict sanitization: it matches raw stored content, so apostrophes, emoji, and CJK punctuation have to survive for the fallback to find them (an emoji-only query would otherwise produce zero terms and return nothing). The helper is therefore split in two: sanitize_fts5_query (strict, used by the FTS MATCH paths) and sanitize_like_terms_query (the previous operator-stripping behavior, used by _search_like in store.py and dag.py).

Verification

New tests cover: don't returning its row on the FTS path with no fallback warning, a full conversational sentence (what did I tell you, don't you remember?) searching without any OperationalError or fallback, quoted phrases with trailing conversational punctuation still matching as phrases, and sanitizer unit cases. On unpatched main the new tests fail with the exact production error (fts5: syntax error near ","). The full tests/test_lcm_core.py suite passes (288 tests), including the existing emoji and LIKE-fallback regression tests.

Incident evidence

One live store logged the FTS-to-LIKE fallback warning over 300 times, meaning nearly every conversational search had been silently running on the degraded fallback path.

…e LIKE fallback

sanitize_fts5_query only stripped a fixed set of FTS5 operator
characters, leaving apostrophes, commas, question marks and other
conversational punctuation in the unquoted query text. FTS5 parses
those as syntax errors ("fts5: syntax error near ..."), so store.search
silently degraded to the LIKE fallback for nearly any conversational
query, e.g. "don't". One production store logged the fallback warning
300+ times.

Fix: outside quoted phrases, keep only bareword-safe characters
(alphanumerics, underscore, whitespace). That is exactly the token
alphabet the unicode61 tokenizer indexed, so replacing everything else
with spaces matches the index instead of erroring. Quoted phrases pass
through verbatim, as before.

The LIKE fallback itself must NOT use that strict sanitization: it
matches raw stored content, so apostrophes, emoji and CJK punctuation
have to survive for the fallback to find them (emoji-only queries
would otherwise return nothing). Split the helper in two:
sanitize_fts5_query (strict, FTS MATCH paths) and
sanitize_like_terms_query (previous operator-stripping behavior, used
by the _search_like paths in store.py and dag.py).

Tests: new unit tests for the sanitizer plus store-level tests
asserting that "don't", a full conversational sentence, and a quoted
phrase with trailing punctuation all return results on the FTS path
with no "falling back to LIKE" warning. On unpatched code they fail
with the exact production symptom.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019uao4D1LKM35dKzivcsWVf
@100yenadmin 100yenadmin added active-continuation Active continuation of an attributed upstream item bug Something isn't working eva-direct Direct impact on Electric Sheep supported Hermes/LCM paths P2 Significant supported-path regression or bounded correctness failure upstream-evidence Preserves links and attribution to the upstream report or pull request upstream-pr Imported upstream pull-request evidence labels Aug 16, 2026
@100yenadmin

Copy link
Copy Markdown
Member Author

Triage: your red CI is stale, not broken — and here is the actual blocker.

This PR shows a failing check, but the failure is a CodeQL infrastructure error from 2026-08-16, not a problem with your code:

##[error]Encountered a fatal error ... CodeQL could not process any code written
in JavaScript/TypeScript. Exit code was 32

This is a Python repo, so that extractor found nothing to analyse and aborted. The job has passed on every PR landed since, so the configuration is already fixed and this PR is simply carrying an old red. It needs a re-run, not repair. Twelve of the sixteen "failing" PRs in this repo are in the same position — detail and recommendation in #241.

The real blocker is the rebase. This branch is ~40 commits behind and conflicts against current main. That is what needs doing before it can be evaluated on its merits.

Recorded disposition: NEEDS-REBASE. Not closed, not abandoned — it is a feature branch whose capability I verified is still absent from main. Priority order in this lane is external contributions, then correctness fixes, then reproducible issues, then feature drafts, so this sits behind the correctness work rather than being forgotten.

If you want it prioritised sooner, say so on the PR and I will pick it up next.

100yenadmin added a commit that referenced this pull request Aug 20, 2026
test: salvage regression coverage from #159 and #160

TEST-ONLY-SALVAGE (the #174 precedent): both source PRs' fixes are on main
independently; their coverage was not. Additive-only across two test files; no
existing assertion touched. Original authors credited per commit (milgauss /
upstream#354; @ai-ag2026 / upstream#359 with Co-authored-by).

Review-hardened before merge (9a59269): the FTS non-vacuity check done
manually during salvage is now a standing in-test assertion
(requires_like_fallback(q) is False on all three FTS-path tests), and the two
ambiguous-overlap tests assert the complete trailing payload in order plus
_ingest_cursor == len(delta) -- the reviewer's expected cursor value was
validated empirically against main before being committed as a fence.

Suites: 1099 passed, 1 skipped, 0 failed. All 13 required checks pass; both
review threads replied to and resolved.

Sensitivity: NO per the #252 terms (tests only, zero weakened/deleted
assertions) -- not architect-gated.

Admin merge rationale: strict-up-to-date + require_last_push_approval remain
jointly unsatisfiable for a single maintainer. Tracked in #241; fix spec posted
there.
@100yenadmin

Copy link
Copy Markdown
Member Author

Closing as salvaged-and-superseded — the useful half of this PR is now on main via #254 (60a110d3).

What was salvaged (with credit to the original author, milgauss, in the commit): 4 regression tests, including three FTS-path fences verified non-vacuous — each asserts requires_like_fallback(query) is False in-test, so they provably exercise the FTS MATCH path rather than passing via the silent LIKE fallback.

What was deliberately dropped, and why: the source hunks to dag.py/search_query.py/store.py. Main has since implemented punctuation handling differently and better — applying this branch's sanitizer would regress main ("Portland, OR hotel" would become a real FTS5 boolean OR; decomposed naïve would split to nai ve). One test was dropped as substantively redundant with main's test_sanitize_fts5_query_reduces_a_question_to_terms.

Full salvage record: session-notes/2026-08-20/lcmx-clearance/artifacts/salv159-*.txt + the #254 thread. The upstream original (hermes-lcm#354) remains open upstream; nothing here forecloses their own fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

active-continuation Active continuation of an attributed upstream item bug Something isn't working eva-direct Direct impact on Electric Sheep supported Hermes/LCM paths P2 Significant supported-path regression or bounded correctness failure upstream-evidence Preserves links and attribution to the upstream report or pull request upstream-pr Imported upstream pull-request evidence

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants