Conversation
…se 0.8.3 Issue #119 — retention score ignored agent-side reads `mem::retention-score` previously hardcoded `accessCount=0` and `accessTimestamps=[]` for episodic memories and used only a single-sample `lastAccessedAt` for semantic memories, so the time-frequency decay formula was a dead path. All read endpoints now record access events to a new `mem:access` KV namespace via a keyed-mutex'd access tracker: - mem::search, mem::smart-search (compact + expand), mem::context, mem::timeline, mem::file-context, mem::get-related, mem::working-context - retention.ts pre-fetches the full access log in one kv.list call, normalizes every row defensively, and builds an O(1) lookup Map - recordAccessBatch uses Promise.allSettled so a slow keyed-lock on one id does not block the rest of the batch - Bounded ring buffer of the last 20 access timestamps per memory - Backwards-compat fallback: pre-0.8.3 semantic memories with only the legacy sem.lastAccessedAt still score correctly - Corrupt lastAccessedAt values are NaN-safe via Date.parse + isFinite - reinforcementBoost is reported as the raw formula output, not the clamped residual Memory deletion sites (retention-evict, evict, auto-forget, governance-delete, governance-bulk, forget, import replace) now call deleteAccessLog to prevent mem:access from accumulating zombie entries. mem::export / mem::import and mem::snapshot-create / mem::snapshot-restore round-trip the new namespace so backup/restore cycles no longer zero out reinforcement signals. Issue #120 — npx agentmemory-mcp returned npm registry 404 The README instructed users to run `npx agentmemory-mcp`, but `agentmemory-mcp` only existed as a `bin` entry inside `@agentmemory/agentmemory`, not a real registry package. Two-part fix: - New sibling package `packages/agentmemory-mcp/` — a thin shim that depends on `@agentmemory/agentmemory` (~0.8.3, tilde to block 0.9.x supply-chain drift) and forwards to dist/standalone.mjs via the new `exports` field on the main package - Canonical `npx @agentmemory/agentmemory mcp` subcommand on the CLI for users who already have the main package installed - Removed the dead `agentmemory-mcp` bin from the main package so the shim owns the name unambiguously - publish.yml publishes both packages in order with idempotent guards and npm-view registry polling (no more fixed sleep) - All MCP install snippets in README + integrations/openclaw + hermes + shim README use `npx -y agentmemory-mcp` to skip install prompts Release 0.8.3 - 670 tests passing (665 + 11 new: access tracker, retention, smart-search integration, corrupt-input, kv.list failure path, Promise.allSettled resilience) - Version bumped across package.json, src/version.ts, types.ts, export-import supportedVersions, plugin.json - package-lock.json regenerated
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdded a new mem:access KV-based access-tracking system and integrated it across read/search/context flows; introduced an agentmemory-mcp shim package and CLI wiring; made publish workflow version-aware with propagation polling; bumped package/plugin/version to 0.8.3 and added export/import/snapshot support for access logs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 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 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/functions/export-import.ts (1)
177-254:⚠️ Potential issue | 🟠 MajorValidate
accessLogsbefore importing them.
accessLogsnow bypasses all of the import limits and shape checks that protect the other top-level collections. A corrupted export can send arbitrarily many logs or hugerecentarrays, and this code will persist them verbatim. Please cap the collection, require a validmemoryId, and clamp/normalizecountandrecentbefore thekv.set()loop.
As per coding guidelines, "TypeScript functions must usesdk.registerFunction()pattern with input validation, state access via kv.get/kv.set/kv.list, and audit recording via recordAudit()".Also applies to: 526-533
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/export-import.ts` around lines 177 - 254, The accessLogs array in importData must be validated and normalized before any kv.set() persistence: enforce a MAX_ACCESS_LOGS cap (e.g., 50_000) and reject/trim if exceeded; for each accessLog entry (accessLogs[i]) require a valid memoryId that exists in importData.memories (or else skip entry), ensure memoryId is a non-empty string, normalize/clamp count to a safe non-negative integer (e.g., Math.min(Math.max(0, parsedCount), MAX_COUNT_PER_LOG)), and normalize recent to an array of strings with a MAX_RECENT_ITEMS cap (e.g., 100) and per-item length limit (trim or drop oversized items); also limit total size of recent arrays to avoid huge payloads; perform these checks in the same function that handles importData (where importData, MAX_SESSIONS, kv.set loop are referenced) before the kv.set loop, and follow the sdk.registerFunction() input/state access pattern and call recordAudit() for the import operation.src/functions/retention.ts (1)
254-258:⚠️ Potential issue | 🟠 MajorSemantic memories are never actually evicted here.
mem::retention-scorecreatesRetentionScoreentries for both regular memories and semantic memories (Lines 120-188), but this loop always deletes fromKV.memories. For semantic candidates, that removes only the score/access log; the row inKV.semanticsurvives and will be scored again on the next run. Please carry the source scope/type into the score entry, or otherwise branch the delete target before reporting success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/retention.ts` around lines 254 - 258, The retention loop that iterates over candidates currently always deletes from KV.memories (in the for loop handling candidates and calling kv.delete(KV.memories, candidate.memoryId), kv.delete(KV.retentionScores, ...), and deleteAccessLog) which removes only scores/access logs for semantic entries while leaving KV.semantic rows; fix this by making the RetentionScore entries include the original source/scope/type when created (see mem::retention-score and the RetentionScore construction) or by branching in the deletion loop using that stored type: if candidate.source === "semantic" (or the stored scope/type field) delete from KV.semantic instead of KV.memories, then remove the retentionScore and access log and only report success after the correct storage row is deleted. Ensure you update the RetentionScore shape and code paths that write/read it so the delete-branch has the needed type information.
🧹 Nitpick comments (5)
src/functions/auto-forget.ts (1)
50-51: Run independent delete operations in parallel.The memory delete and access-log delete are independent; execute them with
Promise.allto reduce latency in the cleanup loop.As per coding guidelines, perform parallel operations where possible using `Promise.all` for independent KV writes/reads in TypeScript.♻️ Suggested refactor
- await kv.delete(KV.memories, mem.id); - await deleteAccessLog(kv, mem.id); + await Promise.all([ + kv.delete(KV.memories, mem.id), + deleteAccessLog(kv, mem.id), + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/auto-forget.ts` around lines 50 - 51, The two independent deletes—kv.delete(KV.memories, mem.id) and deleteAccessLog(kv, mem.id) inside the auto-forget cleanup loop—should be executed in parallel to reduce latency; replace the sequential awaits with a single await Promise.all([...]) that runs both operations for the given mem.id so both complete concurrently while preserving error propagation.src/functions/remember.ts (1)
127-128: Parallelize memory and access-log deletion.These operations are independent and can be executed concurrently.
As per coding guidelines, perform parallel operations where possible using `Promise.all` for independent KV writes/reads in TypeScript.♻️ Suggested refactor
- await kv.delete(KV.memories, data.memoryId); - await deleteAccessLog(kv, data.memoryId); + await Promise.all([ + kv.delete(KV.memories, data.memoryId), + deleteAccessLog(kv, data.memoryId), + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/remember.ts` around lines 127 - 128, The two independent deletions (await kv.delete(KV.memories, data.memoryId) and await deleteAccessLog(kv, data.memoryId)) should be run in parallel; replace the sequential awaits by invoking both operations and awaiting Promise.all([...]) so both delete calls execute concurrently (reference the kv.delete call, deleteAccessLog function, KV.memories and data.memoryId).packages/agentmemory-mcp/bin.mjs (1)
2-10: Optional: include direct fallback command in failure guidance.This would make recovery clearer when deep import resolution fails.
Suggested tweak
import("@agentmemory/agentmemory/dist/standalone.mjs").catch((err) => { console.error( "[agentmemory-mcp] Failed to load standalone entrypoint from `@agentmemory/agentmemory`.", ); console.error( "[agentmemory-mcp] Try installing manually: npm i -g `@agentmemory/agentmemory`", ); + console.error( + "[agentmemory-mcp] Or run directly: npx `@agentmemory/agentmemory` mcp", + ); console.error(err instanceof Error ? err.stack || err.message : String(err)); process.exit(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agentmemory-mcp/bin.mjs` around lines 2 - 10, The catch handler for the dynamic import (the .catch callback handling import("@agentmemory/agentmemory/dist/standalone.mjs")) currently logs a generic install suggestion; update that error path to append a clear, copy‑paste fallback command to the console.error output (e.g., an explicit "npm i -g `@agentmemory/agentmemory`" or equivalent) so users can recover quickly, and ensure the existing err output (err.stack || err.message) and the final process.exit(1) remain unchanged; modify the messages printed by console.error in that catch block to include the direct fallback command text.test/access-tracker.test.ts (1)
30-142: Add direct coverage fordeleteAccessLog()andnormalizeAccessLog().This suite locks in the write path, but the PR also depends on stale-log cleanup and defensive normalization. A focused unit test for each helper would catch the regressions most likely to break retention and backup round-trips.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/access-tracker.test.ts` around lines 30 - 142, Add unit tests that directly exercise deleteAccessLog() and normalizeAccessLog(): write a test that seeds the mockKV with an access log entry, calls deleteAccessLog(kv, memoryId) and asserts the entry is removed (and that other keys remain), and write tests for normalizeAccessLog() feeding malformed/partial log objects (missing count, lastAt, recent; wrong types; extra entries) and asserting it returns a well-formed AccessLog with bounded recent length and ISO lastAt; use the same import pattern as the other tests to import these functions from "../src/functions/access-tracker.js" and the existing mockKV helper to set up and inspect kv.store.test/retention-access.test.ts (1)
98-269: Add a malformedmem:accessfixture case.The new scoring path normalizes raw access-log records before using them, but this suite only covers corrupted
SemanticMemory.lastAccessedAt. Please add a case with badcount/recentvalues inmem:accessso that defensive normalization stays covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/retention-access.test.ts` around lines 98 - 269, Add a new test in test/retention-access.test.ts that injects a malformed mem:access record into the mock KV and asserts retention scoring normalizes it: use mockSdk() and mockKV(...) as in other tests, call registerRetentionFunctions(sdk, kv), then insert a bad access entry into the kv for scope "mem:access" (e.g., invalid count/recent fields like strings, nulls, negatives or NaN) rather than using recordAccess, trigger the "mem::retention-score" via sdk.trigger, and assert the resulting score entry for that memory has a finite numeric reinforcementBoost and accessCount is treated defensively (e.g., coerced to 0 or a sane number) to cover the normalization path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/publish.yml:
- Around line 50-58: The publish step for the "Publish agentmemory-mcp shim"
currently publishes but doesn't wait for registry propagation; add the same
polling/verification loop used for the `@agentmemory/agentmemory` step: after npm
publish in the agentmemory-mcp step, repeatedly run npm view
"agentmemory-mcp@$SHIM_VERSION" version with a sleep/retry loop until it returns
successfully (or times out), so that consumers like npx agentmemory-mcp no
longer see transient 404s; use the existing SHIM_VERSION variable and the
package name "agentmemory-mcp" to locate where to add this check.
In `@CHANGELOG.md`:
- Line 7: Add a reference-style link definition for the new release token
[0.8.3] so the heading resolves like earlier entries; locate the pattern used
for other release link targets (e.g., the existing [0.8.2] / footer definitions)
and append a matching definition for [0.8.3] with the appropriate URL or compare
link and date information in the changelog footer.
In `@README.md`:
- Around line 589-594: Update the two example invocations so they include the
automatic-yes flag to avoid the install prompt: change "npx
`@agentmemory/agentmemory` mcp" to "npx -y `@agentmemory/agentmemory` mcp" and "npx
agentmemory-mcp" to "npx -y agentmemory-mcp" in the README snippet so the
standalone npx examples behave like the rest of the document.
In `@src/functions/access-tracker.ts`:
- Around line 86-93: deleteAccessLog races with recordAccess because it deletes
mem:access/<id> outside the keyed lock; fix by performing the delete inside the
same withKeyedLock used by recordAccess. In deleteAccessLog (and where
KV.accessLog is referenced), call withKeyedLock(memoryId, async () => { await
kv.delete(KV.accessLog, memoryId); }) (keeping the early return if !memoryId)
and preserve error handling (log/throw as appropriate) so deletes and
recordAccess updates are serialized for the same memoryId.
- Around line 19-29: normalizeAccessLog currently allows negative/fractional
counts and arbitrarily long recent arrays; update it so count is coerced to an
integer, clamped to the 0..20 range, and recent is filtered to only finite
numbers and truncated to the last 20 entries (preserving order), then ensure the
returned count is at least recent.length and at most 20 (e.g., count =
Math.max(truncatedRecent.length, Math.min(20, Math.floor(parsedCount)))) — make
these changes inside normalizeAccessLog to reapply the 20-entry/access-count
invariants.
In `@src/functions/auto-forget.ts`:
- Around line 49-52: The TTL-driven deletion branch in auto-forget.ts deletes
persistent state (await kv.delete(KV.memories, mem.id) and await
deleteAccessLog(kv, mem.id)) without recording an audit; add a call to
recordAudit(...) after successful deletions (only when dryRun is false) that
records the deletion event, the resource id (mem.id), actor/context info from
the current request/kv context, and a descriptive reason (e.g., "TTL
expiration"); ensure recordAudit is awaited and placed so it runs only when both
delete and deleteAccessLog succeed to preserve audit consistency.
In `@src/functions/evict.ts`:
- Around line 139-142: The access-log cleanup should only run if the memory
deletion succeeds and its errors must be caught; update both places where you
call kv.delete(KV.memories, mem.id) followed by deleteAccessLog(kv, mem.id) (the
blocks guarded by the dryRun check) to first perform the delete and confirm
success (e.g., await and check result or catch delete error), and only then call
deleteAccessLog inside its own try/catch so any deleteAccessLog failure is
swallowed/logged and does not abort eviction; ensure you apply this change to
both occurrences referencing kv.delete(KV.memories, mem.id) and
deleteAccessLog(kv, mem.id).
In `@src/functions/remember.ts`:
- Around line 126-129: In the mem::forget branch where you call await
kv.delete(KV.memories, data.memoryId) and await deleteAccessLog(kv,
data.memoryId), add a call to recordAudit(...) to emit an audit record for this
structural deletion; invoke recordAudit with the same kv instance, an action
name like "memory.forget" (or "forget"), and include the memoryId (and any
relevant actor/context) so the deletion is recorded; update the block containing
data.memoryId in src/functions/remember.ts (the if (data.memoryId) branch) to
call recordAudit before or after the deletes and keep incrementing deleted.
In `@src/functions/snapshot.ts`:
- Around line 55-57: The snapshot code silently swallows failures when reading
access logs (the kv.list<AccessLogExport>(KV.accessLog).catch(() => [] as
AccessLogExport[])) causing snapshots to succeed with an empty accessLogs array;
change this to fail-fast and surface the error instead of replacing with []:
remove the silent .catch or replace it with a try/catch that rethrows a wrapped
Error (including KV.accessLog and context) or returns an explicit
partial-snapshot error result so snapshot creation fails (or returns an error
state) when kv.list for KV.accessLog fails; locate the accessLogs variable and
the kv.list call in snapshot creation to implement this.
---
Outside diff comments:
In `@src/functions/export-import.ts`:
- Around line 177-254: The accessLogs array in importData must be validated and
normalized before any kv.set() persistence: enforce a MAX_ACCESS_LOGS cap (e.g.,
50_000) and reject/trim if exceeded; for each accessLog entry (accessLogs[i])
require a valid memoryId that exists in importData.memories (or else skip
entry), ensure memoryId is a non-empty string, normalize/clamp count to a safe
non-negative integer (e.g., Math.min(Math.max(0, parsedCount),
MAX_COUNT_PER_LOG)), and normalize recent to an array of strings with a
MAX_RECENT_ITEMS cap (e.g., 100) and per-item length limit (trim or drop
oversized items); also limit total size of recent arrays to avoid huge payloads;
perform these checks in the same function that handles importData (where
importData, MAX_SESSIONS, kv.set loop are referenced) before the kv.set loop,
and follow the sdk.registerFunction() input/state access pattern and call
recordAudit() for the import operation.
In `@src/functions/retention.ts`:
- Around line 254-258: The retention loop that iterates over candidates
currently always deletes from KV.memories (in the for loop handling candidates
and calling kv.delete(KV.memories, candidate.memoryId),
kv.delete(KV.retentionScores, ...), and deleteAccessLog) which removes only
scores/access logs for semantic entries while leaving KV.semantic rows; fix this
by making the RetentionScore entries include the original source/scope/type when
created (see mem::retention-score and the RetentionScore construction) or by
branching in the deletion loop using that stored type: if candidate.source ===
"semantic" (or the stored scope/type field) delete from KV.semantic instead of
KV.memories, then remove the retentionScore and access log and only report
success after the correct storage row is deleted. Ensure you update the
RetentionScore shape and code paths that write/read it so the delete-branch has
the needed type information.
---
Nitpick comments:
In `@packages/agentmemory-mcp/bin.mjs`:
- Around line 2-10: The catch handler for the dynamic import (the .catch
callback handling import("@agentmemory/agentmemory/dist/standalone.mjs"))
currently logs a generic install suggestion; update that error path to append a
clear, copy‑paste fallback command to the console.error output (e.g., an
explicit "npm i -g `@agentmemory/agentmemory`" or equivalent) so users can recover
quickly, and ensure the existing err output (err.stack || err.message) and the
final process.exit(1) remain unchanged; modify the messages printed by
console.error in that catch block to include the direct fallback command text.
In `@src/functions/auto-forget.ts`:
- Around line 50-51: The two independent deletes—kv.delete(KV.memories, mem.id)
and deleteAccessLog(kv, mem.id) inside the auto-forget cleanup loop—should be
executed in parallel to reduce latency; replace the sequential awaits with a
single await Promise.all([...]) that runs both operations for the given mem.id
so both complete concurrently while preserving error propagation.
In `@src/functions/remember.ts`:
- Around line 127-128: The two independent deletions (await
kv.delete(KV.memories, data.memoryId) and await deleteAccessLog(kv,
data.memoryId)) should be run in parallel; replace the sequential awaits by
invoking both operations and awaiting Promise.all([...]) so both delete calls
execute concurrently (reference the kv.delete call, deleteAccessLog function,
KV.memories and data.memoryId).
In `@test/access-tracker.test.ts`:
- Around line 30-142: Add unit tests that directly exercise deleteAccessLog()
and normalizeAccessLog(): write a test that seeds the mockKV with an access log
entry, calls deleteAccessLog(kv, memoryId) and asserts the entry is removed (and
that other keys remain), and write tests for normalizeAccessLog() feeding
malformed/partial log objects (missing count, lastAt, recent; wrong types; extra
entries) and asserting it returns a well-formed AccessLog with bounded recent
length and ISO lastAt; use the same import pattern as the other tests to import
these functions from "../src/functions/access-tracker.js" and the existing
mockKV helper to set up and inspect kv.store.
In `@test/retention-access.test.ts`:
- Around line 98-269: Add a new test in test/retention-access.test.ts that
injects a malformed mem:access record into the mock KV and asserts retention
scoring normalizes it: use mockSdk() and mockKV(...) as in other tests, call
registerRetentionFunctions(sdk, kv), then insert a bad access entry into the kv
for scope "mem:access" (e.g., invalid count/recent fields like strings, nulls,
negatives or NaN) rather than using recordAccess, trigger the
"mem::retention-score" via sdk.trigger, and assert the resulting score entry for
that memory has a finite numeric reinforcementBoost and accessCount is treated
defensively (e.g., coerced to 0 or a sane number) to cover the normalization
path.
🪄 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: c5bbc242-6d39-489a-8fa8-b556505c0399
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
.github/workflows/publish.ymlCHANGELOG.mdREADME.mdintegrations/hermes/README.mdintegrations/openclaw/README.mdpackage.jsonpackages/agentmemory-mcp/LICENSEpackages/agentmemory-mcp/README.mdpackages/agentmemory-mcp/bin.mjspackages/agentmemory-mcp/package.jsonplugin/.claude-plugin/plugin.jsonsrc/cli.tssrc/functions/access-tracker.tssrc/functions/auto-forget.tssrc/functions/context.tssrc/functions/evict.tssrc/functions/export-import.tssrc/functions/file-index.tssrc/functions/governance.tssrc/functions/relations.tssrc/functions/remember.tssrc/functions/retention.tssrc/functions/search.tssrc/functions/smart-search.tssrc/functions/snapshot.tssrc/functions/timeline.tssrc/functions/working-memory.tssrc/state/schema.tssrc/types.tssrc/version.tstest/access-tracker.test.tstest/export-import.test.tstest/retention-access.test.tstest/smart-search.test.ts
Comment on lines
+19
to
+29
| export function normalizeAccessLog(raw: unknown): AccessLog { | ||
| const r = (raw ?? {}) as Partial<AccessLog>; | ||
| return { | ||
| memoryId: typeof r.memoryId === "string" ? r.memoryId : "", | ||
| count: typeof r.count === "number" && Number.isFinite(r.count) ? r.count : 0, | ||
| lastAt: typeof r.lastAt === "string" ? r.lastAt : "", | ||
| recent: Array.isArray(r.recent) | ||
| ? r.recent.filter((x): x is number => typeof x === "number" && Number.isFinite(x)) | ||
| : [], | ||
| }; | ||
| } |
There was a problem hiding this comment.
Reapply the 20-entry/access-count invariants during normalization.
normalizeAccessLog() is the defensive read path, but it currently preserves negative/fractional count values and arbitrarily long recent arrays. retention.ts trusts both fields, so malformed imported data can underflow salience or massively overboost retention while defeating the bounded ring-buffer guarantee.
Suggested hardening
export function normalizeAccessLog(raw: unknown): AccessLog {
const r = (raw ?? {}) as Partial<AccessLog>;
return {
memoryId: typeof r.memoryId === "string" ? r.memoryId : "",
- count: typeof r.count === "number" && Number.isFinite(r.count) ? r.count : 0,
+ count:
+ typeof r.count === "number" && Number.isFinite(r.count)
+ ? Math.max(0, Math.trunc(r.count))
+ : 0,
lastAt: typeof r.lastAt === "string" ? r.lastAt : "",
recent: Array.isArray(r.recent)
- ? r.recent.filter((x): x is number => typeof x === "number" && Number.isFinite(x))
+ ? r.recent
+ .filter((x): x is number => typeof x === "number" && Number.isFinite(x))
+ .slice(-RECENT_CAP)
: [],
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/functions/access-tracker.ts` around lines 19 - 29, normalizeAccessLog
currently allows negative/fractional counts and arbitrarily long recent arrays;
update it so count is coerced to an integer, clamped to the 0..20 range, and
recent is filtered to only finite numbers and truncated to the last 20 entries
(preserving order), then ensure the returned count is at least recent.length and
at most 20 (e.g., count = Math.max(truncatedRecent.length, Math.min(20,
Math.floor(parsedCount)))) — make these changes inside normalizeAccessLog to
reapply the 20-entry/access-count invariants.
Comment on lines
49
to
52
| if (!dryRun) { | ||
| await kv.delete(KV.memories, mem.id); | ||
| await deleteAccessLog(kv, mem.id); | ||
| } |
There was a problem hiding this comment.
Add audit recording for TTL-driven deletions.
Line 50 and Line 51 mutate persistent state, but this branch does not record an audit event. That weakens deletion traceability for structural mutations.
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/auto-forget.ts` around lines 49 - 52, The TTL-driven deletion
branch in auto-forget.ts deletes persistent state (await kv.delete(KV.memories,
mem.id) and await deleteAccessLog(kv, mem.id)) without recording an audit; add a
call to recordAudit(...) after successful deletions (only when dryRun is false)
that records the deletion event, the resource id (mem.id), actor/context info
from the current request/kv context, and a descriptive reason (e.g., "TTL
expiration"); ensure recordAudit is awaited and placed so it runs only when both
delete and deleteAccessLog succeed to preserve audit consistency.
Comment on lines
139
to
142
| if (!dryRun) { | ||
| await kv.delete(KV.memories, mem.id).catch(() => {}); | ||
| await deleteAccessLog(kv, mem.id); | ||
| } |
There was a problem hiding this comment.
Guard access-log deletion on successful memory delete and handle cleanup errors.
At Line 141 and Line 156, access-log cleanup is unconditional even though memory deletion at Line 140/155 can fail silently. Also, uncaught deleteAccessLog failures can terminate eviction mid-run.
Suggested fix
- await kv.delete(KV.memories, mem.id).catch(() => {});
- await deleteAccessLog(kv, mem.id);
+ const deleted = await kv
+ .delete(KV.memories, mem.id)
+ .then(() => true)
+ .catch(() => false);
+ if (deleted) {
+ await deleteAccessLog(kv, mem.id).catch(() => {});
+ }
...
- await kv.delete(KV.memories, mem.id).catch(() => {});
- await deleteAccessLog(kv, mem.id);
+ const deleted = await kv
+ .delete(KV.memories, mem.id)
+ .then(() => true)
+ .catch(() => false);
+ if (deleted) {
+ await deleteAccessLog(kv, mem.id).catch(() => {});
+ }Also applies to: 154-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/functions/evict.ts` around lines 139 - 142, The access-log cleanup should
only run if the memory deletion succeeds and its errors must be caught; update
both places where you call kv.delete(KV.memories, mem.id) followed by
deleteAccessLog(kv, mem.id) (the blocks guarded by the dryRun check) to first
perform the delete and confirm success (e.g., await and check result or catch
delete error), and only then call deleteAccessLog inside its own try/catch so
any deleteAccessLog failure is swallowed/logged and does not abort eviction;
ensure you apply this change to both occurrences referencing
kv.delete(KV.memories, mem.id) and deleteAccessLog(kv, mem.id).
Comment on lines
126
to
129
| if (data.memoryId) { | ||
| await kv.delete(KV.memories, data.memoryId); | ||
| await deleteAccessLog(kv, data.memoryId); | ||
| deleted++; |
There was a problem hiding this comment.
Audit trail is missing for explicit forget deletions.
Line 127 and Line 128 both mutate persistent state in mem::forget; this branch should emit an audit record for the structural deletion action.
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/remember.ts` around lines 126 - 129, In the mem::forget branch
where you call await kv.delete(KV.memories, data.memoryId) and await
deleteAccessLog(kv, data.memoryId), add a call to recordAudit(...) to emit an
audit record for this structural deletion; invoke recordAudit with the same kv
instance, an action name like "memory.forget" (or "forget"), and include the
memoryId (and any relevant actor/context) so the deletion is recorded; update
the block containing data.memoryId in src/functions/remember.ts (the if
(data.memoryId) branch) to call recordAudit before or after the deletes and keep
incrementing deleted.
Comment on lines
+55
to
+57
| const accessLogs = await kv | ||
| .list<AccessLogExport>(KV.accessLog) | ||
| .catch(() => [] as AccessLogExport[]); |
There was a problem hiding this comment.
Don't silently drop mem:access from a successful snapshot.
If kv.list(KV.accessLog) fails here, snapshot creation still succeeds with accessLogs: []. Restoring that snapshot later erases the retention history for every memory. Backup paths should fail fast or surface a partial-snapshot result instead of silently zeroing a namespace.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/functions/snapshot.ts` around lines 55 - 57, The snapshot code silently
swallows failures when reading access logs (the
kv.list<AccessLogExport>(KV.accessLog).catch(() => [] as AccessLogExport[]))
causing snapshots to succeed with an empty accessLogs array; change this to
fail-fast and surface the error instead of replacing with []: remove the silent
.catch or replace it with a try/catch that rethrows a wrapped Error (including
KV.accessLog and context) or returns an explicit partial-snapshot error result
so snapshot creation fails (or returns an error state) when kv.list for
KV.accessLog fails; locate the accessLogs variable and the kv.list call in
snapshot creation to implement this.
Real issues caught and fixed: - access-tracker: deleteAccessLog now takes the same mem:access:<id> keyed mutex used by recordAccess, eliminating the race where a concurrent delete could drop an in-flight RMW update and leave a stale access log for an evicted memory - access-tracker: normalizeAccessLog now coerces count to a non-negative integer, drops non-finite recent[] entries, truncates recent[] to the last 20 on read (enforcing the ring-buffer invariant), and guarantees count >= recent.length so a malformed row can't produce NaN downstream. count is NOT capped at 20 — it is the lifetime access counter, while recent[] is the 20-entry window - export-import: import path now rejects accessLogs arrays above MAX_ACCESS_LOGS (50_000), normalizes each entry via normalizeAccessLog, and drops entries whose memoryId is not in the imported memories set, so a crafted export can't blow up memory or resurrect zombie access rows for memories that don't exist - publish.yml: added a post-publish npm view polling loop for agentmemory-mcp mirroring the one that already guards the main package, so the workflow only succeeds when `npx agentmemory-mcp` is actually visible on the registry - CHANGELOG.md: added the missing [0.8.3] reference-style link target at the bottom to match the [0.8.2]/[0.8.1]/[0.8.0] pattern - README.md: standalone MCP inline examples now use `npx -y` to match the MCP config snippets and skip the install confirmation prompt Tests: 680/680 (+10 new — deleteAccessLog behavior, normalizeAccessLog invariants, malformed mem:access row handling in retention scoring). Findings deliberately not applied: - auto-forget.ts / remember.ts / evict.ts recordAudit calls: existing pattern in the codebase only audits governance deletes (user-initiated admin actions), not automatic eviction. Adding audit everywhere is a policy change, out of scope for this PR - evict.ts gating deleteAccessLog on memory delete success: current order is defensible, deleteAccessLog is already best-effort with its own try/catch, and calling it after a failed memory delete is harmless - snapshot.ts fail-fast on kv.list failure: inconsistent with the existing .catch(() => []) pattern used for observations in the same function - retention.ts semantic evict only deletes KV.memories rows: real bug but pre-existing on main (RetentionScore has no source/type field). Tracked separately — out of scope for this PR - bin.mjs fallback command: already present in the existing catch handler - Promise.all parallelization of delete+deleteAccessLog: micro optimization not worth the readability cost
This was referenced Apr 13, 2026
This was referenced Apr 13, 2026
Merged
This was referenced Apr 15, 2026
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.
Closes #119, closes #120. Ships as v0.8.3.
Summary
Two bug fixes reported in the public issue tracker, plus the fallout from a full specialist review.
#119 — retention score was a dead path
mem::retention-scorehardcodedaccessCount=0andaccessTimestamps=[]for episodic memories and only used a single-samplelastAccessedAtfor semantic ones. Reads frommem::search,mem::smart-search,mem::context,mem::timeline,mem::file-contextwere never recorded, so the time-frequency decay formula never saw any data.Fix:
src/functions/access-tracker.ts—recordAccess,recordAccessBatch,getAccessLog,deleteAccessLog,normalizeAccessLog,emptyAccessLog. Bounded ring buffer (20 timestamps). Keyed-mutex wrapped RMW so concurrent reads on the same id don't lose increments.Promise.allSettledacross ids so one slow lock doesn't block the batch.mem::search,mem::smart-search(compact + expand),mem::context(observation blocks),mem::timeline,mem::file-context,mem::get-related,mem::working-context(archival pages).mem::retention-scorepre-fetches the full access log in onekv.listcall, normalizes every row defensively, and builds an O(1) lookupMap— no N+1.reinforcementBoostis reported as the raw formula output via an extractedcomputeReinforcementBoosthelper, not the clamped residual.sem.lastAccessedAtis NaN-safe viaDate.parse+Number.isFinite.sem.lastAccessedAtstill score correctly. The fallback kicks in only when the fresh access log is empty.retention-evict,evict,auto-forget,governance-delete,governance-bulk,forget,importreplace) now calldeleteAccessLogsomem:accessdoesn't accumulate zombies.mem::export/mem::importandmem::snapshot-create/mem::snapshot-restoreround-trip the new namespace. Backup/restore no longer zeroes reinforcement signals.#120 — npx agentmemory-mcp returned npm registry 404
The README told users to run
npx agentmemory-mcp, but that was only abinentry inside@agentmemory/agentmemory, not a real registry package.npxresolves against the registry, so it 404'd every first-time user.Fix — both a shim package and a canonical CLI subcommand:
packages/agentmemory-mcp/— thin shim depending on@agentmemory/agentmemory(~0.8.3, tilde to block 0.9.x supply-chain drift) that forwards todist/standalone.mjsvia the newexportsfield on the main package.npx @agentmemory/agentmemory mcpsubcommand on the CLI for users who already have the main package.agentmemory-mcpbin from the main package so the shim owns the name unambiguously..github/workflows/publish.ymlpublishes both packages in order, with idempotent guards (skip if version already published) andnpm viewpolling instead of a fixed sleep, so retrying a failed workflow doesn't leave the registry in a half state.README.md,integrations/openclaw/README.md,integrations/hermes/README.md, andpackages/agentmemory-mcp/README.mdusenpx -y agentmemory-mcpnow (skips the first-run confirmation prompt).Review passes
This branch went through the full pipeline in one session:
/simplify(three review agents — reuse, quality, efficiency) →/review(five specialists — testing, security, api-contract, maintainability, red-team). Every critical finding was fixed in the same branch before landing:retention.tsper-memorygetAccessLogcallskv.list(KV.accessLog)+ Map lookupContextBlock & { sourceIds }structural extensionsourceIds?to the canonical typepackage-lock.jsonstale at 0.8.2publish.ymlonly publishes root packagemem::access-record/getregistered but never wired to MCP/RESTpackages/agentmemory-mcpcaret^0.8.3allows 0.9.x~0.8.3+publishConfigmem::export/mem::importdroppedmem:accessnamespacesem.lastAccessedAtDate.parse+isFiniteguardreinforcementBoostreported clamped residual not rawcomputeReinforcementBoosthelper-yflagdeleteAccessLogwired into 6 sitesmem::snapshot-create/restorebypassesmem:accessbincollision onagentmemory-mcpnamesleep 30has no recovery on slow propagationnpm viewpolling + idempotent guardsmem::get-related,mem::working-contextdon't reinforcerecordAccessBatchretention.tsbulk list path bypasses defensive normalizenormalizeAccessLoghelper, used in both pathsTests
test/access-tracker.test.ts— 8 tests (was 7) includingrecordAccessBatchresilience with a failing idtest/retention-access.test.ts— 7 tests (was 4) including paired with-vs-without legacylastAccessedAt, NaN-safe corrupt input,kv.listfailure pathtest/smart-search.test.ts— 7 tests (was 5) including the end-to-end integration test that provesmem::smart-searchactually populatesmem:accessfor returned ids (the whole fix for BUG: Retention Score Formula is Broken #119 was unguarded against silent regression before)Test plan
npm test— 670 / 670 passingnpm run build— cleanimport("@agentmemory/agentmemory/dist/standalone.mjs")smoke-tested vianpm install ~/agentmemoryin a temp dir → resolves through the newexportsfield and prints the standalone bannerpublish.ymlrunnpx agentmemory-mcpfrom a clean/tmpdirectory should boot the MCP stdio server without 404Reviewer notes
start()function),mem::contextsummary blocks not reinforcing (needsSessionSummaryschema change), input validation onmemoryIdin the access tracker (in-process callers are trusted), lock-map exhaustion hardening (not exploitable today).src/version.tsVERSION union,src/types.tsExportData.version union, andsrc/functions/export-import.tssupportedVersions Set each have slightly different shapes (missing0.7.7/0.7.9/0.8.0depending on which one). Pre-existing drift worth fixing separately.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests