fix(store): support CJK search with trigram FTS#537
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SQLite FTS5 trigram tokenization for observation and prompt indexes, migration repair for existing indexes and triggers, and escaped ChangesSQLite FTS5 Trigram Search + CJK/Short-Token Fallback
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SearchCaller
participant StoreSearch
participant SQLiteFTS
SearchCaller->>StoreSearch: search query and match mode
StoreSearch->>StoreSearch: extract terms and choose search path
StoreSearch->>SQLiteFTS: MATCH or escaped LIKE query
SQLiteFTS-->>StoreSearch: ranked or ordered rows
StoreSearch-->>SearchCaller: search results
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Reviewer note: this PR overlaps with #526 ( If #526 merges first, this PR should be rebased and should preserve the weighted BM25 expression in the FTS branch. If this PR merges first, #526 should rebase and apply BM25 only inside the FTS branch, not the short-token LIKE fallback branch. I left the detailed merge-order note on #526: #526 (comment) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/store/store_test.go`:
- Around line 217-250: The TestSearchShortTokensUsesLikeFallbackWithTrigramFTS
test currently only covers happy path scenarios. Expand this test to include
edge case and error behavior assertions for the LIKE-fallback behavior. Add test
cases that verify proper escaping of literal % and _ characters when using the
fallback mechanism, and add assertions for invalid MatchMode values used with
short-token queries. These edge cases should include both success assertions
(that queries still return correct results despite special characters) and error
assertions (that invalid parameters are properly rejected), ensuring the
fallback path behavior is fully deterministic and regression-resistant.
- Around line 320-407: The test TestMigrateRebuildsLegacyFTSTablesWithTrigram
verifies that search works immediately after migration, but does not test
whether the triggers remain functional for subsequent mutations. After the
migrate() call and after verifying the initial search results, add follow-up
write operations (such as updating an existing observation with new content or
deleting a prompt) and then re-query using the Search and SearchPrompts methods
to assert that the FTS tables remain properly synchronized when mutations occur
post-migration. This ensures that the migration not only rebuilds the indexes
but also preserves trigger functionality.
- Around line 252-318: The test currently only exercises the soft-delete trigger
path by calling DeleteObservation with false parameter. Add explicit coverage
for the hard-delete trigger path by creating an additional observation after the
soft-delete tests, then call DeleteObservation with true parameter on this new
observation, and verify via a Search call with the same SearchOptions that the
hard-deleted observation is properly excluded from results, mirroring the
pattern already used for the soft-delete assertion.
In `@internal/store/store.go`:
- Around line 1995-2001: Replace the full FTS index rebuild approach in the
obs_fts_delete and obs_fts_update triggers with an incremental update strategy
using old.* values to modify only the affected rows in the FTS index rather than
rebuilding the entire index. Apply the same incremental approach to the
recreatePromptFTSTriggers function (which has the same rebuild pattern).
Additionally, consider implementing a batching or debouncing mechanism outside
the per-row trigger logic to further reduce write overhead during batch
migrations, particularly in the cleanup UPDATEs at lines 973-998 where multiple
rows are affected sequentially.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fe05a891-b5ae-4cdc-b517-a9a0722ecebe
📒 Files selected for processing (5)
CHANGELOG.mddocs/ARCHITECTURE.mddocs/TEAM-USAGE.mdinternal/store/store.gointernal/store/store_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/store/store_test.go`:
- Around line 328-355: The test currently covers hard-delete on a fresh
observation but does not test the edge case where the same row is soft-deleted
and then hard-deleted, which is where a corruption path can occur. Add a new
test sequence that creates an observation using AddObservation, soft-deletes it
by calling DeleteObservation with false, then hard-deletes the same row by
calling DeleteObservation with true, and finally verify that searching still
returns correct results and the FTS index remains queryable. Use the CJK search
term and assert that the results are empty after the
soft-delete-then-hard-delete sequence.
In `@internal/store/store.go`:
- Around line 1995-2014: Both FTS triggers need to be gated on old.deleted_at IS
NULL to prevent attempting to delete entries from the index that were never
added. For the obs_fts_delete trigger, add a WHEN clause checking old.deleted_at
IS NULL. For the obs_fts_update trigger, add the condition old.deleted_at IS
NULL to the existing WHEN clause to ensure the 'delete' operation only fires for
rows that were actually indexed (non-soft-deleted rows). This prevents FTS index
corruption when soft-deleted rows are modified or hard-deleted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 74c83b81-6a7d-45d2-8e21-73ba24a8b963
📒 Files selected for processing (2)
internal/store/store.gointernal/store/store_test.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/store/store_test.go`:
- Around line 328-355: The test currently covers hard-delete on a fresh
observation but does not test the edge case where the same row is soft-deleted
and then hard-deleted, which is where a corruption path can occur. Add a new
test sequence that creates an observation using AddObservation, soft-deletes it
by calling DeleteObservation with false, then hard-deletes the same row by
calling DeleteObservation with true, and finally verify that searching still
returns correct results and the FTS index remains queryable. Use the CJK search
term and assert that the results are empty after the
soft-delete-then-hard-delete sequence.
In `@internal/store/store.go`:
- Around line 1995-2014: Both FTS triggers need to be gated on old.deleted_at IS
NULL to prevent attempting to delete entries from the index that were never
added. For the obs_fts_delete trigger, add a WHEN clause checking old.deleted_at
IS NULL. For the obs_fts_update trigger, add the condition old.deleted_at IS
NULL to the existing WHEN clause to ensure the 'delete' operation only fires for
rows that were actually indexed (non-soft-deleted rows). This prevents FTS index
corruption when soft-deleted rows are modified or hard-deleted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 74c83b81-6a7d-45d2-8e21-73ba24a8b963
📒 Files selected for processing (2)
internal/store/store.gointernal/store/store_test.go
🛑 Comments failed to post (2)
internal/store/store_test.go (1)
328-355: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add a soft-delete→hard-delete sequence on the same row.
Hard-delete coverage is present, but the soft-delete (Line 317) and hard-delete (Line 346) exercise different observations. The double-
'delete'corruption path I flagged instore.goonly triggers when the same row is soft-deleted (removed from FTS) and later hard-deleted. Add a row that is soft-deleted, then hard-deleted, and assert a subsequent CJK search still returns correct results (and that the FTS index remains queryable).As per path instructions,
**/*_test.go: "Verify coverage of happy path, error paths, and edge cases."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/store_test.go` around lines 328 - 355, The test currently covers hard-delete on a fresh observation but does not test the edge case where the same row is soft-deleted and then hard-deleted, which is where a corruption path can occur. Add a new test sequence that creates an observation using AddObservation, soft-deletes it by calling DeleteObservation with false, then hard-deletes the same row by calling DeleteObservation with true, and finally verify that searching still returns correct results and the FTS index remains queryable. Use the CJK search term and assert that the results are empty after the soft-delete-then-hard-delete sequence.Source: Path instructions
internal/store/store.go (1)
1995-2014: 🩺 Stability & Availability | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # 1) Does getObservationTx filter deleted_at? If not, hard delete can run on a soft-deleted row. ast-grep --pattern $'func (s *Store) getObservationTx($_, $_) ($_, error) { $$$ }' # 2) Do migration cleanup UPDATEs (around 973-998) filter deleted_at? rg -nP -C2 'UPDATE observations SET' internal/store/store.go # 3) Is there any un-soft-delete / restore path that updates deleted_at back to NULL? rg -nP -C2 'deleted_at\s*=\s*NULL|SET deleted_at' internal/store/store.goRepository: Gentleman-Programming/engram
Length of output: 4219
Gate both FTS
'delete'operations onold.deleted_at IS NULLto prevent spurious deletions from the index.The
obs_fts_deleteandobs_fts_updatetriggers attempt to remove rows from the FTS index without verifying they were indexed in the first place. This causes corruption:
Migration cleanup UPDATEs (lines 973-990) touch soft-deleted rows unconditionally. When they modify a soft-deleted observation, the
obs_fts_updatetrigger fires and issues a'delete'command for a row that was never indexed (because it was soft-deleted).Hard DELETE on soft-deleted rows fires
obs_fts_delete, which unconditionally deletes from the FTS index—but soft-deleted rows have no FTS entry.Restore-then-delete path: Even rows restored via
deleted_at = NULL(line 5998) face the same risk if hard-deleted before being re-indexed.For an external-content FTS5 table, attempting to delete an entry whose tokens don't match what's currently indexed yields inconsistent results. Gate both
'delete'operations onold.deleted_at IS NULLso they only remove rows that were actually indexed:🛡️ Proposed fix
CREATE TRIGGER obs_fts_delete AFTER DELETE ON observations + WHEN old.deleted_at IS NULL BEGIN INSERT INTO observations_fts(observations_fts, rowid, title, content, tool_name, type, project, topic_key) VALUES ('delete', old.id, old.title, old.content, old.tool_name, old.type, old.project, old.topic_key); END; CREATE TRIGGER obs_fts_update AFTER UPDATE ON observations WHEN old.title IS NOT new.title OR old.content IS NOT new.content OR old.tool_name IS NOT new.tool_name OR old.type IS NOT new.type OR old.project IS NOT new.project OR old.topic_key IS NOT new.topic_key OR old.deleted_at IS NOT new.deleted_at BEGIN - INSERT INTO observations_fts(observations_fts, rowid, title, content, tool_name, type, project, topic_key) - VALUES ('delete', old.id, old.title, old.content, old.tool_name, old.type, old.project, old.topic_key); + INSERT INTO observations_fts(observations_fts, rowid, title, content, tool_name, type, project, topic_key) + SELECT 'delete', old.id, old.title, old.content, old.tool_name, old.type, old.project, old.topic_key + WHERE old.deleted_at IS NULL; INSERT INTO observations_fts(rowid, title, content, tool_name, type, project, topic_key) SELECT new.id, new.title, new.content, new.tool_name, new.type, new.project, new.topic_key WHERE new.deleted_at IS NULL; END;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/store.go` around lines 1995 - 2014, Both FTS triggers need to be gated on old.deleted_at IS NULL to prevent attempting to delete entries from the index that were never added. For the obs_fts_delete trigger, add a WHEN clause checking old.deleted_at IS NULL. For the obs_fts_update trigger, add the condition old.deleted_at IS NULL to the existing WHEN clause to ensure the 'delete' operation only fires for rows that were actually indexed (non-soft-deleted rows). This prevents FTS index corruption when soft-deleted rows are modified or hard-deleted.
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
The CJK problem is real and the test coverage here is the best of the current FTS PRs — legacy migration, trigger repair, update/delete, and short-token regression are all covered.
Three things before this can land:
- The description doesn't match the diff. It says "replace FTS delete-command triggers with rebuild triggers", but the diff keeps the
'delete'command triggers and only addsWHENguards —rebuildappears only in two error strings. So the stated mitigation for the trigram delete bug isn't actually implemented. Either implement it or correct the description. - The LIKE fallback degrades English, not just CJK.
shouldUseTrigramLikeFallbackreturns true if any term is under 3 runes, so a query likego to proddrops the whole search toLIKE '%…%'over a six-column concatenation — unindexed full scan,0.0 as rank, ordered byupdated_atinstead of relevance. Please narrow it to per-term, or only when all terms are short. - Split the soft-delete trigger fix (
WHERE new.deleted_at IS NULL) into its own PR — it's a separate correctness issue from CJK tokenization. - Rebase on
mainto resolve conflicts.
On merge order: please rebase after #586 and #526 land. Note that #586 is still required even with this PR — your LIKE fallback uses searchTerms() and bypasses sanitizeFTS, but foo"bar is 7 runes so it still takes the FTS path and still crashes. When you rebase over #526, preserve the bm25(...) expression in the FTS branch.
Also worth flagging: ensureObservationsFTSTrigram does a DROP TABLE + reinsert for every existing DB. No data loss since these are external-content tables, but it's a silent blocking O(N) rebuild inside migrate() — consider logging it so users on large DBs know why startup paused.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/store/store.go (1)
1929-1952: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWrap both FTS rebuilds in a transaction.
execHookruns the semicolon-separated SQL sequentially without an automatic transaction, so a failure afterCREATE VIRTUAL TABLE ...can leaveobservations_ftsorprompts_ftspresent but incomplete; later runs then skip rebuilding because the table shape already matches.
internal/store/store.go#L1929-L1952internal/store/store.go#L1965-L1982🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/store.go` around lines 1929 - 1952, Wrap both FTS rebuild blocks in internal/store/store.go at lines 1929-1952 and 1965-1982 in explicit transactions, including the DROP, CREATE, and INSERT operations for observations_fts and prompts_fts. Commit only after all statements succeed and roll back on any failure, while preserving the existing error propagation from each rebuild.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/store/store.go`:
- Around line 1929-1952: Wrap both FTS rebuild blocks in internal/store/store.go
at lines 1929-1952 and 1965-1982 in explicit transactions, including the DROP,
CREATE, and INSERT operations for observations_fts and prompts_fts. Commit only
after all statements succeed and roll back on any failure, while preserving the
existing error propagation from each rebuild.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d5af86f9-f694-44d6-9c13-39b4b9b66c63
📒 Files selected for processing (4)
CHANGELOG.mddocs/ARCHITECTURE.mdinternal/store/store.gointernal/store/store_test.go
Switch the observations and prompts FTS5 tables to tokenize='trigram' so Japanese, Chinese, and Korean substring search works across CLI, MCP, HTTP, and TUI. Legacy/non-trigram FTS tables are rebuilt once during migration (DROP + recreate + reinsert), logged so large DBs know why startup paused. - Preserve short-token searches such as `v2` with a bounded LIKE fallback, engaged ONLY when every query term is under three runes, so mixed English queries like `go to prod` stay on the FTS/bm25 path and keep relevance ranking. - Keep the standard FTS `'delete'`/insert trigger shape (same as main); soft-delete trigger gating (WHERE new.deleted_at IS NULL) is left to a follow-up PR. - Preserve the weighted bm25(observations_fts, 5.0, 1.0, 0.0, 0.0, 0.0, 3.0) ranking on the FTS branch (from Gentleman-Programming#526) and sanitizeFTS quoting (from Gentleman-Programming#586). Adds CJK, legacy-migration, trigger update/delete, and short-token / mixed-query regression tests.
Two robustness fixes to the trigram FTS migration path:
- Create the FTS triggers only after the migration backfill UPDATEs.
ensureFTSTrigramTables() ran before the sync_id/scope/topic_key
backfills, so those UPDATEs fired obs_fts_update / prompt_fts_update,
which issue an external-content 'delete' for old.rowid. On a legacy DB
whose fts table was just created fresh (and therefore empty), that
deletes a row that was never indexed and corrupts the index
("database disk image is malformed"). Moving the call after the
backfills mirrors main's ordering and keeps migration writes off the
trigger path. Fixes the regression caught by
TestNewMigratesLegacyUserPromptsSyncIDSchema.
- Wrap each FTS drop/create/reinsert rebuild in a transaction so a
failure mid-sequence cannot leave observations_fts / prompts_fts
present but empty, which a later run would skip rebuilding because the
trigram shape already matches.
|
Rebased onto latest main (linear history now, no merge commits) and addressed the review. Summary of where each item landed: Blocking items
Merge-order notes
Also fixed (regression not in the original review) Full internal/store suite passes. Force-pushed after the rebase (commits must appear later). Still to come (separate PR): the soft-delete trigger gating (old.deleted_at IS NULL), as agreed. |
6236b4a to
f55b695
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/store/store_test.go`:
- Around line 217-314: Extend
TestSearchShortTokensUsesLikeFallbackWithTrigramFTS to cover SearchPrompts
queries composed entirely of tokens shorter than three runes, including a prompt
containing literal % and _ characters. Assert the expected prompt result, verify
wildcard characters are escaped rather than treated as patterns, and check the
returned rank or ordering matches the LIKE fallback contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ee9e7c15-0ec3-435d-8d85-e2729b3207e5
📒 Files selected for processing (5)
CHANGELOG.mddocs/ARCHITECTURE.mddocs/TEAM-USAGE.mdinternal/store/store.gointernal/store/store_test.go
| func TestSearchShortTokensUsesLikeFallbackWithTrigramFTS(t *testing.T) { | ||
| s := newTestStore(t) | ||
|
|
||
| if err := s.CreateSession("s-short", "engram", "/tmp/engram"); err != nil { | ||
| t.Fatalf("create session: %v", err) | ||
| } | ||
| obsID, err := s.AddObservation(AddObservationParams{ | ||
| SessionID: "s-short", | ||
| Type: "decision", | ||
| Title: "v2", | ||
| Content: "go db", | ||
| Project: "engram", | ||
| Scope: "project", | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("add short-token observation: %v", err) | ||
| } | ||
|
|
||
| results, err := s.Search("v2", SearchOptions{Project: "engram", Limit: 10}) | ||
| if err != nil { | ||
| t.Fatalf("search short token: %v", err) | ||
| } | ||
| if len(results) != 1 || results[0].ID != obsID { | ||
| t.Fatalf("expected short-token search hit, got %+v", results) | ||
| } | ||
|
|
||
| anyResults, err := s.Search("go xx", SearchOptions{Project: "engram", Limit: 10, MatchMode: "any"}) | ||
| if err != nil { | ||
| t.Fatalf("search short token any mode: %v", err) | ||
| } | ||
| if len(anyResults) != 1 || anyResults[0].ID != obsID { | ||
| t.Fatalf("expected short-token any-mode search hit, got %+v", anyResults) | ||
| } | ||
|
|
||
| literalID, err := s.AddObservation(AddObservationParams{ | ||
| SessionID: "s-short", | ||
| Type: "decision", | ||
| Title: "literal wildcard tokens", | ||
| Content: "contains percent % and underscore _ characters", | ||
| Project: "engram", | ||
| Scope: "project", | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("add literal wildcard observation: %v", err) | ||
| } | ||
| percentResults, err := s.Search("%", SearchOptions{Project: "engram", Limit: 10}) | ||
| if err != nil { | ||
| t.Fatalf("search literal percent: %v", err) | ||
| } | ||
| if len(percentResults) != 1 || percentResults[0].ID != literalID { | ||
| t.Fatalf("expected literal percent search hit only, got %+v", percentResults) | ||
| } | ||
| underscoreResults, err := s.Search("_", SearchOptions{Project: "engram", Limit: 10}) | ||
| if err != nil { | ||
| t.Fatalf("search literal underscore: %v", err) | ||
| } | ||
| if len(underscoreResults) != 1 || underscoreResults[0].ID != literalID { | ||
| t.Fatalf("expected literal underscore search hit only, got %+v", underscoreResults) | ||
| } | ||
|
|
||
| _, err = s.Search("v2", SearchOptions{Project: "engram", Limit: 10, MatchMode: "or"}) | ||
| if err == nil { | ||
| t.Fatalf("expected invalid match_mode error for short-token fallback query") | ||
| } | ||
| if !strings.Contains(err.Error(), "invalid match_mode") { | ||
| t.Fatalf("expected invalid match_mode error, got %v", err) | ||
| } | ||
|
|
||
| // Mixed-length queries must stay on FTS (not degrade English ranking via LIKE). | ||
| mixedID, err := s.AddObservation(AddObservationParams{ | ||
| SessionID: "s-short", | ||
| Type: "decision", | ||
| Title: "production rollout", | ||
| Content: "go to prod checklist", | ||
| Project: "engram", | ||
| Scope: "project", | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("add mixed-length observation: %v", err) | ||
| } | ||
| mixedResults, err := s.Search("go to prod", SearchOptions{Project: "engram", Limit: 10}) | ||
| if err != nil { | ||
| t.Fatalf("search mixed-length query: %v", err) | ||
| } | ||
| foundMixed := false | ||
| for _, r := range mixedResults { | ||
| if r.ID == mixedID { | ||
| foundMixed = true | ||
| if r.Rank == 0 { | ||
| t.Fatalf("expected FTS/bm25 rank for mixed-length query, got rank=%v", r.Rank) | ||
| } | ||
| break | ||
| } | ||
| } | ||
| if !foundMixed { | ||
| t.Fatalf("expected mixed-length FTS hit for go to prod, got %+v", mixedResults) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every SearchPrompts call in tests and show the query arg to confirm none is <3 runes.
rg -nP --type=go -C1 '\.SearchPrompts\(' internal/store/store_test.goRepository: Gentleman-Programming/engram
Length of output: 1497
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== store_test relevant SearchPrompts additions/observations =="
rg -n --type=go 'AddPrompt|CreatePrompt|SearchPrompts\(' internal/store/store_test.go -C 2
echo
echo "== store.go SearchPrompts outline around likely line ranges =="
wc -l internal/store/store.go
rg -n 'func \(.*SearchPrompts|shouldUseTrigramLikeFallback|FROM user_prompts|FROM prompts_fts|appendLikeSearchCondition' internal/store/store.go -C 3
echo
echo "== exact StoreTest around first SearchPrompts =="
sed -n '180,220p' internal/store/store_test.go
sed -n '365,410p' internal/store/store_test.go
sed -n '480,542p' internal/store/store_test.goRepository: Gentleman-Programming/engram
Length of output: 22357
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SearchPrompts implementation =="
sed -n '2767,2815p' internal/store/store.go
echo
echo "== helper definitions =="
sed -n '6690,6745p' internal/store/store.go
echo
echo "== deterministic probe: shouldUseTrigramLikeFallback/searchTerms across SearchPrompts query args =="
python3 - <<'PY'
from pathlib import Path
import re
text = Path('internal/store/store_test.go').read_text()
for m in re.finditer(r'\.SearchPrompts\(\s*"((?:[^"\\]|\\[\\"])*)"', text):
raw = m.group(1)
s = ''
i = 0
while i < len(raw):
if raw[i] == '\\' and i+1 < len(raw):
s += raw[i+1]
i += 2
else:
s += raw[i]
i += 1
print(f'query={s!r}, runes={len(s)}, ascii_len={len(s.encode("utf-8"))}')
PYRepository: Gentleman-Programming/engram
Length of output: 3729
Add coverage for the SearchPrompts short-token LIKE fallback.
SearchPrompts uses a distinct wildcard path when every query token is under 3 runes, but the tests only cover FTS queries or single "x" closed-db cases. Add a short token wildcard prompt search, including literal %/_ escaping, with expected result and rank/ordering assertions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/store_test.go` around lines 217 - 314, Extend
TestSearchShortTokensUsesLikeFallbackWithTrigramFTS to cover SearchPrompts
queries composed entirely of tokens shorter than three runes, including a prompt
containing literal % and _ characters. Assert the expected prompt result, verify
wildcard characters are escaped rather than treated as patterns, and check the
returned rank or ordering matches the LIKE fallback contract.
Source: Path instructions
Linked Issue
Closes #140
PR Type
type:bug— Bug fixtype:feature— New featuretype:docs— Documentation onlytype:refactor— Code refactoring (no behavior change)type:chore— Maintenance/toolingtype:breaking-change— Breaking changeSummary
tokenize='trigram'so Japanese, Chinese, and Korean substring search works.DROP+ reinsert), with a log line so large DBs know why startup paused.v2with a boundedLIKEfallback only when every query term is under three characters (mixed queries likego to prodstay on the FTS/bm25 path).'delete'/inserttriggers (same shape asmain). Soft-delete trigger gating (WHERE new.deleted_at IS NULL) is intentionally deferred to a follow-up PR.Changes
internal/store/store.gobm25(...)on the FTS branch.internal/store/store_test.goCHANGELOG.mddocs/ARCHITECTURE.mddocs/TEAM-USAGE.mdTest Plan
go test ./internal/store -run 'TestSearch|Trigram|FTS'go test ./...go test -tags e2e ./internal/server/...Contributor Checklist
Closes #140)type:*label to this PRgo test ./...go test -tags e2e ./internal/server/...Co-Authored-Bytrailers in commitsNotes for Reviewers
mainafter feat(store): replace FTS5 default ranking with weighted BM25 #526 (bm25) and fix(store): escape interior double-quotes in sanitizeFTS to prevent FTS5 crash #586 (sanitizeFTS quotes).bm25(observations_fts, 5.0, 1.0, 0.0, 0.0, 0.0, 3.0).Summary by CodeRabbit
v2) via a boundedLIKEfallback when queries are very short.