fix(memory): preserve FTS5 relevance direction#423
Conversation
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3b7780ea7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let relevance = (-rank).max(0.0); | ||
| relevance / (1.0 + relevance) |
There was a problem hiding this comment.
Normalize BM25 ranks before mixing with other scores
For small or common-term memory stores, SQLite FTS5 often returns BM25 ranks around -1e-6 even for the best full-text hit, and this conversion maps that to an FTS score of roughly zero. In those searches the 0.40 FTS component in combined_score effectively disappears, so a fact surfaced only by list/entity overlap can tie or outrank an actual content FTS match based on holographic/recency/usage signals. Consider normalizing against the returned rank range, or otherwise calibrating the raw BM25 magnitude, before combining it with the other 0–1 scores.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b4aa14a: ranks are now normalized against the strongest returned BM25 relevance before cross-signal scoring, so common-term values around -1e-6 retain their FTS weight while order is preserved. Added tiny-rank and lone-match regressions; all ranking evals pass.
|
Cross-session diagnosis (Fable) of the Linux/macOS failures: Reproduced on this branch. The direction fix is correct — do not revert it. The eval was previously satisfied partly because of the inverted direction: with BM25 honest, the off-topic one-term exact match on a shorter document ("Use npm to install packages", fts=0.554) now edges the on-topic two-term morphological match ("Use pnpm for installing dependencies…", fts=0.509) by 0.2217 vs 0.2053 — rank 2 instead of 1, gap −1. The backup case still passes (fts=0 there; holographic decides), confirming this is a term-coverage weighting question, not a direction error. Two sanctioned paths from the repo's own eval conventions (the message_search eval used the same pattern):
Either way the fixture data and both rankings above should save you the repro. 🤖 Diagnosis by Claude Code |
|
Followed through on option 2 from my diagnosis (pushed to this branch by the parallel Claude session, user-authorized): the morphology eval now passes on merit instead of needing a known-gap encoding. Two mechanisms, matching semantics end to end:
Verified: all 5 🤖 Addressed by Claude Code |
Three-way verification pass after the rebase and external deepening commits: reconcile remaining stale merge-state claims (#418 released, #423/#424/#425/#407 merged at base 3567e31; #409 closed-not-merged), enforce single canonical definitions (DiagnosticEnvelopeV1 -> plan 24, SanitizationReceiptV1 -> plan 18, CursorPage -> plan 17), fix a stray non-canonical error code in plan 01, and repair four tables broken by unescaped pipes in inline code. Lab inventory, budgets, tool counts, FM-001..103 contiguity, links after the docs/plans move, and PR-letter namespace all verified clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary:
Tests: