fix(store): escape interior double-quotes in sanitizeFTS to prevent FTS5 crash#586
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesFTS Sanitization Fix
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
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
mainto resolve the conflict. It's positional, not semantic — you and #537 both edit the region right aftersanitizeFTS.
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.
9190069 to
08fd805
Compare
|
Heads up: I rebased this on What the conflict actually was: both Verified locally before pushing: If you have local work on this branch, 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 |
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Rebase done and CI green across all gates. The escape is correct and the coverage is solid — merging.
c71d08b
into
Gentleman-Programming:main
|
Thanks so much, Alan, really didn’t need to, but I appreciate it a lot. Thanks for the rebase and for the review too! |
🔗 Linked Issue
Closes #574
🏷️ PR Type
type:bug— Bug fix📝 Summary
hello"world→SQL logic error: unterminated string (1)).sanitizeFTSper the FTS5 string-literal escape rule ("""), sohello"world→"hello""world".TestSanitizeFTSwith 8 cases (written first, failing per TDD; all pass after the fix).🔧 Changes
internal/store/store.gostrings.ReplaceAll(w, \"\", \"\"\"\")afterTriminsanitizeFTSto escape interior double-quotes for FTS5.internal/store/store_test.goTestSanitizeFTScovering plain words, multiple words, single/multiple interior quotes, already-quoted, just-quotes, mixed, and empty input.🧪 Test Plan
TestSanitizeFTSfails on unpatched code (RED) and passes after the fix (GREEN):go test ./internal/store/ -run TestSanitizeFTS -vgo test ./internal/store/go build -o /tmp/engram-fix ./cmd/engram && /tmp/engram-fix search 'hello"world'→ clean "No memories found" (exit 0)./tmp/engram-fix search 'sanitizeFTS'→ returns results.✅ Contributor Checklist
type:*label (type:bug)Co-Authored-BytrailersSummary by CodeRabbit