fix: post-tool-use skips self-capturing memory_* MCP tools (#993)#1044
fix: post-tool-use skips self-capturing memory_* MCP tools (#993)#1044ziishanahmad wants to merge 16 commits into
Conversation
…ohitg00#1022) Signed-off-by: Zeeshan Ahmad <ziishanahmad@gmail.com>
…ddress review feedback (rohitg00#1022) Signed-off-by: Zeeshan Ahmad <ziishanahmad@gmail.com>
- Make jaccardSimilarity CJK-aware: use segmentCjk + character bigrams for overlap so no-space Chinese/Japanese/Korean text produces useful similarity scores instead of near-disjoint token sets - Add negation guard (memory-dedup.ts) so 'do X' vs 'do not X' and '使用' vs '不要使用' are not superseded despite high token overlap - Preserve original English whitespace-split behaviour when no CJK characters are present (fast path, zero regression) - Add tests: pure-function similarity (19), end-to-end CJK dedup (6) covering duplicate supersede, distinct non-supersede, negation conflict, mixed-language, and cross-project isolation
- Add mem::update engine function: updates existing memory by ID, increments version, preserves fields not provided, re-indexes BM25 + vector index, records audit trail - Add REST API endpoint PUT /agentmemory/memories/update - Add memory_update MCP tool definition in tools-registry.ts - Add memory_update to standalone server (validate, handleLocal, handleProxy) so it works in both connected and fallback modes - Add 9 tests covering: version increment, in-place update (no duplicate), field preservation, type/concept overwrite, not-found error, missing memoryId, missing content, multi-update versioning, no-duplicate guarantee - Update existing standalone test counts (CORE_TOOLS 14→15, IMPLEMENTED_TOOLS 7→8)
- Package name: ziiagentmemory (was @agentmemory/agentmemory) - Author: Zeeshan Ahmad - Repository: github.com/ziishanahmad/ziiagentmemory - Bin: ziiagentmemory + agentmemory (backward compat) - Public access, npm registry
SNAPSHOT_ENABLED and SNAPSHOT_INTERVAL were read and logged but never passed to setInterval. The snapshot function was registered but only fired on manual trigger (MCP tool or POST /snapshot/create). Add setInterval(sdk.trigger(mem::snapshot-create), interval * 1000) with .unref() matching the pattern used by auto-forget, lesson-decay, insight-decay, and consolidation timers.
ResilientProvider renames 'noop' to 'resilient(noop)', so the provider.name === 'noop' check in mem::summarize never matched. Zero-LLM installs got misleading parse failures instead of a clean no-op skip. Now checks both names.
The api::consolidate handler had no try/catch, so when mem::consolidate
threw an Error the engine's default handler stringified it as
'[object Object]' instead of the actual .message. Now catches errors
and returns { error: message } with a 500 status.
…itg00#1002) dimensions was hardcoded to 1536 regardless of model. With qwen/qwen3-embedding-8b (4096-dim), vectors were silently dropped. Now uses a MODEL_DIMENSIONS table (matching the OpenAI provider pattern) and supports OPENROUTER_EMBEDDING_DIMENSIONS env override for models not in the table.
) post-tool-use recorded every tool call to /agentmemory/observe with no filtering — including agentmemory's own MCP tools (memory_smart_search, memory_recall, etc.). Searching memory created new observations, which then surfaced in later recall results, creating a feedback loop that polluted the corpus and wasted context. Two changes: 1. Skip observation capture for tools whose name starts with 'memory_' or 'mem::' (self-capture filter). 2. Add AGENTMEMORY_CAPTURE_TOOL_USE env override (default: true, set to false to disable all post-tool-use observation capture). Mirrors the opt-in/opt-out pattern used by AGENTMEMORY_INJECT_CONTEXT. Verified locally: - memory_smart_search tool → 0 observations (filtered) - read_file tool → 1 observation (captured normally) - write_file + AGENTMEMORY_CAPTURE_TOOL_USE=false → 0 observations Signed-off-by: Zeeshan Ahmad <ziishanahmad@gmail.com>
|
@ziishanahmad is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR renames the npm package to ziiagentmemory, adds a new mem::update memory-versioning function surfaced through MCP tools and a REST endpoint, adds negation-aware and CJK-aware deduplication for mem::remember, fixes post-tool-use self-capture, snapshot interval scheduling, session pagination, consolidate error handling, and OpenRouter embedding dimensions. ChangesPackage Rebrand
mem::update Memory Versioning
Negation-Aware and CJK-Aware Dedup
post-tool-use Self-Capture Prevention
Snapshot Interval Timer
Session Pagination and Consolidate Hardening
OpenRouter Embedding Dimensions
Summarize Noop Detection
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (3)
test/sessions-limit.test.ts (1)
15-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSource-level regex assertions are fragile — consider exporting
parsePositiveLimitfor direct unit testing.These tests verify code patterns, not behavior. Renaming
sliced/filteredor refactoring the slicing expression would break tests even if behavior is correct, while a brokenparsePositiveLimitimplementation that still matches the regex would pass silently.
parsePositiveLimitis a pure function with no engine dependency. Exporting it and adding direct unit tests (negative, zero, non-numeric, undefined, valid positive) would provide real behavioral coverage at minimal cost.♻️ Proposed refactor: export and unit-test `parsePositiveLimit`
In
src/triggers/api.ts:-function parsePositiveLimit(raw: unknown): number | undefined { +export function parsePositiveLimit(raw: unknown): number | undefined {New test file
test/parse-positive-limit.test.ts:import { describe, it, expect } from "vitest"; import { parsePositiveLimit } from "../src/triggers/api.ts"; describe("parsePositiveLimit", () => { it("returns the number for positive integers", () => { expect(parsePositiveLimit(5)).toBe(5); expect(parsePositiveLimit("10")).toBe(10); }); it("returns undefined for non-positive values", () => { expect(parsePositiveLimit(0)).toBeUndefined(); expect(parsePositiveLimit(-1)).toBeUndefined(); expect(parsePositiveLimit("-5")).toBeUndefined(); }); it("returns undefined for non-numeric and empty values", () => { expect(parsePositiveLimit("abc")).toBeUndefined(); expect(parsePositiveLimit(undefined)).toBeUndefined(); expect(parsePositiveLimit(null)).toBeUndefined(); expect(parsePositiveLimit("")).toBeUndefined(); }); });🤖 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/sessions-limit.test.ts` around lines 15 - 55, The current tests in sessions-limit.test.ts rely on source-level regex matching, which is fragile and can miss real regressions in parsePositiveLimit. Export parsePositiveLimit from the api trigger module (the same module that defines sdk.registerFunction for api::sessions and api::replay::sessions), then add direct unit tests that assert its behavior for positive, zero, negative, non-numeric, empty, and undefined inputs. Keep the existing session limit behavior covered through behavior-focused assertions where possible, rather than matching internal variable names like filtered or sliced.Source: Learnings
src/functions/remember.ts (1)
245-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the already-captured
nowinstead of a freshDate.now().Per repo convention, timestamps should be captured once and reused; this branch calls
Date.now()independently ofnowcomputed above (line 203). Low impact (mirrors an existing pattern inmem::remember), but worth aligning here since this is new code.🧹 Proposed fix
- updated.forgetAfter = new Date( - Date.now() + data.ttlDays * 86400000, - ).toISOString(); + updated.forgetAfter = new Date( + Date.parse(now) + data.ttlDays * 86400000, + ).toISOString();As per path instructions for
src/**/*.ts: "Timestamps should be captured once withnew Date().toISOString()and reused."🤖 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/remember.ts` around lines 245 - 249, The TTL handling in remember.ts should reuse the already captured now timestamp instead of calling Date.now() again. Update the forgetAfter calculation in the ttlDays branch to derive from the existing now value used earlier in the function, keeping timestamp generation consistent with the mem::remember pattern and the src/**/*.ts convention. Use the existing remember-related symbols such as data.ttlDays, updated.forgetAfter, and the surrounding now capture to locate the change.Source: Path instructions
test/memory-update.test.ts (1)
76-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting on
supersedesto catch the self-referential regression.None of the tests inspect
memory.supersedesafter an update. Adding an assertion (e.g.expect(updated.memory.supersedes).not.toContain(updated.memory.id)) would have caught the bug flagged insrc/functions/remember.tswhere the record's own id gets appended to its ownsupersedesarray.Also applies to: 232-246
🤖 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/memory-update.test.ts` around lines 76 - 104, The update test for mem::update only checks version, content, and isLatest, so it misses the self-referential supersedes regression. In the memory update flow exercised by sdk.trigger in this test, add an assertion on updated.memory.supersedes to ensure it does not include the updated memory’s own id and still reflects the previous version correctly; use the existing saved.memory.id and updated.memory fields so this catches the bug in src/functions/remember.ts.
🤖 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 `@package.json`:
- Around line 2-4: Update README.md to use the rebranded package and CLI names
instead of the old agentmemory references. Replace the installation example that
mentions `@agentmemory/agentmemory` with ziiagentmemory, and change any CLI
usage/examples from agentmemory to ziiagentmemory so users install and run the
correct command. Also scan the repo for stale agentmemory references and update
any user-facing docs or examples that still point to the old package/bin names.
- Around line 2-4: The publish workflow is still checking the old npm package
name, causing the pre-publish existence check and registry propagation wait to
query the wrong package. Update the package references in
`.github/workflows/publish.yml` to use the renamed package from `package.json`
(`ziiagentmemory`) in both the publish guard and the propagation verification
step, keeping the `npm publish` flow aligned with the actual package name.
In `@src/functions/remember.ts`:
- Around line 189-195: The mem::update path in remember.ts is missing validation
for concepts and files, unlike mem::remember, so add the same array-type checks
before persisting updates. Update the validation logic in the update flow that
builds the Memory record so data.concepts and data.files must be arrays when
present, and reject non-array values before reaching the existing data.concepts
?? existing.concepts / data.files ?? existing.files assignment. Use
mem::remember as the reference behavior and keep the checks close to the update
handler that writes the Memory object so downstream consumers continue to
receive string[] values.
- Around line 251-257: The duplicate KV write in remember.ts is unnecessary
because existing.id and updated.id point to the same record, so the first write
is immediately overwritten. In the memory update flow around the
existing/updated kv.set calls, remove the intermediate persist of existing and
write only the final updated object (with the latest version state applied) once
via kv.set(KV.memories, updated.id, updated) to avoid wasted I/O and transient
inconsistent state.
- Around line 233-236: The `supersedes` assignment in `remember` is
self-referential because `mem::update` keeps the same `existing.id`, so
appending `existing.id` just records the memory as superseding itself and grows
duplicates on every update. Update the logic around the `mem::update` path to
stop adding the current id to `supersedes`; if history is needed, store it in a
separate version-history field instead of reusing `supersedes`. Keep
`mem::remember` behavior unchanged, since it correctly references a distinct
prior memory id.
- Around line 197-198: The mutation paths in remember.ts use different lock
keys, which allows mem::remember and mem::update to race on the same KV.memories
records. Make both write paths serialize through a shared lock in the
remember/update flow by changing withKeyedLock usage in the mem::update path and
the corresponding mem::remember supersede flow to use the same lock name,
ideally keyed by the target memoryId or supersededId so the same record cannot
be read and written concurrently. Ensure the locking logic is applied around the
read-then-write sections in the remember/update functions.
- Around line 277-282: The audit call in recordAudit for the memory update flow
is using an invalid operation literal, so fix the remember.ts update path by
replacing "update" with one of the existing AuditEntry["operation"] values used
in src/types.ts, or extend the audit operation union if memory updates need a
dedicated event. Keep the change localized around the recordAudit invocation in
the update logic that builds the mem::update audit entry.
In `@src/mcp/standalone.ts`:
- Around line 322-327: The local fallback in the update path is adding the
record’s own memory id into supersedes, causing self-references to accumulate on
every in-place update. Update the logic in the standalone memory update flow so
the supersedes list only carries prior ids from the existing record and never
appends v.memoryId! when it is the same as the current record id, mirroring the
fix needed in mem::update/remember.ts.
- Around line 207-220: The memory_update handler is overwriting existing
concepts/files with empty arrays when those fields are omitted. Update the
standalone proxy path in memory_update so it only includes concepts and files
when the input actually provided them, matching the local path behavior and
avoiding accidental clearing; use the existing memory_update case and
normalizeList call sites to ensure absent fields stay undefined instead of [].
In `@src/providers/embedding/openrouter.ts`:
- Around line 45-49: The OpenRouter embedding path is not forwarding the
configured embedding size, so the override is computed in the constructor but
never used. Update the request-building flow in OpenRouterEmbeddingProvider to
pass this.dimensions into the embedding request when
OPENROUTER_EMBEDDING_DIMENSIONS is set, so withDimensionGuard does not reject
valid text-embedding-3-* dimensions. Use the existing this.dimensions field and
the embedding request method in openrouter.ts to locate the change.
In `@src/state/memory-dedup.ts`:
- Line 16: The CJK negation guard in CJK_NEGATION is too broad because
single-character matches like 不, 无, and 안 can trigger on common affirmative
words and block valid dedup. Narrow the pattern by removing ambiguous
single-character entries, especially 안, and replace 不 with more specific
negation compounds or multi-character phrases that represent true negation.
Update the regex in memory-dedup.ts so the negation check remains useful without
causing false positives in the dedup logic.
In `@src/triggers/api.ts`:
- Around line 1030-1047: The `mem::update` trigger path in `src/triggers/api.ts`
forwards `req.body.concepts` and `req.body.files` without validating their
shape, so add boundary checks in this handler before building the `payload`.
Mirror the existing `content` validation by ensuring `concepts` and `files` are
either omitted or arrays (reject malformed values with a 400 response), and keep
the payload construction in the `sdk.trigger` call unchanged except for only
passing validated values.
- Around line 1013-1054: The api::memory-update handler currently calls
sdk.trigger without the same error guard used in api::consolidate, so thrown
errors can bypass the intended response handling. Wrap the sdk.trigger call in
try/catch inside api::memory-update, return a proper error response from the
catch path, and keep the existing 404 handling for the "memory not found" result
while preserving the payload shape used by mem::update.
- Around line 833-840: The api::sessions flow is applying the limit before
ordering, so the returned page depends on kv.list() rather than startedAt.
Update the logic around the sessions listing in api.ts to sort the KV results
descending by startedAt first, then apply parsePositiveLimit and slice, matching
the behavior of api::replay::sessions.
In `@test/snapshot-interval-timer.test.ts`:
- Around line 40-82: The snapshot interval timer tests are not exercising the
runtime code, so the fix in main/index is never validated. Update the tests to
import and invoke main() (or extract and call a helper around the snapshot
scheduling logic), then assert setIntervalCalls contains the expected 3600000ms
interval and that the scheduled callback triggers mem::snapshot-create. Remove
the tautological expectation and make sure the vi.mock setup is actually used by
the code under test.
---
Nitpick comments:
In `@src/functions/remember.ts`:
- Around line 245-249: The TTL handling in remember.ts should reuse the already
captured now timestamp instead of calling Date.now() again. Update the
forgetAfter calculation in the ttlDays branch to derive from the existing now
value used earlier in the function, keeping timestamp generation consistent with
the mem::remember pattern and the src/**/*.ts convention. Use the existing
remember-related symbols such as data.ttlDays, updated.forgetAfter, and the
surrounding now capture to locate the change.
In `@test/memory-update.test.ts`:
- Around line 76-104: The update test for mem::update only checks version,
content, and isLatest, so it misses the self-referential supersedes regression.
In the memory update flow exercised by sdk.trigger in this test, add an
assertion on updated.memory.supersedes to ensure it does not include the updated
memory’s own id and still reflects the previous version correctly; use the
existing saved.memory.id and updated.memory fields so this catches the bug in
src/functions/remember.ts.
In `@test/sessions-limit.test.ts`:
- Around line 15-55: The current tests in sessions-limit.test.ts rely on
source-level regex matching, which is fragile and can miss real regressions in
parsePositiveLimit. Export parsePositiveLimit from the api trigger module (the
same module that defines sdk.registerFunction for api::sessions and
api::replay::sessions), then add direct unit tests that assert its behavior for
positive, zero, negative, non-numeric, empty, and undefined inputs. Keep the
existing session limit behavior covered through behavior-focused assertions
where possible, rather than matching internal variable names like filtered or
sliced.
🪄 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: 6a497100-e3b2-41c1-9b18-bfe05b91c8aa
📒 Files selected for processing (18)
package.jsonsrc/functions/remember.tssrc/functions/summarize.tssrc/hooks/post-tool-use.tssrc/index.tssrc/mcp/standalone.tssrc/mcp/tools-registry.tssrc/providers/embedding/openrouter.tssrc/state/memory-dedup.tssrc/state/schema.tssrc/triggers/api.tstest/mcp-standalone-proxy.test.tstest/mcp-standalone.test.tstest/memory-update.test.tstest/remember-cjk-dedup.test.tstest/remember-dedup-similarity.test.tstest/sessions-limit.test.tstest/snapshot-interval-timer.test.ts
| "name": "ziiagentmemory", | ||
| "version": "0.3.0", | ||
| "description": "Persistent memory for AI coding agents — CJK-aware dedup, memory_update versioning, standalone MCP server", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
README.md still references the old package name and CLI command.
README.md (lines 88-104) references @agentmemory/agentmemory for installation and the agentmemory CLI command. With the rebrand to ziiagentmemory and bin key ziiagentmemory, users following README instructions will install the wrong package or get a "command not found" error.
Run the following script to find all stale references:
#!/bin/bash
# Search for old package name and CLI command references across the repo
rg -n 'agentmemory' --glob '!package.json' --glob '!node_modules' --glob '!.git' | head -50Also applies to: 17-17
🤖 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 `@package.json` around lines 2 - 4, Update README.md to use the rebranded
package and CLI names instead of the old agentmemory references. Replace the
installation example that mentions `@agentmemory/agentmemory` with ziiagentmemory,
and change any CLI usage/examples from agentmemory to ziiagentmemory so users
install and run the correct command. Also scan the repo for stale agentmemory
references and update any user-facing docs or examples that still point to the
old package/bin names.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
CI publish workflow still references the old @agentmemory/agentmemory package name.
The .github/workflows/publish.yml workflow checks npm view "@agentmemory/agentmemory@$(node -p "require('./package.json').version")" before publishing and again during registry propagation verification. With the package renamed to ziiagentmemory, these checks will query the wrong package — the pre-publish existence check will always report "not found" (causing re-publishes) and the propagation wait will never succeed, failing the job.
The npm publish step itself will publish ziiagentmemory (read from package.json), so the check/publish mismatch is the core defect.
Update the workflow to reference ziiagentmemory:
# .github/workflows/publish.yml
- name: Publish `@agentmemory/agentmemory`
+ - name: Publish ziiagentmemory
run: |
- if npm view "`@agentmemory/agentmemory`@$(node -p "require('./package.json').version")" version >/dev/null 2>&1; then
+ if npm view "ziiagentmemory@$(node -p "require('./package.json').version")" version >/dev/null 2>&1; then
echo "Version already published, skipping"
else
npm publish --provenance --access public
fiAnd in the propagation wait step:
- if npm view "`@agentmemory/agentmemory`@$VERSION" version >/dev/null 2>&1; then
+ if npm view "ziiagentmemory@$VERSION" version >/dev/null 2>&1; then🤖 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 `@package.json` around lines 2 - 4, The publish workflow is still checking the
old npm package name, causing the pre-publish existence check and registry
propagation wait to query the wrong package. Update the package references in
`.github/workflows/publish.yml` to use the renamed package from `package.json`
(`ziiagentmemory`) in both the publish guard and the propagation verification
step, keeping the `npm publish` flow aligned with the actual package name.
| if ( | ||
| !data.content || | ||
| typeof data.content !== "string" || | ||
| !data.content.trim() | ||
| ) { | ||
| return { success: false, error: "content is required" }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
mem::update doesn't validate concepts/files are arrays, unlike mem::remember.
mem::remember explicitly rejects non-array concepts/files (lines 33-38), but mem::update accepts them unchecked and writes data.concepts ?? existing.concepts / data.files ?? existing.files straight into the persisted Memory record (typed string[]). A caller passing a string instead of an array corrupts the stored schema and can break downstream consumers (BM25 indexing, exports) that assume array semantics.
🛡️ Proposed fix
if (
!data.content ||
typeof data.content !== "string" ||
!data.content.trim()
) {
return { success: false, error: "content is required" };
}
+ if (data.concepts && !Array.isArray(data.concepts)) {
+ return { success: false, error: "concepts must be an array" };
+ }
+ if (data.files && !Array.isArray(data.files)) {
+ return { success: false, error: "files must be an array" };
+ }Also applies to: 229-230
🤖 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/remember.ts` around lines 189 - 195, The mem::update path in
remember.ts is missing validation for concepts and files, unlike mem::remember,
so add the same array-type checks before persisting updates. Update the
validation logic in the update flow that builds the Memory record so
data.concepts and data.files must be arrays when present, and reject non-array
values before reaching the existing data.concepts ?? existing.concepts /
data.files ?? existing.files assignment. Use mem::remember as the reference
behavior and keep the checks close to the update handler that writes the Memory
object so downstream consumers continue to receive string[] values.
| return withKeyedLock("mem:update", async () => { | ||
| const existing = await kv.get<Memory>(KV.memories, data.memoryId); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Lock key mismatch lets mem::remember and mem::update interleave on the same record.
mem::remember locks on "mem:remember" while mem::update locks on "mem:update". Since both read-then-write the same KV.memories record set (remember can supersede any isLatest memory by similarity, update targets by id directly), a concurrent mem::remember call whose content is jaccard-similar to the memory being updated can read the pre-update state, then both writers race to the same key — last write wins and one update is silently lost.
🔒 Suggested direction
- return withKeyedLock("mem:update", async () => {
+ return withKeyedLock("mem:remember", async () => {Using the same lock name serializes both mutation paths against KV.memories. A more scalable fix would be a shared lock keyed by data.memoryId that mem::remember's supersede-scan also acquires once it picks a supersededId.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return withKeyedLock("mem:update", async () => { | |
| const existing = await kv.get<Memory>(KV.memories, data.memoryId); | |
| return withKeyedLock("mem:remember", async () => { | |
| const existing = await kv.get<Memory>(KV.memories, data.memoryId); |
🤖 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/remember.ts` around lines 197 - 198, The mutation paths in
remember.ts use different lock keys, which allows mem::remember and mem::update
to race on the same KV.memories records. Make both write paths serialize through
a shared lock in the remember/update flow by changing withKeyedLock usage in the
mem::update path and the corresponding mem::remember supersede flow to use the
same lock name, ideally keyed by the target memoryId or supersededId so the same
record cannot be read and written concurrently. Ensure the locking logic is
applied around the read-then-write sections in the remember/update functions.
| version: newVersion, | ||
| parentId: existing.parentId, | ||
| supersedes: [...(existing.supersedes ?? []), existing.id], | ||
| sourceObservationIds: existing.sourceObservationIds, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
supersedes becomes self-referential — existing.id always equals updated.id.
Because mem::update is an in-place update (id: existing.id), existing.id and updated.id are the same string. Appending existing.id to existing.supersedes therefore makes the memory list itself as something it supersedes, and every subsequent update appends another duplicate of its own id — the array grows unbounded with no meaningful history (contrast with mem::remember, where supersededId is a genuinely different, prior memory's id).
🐛 Proposed fix
- supersedes: [...(existing.supersedes ?? []), existing.id],
+ supersedes: existing.supersedes ?? [],If version history is intended, track it via a separate field (e.g. version log) rather than supersedes, which is otherwise used to reference distinct prior memory ids.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| version: newVersion, | |
| parentId: existing.parentId, | |
| supersedes: [...(existing.supersedes ?? []), existing.id], | |
| sourceObservationIds: existing.sourceObservationIds, | |
| version: newVersion, | |
| parentId: existing.parentId, | |
| supersedes: existing.supersedes ?? [], | |
| sourceObservationIds: existing.sourceObservationIds, |
🤖 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/remember.ts` around lines 233 - 236, The `supersedes`
assignment in `remember` is self-referential because `mem::update` keeps the
same `existing.id`, so appending `existing.id` just records the memory as
superseding itself and grows duplicates on every update. Update the logic around
the `mem::update` path to stop adding the current id to `supersedes`; if history
is needed, store it in a separate version-history field instead of reusing
`supersedes`. Keep `mem::remember` behavior unchanged, since it correctly
references a distinct prior memory id.
| // Mark the old version as no longer latest. | ||
| existing.isLatest = false; | ||
| existing.updatedAt = now; | ||
| await kv.set(KV.memories, existing.id, existing); | ||
|
|
||
| // Save the updated memory in-place (same ID, new version). | ||
| await kv.set(KV.memories, updated.id, updated); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Redundant/wasted KV write: existing is written then immediately clobbered by updated at the same key.
existing.id === updated.id, so kv.set(KV.memories, existing.id, existing) and the very next kv.set(KV.memories, updated.id, updated) target the identical KV key. The first write's effect (isLatest=false) is never observable in the final persisted state — it's just extra I/O, and it briefly exposes a transient inconsistent state (stale content marked not-latest) to any concurrent reader for no lasting benefit.
♻️ Proposed fix
- // Mark the old version as no longer latest.
- existing.isLatest = false;
- existing.updatedAt = now;
- await kv.set(KV.memories, existing.id, existing);
-
// Save the updated memory in-place (same ID, new version).
await kv.set(KV.memories, updated.id, updated);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Mark the old version as no longer latest. | |
| existing.isLatest = false; | |
| existing.updatedAt = now; | |
| await kv.set(KV.memories, existing.id, existing); | |
| // Save the updated memory in-place (same ID, new version). | |
| await kv.set(KV.memories, updated.id, updated); | |
| // Save the updated memory in-place (same ID, new version). | |
| await kv.set(KV.memories, updated.id, updated); |
🤖 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/remember.ts` around lines 251 - 257, The duplicate KV write in
remember.ts is unnecessary because existing.id and updated.id point to the same
record, so the first write is immediately overwritten. In the memory update flow
around the existing/updated kv.set calls, remove the intermediate persist of
existing and write only the final updated object (with the latest version state
applied) once via kv.set(KV.memories, updated.id, updated) to avoid wasted I/O
and transient inconsistent state.
| */ | ||
|
|
||
| const EN_NEGATION = /\b(not|never|don't|do not|cannot|should not|must not|no)\b/i; | ||
| const CJK_NEGATION = /不要|别|勿|無|无|不|なし|않|안|못/i; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
CJK negation patterns 不, 无, 안 are too broad and will cause false positives.
These single characters appear frequently in non-negation contexts:
不in 不断 (continuously), 不过 (however), 不仅 (not only)无in 无论 (regardless), 无限 (unlimited)안in Korean 안전 (safety), 안정 (stability), 안녕 (hello)
When the guard fires on affirmative text, it prevents legitimate dedup of similar CJK memories — the very feature this PR adds. For Korean, 안 would trigger on common words like 안전, making CJK dedup nearly unusable.
Consider replacing single-character patterns with multi-character negation phrases, or at minimum removing 안 and narrowing 不 to common negation compounds:
🛡️ Suggested fix
-const CJK_NEGATION = /不要|别|勿|無|无|不|なし|않|안|못/i;
+const CJK_NEGATION = /不要|不用|不会|不能|不可|不该|不是|别|勿|勿要|沒有|没有|無|なし|않|못/i;This keeps 别, 勿, 沒/没, 無, なし, 않, 못 (which are less ambiguous) and replaces bare 不 with common two-character negation compounds. 안 is removed entirely as it's too ambiguous in Korean.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const CJK_NEGATION = /不要|别|勿|無|无|不|なし|않|안|못/i; | |
| const CJK_NEGATION = /不要|不用|不会|不能|不可|不该|不是|别|勿|勿要|沒有|没有|無|なし|않|못/i; |
🤖 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/memory-dedup.ts` at line 16, The CJK negation guard in CJK_NEGATION
is too broad because single-character matches like 不, 无, and 안 can trigger on
common affirmative words and block valid dedup. Narrow the pattern by removing
ambiguous single-character entries, especially 안, and replace 不 with more
specific negation compounds or multi-character phrases that represent true
negation. Update the regex in memory-dedup.ts so the negation check remains
useful without causing false positives in the dedup logic.
| const limit = parsePositiveLimit(req.query_params?.["limit"]); | ||
| const sliced = limit !== undefined ? filtered.slice(0, limit) : filtered; | ||
| const summaries = await Promise.all( | ||
| filtered.map((s) => | ||
| sliced.map((s) => | ||
| kv.get<SessionSummary>(KV.summaries, s.id).catch(() => null), | ||
| ), | ||
| ); | ||
| const withSummary = filtered.map((s, i) => | ||
| const withSummary = sliced.map((s, i) => |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant endpoint regions.
sed -n '780,870p' src/triggers/api.ts
printf '\n--- replay sessions ---\n'
sed -n '480,530p' src/triggers/api.ts
printf '\n--- search for kv.list<Session>(KV.sessions) and startedAt sorting ---\n'
rg -n "kv\.list<Session>\(KV\.sessions\)|startedAt.*localeCompare|sort\(" src/triggers/api.ts src -g '!**/dist/**' -g '!**/build/**'Repository: rohitg00/agentmemory
Length of output: 17144
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '780,870p' src/triggers/api.ts
printf '\n--- replay sessions ---\n'
sed -n '480,530p' src/triggers/api.ts
printf '\n--- search for kv.list<Session>(KV.sessions) and startedAt sorting ---\n'
rg -n "kv\.list<Session>\(KV\.sessions\)|startedAt.*localeCompare|sort\(" src/triggers/api.ts src -g '!**/dist/**' -g '!**/build/**'Repository: rohitg00/agentmemory
Length of output: 17144
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the kv implementation or typing so we can understand iteration order.
rg -n "list<|function list|class .*KV|interface .*KV|export .*list" src . -g '!**/dist/**' -g '!**/build/**'
printf '\n--- sessions endpoint ---\n'
sed -n '810,850p' src/triggers/api.ts
printf '\n--- replay sessions endpoint ---\n'
sed -n '500,520p' src/triggers/api.tsRepository: rohitg00/agentmemory
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/mcp/in-memory-kv.ts ---'
cat -n src/mcp/in-memory-kv.ts | sed -n '1,140p'
printf '\n%s\n' '--- src/state/kv.ts ---'
cat -n src/state/kv.ts | sed -n '1,140p'
printf '\n%s\n' '--- sessions write paths ---'
rg -n "KV\.sessions|mem:sessions|sessions\)" src -g '!**/dist/**' -g '!**/build/**' | sed -n '1,120p'Repository: rohitg00/agentmemory
Length of output: 11695
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'function_id:\s*"state::list"|state::list|registerFunction\("state::list"|registerTrigger\(\{[^}]*state::list' src test -g '!**/dist/**' -g '!**/build/**'
printf '\n--- likely kv state implementation areas ---\n'
rg -n 'list<T = unknown>\(scope: string\)|entries\.values\(\)|Object\.values|Map<.*>\(\)' src/state src/mcp src -g '!**/dist/**' -g '!**/build/**'Repository: rohitg00/agentmemory
Length of output: 6811
Sort api::sessions before applying limit. This endpoint slices the raw kv.list() result, so its order depends on the KV backend rather than startedAt; match api::replay::sessions and sort descending first.
🤖 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/triggers/api.ts` around lines 833 - 840, The api::sessions flow is
applying the limit before ordering, so the returned page depends on kv.list()
rather than startedAt. Update the logic around the sessions listing in api.ts to
sort the KV results descending by startedAt first, then apply parsePositiveLimit
and slice, matching the behavior of api::replay::sessions.
| sdk.registerFunction("api::memory-update", | ||
| async ( | ||
| req: ApiRequest<{ | ||
| memoryId: string; | ||
| content: string; | ||
| type?: string; | ||
| concepts?: string[]; | ||
| files?: string[]; | ||
| ttlDays?: number; | ||
| agentId?: string; | ||
| }>, | ||
| ): Promise<Response> => { | ||
| const authErr = checkAuth(req, secret); | ||
| if (authErr) return authErr; | ||
| if (!req.body?.memoryId) { | ||
| return { status_code: 400, body: { error: "memoryId is required" } }; | ||
| } | ||
| if ( | ||
| !req.body?.content || | ||
| typeof req.body.content !== "string" || | ||
| !req.body.content.trim() | ||
| ) { | ||
| return { status_code: 400, body: { error: "content is required" } }; | ||
| } | ||
| const result = await sdk.trigger({ | ||
| function_id: "mem::update", | ||
| payload: { | ||
| memoryId: req.body.memoryId, | ||
| content: req.body.content, | ||
| ...(req.body.type !== undefined && { type: req.body.type }), | ||
| ...(req.body.concepts !== undefined && { concepts: req.body.concepts }), | ||
| ...(req.body.files !== undefined && { files: req.body.files }), | ||
| ...(req.body.ttlDays !== undefined && { ttlDays: req.body.ttlDays }), | ||
| ...(req.body.agentId !== undefined && { agentId: req.body.agentId }), | ||
| }, | ||
| }); | ||
| const r = result as { success: boolean; error?: string }; | ||
| if (!r.success && r.error === "memory not found") { | ||
| return { status_code: 404, body: result }; | ||
| } | ||
| return { status_code: 200, body: result }; | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
api::memory-update doesn't wrap sdk.trigger in try/catch, unlike the sibling fix applied to api::consolidate in this same PR.
api::consolidate (lines 1088-1107, this same diff) was just hardened to catch thrown errors because "the engine's default error handler stringifies the Error as '[object Object]'" (#1008). mem::update can throw from unguarded kv.set/recordAudit calls; without a try/catch here, this new endpoint reintroduces the exact failure mode that was just fixed a few functions down.
🔧 Proposed fix
- const result = await sdk.trigger({
- function_id: "mem::update",
- payload: {
- memoryId: req.body.memoryId,
- content: req.body.content,
- ...(req.body.type !== undefined && { type: req.body.type }),
- ...(req.body.concepts !== undefined && { concepts: req.body.concepts }),
- ...(req.body.files !== undefined && { files: req.body.files }),
- ...(req.body.ttlDays !== undefined && { ttlDays: req.body.ttlDays }),
- ...(req.body.agentId !== undefined && { agentId: req.body.agentId }),
- },
- });
- const r = result as { success: boolean; error?: string };
- if (!r.success && r.error === "memory not found") {
- return { status_code: 404, body: result };
- }
- return { status_code: 200, body: result };
+ try {
+ const result = await sdk.trigger({
+ function_id: "mem::update",
+ payload: {
+ memoryId: req.body.memoryId,
+ content: req.body.content,
+ ...(req.body.type !== undefined && { type: req.body.type }),
+ ...(req.body.concepts !== undefined && { concepts: req.body.concepts }),
+ ...(req.body.files !== undefined && { files: req.body.files }),
+ ...(req.body.ttlDays !== undefined && { ttlDays: req.body.ttlDays }),
+ ...(req.body.agentId !== undefined && { agentId: req.body.agentId }),
+ },
+ });
+ const r = result as { success: boolean; error?: string };
+ if (!r.success && r.error === "memory not found") {
+ return { status_code: 404, body: result };
+ }
+ return { status_code: 200, body: result };
+ } catch (err) {
+ const message = err instanceof Error ? err.message : String(err);
+ logger.warn("api::memory-update failed", { error: message });
+ return { status_code: 500, body: { error: message } };
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sdk.registerFunction("api::memory-update", | |
| async ( | |
| req: ApiRequest<{ | |
| memoryId: string; | |
| content: string; | |
| type?: string; | |
| concepts?: string[]; | |
| files?: string[]; | |
| ttlDays?: number; | |
| agentId?: string; | |
| }>, | |
| ): Promise<Response> => { | |
| const authErr = checkAuth(req, secret); | |
| if (authErr) return authErr; | |
| if (!req.body?.memoryId) { | |
| return { status_code: 400, body: { error: "memoryId is required" } }; | |
| } | |
| if ( | |
| !req.body?.content || | |
| typeof req.body.content !== "string" || | |
| !req.body.content.trim() | |
| ) { | |
| return { status_code: 400, body: { error: "content is required" } }; | |
| } | |
| const result = await sdk.trigger({ | |
| function_id: "mem::update", | |
| payload: { | |
| memoryId: req.body.memoryId, | |
| content: req.body.content, | |
| ...(req.body.type !== undefined && { type: req.body.type }), | |
| ...(req.body.concepts !== undefined && { concepts: req.body.concepts }), | |
| ...(req.body.files !== undefined && { files: req.body.files }), | |
| ...(req.body.ttlDays !== undefined && { ttlDays: req.body.ttlDays }), | |
| ...(req.body.agentId !== undefined && { agentId: req.body.agentId }), | |
| }, | |
| }); | |
| const r = result as { success: boolean; error?: string }; | |
| if (!r.success && r.error === "memory not found") { | |
| return { status_code: 404, body: result }; | |
| } | |
| return { status_code: 200, body: result }; | |
| }, | |
| sdk.registerFunction("api::memory-update", | |
| async ( | |
| req: ApiRequest<{ | |
| memoryId: string; | |
| content: string; | |
| type?: string; | |
| concepts?: string[]; | |
| files?: string[]; | |
| ttlDays?: number; | |
| agentId?: string; | |
| }>, | |
| ): Promise<Response> => { | |
| const authErr = checkAuth(req, secret); | |
| if (authErr) return authErr; | |
| if (!req.body?.memoryId) { | |
| return { status_code: 400, body: { error: "memoryId is required" } }; | |
| } | |
| if ( | |
| !req.body?.content || | |
| typeof req.body.content !== "string" || | |
| !req.body.content.trim() | |
| ) { | |
| return { status_code: 400, body: { error: "content is required" } }; | |
| } | |
| try { | |
| const result = await sdk.trigger({ | |
| function_id: "mem::update", | |
| payload: { | |
| memoryId: req.body.memoryId, | |
| content: req.body.content, | |
| ...(req.body.type !== undefined && { type: req.body.type }), | |
| ...(req.body.concepts !== undefined && { concepts: req.body.concepts }), | |
| ...(req.body.files !== undefined && { files: req.body.files }), | |
| ...(req.body.ttlDays !== undefined && { ttlDays: req.body.ttlDays }), | |
| ...(req.body.agentId !== undefined && { agentId: req.body.agentId }), | |
| }, | |
| }); | |
| const r = result as { success: boolean; error?: string }; | |
| if (!r.success && r.error === "memory not found") { | |
| return { status_code: 404, body: result }; | |
| } | |
| return { status_code: 200, body: result }; | |
| } catch (err) { | |
| const message = err instanceof Error ? err.message : String(err); | |
| logger.warn("api::memory-update failed", { error: message }); | |
| return { status_code: 500, body: { error: message } }; | |
| } | |
| }, |
🤖 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/triggers/api.ts` around lines 1013 - 1054, The api::memory-update handler
currently calls sdk.trigger without the same error guard used in
api::consolidate, so thrown errors can bypass the intended response handling.
Wrap the sdk.trigger call in try/catch inside api::memory-update, return a
proper error response from the catch path, and keep the existing 404 handling
for the "memory not found" result while preserving the payload shape used by
mem::update.
| if ( | ||
| !req.body?.content || | ||
| typeof req.body.content !== "string" || | ||
| !req.body.content.trim() | ||
| ) { | ||
| return { status_code: 400, body: { error: "content is required" } }; | ||
| } | ||
| const result = await sdk.trigger({ | ||
| function_id: "mem::update", | ||
| payload: { | ||
| memoryId: req.body.memoryId, | ||
| content: req.body.content, | ||
| ...(req.body.type !== undefined && { type: req.body.type }), | ||
| ...(req.body.concepts !== undefined && { concepts: req.body.concepts }), | ||
| ...(req.body.files !== undefined && { files: req.body.files }), | ||
| ...(req.body.ttlDays !== undefined && { ttlDays: req.body.ttlDays }), | ||
| ...(req.body.agentId !== undefined && { agentId: req.body.agentId }), | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
No array-shape validation for concepts/files before forwarding to mem::update.
This endpoint checks content is a non-empty string but forwards req.body.concepts/req.body.files unchecked. Combined with mem::update also lacking array validation, a malformed request can persist non-array values into the Memory record.
As per coding guidelines for {src/mcp/server.ts,src/triggers/api.ts}: "Input validation must occur at system boundaries: MCP handlers and REST endpoints."
🤖 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/triggers/api.ts` around lines 1030 - 1047, The `mem::update` trigger path
in `src/triggers/api.ts` forwards `req.body.concepts` and `req.body.files`
without validating their shape, so add boundary checks in this handler before
building the `payload`. Mirror the existing `content` validation by ensuring
`concepts` and `files` are either omitted or arrays (reject malformed values
with a 400 response), and keep the payload construction in the `sdk.trigger`
call unchanged except for only passing validated values.
Source: Coding guidelines
| describe("#1006 — snapshot interval timer", () => { | ||
| beforeEach(() => { | ||
| setIntervalCalls = []; | ||
| // Wrap setInterval to capture calls | ||
| global.setInterval = ((cb: Function, ms?: number) => { | ||
| const result = originalSetInterval(cb, ms); | ||
| setIntervalCalls.push({ cb, ms: ms ?? 0 }); | ||
| return result; | ||
| }) as typeof global.setInterval; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| global.setInterval = originalSetInterval; | ||
| }); | ||
|
|
||
| it("loadSnapshotConfig returns interval value", () => { | ||
| const config = loadSnapshotConfig(); | ||
| expect(config.enabled).toBe(true); | ||
| expect(config.interval).toBe(3600); | ||
| }); | ||
|
|
||
| it("the fix adds setInterval for snapshot-create when enabled", () => { | ||
| // The fix in index.ts adds: | ||
| // const snapshotIntervalMs = snapshotConfig.interval * 1000; | ||
| // const snapshotTimer = setInterval(async () => { | ||
| // await sdk.trigger({ function_id: "mem::snapshot-create", payload: {} }); | ||
| // }, snapshotIntervalMs); | ||
| // snapshotTimer.unref(); | ||
| // | ||
| // We verify the math: 3600s * 1000 = 3600000ms | ||
| const config = loadSnapshotConfig(); | ||
| const expectedMs = config.interval * 1000; | ||
| expect(expectedMs).toBe(3600000); | ||
|
|
||
| // Simulate what the fix does | ||
| const dummyCb = async () => {}; | ||
| const timer = originalSetInterval(dummyCb, expectedMs); | ||
| timer.unref(); | ||
| clearInterval(timer); | ||
|
|
||
| // If we got here without throwing, the timer creation works | ||
| expect(true).toBe(true); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Tests don't exercise the code under test.
Neither test imports or invokes main() from src/index.ts, so the actual fix is never exercised. The setIntervalCalls capture array is populated but never asserted against, and Test 2 ends with expect(true).toBe(true) — a tautology that passes regardless of the implementation. The vi.mock calls are effectively no-ops since no runtime code imports the mocked modules.
To properly verify the fix, the test should import and call main() (or the relevant subset), then assert that setIntervalCalls contains an entry with ms === 3600000 and that the callback triggers mem::snapshot-create.
Based on learnings, when reviewing tests that use vi.mock(...), confirm the module is used at runtime by the code under test; if the only usage is via type-only imports, the mock is a no-op.
♻️ Suggested test approach
import { loadSnapshotConfig } from "../src/config.js";
+import { main } from "../src/index.js";
describe("`#1006` — snapshot interval timer", () => {
beforeEach(() => {
setIntervalCalls = [];
global.setInterval = ((cb: Function, ms?: number) => {
const result = originalSetInterval(cb, ms);
setIntervalCalls.push({ cb, ms: ms ?? 0 });
return result;
}) as typeof global.setInterval;
});
afterEach(() => {
global.setInterval = originalSetInterval;
});
- it("loadSnapshotConfig returns interval value", () => {
- const config = loadSnapshotConfig();
- expect(config.enabled).toBe(true);
- expect(config.interval).toBe(3600);
- });
-
- it("the fix adds setInterval for snapshot-create when enabled", () => {
- const config = loadSnapshotConfig();
- const expectedMs = config.interval * 1000;
- expect(expectedMs).toBe(3600000);
- const dummyCb = async () => {};
- const timer = originalSetInterval(dummyCb, expectedMs);
- timer.unref();
- clearInterval(timer);
- expect(true).toBe(true);
- });
+ it("schedules setInterval with interval * 1000 when snapshots are enabled", async () => {
+ await main();
+ const snapshotCall = setIntervalCalls.find(
+ (c) => c.ms === 3600 * 1000,
+ );
+ expect(snapshotCall).toBeDefined();
+ });
});Note: main() will need additional mocks for registerWorker, StateKV, and other dependencies. Consider extracting the snapshot-scheduling block into a testable helper if mocking the full main() is too heavy.
🤖 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/snapshot-interval-timer.test.ts` around lines 40 - 82, The snapshot
interval timer tests are not exercising the runtime code, so the fix in
main/index is never validated. Update the tests to import and invoke main() (or
extract and call a helper around the snapshot scheduling logic), then assert
setIntervalCalls contains the expected 3600000ms interval and that the scheduled
callback triggers mem::snapshot-create. Remove the tautological expectation and
make sure the vi.mock setup is actually used by the code under test.
Source: Learnings
1 participant
Problem
post-tool-userecorded every tool call to/agentmemory/observewith no filtering — including agentmemory's own MCP tools (memory_smart_search,memory_recall, etc.). Searching memory created new observations, which then surfaced in later recall results, creating a feedback loop that polluted the corpus and wasted context.Fix
Two changes in
src/hooks/post-tool-use.ts:Self-capture filter: Skip observation capture for any tool whose name starts with
memory_ormem::. This prevents agentmemory's own MCP tools from creating observations that pollute recall.Env override: Add
AGENTMEMORY_CAPTURE_TOOL_USEenv var (default:true, set tofalseto disable all post-tool-use observation capture). Mirrors the opt-in/opt-out pattern already used byAGENTMEMORY_INJECT_CONTEXTonpre-tool-use.Verification
Tested locally against running engine on
localhost:3111:memory_smart_searchread_filewrite_fileAGENTMEMORY_CAPTURE_TOOL_USE=falseCloses #993
Summary by CodeRabbit
New Features
Bug Fixes