Repository navigation
fix(recall): make the hybrid path in hooks actually run, and stop hiding it when it doesn't - #21
Merged
Merged
Conversation
…ing it when it doesn't Swapping a hook from recallMemories() to recallMemoriesHybrid() looks like a one-line upgrade. On its own it is a no-op, and nothing says so. _embeddingConfig is only assigned inside initMemory(). A caller that imports index.mjs and calls recall directly goes through getDb() and never touches initMemory(), so _embeddingConfig stays null, recallMemoriesHybrid() takes its fallback branch, and returns plain FTS rows. Measured on the live hook: hybrid and FTS return identical rowids in identical order in ~2ms with zero network, while the embedding endpoint answers fine (HTTP 200, 1024d, 221ms). The hybrid path had never run there. Three changes, because one is not enough: 1. hooks/mneme-recall.mjs — the runner moves into this repo (same reasoning as PR #19 for transcript-sweep.mjs: one live copy, next to the engine it imports). It loads .env.local before calling initMemory(), mirroring mcp-server.mjs, which already carries a comment about this exact failure. 2. recallMemoriesHybrid() no longer degrades silently: it warns once per process per reason and tags the returned array with non-enumerable _degradedTo / _degradeReason, so a caller can assert it is really on the hybrid path instead of assuming. The reason is computed after touching getDb(), since sqlite-vec loads lazily and would otherwise be misreported as unloaded on a process's first call. 3. The hook races hybrid against a budget (MNEME_HOOK_HYBRID_TIMEOUT_MS, default 800ms) and serves the FTS rows it already holds if hybrid overruns. UserPromptSubmit is on the synchronous critical path of every prompt. Measured after the fix, per stage: env 2ms, import 7ms, initMemory 37ms, first FTS query 459-473ms, hybrid 230-419ms. The first FTS query, not hybrid, is the largest single cost — and it is a cost the FTS-only version was already paying. Recall quality changes as expected: all 5 sampled queries return different rows than FTS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DottytheHomeless
commented
Aug 3, 2026
DottytheHomeless
left a comment
Contributor
Author
There was a problem hiding this comment.
Reviewed by 千夏 in Feishu (approved). Post-merge steps in the review were already applied before this PR opened: settings.json points at the repo copy, old hook retired to .retired-20260803.
DottytheHomeless
pushed a commit
that referenced
this pull request
Aug 3, 2026
The 28.3% → 41.7% precision figures behind PR #21 were measured against a sample that was never written to disk. Nothing to re-run, diff, or audit — and the KOS side had shipped a gate requiring exactly that (per-question results plus a set hash) the same morning. The rule was installed in one system while the other ran bare. Rebuilding it turned up three things worth more than the numbers. Machine text is 31% of the eligible population. conversations stores harness-injected content under role='user': task notifications, the per-turn "current reality" block, forwarded group history. 22 of the first 60 sampled questions were this. Those prompts are dense with generic vocabulary — the same thing driving the single-common-word false positives the hook's precision problem is made of — so the earlier measurement was likely taken partly against traffic the hook should never have been judged on. The filter is deliberately narrow: structural markers only, so a person writing about a task notification in their own words stays in. The first split was dead. Ordering by sha256(content) and then splitting on the first byte of that same hash sends everything one way, because the first N of a hash-sorted list all have small leading bytes. It printed dev 60 / held-out 0 while looking healthy: deterministic, hashed, written to disk. Only the split counts gave it away. The split now uses an independently salted hash. And the set can't carry its own text. This repo is public and the sample comes from real conversations; scanning the first build found questions about a nominee-shareholding agreement and someone's exit package. Chasing reproducibility was about to publish them. The committed file now holds hashes, splits, lengths and row ids — no prompt text — and the builder writes the text to a gitignored sibling. The set stays pinned and reproducible; the text stays on the machine. Now: 3475 user rows → 265 pass the gate, 60 picked, dev 31 / held-out 29, set sha256 63530af4, byte-identical across reruns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
Swapping the recall hook from
recallMemories()torecallMemoriesHybrid()reads like a one-line upgrade. On its own it is a no-op, and nothing tells you so._embeddingConfigis only assigned insideinitMemory(). A caller thatimportsindex.mjsand calls recall directly goes throughgetDb()and never touchesinitMemory()→_embeddingConfigstays null →recallMemoriesHybrid()takes its fallback branch at L2007 and returns plain FTS rows.Measured on the live hook, before this PR:
Identical rowids, identical order, 2ms, zero network. The hybrid path had never run in the hook.
mcp-server.mjsL51-57 already carries a comment about this exact failure — the fix just never reached the hook, which lived outside any repo in~/.claude/hooks/.Three changes, because one is not enough
hooks/mneme-recall.mjsmoves into this repo — same reasoning as feat: ship transcript sweep entry point with the engine #19 shippingscripts/transcript-sweep.mjs: one live copy of a runner, next to the engine it imports. It loads.env.localbeforeinitMemory(), mirroringmcp-server.mjs.recallMemoriesHybrid()stops degrading silently — warns once per process per reason, and tags the returned array with non-enumerable_degradedTo/_degradeReasonso a caller can assert it is on the hybrid path instead of assuming. Non-enumerable = invisible toJSON.stringify/ spread / every existing caller. The reason is computed after touchinggetDb(), since sqlite-vec loads lazily and would otherwise be misreported as unloaded on a process's first call.Latency budget — the hook races hybrid against
MNEME_HOOK_HYBRID_TIMEOUT_MS(default 800ms) and serves the FTS rows it already holds if hybrid overruns.UserPromptSubmitsits on the synchronous critical path of every prompt.Measurements after the fix
Per stage, 3 runs:
The first FTS query, not hybrid, is the largest single cost — and the FTS-only version was already paying it. That reframes the latency question: the win from moving the hook to the resident HTTP server (#17) would be larger than anything gained by tuning hybrid alone.
End-to-end through the registered hook path: 707 / 712 / 845 / 875 / 1382ms, all
path=hybrid, against a 6s hook timeout. Timeout fallback verified withMNEME_HOOK_HYBRID_TIMEOUT_MS=1→path=fts-timeout, still injects FTS rows rather than nothing.Recall quality changes as expected — 5/5 sampled queries return different rows:
Migration
~/.claude/settings.jsonnow points atE:/project/mneme/hooks/mneme-recall.mjs; the old~/.claude/hooks/mneme-recall.mjsis retired to.retired-20260803rather than left in place — an idle second copy of a runner is exactly how #19/#20 drifted within three hours.Known, not addressed here
CUR_PROJECT_MEM_DIRis still hardcoded toE--project, carried over from v0.2. Wrong for a public repo, but making it configurable in this PR would mix a behaviour change into a fix — flagging for a follow-up.🤖 Generated with Claude Code