Repository navigation
Kody memory subsystem - #133
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
|
Cursor Agent can help with this pull request. Just |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughImplements a long-term memory subsystem: DB schema, types, repo and service layers (upsert/get/delete/search/verify/surface), vector embedding/indexing and reindex endpoint, five MCP memory capabilities, tool integrations to surface memories, tests, and docs updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent/Caller
participant Verify as meta_memory_verify
participant Service as Memory Service
participant DB as Database
participant Vector as Vector Index
Agent->>Verify: meta_memory_verify(candidate)
Verify->>Service: verifyMemoryCandidate(...)
Service->>DB: list/search memories
DB-->>Service: memory rows
Service->>Vector: vector lookup / ranking
Vector-->>Service: ranked ids/scores
Service-->>Verify: candidate + related_memories + recommended_actions
Verify-->>Agent: response
sequenceDiagram
participant Agent as Agent/Caller
participant Execute as execute tool
participant ToolMem as Memory Tool Context
participant Service as Memory Service
participant DB as Database
participant Vector as Vector Index
Agent->>Execute: execute(task, memoryContext, conversationId)
Execute->>ToolMem: loadRelevantMemoriesForTool(...)
ToolMem->>Service: surfaceRelevantMemories(retrievalQuery, conversationId)
Service->>DB: listMemoriesByUserId(...)
Service->>Vector: search vectors / embed query
Vector-->>Service: ranked ids/scores
Service->>DB: getConversationSuppressions(...)
Service->>DB: upsertConversationSuppressions(...)
Service-->>ToolMem: memories + suppressedCount
ToolMem->>Execute: formatted ContentBlock(s), structuredContent
Execute-->>Agent: response with surfaced memories
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
🔎 Preview deployed: https://kody-pr-133.kentcdodds.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
packages/worker/migrations/0016-mcp-memories.sql (1)
26-38: Consider adding an index for suppression lookups by user and conversation.
getConversationSuppressionsqueries by(user_id, conversation_id, expires_at > ?). The composite primary key covers equality lookups, but an additional index on(user_id, conversation_id, expires_at)would optimize range filtering onexpires_at. If suppression tables are expected to grow significantly, this could improve query performance.📊 Suggested index
CREATE INDEX IF NOT EXISTS idx_mcp_memory_suppressions_user_conv_expires ON mcp_memory_conversation_suppressions(user_id, conversation_id, expires_at);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/migrations/0016-mcp-memories.sql` around lines 26 - 38, Add a composite index to speed up getConversationSuppressions queries that filter by user_id, conversation_id and a range on expires_at: create an index on mcp_memory_conversation_suppressions(user_id, conversation_id, expires_at) (e.g., name it idx_mcp_memory_suppressions_user_conv_expires) so range/filtering on expires_at after equality on user_id and conversation_id uses the index; update the migration file that defines mcp_memory_conversation_suppressions to include this CREATE INDEX IF NOT EXISTS statement alongside the existing idx_mcp_memory_suppressions_expires_at.packages/worker/src/mcp/memory/service.node.test.ts (1)
182-207: Test double doesn't replicate ON CONFLICT behavior for suppression upserts.The real
upsertConversationSuppressions(repo.ts:232-260) usesON CONFLICT(user_id, conversation_id, memory_id) DO UPDATE SET last_seen_at = excluded.last_seen_at, expires_at = excluded.expires_at, preservingcreated_at. This mock simply overwrites all fields includingcreated_at, which could mask bugs where the original creation timestamp should be preserved on re-suppression.🔧 Suggested fix
if ( normalizedQuery.startsWith( 'insert into mcp_memory_conversation_suppressions', ) ) { const [ userId, conversationId, memoryId, createdAt, lastSeenAt, expiresAt, ] = params as Array<string> + const key = suppressionKey(userId, conversationId, memoryId) + const existing = suppressions.get(key) - suppressions.set( - suppressionKey(userId, conversationId, memoryId), - { + suppressions.set(key, { + user_id: userId, + conversation_id: conversationId, + memory_id: memoryId, + created_at: existing?.created_at ?? createdAt, + last_seen_at: lastSeenAt, + expires_at: expiresAt, + }) - user_id: userId, - conversation_id: conversationId, - memory_id: memoryId, - created_at: createdAt, - last_seen_at: lastSeenAt, - expires_at: expiresAt, - }, - ) return { meta: { changes: 1 } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/memory/service.node.test.ts` around lines 182 - 207, The test double currently overwrites created_at for suppression upserts; update the insert branch that handles normalizedQuery starting with 'insert into mcp_memory_conversation_suppressions' so it mirrors upsertConversationSuppressions (repo.ts) by using suppressionKey(userId, conversationId, memoryId) to check if an entry already exists in suppressions and, on conflict, only update last_seen_at and expires_at while preserving the existing created_at; if no entry exists, create a new record using createdAt from params. Ensure the params destructuring (userId, conversationId, memoryId, createdAt, lastSeenAt, expiresAt) is still used and return the same meta response as before.packages/worker/src/mcp/memory/memory-reindex.ts (1)
23-28: UnboundedlistAllMemoriesmay cause memory pressure at scale.
listAllMemories(repo.ts:181-193) loads all rows into memory before iteration. For large datasets (tens of thousands of memories), this could exhaust worker memory. Consider adding pagination with cursor-based iteration if this reindex operation is expected to run on production data at scale.For a rarely-run maintenance operation on modest data volumes, this is acceptable. Flag for revisit if memory counts grow significantly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/memory/memory-reindex.ts` around lines 23 - 28, The current memory-reindex uses listAllMemories which loads all rows into RAM (rows variable) and can cause OOM at scale; change the reindex routine in memory-reindex.ts to iterate pages/cursor instead of calling listAllMemories: replace the single call to listAllMemories with a paginated loop that requests batches (e.g., listMemoriesPage or add startCursor/limit params to repo list function) and processes each batch before fetching the next, ensuring you only keep one page in memory at a time and return the aggregated upserted count; update or add a cursor/limit-aware helper in the repo if needed to support this iteration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/mcp/capabilities/meta/meta-memory-delete.ts`:
- Around line 70-80: The response currently returns ok: true even when
deleteMemory(...) returns null; change the logic in meta-memory-delete handler
to check the returned memory value from deleteMemory and treat null as a
failure: if memory is null, return ok: false (include force: args.force ??
false) and a guidance message asking the caller to re-verify/retry (use/extend
verifyFirstGuidance), otherwise proceed to return ok: true with memory:
formatMemoryRecord(memory). Ensure you reference deleteMemory,
formatMemoryRecord, memory, args.force and verifyFirstGuidance when making the
check and constructing the two different responses.
In `@packages/worker/src/mcp/capabilities/meta/meta-memory-shared.ts`:
- Around line 5-20: The schema definitions (e.g., memoryCategoryField,
memoryTagInputSchema, memoryTagsField and other input schemas in this file)
currently accept whitespace-only values and allow lengths larger than the
service normalization in packages/worker/src/mcp/memory/service.ts, causing
silent truncation/drop; update each string schema to first trim input and then
validate trimmed length and max length to match the service normalization rules
(use .transform(s => s.trim()) or .trim() equivalent, then enforce .min(1) on
the trimmed value and set .max(...) to the service's kept limits), ensure tag
strings are trimmed and validated and the tags array still enforces max items,
and apply the same trimmed+bounded validation to
subject/summary/details/category/dedupe_key schemas mentioned (lines 22-48) so
validation matches service behavior rather than allowing whitespace-only or
overlong inputs.
In `@packages/worker/src/mcp/capabilities/meta/meta-memory-upsert.ts`:
- Around line 97-100: The current guard in meta_memory_upsert that checks only
args.verified_by_agent is insufficient because it trusts caller-supplied
booleans; replace it with a server-side verification check tied to the
user/conversation created by meta_memory_verify—e.g., when meta_memory_verify
runs it should create a short-lived verification token or set a verification
state in a server store (cache/db) mapped to conversationId/agentId; then in
meta_memory_upsert look up that server-side proof (not args.verified_by_agent)
via a helper like isConversationVerified(conversationId, agentId) or
validateVerificationToken(token) and reject the upsert unless the proof exists
and is still valid. Ensure to invalidate/expire the proof after use or time
window to prevent replay.
- Around line 102-113: The current upsert call always coerces omitted optional
fields to defaults (category, details, tags, dedupe_key) and forces status to
'active', which causes updates to wipe existing metadata; change the update path
to preserve existing values: when args.memory_id is present load the existing
memory (e.g., via getMemory or readMemory) and pass merged values into
upsertMemory (use existingMemory.category/details/tags/dedupeKey/status when the
corresponding args.* are null/undefined), and stop defaulting status to 'active'
on updates so an omitted status does not unarchive the record. Ensure you
reference upsertMemory, args.memory_id, args.category, args.details, args.tags,
args.dedupe_key, and args.status when implementing the merge.
In `@packages/worker/src/mcp/memory/memory-search.ts`:
- Line 24: The code currently calls getCapabilityVectorIndex(input.env)! which
may throw when the optional binding is missing and then sets offline via
isCapabilitySearchOffline(input.env); change behavior to treat a missing vector
index as offline by checking getCapabilityVectorIndex(input.env) for
null/undefined before using it (do not use the non-null assertion), and when
running vector queries ensure results are scoped to the current user by adding a
user-specific filter (e.g., include the user id in the query or filter by
memoryId namespace) so that other users' memory_* vectors cannot occupy topK
before you prune with rowsById.has(memoryId); update the logic around offline,
getCapabilityVectorIndex, and the result filtering (references:
isCapabilitySearchOffline, getCapabilityVectorIndex, offline, rowsById.has,
memoryId, topK, kind) accordingly for both the initial block and the similar
code at lines 48-57.
In `@packages/worker/src/mcp/memory/service.node.test.ts`:
- Around line 236-239: The env() test helper returns only APP_DB but is cast to
Pick<Env, 'APP_DB' | 'AI'> which is unsafe; either add a stub AI binding to the
mock (e.g., provide env().AI with the minimal methods used by functions like
surfaceRelevantMemories / any embedding callers) or change the cast to Pick<Env,
'APP_DB'> so the type accurately reflects the provided bindings; update the
env() helper accordingly and ensure any tests that might exercise embedding use
the stubbed env.AI implementation.
In `@packages/worker/src/mcp/memory/service.ts`:
- Around line 137-167: The code currently commits the SQL mutation via
insertMemory/updateMemory and only afterwards calls
upsertMemoryVector/deleteMemoryVector, so if the vector/embedding step fails
callers see an error even though the DB change is durable; to fix, make the
vector I/O best-effort and non-fatal by wrapping calls to upsertMemoryVector and
deleteMemoryVector in try/catch (log the error with context including memoryId
and userId) or alternatively reorder so the embedding/vector operation and
buildMemoryEmbedText are done before committing the DB change and only commit if
both succeed (choose one consistent approach), updating code around
insertMemory, updateMemory, upsertMemoryVector, deleteMemoryVector and
buildMemoryEmbedText accordingly.
- Around line 304-316: The code is over-constraining search by passing category:
candidate.category to searchMemoryRecords, which hides related or uncategorized
memories already intended to be matched by the verify query built by
buildVerifyQuery; change the call in the verification flow so
searchMemoryRecords is called without the category filter (remove the category:
candidate.category argument or pass undefined/null) while keeping
normalizeMemoryPayload and buildVerifyQuery unchanged, so verification can find
records across categories and null-category records.
- Around line 269-286: The current overfetch of normalizeLimit(input.limit) * 3
can still yield fewer than the requested visible memories if more than 3*limit
top-ranked matches are suppressed; update the logic around searchMemories +
filterSuppressedMatches to iteratively fetch additional pages until you have at
least normalizeLimit(input.limit) non-suppressed matches or there are no more
matches. Concretely, replace the single call to searchMemories(...) with a loop
that calls searchMemories using the existing rows/filteredRows pagination,
accumulates matches, calls filterSuppressedMatches(...) on the accumulated
result, and stops when filtered.matches.length >= normalizeLimit(input.limit) or
when searchMemories returns no new matches; then return
filtered.matches.slice(0, normalizeLimit(input.limit)) and
filtered.suppressedCount. Ensure you use the same symbols: searchMemories,
filterSuppressedMatches, normalizeLimit, matches, filteredRows, and input.limit.
In `@packages/worker/src/mcp/tools/memory-tool-context.ts`:
- Around line 110-117: The early return drops suppression-only notices: update
the logic in surfaceRelevantMemories (the block that currently returns content
when !memorySummary || memorySummary.memories.length === 0) to instead check
memorySummary.suppressedCount and, if suppressedCount > 0, append a ContentBlock
with formatRelevantMemoriesMarkdown(memorySummary) even when memories is empty;
make the analogous change in the other similar block (around the second
occurrence currently at lines 141-147) so suppression-only summaries are
rendered rather than skipped.
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 668-674: The code is concatenating serialized search text with
formatSurfacedMemoriesMarkdown(memoryToolContext) (which returns
ContentBlock[]), causing [object Object] to appear; in the truncateSearchText
call (around truncateSearchText(...) where serialized and
formatSurfacedMemoriesMarkdown are joined) remove the
formatSurfacedMemoriesMarkdown(...) from the joined array and pass only
serialized (i.e., return serialized instead of joining the ContentBlock[]),
keeping existing use of memories passed into formatSearchMarkdown unchanged.
---
Nitpick comments:
In `@packages/worker/migrations/0016-mcp-memories.sql`:
- Around line 26-38: Add a composite index to speed up
getConversationSuppressions queries that filter by user_id, conversation_id and
a range on expires_at: create an index on
mcp_memory_conversation_suppressions(user_id, conversation_id, expires_at)
(e.g., name it idx_mcp_memory_suppressions_user_conv_expires) so range/filtering
on expires_at after equality on user_id and conversation_id uses the index;
update the migration file that defines mcp_memory_conversation_suppressions to
include this CREATE INDEX IF NOT EXISTS statement alongside the existing
idx_mcp_memory_suppressions_expires_at.
In `@packages/worker/src/mcp/memory/memory-reindex.ts`:
- Around line 23-28: The current memory-reindex uses listAllMemories which loads
all rows into RAM (rows variable) and can cause OOM at scale; change the reindex
routine in memory-reindex.ts to iterate pages/cursor instead of calling
listAllMemories: replace the single call to listAllMemories with a paginated
loop that requests batches (e.g., listMemoriesPage or add startCursor/limit
params to repo list function) and processes each batch before fetching the next,
ensuring you only keep one page in memory at a time and return the aggregated
upserted count; update or add a cursor/limit-aware helper in the repo if needed
to support this iteration.
In `@packages/worker/src/mcp/memory/service.node.test.ts`:
- Around line 182-207: The test double currently overwrites created_at for
suppression upserts; update the insert branch that handles normalizedQuery
starting with 'insert into mcp_memory_conversation_suppressions' so it mirrors
upsertConversationSuppressions (repo.ts) by using suppressionKey(userId,
conversationId, memoryId) to check if an entry already exists in suppressions
and, on conflict, only update last_seen_at and expires_at while preserving the
existing created_at; if no entry exists, create a new record using createdAt
from params. Ensure the params destructuring (userId, conversationId, memoryId,
createdAt, lastSeenAt, expiresAt) is still used and return the same meta
response as before.
🪄 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: e597d9a1-c67d-43d0-bda8-b1ee18ba051a
📒 Files selected for processing (32)
docs/use/execute.mddocs/use/first-steps.mddocs/use/memory.mddocs/use/search.mdpackages/worker/migrations/0016-mcp-memories.sqlpackages/worker/src/index.tspackages/worker/src/mcp/capabilities/meta/domain.tspackages/worker/src/mcp/capabilities/meta/meta-memory-delete.tspackages/worker/src/mcp/capabilities/meta/meta-memory-get.tspackages/worker/src/mcp/capabilities/meta/meta-memory-search.tspackages/worker/src/mcp/capabilities/meta/meta-memory-shared.tspackages/worker/src/mcp/capabilities/meta/meta-memory-upsert.tspackages/worker/src/mcp/capabilities/meta/meta-memory-verify.tspackages/worker/src/mcp/mcp-server.mcp-e2e.test.tspackages/worker/src/mcp/memory/memory-embed.tspackages/worker/src/mcp/memory/memory-reindex.tspackages/worker/src/mcp/memory/memory-search.tspackages/worker/src/mcp/memory/memory-vectorize.tspackages/worker/src/mcp/memory/repo.tspackages/worker/src/mcp/memory/service.node.test.tspackages/worker/src/mcp/memory/service.tspackages/worker/src/mcp/memory/types.tspackages/worker/src/mcp/server-instructions.tspackages/worker/src/mcp/tools/execute.node.test.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/mcp/tools/memory-tool-context.tspackages/worker/src/mcp/tools/open-generated-ui.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/mcp/tools/tool-call-context.tspackages/worker/src/mcp/tools/tool-response-content.tspackages/worker/src/memory-maintenance.ts
Comment on lines
+70
to
+80
| const memory = await deleteMemory({ | ||
| env: ctx.env, | ||
| userId: user.userId, | ||
| memoryId: args.memory_id, | ||
| force: args.force ?? false, | ||
| }) | ||
| return { | ||
| ok: true, | ||
| force: args.force ?? false, | ||
| memory: formatMemoryRecord(memory), | ||
| guidance: `${verifyFirstGuidance} Use soft delete by default; reserve force=true for records that should be removed permanently.`, |
There was a problem hiding this comment.
Don't acknowledge a delete that didn't actually happen.
deleteMemory() is nullable, so a concurrent delete or failed hard-delete can currently return { ok: true, memory: null }. Treat a null result as a failure and ask the caller to re-verify/retry instead of reporting success.
Suggested fix
const memory = await deleteMemory({
env: ctx.env,
userId: user.userId,
memoryId: args.memory_id,
force: args.force ?? false,
})
+ if (!memory) {
+ throw new Error(
+ 'Memory changed or was already removed before deletion completed. Re-run meta_memory_verify and retry.',
+ )
+ }
return {
ok: true,
force: args.force ?? false,
memory: formatMemoryRecord(memory),📝 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 memory = await deleteMemory({ | |
| env: ctx.env, | |
| userId: user.userId, | |
| memoryId: args.memory_id, | |
| force: args.force ?? false, | |
| }) | |
| return { | |
| ok: true, | |
| force: args.force ?? false, | |
| memory: formatMemoryRecord(memory), | |
| guidance: `${verifyFirstGuidance} Use soft delete by default; reserve force=true for records that should be removed permanently.`, | |
| const memory = await deleteMemory({ | |
| env: ctx.env, | |
| userId: user.userId, | |
| memoryId: args.memory_id, | |
| force: args.force ?? false, | |
| }) | |
| if (!memory) { | |
| throw new Error( | |
| 'Memory changed or was already removed before deletion completed. Re-run meta_memory_verify and retry.', | |
| ) | |
| } | |
| return { | |
| ok: true, | |
| force: args.force ?? false, | |
| memory: formatMemoryRecord(memory), | |
| guidance: `${verifyFirstGuidance} Use soft delete by default; reserve force=true for records that should be removed permanently.`, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/meta/meta-memory-delete.ts` around lines
70 - 80, The response currently returns ok: true even when deleteMemory(...)
returns null; change the logic in meta-memory-delete handler to check the
returned memory value from deleteMemory and treat null as a failure: if memory
is null, return ok: false (include force: args.force ?? false) and a guidance
message asking the caller to re-verify/retry (use/extend verifyFirstGuidance),
otherwise proceed to return ok: true with memory: formatMemoryRecord(memory).
Ensure you reference deleteMemory, formatMemoryRecord, memory, args.force and
verifyFirstGuidance when making the check and constructing the two different
responses.
Comment on lines
+5
to
+20
| export const memoryCategoryField = z | ||
| .string() | ||
| .min(1) | ||
| .max(120) | ||
| .optional() | ||
| .describe( | ||
| 'Optional freeform category label for the memory. Suggested examples: preference, identifier, relationship, workflow, project, profile.', | ||
| ) | ||
|
|
||
| export const memoryTagInputSchema = z.string().min(1).max(80) | ||
|
|
||
| export const memoryTagsField = z | ||
| .array(memoryTagInputSchema) | ||
| .max(12) | ||
| .optional() | ||
| .describe('Optional short tags for retrieval and filtering.') |
There was a problem hiding this comment.
Make the capability input contract match the service normalization rules.
These schemas accept whitespace-only strings and longer subject/summary/details/category/dedupe_key values than packages/worker/src/mcp/memory/service.ts actually keeps. The current behavior becomes a mix of handler-thrown “required” errors and silent truncation/drop after validation, which is a poor contract for an MCP capability.
Also applies to: 22-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/meta/meta-memory-shared.ts` around lines
5 - 20, The schema definitions (e.g., memoryCategoryField, memoryTagInputSchema,
memoryTagsField and other input schemas in this file) currently accept
whitespace-only values and allow lengths larger than the service normalization
in packages/worker/src/mcp/memory/service.ts, causing silent truncation/drop;
update each string schema to first trim input and then validate trimmed length
and max length to match the service normalization rules (use .transform(s =>
s.trim()) or .trim() equivalent, then enforce .min(1) on the trimmed value and
set .max(...) to the service's kept limits), ensure tag strings are trimmed and
validated and the tags array still enforces max items, and apply the same
trimmed+bounded validation to subject/summary/details/category/dedupe_key
schemas mentioned (lines 22-48) so validation matches service behavior rather
than allowing whitespace-only or overlong inputs.
Comment on lines
+97
to
+100
| if (!args.verified_by_agent) { | ||
| throw new Error( | ||
| 'Agents must run meta_memory_verify before calling meta_memory_upsert. Set verified_by_agent=true only after review.', | ||
| ) |
There was a problem hiding this comment.
verified_by_agent doesn't enforce the verify-first contract.
Lines 97-100 only check a caller-supplied boolean, so any client can skip meta_memory_verify and still write durable memory by sending true. If verification is meant to be a real safety gate, this needs a server-issued proof or short-lived verification state tied to the user/conversation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/meta/meta-memory-upsert.ts` around lines
97 - 100, The current guard in meta_memory_upsert that checks only
args.verified_by_agent is insufficient because it trusts caller-supplied
booleans; replace it with a server-side verification check tied to the
user/conversation created by meta_memory_verify—e.g., when meta_memory_verify
runs it should create a short-lived verification token or set a verification
state in a server store (cache/db) mapped to conversationId/agentId; then in
meta_memory_upsert look up that server-side proof (not args.verified_by_agent)
via a helper like isConversationVerified(conversationId, agentId) or
validateVerificationToken(token) and reject the upsert unless the proof exists
and is still valid. Ensure to invalidate/expire the proof after use or time
window to prevent replay.
Comment on lines
+102
to
+113
| const result = await upsertMemory({ | ||
| env: ctx.env, | ||
| userId: user.userId, | ||
| memoryId: args.memory_id ?? null, | ||
| category: args.category ?? null, | ||
| subject: args.subject, | ||
| summary: args.summary, | ||
| details: args.details ?? '', | ||
| tags: args.tags ?? [], | ||
| dedupeKey: args.dedupe_key ?? null, | ||
| status: args.status ?? 'active', | ||
| verificationReference: args.verification_reference ?? null, |
There was a problem hiding this comment.
Updating a memory here will clear omitted optional fields.
On the update path, omitted category, details, tags, and dedupe_key are coerced to empty/default values, and Line 112 defaults status back to active. A caller that only wants to tweak summary will silently drop existing metadata and can unintentionally unarchive the record. Preserve existing values on update, or make replacement semantics explicit by requiring the full document.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/meta/meta-memory-upsert.ts` around lines
102 - 113, The current upsert call always coerces omitted optional fields to
defaults (category, details, tags, dedupe_key) and forces status to 'active',
which causes updates to wipe existing metadata; change the update path to
preserve existing values: when args.memory_id is present load the existing
memory (e.g., via getMemory or readMemory) and pass merged values into
upsertMemory (use existingMemory.category/details/tags/dedupeKey/status when the
corresponding args.* are null/undefined), and stop defaulting status to 'active'
on updates so an omitted status does not unarchive the record. Ensure you
reference upsertMemory, args.memory_id, args.category, args.details, args.tags,
args.dedupe_key, and args.status when implementing the merge.
| const query = input.query.trim() | ||
| const rowsById = new Map(input.rows.map((row) => [row.id, row] as const)) | ||
| const ids = [...rowsById.keys()] | ||
| const offline = isCapabilitySearchOffline(input.env) |
There was a problem hiding this comment.
Treat a missing vector index as offline and scope the query to one user.
getCapabilityVectorIndex(input.env)! can throw in environments that omit the optional binding, and filtering only by kind lets other users' memory_* vectors consume topK before you discard them with rowsById.has(memoryId). In a shared index, that turns scale into missed matches for the current user.
💡 Suggested change
- const offline = isCapabilitySearchOffline(input.env)
+ const index = getCapabilityVectorIndex(input.env)
+ const offline = isCapabilitySearchOffline(input.env) || !index
@@
- const index = getCapabilityVectorIndex(input.env)!
const queryVector = await embedTextForVectorize(input.env, query)
const topK = Math.min(Math.max(ids.length, input.limit * 5), 100)
const vectorMatches = await index.query(queryVector, {
topK,
returnMetadata: 'none',
filter: {
kind: { $eq: 'memory' },
+ userId: { $eq: input.rows[0]!.user_id },
},
})Also applies to: 48-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/memory/memory-search.ts` at line 24, The code
currently calls getCapabilityVectorIndex(input.env)! which may throw when the
optional binding is missing and then sets offline via
isCapabilitySearchOffline(input.env); change behavior to treat a missing vector
index as offline by checking getCapabilityVectorIndex(input.env) for
null/undefined before using it (do not use the non-null assertion), and when
running vector queries ensure results are scoped to the current user by adding a
user-specific filter (e.g., include the user id in the query or filter by
memoryId namespace) so that other users' memory_* vectors cannot occupy topK
before you prune with rowsById.has(memoryId); update the logic around offline,
getCapabilityVectorIndex, and the result filtering (references:
isCapabilitySearchOffline, getCapabilityVectorIndex, offline, rowsById.has,
memoryId, topK, kind) accordingly for both the initial block and the similar
code at lines 48-57.
Comment on lines
+269
to
+286
| const { matches } = await searchMemories({ | ||
| env: input.env as Env, | ||
| query, | ||
| limit: normalizeLimit(input.limit) * 3, | ||
| rows: filteredRows, | ||
| }) | ||
| const filtered = await filterSuppressedMatches({ | ||
| db: input.env.APP_DB, | ||
| userId: input.userId, | ||
| conversationId: input.conversationId ?? null, | ||
| includeSuppressedInConversation: | ||
| input.includeSuppressedInConversation ?? false, | ||
| matches, | ||
| }) | ||
| return { | ||
| query, | ||
| matches: filtered.matches.slice(0, normalizeLimit(input.limit)), | ||
| suppressedCount: filtered.suppressedCount, |
There was a problem hiding this comment.
The fixed limit * 3 overfetch runs out after a few suppressed batches.
When a conversation has already suppressed more than 3 * limit top-ranked matches, this path returns fewer than limit visible memories even though lower-ranked unsuppressed ones still exist. Repeated surfaceRelevantMemories calls will stop surfacing new results too early.
💡 Suggested change
- const { matches } = await searchMemories({
+ const requestedLimit = normalizeLimit(input.limit)
+ const searchLimit =
+ input.conversationId &&
+ !(input.includeSuppressedInConversation ?? false)
+ ? filteredRows.length
+ : requestedLimit * 3
+ const { matches } = await searchMemories({
env: input.env as Env,
query,
- limit: normalizeLimit(input.limit) * 3,
+ limit: searchLimit,
rows: filteredRows,
})
@@
return {
query,
- matches: filtered.matches.slice(0, normalizeLimit(input.limit)),
+ matches: filtered.matches.slice(0, requestedLimit),
suppressedCount: filtered.suppressedCount,
}📝 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 { matches } = await searchMemories({ | |
| env: input.env as Env, | |
| query, | |
| limit: normalizeLimit(input.limit) * 3, | |
| rows: filteredRows, | |
| }) | |
| const filtered = await filterSuppressedMatches({ | |
| db: input.env.APP_DB, | |
| userId: input.userId, | |
| conversationId: input.conversationId ?? null, | |
| includeSuppressedInConversation: | |
| input.includeSuppressedInConversation ?? false, | |
| matches, | |
| }) | |
| return { | |
| query, | |
| matches: filtered.matches.slice(0, normalizeLimit(input.limit)), | |
| suppressedCount: filtered.suppressedCount, | |
| const requestedLimit = normalizeLimit(input.limit) | |
| const searchLimit = | |
| input.conversationId && | |
| !(input.includeSuppressedInConversation ?? false) | |
| ? filteredRows.length | |
| : requestedLimit * 3 | |
| const { matches } = await searchMemories({ | |
| env: input.env as Env, | |
| query, | |
| limit: searchLimit, | |
| rows: filteredRows, | |
| }) | |
| const filtered = await filterSuppressedMatches({ | |
| db: input.env.APP_DB, | |
| userId: input.userId, | |
| conversationId: input.conversationId ?? null, | |
| includeSuppressedInConversation: | |
| input.includeSuppressedInConversation ?? false, | |
| matches, | |
| }) | |
| return { | |
| query, | |
| matches: filtered.matches.slice(0, requestedLimit), | |
| suppressedCount: filtered.suppressedCount, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/memory/service.ts` around lines 269 - 286, The
current overfetch of normalizeLimit(input.limit) * 3 can still yield fewer than
the requested visible memories if more than 3*limit top-ranked matches are
suppressed; update the logic around searchMemories + filterSuppressedMatches to
iteratively fetch additional pages until you have at least
normalizeLimit(input.limit) non-suppressed matches or there are no more matches.
Concretely, replace the single call to searchMemories(...) with a loop that
calls searchMemories using the existing rows/filteredRows pagination,
accumulates matches, calls filterSuppressedMatches(...) on the accumulated
result, and stops when filtered.matches.length >= normalizeLimit(input.limit) or
when searchMemories returns no new matches; then return
filtered.matches.slice(0, normalizeLimit(input.limit)) and
filtered.suppressedCount. Ensure you use the same symbols: searchMemories,
filterSuppressedMatches, normalizeLimit, matches, filteredRows, and input.limit.
Comment on lines
+110
to
+117
| if (!memorySummary || memorySummary.memories.length === 0) return content | ||
| return [ | ||
| ...content, | ||
| { | ||
| type: 'text', | ||
| text: formatRelevantMemoriesMarkdown(memorySummary), | ||
| }, | ||
| ] satisfies Array<ContentBlock> |
There was a problem hiding this comment.
Still render suppression-only context.
surfaceRelevantMemories can return suppressedCount > 0 with an empty memories array on repeated calls, but both helpers return early before formatRelevantMemoriesMarkdown can emit the suppression notice. That drops the only response-visible signal that relevant memories were intentionally withheld.
💡 Suggested change
export function appendRelevantMemoriesContent(
content: Array<ContentBlock>,
memorySummary: MemoryToolSummary | null,
) {
- if (!memorySummary || memorySummary.memories.length === 0) return content
+ if (
+ !memorySummary ||
+ (memorySummary.memories.length === 0 &&
+ memorySummary.suppressedCount === 0)
+ ) {
+ return content
+ }
return [
@@
export function formatSurfacedMemoriesMarkdown(
memorySummary: MemoryToolSummary | null,
) {
- if (!memorySummary || memorySummary.memories.length === 0) return []
+ if (
+ !memorySummary ||
+ (memorySummary.memories.length === 0 &&
+ memorySummary.suppressedCount === 0)
+ ) {
+ return []
+ }
return [Also applies to: 141-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/tools/memory-tool-context.ts` around lines 110 - 117,
The early return drops suppression-only notices: update the logic in
surfaceRelevantMemories (the block that currently returns content when
!memorySummary || memorySummary.memories.length === 0) to instead check
memorySummary.suppressedCount and, if suppressedCount > 0, append a ContentBlock
with formatRelevantMemoriesMarkdown(memorySummary) even when memories is empty;
make the analogous change in the other similar block (around the second
occurrence currently at lines 141-147) so suppression-only summaries are
rendered rather than skipped.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/worker/src/mcp/memory/memory-embed.ts (1)
3-11: Extract tag JSON parsing into a shared memory helper.This has the same recovery semantics as
packages/worker/src/mcp/memory/memory-search.tsLines 112-120. Keeping two copies oftags_jsonparsing in sync is easy to miss the first time validation or normalization changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/memory/memory-embed.ts` around lines 3 - 11, Extract the JSON array parsing logic used in parseJsonStringArray from memory-embed.ts into a shared helper (e.g., export a function like parseJsonStringArray or parseTagsJson from a new or existing shared module under packages/worker/src/mcp/memory), then import and use that helper in both memory-embed.ts and the corresponding tags_json parsing site in memory-search.ts; ensure the helper preserves the same semantics (try/catch returning [] on error, validating Array.isArray, and filtering items to strings with the same type guard) and update both call sites to remove their local duplicate parsing code and call the shared function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/mcp/capabilities/meta/meta-memory-verify.ts`:
- Around line 17-31: The meta_memory_verify capability is dropping suppressed
verify hits and defaulting include_suppressed_in_conversation to false; update
the logic and schema so suppressed hits are surfaced: add a suppressed_count
(number) field to the outputSchema alongside related_memories and ensure the
meta_memory_verify code passes include_suppressed_in_conversation=true (or
preserves the suppressedCount returned by the memory service) when calling the
memory service so suppressedCount is returned and not lost; reference
outputSchema, related_memories, suppressedCount and
include_suppressed_in_conversation in meta_memory_verify to locate and update
the behavior.
In `@packages/worker/src/mcp/memory/memory-search.ts`:
- Around line 58-68: The current logic builds fromIndex from real vector hits
(using vectorMatches.matches, seen, rowsById) but then sets vectorOrder by
appending ids.filter(...) which gives fake ranks to non-hits; instead keep
vectorOrder limited to actual index hits only (i.e., set vectorOrder to
fromIndex or to fromIndex filtered/ordered by ids intersection) so
reciprocalRankFusion receives only real ranked entries; remove the concatenation
with ids.filter(...) and ensure any downstream use of vectorOrder (e.g.,
reciprocalRankFusion) only sees IDs present in fromIndex.
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 562-567: The call to loadRelevantMemoriesForTool happens outside
the guarded try/catch so any DB/vector failure will escape after searchSpan
already logged success; either move the await into the existing guarded
searchSpan block (so failures are caught by its try/catch) or wrap this call in
its own try/catch inside search.ts and, on error, log a warning via the same
logger/context and set memoryToolContext to an empty/degraded value so the
search results still return; reference loadRelevantMemoriesForTool, searchSpan,
agent.getEnv(), conversationId, memoryContext and callerContext when locating
where to move or wrap the call.
---
Nitpick comments:
In `@packages/worker/src/mcp/memory/memory-embed.ts`:
- Around line 3-11: Extract the JSON array parsing logic used in
parseJsonStringArray from memory-embed.ts into a shared helper (e.g., export a
function like parseJsonStringArray or parseTagsJson from a new or existing
shared module under packages/worker/src/mcp/memory), then import and use that
helper in both memory-embed.ts and the corresponding tags_json parsing site in
memory-search.ts; ensure the helper preserves the same semantics (try/catch
returning [] on error, validating Array.isArray, and filtering items to strings
with the same type guard) and update both call sites to remove their local
duplicate parsing code and call the shared function.
🪄 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: 735ef39c-0675-4176-8a24-bd455dc5e76c
📒 Files selected for processing (7)
packages/worker/src/mcp/capabilities/meta/meta-memory-shared.tspackages/worker/src/mcp/capabilities/meta/meta-memory-upsert.tspackages/worker/src/mcp/capabilities/meta/meta-memory-verify.tspackages/worker/src/mcp/memory/memory-embed.tspackages/worker/src/mcp/memory/memory-search.tspackages/worker/src/mcp/tools/memory-tool-context.tspackages/worker/src/mcp/tools/search.ts
✅ Files skipped from review due to trivial changes (1)
- packages/worker/src/mcp/capabilities/meta/meta-memory-shared.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker/src/mcp/capabilities/meta/meta-memory-upsert.ts
Comment on lines
+17
to
+31
| const outputSchema = z.object({ | ||
| candidate: z.object({ | ||
| subject: z.string(), | ||
| summary: z.string(), | ||
| details: z.string(), | ||
| category: z.string().nullable(), | ||
| tags: z.array(z.string()), | ||
| dedupe_key: z.string().nullable(), | ||
| }), | ||
| related_memories: z.array(memoryMatchSchema), | ||
| guidance: z.string(), | ||
| recommended_actions: z.array( | ||
| z.enum(['upsert', 'delete', 'upsert_and_delete', 'none']), | ||
| ), | ||
| }) |
There was a problem hiding this comment.
Don't hide suppressed verify hits.
packages/worker/src/mcp/memory/service.ts already returns suppressedCount, but this capability drops it while defaulting include_suppressed_in_conversation to false. Re-running meta_memory_verify in the same conversation can therefore return related_memories: [] even when close matches were found and intentionally withheld, which weakens the verify-before-write contract.
💡 Suggested change
const outputSchema = z.object({
candidate: z.object({
subject: z.string(),
summary: z.string(),
details: z.string(),
category: z.string().nullable(),
tags: z.array(z.string()),
dedupe_key: z.string().nullable(),
}),
related_memories: z.array(memoryMatchSchema),
+ suppressed_count: z.number().int().nonnegative(),
guidance: z.string(),
recommended_actions: z.array(
z.enum(['upsert', 'delete', 'upsert_and_delete', 'none']),
),
})
@@
return {
candidate: result.candidate,
related_memories: result.relatedMemories.map((match) =>
formatMemoryMatch(match.memory, match.score),
),
+ suppressed_count: result.suppressedCount,
guidance: verifyFirstGuidance,
recommended_actions: recommendedActions,
}Also applies to: 58-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/meta/meta-memory-verify.ts` around lines
17 - 31, The meta_memory_verify capability is dropping suppressed verify hits
and defaulting include_suppressed_in_conversation to false; update the logic and
schema so suppressed hits are surfaced: add a suppressed_count (number) field to
the outputSchema alongside related_memories and ensure the meta_memory_verify
code passes include_suppressed_in_conversation=true (or preserves the
suppressedCount returned by the memory service) when calling the memory service
so suppressedCount is returned and not lost; reference outputSchema,
related_memories, suppressedCount and include_suppressed_in_conversation in
meta_memory_verify to locate and update the behavior.
Comment on lines
+58
to
+68
| const seen = new Set<string>() | ||
| const fromIndex: Array<string> = [] | ||
| for (const match of vectorMatches.matches) { | ||
| if (typeof match.id !== 'string' || seen.has(match.id)) continue | ||
| if (!match.id.startsWith('memory_')) continue | ||
| const memoryId = match.id.slice('memory_'.length) | ||
| if (!rowsById.has(memoryId)) continue | ||
| seen.add(match.id) | ||
| fromIndex.push(memoryId) | ||
| } | ||
| vectorOrder = [...fromIndex, ...ids.filter((id) => !fromIndex.includes(id))] |
There was a problem hiding this comment.
Don't assign fake vector ranks to IDs the index never returned.
fromIndex contains real vector hits, but Line 68 appends every remaining ID in input order and then feeds that list into reciprocalRankFusion. That gives arbitrary vector credit to memories with no vector match and can reorder results based on row iteration order instead of similarity. RRF already handles partial ranked lists, so vectorOrder should stay limited to actual index hits.
💡 Suggested change
const seen = new Set<string>()
const fromIndex: Array<string> = []
for (const match of vectorMatches.matches) {
if (typeof match.id !== 'string' || seen.has(match.id)) continue
if (!match.id.startsWith('memory_')) continue
const memoryId = match.id.slice('memory_'.length)
if (!rowsById.has(memoryId)) continue
seen.add(match.id)
fromIndex.push(memoryId)
}
- vectorOrder = [...fromIndex, ...ids.filter((id) => !fromIndex.includes(id))]
+ vectorOrder = fromIndex
}📝 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 seen = new Set<string>() | |
| const fromIndex: Array<string> = [] | |
| for (const match of vectorMatches.matches) { | |
| if (typeof match.id !== 'string' || seen.has(match.id)) continue | |
| if (!match.id.startsWith('memory_')) continue | |
| const memoryId = match.id.slice('memory_'.length) | |
| if (!rowsById.has(memoryId)) continue | |
| seen.add(match.id) | |
| fromIndex.push(memoryId) | |
| } | |
| vectorOrder = [...fromIndex, ...ids.filter((id) => !fromIndex.includes(id))] | |
| const seen = new Set<string>() | |
| const fromIndex: Array<string> = [] | |
| for (const match of vectorMatches.matches) { | |
| if (typeof match.id !== 'string' || seen.has(match.id)) continue | |
| if (!match.id.startsWith('memory_')) continue | |
| const memoryId = match.id.slice('memory_'.length) | |
| if (!rowsById.has(memoryId)) continue | |
| seen.add(match.id) | |
| fromIndex.push(memoryId) | |
| } | |
| vectorOrder = fromIndex |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/memory/memory-search.ts` around lines 58 - 68, The
current logic builds fromIndex from real vector hits (using
vectorMatches.matches, seen, rowsById) but then sets vectorOrder by appending
ids.filter(...) which gives fake ranks to non-hits; instead keep vectorOrder
limited to actual index hits only (i.e., set vectorOrder to fromIndex or to
fromIndex filtered/ordered by ids intersection) so reciprocalRankFusion receives
only real ranked entries; remove the concatenation with ids.filter(...) and
ensure any downstream use of vectorOrder (e.g., reciprocalRankFusion) only sees
IDs present in fromIndex.
Comment on lines
+562
to
+567
| const memoryToolContext = await loadRelevantMemoriesForTool({ | ||
| env: agent.getEnv(), | ||
| callerContext, | ||
| conversationId, | ||
| memoryContext: args.memoryContext, | ||
| }) |
There was a problem hiding this comment.
Keep memory enrichment inside the guarded search path.
This await happens after the only try/catch, so a DB/vector failure in loadRelevantMemoriesForTool will bubble out as an unhandled tool error after Line 534 already logged success. Move the memory lookup into searchSpan or catch it here and degrade it to a warning so the search results still return.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/tools/search.ts` around lines 562 - 567, The call to
loadRelevantMemoriesForTool happens outside the guarded try/catch so any
DB/vector failure will escape after searchSpan already logged success; either
move the await into the existing guarded searchSpan block (so failures are
caught by its try/catch) or wrap this call in its own try/catch inside search.ts
and, on error, log a warning via the same logger/context and set
memoryToolContext to an empty/degraded value so the search results still return;
reference loadRelevantMemoriesForTool, searchSpan, agent.getEnv(),
conversationId, memoryContext and callerContext when locating where to move or
wrap the call.
| if (!deleted) return null | ||
| await deleteMemoryVector(input.env as Env, input.memoryId) | ||
| return toMemoryRecord(existing) | ||
| } |
Contributor
There was a problem hiding this comment.
Force delete returns stale pre-deletion status
Medium Severity
The deleteMemory function's force-delete path returns toMemoryRecord(existing), which preserves the original status (e.g., 'active') and deleted_at (likely null). The soft-delete path correctly constructs a deletedRow with status: 'deleted' and a deleted_at timestamp. This inconsistency means a force-delete response looks like { ok: true, force: true, memory: { status: 'active', deleted_at: null } }, which could mislead consuming agents into thinking the deletion didn't succeed.
Additional Locations (1)
| suppressedCount: result.suppressedCount, | ||
| retrievalQuery: result.retrievalQuery, | ||
| } satisfies MemoryToolSummary | ||
| } |
Contributor
There was a problem hiding this comment.
Redundant memory surfacing functions duplicate same logic
Low Severity
loadRelevantMemoriesForTool and surfaceToolMemories are functionally identical — both extract userId from callerContext, validate the query, build the same storageContext object, call surfaceRelevantMemories, and map results with toMemoryToolSummaryItem. The only difference is the input shape (raw memoryContext vs. pre-built retrievalQuery string). loadRelevantMemoriesForTool could simply compose buildMemoryRetrievalQuery and delegate to surfaceToolMemories, eliminating the duplicated null-checks and service call.
There was a problem hiding this comment.
♻️ Duplicate comments (5)
packages/worker/src/mcp/memory/service.ts (3)
272-289:⚠️ Potential issue | 🟠 Major
limit * 3can still return fewer visible results than requested.If suppressed matches exceed the overfetch window, results get underfilled despite available unsuppressed candidates.
💡 Proposed fix
- const { matches } = await searchMemories({ + const requestedLimit = normalizeLimit(input.limit) + const searchLimit = + input.conversationId && + !(input.includeSuppressedInConversation ?? false) + ? filteredRows.length + : requestedLimit * 3 + const { matches } = await searchMemories({ env: input.env as Env, query, - limit: normalizeLimit(input.limit) * 3, + limit: searchLimit, rows: filteredRows, }) @@ return { query, - matches: filtered.matches.slice(0, normalizeLimit(input.limit)), + matches: filtered.matches.slice(0, requestedLimit), suppressedCount: filtered.suppressedCount, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/memory/service.ts` around lines 272 - 289, The current overfetch uses searchMemories with limit: normalizeLimit(input.limit) * 3 which can still return fewer visible results than requested when many matches are suppressed; update the logic in the flow around searchMemories, filterSuppressedMatches, and the final slice to guarantee up to normalizeLimit(input.limit) unsuppressed items by repeatedly fetching more (or increasing the overfetch) until filtered.matches has at least normalizeLimit(input.limit) non-suppressed results or no more candidates remain; ensure you reference and adjust the parameters passed to searchMemories and re-run filterSuppressedMatches on additional fetched matches (using the existing functions searchMemories and filterSuppressedMatches and the normalizeLimit(input.limit) target) and then return the first target unsuppressed items and correct suppressedCount.
309-315:⚠️ Potential issue | 🟠 MajorVerification search should not be hard-filtered by candidate category.
Line 314 narrows recall and can miss relevant memories that are uncategorized or categorized differently.
💡 Proposed fix
const result = await searchMemoryRecords({ env: input.env, userId: input.userId, storageContext: input.storageContext, query, - category: candidate.category, limit: input.limit, conversationId: input.conversationId ?? null, includeSuppressedInConversation: input.includeSuppressedInConversation ?? false, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/memory/service.ts` around lines 309 - 315, The verification recall is being hard-filtered by candidate.category in the call to searchMemoryRecords, which can miss relevant memories; update the call in service.ts (the searchMemoryRecords invocation that sets result) to omit or set category undefined for verification searches (i.e., only include candidate.category when performing a normal recall, not verification) so the verification search is not constrained by category while keeping other args like env, userId, storageContext, query, and limit unchanged.
157-170:⚠️ Potential issue | 🔴 CriticalDon’t fail the request after the D1 mutation is already committed.
These vector calls run after durable DB writes; if they throw, callers see an error even though state already changed.
💡 Proposed fix (best-effort vector sync)
- await upsertMemoryVector(input.env as Env, { - memoryId: row.id, - userId: input.userId, - category: row.category, - status: row.status, - embedText: buildMemoryEmbedText({ - category: row.category, - subject: row.subject, - summary: row.summary, - details: row.details, - tags: normalized.tags, - dedupeKey: row.dedupe_key, - }), - }) + try { + await upsertMemoryVector(input.env as Env, { + memoryId: row.id, + userId: input.userId, + category: row.category, + status: row.status, + embedText: buildMemoryEmbedText({ + category: row.category, + subject: row.subject, + summary: row.summary, + details: row.details, + tags: normalized.tags, + dedupeKey: row.dedupe_key, + }), + }) + } catch (error) { + console.error('memory vector upsert failed', { + memoryId: row.id, + userId: input.userId, + error, + }) + }Apply the same pattern to
deleteMemoryVector(...)and the soft-deleteupsertMemoryVector(...)call.Also applies to: 203-204, 227-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/memory/service.ts` around lines 157 - 170, The vector operations (upsertMemoryVector and deleteMemoryVector) run after durable D1 mutations and must not surface errors to callers; wrap each post-commit call to upsertMemoryVector(...) and deleteMemoryVector(...) (including the soft-delete upsert calls around the other mentioned sites) in a try/catch, log the error with context (memoryId, userId, category) and do not rethrow so the request succeeds even if vector sync fails; apply this best-effort pattern to the occurrences around the shown upsert (and the other call sites referenced).packages/worker/src/mcp/memory/memory-search.ts (2)
24-25:⚠️ Potential issue | 🟠 MajorHandle missing vector index as offline, and scope vector query to the current user.
Line 24/Line 48 can still throw when
CAPABILITY_VECTOR_INDEXis unset, and Line 55-only filtering allows unrelated users’ vectors to occupytopKbefore local pruning.💡 Proposed fix
- const offline = isCapabilitySearchOffline(input.env) + const index = getCapabilityVectorIndex(input.env) + const offline = isCapabilitySearchOffline(input.env) || !index @@ - const index = getCapabilityVectorIndex(input.env)! const queryVector = await embedTextForVectorize(input.env, query) const topK = Math.min(Math.max(ids.length, input.limit * 5), 100) - const vectorMatches = await index.query(queryVector, { + const vectorMatches = await index.query(queryVector, { topK, returnMetadata: 'none', filter: { kind: { $eq: 'memory' }, + userId: { $eq: input.rows[0]!.user_id }, }, })Also applies to: 48-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/memory/memory-search.ts` around lines 24 - 25, The code should treat a missing CAPABILITY_VECTOR_INDEX as offline and restrict vector searches to the current user: update the offline check around isCapabilitySearchOffline(input.env) to consider CAPABILITY_VECTOR_INDEX unset as offline, and when performing the vector query (the block using topK and vectorIndex lookup) add a metadata filter (e.g. metadata.userId or owner) tied to input.user/id to scope results to the current user before applying local pruning; ensure local pruning logic (the post-query topK trimming) runs after filtering so unrelated users’ vectors cannot occupy the topK.
58-68:⚠️ Potential issue | 🟠 MajorOnly assign vector ranks to actual index hits.
Line 68 currently gives vector rank credit to IDs not returned by vector search, which distorts fusion scoring.
💡 Proposed fix
- vectorOrder = [...fromIndex, ...ids.filter((id) => !fromIndex.includes(id))] + vectorOrder = fromIndex🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/memory/memory-search.ts` around lines 58 - 68, The code gives vector-rank credit to IDs that don't match the vector search because fromIndex stores memoryId (no "memory_" prefix) while ids likely contains the full prefixed IDs; normalize formats before composing vectorOrder: while iterating vectorMatches.matches keep the existing memoryId for rowsById lookups but also build a parallel array (e.g., fromIndexFull) that pushes the original match.id (with "memory_" prefix), then compute vectorOrder = [...fromIndexFull, ...ids.filter(id => !fromIndexFull.includes(id))] so only actual vector hits get front-ranked; ensure you reference vectorMatches.matches, match.id, rowsById, fromIndex (or new fromIndexFull) and ids in the change.
🧹 Nitpick comments (1)
packages/worker/src/mcp/memory/service.ts (1)
329-343: Consider extracting sharedMemorySearchMatch -> MemoryRecordmapping.The same field mapping appears twice; a small helper would reduce drift risk.
Also applies to: 375-388
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/memory/service.ts` around lines 329 - 343, Extract the duplicated mapping from a MemorySearchMatch to the MemoryRecord object into a single helper (e.g., mapSearchMatchToMemoryRecord) and use it wherever result.matches.map(...) is currently expanding the memory fields (the block building relatedMemories and the similar block around lines 375-388). Update the two places to call mapSearchMatchToMemoryRecord(match) to return the { memory: { id, category, status, subject, summary, details, tags, dedupeKey, createdAt, updatedAt, lastAccessedAt, deletedAt } } shape, ensuring types/signatures match the existing MemorySearchMatch and MemoryRecord definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/worker/src/mcp/memory/memory-search.ts`:
- Around line 24-25: The code should treat a missing CAPABILITY_VECTOR_INDEX as
offline and restrict vector searches to the current user: update the offline
check around isCapabilitySearchOffline(input.env) to consider
CAPABILITY_VECTOR_INDEX unset as offline, and when performing the vector query
(the block using topK and vectorIndex lookup) add a metadata filter (e.g.
metadata.userId or owner) tied to input.user/id to scope results to the current
user before applying local pruning; ensure local pruning logic (the post-query
topK trimming) runs after filtering so unrelated users’ vectors cannot occupy
the topK.
- Around line 58-68: The code gives vector-rank credit to IDs that don't match
the vector search because fromIndex stores memoryId (no "memory_" prefix) while
ids likely contains the full prefixed IDs; normalize formats before composing
vectorOrder: while iterating vectorMatches.matches keep the existing memoryId
for rowsById lookups but also build a parallel array (e.g., fromIndexFull) that
pushes the original match.id (with "memory_" prefix), then compute vectorOrder =
[...fromIndexFull, ...ids.filter(id => !fromIndexFull.includes(id))] so only
actual vector hits get front-ranked; ensure you reference vectorMatches.matches,
match.id, rowsById, fromIndex (or new fromIndexFull) and ids in the change.
In `@packages/worker/src/mcp/memory/service.ts`:
- Around line 272-289: The current overfetch uses searchMemories with limit:
normalizeLimit(input.limit) * 3 which can still return fewer visible results
than requested when many matches are suppressed; update the logic in the flow
around searchMemories, filterSuppressedMatches, and the final slice to guarantee
up to normalizeLimit(input.limit) unsuppressed items by repeatedly fetching more
(or increasing the overfetch) until filtered.matches has at least
normalizeLimit(input.limit) non-suppressed results or no more candidates remain;
ensure you reference and adjust the parameters passed to searchMemories and
re-run filterSuppressedMatches on additional fetched matches (using the existing
functions searchMemories and filterSuppressedMatches and the
normalizeLimit(input.limit) target) and then return the first target
unsuppressed items and correct suppressedCount.
- Around line 309-315: The verification recall is being hard-filtered by
candidate.category in the call to searchMemoryRecords, which can miss relevant
memories; update the call in service.ts (the searchMemoryRecords invocation that
sets result) to omit or set category undefined for verification searches (i.e.,
only include candidate.category when performing a normal recall, not
verification) so the verification search is not constrained by category while
keeping other args like env, userId, storageContext, query, and limit unchanged.
- Around line 157-170: The vector operations (upsertMemoryVector and
deleteMemoryVector) run after durable D1 mutations and must not surface errors
to callers; wrap each post-commit call to upsertMemoryVector(...) and
deleteMemoryVector(...) (including the soft-delete upsert calls around the other
mentioned sites) in a try/catch, log the error with context (memoryId, userId,
category) and do not rethrow so the request succeeds even if vector sync fails;
apply this best-effort pattern to the occurrences around the shown upsert (and
the other call sites referenced).
---
Nitpick comments:
In `@packages/worker/src/mcp/memory/service.ts`:
- Around line 329-343: Extract the duplicated mapping from a MemorySearchMatch
to the MemoryRecord object into a single helper (e.g.,
mapSearchMatchToMemoryRecord) and use it wherever result.matches.map(...) is
currently expanding the memory fields (the block building relatedMemories and
the similar block around lines 375-388). Update the two places to call
mapSearchMatchToMemoryRecord(match) to return the { memory: { id, category,
status, subject, summary, details, tags, dedupeKey, createdAt, updatedAt,
lastAccessedAt, deletedAt } } shape, ensuring types/signatures match the
existing MemorySearchMatch and MemoryRecord definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 871873d6-fd2e-487d-be81-842de861ed83
📒 Files selected for processing (2)
packages/worker/src/mcp/memory/memory-search.tspackages/worker/src/mcp/memory/service.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
| callerContext, | ||
| conversationId: resolvedConversationId, | ||
| retrievalQuery: buildMemoryRetrievalQuery(memoryContext), | ||
| }) |
Contributor
There was a problem hiding this comment.
Memory surfacing errors crash all primary tool handlers
High Severity
The surfaceToolMemories and loadRelevantMemoriesForTool calls in execute, search, and open_generated_ui tool handlers lack try/catch. If the memory subsystem throws (e.g., a D1 error in pruneExpiredConversationSuppressions, an AI embedding failure in searchMemories, or a DB write error in upsertConversationSuppressions), the entire tool call fails — even though memory surfacing is supplementary. The core tool functionality (code execution, search ranking, UI generation) is fully computed or ready but never returned to the client.
Additional Locations (2)
This was referenced Apr 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request contains changes generated by a Cursor Cloud Agent
Summary by CodeRabbit
New Features
Documentation
Tests