fix: surface saved memories in smart_search/recall (#265)#269
Merged
Conversation
mem::remember persists memories to KV.memories under a synthetic
sessionId ("memory" or memory.sessionIds[0]). The BM25 index sees that
synthetic sessionId, but KV.observations under it never has an entry —
so both enrichment paths silently dropped every memory hit:
- HybridSearch.enrichResults (powers /smart-search, memory_smart_search)
- mem::search obsResults map (powers /search, memory_recall)
Result: memory_save succeeds and the memory shows up in the viewer and
GET /memories, but every search/recall returns []. The save→search flow
was effectively write-only.
Both sites now try KV.observations first, then fall back to KV.memories
and coerce the Memory record into a CompressedObservation. Verified live
end-to-end against an iii-engine 0.11.2 stack: prior to the fix
/smart-search and /search both returned [] for a saved memory; after the
fix the memory surfaces with score > 0 in both responses.
Regression tests added to test/hybrid-search.test.ts and test/search.test.ts.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds memoryToObservation and updates enrichment and indexing paths so searches fall back from KV.observations to KV.memories (converted to observation shape), plus tests and test setup changes to validate the fallback. ChangesMemory Search Fallback
Sequence Diagram(s)sequenceDiagram
participant Client
participant HybridSearch
participant KV_Observations
participant KV_Memories
Client->>HybridSearch: search(query)
activate HybridSearch
HybridSearch->>KV_Observations: lookup observation (sessionId, obsId)
KV_Observations-->>HybridSearch: not found / null
HybridSearch->>KV_Memories: lookup memory (obsId)
KV_Memories-->>HybridSearch: memory record / null
alt memory found
HybridSearch->>HybridSearch: memoryToObservation convert
HybridSearch-->>Client: enriched result
else not found
HybridSearch-->>Client: no enriched result (filtered)
end
deactivate HybridSearch
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/functions/search.ts (3)
9-14: ⚡ Quick winRemove comment explaining WHAT the code does.
The comment explains what the helper does rather than why. The function name
memoryAsIndexableand the return typeCompressedObservationalready make the purpose clear. As per coding guidelines, avoid code comments explaining WHAT — use clear naming instead.♻️ Proposed fix
-// Memories share the same searchable fields as observations (title + -// content + concepts + files), so we wrap them in the observation shape -// before indexing. Type is normalized to "decision" to keep memories -// distinguishable in result metadata. Mirrors the helper in -// functions/remember.ts; kept inline here to avoid a circular import -// (remember.ts imports from this file). function memoryAsIndexable(memory: Memory): CompressedObservation {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/functions/search.ts` around lines 9 - 14, Remove the explanatory block comment that describes what the helper does; instead rely on the clear function name memoryAsIndexable and its return type CompressedObservation. Locate the comment immediately above the memoryAsIndexable helper and delete those comment lines so only the function and its signature remain (do not change memoryAsIndexable implementation or the CompressedObservation type).
167-170: ⚡ Quick winRemove comment explaining WHAT the code does.
The comment explains what the fallback logic does rather than documenting why it's needed. As per coding guidelines, avoid code comments explaining WHAT — use clear naming instead.
♻️ Proposed fix
- // Second pass: load observations in parallel. Fall back to - // KV.memories when the observation lookup misses — entries indexed - // via mem::remember live in the memories scope under a synthetic - // sessionId, so the observation key never exists (`#265`). const obsResults = await Promise.all(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/functions/search.ts` around lines 167 - 170, Remove the explanatory "Second pass: load observations..." comment in src/functions/search.ts and delete any similar comments that describe WHAT the code does; instead, if rationale is needed, add a brief WHY note referencing the underlying bug (e.g., "#265") above the fallback logic that mentions only the reason (observations may be indexed under a synthetic sessionId) and keep variable/function names (KV.memories, mem::remember, sessionId, observation lookup) clear and self-descriptive so the code itself documents the behavior.
43-46: ⚡ Quick winRemove comment explaining WHAT the code does.
The comment explains what the separate walk does rather than why it's necessary. As per coding guidelines, avoid code comments explaining WHAT — use clear naming instead.
♻️ Proposed fix
- // Memories live in their own KV scope outside per-session observation - // scopes, so they need a separate walk. Without this, mem::remember - // entries vanish from BM25 on every restart even after the live-write - // fix in remember.ts (`#257`). try {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/functions/search.ts` around lines 43 - 46, Remove the existing WHAT-focused comment that explains how the separate walk works and either delete it or replace it with a brief WHY-focused note that explains the necessity: that memories are stored in a separate KV scope and therefore require a distinct walk to ensure mem::remember entries are preserved in the BM25 index across restarts (referenced in remember.ts), keeping the code intent clear without describing internal mechanics; update the comment near the separate-walk code in search.ts to state that this is required to maintain persistent BM25 entries for the memory KV scope, and ensure naming and function calls (e.g., mem::remember, BM25) remain self-explanatory in the code itself.src/state/hybrid-search.ts (1)
296-299: ⚡ Quick winRemove comment explaining WHAT the code does.
The comment explains what the fallback logic does rather than why it's necessary. As per coding guidelines, avoid code comments explaining WHAT — use clear naming instead.
♻️ Proposed fix
if (obs) return obs; - // Fallback: indexed entry may originate from mem::remember, which - // writes to KV.memories with a synthetic sessionId ("memory" or the - // memory's first associated session). Coerce the Memory record into - // a CompressedObservation so search/recall surface saved memories. const mem = await this.kv🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/state/hybrid-search.ts` around lines 296 - 299, Remove the explanatory comment block that describes what the fallback does and instead make the code self-documenting: delete the multi-line comment mentioning mem::remember, KV.memories, synthetic sessionId, and the coercion into CompressedObservation; if clarity is needed rename the local variable(s) and helper(s) involved in the fallback (e.g., the variable that holds the coerced object and any helper function used for coercion such as the CompressedObservation construction or a helper like coerceToCompressedObservation) so the intent is clear from names alone; ensure any remaining brief comment (if absolutely necessary) states only WHY the fallback exists, not WHAT it does.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/state/hybrid-search.ts`:
- Around line 325-338: The memoryToObservation implementation duplicates
memoryAsIndexable; extract a single helper (e.g., memoryToObservation) into a
new shared module (suggested name: memory-utils.ts) and have both
src/state/hybrid-search.ts and src/functions/search.ts import and call that
helper instead of their local implementations; specifically, move the existing
conversion logic (fields id, sessionId from sessionIds[0] ?? "memory",
timestamp, type:"decision", title, facts:[content], narrative:content, concepts,
files, importance) into the shared function and replace both local functions
memoryToObservation and memoryAsIndexable with imports from the new module.
---
Nitpick comments:
In `@src/functions/search.ts`:
- Around line 9-14: Remove the explanatory block comment that describes what the
helper does; instead rely on the clear function name memoryAsIndexable and its
return type CompressedObservation. Locate the comment immediately above the
memoryAsIndexable helper and delete those comment lines so only the function and
its signature remain (do not change memoryAsIndexable implementation or the
CompressedObservation type).
- Around line 167-170: Remove the explanatory "Second pass: load
observations..." comment in src/functions/search.ts and delete any similar
comments that describe WHAT the code does; instead, if rationale is needed, add
a brief WHY note referencing the underlying bug (e.g., "#265") above the
fallback logic that mentions only the reason (observations may be indexed under
a synthetic sessionId) and keep variable/function names (KV.memories,
mem::remember, sessionId, observation lookup) clear and self-descriptive so the
code itself documents the behavior.
- Around line 43-46: Remove the existing WHAT-focused comment that explains how
the separate walk works and either delete it or replace it with a brief
WHY-focused note that explains the necessity: that memories are stored in a
separate KV scope and therefore require a distinct walk to ensure mem::remember
entries are preserved in the BM25 index across restarts (referenced in
remember.ts), keeping the code intent clear without describing internal
mechanics; update the comment near the separate-walk code in search.ts to state
that this is required to maintain persistent BM25 entries for the memory KV
scope, and ensure naming and function calls (e.g., mem::remember, BM25) remain
self-explanatory in the code itself.
In `@src/state/hybrid-search.ts`:
- Around line 296-299: Remove the explanatory comment block that describes what
the fallback does and instead make the code self-documenting: delete the
multi-line comment mentioning mem::remember, KV.memories, synthetic sessionId,
and the coercion into CompressedObservation; if clarity is needed rename the
local variable(s) and helper(s) involved in the fallback (e.g., the variable
that holds the coerced object and any helper function used for coercion such as
the CompressedObservation construction or a helper like
coerceToCompressedObservation) so the intent is clear from names alone; ensure
any remaining brief comment (if absolutely necessary) states only WHY the
fallback exists, not WHAT it does.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54f01ecc-23f3-4ac8-86f8-69b2791cceab
📒 Files selected for processing (4)
src/functions/search.tssrc/state/hybrid-search.tstest/hybrid-search.test.tstest/search.test.ts
Three copies of the Memory→CompressedObservation conversion existed: hybrid-search.ts (added in #265 fix), search.ts, remember.ts. Consolidate into src/state/memory-utils.ts and import from all three callsites. Behavior unchanged. Full suite: 854 passing.
Merged
rohitg00
added a commit
that referenced
this pull request
May 10, 2026
Three reliability fixes from #269/#270/#271: - search/recall surfaces saved memories (closes #265) - MCP shim proxies full server tool set (closes #234) - session/subagent hooks no longer block startup (closes #221) Also fixes packages/mcp version drift — was stuck at 0.9.4 through v0.9.5, now lockstepped with main.
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.
Summary
memory_savesucceeded butmemory_smart_search/memory_recallalways returned[]— the save→search flow was effectively write-only.KV.observations(sessionId, obsId), whilemem::rememberwrites toKV.memoriesunder a syntheticsessionId.KV.memoriesand coerce theMemoryinto aCompressedObservation.Affected paths
HybridSearch.enrichResults→ powers/smart-search, MCPmemory_smart_searchmem::searchobsResultsmap → powers/search, MCPmemory_recallThe first site was the one called out in the issue. The second was found while validating end-to-end and has the same bug — without it
memory_recallwould still be broken even after the smart-search fix.Live e2e (iii-engine 0.11.2 + agentmemory 0.9.5,
EMBEDDING_PROVIDER=local)Pre-fix:
Post-fix:
Tests
test/hybrid-search.test.ts— new regression case: BM25 indexes a memory,enrichResultsfalls back toKV.memories.test/search.test.ts— new regression case:mem::searchsurfaces memories saved toKV.memories.Closes #265.
Summary by CodeRabbit
Bug Fixes
New Features
Tests