fix(entities): match file-path entities case-insensitively - #29
Conversation
extractEntities stored file paths in their original case while every other entity kind was normalized. findEntityMatches compares with `me.entity IN (...)`, which is case-sensitive on TEXT in SQLite, so a memory mentioning src/Auth.ts never matched a query mentioning src/auth.ts even though they are the same file. Lowercase file paths at extraction time. Both the write path (recordEntities) and the read path (findEntityMatches) go through extractEntities, so the two stay consistent by construction, and the BINARY-collation index on memory_entities.entity is still usable. Entities are only ever a matching key and are never displayed, so no display case is lost. Closes 7vignesh#26
|
@anush-933 is attempting to deploy a commit to the vignesh's projects Team on Vercel. A member of the Team first needs to authorize it. |
7vignesh
left a comment
There was a problem hiding this comment.
Verified locally and green on Node 20/22 + CodeQL. The reasoning in your description is exactly right - lowercasing at extraction keeps the write and read paths in agreement without forcing a scan, and leaving the bench snapshot alone was the correct call since the drift pre-exists on main.
|
On the migration question: no need. Entities re-record whenever a memory is updated, and the entity leg is a recall booster rather than a correctness guarantee, so existing rows healing lazily is fine - not worth the migration risk. Appreciate you flagging it rather than assuming. Thanks for the clean fix! |
extractEntitieskeeps file paths in their original case, but every other entity kind gets normalized. SincefindEntityMatchescompares withme.entity IN (...)and SQLite'sINon TEXT is case-sensitive, a memory that mentionssrc/Auth.tsnever matches a query that mentionssrc/auth.ts.I went with lowercasing the path at extraction time. Both
recordEntitiesandfindEntityMatchescallextractEntities, so the two can't drift apart, and the index onmemory_entities.entitystill gets used. I skippedCOLLATE NOCASEbecause it would force a scan on the retrieval path and would apply to every kind rather than just files. Nothing ever displays the entity string, so there's no display case to preserve.Closes #26
Testing
npm run cipasses (234 tests). Added three tests totests/entities.test.ts: lowercase normalization, mixed-case dedupe, and the repro from the issue. All three fail before the change and pass after.I also ran
npm run benchbefore and after the change and the output is identical, so there's no retrieval drift.bench:checkdoes report drift against the committed snapshot, but it reports the same thing on a cleanmain, so I leftbench/results/repo-memory.jsonalone rather than fold an unrelated snapshot update into this PR.One question
This only fixes entities going forward. Rows already in an existing
memory.dbkeep their old casing until that memory is re-recorded. I kept the change insideentities.tssince the issue scoped it that way, but I'm happy to add a migration that lowercases existingkind = 'file'rows if you'd rather have it here.