fix: consolidate endpoint returns proper error message (#1008)#1041
Open
ziishanahmad wants to merge 12 commits into
Open
fix: consolidate endpoint returns proper error message (#1008)#1041ziishanahmad wants to merge 12 commits into
ziishanahmad wants to merge 12 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.
|
@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. |
Contributor
📝 WalkthroughWalkthroughThis PR adds a versioned ChangesMemory update, dedup, and API changes
Estimated code review effort: 3 (Moderate) | ~35 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant MemUpdateFn
participant KVStore
participant SearchIndex
participant AuditLog
Caller->>MemUpdateFn: mem::update(memoryId, content, ...)
MemUpdateFn->>KVStore: load existing memory
MemUpdateFn->>KVStore: mark old version isLatest=false
MemUpdateFn->>KVStore: persist new version
MemUpdateFn->>SearchIndex: remove old, add updated BM25/vector entries
MemUpdateFn->>AuditLog: record old/new version
MemUpdateFn-->>Caller: updated memory
sequenceDiagram
participant MCPClient
participant StandaloneShim
participant RemoteServer
participant LocalKV
MCPClient->>StandaloneShim: call memory_update(memoryId, content)
alt proxy mode
StandaloneShim->>RemoteServer: PUT /agentmemory/memories/update
RemoteServer-->>StandaloneShim: updated memory response
else local fallback
StandaloneShim->>LocalKV: load and update memory by id
LocalKV-->>StandaloneShim: updated memory
end
StandaloneShim-->>MCPClient: pretty-printed result
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/triggers/api.ts (1)
1094-1096: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRaw
req.bodypassed tosdk.triggerwithout whitelisting.
payload: req.bodyforwards the raw request body unfiltered. Per the path instruction for this file, REST endpoints should whitelist fields explicitly rather than passing the body through as-is (seeapi::rememberorapi::memory-updatea few lines above for the whitelisted pattern). This is a pre-existing pattern also present elsewhere in the file (e.g.api::forget,api::file-context), so it's not a new regression from this PR, but it's worth tightening here since the payload shape (project,minObservations) is already well-known.As per path instructions: "REST endpoints must whitelist fields — never pass raw request body to
sdk.trigger()."🤖 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 1094 - 1096, The `sdk.trigger` call in `api::consolidate` is forwarding `req.body` directly, which bypasses the required REST field whitelist. Update this handler to explicitly pick only the known payload fields for `mem::consolidate` (following the same whitelisting pattern used in `api::remember` and `api::memory-update`) before passing the payload into `sdk.trigger`, so no raw request body is sent through.Source: Path instructions
🤖 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 the downstream user-facing references to match the
rebrand in the affected consumer files: in README.md and
website/components/Install.tsx, replace all hardcoded old package/repo/CLI
identifiers with the new package name, repository identity, and executable used
by package.json. Check the badge links and install snippets so they no longer
reference `@agentmemory/agentmemory`, rohitg00/agentmemory, or the old agentmemory
binary, and keep the changes consistent with the metadata updated in
package.json.
In `@src/functions/remember.ts`:
- Around line 186-195: The mem::update validation in the update handler
currently checks memoryId and content but omits array type checks for concepts
and files, unlike mem::remember. Update the same validation block in the
mem::update flow to verify concepts and files are arrays before storing them,
and reject non-array values with a clear validation error. Use the existing
update/validation logic around the mem::update handler and the Memory shape as
the reference points so downstream consumers only receive correctly typed data.
- Around line 171-257: `mem::update` is mixing in-place overwrite with
versioning, which breaks memory history and latest-state integrity. Update the
`sdk.registerFunction("mem::update")` flow to create a new memory record with a
fresh ID like `mem::remember` does, set `parentId` to the current `existing.id`,
and append the previous ID to `supersedes` without self-referencing. Also add a
guard so already superseded records (`isLatest: false`) cannot be promoted back
to latest, and preserve the old record instead of overwriting it.
In `@src/state/memory-dedup.ts`:
- Line 16: The negation pattern in CJK_NEGATION is too broad because it matches
bare “不”, which causes false positives in common Chinese compound words and
blocks valid superseding in memory deduplication. Update the CJK_NEGATION regex
in memory-dedup.ts to remove the standalone “不” while keeping the explicit
negation markers already listed, and ensure the negation guard in the dedup
logic continues to rely on the refined pattern.
In `@src/triggers/api.ts`:
- Around line 1013-1054: The `api::memory-update` handler needs stricter input
validation and better failure mapping. In the
`sdk.registerFunction("api::memory-update", ...)` flow, validate
`req.body.memoryId` the same way `content` is validated by checking it is a
non-empty string before calling `sdk.trigger`, and return a 400 if it is
invalid. Also update the post-`sdk.trigger` result handling so any
`success:false` from `mem::update` is returned as a 4xx response: keep the 404
for `"memory not found"`, but map all other non-success outcomes to an
appropriate client-error status instead of falling through to 200.
In `@test/snapshot-interval-timer.test.ts`:
- Around line 55-82: The snapshot timer test is only validating mocked config
math and never exercises the real scheduling path. Update the test to import and
invoke the actual scheduling logic from src/index.ts (or a helper extracted from
it) so the code that calls setInterval and sdk.trigger runs under test, and
assert against the captured setIntervalCalls instead of using
originalSetInterval directly. Make sure iii-sdk is mocked with
vi.mock('iii-sdk') and the mock sdk.trigger/kv methods are used by the code
under test, then verify the interval is 3600000 and the callback targets
mem::snapshot-create.
---
Nitpick comments:
In `@src/triggers/api.ts`:
- Around line 1094-1096: The `sdk.trigger` call in `api::consolidate` is
forwarding `req.body` directly, which bypasses the required REST field
whitelist. Update this handler to explicitly pick only the known payload fields
for `mem::consolidate` (following the same whitelisting pattern used in
`api::remember` and `api::memory-update`) before passing the payload into
`sdk.trigger`, so no raw request body is sent through.
🪄 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: 7de84ad3-c363-45e3-8c7c-2c1ede9b3aa3
📒 Files selected for processing (16)
package.jsonsrc/functions/remember.tssrc/functions/summarize.tssrc/index.tssrc/mcp/standalone.tssrc/mcp/tools-registry.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
Comment on lines
+2
to
+4
| "name": "ziiagentmemory", | ||
| "version": "0.2.0", | ||
| "description": "Persistent memory for AI coding agents — CJK-aware dedup, memory_update versioning, standalone MCP server", |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Update downstream consumer files to match the rebrand.
The package name, bin executable, and repository metadata have been updated in package.json, but user-facing surfaces still reference the old identity:
README.md:49-54— npm badge, CI badge, license badge, and stars badge all hardcode@agentmemory/agentmemoryandrohitg00/agentmemory.website/components/Install.tsx:13-35— install commands usenpm install -g@agentmemory/agentmemory`` and the CLI executable is invoked asagentmemoryinstead of `ziiagentmemory`.
These will direct users to the old package and repo after the rebrand.
Also 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 the downstream user-facing
references to match the rebrand in the affected consumer files: in README.md and
website/components/Install.tsx, replace all hardcoded old package/repo/CLI
identifiers with the new package name, repository identity, and executable used
by package.json. Check the badge links and install snippets so they no longer
reference `@agentmemory/agentmemory`, rohitg00/agentmemory, or the old agentmemory
binary, and keep the changes consistent with the metadata updated in
package.json.
Comment on lines
+171
to
+257
| // ── mem::update ───────────────────────────────────────────────────── | ||
| // Updates an existing memory by ID: sets new content, increments version, | ||
| // and marks the old memory as superseded. This fills the gap identified | ||
| // in #1018 where calling memory_save twice with the same concepts created | ||
| // two separate v1 memories instead of versioning. | ||
| sdk.registerFunction("mem::update", | ||
| async (data: { | ||
| memoryId: string; | ||
| content: string; | ||
| type?: string; | ||
| concepts?: string[]; | ||
| files?: string[]; | ||
| ttlDays?: number; | ||
| agentId?: string; | ||
| }) => { | ||
| if (!data.memoryId || typeof data.memoryId !== "string") { | ||
| return { success: false, error: "memoryId is required" }; | ||
| } | ||
| if ( | ||
| !data.content || | ||
| typeof data.content !== "string" || | ||
| !data.content.trim() | ||
| ) { | ||
| return { success: false, error: "content is required" }; | ||
| } | ||
|
|
||
| return withKeyedLock("mem:update", async () => { | ||
| const existing = await kv.get<Memory>(KV.memories, data.memoryId); | ||
| if (!existing) { | ||
| return { success: false, error: "memory not found" }; | ||
| } | ||
|
|
||
| const now = new Date().toISOString(); | ||
| const newVersion = (existing.version ?? 1) + 1; | ||
| const validTypes = new Set([ | ||
| "pattern", | ||
| "preference", | ||
| "architecture", | ||
| "bug", | ||
| "workflow", | ||
| "fact", | ||
| ]); | ||
| const memType = validTypes.has(data.type || "") | ||
| ? (data.type as Memory["type"]) | ||
| : existing.type; | ||
|
|
||
| const callAgentId = | ||
| typeof data.agentId === "string" && data.agentId.trim().length > 0 | ||
| ? data.agentId.trim().slice(0, 128) | ||
| : getAgentId(); | ||
|
|
||
| const updated: Memory = { | ||
| id: existing.id, | ||
| createdAt: existing.createdAt, | ||
| updatedAt: now, | ||
| type: memType, | ||
| title: data.content.slice(0, 80), | ||
| content: data.content, | ||
| concepts: data.concepts ?? existing.concepts, | ||
| files: data.files ?? existing.files, | ||
| sessionIds: existing.sessionIds, | ||
| strength: existing.strength, | ||
| version: newVersion, | ||
| parentId: existing.parentId, | ||
| supersedes: [...(existing.supersedes ?? []), existing.id], | ||
| sourceObservationIds: existing.sourceObservationIds, | ||
| isLatest: true, | ||
| forgetAfter: existing.forgetAfter, | ||
| ...(existing.imageRef ? { imageRef: existing.imageRef } : {}), | ||
| ...(existing.imageData ? { imageData: existing.imageData } : {}), | ||
| ...(callAgentId ? { agentId: callAgentId } : {}), | ||
| ...(existing.project !== undefined && { project: existing.project }), | ||
| }; | ||
|
|
||
| if (data.ttlDays && typeof data.ttlDays === "number" && data.ttlDays > 0) { | ||
| updated.forgetAfter = new Date( | ||
| Date.now() + data.ttlDays * 86400000, | ||
| ).toISOString(); | ||
| } | ||
|
|
||
| // 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); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
mem::update versioning logic has data integrity issues.
-
Lines 251-257 — Same-ID overwrite loses old version:
existingis saved withisLatest: falsethen immediately overwritten byupdated(sameexisting.id). The firstkv.setis a wasted write and the old version is permanently lost.mem::rememberavoids this by usinggenerateId("mem")for the new version, preserving both records. -
Line 235 — Self-referencing
supersedes:supersedes: [...(existing.supersedes ?? []), existing.id]adds the memory's own ID sinceupdated.id === existing.id. Inmem::remember,supersedesreferences a different memory's ID. -
Lines 197-201 — No
isLatestguard: Updating a memory withisLatest: false(already superseded) would mark it as latest again, creating two "latest" entries for the same logical entity. -
Line 234 —
parentIdpoints to grandparent:parentId: existing.parentIdpreserves the original parent, not the immediate predecessor. For a new version,parentIdshould beexisting.id.
If the intent is true versioning (preserving old versions), use generateId("mem") for the new version. If the intent is in-place update, remove the first kv.set and the isLatest/supersedes logic entirely.
🔧 Proposed fix: use new ID for versioned update (consistent with mem::remember)
const updated: Memory = {
- id: existing.id,
+ id: generateId("mem"),
createdAt: existing.createdAt,
updatedAt: now,
type: memType,
title: data.content.slice(0, 80),
content: data.content,
concepts: data.concepts ?? existing.concepts,
files: data.files ?? existing.files,
sessionIds: existing.sessionIds,
strength: existing.strength,
version: newVersion,
- parentId: existing.parentId,
+ parentId: existing.id,
supersedes: [...(existing.supersedes ?? []), existing.id],
sourceObservationIds: existing.sourceObservationIds,
isLatest: true,
forgetAfter: existing.forgetAfter,
...(existing.imageRef ? { imageRef: existing.imageRef } : {}),
...(existing.imageData ? { imageData: existing.imageData } : {}),
...(callAgentId ? { agentId: callAgentId } : {}),
...(existing.project !== undefined && { project: existing.project }),
};And add an isLatest guard:
const existing = await kv.get<Memory>(KV.memories, data.memoryId);
if (!existing) {
return { success: false, error: "memory not found" };
}
+if (existing.isLatest === false) {
+ return { success: false, error: "memory is not the latest version" };
+}📝 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.
Suggested change
| // ── mem::update ───────────────────────────────────────────────────── | |
| // Updates an existing memory by ID: sets new content, increments version, | |
| // and marks the old memory as superseded. This fills the gap identified | |
| // in #1018 where calling memory_save twice with the same concepts created | |
| // two separate v1 memories instead of versioning. | |
| sdk.registerFunction("mem::update", | |
| async (data: { | |
| memoryId: string; | |
| content: string; | |
| type?: string; | |
| concepts?: string[]; | |
| files?: string[]; | |
| ttlDays?: number; | |
| agentId?: string; | |
| }) => { | |
| if (!data.memoryId || typeof data.memoryId !== "string") { | |
| return { success: false, error: "memoryId is required" }; | |
| } | |
| if ( | |
| !data.content || | |
| typeof data.content !== "string" || | |
| !data.content.trim() | |
| ) { | |
| return { success: false, error: "content is required" }; | |
| } | |
| return withKeyedLock("mem:update", async () => { | |
| const existing = await kv.get<Memory>(KV.memories, data.memoryId); | |
| if (!existing) { | |
| return { success: false, error: "memory not found" }; | |
| } | |
| const now = new Date().toISOString(); | |
| const newVersion = (existing.version ?? 1) + 1; | |
| const validTypes = new Set([ | |
| "pattern", | |
| "preference", | |
| "architecture", | |
| "bug", | |
| "workflow", | |
| "fact", | |
| ]); | |
| const memType = validTypes.has(data.type || "") | |
| ? (data.type as Memory["type"]) | |
| : existing.type; | |
| const callAgentId = | |
| typeof data.agentId === "string" && data.agentId.trim().length > 0 | |
| ? data.agentId.trim().slice(0, 128) | |
| : getAgentId(); | |
| const updated: Memory = { | |
| id: existing.id, | |
| createdAt: existing.createdAt, | |
| updatedAt: now, | |
| type: memType, | |
| title: data.content.slice(0, 80), | |
| content: data.content, | |
| concepts: data.concepts ?? existing.concepts, | |
| files: data.files ?? existing.files, | |
| sessionIds: existing.sessionIds, | |
| strength: existing.strength, | |
| version: newVersion, | |
| parentId: existing.parentId, | |
| supersedes: [...(existing.supersedes ?? []), existing.id], | |
| sourceObservationIds: existing.sourceObservationIds, | |
| isLatest: true, | |
| forgetAfter: existing.forgetAfter, | |
| ...(existing.imageRef ? { imageRef: existing.imageRef } : {}), | |
| ...(existing.imageData ? { imageData: existing.imageData } : {}), | |
| ...(callAgentId ? { agentId: callAgentId } : {}), | |
| ...(existing.project !== undefined && { project: existing.project }), | |
| }; | |
| if (data.ttlDays && typeof data.ttlDays === "number" && data.ttlDays > 0) { | |
| updated.forgetAfter = new Date( | |
| Date.now() + data.ttlDays * 86400000, | |
| ).toISOString(); | |
| } | |
| // 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); | |
| // ── mem::update ───────────────────────────────────────────────────── | |
| // Updates an existing memory by ID: sets new content, increments version, | |
| // and marks the old memory as superseded. This fills the gap identified | |
| // in `#1018` where calling memory_save twice with the same concepts created | |
| // two separate v1 memories instead of versioning. | |
| sdk.registerFunction("mem::update", | |
| async (data: { | |
| memoryId: string; | |
| content: string; | |
| type?: string; | |
| concepts?: string[]; | |
| files?: string[]; | |
| ttlDays?: number; | |
| agentId?: string; | |
| }) => { | |
| if (!data.memoryId || typeof data.memoryId !== "string") { | |
| return { success: false, error: "memoryId is required" }; | |
| } | |
| if ( | |
| !data.content || | |
| typeof data.content !== "string" || | |
| !data.content.trim() | |
| ) { | |
| return { success: false, error: "content is required" }; | |
| } | |
| return withKeyedLock("mem:update", async () => { | |
| const existing = await kv.get<Memory>(KV.memories, data.memoryId); | |
| if (!existing) { | |
| return { success: false, error: "memory not found" }; | |
| } | |
| if (existing.isLatest === false) { | |
| return { success: false, error: "memory is not the latest version" }; | |
| } | |
| const now = new Date().toISOString(); | |
| const newVersion = (existing.version ?? 1) + 1; | |
| const validTypes = new Set([ | |
| "pattern", | |
| "preference", | |
| "architecture", | |
| "bug", | |
| "workflow", | |
| "fact", | |
| ]); | |
| const memType = validTypes.has(data.type || "") | |
| ? (data.type as Memory["type"]) | |
| : existing.type; | |
| const callAgentId = | |
| typeof data.agentId === "string" && data.agentId.trim().length > 0 | |
| ? data.agentId.trim().slice(0, 128) | |
| : getAgentId(); | |
| const updated: Memory = { | |
| id: generateId("mem"), | |
| createdAt: existing.createdAt, | |
| updatedAt: now, | |
| type: memType, | |
| title: data.content.slice(0, 80), | |
| content: data.content, | |
| concepts: data.concepts ?? existing.concepts, | |
| files: data.files ?? existing.files, | |
| sessionIds: existing.sessionIds, | |
| strength: existing.strength, | |
| version: newVersion, | |
| parentId: existing.id, | |
| supersedes: [...(existing.supersedes ?? []), existing.id], | |
| sourceObservationIds: existing.sourceObservationIds, | |
| isLatest: true, | |
| forgetAfter: existing.forgetAfter, | |
| ...(existing.imageRef ? { imageRef: existing.imageRef } : {}), | |
| ...(existing.imageData ? { imageData: existing.imageData } : {}), | |
| ...(callAgentId ? { agentId: callAgentId } : {}), | |
| ...(existing.project !== undefined && { project: existing.project }), | |
| }; | |
| if (data.ttlDays && typeof data.ttlDays === "number" && data.ttlDays > 0) { | |
| updated.forgetAfter = new Date( | |
| Date.now() + data.ttlDays * 86400000, | |
| ).toISOString(); | |
| } | |
| // 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); |
🤖 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 171 - 257, `mem::update` is mixing
in-place overwrite with versioning, which breaks memory history and latest-state
integrity. Update the `sdk.registerFunction("mem::update")` flow to create a new
memory record with a fresh ID like `mem::remember` does, set `parentId` to the
current `existing.id`, and append the previous ID to `supersedes` without
self-referencing. Also add a guard so already superseded records (`isLatest:
false`) cannot be promoted back to latest, and preserve the old record instead
of overwriting it.
Comment on lines
+186
to
+195
| if (!data.memoryId || typeof data.memoryId !== "string") { | ||
| return { success: false, error: "memoryId is required" }; | ||
| } | ||
| if ( | ||
| !data.content || | ||
| typeof data.content !== "string" || | ||
| !data.content.trim() | ||
| ) { | ||
| return { success: false, error: "content is required" }; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
mem::update missing array validation for concepts and files.
Unlike mem::remember (Lines 33-38), mem::update doesn't validate that concepts and files are arrays. A non-array value would be stored directly, violating the Memory interface and potentially causing runtime errors in downstream consumers. As per coding guidelines, inputs must be validated.
🛡️ Proposed validation additions
if (!data.memoryId || typeof data.memoryId !== "string") {
return { success: false, error: "memoryId is required" };
}
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" };
+ }📝 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.
Suggested change
| if (!data.memoryId || typeof data.memoryId !== "string") { | |
| return { success: false, error: "memoryId is required" }; | |
| } | |
| if ( | |
| !data.content || | |
| typeof data.content !== "string" || | |
| !data.content.trim() | |
| ) { | |
| return { success: false, error: "content is required" }; | |
| } | |
| if (!data.memoryId || typeof data.memoryId !== "string") { | |
| return { success: false, error: "memoryId is required" }; | |
| } | |
| 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" }; | |
| } |
🤖 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 186 - 195, The mem::update validation
in the update handler currently checks memoryId and content but omits array type
checks for concepts and files, unlike mem::remember. Update the same validation
block in the mem::update flow to verify concepts and files are arrays before
storing them, and reject non-array values with a clear validation error. Use the
existing update/validation logic around the mem::update handler and the Memory
shape as the reference points so downstream consumers only receive correctly
typed data.
Source: Coding guidelines
| */ | ||
|
|
||
| const EN_NEGATION = /\b(not|never|don't|do not|cannot|should not|must not|no)\b/i; | ||
| const CJK_NEGATION = /不要|别|勿|無|无|不|なし|않|안|못/i; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Bare 不 in CJK_NEGATION causes false positives in Chinese text.
不 appears in many common non-negation compound words (不同 = "different", 不断 = "continuous", 不久 = "soon", 不过 = "but"). This will incorrectly trigger the negation guard, blocking legitimate superseding and creating duplicate memories. For example, "不断改进代码质量" vs "持续改进代码质量" (same meaning) would be prevented from superseding due to 不 in "不断".
🔧 Proposed fix: remove bare `不`, keep specific negation markers
-const CJK_NEGATION = /不要|别|勿|無|无|不|なし|않|안|못/i;
+const CJK_NEGATION = /不要|别|勿|無|无|なし|않|안|못/i;不要 (already in the regex) covers the explicit "do not" imperative. The bare 不 is too broad for compound-word false positives.
📝 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.
Suggested change
| 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 negation pattern in CJK_NEGATION
is too broad because it matches bare “不”, which causes false positives in common
Chinese compound words and blocks valid superseding in memory deduplication.
Update the CJK_NEGATION regex in memory-dedup.ts to remove the standalone “不”
while keeping the explicit negation markers already listed, and ensure the
negation guard in the dedup logic continues to rely on the refined pattern.
Comment on lines
+1013
to
+1054
| 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 }; | ||
| }, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate memoryId type and map non-"not found" failures to a 4xx status.
Two gaps in the new handler:
- Line 1027 only checks truthiness of
memoryId, not its type (unlikecontentat Line 1032, which is type-checked). A non-string truthy value passes here and reachesmem::update. - Only
error === "memory not found"maps to 404 (Line 1050); any othersuccess:falseresult frommem::update(e.g.,"memoryId is required","content is required", or an unexpected failure) falls through to the default200response at Line 1053 — a failed operation is reported as success.
🐛 Proposed fix
- if (!req.body?.memoryId) {
+ if (!req.body?.memoryId || typeof req.body.memoryId !== "string") {
return { status_code: 400, body: { error: "memoryId is required" } };
}
@@
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 };
+ if (!r.success) {
+ return {
+ status_code: r.error === "memory not found" ? 404 : 400,
+ body: result,
+ };
+ }
+ return { status_code: 200, body: result };📝 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.
Suggested change
| 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 || typeof req.body.memoryId !== "string") { | |
| 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) { | |
| return { | |
| status_code: r.error === "memory not found" ? 404 : 400, | |
| body: result, | |
| }; | |
| } | |
| return { status_code: 200, body: result }; | |
| }, |
🤖 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 needs stricter input validation and better failure mapping. In the
`sdk.registerFunction("api::memory-update", ...)` flow, validate
`req.body.memoryId` the same way `content` is validated by checking it is a
non-empty string before calling `sdk.trigger`, and return a 400 if it is
invalid. Also update the post-`sdk.trigger` result handling so any
`success:false` from `mem::update` is returned as a 4xx response: keep the 404
for `"memory not found"`, but map all other non-success outcomes to an
appropriate client-error status instead of falling through to 200.
Comment on lines
+55
to
+82
| 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); | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Test does not exercise the actual code under test.
The test verifies that loadSnapshotConfig() returns the mocked values and that 3600 * 1000 === 3600000 — both trivially true. It never imports src/index.ts or invokes main(), so the actual snapshot scheduling logic (lines 353-362 of src/index.ts) is never executed. The setIntervalCalls capture array is populated in beforeEach but never asserted against, and the test body at line 76 uses originalSetInterval instead of the wrapped global.setInterval, bypassing the capture entirely.
This gives false confidence that #1006 is covered. The test should import and invoke the scheduling code (or an extracted helper), mock iii-sdk per the test guidelines, and assert that setInterval is called with the correct interval (3600000) and a callback that triggers mem::snapshot-create.
As per coding guidelines, test files should use vi.mock('iii-sdk') with mock implementations of sdk.trigger, kv.get, kv.set, kv.list and follow patterns in test/crystallize.test.ts. Based on learnings, confirm that mocked modules are actually used at runtime by the code under test — here the code under test is never invoked.
♻️ Proposed refactor: test the actual scheduling logic
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
-// Mock setInterval to capture calls
-let setIntervalCalls: Array<{ cb: Function; ms: number }> = [];
+let setIntervalCalls: Array<{ cb: Function; ms: number }> = [];
const originalSetInterval = global.setInterval;
vi.mock("../src/logger.js", () => ({
logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() },
}));
+vi.mock("iii-sdk", () => ({
+ registerWorker: () => mockSdk,
+ // ... other exports as needed
+}));
+
+const mockSdk = {
+ registerFunction: vi.fn(),
+ trigger: vi.fn().mockResolvedValue(undefined),
+ getMeter: vi.fn(),
+ shutdown: vi.fn(),
+};
+
+const mockKV = {
+ get: vi.fn().mockResolvedValue(null),
+ set: vi.fn().mockResolvedValue(undefined),
+ list: vi.fn().mockResolvedValue([]),
+};
+
vi.mock("../src/config.js", () => ({
loadConfig: () => ({ restPort: 3111, enginePort: 3112, viewerPort: 3115 }),
getEnvVar: (key: string) => process.env[key] || undefined,
loadEmbeddingConfig: () => ({
bm25Weight: 0.7,
vectorWeight: 0.3,
provider: "none",
model: "",
dimensions: 0,
apiKey: "",
baseUrl: "",
}),
loadFallbackConfig: () => ({ enabled: false }),
loadClaudeBridgeConfig: () => ({ enabled: false }),
loadTeamConfig: () => ({ enabled: false }),
loadSnapshotConfig: () => ({
enabled: true,
interval: 3600,
dir: "/tmp/test-snapshots",
}),
isGraphExtractionEnabled: () => false,
isAutoCompressEnabled: () => false,
isConsolidationEnabled: () => false,
isContextInjectionEnabled: () => false,
isDropStaleIndexEnabled: () => false,
}));
-import { loadSnapshotConfig } from "../src/config.js";
+// Import main after mocks are set up
+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 for snapshot-create with correct interval", async () => {
+ await main();
+
+ const snapshotCall = setIntervalCalls.find(
+ (c) => c.ms === 3600000,
+ );
+ expect(snapshotCall).toBeDefined();
+
+ // Verify the callback triggers mem::snapshot-create
+ await snapshotCall!.cb();
+ expect(mockSdk.trigger).toHaveBeenCalledWith({
+ function_id: "mem::snapshot-create",
+ payload: {},
+ });
});
});🤖 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 55 - 82, The snapshot
timer test is only validating mocked config math and never exercises the real
scheduling path. Update the test to import and invoke the actual scheduling
logic from src/index.ts (or a helper extracted from it) so the code that calls
setInterval and sdk.trigger runs under test, and assert against the captured
setIntervalCalls instead of using originalSetInterval directly. Make sure
iii-sdk is mocked with vi.mock('iii-sdk') and the mock sdk.trigger/kv methods
are used by the code under test, then verify the interval is 3600000 and the
callback targets mem::snapshot-create.
Sources: Coding guidelines, Learnings
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.
Problem
POST /agentmemory/consolidateconsistently returns HTTP 500 with{"error":"[object Object]"}instead of the actual error message.The
api::consolidatehandler callssdk.trigger({ function_id: "mem::consolidate" })without a try/catch. When the function throws, the engine's default error handler stringifies the Error object as[object Object]because it doesn't extract.message.Fix
Wrap the
sdk.triggercall in a try/catch that extracts the error message properly:Closes #1008
Summary by CodeRabbit
New Features
Bug Fixes