Skip to content

fix(store): escape interior double-quotes in sanitizeFTS to prevent FTS5 crash#586

Merged
Alan-TheGentleman merged 1 commit into
Gentleman-Programming:mainfrom
blak0p:fix/fts5-interior-quote
Jul 20, 2026
Merged

fix(store): escape interior double-quotes in sanitizeFTS to prevent FTS5 crash#586
Alan-TheGentleman merged 1 commit into
Gentleman-Programming:mainfrom
blak0p:fix/fts5-interior-quote

Conversation

@blak0p

@blak0p blak0p commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

🔗 Linked Issue

Closes #574


🏷️ PR Type

  • type:bug — Bug fix

📝 Summary

  • Fixes FTS5 search crash on queries containing an interior double-quote (e.g. hello"worldSQL logic error: unterminated string (1)).
  • Doubles interior " inside sanitizeFTS per the FTS5 string-literal escape rule ("""), so hello"world"hello""world".
  • Adds TestSanitizeFTS with 8 cases (written first, failing per TDD; all pass after the fix).

🔧 Changes

File Change
internal/store/store.go Add strings.ReplaceAll(w, \"\", \"\"\"\") after Trim in sanitizeFTS to escape interior double-quotes for FTS5.
internal/store/store_test.go Add TestSanitizeFTS covering plain words, multiple words, single/multiple interior quotes, already-quoted, just-quotes, mixed, and empty input.

🧪 Test Plan

  • New test TestSanitizeFTS fails on unpatched code (RED) and passes after the fix (GREEN): go test ./internal/store/ -run TestSanitizeFTS -v
  • Full store package passes (no regressions): go test ./internal/store/
  • Built binary no longer crashes: go build -o /tmp/engram-fix ./cmd/engram && /tmp/engram-fix search 'hello"world' → clean "No memories found" (exit 0).
  • Normal search still works: /tmp/engram-fix search 'sanitizeFTS' → returns results.

✅ Contributor Checklist

Summary by CodeRabbit

  • Bug Fixes
    • Improved search query handling so text containing double quotes no longer produces malformed search expressions, resulting in more reliable search behavior.
  • Tests
    • Added unit test coverage for search-term formatting across common scenarios, including plain terms, multi-word input, inputs with interior quotes, mixed quoted/unquoted sequences, and empty input.

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 72e3028e-7143-46e8-98fa-796df51675e0

📥 Commits

Reviewing files that changed from the base of the PR and between 9190069 and 08fd805.

📒 Files selected for processing (2)
  • internal/store/store.go
  • internal/store/store_test.go

📝 Walkthrough

Walkthrough

sanitizeFTS now escapes interior double quotes within tokens before wrapping them in FTS5-quoted literals. A table-driven test validates the updated formatting across quoted, unquoted, mixed, and empty inputs.

Changes

FTS Sanitization Fix

Layer / File(s) Summary
Escape interior quotes in FTS sanitization
internal/store/store.go, internal/store/store_test.go
sanitizeFTS doubles interior double quotes before FTS5 quoting, and TestSanitizeFTS validates the resulting query strings across multiple input cases.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Suggested reviewers: gentleman-programming, alan-thegentleman

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: escaping interior double quotes in sanitizeFTS to prevent FTS5 crashes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@Alan-TheGentleman Alan-TheGentleman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Correct fix, exemplary size. strings.ReplaceAll(w, "\"", "\"\"") after the Trim is the right FTS5 literal escape, and the 8-case table-driven test is exactly what this needed.

  • Only blocker: rebase on main to resolve the conflict. It's positional, not semantic — you and #537 both edit the region right after sanitizeFTS.

Worth noting for whoever reviews #537: this fix is still required after that one lands. Its LIKE fallback uses searchTerms() and bypasses sanitizeFTS entirely, but foo"bar is 7 runes so it still takes the FTS path and still crashes without your escape.

One cosmetic thing, not blocking: Trim still strips an unbalanced trailing quote (a""a"), losing a character. Not a crash, just slightly lossy.

type:bug applied. Ping me after the rebase and I'll merge.

…TS5 crash

sanitizeFTS wrapped each whitespace-separated word in double-quotes for
FTS5 MATCH queries, but only stripped surrounding quotes via
strings.Trim(w, `"`). Interior quotes were left untouched, so an input
like `hello"world` produced `"hello"world"` — an unterminated FTS5 string
literal that crashed every search path (mem_search, candidate detection)
with "SQL logic error: unterminated string (1)". See issue Gentleman-Programming#574.

FTS5 escapes a literal double-quote inside a quoted phrase by doubling
it (`""`), so we apply strings.ReplaceAll(w, `"`, `""`) after the Trim.
Now `hello"world` correctly becomes `"hello""world"`.

Why this spot and not the query layer: sanitizeFTS is the single funnel
used by both search entry points (store.go:2171, store.go:2644), so the
fix covers all affected call sites without touching sanitizeFTSCandidates
(relations.go), which builds OR-based queries and has its own escaping.

Adds TestSanitizeFTS covering plain words, multiple words, single and
multiple interior quotes, already-quoted input, just-quotes, mixed input,
and the empty case. Written first (failing) per TDD; all 8 cases pass
after the one-line fix.
@Alan-TheGentleman
Alan-TheGentleman force-pushed the fix/fts5-interior-quote branch from 9190069 to 08fd805 Compare July 20, 2026 12:07
@Alan-TheGentleman

Copy link
Copy Markdown
Collaborator

Heads up: I rebased this on main for you and force-pushed to fix/fts5-interior-quote, since the conflict was purely mechanical and this is a crash fix worth landing quickly. Your commit and authorship are unchanged — only the parent moved (919006908fd805).

What the conflict actually was: both main and your branch appended new tests to the end of internal/store/store_test.go, so git interleaved them. I took main's version of the file and re-appended your TestSanitizeFTS intact. Your one-line production change in sanitizeFTS applied cleanly with no conflict at all.

Verified locally before pushing: go build ./... clean, gofmt clean, and the full internal/store suite passes including all 8 of your table cases.

If you have local work on this branch, git fetch && git reset --hard origin/fix/fts5-interior-quote before continuing.

Merging once CI goes green. Thanks for the fix — the escape is exactly right, and it stays necessary even after #537 lands, since that PR's LIKE fallback bypasses sanitizeFTS but foo"bar is 7 runes and still takes the FTS path.

@Alan-TheGentleman Alan-TheGentleman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Rebase done and CI green across all gates. The escape is correct and the coverage is solid — merging.

@Alan-TheGentleman
Alan-TheGentleman merged commit c71d08b into Gentleman-Programming:main Jul 20, 2026
9 checks passed
@blak0p

blak0p commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks so much, Alan, really didn’t need to, but I appreciate it a lot. Thanks for the rebase and for the review too!

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

Labels

type:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] FTS5 search returns HTTP 500 on queries containing an interior double-quote (e.g. foo"bar)

2 participants