fix: route semantic memory eviction to correct KV store using source …#132
Merged
rohitg00 merged 4 commits intorohitg00:mainfrom Apr 18, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThe PR modifies memory eviction routing to safely probe KV namespaces before deletion, treating source identifiers as nullable and only resolving them after confirming backing data exists. It adds a source discriminator field to track whether retained memories are episodic or semantic, and removes Changes
Sequence DiagramsequenceDiagram
participant Eviction as Eviction Logic
participant KVMem as KV.memories
participant KVSem as KV.semantic
participant Counter as Counters & Audit
Eviction->>Eviction: Check candidate.source
alt source is missing/unknown
Eviction->>KVMem: Probe for entry
alt found in memories
KVMem-->>Eviction: Entry exists
Eviction->>Eviction: Set resolvedSource = "episodic"
Eviction->>KVMem: Delete entry
else not in memories
KVMem-->>Eviction: Not found
Eviction->>KVSem: Probe for entry
alt found in semantic
KVSem-->>Eviction: Entry exists
Eviction->>Eviction: Set resolvedSource = "semantic"
Eviction->>KVSem: Delete entry
else not in semantic
KVSem-->>Eviction: Not found
Eviction->>Eviction: Skip candidate (continue)
end
end
else source is provided
Eviction->>Eviction: Use source directly
Eviction->>Eviction: Delete from appropriate namespace
end
Eviction->>Counter: Update counters & recordAudit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
🧹 Nitpick comments (2)
src/types.ts (1)
782-782: Prefer makingRetentionScore.sourcerequired for new writes.This field now drives eviction routing; keeping it optional reduces compile-time safety and makes future omissions easier. A stricter type plus legacy fallback handling in eviction is safer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types.ts` at line 782, Make the RetentionScore.source property required (change source?: to source:) so new writes must include "episodic" | "semantic"; update any constructors/builders that create RetentionScore to supply a source. Then update the eviction routing logic (the function/method that inspects RetentionScore.source, e.g., the eviction handler or routeEvictionByScore) to treat missing/legacy scores defensively by detecting undefined/null and mapping them to a sensible default (explicit fallback branch such as "semantic" or a legacy tag) so existing data still evaporates correctly while new records are type-safe.test/retention.test.ts (1)
246-277: Add a legacy retention-score regression case (sourcemissing).This test covers the new happy path. Please add one case that seeds
mem:retentionentries withoutsourceand verifies semantic IDs still evict frommem:semantic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/retention.test.ts` around lines 246 - 277, Add a regression test that seeds a legacy retention entry missing the `source` field and verifies semantic items still get evicted: in the same test file use the existing helpers (makeMemory, makeSemanticMemory, mockKV, mockSdk) and registerRetentionFunctions to insert a mem:retention record without `source` (e.g., directly into the mockKV store), trigger the existing "mem::retention-score" and then "mem::retention-evict" flows, and assert that mem:semantic entries are removed and evicted count increments; refer to registerRetentionFunctions, mem::retention-score, mem::retention-evict, and the KV keys "mem:retention", "mem:memories", and "mem:semantic" when locating where to add this case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/functions/retention.ts`:
- Around line 221-229: The deletion branch for evicted candidates currently
calls kv.delete(...) (for KV.semantic or KV.memories and KV.retentionScores)
without audit records; update both branches in the eviction logic to call
recordAudit(...) for each kv.delete call (include memoryId, the KV namespace key
like KV.semantic/KV.memories/KV.retentionScores, operation type "delete", and
any relevant candidate metadata) immediately before or after each kv.delete to
ensure all state-changing deletes are recorded; make changes around the eviction
block that references candidate.memoryId, candidate.source, KV.semantic,
KV.memories, KV.retentionScores, kv.delete and evicted and reuse the same audit
schema used elsewhere in the codebase when invoking recordAudit.
- Around line 221-229: The eviction branch currently treats any non-"semantic"
candidate as episodic, which lets semantic entries missing candidate.source
survive; update the logic in retention.ts around candidate.source to explicitly
check: if candidate.source === "semantic" delete KV.semantic and
KV.retentionScores for candidate.memoryId; else if candidate.source ===
"episodic" delete KV.memories and KV.retentionScores; else (unknown/undefined)
log a warning and delete both KV.semantic and KV.memories plus
KV.retentionScores for candidate.memoryId to ensure entries without a source are
not misrouted.
---
Nitpick comments:
In `@src/types.ts`:
- Line 782: Make the RetentionScore.source property required (change source?: to
source:) so new writes must include "episodic" | "semantic"; update any
constructors/builders that create RetentionScore to supply a source. Then update
the eviction routing logic (the function/method that inspects
RetentionScore.source, e.g., the eviction handler or routeEvictionByScore) to
treat missing/legacy scores defensively by detecting undefined/null and mapping
them to a sensible default (explicit fallback branch such as "semantic" or a
legacy tag) so existing data still evaporates correctly while new records are
type-safe.
In `@test/retention.test.ts`:
- Around line 246-277: Add a regression test that seeds a legacy retention entry
missing the `source` field and verifies semantic items still get evicted: in the
same test file use the existing helpers (makeMemory, makeSemanticMemory, mockKV,
mockSdk) and registerRetentionFunctions to insert a mem:retention record without
`source` (e.g., directly into the mockKV store), trigger the existing
"mem::retention-score" and then "mem::retention-evict" flows, and assert that
mem:semantic entries are removed and evicted count increments; refer to
registerRetentionFunctions, mem::retention-score, mem::retention-evict, and the
KV keys "mem:retention", "mem:memories", and "mem:semantic" when locating where
to add this case.
🪄 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: d03285df-a47d-466e-b6d8-a427b919b67d
📒 Files selected for processing (3)
src/functions/retention.tssrc/types.tstest/retention.test.ts
… and legacy source handling
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/functions/retention.ts (1)
244-248:⚠️ Potential issue | 🟠 MajorAudit the retention-score delete too.
Line 248 still mutates state without an audit entry. That means the fallback path can remove
KV.retentionScoreswith no trace even though this function now audits the scoped memory delete.Suggested fix
if (deletedScope) { await recordAudit(kv, "delete", "mem::retention-evict", [candidate.memoryId], { scope: deletedScope }); } + await recordAudit( + kv, + "delete", + "mem::retention-evict", + [candidate.memoryId], + { scope: KV.retentionScores }, + ); await kv.delete(KV.retentionScores, candidate.memoryId); evicted++;As per coding guidelines:
Use recordAudit() for all state-changing operations in TypeScript functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/retention.ts` around lines 244 - 248, The code deletes KV.retentionScores via kv.delete(KV.retentionScores, candidate.memoryId) without an audit entry; add a corresponding recordAudit call before or after this delete (similar to the scoped delete) so that the deletion of retention scores is recorded — reference the existing recordAudit function and use the same action/type pattern (e.g., "delete", "mem::retention-evict") and include candidate.memoryId and any relevant metadata (scope or null) when recording this operation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/functions/retention.ts`:
- Around line 230-241: The current use of .catch(() => null) on kv.get calls
(for KV.memories and KV.semantic using candidate.memoryId) treats transient
storage errors as "not found" and can cause deletion of the retention score and
incrementing evicted without confirming existence; change this to explicitly
distinguish NotFound vs other errors: call kv.get(...) without swallowing errors
or wrap each call in a try/catch that rethrows or returns an error sentinel for
non-NotFound errors, only proceed to kv.delete(...) and set deletedScope
("episodic"/"semantic") when the get successfully returns a record, and ensure
evicted is incremented only after a confirmed delete; if a storage error occurs,
surface or log the error and skip modifying retention/evicted so the failure
isn't masked.
---
Duplicate comments:
In `@src/functions/retention.ts`:
- Around line 244-248: The code deletes KV.retentionScores via
kv.delete(KV.retentionScores, candidate.memoryId) without an audit entry; add a
corresponding recordAudit call before or after this delete (similar to the
scoped delete) so that the deletion of retention scores is recorded — reference
the existing recordAudit function and use the same action/type pattern (e.g.,
"delete", "mem::retention-evict") and include candidate.memoryId and any
relevant metadata (scope or null) when recording this operation.
🪄 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: 82bf8321-7aba-43f2-b6d7-5b1ae7ab3de6
📒 Files selected for processing (1)
src/functions/retention.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/functions/retention.ts (1)
244-249:⚠️ Potential issue | 🟠 MajorAudit is still missing for retention-score deletion.
Line 248 mutates state (
KV.retentionScoresdelete) without a correspondingrecordAudit()entry. Please audit this delete too, including the fallback/orphan path wheredeletedScopeisnull.
As per coding guidelines:Use recordAudit() for all state-changing operations in TypeScript functions.Suggested patch
- if (deletedScope) { - await recordAudit(kv, "delete", "mem::retention-evict", [candidate.memoryId], { scope: deletedScope }); - } - - await kv.delete(KV.retentionScores, candidate.memoryId); + if (deletedScope) { + await recordAudit( + kv, + "delete", + "mem::retention-evict", + [candidate.memoryId], + { scope: deletedScope }, + ); + } + + await recordAudit( + kv, + "delete", + "mem::retention-evict", + [candidate.memoryId], + { scope: "retentionScores", memoryScope: deletedScope ?? "unknown" }, + ); + await kv.delete(KV.retentionScores, candidate.memoryId); evicted++;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/retention.ts` around lines 244 - 249, The deletion of the retention score (kv.delete(KV.retentionScores, candidate.memoryId)) is not being audited; add a recordAudit() call for that state change and ensure it runs regardless of whether deletedScope is non-null or null (use deletedScope or a clear fallback like "orphan" in the audit metadata). Locate the block around recordAudit(..., "mem::retention-evict", ...) and KV.retentionScores deletion in retention.ts and insert a call such as recordAudit(kv, "delete", "kv::retentionScores", [candidate.memoryId], { scope: deletedScope ?? nullOrphanValue }) so every retention-score deletion is recorded; keep audit ordering consistent with other audits in the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/functions/retention.ts`:
- Around line 244-249: The deletion of the retention score
(kv.delete(KV.retentionScores, candidate.memoryId)) is not being audited; add a
recordAudit() call for that state change and ensure it runs regardless of
whether deletedScope is non-null or null (use deletedScope or a clear fallback
like "orphan" in the audit metadata). Locate the block around recordAudit(...,
"mem::retention-evict", ...) and KV.retentionScores deletion in retention.ts and
insert a call such as recordAudit(kv, "delete", "kv::retentionScores",
[candidate.memoryId], { scope: deletedScope ?? nullOrphanValue }) so every
retention-score deletion is recorded; keep audit ordering consistent with other
audits in the function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad15d994-9969-4d60-9f3d-69b4e5a24932
📒 Files selected for processing (1)
src/functions/retention.ts
Owner
|
Is this related to some issue? |
Resolve conflicts introduced by subsequent main commits touching the same retention paths: - types.ts: union the AuditEntry operation set — keep main's superset (adds relation_create/relation_update/retention_score/skill_extract/ core_add/core_remove/auto_page). Drop HEAD's duplicate source key on RetentionScore (already present). - functions/retention.ts: keep main's cleaner score assembly that uses pre-computed temporalDecay / reinforcementBoost / logs-based lastAccessed and accessCount. Keep main's evictedIds / evictedEpisodic / evictedSemantic counters. Adopt main's batched audit emission (single record per invocation with reason) from after the loop — HEAD's per-candidate audit emission fails the 'single batched audit record' test and would also flood the audit log during normal eviction sweeps. Preserve HEAD's null-guarded continue when neither the episodic nor the semantic store has the candidate, so legacy rows with a missing source field still evict cleanly without a spurious delete against the wrong scope. - test/retention.test.ts: drop HEAD's smaller test in favor of main's more comprehensive suite (scores tag rows with source / evicts from mem:semantic / batched audit on success / no audit when evicted=0 / score emits per-rescore audit / probes namespaces for legacy semantic rows / routes legacy missing-source episodic rows to mem:memories). Main's suite is a strict superset of HEAD's original assertion.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/types.ts (1)
796-810:⚠️ Potential issue | 🟠 MajorDuplicate
sourcefield inRetentionScore.The
source?: "episodic" | "semantic"property is declared twice — once at line 802 and again at line 809. TypeScript will accept this (identical signatures), but it's clearly unintended and will confuse readers/tooling. Drop the second declaration.🔧 Proposed fix
lastAccessed: string; accessCount: number; - source?: "episodic" | "semantic"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types.ts` around lines 796 - 810, The RetentionScore interface declares the optional property source twice; remove the duplicate declaration so RetentionScore only has a single source?: "episodic" | "semantic" entry. Locate the RetentionScore interface in src/types.ts and delete the redundant source line (the second occurrence after accessCount) so the interface contains one definitive source field and the existing comment above it remains accurate.
🧹 Nitpick comments (1)
src/types.ts (1)
798-801: Consider trimming the inline comment.The rationale about pre-0.8.10 backward-compat is useful, but the first two lines describe WHAT the field is (already evident from the name/type). Per coding guidelines, prefer clear naming over WHAT-comments; keep only the backward-compat/"unknown" note that conveys non-obvious WHY.
As per coding guidelines: "Avoid code comments explaining WHAT — use clear naming instead".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types.ts` around lines 798 - 801, Trim the three-line inline comment above the KV scope field to remove the WHAT-description and leave only the backward-compatibility note: keep the sentence about pre-0.8.10 rows and that callers must treat `undefined` as "unknown" and probe both scopes (referencing `#124`), and delete the first two lines that restate the field's purpose; this comment sits with the field used by mem::retention-evict that routes to KV.memories or KV.semantic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/types.ts`:
- Around line 796-810: The RetentionScore interface declares the optional
property source twice; remove the duplicate declaration so RetentionScore only
has a single source?: "episodic" | "semantic" entry. Locate the RetentionScore
interface in src/types.ts and delete the redundant source line (the second
occurrence after accessCount) so the interface contains one definitive source
field and the existing comment above it remains accurate.
---
Nitpick comments:
In `@src/types.ts`:
- Around line 798-801: Trim the three-line inline comment above the KV scope
field to remove the WHAT-description and leave only the backward-compatibility
note: keep the sentence about pre-0.8.10 rows and that callers must treat
`undefined` as "unknown" and probe both scopes (referencing `#124`), and delete
the first two lines that restate the field's purpose; this comment sits with the
field used by mem::retention-evict that routes to KV.memories or KV.semantic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05a9c404-aafe-4e06-b721-cb4f91691309
📒 Files selected for processing (2)
src/functions/retention.tssrc/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/functions/retention.ts
This was referenced Apr 18, 2026
rohitg00
added a commit
that referenced
this pull request
Apr 18, 2026
Bump version + ship CHANGELOG covering everything that merged since v0.8.13: - #118 security advisory drafts for v0.8.2 CVEs - #132 semantic eviction routing + batched retention audit - #157 iii console docs + vendored screenshots in README - #160 (#158) health gated on RSS floor - #161 (#159) standalone MCP proxies to the running server - #162 (#125) mem::forget audit coverage + policy doc - #163 (#62) @agentmemory/fs-watcher filesystem connector - #164 Next.js website (website/ root, ship to Vercel) Version bumps (8 files): - package.json / package-lock.json (top + packages['']) - plugin/.claude-plugin/plugin.json - packages/mcp/package.json (self + ~0.9.0 dep pin) - src/version.ts (union extended, assigned 0.9.0) - src/types.ts (ExportData.version union) - src/functions/export-import.ts (supportedVersions set) - test/export-import.test.ts (export assertion) Tests: 777 passing. Build clean.
Merged
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
2 participants
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.
The mem::retention-evict loop was previously hardcoded to delete candidates
only from KV.memories. This caused semantic memories to fail silently and
remain alive in KV.semantic indefinitely, even with sub-threshold scores.
Changes implemented:
source: 'episodic' | 'semantic'to the RetentionScore interface.sourcefield during mem::retention-score calculation.to either KV.memories or KV.semantic based on the candidate's source.
from their respective KV stores when scores drop below the threshold.
Summary by CodeRabbit
Bug Fixes
Updates