Fix: hybrid lexical + semantic retrieval ordering - #348
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe changes align search documentation with the implemented lexical and cosine retrieval behavior. They also document lexical fallback when embeddings are disabled and the reason BM25 is not used. ChangesHybrid search documentation
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to Documentation clarifies hybrid retrieval ordering and embedding-disabled fallback behavior without changing runtime behavior. No merge-readiness risk is identified. 🚥 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 |
There was a problem hiding this comment.
🟡 Changes recommended
The updated docs introduce a broken JSDoc sentence and an internal inconsistency in the BM25 rationale wording that should be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates repository documentation to accurately describe the current hybrid lexical + semantic retrieval strategy (sentinel-score UNION ALL of lexical LIKE/ILIKE rows plus semantic cosine-similarity rows) and clarifies why BM25 was evaluated but removed.
Changes:
- Updated README feature bullet to describe sentinel-score fusion behavior and rationale for dropping BM25.
- Corrected
src/embeddings/disable.tsJSDoc to refer to lexicalLIKE/ILIKEfallback (not “BM25 / ILIKE”).
File summaries
| File | Description |
|---|---|
| README.md | Updates the hybrid retrieval description and BM25 rationale in the feature list. |
| src/embeddings/disable.ts | Updates embeddings-disable JSDoc to reflect lexical fallback behavior and BM25 rationale. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * lexical `LIKE`/`ILIKE` matching on text columns (BM25 was evaluated but | ||
| * dropped — its score scale (~1..3) is incompatible with cosine in a single | ||
| * ORDER BY without RRF or normalisation; see grep-core.ts PR-NOTES F4c). | ||
| * readable. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Closing in favour of #350, which fixes the same stale BM25 wording from #337 with a minimal change to the three affected spots (README, docs/EMBEDDINGS.md, disable.ts JSDoc) and has a green CI run. Your reading of the hybrid ranking is correct (UNION ALL, 1.0 sentinel for lexical rows, ORDER BY score, BM25 dropped per PR-NOTES F4c), but that explanation already lives in src/shell/grep-core.ts and is too much detail for the README feature list. Two small things here also needed fixing before it could land: the JSDoc in src/embeddings/disable.ts was left with a dangling "readable." line, and the PR title says "Fix" for a docs-only change. Thanks for taking the time to verify it against the code. |
Summary
Fix stale documentation that described the hybrid retrieval strategy as using BM25 as a fallback. BM25 was evaluated and removed (PR-NOTES F4c) because its unbounded score scale (~1–3) overwhelms cosine similarity values (0–1) in a shared
ORDER BY, causing semantic hits to be pushed out of the top-K entirely. Using it correctly would require rank-based fusion (RRF) or score normalisation, which adds complexity without clear benefit for the "find any session mentioning X" use case.The actual hybrid strategy is a
UNION ALLof:LIKE/ILIKEsubstring rows, emitting a sentinel score of 1.0 (capped byHIVEMIND_HYBRID_LEXICAL_LIMIT, default 20)Results are ordered by score descending, so exact keyword matches always lead the ranking while semantic concept matches fill in below. When embeddings are off, retrieval falls back to lexical
LIKE/ILIKEonly.Files changed:
README.md— updated the feature bullet to describe the sentinel-score fusion and document why BM25 was droppedsrc/embeddings/disable.ts— corrected the JSDoc comment from"BM25 / ILIKE matching"to"lexical LIKE/ILIKE matching"with the same rationaleVersion Bump
This is a documentation-only change — no release needed. No version bump required.
Test plan
npm test)package.json, or no release needed for this change ✓ (docs only)Summary by CodeRabbit
LIKE/ILIKEwhen embeddings are disabled.