[AI-assisted] Fix memory_grep default session recall - #303
Conversation
|
Warning Review limit reached
More reviews will be available in 13 minutes and 8 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR refactors ChangesMulti-collection grep search
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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. ✨ 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 |
![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 (2)
test/unit/memory-recall.test.ts (1)
126-147: ⚡ Quick winAdd a duplicate-hit regression for the new multi-collection path.
This test proves
session:active-sessionis searched and classified as a turn, but it never exercises the new dedup contract from the PR. A case where the sameidappears in bothsession_raw:active-sessionandsession:active-sessionwould lock in the intendedcollection:iddedup behavior and catch accidental over-deduping between raw/default-session hits.🤖 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 `@test/unit/memory-recall.test.ts` around lines 126 - 147, The test doesn't exercise the new deduplication when the same id appears in both collections; update the unit test for DefaultSessionRecallClient/createMemoryGrepTool to mock a duplicate hit with the same id present in both "session_raw:active-session" and "session:active-session" (via the client’s returned hits used by tool.execute) and assert the dedup contract: client.calls still requested both collections, details.totalMatches remains 1, and the single returned turn corresponds to that id (ensure the returned turn's collection:id or turnId matches the deduped identifier and the snippet/role/score remain as expected).src/tools/memory-recall.ts (1)
520-533: 💤 Low valueMinor: collapse the declare-then-reassign pairs.
evictionCue(520-521) androle(532-533) are declared and then immediately overwritten on the next line; assign once.Proposed change
- let evictionCue: string | undefined; - evictionCue = typeof meta.eviction_cue === "string" ? meta.eviction_cue : undefined; + const evictionCue = typeof meta.eviction_cue === "string" ? meta.eviction_cue : undefined;- let role = "unknown"; - role = typeof meta.role === "string" ? meta.role : "unknown"; + const role = typeof meta.role === "string" ? meta.role : "unknown";🤖 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/tools/memory-recall.ts` around lines 520 - 533, Collapse the declare-then-reassign pairs by initializing variables in one statement: replace the separate declaration and subsequent conditional assignment for evictionCue with a single initialization using the existing ternary (e.g., let evictionCue = typeof meta.eviction_cue === "string" ? meta.eviction_cue : undefined;) and do the same for role (e.g., let role = typeof meta.role === "string" ? meta.role : "unknown;") in the same blocks where summaries.push and snippet/role are set so you don't declare then immediately overwrite these variables.
🤖 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 255-262: In isSummaryResult, tighten the summary-ID check by
matching the explicit "sum_" prefix instead of the loose startsWith("sum")
check; update the condition that currently uses result.id.startsWith("sum") to
require result.id.startsWith("sum_") (or an equivalent anchored regex) so only
IDs in the sum_xxx format are classified as summaries.
---
Nitpick comments:
In `@src/tools/memory-recall.ts`:
- Around line 520-533: Collapse the declare-then-reassign pairs by initializing
variables in one statement: replace the separate declaration and subsequent
conditional assignment for evictionCue with a single initialization using the
existing ternary (e.g., let evictionCue = typeof meta.eviction_cue === "string"
? meta.eviction_cue : undefined;) and do the same for role (e.g., let role =
typeof meta.role === "string" ? meta.role : "unknown;") in the same blocks where
summaries.push and snippet/role are set so you don't declare then immediately
overwrite these variables.
In `@test/unit/memory-recall.test.ts`:
- Around line 126-147: The test doesn't exercise the new deduplication when the
same id appears in both collections; update the unit test for
DefaultSessionRecallClient/createMemoryGrepTool to mock a duplicate hit with the
same id present in both "session_raw:active-session" and
"session:active-session" (via the client’s returned hits used by tool.execute)
and assert the dedup contract: client.calls still requested both collections,
details.totalMatches remains 1, and the single returned turn corresponds to that
id (ensure the returned turn's collection:id or turnId matches the deduped
identifier and the snippet/role/score remain as expected).
🪄 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: 18d30e9f-6496-41c1-8f98-9baf7df8b54a
📒 Files selected for processing (2)
src/tools/memory-recall.tstest/unit/memory-recall.test.ts
Vale ReviewVerdict: request changes. The bug target is real: Findings
Then This also weakens Fix direction: preserve bucket-specific search budgets. Search summaries and messages/default-session separately, or otherwise guarantee per-bucket/per-collection quotas before combining results.
In That is especially dangerous because Fix direction: classify and
The new duplicate test actually exposes the problem: If duplicates are genuinely the same memory projected into two collections, keep the better candidate deterministically: highest score, or an explicit collection preference if raw/default-session should win. “First item returned by daemon” is not a stable semantic rule.
The generated contract for
The tests inject Fix direction: either document and test the daemon invariant that
Add tests for:
Positive notesThe regression target is valid, and adding default CI / verification stateGitHub reports this PR as mergeable, but I found no GitHub Actions runs for head |
2 participants
Summary
memory_greponly searched the experimentalsession_summary:<id>andsession_raw:<id>collections, so it could miss active-session recall stored in LibraVDB's defaultsession:<id>collection.session:<id>collection inmemory_grepcollection selection while preserving the summary/raw split when those collections exist.memory_grepnow searches a scoped collection set withsearchTextCollectionswhen needed, classifies summary vs turn hits from collection/metadata, and deduplicates results.memory_search, ormemory_expandbehavior changed.Motivation
memory_grepis the exact-match fallback for active-session recall. After the recall tools were exposed, it still did not search the same default active-session collection used by normal session recall. That made the tool look available but unreliable for exact keys or text that lived undersession:<id>.Change Type
Scope
Linked Issue/PR
memory_grep.memory_searchmetadata surfacing and does not changememory_grepcollection routing.Real behavior proof
memory_grepnow finds exact active-session hits stored under the defaultsession:<id>collection.main(944bbbd) plus this patch, using Nodev24.15.0andpnpm@9.15.9.user, score0.88, and the expected snippet.Root Cause
memory_grephard-codedsession_summary:<id>for summaries andsession_raw:<id>for messages, but the default session recall path can store/search hits undersession:<id>.memory_grepcoverage only asserted the experimental summary collection, not the default session collection used by ordinary active-session recall.memory_grepcallable, which exposed that its collection routing was narrower thanmemory_searchsession recall.Regression Test Plan
test/unit/memory-recall.test.tsmemory_grepwithscope=messagessearches bothsession_raw:<id>and defaultsession:<id>, and returns the default-session hit.User-visible / Behavior Changes
memory_grepcan now find exact active-session text stored in the default session recall collection. Existing summary/raw collection behavior remains available.Diagram
Security Impact
Yes, explain risk + mitigation: N/ARepro + Verification
Environment
v24.15.0,pnpm@9.15.9Steps
mainat944bbbd.corepack pnpm@9 exec tsc -p tsconfig.tests.json.session:active-session.memory_grepsearches onlysession_raw:active-sessionand returns zero matches.memory_grepsearchessession_raw:active-sessionplussession:active-sessionand returns the hit.Expected
mainmisses the default-session hit.Actual
mainmissed the default-session hit.host-flow, andplugin:ciremained red on both base and patched with baseline-identical exits/output class.Evidence
Human Verification
session:<id>message hit, existing summary collection search, scopedmessagesrouting, targeted recall tools, adjacent memory/runtime/before-turn units, TypeScript, production build, whitespace diff check, and baseline comparisons for known red broad gates.Review Conversations
No bot or reviewer conversations exist yet for this draft PR.
Compatibility / Migration
Risks and Mitigations
host-flow, andplugin:ci; targeted/adjacent/TypeScript/build gates passed.Summary by CodeRabbit