fix(memory): use per-keyword FTS search in hybrid mode #39484#73976
Conversation
openclaw-barnacle
Bot
added
extensions: memory-core
|
Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit. |
Greptile SummaryThis PR refactors the per-keyword FTS fallback for hybrid memory search into a dedicated Confidence Score: 5/5Safe to merge; no new P1/P0 issues introduced by this PR. All findings are P2. The structural refactor is clean and the fan-out cap is correctly enforced. The relaxedMinScore = 0 concern was flagged in a prior review. Only a minor test-reliability observation remains. No files require special attention beyond the pre-existing relaxedMinScore = 0 concern in manager.ts. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/memory-core/src/memory/index.test.ts
Line: 420-436
Comment:
**`db.prepare` spy measures an implementation detail, not query executions**
The fan-out bound assertion relies on counting how many times `db.prepare(sql)` is called with an FTS SQL pattern. If `searchKeyword` (or its underlying helper) ever starts caching prepared statements — a natural performance improvement for `better-sqlite3` — the same `Statement` object is reused across invocations without calling `prepare` again. In that case `ftsSelects` stays at 1 even when 7 queries actually execute, making `expect(ftsSelects).toBeGreaterThan(1)` a false failure and `expect(ftsSelects).toBeLessThanOrEqual(7)` trivially true. Spying on a lower-level execution hook (`.all` / `.get` on the prepared statement) or using an integration assertion (e.g. counting distinct results per term) would be more durable.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "fix(memory): use per-keyword FTS search ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
openclaw-barnacle
Bot
removed
the
r: too-many-prs
|
Codex review: needs changes before merge. Reviewed June 15, 2026, 2:37 AM ET / 06:37 UTC. Summary PR surface: Source +29, Tests +40, Docs +1. Total +70 across 3 files. Reproducibility: yes. at source level, but I did not execute tests in this read-only review. Current main and v2026.6.6 still run only the full AND-joined keyword query in provider-backed hybrid mode, while related reports show indexed terms missed until per-keyword lexical broadening is used. Review metrics: 2 noteworthy metrics.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Port the bounded provider-backed per-keyword FTS fallback onto current main, integrate cleanly with the current hybrid selection code, drop the changelog edit, and validate memory-core recall/fan-out behavior before merge. Do we have a high-confidence way to reproduce the issue? Yes at source level, but I did not execute tests in this read-only review. Current main and v2026.6.6 still run only the full AND-joined keyword query in provider-backed hybrid mode, while related reports show indexed terms missed until per-keyword lexical broadening is used. Is this the best way to solve the issue? Yes in direction, but not as submitted. The bounded fallback belongs in memory-core's manager path, but this branch needs a current-main port, changelog removal, and focused ranking/fan-out validation before it is the best mergeable fix. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against bcb016a528b1. Label changesLabel justifications:
Evidence reviewedPR surface: Source +29, Tests +40, Docs +1. Total +70 across 3 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
This pull request has been automatically marked as stale due to inactivity. |
clawsweeper
Bot
added
the
rating: 🌊 off-meta tidepool
|
This pull request has been automatically marked as stale due to inactivity. |
clawsweeper
Bot
added
rating: 🦐 gold shrimp
610b9c2 to
df43337
Compare
…openclaw#73976) Co-authored-by: openclaw-clownfish[bot] <280122609+openclaw-clownfish[bot]@users.noreply.github.com>
…openclaw#73976) Co-authored-by: openclaw-clownfish[bot] <280122609+openclaw-clownfish[bot]@users.noreply.github.com>
1 participant
Canonical repair path for #39555. Before merge, rebase or refresh the contributor branch against main 79159f1 if needed, verify the bounded per-keyword hybrid FTS fan-out remains narrow, rerun Codex /review and address every finding, then run pnpm check:changed. This preserves @yangzi33's source PR credit and keeps the existing changelog attribution.
ProjectClownfish replacement details:
! [remote rejected] HEAD -> yangzi33/memory_search-0hit-fix (refusing to allow a GitHub App to create or update workflow
.github/workflows/ci-build-artifacts-testbox.ymlwithoutworkflowspermission)error: failed to push some refs to 'https://github.com/yangzi33/openclaw.git'