fix(mcp): memory_recall hits the right endpoint and forwards format/token_budget (#507)#516
Conversation
…at/token_budget memory_recall and memory_smart_search were sharing the smart-search endpoint, which always returns compact mode and silently drops the format and token_budget parameters that the tool schema advertises. Split the cases so memory_recall hits /agentmemory/search (which honors format) while memory_smart_search keeps its own endpoint. Default format to "full" for memory_recall so the documented behavior matches the wire call. Signed-off-by: serhiizghama <zmrser@gmail.com>
Two new proxy tests for issue rohitg00#507: one asserts memory_recall calls POST /agentmemory/search with the format and token_budget fields, and never falls through to smart-search; the other pins the default format to "full" when the caller omits it. Signed-off-by: serhiizghama <zmrser@gmail.com>
|
@serhiizghama 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 fixes a bug where Changesmemory_recall proxy fix with format and token_budget support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/mcp/standalone.ts (2)
123-133: ⚡ Quick winConsider validating format against known values.
The format parameter is normalized to lowercase but not validated against expected values (likely "full" or "compact"). Invalid format strings will be forwarded to the server, potentially causing less clear error messages.
🛡️ Proposed validation enhancement
const fmt = args["format"]; if (typeof fmt === "string" && fmt.trim()) { - v.format = fmt.trim().toLowerCase(); + const normalized = fmt.trim().toLowerCase(); + if (normalized === "full" || normalized === "compact") { + v.format = normalized; + } else { + throw new Error(`format must be "full" or "compact", got "${fmt}"`); + } }🤖 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/standalone.ts` around lines 123 - 133, Normalize and validate args["format"] before assigning to v.format: accept only known values (e.g., "full" and "compact") after trimming and lowercasing; if the provided value is not one of the allowed options, either set a sensible default (e.g., "full") or reject it by not setting v.format and returning/logging a clear error. Update the block handling fmt (the variable and v.format assignment) to perform this whitelist check so invalid format strings are not forwarded to the server.
254-276: Local fallback doesn't honor format parameter (by design).For awareness: The local InMemoryKV fallback always returns
mode: "compact"regardless of theformatparameter. This is mentioned in the PR description as "Local InMemoryKV fallback unchanged." The proxy mode (primary use case) correctly honors the format parameter. Implementing "full" mode locally would require restructuring how the in-memory store tracks narrative, concepts, and files.🤖 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/standalone.ts` around lines 254 - 276, The local InMemoryKV fallback currently hardcodes mode: "compact" in the memory_recall/memory_smart_search handler; change it to honor the incoming format parameter by reading v.format and setting a mode variable (e.g., const mode = v.format === "full" ? "full" : "compact"), then pass that mode into textResponse({ mode, results }, true); if you cannot produce a true "full" payload from the in-memory records, log or warn that "full" is not supported locally and still return the compact-shaped results but with mode reflecting the requested format behavior; update the memory_recall/memory_smart_search case (variables: query, limit, kvInstance.list, results, textResponse) accordingly.test/mcp-standalone-proxy.test.ts (1)
78-131: ⚡ Quick winConsider adding test coverage for memory_smart_search format/token_budget forwarding.
While the PR focuses on fixing memory_recall, the changes also enable memory_smart_search to forward format and token_budget parameters. The existing test on lines 56-76 doesn't exercise these new parameters. Adding a test would provide confidence that the enhancement works correctly for both tools.
📋 Example test structure
it("memory_smart_search forwards format and token_budget when provided", async () => { let capturedBody: Record<string, unknown> | undefined; installFetch((url, init) => { if (url.endsWith("/agentmemory/livez")) return new Response("ok", { status: 200 }); if (url.endsWith("/agentmemory/smart-search")) { capturedBody = init?.body ? JSON.parse(init.body as string) : undefined; return new Response(JSON.stringify({ mode: "full", results: [] }), { status: 200 }); } return new Response("", { status: 404 }); }); await handleToolCall("memory_smart_search", { query: "test", limit: 10, format: "full", token_budget: 500, }); expect(capturedBody).toEqual({ query: "test", limit: 10, format: "full", token_budget: 500, }); });🤖 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/mcp-standalone-proxy.test.ts` around lines 78 - 131, Add a new unit test to verify memory_smart_search forwards format and token_budget to the agentmemory smart-search endpoint: use installFetch to stub requests (return ok for "/agentmemory/livez" and capture request body for "/agentmemory/smart-search"), call handleToolCall("memory_smart_search", { query, limit, format, token_budget }), and assert the captured body equals the sent payload (query, limit, format, token_budget) and that the response path is "/agentmemory/smart-search"; mirror the pattern used in the existing memory_recall tests and name the captured variable (e.g., capturedBody) so assertions can access it.
🤖 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.
Nitpick comments:
In `@src/mcp/standalone.ts`:
- Around line 123-133: Normalize and validate args["format"] before assigning to
v.format: accept only known values (e.g., "full" and "compact") after trimming
and lowercasing; if the provided value is not one of the allowed options, either
set a sensible default (e.g., "full") or reject it by not setting v.format and
returning/logging a clear error. Update the block handling fmt (the variable and
v.format assignment) to perform this whitelist check so invalid format strings
are not forwarded to the server.
- Around line 254-276: The local InMemoryKV fallback currently hardcodes mode:
"compact" in the memory_recall/memory_smart_search handler; change it to honor
the incoming format parameter by reading v.format and setting a mode variable
(e.g., const mode = v.format === "full" ? "full" : "compact"), then pass that
mode into textResponse({ mode, results }, true); if you cannot produce a true
"full" payload from the in-memory records, log or warn that "full" is not
supported locally and still return the compact-shaped results but with mode
reflecting the requested format behavior; update the
memory_recall/memory_smart_search case (variables: query, limit,
kvInstance.list, results, textResponse) accordingly.
In `@test/mcp-standalone-proxy.test.ts`:
- Around line 78-131: Add a new unit test to verify memory_smart_search forwards
format and token_budget to the agentmemory smart-search endpoint: use
installFetch to stub requests (return ok for "/agentmemory/livez" and capture
request body for "/agentmemory/smart-search"), call
handleToolCall("memory_smart_search", { query, limit, format, token_budget }),
and assert the captured body equals the sent payload (query, limit, format,
token_budget) and that the response path is "/agentmemory/smart-search"; mirror
the pattern used in the existing memory_recall tests and name the captured
variable (e.g., capturedBody) so assertions can access it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0444cbf-a79f-4a21-a603-ed4b43ba81ca
📒 Files selected for processing (2)
src/mcp/standalone.tstest/mcp-standalone-proxy.test.ts
|
Merged + shipping in next release. Thanks @serhiizghama — closes #507 and #440 (memory_recall returning compact mode + dropping format/token_budget). Both root causes addressed in a single PR: routes through /agentmemory/search instead of /smart-search, forwards format + token_budget end-to-end, defaults format to 'full' matching the schema. |
…oken_budget (rohitg00#507) (rohitg00#516) * fix(mcp): route memory_recall to /agentmemory/search and forward format/token_budget memory_recall and memory_smart_search were sharing the smart-search endpoint, which always returns compact mode and silently drops the format and token_budget parameters that the tool schema advertises. Split the cases so memory_recall hits /agentmemory/search (which honors format) while memory_smart_search keeps its own endpoint. Default format to "full" for memory_recall so the documented behavior matches the wire call. Signed-off-by: serhiizghama <zmrser@gmail.com> * test(mcp): cover memory_recall endpoint, format forwarding, and defaults Two new proxy tests for issue rohitg00#507: one asserts memory_recall calls POST /agentmemory/search with the format and token_budget fields, and never falls through to smart-search; the other pins the default format to "full" when the caller omits it. Signed-off-by: serhiizghama <zmrser@gmail.com> --------- Signed-off-by: serhiizghama <zmrser@gmail.com>
Quality + integration wave. Bundles 11 PRs since v0.9.20: Contributor feature: - #237 OpenCode plugin with 22 auto-capture hooks (@cl0ckt0wer) Bug fixes (9): - #516 memory_recall endpoint + format/token_budget (@serhiizghama, closes #507/#440) - #461 env-file AGENTMEMORY_DROP_STALE_INDEX flag honored (@honor2030, closes #456) - #487 Windows hook path quoting (@honor2030, closes #477) - #517 viewer IME composition guard (@jonathanzhan1975) - #472 chunk large sessions for LLM context window (@efenex) - #473 surface lessons in smart-search + diagnose tally (@efenex) - #486 declare all Hermes plugin hooks (@honor2030) - #500 rebuildIndex non-blocking on boot (@efenex) - #504 batched embed in rebuildIndex (25h -> 3h) (@efenex) - #491 cli skip onboarding without tty (@honor2030) Upstream-installer revert: - #546 drop --next workaround now that iii-hq/iii#1660 shipped 1067/1067 tests pass across 95 files.
Problem
memory_recallin the standalone MCP shim always returns compact results, ignoring theformatandtoken_budgetparameters that the tool's input schema advertises (src/mcp/tools-registry.ts).Two root causes in
src/mcp/standalone.ts:memory_recallandmemory_smart_searchshare a singlecasethat routes both toPOST /agentmemory/smart-search. That endpoint always returns compact mode and does not honorformat. The endpoint that supportsformat(and exposes the fullfacts/narrative/concepts/filesshape) isPOST /agentmemory/search.validate()never parsesformatortoken_budget, andhandleProxy()never includes them in the request body, so even after the endpoint is corrected the server has nothing to act on.Fixes #507.
Solution
format(string, lower-cased) andtoken_budget(positive integer, accepts string or number) invalidate()for bothmemory_recallandmemory_smart_search.memory_recallcallsPOST /agentmemory/searchwithformatdefaulted to"full"(matching the schema's documented default) and forwardstoken_budgetwhen set.memory_smart_searchkeeps its existingPOST /agentmemory/smart-searchendpoint but now also forwardsformatandtoken_budgetwhen the caller supplies them.InMemoryKVfallback behavior is unchanged.Testing
npm test— 1036 / 1036 passing locally.Two new tests in
test/mcp-standalone-proxy.test.ts:proxies memory_recall to POST /agentmemory/search and forwards format/token_budget (#507)— asserts the request URL, the body shape ({query, limit, format, token_budget}), and thatsmart-searchis not called.memory_recall defaults format to 'full' when omitted (#507)— pins the default so the wire call matches the schema's documented default.Summary by CodeRabbit
New Features
formatandtoken_budgetoptional parameters for customized query behavior"full"format when format parameter is unspecifiedTests