[AI-assisted] Fix memory_grep default session recall - #342
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ChangesMessage grep deduplication and collection-aware search
Sequence Diagram(s)(Skipped — changes are focused and the flow is simple enough to follow from the summaries.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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. 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
🤖 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/tools/memory-recall.ts`:
- Around line 209-215: The function buildMessageGrepCollections always pushes
`session_raw:${sessionId}` even when sessionId is empty, causing malformed
collection names; update buildMessageGrepCollections to guard `session_raw` the
same as `session:${sessionId}` (or return/throw early when sessionId is empty)
so both collections are only added when sessionId.length > 0; locate and modify
buildMessageGrepCollections and ensure this aligns with how `memory_grep`
derives sessionId (readStr/getSessionId) to avoid querying `session_raw:`.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3b3260f-1dd9-4d6b-9490-58b9bdddee14
📒 Files selected for processing (2)
src/tools/memory-recall.tstest/unit/memory-recall.test.ts
compoodment
left a comment
compoodment
left a comment
There was a problem hiding this comment.
Vale Review
Verdict: request changes. The rewrite fixes the big #303 problems: summary/message searches stay independent, dedup now happens after exact/regex filtering, duplicate turn IDs keep the higher score, and the code no longer depends on collection provenance from metadataJson. That part is solid.
Finding
- Major: non-object metadata can now make
memory_grepdrop all message results.
parseGrepMetadata() returns the parsed JSON value cast as Record<string, unknown> (src/tools/memory-recall.ts:193-201), but it does not verify that the value is actually a non-null object. readGrepRole() then immediately reads meta.role (src/tools/memory-recall.ts:204-206). If the daemon returns syntactically valid but non-object metadata such as JSON null, that property access throws.
That is a regression from the previous inline metadata parsing, where the try covered both JSON.parse(...) and the meta.role read, so malformed/non-object metadata merely fell back to role: "unknown".
I reproduced this against the PR branch with a fake matching turn:
metadataJson = TextEncoder().encode("null")
WARN memory_grep failed: Cannot read properties of null (reading 'role')
{"pattern":"needle","mode":"text","totalMatches":0,"summaries":[],"turns":[],"truncated":false}
That means one weird metadata payload can make memory_grep(scope="messages") return zero results even though the text matched. Since metadata parsing is explicitly best-effort everywhere else in this file, this helper should preserve that behavior.
Fix direction: make parseGrepMetadata() return {} unless the parsed value is a non-null, non-array object, or make readGrepRole() guard before reading role. Add a regression test with metadataJson set to JSON null and assert the matching turn is still returned with role: "unknown".
Verification
I ran:
corepack pnpm@9 exec tsc -p tsconfig.tests.json
node --test .ts-build/test/unit/memory-recall.test.js
git diff --check upstream/main..review/pr-342
Current added tests pass, but they do not cover this non-object metadata case.
compoodment
left a comment
compoodment
left a comment
There was a problem hiding this comment.
Re-reviewed latest head e00b029 after @vale review.
Verdict: approve. The previous blocker is fixed: parseGrepMetadata() now ignores non-null/non-array violations by returning {}, readGrepRole() falls back to unknown, and the regression test with JSON null metadata keeps the matching turn instead of dropping results.
I also checked the session collection behavior while I was here. Empty session IDs now avoid malformed message collection queries, memory_grep(scope="messages") checks both raw and default session collections when a session is present, exact filtering happens before dedup, and duplicate turn IDs keep the higher score.
Verification run:
corepack pnpm@9 exec tsc -p tsconfig.tests.json
node --test .ts-build/test/unit/memory-recall.test.js
git diff --check origin/main...HEAD
Result: 12/12 focused unit tests passed. CodeRabbit's latest check is also passing. I did not run host-backed daemon tests for this review.
– Vale
2 participants
Summary
memory_grepsearchedsession_raw:<id>for message scope but missed exact active-session hits stored in LibraVDB's defaultsession:<id>recall collection.session:<id>collection as part of the message grep bucket while keeping summary and message searches independent.memory_grepnow queriessession_raw:<id>andsession:<id>separately for message results when a session ID is available, skips malformed empty-session message collection names, treats non-object grep metadata as missing metadata, filters exact/regex matches before deduplication, and keeps the highest-scored duplicate turn ID.memory_search, ormemory_expandbehavior changed.Motivation
memory_grepis the exact-match fallback for active-session recall. Currentmainalready uses the defaultsession:<id>collection for normal session recall, butmemory_grep(scope="messages")still searched onlysession_raw:<id>. That made exact grep unreliable for active-session text indexed in the default session collection.Change Type
Scope
Linked Issue/PR
mainand addresses the review feedback about mixed top-k, scope, provenance, and deduplication.Real behavior proof
memory_grep(scope="messages")now finds exact active-session hits stored under the defaultsession:<id>collection.mainat96b7694plus this patch through heade00b029, using Nodev24.16.0andpnpm@9.15.9.session:<id>, empty session IDs do not query malformed message collections likesession_raw:, syntactically valid non-object message metadata falls back torole: "unknown"without dropping the hit,scope="both"preserves independent summary/message budgets, non-matching duplicate IDs no longer suppress matching hits, and duplicate matching turn IDs keep the highest score.Root Cause
memory_grephard-coded the experimental message collectionsession_raw:<id>but did not include the default session recall collectionsession:<id>.Regression Test Plan
test/unit/memory-recall.test.tsmemory_grep(scope="messages")searches bothsession_raw:<id>and defaultsession:<id>for non-empty sessions, returns a hit that exists only in the default session collection, does not query malformed message collections when no session ID is available, and keeps matching turns whenmetadataJsonparses to a non-object such as JSONnull.User-visible / Behavior Changes
memory_grepcan now find exact active-session text stored in the default session recall collection. Existing summary search and raw-message search behavior remain available.Diagram
Security Impact
SearchTextrequest for the active session's defaultsession:<id>collection.memory_grepwith the same active-session collection already used by LibraVDB session recall.Yes, explain risk + mitigation: The additional lookup is limited to a non-empty current active session ID and covered by unit tests that assert summary scope does not querysession:<id>and empty message scope does not querysession_raw:.Repro + Verification
Environment
v24.16.0,pnpm@9.15.9Steps
mainat96b7694.pnpm@9and a throwaway store.tsc -p tsconfig.tests.json.memory-recalltests.Expected
memory_grep(scope="summaries")searches onlysession_summary:<id>.memory_grep(scope="messages")searchessession_raw:<id>andsession:<id>.memory_grep(scope="messages")with no session ID does not query malformed message collections.role: "unknown"without dropping matching turns.memory_grep(scope="both")keeps independent summary and message result budgets.Actual
session_summary:<id>.session_raw:lookup.metadataJsonset to JSONnullreturned withrole: "unknown".scope="both"returned one summary and one message withlimit=1.Evidence
Human Verification
scope="both"budgets, dedup after exact matching, highest-score duplicate selection, role fallback from metadata, targeted recall tools, adjacent memory/runtime/before-turn units, TypeScript, production build, and whitespace diff check.nullmetadata, and no dependency/lockfile changes.Review Conversations
CodeRabbit's PR #342 review thread about empty
sessionIdmessage collection names is addressed by commitad33283and was resolved bycoderabbitai[bot]after re-review. No human GitHub review reply or manual thread resolve was posted. The code also addresses the prior #303 top-level review feedback about mixed top-k, summaries scope, dedup-before-filtering, duplicate selection, and collection provenance.The Vale/compoodment request-changes review about non-object
metadataJsonis addressed by commite00b029. No human GitHub review reply or manual thread resolve was posted; reviewer re-review is still needed.Compatibility / Migration
Risks and Mitigations
session:<id>is never queried forscope="summaries".metadataJson.collection; the code knows which bucket it is searching.nullcould throw while readingroleand drop matching message results.{}androle: "unknown", covered by regression test.Summary
This PR fixes a correctness bug in
memory_grep(scope="messages")where active-session message hits stored in the defaultsession:<id>collection were missed because only the experimentalsession_raw:<id>collection was queried.Changes
src/tools/memory-recall.tsAlgorithmic Changes:
session_raw:${sessionId}) with dual-collection queries via new helperbuildMessageGrepCollections(sessionId), which returns["session_raw:${sessionId}", "session:${sessionId}"]for non-empty session IDs or[]for empty ones (guards malformed collection names).Map<turnId, {turnId, snippet, role, score}>to deduplicate results perturnId, retaining the highest-scored duplicate when the same turn appears in both collections.safeMatch()is applied before deduplication, ensuring only exact matches contribute to the deduped set.parseGrepMetadata()- Safely decodes and validates metadata JSON; returns empty object for malformed/non-object metadata.readGrepRole()- Extractsrolefield from parsed metadata with "unknown" fallback.buildMessageGrepCollections()- O(1), returns 0 or 2 collection names.Complexity Analysis:
Big O: Worst-case remains O(k·L + k log k) where k = searchK ≈ min(limit·3, 200) and L = average text snippet length. The sorting of deduped results (
dedupedTurns.sort()) dominates. However, number of SearchText requests increases from 1 to 2 (constant factor of 2× for active session).safeMatch(), O(L) fortruncateSnippet(), O(M) for metadata parse (M = metadata size), O(1) for Map operationsCyclomatic Complexity: No regression. Loop structure (for each of 2 collections → filter results → dedup → sort) adds ~1 conditional branch but maintains baseline complexity of ~6–7 (comparable to original ~5–6).
Space Complexity: O(k) for
bestTurnByIdMap and O(k) for sorted results—no regression vs. single-collection approach.Mitigations for 2× search overhead:
safeMatch()is applied per result before Map insertion, reducing actual objects stored from 400 (2×200) to only those matching the pattern.test/unit/memory-recall.test.tsCoverage Additions:
session_raw:andsession:lookups).session_raw:active-sessionandsession:active-session, validating metadata decoding (role extraction with "unknown" fallback).null) is treated as missing and role defaults to "unknown".scope="both"maintains separate per-collection result limits for summaries and messages (e.g., limit:1 can yield 1 summary + 1 message).turnIdappears in multiple collections, the higher-scored variant is retained.Test Infrastructure:
FakeSearchResultinterface with optionalmetadataJson: Uint8Array.CollectionRecallClientsubclass ofFakeRecallClientto return per-collection results based on thecollectionparameter.encodeMetadata()helper for test fixture construction.Known Risks & Limitations
session:collections is untested in production.Unaffected
memory_search,memory_expand,memory_describetools and their prompt guidance.session_summary:<id>queried only forscope="summaries"orscope="both").