feat(mcp): mem::query — server-side composable retrieval pipeline (v5-a)#574
Open
efenex wants to merge 8 commits into
Open
feat(mcp): mem::query — server-side composable retrieval pipeline (v5-a)#574efenex wants to merge 8 commits into
efenex wants to merge 8 commits into
Conversation
Returns chronologically-sorted hits across observation/memory/lesson/ summary channels — answers "when did this term enter the corpus and what surrounded it?". Includes BM25 sweep over obs+memory, substring scan for lessons/summaries, optional adjacent-turn enrichment, and optional graph-neighbor attachment. Gap-2 fix bundled: BM25 sweep cap raised from min(limit*4, 500) to min(limit*20, 5000) so deep in-session refs in large jsonl-imported sessions (10k+ obs) still rank into the channel-filtered top N. Wires: - src/functions/lineage.ts (new) - mem::lineage MCP tool in CORE_TOOLS - POST /agentmemory/lineage REST endpoint - AuditEntry operation: + "query" - LineageChannel / TimelineItem / LineageGraphNeighbor / LineageResult types - design + test-case docs under docs/plans/ Counts bumped to keep README/AGENTS/boot message/test in sync: CORE_TOOLS 12 → 13, total MCP tools 51 → 52, REST endpoints 121 → 122. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@efenex 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
📝 WalkthroughWalkthroughThis PR adds mem::lineage (chronological multi-channel lineage with optional session/graph enrichment) and mem::query (a server-side composable pipeline executor). Both include types, implementations, MCP/HTTP exposure, worker registration, tests, and design documentation. ChangesLineage & Query Retrieval System
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as /agentmemory/lineage
participant MCP as MCP tool call
participant Engine as mem::lineage / mem::query
participant KV as StateKV
participant LLM as MemoryProvider/LLM
Client->>API: POST query payload
API->>Engine: sdk.trigger(mem::lineage)
alt mem::lineage flow
Engine->>KV: BM25/index query & KV resolves
Engine->>KV: load session / graph nodes
else mem::query flow
Engine->>MCP: trigger producer tools (sessions/search/lineage/...)
MCP->>Engine: EnvelopedRecord[] results
Engine->>LLM: rank_by_relevance / synthesize calls
end
Engine->>Client: LineageResult / QueryResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/functions/lineage.ts (1)
358-381: 💤 Low value
firstMentionreflects only returned items, not all matches.When
items.length > limit,firstMentionuses the trimmed array and may not represent the true earliest match across all candidates. ThetotalsByChannelcounts all items, creating a slight semantic mismatch. If the intent is to show the absolute earliest match, consider derivingfirstMentionfrom the fullitemsarray (e.g.,items[0]when sorted ascending, oritems[items.length - 1]when sorted descending before trimming).If returning the earliest among displayed results is intentional, a clarifying comment would help.
🤖 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/lineage.ts` around lines 358 - 381, The code currently computes firstMention from trimmed (displayed) results while totalsByChannel is derived from items (all matches); change firstMention to be derived from the full items array (not trimmed) so it reflects the absolute earliest match: when order === "asc" use items[0], otherwise use items[items.length - 1], guarding for empty items to return null; update the assignment to firstMention and keep totalsByChannel as-is (or add a clarifying comment if the original intention was to use displayed results).src/functions/query.ts (1)
557-564: 💤 Low value
applyProjectrename preserves original field.When renaming (e.g.,
{ title: "headline" }), both the originaltitleand newheadlinekeys will exist in the output. This behavior is documented in the tests, but if callers expect the original key to be removed, this could cause confusion or bloated records. Consider adding aremoveOriginaloption or documenting this clearly in type-level comments.🤖 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/query.ts` around lines 557 - 564, The rename loop in applyProject currently preserves the original field (using rename, resolveDotPath and out) which leaves both keys; update the logic to remove the original path after successfully assigning the renamed value: after setting out[to] = v, delete the original property at the dot-path `from` from `out` (taking care when `from === to` to skip deletion), and use or add a helper like deleteDotPath(out, from) that handles nested/dotted keys; ensure any type signatures for applyProject reflect this behavior or add a removeOriginal option flag if you prefer to make removal configurable.
🤖 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 `@docs/plans/v4-lineage-design.md`:
- Line 126: The fenced code block that currently uses only triple backticks
(```) in docs/plans/v4-lineage-design.md should include an explicit language
identifier (e.g., change ``` to ```text) to satisfy markdown linting and
tooling; update the fence so the block starts with ```text (or another
appropriate language) and leave the block contents unchanged.
In `@docs/plans/v4-lineage-test-case-careful-generator.md`:
- Line 19: The markdown contains a fenced code block opened with triple
backticks that lacks a language tag; update that fence (the ``` block) to
include a language identifier (e.g., text) so the block becomes ```text to
satisfy MD040/markdownlint and stop the lint warning.
In `@README.md`:
- Line 802: The README now states "55 tools, 6 resources, 3 prompts, and 4
skills" but other nearby strings still reference "51 Tools" and "51-tool
surface"; update all tool-count mentions to be consistent (replace "51 Tools",
"51-tool surface", and any other "51" occurrences referring to the toolset) so
they read "55 Tools" / "55-tool surface" (or match the exact phrasing "55 tools,
6 resources, 3 prompts, and 4 skills") across the README; ensure
pluralization/casing stays consistent with the surrounding headings and any
table of contents entries.
In `@src/mcp/server.ts`:
- Around line 285-299: The memory_lineage handler currently parses channels with
parseCsvList and forwards order directly; add whitelist validation to fail fast
with a 400: after channels = parseCsvList(args.channels) validate each channel
against an allowed set (e.g., a constant array or Set) and return a 400 error if
any channel is not allowed, and validate args.order to only accept "asc" or
"desc" (otherwise return 400); keep using asNumber for limit and
sdk.trigger(...) for the call, and only add the validated channels and
normalized order to payload (use the same symbols: parseCsvList, asNumber,
sdk.trigger, and the memory_lineage handler) so invalid inputs are rejected
before triggering internal functions.
In `@src/triggers/api.ts`:
- Around line 1039-1042: The code currently forwards the entire raw request body
into sdk.trigger in the api::lineage handler (payload: req.body); instead build
and pass a sanitized payload object containing only the validated/whitelisted
fields (e.g., the specific fields extracted/validated earlier in this handler or
from validateLineageInput) and use that object as payload to sdk.trigger({
function_id: "mem::lineage", payload: sanitizedPayload }); ensure you do not
pass req.body anywhere to sdk.trigger and reference the handler and sdk.trigger
call so the change is localized.
---
Nitpick comments:
In `@src/functions/lineage.ts`:
- Around line 358-381: The code currently computes firstMention from trimmed
(displayed) results while totalsByChannel is derived from items (all matches);
change firstMention to be derived from the full items array (not trimmed) so it
reflects the absolute earliest match: when order === "asc" use items[0],
otherwise use items[items.length - 1], guarding for empty items to return null;
update the assignment to firstMention and keep totalsByChannel as-is (or add a
clarifying comment if the original intention was to use displayed results).
In `@src/functions/query.ts`:
- Around line 557-564: The rename loop in applyProject currently preserves the
original field (using rename, resolveDotPath and out) which leaves both keys;
update the logic to remove the original path after successfully assigning the
renamed value: after setting out[to] = v, delete the original property at the
dot-path `from` from `out` (taking care when `from === to` to skip deletion),
and use or add a helper like deleteDotPath(out, from) that handles nested/dotted
keys; ensure any type signatures for applyProject reflect this behavior or add a
removeOriginal option flag if you prefer to make removal configurable.
🪄 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: 3517000d-ef14-4fa4-a5c4-8e835788a94c
📒 Files selected for processing (14)
AGENTS.mdREADME.mddocs/plans/v4-lineage-design.mddocs/plans/v4-lineage-test-case-careful-generator.mdsrc/functions/lineage.tssrc/functions/query.tssrc/index.tssrc/mcp/server.tssrc/mcp/tools-registry.tssrc/triggers/api.tssrc/types.tstest/mcp-standalone.test.tstest/query-integration.test.tstest/query-transformers.test.ts
Comment on lines
+285
to
+299
| const channels = parseCsvList(args.channels); | ||
| const payload: Record<string, unknown> = { | ||
| query: args.query, | ||
| }; | ||
| const limit = asNumber(args.limit); | ||
| if (limit !== undefined) payload.limit = Math.max(1, Math.min(500, limit)); | ||
| if (typeof args.since === "string") payload.since = args.since; | ||
| if (typeof args.until === "string") payload.until = args.until; | ||
| if (channels.length > 0) payload.channels = channels; | ||
| if (typeof args.includeAdjacentTurns === "boolean") | ||
| payload.includeAdjacentTurns = args.includeAdjacentTurns; | ||
| if (typeof args.includeGraph === "boolean") | ||
| payload.includeGraph = args.includeGraph; | ||
| if (typeof args.order === "string") payload.order = args.order; | ||
| const result = await sdk.trigger({ |
Contributor
There was a problem hiding this comment.
Whitelist channels and order in memory_lineage handler.
You parse channels, but don’t validate allowed channel names or constrain order to asc|desc before forwarding. Add boundary checks to fail fast with 400.
Suggested patch
const channels = parseCsvList(args.channels);
+ const allowedChannels = new Set(["observation", "memory", "lesson", "summary"]);
+ if (channels.some((c) => !allowedChannels.has(c))) {
+ return {
+ status_code: 400,
+ body: { error: "channels must be observation,memory,lesson,summary" },
+ };
+ }
const payload: Record<string, unknown> = {
query: args.query,
};
@@
- if (typeof args.order === "string") payload.order = args.order;
+ if (typeof args.order === "string") {
+ const order = args.order.trim().toLowerCase();
+ if (order !== "asc" && order !== "desc") {
+ return { status_code: 400, body: { error: "order must be 'asc' or 'desc'" } };
+ }
+ payload.order = order;
+ }As per coding guidelines: "MCP tool handlers must validate arguments with typeof checks, parse CSV arguments using split/map/filter, trigger internal functions via sdk.trigger(), and return results formatted as MCP text content".
📝 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 channels = parseCsvList(args.channels); | |
| const payload: Record<string, unknown> = { | |
| query: args.query, | |
| }; | |
| const limit = asNumber(args.limit); | |
| if (limit !== undefined) payload.limit = Math.max(1, Math.min(500, limit)); | |
| if (typeof args.since === "string") payload.since = args.since; | |
| if (typeof args.until === "string") payload.until = args.until; | |
| if (channels.length > 0) payload.channels = channels; | |
| if (typeof args.includeAdjacentTurns === "boolean") | |
| payload.includeAdjacentTurns = args.includeAdjacentTurns; | |
| if (typeof args.includeGraph === "boolean") | |
| payload.includeGraph = args.includeGraph; | |
| if (typeof args.order === "string") payload.order = args.order; | |
| const result = await sdk.trigger({ | |
| const channels = parseCsvList(args.channels); | |
| const allowedChannels = new Set(["observation", "memory", "lesson", "summary"]); | |
| if (channels.some((c) => !allowedChannels.has(c))) { | |
| return { | |
| status_code: 400, | |
| body: { error: "channels must be observation,memory,lesson,summary" }, | |
| }; | |
| } | |
| const payload: Record<string, unknown> = { | |
| query: args.query, | |
| }; | |
| const limit = asNumber(args.limit); | |
| if (limit !== undefined) payload.limit = Math.max(1, Math.min(500, limit)); | |
| if (typeof args.since === "string") payload.since = args.since; | |
| if (typeof args.until === "string") payload.until = args.until; | |
| if (channels.length > 0) payload.channels = channels; | |
| if (typeof args.includeAdjacentTurns === "boolean") | |
| payload.includeAdjacentTurns = args.includeAdjacentTurns; | |
| if (typeof args.includeGraph === "boolean") | |
| payload.includeGraph = args.includeGraph; | |
| if (typeof args.order === "string") { | |
| const order = args.order.trim().toLowerCase(); | |
| if (order !== "asc" && order !== "desc") { | |
| return { status_code: 400, body: { error: "order must be 'asc' or 'desc'" } }; | |
| } | |
| payload.order = order; | |
| } | |
| const result = await 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/mcp/server.ts` around lines 285 - 299, The memory_lineage handler
currently parses channels with parseCsvList and forwards order directly; add
whitelist validation to fail fast with a 400: after channels =
parseCsvList(args.channels) validate each channel against an allowed set (e.g.,
a constant array or Set) and return a 400 error if any channel is not allowed,
and validate args.order to only accept "asc" or "desc" (otherwise return 400);
keep using asNumber for limit and sdk.trigger(...) for the call, and only add
the validated channels and normalized order to payload (use the same symbols:
parseCsvList, asNumber, sdk.trigger, and the memory_lineage handler) so invalid
inputs are rejected before triggering internal functions.
Three real issues caught in review: 1. firstMention computed from `trimmed` (post-limit page) instead of `items` (entire filtered set). When `order:desc` + a small `limit` truncated a session with many hits, the reported firstMention was the oldest-in-page, not the actual earliest filtered hit. Switch to `items` so the semantic contract holds regardless of page size. 2. MCP boundary (memory_lineage in src/mcp/server.ts) accepted any non-integer `limit` and any `order` string. Now: validate `limit` is a positive integer (400 otherwise), validate `order` is "asc"|"desc" (400 otherwise), filter `channels` to the known enum before forwarding. 3. REST boundary (api::lineage in src/triggers/api.ts) was forwarding raw `req.body` after validation, which leaks caller-controlled keys to the downstream function. Build a whitelisted payload from the validated fields only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
efenex
added a commit
to efenex/agentmemory
that referenced
this pull request
May 20, 2026
…stency - Tag previously-unlabelled fenced code blocks in docs/plans/v4-lineage-* with `text` to satisfy markdownlint MD040 (CodeRabbit comments on v4-lineage-design.md:126 and v4-lineage-test-case-careful-generator.md:19). - Normalize stale "51 tools" / "51-tool" / "(51 tools)" references in README.md to "55 tools" (CodeRabbit rohitg00#574 README.md:802). The headline count was already 55 in three spots after v5-a; the helper-paragraphs, the "### 51 Tools" heading, and the AGENTMEMORY_TOOLS comment lagged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
efenex
force-pushed
the
feat/v5-a-mem-query
branch
from
310385a to
4c4b22b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/triggers/api.ts (1)
1019-1027:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject unknown lineage channels at the REST boundary.
This only verifies that
channelsis astring[]; any string is then forwarded in the whitelisted payload. Invalid channel names should fail with 400 here instead of relying on downstream validation.As per coding guidelines: "Perform input validation at system boundaries (MCP handlers and REST endpoints)".Suggested patch
+ const allowedChannels = new Set([ + "observation", + "memory", + "lesson", + "summary", + ]); if ( body.channels !== undefined && (!Array.isArray(body.channels) || - !body.channels.every((c) => typeof c === "string")) + !body.channels.every( + (c) => typeof c === "string" && allowedChannels.has(c), + )) ) { return { status_code: 400, - body: { error: "channels must be an array of strings" }, + body: { + error: + "channels must only contain observation, memory, lesson, or summary", + }, }; }Also applies to: 1047-1047
🤖 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 1019 - 1027, The handler currently only checks that body.channels is a string[], but must also reject unknown lineage channel names; update the validation around body.channels to compare each string against the canonical allowed set (e.g., validLineageChannels or a CHANNEL_LINEAGE_VALUES constant) and return a 400 with an explanatory error listing the invalid entries if any are present; implement this using a Set lookup (or Array.prototype.every/includes) for efficiency and apply the same check at the other occurrence mentioned near the second body.channels check.src/mcp/tools-registry.ts (1)
1506-1516:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpose
memory_lineagein core tool listings.
getVisibleTools()defaults tocore, andmemory_lineageis still missing fromESSENTIAL_TOOLS, so default MCP clients will not discover it even though this cohort ships the handler/schema.Suggested patch
const ESSENTIAL_TOOLS = new Set([ "memory_save", "memory_recall", "memory_consolidate", "memory_smart_search", + "memory_lineage", "memory_sessions", "memory_diagnose", "memory_lesson_save", "memory_reflect", "memory_query",🤖 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/mcp/tools-registry.ts` around lines 1506 - 1516, The ESSENTIAL_TOOLS set used by getVisibleTools() (which defaults to the "core" cohort) is missing "memory_lineage", so add the string "memory_lineage" to the ESSENTIAL_TOOLS Set definition in src/mcp/tools-registry.ts (the constant named ESSENTIAL_TOOLS) so default MCP clients will discover that handler/schema; ensure no duplicates and run any existing tool-discovery tests that reference ESSENTIAL_TOOLS or getVisibleTools() to validate the change.
🤖 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 `@src/functions/query.ts`:
- Around line 853-864: The deadline guard currently only runs before steps and
does not abort long awaits like sdk.trigger() or provider.summarize(), letting
slow I/O exceed timeout; modify applyRankByRelevance (and the other affected
call sites sdk.trigger and provider.summarize around lines indicated) to race
their awaited operations against the ExecCtx deadline or pass an Abort/timeout
signal to the provider SDK so the call is cancelled when
ctx.timeoutMs/ctx.deadline elapses; ensure you increment ctx.llmCalls only after
a successful non-timed-out response and propagate/handle the timeout rejection
consistently.
- Around line 877-925: validatePipeline currently always uses DEFAULTS.maxDepth
and is called before data.options is read in registerQueryFunction, so callers'
options.maxDepth are ignored; change validatePipeline to accept an explicit
maxDepth (e.g., add a maxDepth number in the ctx or as a third parameter) and
use that instead of DEFAULTS.maxDepth, then update registerQueryFunction to
extract data.options.maxDepth (falling back to DEFAULTS.maxDepth) and pass it
into validatePipeline when validating the top-level pipeline (and likewise when
recursing into for_each calls via the ctx.depth). Ensure all internal recursive
calls to validatePipeline propagate the same maxDepth.
- Around line 1281-1336: The final return always reads streams.get("_") which
drops results when the pipeline's last step writes to a named stream; update the
end of the loop to return the stream written by the final pipeline step instead
of hardcoding "_". Use the pipeline variable to get the last step (e.g., const
finalOut = typeof pipeline[pipeline.length - 1].out === "string" ?
pipeline[pipeline.length - 1].out : "_") and return streams.get(finalOut) ?? []
in place of streams.get("_") so named outputs produced by executeStep/streams
are preserved.
In `@src/mcp/tools-registry.ts`:
- Around line 1028-1030: The schema entries for the recursive predicate/step
branches ('all', 'any', 'not', and 'for_each.do') are currently typed as
unstructured arrays/objects; update them to reference the appropriate
predicate/step schema so nested predicates and sub-steps preserve typing.
Replace the bare { type: "array" } / { type: "object" } shapes for 'all' and
'any' with items: { $ref: "`#/definitions/`<PredicateSchema>" } (or the actual
predicate definition name used in this file), make 'not' a single { $ref:
"`#/definitions/`<PredicateSchema>" }, and change 'for_each.do' to reference the
step/sub-step schema via $ref (or an array of that $ref if it should accept
multiple steps); apply the same change at the other occurrence noted (the
similar entries around the later block).
---
Outside diff comments:
In `@src/mcp/tools-registry.ts`:
- Around line 1506-1516: The ESSENTIAL_TOOLS set used by getVisibleTools()
(which defaults to the "core" cohort) is missing "memory_lineage", so add the
string "memory_lineage" to the ESSENTIAL_TOOLS Set definition in
src/mcp/tools-registry.ts (the constant named ESSENTIAL_TOOLS) so default MCP
clients will discover that handler/schema; ensure no duplicates and run any
existing tool-discovery tests that reference ESSENTIAL_TOOLS or
getVisibleTools() to validate the change.
In `@src/triggers/api.ts`:
- Around line 1019-1027: The handler currently only checks that body.channels is
a string[], but must also reject unknown lineage channel names; update the
validation around body.channels to compare each string against the canonical
allowed set (e.g., validLineageChannels or a CHANNEL_LINEAGE_VALUES constant)
and return a 400 with an explanatory error listing the invalid entries if any
are present; implement this using a Set lookup (or
Array.prototype.every/includes) for efficiency and apply the same check at the
other occurrence mentioned near the second body.channels check.
🪄 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: 85c4a55f-47ff-42e8-8d44-8cedb5d1b7c8
📒 Files selected for processing (13)
AGENTS.mdREADME.mddocs/plans/v4-lineage-design.mddocs/plans/v4-lineage-test-case-careful-generator.mdsrc/functions/lineage.tssrc/functions/query.tssrc/index.tssrc/mcp/server.tssrc/mcp/tools-registry.tssrc/triggers/api.tssrc/types.tstest/query-integration.test.tstest/query-transformers.test.ts
✅ Files skipped from review due to trivial changes (4)
- AGENTS.md
- docs/plans/v4-lineage-test-case-careful-generator.md
- docs/plans/v4-lineage-design.md
- README.md
Comment on lines
+1028
to
+1030
| { type: "object", properties: { all: { type: "array" } }, required: ["all"] }, | ||
| { type: "object", properties: { any: { type: "array" } }, required: ["any"] }, | ||
| { type: "object", properties: { not: {} }, required: ["not"] }, |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Make the recursive branches actually recursive.
all/any/not and for_each.do currently bottom out at untyped arrays/objects, so schema-aware MCP clients lose autocomplete/validation as soon as a query nests predicates or sub-steps. That undercuts the main discoverability win of memory_query.
Also applies to: 1332-1333
🤖 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/mcp/tools-registry.ts` around lines 1028 - 1030, The schema entries for
the recursive predicate/step branches ('all', 'any', 'not', and 'for_each.do')
are currently typed as unstructured arrays/objects; update them to reference the
appropriate predicate/step schema so nested predicates and sub-steps preserve
typing. Replace the bare { type: "array" } / { type: "object" } shapes for 'all'
and 'any' with items: { $ref: "`#/definitions/`<PredicateSchema>" } (or the actual
predicate definition name used in this file), make 'not' a single { $ref:
"`#/definitions/`<PredicateSchema>" }, and change 'for_each.do' to reference the
step/sub-step schema via $ref (or an array of that $ref if it should accept
multiple steps); apply the same change at the other occurrence noted (the
similar entries around the later block).
…+ firstMention tiebreak Two follow-up issues from CodeRabbit's review of 6a4de14: 1. `channels` silent broadening: when the user passed `channels` but none were in the known enum (e.g. `["foobar","baz"]`), the previous fix dropped to an empty `validChannels` and the conditional then omitted `payload.channels` entirely — falling back to all-channels default. Now: if the user explicitly passed channels but none are valid, return 400. Silently broadening invalidates caller intent. 2. `firstMention` could differ by `order`: picking `items[0]` (asc) or `items[items.length-1]` (desc) relied on the array's tiebreak rule to settle equal-timestamp ties. Two items sharing the earliest timestamp on different channels would resolve differently depending on `order`. Switch to an order-independent min-by-timestamp reduce so the "earliest in filtered set" contract is stable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Today every mem::* call is a discrete MCP round-trip. Multi-step
recall ("trace a decision, expand session context, summarize") is
N round-trips with manual context-passing, so the LLM falls back to
grep/find because shell composition is cheaper. mem::query collapses
the whole pipeline into one MCP call: ship a JSON pipeline, the
daemon executes it in-process (composing existing mem::* primitives
through sdk.trigger), returns the assembled result + per-step trace.
The point is NOT to add new retrieval primitives. It's to make
composition of the existing 51 primitives a first-class server-side
operation, so memory becomes the first reach for "what do I remember
about X" questions.
PIPELINE SURFACE:
- 12 producers (search/smart_search/lineage/lesson_recall/graph_query/
facet_query/insight_list/timeline/sessions/frontier/vision_search/
profile) — wrap existing tools via sdk.trigger
- 11 transformers (filter/sort/limit/take/drop/project/distinct/flatten/
concat/group_by/top_n_per_group) — pure JS over a normalized envelope
- 3 cross-step (for_each — synthesize/rank rejected inside / join /
expand_by_session)
- 2 aggregators (synthesize — terminal / rank_by_relevance — one LLM
call each)
- Named streams (in/out, default "_") for fork+join workflows
- Structured predicates with dot-paths: {field,op,value} + all/any/not
- Read-only by construction (writer ops fail validation)
GUARDS: budget (default 30, max 100), timeoutMs (default 10000, max
30000), maxStepOut (default 500), maxDepth (default 3, max 5),
dry_run (returns plan + estimatedCost without executing).
DISCOVERABILITY: MCP inputSchema declares the full discriminated union
(28 oneOf branches with per-op required fields and enums), description
inlines 3 literal example pipelines + the envelope kinds + the
dry_run-first workflow, so a fresh schema-aware LLM can author
pipelines without reading source.
WIRES:
- src/functions/query.ts (new — executor, per-producer envelope
mappers, predicate evaluator, dot-path resolver, dry-run, budget/
timeout/depth/writer-rejection guards)
- mem::query MCP tool in new V020_QUERY_TOOLS block + ESSENTIAL_TOOLS
- McpToolDef.inputSchema.properties type loosened to accept nested
JSON Schema (items/oneOf/anyOf/const/enum/...) so the discriminated
union expresses cleanly
- src/types.ts: EnvelopedKind, EnvelopedRecord, Predicate,
PipelineStep (discriminated union, 28 variants), QueryOptions,
QueryRequest, QueryResult, StepTrace, QueryCost
- 28 unit tests (query-transformers): predicate evaluator, dot-paths,
stable sort, project, distinct, flatten, group_by, top_n_per_group,
join
- 17 integration tests (query-integration): writer rejection, dry_run,
records/synthesis modes, budget_exceeded, named-stream join, group_by
+ top_n_per_group, for_each merge, rank_by_relevance, expand_by_session
from KV, out-defaults-to-_, sessions producer, trace shape
Total MCP tool count 52 → 53.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
test/consistency.test.ts only checks AGENTS.md for REST endpoint count (125, which I set in the v4-a commit when /agentmemory/lineage was added); it does NOT check AGENTS.md for MCP tool count. So when v5-a added memory_query (total tools 54 → 55), the AGENTS.md "Current Stats" block silently drifted out of sync. Bump to match the actual count across all V0xx_TOOLS arrays in src/mcp/tools-registry.ts (CORE_TOOLS 15 + V040 8 + V050 10 + V051 9 + V061 1 + V070 3 + V073 2 + V010_SLOTS 6 + V020_QUERY 1 = 55). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…stency - Tag previously-unlabelled fenced code blocks in docs/plans/v4-lineage-* with `text` to satisfy markdownlint MD040 (CodeRabbit comments on v4-lineage-design.md:126 and v4-lineage-test-case-careful-generator.md:19). - Normalize stale "51 tools" / "51-tool" / "(51 tools)" references in README.md to "55 tools" (CodeRabbit rohitg00#574 README.md:802). The headline count was already 55 in three spots after v5-a; the helper-paragraphs, the "### 51 Tools" heading, and the AGENTMEMORY_TOOLS comment lagged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
efenex
force-pushed
the
feat/v5-a-mem-query
branch
from
4c4b22b to
1f06dd3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
README.md (1)
427-427:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTool-count claims are still inconsistent across MCP sections.
Line 802/804/806 says 55 tools, but Line 449, Line 474, and Line 828 still reference 51. Please normalize these remaining references to the current count.
Suggested doc patch
-Install agentmemory for OpenClaw. ... available with all 51 memory tools: +Install agentmemory for OpenClaw. ... available with all 55 memory tools: -Install agentmemory for Hermes. ... with all 51 memory tools: +Install agentmemory for Hermes. ... with all 55 memory tools: -<summary>Extended tools (51 total — set AGENTMEMORY_TOOLS=all)</summary> +<summary>Extended tools (55 total — set AGENTMEMORY_TOOLS=all)</summary>As per coding guidelines: “When adding or removing MCP tools, update ...
README.md(tool counts).”Also applies to: 449-449, 474-474, 802-806, 828-828
🤖 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 `@README.md` at line 427, Update the inconsistent MCP tool-count mentions in README.md so all MCP sections report the current count (55); specifically, find the occurrences that reference the MCP server line with `@agentmemory/mcp` and the other MCP mentions that currently say 51 (the ones called out in the review) and change them to 55, ensuring any surrounding descriptive text remains grammatically correct and the numeric count is consistent across all MCP-related paragraphs and bullet points.src/mcp/server.ts (1)
285-301:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject partially invalid
channelslists.This still only returns 400 when none of the supplied tokens are valid. A payload like
"memory,typo"silently degrades to["memory"]instead of surfacing the caller error, which makes typos hard to detect at the MCP boundary.Suggested patch
- const validChannels = channels.filter((c) => - ["observation", "memory", "lesson", "summary"].includes(c), - ); - if (channels.length > 0 && validChannels.length === 0) { + const allowedChannels = new Set(["observation", "memory", "lesson", "summary"]); + const validChannels = channels.filter((c) => allowedChannels.has(c)); + if (validChannels.length !== channels.length) { return { status_code: 400, body: { - error: - "channels must contain at least one of: observation, memory, lesson, summary", + error: + "channels must be observation,memory,lesson,summary", }, }; }As per coding guidelines: "Perform input validation at system boundaries (MCP handlers, REST endpoints)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp/server.ts` around lines 285 - 301, The current validation only errors when none of the supplied channels are valid, allowing partially invalid lists like "memory,typo" to silently accept "memory"; update the handler using parseCsvList(args.channels) to reject any input where some tokens are invalid by comparing channels.length to validChannels.length and returning a 400 with an error listing the invalid tokens and the allowed set ("observation", "memory", "lesson", "summary"); adjust the response in the same block that currently builds the 400 body so callers see which entries were invalid.src/mcp/tools-registry.ts (1)
7-21:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRecursive branches still fall back to untyped schemas.
all/any/notandfor_each.dostill accept arbitrary arrays/objects here, becauseMcpPropertySchemacannot express references/definitions. That means nested predicates and sub-pipelines bypass MCP-layer validation/autocomplete and malformed recursive queries are only caught at execution time, which undercuts the typedmemory_querycontract.Also applies to: 1028-1030, 1332-1333
🤖 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/mcp/tools-registry.ts` around lines 7 - 21, McpPropertySchema currently can't represent schema references/definitions so recursive branches (e.g., nested predicates like all/any/not and pipeline nodes such as for_each.do) fall back to untyped objects; add a reusable reference schema type (e.g., RefSchema with a $ref: string) and update McpPropertySchema to permit either a full schema or a RefSchema wherever schemas are used: properties, items, oneOf/anyOf/allOf arrays, additionalProperties, and the root union for type. Ensure properties: Record<string, McpPropertySchema | RefSchema>, items: McpPropertySchema | RefSchema, oneOf/anyOf/allOf: (McpPropertySchema | RefSchema)[], additionalProperties: boolean | McpPropertySchema | RefSchema, and include an optional definitions?: Record<string, McpPropertySchema | RefSchema> field so referenced/recursive schemas are expressible and validated at the MCP layer.
🧹 Nitpick comments (3)
test/query-integration.test.ts (1)
22-61: ⚡ Quick winRefactor test harness to use the standardized
vi.mock("iii-sdk")pattern.This integration suite defines custom
mockKV()andmockSdk()functions instead of using the standardizedvi.mock("iii-sdk")pattern required by coding guidelines. Other test files in the repo (e.g.,test/multimodal.test.ts) already follow this pattern; aligning this file ensures consistency across the test suite and better maintainability.🤖 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/query-integration.test.ts` around lines 22 - 61, Replace the ad-hoc mockKV() and mockSdk() helpers with the repo-standard vi.mock("iii-sdk") pattern: create a vi.mock("iii-sdk", () => ({ KV: { get: ..., set: ..., delete: ..., list: ... }, SDK: { registerFunction: ..., registerTrigger: ..., trigger: ... } })) implementation that internally uses the same Map-based behavior currently in mockKV and mockSdk, keep the same method names (get, set, delete, list, registerFunction, registerTrigger, trigger) so existing tests continue to call them, and update the test file to remove mockKV()/mockSdk() definitions and rely on the mocked iii-sdk import instead.src/functions/lineage.ts (1)
439-446: 💤 Low valueAudit operation mismatch: uses
"query"formem::lineage.The audit entry uses
"query"as the operation, but this function ismem::lineage. TheAuditEntry.operationtype already includes"query"formem::query. Consider whether lineage should have its own operation type or if sharing"query"is intentional for all retrieval operations.If lineage should have its own audit operation, you'd need to add
"lineage"to theAuditEntry.operationunion insrc/types.ts.🤖 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/lineage.ts` around lines 439 - 446, The audit call in mem::lineage currently logs operation "query" but should use a distinct operation; update the safeAudit invocation inside the mem::lineage logic to use "lineage" instead of "query", and add "lineage" to the AuditEntry.operation union in src/types.ts so the type system accepts it (adjust any switch/consumers if they rely on operation values). Ensure you change the string in the call site where safeAudit(...) is invoked and add the new literal to the AuditEntry.operation type definition.src/functions/query.ts (1)
557-564: 💤 Low valueRename keeps original field (non-destructive).
The
renamelogic copies values to new keys but doesn't delete the original keys. If a user specifiesrename: { "title": "name" }, the output will have bothtitleandnamefields. This may be intentional for safety, but if destructive rename is ever needed, the current behavior would be surprising.🤖 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/query.ts` around lines 557 - 564, The rename loop currently copies values from paths in `rename` into new keys but leaves the original keys intact (using `resolveDotPath` and `out`), causing non-destructive renames; change the loop in the rename handling so that after assigning `out[to] = v` it also removes the original path `from` from `out` (i.e., perform a destructive rename). Locate the block that iterates `for (const [from, to] of Object.entries(rename))` in the function handling query output and, after the existing `out[to] = v` assignment, call or implement a delete/unset of the dot-path `from` on `out` (use existing utility if there is one or add a small helper to remove a nested key) so the original field is removed when a rename is performed.
🤖 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 `@README.md`:
- Line 1192: The README’s tool-count summary is inconsistent: the header line
showing 'core' = 8 tools conflicts with the "Core tools (always available)"
table which lists 11 tools (and the same mismatch appears around lines 813-823);
update the README so the numeric count for the 'core' visibility matches the
actual entries in the core-tools table (or update the table to match the
intended count), and ensure any mention of AGENTMEMORY_TOOLS or the "Tool
visibility" header reflects the corrected count consistently across the
document.
In `@src/mcp/server.ts`:
- Around line 356-357: The current guard in the MCP handler that assigns
payload.options from args.options (check around the args.options branch that
assigns payload.options) wrongly accepts arrays because typeof [] === "object";
update the validation to reject arrays by adding an explicit Array.isArray check
(e.g., ensure args.options is an object, not null, and not an array) before
assigning to payload.options so that only plain objects are allowed at the MCP
boundary.
---
Duplicate comments:
In `@README.md`:
- Line 427: Update the inconsistent MCP tool-count mentions in README.md so all
MCP sections report the current count (55); specifically, find the occurrences
that reference the MCP server line with `@agentmemory/mcp` and the other MCP
mentions that currently say 51 (the ones called out in the review) and change
them to 55, ensuring any surrounding descriptive text remains grammatically
correct and the numeric count is consistent across all MCP-related paragraphs
and bullet points.
In `@src/mcp/server.ts`:
- Around line 285-301: The current validation only errors when none of the
supplied channels are valid, allowing partially invalid lists like "memory,typo"
to silently accept "memory"; update the handler using
parseCsvList(args.channels) to reject any input where some tokens are invalid by
comparing channels.length to validChannels.length and returning a 400 with an
error listing the invalid tokens and the allowed set ("observation", "memory",
"lesson", "summary"); adjust the response in the same block that currently
builds the 400 body so callers see which entries were invalid.
In `@src/mcp/tools-registry.ts`:
- Around line 7-21: McpPropertySchema currently can't represent schema
references/definitions so recursive branches (e.g., nested predicates like
all/any/not and pipeline nodes such as for_each.do) fall back to untyped
objects; add a reusable reference schema type (e.g., RefSchema with a $ref:
string) and update McpPropertySchema to permit either a full schema or a
RefSchema wherever schemas are used: properties, items, oneOf/anyOf/allOf
arrays, additionalProperties, and the root union for type. Ensure properties:
Record<string, McpPropertySchema | RefSchema>, items: McpPropertySchema |
RefSchema, oneOf/anyOf/allOf: (McpPropertySchema | RefSchema)[],
additionalProperties: boolean | McpPropertySchema | RefSchema, and include an
optional definitions?: Record<string, McpPropertySchema | RefSchema> field so
referenced/recursive schemas are expressible and validated at the MCP layer.
---
Nitpick comments:
In `@src/functions/lineage.ts`:
- Around line 439-446: The audit call in mem::lineage currently logs operation
"query" but should use a distinct operation; update the safeAudit invocation
inside the mem::lineage logic to use "lineage" instead of "query", and add
"lineage" to the AuditEntry.operation union in src/types.ts so the type system
accepts it (adjust any switch/consumers if they rely on operation values).
Ensure you change the string in the call site where safeAudit(...) is invoked
and add the new literal to the AuditEntry.operation type definition.
In `@src/functions/query.ts`:
- Around line 557-564: The rename loop currently copies values from paths in
`rename` into new keys but leaves the original keys intact (using
`resolveDotPath` and `out`), causing non-destructive renames; change the loop in
the rename handling so that after assigning `out[to] = v` it also removes the
original path `from` from `out` (i.e., perform a destructive rename). Locate the
block that iterates `for (const [from, to] of Object.entries(rename))` in the
function handling query output and, after the existing `out[to] = v` assignment,
call or implement a delete/unset of the dot-path `from` on `out` (use existing
utility if there is one or add a small helper to remove a nested key) so the
original field is removed when a rename is performed.
In `@test/query-integration.test.ts`:
- Around line 22-61: Replace the ad-hoc mockKV() and mockSdk() helpers with the
repo-standard vi.mock("iii-sdk") pattern: create a vi.mock("iii-sdk", () => ({
KV: { get: ..., set: ..., delete: ..., list: ... }, SDK: { registerFunction:
..., registerTrigger: ..., trigger: ... } })) implementation that internally
uses the same Map-based behavior currently in mockKV and mockSdk, keep the same
method names (get, set, delete, list, registerFunction, registerTrigger,
trigger) so existing tests continue to call them, and update the test file to
remove mockKV()/mockSdk() definitions and rely on the mocked iii-sdk import
instead.
🪄 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: 0460a491-1a64-4818-830b-b19dfc5201af
📒 Files selected for processing (12)
AGENTS.mdREADME.mddocs/plans/v4-lineage-design.mddocs/plans/v4-lineage-test-case-careful-generator.mdsrc/functions/lineage.tssrc/functions/query.tssrc/index.tssrc/mcp/server.tssrc/mcp/tools-registry.tssrc/types.tstest/query-integration.test.tstest/query-transformers.test.ts
✅ Files skipped from review due to trivial changes (3)
- AGENTS.md
- docs/plans/v4-lineage-test-case-careful-generator.md
- docs/plans/v4-lineage-design.md
| # TEAM_MODE=private | ||
|
|
||
| # Tool visibility: "core" (8 tools) or "all" (51 tools) | ||
| # Tool visibility: "core" (8 tools) or "all" (55 tools) |
Contributor
There was a problem hiding this comment.
core tool-count text conflicts with the core-tools table.
Line 1192 says core = 8 tools, but the “Core tools (always available)” table lists 11 tools. Please align one of these so users don’t misconfigure AGENTMEMORY_TOOLS.
As per coding guidelines: “When adding or removing MCP tools, update ... README.md (tool counts).”
Also applies to: 813-823
🤖 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 `@README.md` at line 1192, The README’s tool-count summary is inconsistent: the
header line showing 'core' = 8 tools conflicts with the "Core tools (always
available)" table which lists 11 tools (and the same mismatch appears around
lines 813-823); update the README so the numeric count for the 'core' visibility
matches the actual entries in the core-tools table (or update the table to match
the intended count), and ensure any mention of AGENTMEMORY_TOOLS or the "Tool
visibility" header reflects the corrected count consistently across the
document.
…guard Two nits from CodeRabbit's second pass on rohitg00#574: 1. README:1192 said `core` = 8 tools. ESSENTIAL_TOOLS now contains 9 entries after this PR adds memory_query. Bump 8 → 9. Also bump "Extended tools (51 total)" → 55 to match the headline (51 → 55 was missed when I normalized the other README spots earlier). Note: the "Core tools (always available)" table at README:809 lists 11 entries vs ESSENTIAL_TOOLS' 9 — that's pre-existing doc drift unrelated to this PR. Leaving for a separate cleanup. 2. src/mcp/server.ts memory_query options guard accepted arrays because `typeof [] === "object"`. Reject arrays explicitly so a malformed `options: []` is caught at the MCP boundary with 400 instead of relying on downstream validation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_run + last stream Four issues from CodeRabbit's rohitg00#574 review of v5-a, addressed together since they touch the same file and share testing surface. 1. `timeoutMs` only enforced between steps (deadline check ran before each step but not during awaited I/O). A single slow `sdk.trigger()` producer or `provider.summarize()` LLM call could hang well past the user's timeout. Added `withDeadline()` helper that races the awaited promise against `ctx.deadlineAt`; wrapped the three unbounded await sites: - `runProducer`'s `sdk.trigger` - `synthesize`'s `provider.summarize` - `rank_by_relevance`'s `provider.summarize` The race rejects with `QueryRuntimeError("deadline_exceeded during <label>")` which surfaces as `{kind:"error"}` through the existing error pathway. 2. `validatePipeline()` hard-coded `DEFAULTS.maxDepth` for the for_each-nesting check, so a user setting `options.maxDepth=5` on a 4-deep pipeline still got rejected. The validator now accepts `maxDepth` through its ctx parameter; the registration handler computes `earlyMaxDepth` from options BEFORE calling validate, then threads it into both the top-level call and the recursive descent. 3. `dry_run` cost estimate ignored `for_each.do` contents — a heavy inner pipeline reported the same cost as an empty one, making the planning output deceptive. `estimatePipelineCost()` is now recursive: top-level cost plus `for_each` inner cost as a range (min: 1 iteration, max: maxStepOut iterations). The min/max gap reflects that we don't know the input cardinality at plan time. 4. Records pipelines always returned `streams.get("_") ?? []`, so a pipeline whose final step writes `out: "foo"` got an empty result back. Now the executor tracks the last step's `outputName` and returns that stream. Default-flow pipelines (no explicit `out`) are unchanged because the last step's `outputName` defaults to "_". Not addressed (heavy lift, separate PR if pursued): - Recursive predicate / for_each.do schemas in tools-registry.ts (CodeRabbit flagged that `all`/`any`/`not` and `for_each.do` currently bottom out at untyped arrays, losing schema-aware autocomplete past one level of nesting). Doable with JSON Schema `$ref` but a meaningful design surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Motivation
Today every `mem::*` MCP call is a discrete round-trip. Multi-step recall ("trace a decision, expand session context, summarize") is N round-trips with manual context-passing — by the time Claude has serialized intermediate JSON between calls, it has burned more tokens than the synthesis itself. So when Claude wants to recall something, it reaches for `grep` first because shell composition is cheap.
`mem::query` collapses N round-trips into one MCP call: ship a JSON pipeline, the daemon executes it in-process (composing existing `mem::*` primitives via `sdk.trigger`), returns the assembled result + per-step trace. The point is not to add new retrieval primitives — it is to make composition of the existing 53 primitives a first-class server-side operation, so memory becomes the first reach for "what do I remember about X" questions.
Surface (28-variant discriminated-union, validated at MCP layer)
Plus: named streams (`in`/`out`, default `_`) for fork+join, structured predicates with dot-paths (`{field, op, value}` + `all`/`any`/`not`), normalized record envelope (`_kind`, `_id`, `_sessionId`, `_project`, `_createdAt`, `_score`, ...).
Safety
Discoverability
The MCP `inputSchema` is a proper discriminated union (28 `oneOf` branches with per-op `required` fields and enums). The tool description inlines 3 literal example pipelines, enumerates the legal `_kind` values, and documents the `dry_run`-first workflow. Schema-aware tool-use LLMs (Claude, GPT, Gemini) auto-narrow per-op fields — no guesswork.
Worked example
```json
{
"pipeline": [
{ "op": "lineage", "out": "ctx", "query": "X", "since": "2026-05-12T00:00:00Z", "limit": 100 },
{ "op": "lesson_recall", "out": "lessons", "query": "X", "limit": 30 },
{ "op": "join", "in": "ctx", "right": "lessons", "on": { "left": "_sessionId", "right": "_sessionId" }, "type": "left" },
{ "op": "rank_by_relevance", "target": "explain X using recent activity and lessons", "topK": 12 },
{ "op": "synthesize", "question": "Explain X in the last 7 days.", "style": "bullets", "maxCitations": 10 }
],
"options": { "budget": 50, "timeoutMs": 20000 }
}
```
That's 5 ops, 1 LLM call (the terminal `synthesize`), 1 round-trip. Without `mem::query`, the same workflow would be 4 MCP calls + token-burning JSON glue between them.
Tests
Live validation
I have this running against my own agentmemory daemon. Two real lookups, both completed in one round-trip:
Why this might be controversial
Happy to scope down if any of these are blockers. The walking-skeleton version (just `search/lineage/lesson_recall` producers + `filter/sort/limit` transformers + terminal `synthesize`) is recoverable as a smaller PR if you'd prefer to ship that first and grow later.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores