fix: make proactive recall reliably deliver value (5 root-cause fixes)#287
Merged
khustup2 merged 6 commits intoJun 25, 2026
Merged
Conversation
…shold RECALL_LOSSY: the injected hint carried only the gist 'description' (derived from ## What Happened), which by design drops exact identifiers /values. Now select the full 'summary' and inject an excerpt built from the high-signal fact sections (## Key Facts / ## Decisions / ## Entities) that hold verbatim, non-derivable values; description is the fallback for legacy rows. Strengthen WIKI_PROMPT_TEMPLATE so What Happened + Key Facts preserve exact values verbatim. RECALL_NOT_FIRED: lower the semantic cosine bar 0.55 -> 0.50 (query vs long document embeddings routinely land in 0.50-0.55 even on clear hits), operator-tunable via HIVEMIND_RECALL_THRESHOLD (bounded 0<t<=1). Backward-compatible: summary is optional on RecallHit; lexical path already searched summary. Tests: +12 recall cases, full claude-code + recall suites green (2822).
…mary A finalized summary row (real summary body + non-placeholder description) must never be overwritten by a SessionStart placeholder or a content-free stub. In production ~56% of summaries on the activeloop memory table were stuck at description='in progress', invisible to proactive recall, because uploadSummary did a blind UPDATE-by-path: a stale/duplicate writer (resumed session, or a late wiki worker that produced empty/content-free text) could downgrade an already-finalized row back to a stub. uploadSummary now fetches the existing summary+description and skips the write when the incoming text is not a real summary (no populated '## What Happened' section) AND the existing row is already finalized. extractDescription's 'completed' fallback is not a reliable finalized signal, so the guard keys on the populated section instead. Backward compatible: INSERT path and finalized->finalized refresh (resumed sessions) are unchanged; only stub->finalized downgrades are blocked.
…dding coverage Production memory table: only 25% of summaries carry a summary_embedding, so proactive recall silently degrades to weak lexical matching for ~75% of the corpus. A large share of those NULLs are ENABLED users whose embed daemon was cold at the single moment the wiki-worker finalized the summary. The wiki-worker called EmbedClient.embed() directly. That call is tuned for the latency-critical capture hook: on a cold daemon it returns null immediately while spawning in the background (fire-and-forget). Since no backfill ever revisits a NULL row, one cold-start race stranded the summary in lexical-only mode permanently. The wiki-worker is a detached background process, not latency-critical, so it can afford to warm the daemon and retry. Add embedSummaryWithWarmup() (warmup -> embed -> one retry, never throws) and use it in the claude-code wiki-worker. No change to the hot capture path or the opt-in semantics (callers still gate on embeddingsDisabled()); graceful NULL fallback when the daemon truly cannot be reached.
… of a session doesn't miss semantic recall
… recycle race); update EmbedClient test mock
… finalized summaries to 'in progress' stubs
Root cause of the production clobber (~56% of summaries stuck at 'in
progress', ~75% with NULL embeddings): the memory table has NO unique
constraint on `path` (deeplake-schema.ts MEMORY_COLUMNS). createPlaceholder
used a SELECT-then-INSERT guard:
SELECT path ... WHERE path = $p LIMIT 1
if (rows.length === 0) INSERT placeholder
This is a TOCTOU race. Deeplake reads are eventually-consistent, so a
second SessionStart for the SAME session id (a --resume, a source:
resume|clear re-fire, or two near-simultaneous SessionStart invocations)
can read ZERO rows even though the wiki worker finalized + embedded the row
seconds earlier. The guard passes and a SECOND, stub placeholder row is
INSERTed at the same path. Downstream reads use `... WHERE path=$p LIMIT 1`
with no ORDER BY, so the duplicate `description='in progress',
summary_embedding=NULL` stub shadows the finalized row — recall silently
drops it. This path bypasses uploadSummary's finalize-wins guard entirely.
Fix: extract the placeholder write into a shared helper
(src/hooks/shared/placeholder-summary.ts) used by all four agent variants
(claude-code, codex, cursor, hermes). The write is now a single atomic,
finalize-aware statement:
INSERT INTO t (...) SELECT <values>
WHERE NOT EXISTS (SELECT 1 FROM t WHERE path = $p)
The existence check and insert happen in one server-side statement, so no
stale client-side read can wedge a duplicate in between. If ANY row already
exists at the path (placeholder OR finalized), the INSERT writes nothing — a
finalized row can never be reverted to a stub or have its embedding nulled
by a placeholder write. The fast-path SELECT is kept as a logging/skip
optimization but is no longer the safety boundary; a failed SELECT no longer
blocks the (race-safe) write.
Also de-duplicates four near-identical createPlaceholder copies.
Regression test (tests/claude-code/placeholder-summary.test.ts): a
finalized+embedded row survives a subsequent placeholder/SessionStart write,
a STALE-READ SessionStart, and a second concurrent worker pass. Red before
(plain VALUES insert appends a duplicate stub), green after.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Contributor
Author
|
Superseded by #288. The root cause of unreliable recall was a pg-deeplake read-consistency bug (in-place UPDATE'd summary rows reading back stale), fixed separately on the backend. With that fixed, #288 carries the minimal, non-embedding improvements that help every path including the no-embeddings (lexical) default — finalize-wins, atomic placeholder, and recall content fidelity. The embedding-specific changes here (recall warm-daemon, embedding-coverage warm+retry, model-swap) are deferred to separate branches pending a decision to prioritize the semantic path. Closing this in favor of #288. |
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
1 participant
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.
Make proactive recall actually deliver value (5 fixes)
Built an isolated-Docker A/B harness that measures whether Hivemind's
cross-session memory changes a second session's answer. It surfaced that recall
rarely fires in practice, and production confirms it: of 4,732 real
session summaries, only 11.5% are recall-eligible — 56% stuck
description='in progress'(never finalized) and only 25% embedded. ThisPR fixes the five root causes. After them, the A/B value-win rate goes 1/9 →
5/9, "recall fired but answered wrong" goes 80% → 0%, and recall firing on
a cross-person demo goes 1/3 → 3/3.
Stacked on
feat/proactive-recall(the diff is exactly these 5 commits).The fixes
recall-format/gate/query.ts,spawn-wiki-worker.ts) —inject a verbatim
## Key Factsexcerpt (not just the gistdescription);preserve exact identifiers in the summary prompt; threshold 0.55→0.50
(env-tunable). Kills the "recall fired but dropped the exact fact" failure.
upload-summary.ts) — a placeholder/empty write can nolonger downgrade a finalized summary back to a stub.
embed-summary.ts,wiki-worker.ts) — warm the embeddaemon + retry on finalize so a cold daemon no longer strands
summary_embedding = NULLforever (there was no backfill).recall.ts) — warm the daemon + retry the queryembed so the first prompt of a session doesn't miss semantic recall
(cold-daemon race); budget 1000→1500ms, still under the 2s hook timeout.
shared/placeholder-summary.ts+ 4 session-startvariants) — the root blocker:
createPlaceholderdid SELECT-then-INSERTand the
memorytable has no unique constraint onpath, so a stale read ona second SessionStart inserted a duplicate
in progressstub that shadowedthe finalized row (unordered
LIMIT 1reads). Replaced with an atomicINSERT … SELECT … WHERE NOT EXISTS. This is the mechanism behind the 56%stuck-in-progress production stat.
Tests
Each fix is red→green with unit tests (vitest): finalize-wins, embed-summary
warmup/retry, recall fidelity/threshold, placeholder revert-race (incl. the
stale-read duplicate case), and an updated EmbedClient mock. Target suites green;
remaining whole-suite failures are pre-existing env issues (tree-sitter native
addons, network smoke tests) unrelated to these changes.
Validation
Measured against the live backend in isolated containers (one per
claudesession; continuity only through Deeplake). Reproducible harness + before/after
demo cards available separately.
🤖 Generated with Claude Code