Skip to content

fix: wire advanced FTS5 parser into local keyword search - #67

Open
bernoussama wants to merge 1 commit into
improvement/improve-retrievalfrom
fix/wire-up-advanced-fts5
Open

fix: wire advanced FTS5 parser into local keyword search#67
bernoussama wants to merge 1 commit into
improvement/improve-retrievalfrom
fix/wire-up-advanced-fts5

Conversation

@bernoussama

Copy link
Copy Markdown
Owner

Summary

Resolves the deferred issue from #66: the operator-aware ftsQuery parser (AND/OR/NOT, column filters, prefix terms, NEAR, syntax validation) was only exercised by unit tests — the runtime keyword path used the simpler localFtsQuery, so documented advanced syntax was silently ignored and syntax errors never surfaced.

This PR wires ftsQuery into the runtime keyword-search path and fixes three latent bugs that wiring exposed.

Changes

Core

  • local-backend.ts: route searchLocalKeywordExact through ftsQuery so advanced operators, column filters, and syntax validation work as documented; delete the now-dead localFtsQuery.

Latent bugs fixed (discovered during implementation)

  • NEAR never worked. tokenizeFtsQuery split ( into a separate token, so NEAR(token nft, 5) could never survive as a unit. Now captures NEAR(...) as a single token and normalizes the forgiving comma form (NEAR(a, b, 5)) to FTS5's real space-separated form (NEAR(a b, 5)) — verified against better-sqlite3.
  • NEAR syntax was wrong in docs + error hint. FTS5 uses space-separated terms, not commas. Updated search-solutions.md and the ftsSyntaxError message to the canonical NEAR(term term [, N]) form.
  • Dashes were rejected in simple mode. buildSimpleQuery captured -wal then rejected it as bare negation, which would have broken the existing "sqlite -wal" integration test. Now treats a leading hyphen as punctuation (strips it), matching the old localFtsQuery behavior.

Error surfacing

  • server.ts: FtsQuerySyntaxError now returns its message as text content instead of re-throwing, so MCP clients can read the hint and retry.
  • index.ts: both search and local search catch blocks show a clearer "Invalid search syntax:" header for FtsQuerySyntaxError.

Tests

  • 6 new cases: balanced parens, NEAR normalization (3 forms), unterminated NEAR, and 3 runtime integration tests (AND operator, column filter, malformed-query rejection through the real backend).
  • Updated the leading-dash unit test to reflect the new punctuation behavior.

Notes

  • The relaxed/hybrid path is intentionally untouched, so hybrid-search quality is unaffected.
  • Column-filter rendering (tags : "auth") was verified valid against real SQLite — no change needed there.

Validation

  • 129/129 CLI tests pass (10 test files)
  • Typecheck clean, lint clean (0 errors), format clean

The operator-aware ftsQuery parser (AND/OR/NOT, column filters, prefix
terms, NEAR, syntax validation) was only exercised by unit tests; the
runtime keyword path used the simpler localFtsQuery, so documented
advanced syntax was silently ignored and syntax errors never surfaced.

- local-backend: route searchLocalKeywordExact through ftsQuery so
  advanced operators, column filters, and syntax validation work as
  documented; delete the now-dead localFtsQuery
- local-backend: fix NEAR passthrough — tokenizeFtsQuery now captures
  NEAR(...) as a unit and normalizes the forgiving comma form
  (NEAR(a, b, 5)) to FTS5's space-separated form (NEAR(a b, 5))
- local-backend: treat a leading hyphen as punctuation in simple mode
  (e.g. "sqlite -wal" searches for both terms) instead of rejecting it
- local-backend: correct the FTS5 syntax hint (NEAR uses space-separated
  terms, not commas)
- server: surface FtsQuerySyntaxError as text content instead of
  re-throwing, so MCP clients can read the hint and retry
- index: special-case FtsQuerySyntaxError in the CLI catch blocks with a
  clearer "Invalid search syntax" header
- search-solutions.md: show the canonical NEAR(token nft, 5) form
@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 848ebe33-721c-4212-9595-3e74770604e9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/wire-up-advanced-fts5

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@bernoussama bernoussama left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nightly review — clankeroverflow#67 (fix: wire advanced FTS5 parser into local keyword search)

Verdict: Approve with nits. Good change: kills the ad-hoc localFtsQuery string munging and routes exact search through the validated ftsQuery tokenizer (consistent syntax + quoting), normalizes NEAR(a, b, 5)NEAR(a b, 5), and surfaces FtsQuerySyntaxError cleanly in both CLI and MCP. Tests cover the new paths.

Nits:

  • captureNear() backtracks a hardcoded 4 chars (query.slice(parenIndex - 4, ...)) for the NEAR word. NEAR (a b) (space before paren) isn't captured by the /^NEAR$/ guard path and can produce a confusing error or wrong tokens — compute the start from current.length instead.
  • normalizeNear() passes inner terms through raw without quoting/escaping — NEAR(a"b) can yield an unbalanced FTS string and a raw SQLite error instead of your clean syntax error. Quote terms like the rest of the pipeline does.
  • Behavior change to flag: simple mode now silently strips a leading dash (-wal"wal"), which inverts user intent for anyone typing an exclusion. Old error message was arguably more correct; consider stripping only when the dash is followed by a digit (negative numbers / version strings), erroring otherwise.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant