feat(memory): enable query expansion in hybrid search mode#57711
feat(memory): enable query expansion in hybrid search mode#57711ckybit wants to merge 2 commits into
Conversation
Query expansion (stop-word removal and keyword extraction) was only active in the FTS-only fallback path. In hybrid mode the raw conversational query was passed directly to FTS, which AND-joins every token — stop words like "what", "that", "about" had to appear in the indexed text for a match, producing zero keyword results for most natural-language queries. Extract keywords via `extractKeywords` before the hybrid keyword search, matching the existing FTS-only behaviour. Vector search continues to receive the original query for semantic matching. Made-with: Cursor
openclaw-barnacle
Bot
added
extensions: memory-core
Greptile SummaryThis PR fixes a real bug in hybrid search mode: conversational queries were passed verbatim to FTS, causing Key observations:
Confidence Score: 5/5Safe to merge — the core fix is correct, backward-compatible, and the remaining findings are minor test quality issues. The one-line production change in manager.ts is straightforward and directly mirrors an already-proven pattern from FTS-only mode. Both findings are P2: the AND-vs-OR semantics difference is a defensible design choice given vector search covers broad recall, and the misleading test name does not affect runtime behaviour. No data integrity, security, or primary-path correctness issues were found. The test file manager.hybrid-query-expansion.test.ts has a mislabelled test case and a gap in fallback-path coverage, but neither blocks the merge.
|
| Filename | Overview |
|---|---|
| extensions/memory-core/src/memory/manager.ts | Adds extractKeywords call before the hybrid FTS search so stop words are stripped from conversational queries, fixing the AND-join over-restriction. Core logic is correct; minor note that AND semantics over extracted keywords differ from the FTS-only mode's per-keyword OR approach. |
| extensions/memory-core/src/memory/manager.hybrid-query-expansion.test.ts | New test file with 5 cases covering English, Chinese, and edge cases. One test is misleadingly named but exercises the wrong code path; the true fallback path (all-stop-word query) is not covered. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: extensions/memory-core/src/memory/manager.hybrid-query-expansion.test.ts
Line: 130-149
Comment:
**Misleading test name and missing fallback coverage**
The test is titled "falls back to original query when no keywords are extracted", but the body immediately asserts `expect(keywords.length).toBeGreaterThan(0)` — confirming that keywords _were_ extracted from `"API PostgreSQL"`. This means the fallback branch (`keywords.length === 0 → use cleaned`) is never exercised. There is no test for the actual fallback path, which would be triggered by a query composed entirely of stop words.
Consider splitting into two tests: one that covers AND-joining multiple extracted keywords, and one that genuinely tests the fallback by passing a query where all tokens are stop words, asserts the returned keywords array is empty, and verifies the search still returns a defined (non-throwing) result.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/memory-core/src/memory/manager.ts
Line: 383-386
Comment:
**AND-join semantics differ from FTS-only mode's per-keyword OR approach**
In FTS-only mode each keyword is searched independently and result sets are union-merged (OR semantics). In the new hybrid code, keywords are space-joined and passed to `buildFtsQuery`, which AND-joins every token (e.g. `"API keys"` becomes `"API" AND "keys"`).
This means a multi-keyword query that extracts several terms will produce zero FTS hits if no single document contains all extracted terms — whereas FTS-only mode would still surface documents matching any individual keyword.
Given that vector search covers broad semantic recall in hybrid mode this is defensible and arguably more precise, but the asymmetry compared to FTS-only mode is worth documenting in a code comment, and a test covering a multi-keyword, multi-document scenario would help lock in the intended AND-join behaviour.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(memory): enable query expansion in ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| rows, | ||
| query: keywords.join(" "), | ||
| }); | ||
| // Expanded query should return at least as many results as raw | ||
| expect(expandedResults.length).toBeGreaterThanOrEqual(rawResults.length); | ||
| }); | ||
|
|
||
| it("falls back to original query when no keywords are extracted", async () => { | ||
| const keywords = extractKeywords("API PostgreSQL"); | ||
| // Pure keywords without stop words — extractKeywords returns them as-is | ||
| expect(keywords.length).toBeGreaterThan(0); | ||
|
|
||
| const results = await runSearch({ | ||
| rows: indexedRows, | ||
| query: keywords.join(" "), | ||
| }); | ||
| // "API" AND "PostgreSQL" — only row 1 has "API" but not "PostgreSQL", | ||
| // so this may return 0. The point is the code doesn't break. | ||
| expect(results).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
The test is titled "falls back to original query when no keywords are extracted", but the body immediately asserts expect(keywords.length).toBeGreaterThan(0) — confirming that keywords were extracted from "API PostgreSQL". This means the fallback branch (keywords.length === 0 → use cleaned) is never exercised. There is no test for the actual fallback path, which would be triggered by a query composed entirely of stop words.
Consider splitting into two tests: one that covers AND-joining multiple extracted keywords, and one that genuinely tests the fallback by passing a query where all tokens are stop words, asserts the returned keywords array is empty, and verifies the search still returns a defined (non-throwing) result.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/memory-core/src/memory/manager.hybrid-query-expansion.test.ts
Line: 130-149
Comment:
**Misleading test name and missing fallback coverage**
The test is titled "falls back to original query when no keywords are extracted", but the body immediately asserts `expect(keywords.length).toBeGreaterThan(0)` — confirming that keywords _were_ extracted from `"API PostgreSQL"`. This means the fallback branch (`keywords.length === 0 → use cleaned`) is never exercised. There is no test for the actual fallback path, which would be triggered by a query composed entirely of stop words.
Consider splitting into two tests: one that covers AND-joining multiple extracted keywords, and one that genuinely tests the fallback by passing a query where all tokens are stop words, asserts the returned keywords array is empty, and verifies the search still returns a defined (non-throwing) result.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Addressed in 1745ffe: split into two tests — "AND-joins multiple extracted keywords" and "falls back to original query when all tokens are stop words" (exercises the keywords.length === 0 branch).
| const keywords = extractKeywords(cleaned, { | ||
| ftsTokenizer: this.settings.store.fts.tokenizer, | ||
| }); | ||
| const keywordQuery = keywords.length > 0 ? keywords.join(" ") : cleaned; |
There was a problem hiding this comment.
In FTS-only mode each keyword is searched independently and result sets are union-merged (OR semantics). In the new hybrid code, keywords are space-joined and passed to buildFtsQuery, which AND-joins every token (e.g. "API keys" becomes "API" AND "keys").
This means a multi-keyword query that extracts several terms will produce zero FTS hits if no single document contains all extracted terms — whereas FTS-only mode would still surface documents matching any individual keyword.
Given that vector search covers broad semantic recall in hybrid mode this is defensible and arguably more precise, but the asymmetry compared to FTS-only mode is worth documenting in a code comment, and a test covering a multi-keyword, multi-document scenario would help lock in the intended AND-join behaviour.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/memory-core/src/memory/manager.ts
Line: 383-386
Comment:
**AND-join semantics differ from FTS-only mode's per-keyword OR approach**
In FTS-only mode each keyword is searched independently and result sets are union-merged (OR semantics). In the new hybrid code, keywords are space-joined and passed to `buildFtsQuery`, which AND-joins every token (e.g. `"API keys"` becomes `"API" AND "keys"`).
This means a multi-keyword query that extracts several terms will produce zero FTS hits if no single document contains all extracted terms — whereas FTS-only mode would still surface documents matching any individual keyword.
Given that vector search covers broad semantic recall in hybrid mode this is defensible and arguably more precise, but the asymmetry compared to FTS-only mode is worth documenting in a code comment, and a test covering a multi-keyword, multi-document scenario would help lock in the intended AND-join behaviour.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Addressed in 1745ffe: added a comment documenting the intentional AND-join semantics in hybrid mode vs the per-keyword OR approach in FTS-only mode.
- Document intentional AND-join semantics in hybrid mode vs OR in FTS-only mode (vector search covers broad recall). - Split misleading "falls back" test into two: one for AND-join of multiple keywords, one for genuine fallback on all-stop-word input. Made-with: Cursor
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1745ffe86b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const keywords = extractKeywords(cleaned, { | ||
| ftsTokenizer: this.settings.store.fts.tokenizer, | ||
| }); | ||
| const keywordQuery = keywords.length > 0 ? keywords.join(" ") : cleaned; |
There was a problem hiding this comment.
When hybrid search now switches to keywords.join(" "), numeric-only terms can be silently dropped before FTS runs. extractKeywords filters pure numbers, so a query like error 404 becomes error (if at least one other keyword remains), and searchKeyword no longer requires 404, which broadens matches and can surface irrelevant chunks for version/error-code lookups. Before this change, the raw hybrid query kept all tokens and enforced both terms through buildFtsQuery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for the catch. extractKeywords filtering pure numeric tokens is its existing behaviour (already active in FTS-only mode since #18304) — this PR only reuses the same function in the hybrid path. In hybrid mode, vector search still receives the original query including numeric terms, so semantic recall for error codes/versions is preserved. If we want to refine numeric handling in extractKeywords itself, that's a good candidate for a follow-up issue. Not addressing in this PR to keep scope tight.
CI Status NoteAll CI failures on this PR are pre-existing on The failures here are:
All PR-specific checks pass cleanly: @vignesh07 This PR is ready for review. It's a small, scoped fix (net +9 lines in production code) that brings keyword extraction to the hybrid search path, matching what FTS-only mode already does. All bot review feedback has been addressed. Would appreciate your review when you have a moment. Thanks! |
|
Hi @vincentkoc — hope you don't mind the ping. This is a small scoped fix for the memory module (net +9 lines in production code) that brings keyword extraction to the hybrid search path. It's been open for about a day and all bot review feedback has been addressed. Would really appreciate a look when you have a moment. Thanks for all the work on memory-core! |
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. This PR is a valid small fix, but it is superseded by open #39555, which now carries forward the same hybrid query-expansion behavior and broadens the fix to bounded per-keyword hybrid keyword fan-out with regression coverage. Current main does not yet implement either fix, so the best path is to keep #39555 as canonical. Best possible solution: Close this PR as superseded and use #39555 as the canonical implementation/review path, because it carries forward the same hybrid query-expansion intent while addressing the broader per-keyword hybrid recall bug with bounded fan-out and tests. What I checked:
So I’m closing this here and keeping the remaining discussion on the canonical linked item. Codex review notes: model gpt-5.5, reasoning high; reviewed against f256eeba431b. |
1 participant
Summary
extractKeywords(already used in FTS-only mode) before the hybrid keyword search. Vector search continues to receive the original query for semantic matching.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
extractKeywordsand stop-word lists already exist inpackages/memory-host-sdk/src/host/query-expansion.tsand are imported inmanager.ts, but only used in theif (!this.provider)branch.Regression Test Plan (if applicable)
extensions/memory-core/src/memory/manager.hybrid-query-expansion.test.tssearchKeyworddirectly with indexed content, proving the AND-join problem and the keyword extraction fix.User-visible / Behavior Changes
memory_searchin hybrid mode now returns better keyword-side results for conversational queries. Previously, queries like "what was that thing about API keys" would produce zero FTS hits (only vector results); now the FTS component correctly matches on "API" and "keys".Diagram (if applicable)
Security Impact (required)
Repro + Verification
Environment
Steps
Expected
Actual
Evidence
New test file
manager.hybrid-query-expansion.test.tswith 5 test cases covering English, Chinese, and edge cases.Human Verification (required)
pnpm checkclean;pnpm buildclean.Review Conversations
Compatibility / Migration
Risks and Mitigations
extractKeywordsfunction has been used in FTS-only mode since feat: FTS fallback + query expansion for memory search #18304 without issues.AI-assisted PR (developed with Claude). Fully tested locally:
pnpm check✅pnpm build✅pnpm test:extension memory-core✅ (7 pre-existing QMD failures unrelated to this change). Author has reviewed and understands all code changes.Made with Cursor