Add optional external_id metadata passthrough for memory retrieval results#583
Add optional external_id metadata passthrough for memory retrieval results#583XelHaku wants to merge 1 commit into
Conversation
|
@XelHaku is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR adds optional ChangesExternal ID and Metadata Passthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/remember-external-metadata.test.ts (1)
58-120: ⚡ Quick winAdd compact and narrative format assertions for passthrough fields.
These tests validate passthrough on default/full results, but the PR also changes
format: "compact"andformat: "narrative"mapping. Add targeted assertions there to prevent silent regressions in the exact changed surface.🤖 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/remember-external-metadata.test.ts` around lines 58 - 120, The tests "keeps duplicate-content memories distinguishable by external_id in search results" and "returns saved metadata unchanged in search results" need additional assertions for the new format mapping; after calling sdk.trigger for function_id "mem::search" add explicit checks that when payload.format is "compact" and when payload.format is "narrative" the returned result.results[*].observation still contains the exact external_id and metadata passthrough (use the same external_id "locomo-conv-1-turn-3" and metadata object), and for the duplicate-content case assert that both external_ids ("source-a","source-b") are present in compact and narrative responses as well; locate these tests and update the search-trigger calls to include payload.format = "compact" and payload.format = "narrative" and add corresponding expect(...) assertions for observation.external_id and observation.metadata.
🤖 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 `@test/remember-external-metadata.test.ts`:
- Around line 37-51: Current test defines a local mockSdk (with functions Map,
registerFunction/registerTrigger/trigger) instead of using the repository-wide
iii-sdk mocking pattern; replace this local mock with a vi.mock("iii-sdk") mock
in this test that exposes the same API used across tests (a mocked sdk with
trigger, and a kv object implementing get/set/list) and ensure the mocked
trigger honors registration semantics similar to the existing
registerFunction/registerTrigger/trigger behavior so existing calls relying on
functions Map continue to work (e.g., handlers registered via registerFunction
are invoked by mock trigger and special-case "mem::cascade-update" remains
supported).
---
Nitpick comments:
In `@test/remember-external-metadata.test.ts`:
- Around line 58-120: The tests "keeps duplicate-content memories
distinguishable by external_id in search results" and "returns saved metadata
unchanged in search results" need additional assertions for the new format
mapping; after calling sdk.trigger for function_id "mem::search" add explicit
checks that when payload.format is "compact" and when payload.format is
"narrative" the returned result.results[*].observation still contains the exact
external_id and metadata passthrough (use the same external_id
"locomo-conv-1-turn-3" and metadata object), and for the duplicate-content case
assert that both external_ids ("source-a","source-b") are present in compact and
narrative responses as well; locate these tests and update the search-trigger
calls to include payload.format = "compact" and payload.format = "narrative" and
add corresponding expect(...) assertions for observation.external_id and
observation.metadata.
🪄 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: 5f11b521-2f36-470c-a37d-aaf4c5aa9b63
📒 Files selected for processing (11)
CHANGELOG.mdREADME.mdsrc/functions/remember.tssrc/functions/search.tssrc/mcp/server.tssrc/mcp/standalone.tssrc/mcp/tools-registry.tssrc/state/memory-utils.tssrc/triggers/api.tssrc/types.tstest/remember-external-metadata.test.ts
| function mockSdk() { | ||
| const functions = new Map<string, Function>(); | ||
| return { | ||
| registerFunction: (id: string, handler: Function) => { | ||
| functions.set(id, handler); | ||
| }, | ||
| registerTrigger: () => {}, | ||
| trigger: async (input: { function_id: string; payload: unknown }) => { | ||
| const fn = functions.get(input.function_id); | ||
| if (!fn && input.function_id === "mem::cascade-update") return undefined; | ||
| if (!fn) throw new Error(`unknown fn ${input.function_id}`); | ||
| return fn(input.payload); | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Align this test harness with the repository’s iii-sdk mocking contract.
This file builds a custom SDK/KV harness instead of mocking iii-sdk via vi.mock("iii-sdk"), which diverges from the enforced test pattern and makes this suite inconsistent with the rest of the test surface.
As per coding guidelines, test/**/*.test.{ts,tsx}: Mock iii-sdk in tests using vi.mock("iii-sdk") with mock sdk.trigger, kv.get/set/list.
🤖 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/remember-external-metadata.test.ts` around lines 37 - 51, Current test
defines a local mockSdk (with functions Map,
registerFunction/registerTrigger/trigger) instead of using the repository-wide
iii-sdk mocking pattern; replace this local mock with a vi.mock("iii-sdk") mock
in this test that exposes the same API used across tests (a mocked sdk with
trigger, and a kv object implementing get/set/list) and ensure the mocked
trigger honors registration semantics similar to the existing
registerFunction/registerTrigger/trigger behavior so existing calls relying on
functions Map continue to work (e.g., handlers registered via registerFunction
are invoked by mock trigger and special-case "mem::cascade-update" remains
supported).
1 participant
Hi! This PR adds optional
external_idand metadata passthrough support for saved memories and retrieval results.The motivation is to make integrations easier when memories originate from another system or dataset and need stable source identifiers. This helps with import/export workflows, deduplication, observability, debugging, and deterministic evaluation where callers need to know exactly which source memory was retrieved.
The change is additive: existing callers can ignore the new fields, and default behavior should remain unchanged.
Included:
external_idon memory save/createexternal_idand metadataValidation run locally:
npm test -- test/remember-external-metadata.test.tsnpm testnpm run buildSummary by CodeRabbit
New Features
external_idandmetadatafields when saving memories. These fields are persisted and returned in search results, supporting deduplication, observability, and deterministic evaluation.Documentation