Repository navigation
Add support for source URIs on memories - #168
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds optional Changes
Sequence Diagram(s)mermaid Client->>MCP: codemode.meta_memory_upsert { source_uris: [...] } Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 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 |
|
🔎 Preview deployed: https://kody-pr-168.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts (1)
81-90: Consider an explicit guard formemoryIdbefore templating it into execute code.This makes failures clearer and avoids accidentally generating
memory_id: undefinedif earlier expectations are refactored later.Optional test hardening diff
const memoryId = upsertStructured?.result?.memory?.id + expect(memoryId).toBeDefined() + if (!memoryId) { + throw new Error('Expected memory id from meta_memory_upsert') + } const getResult = await mcpClient.client.callTool({ name: 'execute', arguments: { code: `async () => { return await codemode.meta_memory_get({ memory_id: ${JSON.stringify(memoryId)}, }) }`, }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts` around lines 81 - 90, The test currently templates memoryId (from upsertStructured?.result?.memory?.id) directly into the execute code which can produce `memory_id: undefined`; add an explicit guard/assert for memoryId before calling mcpClient.client.callTool (for example check `if (!memoryId) throw new Error("missing memoryId")` or use the test framework's expect/assert) so the test fails with a clear message and never interpolates an undefined value into the code string passed to mcpClient.client.callTool.packages/worker/src/mcp/capabilities/meta/meta-memory-verify.ts (1)
38-38: Consider aligning the schema withmemoryRecordSchemafor consistency.The
candidate.source_urisis defined as a required arrayz.array(z.string().url()), whilememoryRecordSchemainmeta-memory-shared.tsdefines it asz.array(memorySourceUriSchema).optional().While this works correctly (since
verifyMemoryCandidatealways returnssource_urisas an array), using the sharedmemorySourceUriSchemawould improve consistency:♻️ Suggested improvement
+import { + memoryRecordSchema, + memorySourceUriSchema, + memoryVerifyInputSchema, + requireMcpUser, +} from './meta-memory-shared.ts' 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()), - source_uris: z.array(z.string().url()), + source_uris: z.array(memorySourceUriSchema), dedupe_key: z.string().nullable(), }),🤖 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` at line 38, The candidate schema in meta-memory-verify.ts uses source_uris: z.array(z.string().url()) but should align with the shared memoryRecordSchema; replace this with the shared memorySourceUriSchema (and make it optional like memoryRecordSchema) so the candidate validation uses z.array(memorySourceUriSchema).optional() (or the equivalent pattern used in meta-memory-shared.ts) — update the schema where candidate.source_uris is defined and import memorySourceUriSchema from meta-memory-shared.ts and keep verifyMemoryCandidate behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/meta/meta-memory-verify.ts`:
- Line 38: The candidate schema in meta-memory-verify.ts uses source_uris:
z.array(z.string().url()) but should align with the shared memoryRecordSchema;
replace this with the shared memorySourceUriSchema (and make it optional like
memoryRecordSchema) so the candidate validation uses
z.array(memorySourceUriSchema).optional() (or the equivalent pattern used in
meta-memory-shared.ts) — update the schema where candidate.source_uris is
defined and import memorySourceUriSchema from meta-memory-shared.ts and keep
verifyMemoryCandidate behavior intact.
In `@packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts`:
- Around line 81-90: The test currently templates memoryId (from
upsertStructured?.result?.memory?.id) directly into the execute code which can
produce `memory_id: undefined`; add an explicit guard/assert for memoryId before
calling mcpClient.client.callTool (for example check `if (!memoryId) throw new
Error("missing memoryId")` or use the test framework's expect/assert) so the
test fails with a clear message and never interpolates an undefined value into
the code string passed to mcpClient.client.callTool.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd192d2e-925f-4658-851f-d70925e8cee8
📒 Files selected for processing (15)
docs/use/memory.mdpackages/worker/migrations/0018-mcp-memory-source-uris.sqlpackages/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-search.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/tools/memory-tool-context.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Source URIs array lacks max size constraint
- Added explicit source URI count/length limits in the schema and normalization logic to prevent unbounded storage.
- ✅ Fixed: Redundant identical function duplicates existing helper
- Deduplicated the JSON array parsing by using a shared helper for tags and source URIs.
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit c6d8de0. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
2 participants
Summary
source_urissupport to memory types, zod schemas, service normalization, D1 persistence, and all MCP memory read responses[]and adding a D1 migration with asource_uris_jsoncolumn defaultTesting
npx vitest run --project node-unit packages/worker/src/mcp/memory/service.node.test.tsnode tools/prepare-e2e-env.ts && npm run build:mcp-apps && npx vitest run --project mcp-e2e packages/worker/src/mcp/mcp-server.mcp-e2e.test.tsnpm run typecheckFollow-up
memorySourceUriSchemainmeta-memory-verify.tsmemoryIdguard inmcp-server.mcp-e2e.test.tsbefore templating themeta_memory_getexecute payloadSummary by CodeRabbit
New Features
Documentation
Tests
Bug Fix / Migration