fix(mcp): preserve recall format option#489
Closed
honor2030 wants to merge 1 commit into
Closed
Conversation
|
@honor2030 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 support for an optional ChangesFormat Parameter Support for Memory Search
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/standalone.ts (1)
241-246:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecall local fallback default should not be hardcoded to compact.
Line 245 defaults both
memory_recallandmemory_smart_searchto"compact". For recall semantics, defaulting to full content is expected.Suggested fix
- const format = v.format ?? "compact"; + const format = + v.format ?? (v.tool === "memory_recall" ? "full" : "compact");🤖 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 241 - 246, The default format is incorrectly hardcoded to "compact" for both "memory_recall" and "memory_smart_search"; update the logic in the switch branch so that when handling the "memory_recall" case you default format to "full" (e.g., set format = v.format ?? "full") while preserving "compact" as the default for "memory_smart_search" (keep format = v.format ?? "compact" for that case); locate and modify the code that assigns the variable format inside the case block that handles the "memory_recall" and "memory_smart_search" branches so it uses the operation type to choose the appropriate default.
🤖 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/mcp/standalone.ts`:
- Around line 67-80: The parseSearchFormat function currently returns undefined
for invalid inputs and doesn't accept the "narrative" format; update the
SearchFormat union to include "narrative" (or create a separate
RecallSearchFormat if domain-specific) and change parseSearchFormat to validate
strictly at the boundary by rejecting invalid values (e.g., throw an Error or
return a Result/Error object) instead of returning undefined; keep the existing
trimming/lowercasing and normalize "compact", "full", and the new "narrative"
values, and ensure callers of parseSearchFormat (search code paths that
currently treat undefined as “no-op”) are updated to handle/reject the parse
error accordingly (reference: SearchFormat type and parseSearchFormat function).
- Around line 172-181: The switch branch handling "memory_recall" incorrectly
calls the smart-search endpoint; update the "memory_recall" case to call
"/agentmemory/search" (leave "memory_smart_search" calling
"/agentmemory/smart-search"), i.e., change the handle.call target in the
"memory_recall" branch from "/agentmemory/smart-search" to "/agentmemory/search"
while keeping the same body construction (query, limit, optional format) and
response handling around result to preserve behavior.
In `@test/mcp-standalone.test.ts`:
- Around line 241-269: The test currently only validates request bodies but not
endpoints, so add assertions on the first argument of each proxyFetch call to
ensure routing is correct; after calling handleToolCall for "memory_recall" and
"memory_smart_search" (functions referenced) inspect proxyFetch.mock.calls and
assert the URL/path (first call equals "/agentmemory/search" or full expected
URL for memory_recall and second call equals "/agentmemory/smart-search" for
memory_smart_search) before checking the RequestInit body, so the test fails if
the tools are routed to the wrong proxy endpoint.
---
Outside diff comments:
In `@src/mcp/standalone.ts`:
- Around line 241-246: The default format is incorrectly hardcoded to "compact"
for both "memory_recall" and "memory_smart_search"; update the logic in the
switch branch so that when handling the "memory_recall" case you default format
to "full" (e.g., set format = v.format ?? "full") while preserving "compact" as
the default for "memory_smart_search" (keep format = v.format ?? "compact" for
that case); locate and modify the code that assigns the variable format inside
the case block that handles the "memory_recall" and "memory_smart_search"
branches so it uses the operation type to choose the appropriate default.
🪄 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: e0d3f760-2dc5-44bb-98fd-adcf6697906d
📒 Files selected for processing (2)
src/mcp/standalone.tstest/mcp-standalone.test.ts
Comment on lines
+67
to
+80
| type SearchFormat = "compact" | "full"; | ||
|
|
||
| function parseLimit(raw: unknown, fallback = DEFAULT_LIMIT): number { | ||
| if (typeof raw !== "number" && typeof raw !== "string") return fallback; | ||
| const n = Number(raw); | ||
| if (!Number.isFinite(n) || n <= 0) return fallback; | ||
| return Math.min(Math.floor(n), MAX_LIMIT); | ||
| } | ||
|
|
||
| function parseSearchFormat(raw: unknown): SearchFormat | undefined { | ||
| if (typeof raw !== "string") return undefined; | ||
| const format = raw.trim().toLowerCase(); | ||
| return format === "compact" || format === "full" ? format : undefined; | ||
| } |
There was a problem hiding this comment.
Enforce format at the boundary instead of silently dropping invalid values.
Line 130 currently accepts invalid format inputs by mapping them to undefined, which bypasses rejection. Also, the parser currently cannot represent "narrative" for recall requests.
Suggested fix
-type SearchFormat = "compact" | "full";
+type SearchFormat = "compact" | "full" | "narrative";
function parseSearchFormat(raw: unknown): SearchFormat | undefined {
- if (typeof raw !== "string") return undefined;
+ if (raw === undefined) return undefined;
+ if (typeof raw !== "string") return undefined;
const format = raw.trim().toLowerCase();
- return format === "compact" || format === "full" ? format : undefined;
+ return format === "compact" || format === "full" || format === "narrative"
+ ? format
+ : undefined;
}
@@
- v.format = parseSearchFormat(args["format"]);
+ v.format = parseSearchFormat(args["format"]);
+ if (args["format"] !== undefined && !v.format) {
+ throw new Error('format must be one of: "full", "compact", "narrative"');
+ }Also applies to: 130-131
🤖 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 67 - 80, The parseSearchFormat function
currently returns undefined for invalid inputs and doesn't accept the
"narrative" format; update the SearchFormat union to include "narrative" (or
create a separate RecallSearchFormat if domain-specific) and change
parseSearchFormat to validate strictly at the boundary by rejecting invalid
values (e.g., throw an Error or return a Result/Error object) instead of
returning undefined; keep the existing trimming/lowercasing and normalize
"compact", "full", and the new "narrative" values, and ensure callers of
parseSearchFormat (search code paths that currently treat undefined as “no-op”)
are updated to handle/reject the parse error accordingly (reference:
SearchFormat type and parseSearchFormat function).
Comment on lines
172
to
+181
| case "memory_recall": | ||
| case "memory_smart_search": { | ||
| const body: { query?: string; limit?: number; format?: SearchFormat } = { | ||
| query: v.query, | ||
| limit: v.limit, | ||
| }; | ||
| if (v.format) body.format = v.format; | ||
| const result = await handle.call("/agentmemory/smart-search", { | ||
| method: "POST", | ||
| body: JSON.stringify({ query: v.query, limit: v.limit }), | ||
| body: JSON.stringify(body), |
There was a problem hiding this comment.
memory_recall is still proxied to the smart-search endpoint.
Line 179 routes both tools to /agentmemory/smart-search; this keeps recall on compact-search semantics and prevents dedicated recall behavior from /agentmemory/search.
Suggested fix
- case "memory_recall":
- case "memory_smart_search": {
+ case "memory_recall":
+ case "memory_smart_search": {
+ const endpoint =
+ v.tool === "memory_recall"
+ ? "/agentmemory/search"
+ : "/agentmemory/smart-search";
const body: { query?: string; limit?: number; format?: SearchFormat } = {
query: v.query,
limit: v.limit,
};
if (v.format) body.format = v.format;
- const result = await handle.call("/agentmemory/smart-search", {
+ const result = await handle.call(endpoint, {
method: "POST",
body: JSON.stringify(body),
});🤖 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 172 - 181, The switch branch handling
"memory_recall" incorrectly calls the smart-search endpoint; update the
"memory_recall" case to call "/agentmemory/search" (leave "memory_smart_search"
calling "/agentmemory/smart-search"), i.e., change the handle.call target in the
"memory_recall" branch from "/agentmemory/smart-search" to "/agentmemory/search"
while keeping the same body construction (query, limit, optional format) and
response handling around result to preserve behavior.
Comment on lines
+241
to
+269
| it("memory_recall and memory_smart_search forward safe format to smart-search proxy (#440)", async () => { | ||
| const proxyProbe = vi.fn(async () => ({ ok: true, status: 200 })); | ||
| setLivezProbe(proxyProbe); | ||
|
|
||
| const proxyFetch = vi.fn(async () => | ||
| new Response(JSON.stringify({ results: [] }), { | ||
| status: 200, | ||
| headers: { "content-type": "application/json" }, | ||
| }), | ||
| ); | ||
| (globalThis as { fetch: typeof fetch }).fetch = proxyFetch as unknown as typeof fetch; | ||
|
|
||
| for (const toolName of ["memory_recall", "memory_smart_search"]) { | ||
| resetHandleForTests(); | ||
| setLivezProbe(proxyProbe); | ||
| await handleToolCall( | ||
| toolName, | ||
| { query: "typescript", limit: 3, format: "full" }, | ||
| new InMemoryKV(), | ||
| ); | ||
| } | ||
|
|
||
| expect(proxyFetch).toHaveBeenCalledTimes(2); | ||
| for (const [, init] of proxyFetch.mock.calls) { | ||
| expect((init as RequestInit).method).toBe("POST"); | ||
| const body = JSON.parse((init as RequestInit).body as string); | ||
| expect(body).toEqual({ query: "typescript", limit: 3, format: "full" }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Add endpoint assertions so this test catches recall routing regressions.
This test verifies request body only; it won’t fail if memory_recall is sent to the wrong endpoint. Assert URL/path per call (/agentmemory/search vs /agentmemory/smart-search) to lock the contract.
Suggested assertion addition
expect(proxyFetch).toHaveBeenCalledTimes(2);
+ expect(String(proxyFetch.mock.calls[0][0])).toContain("/agentmemory/search");
+ expect(String(proxyFetch.mock.calls[1][0])).toContain("/agentmemory/smart-search");
for (const [, init] of proxyFetch.mock.calls) {
expect((init as RequestInit).method).toBe("POST");
const body = JSON.parse((init as RequestInit).body as string);
expect(body).toEqual({ query: "typescript", limit: 3, format: "full" });
}📝 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
| it("memory_recall and memory_smart_search forward safe format to smart-search proxy (#440)", async () => { | |
| const proxyProbe = vi.fn(async () => ({ ok: true, status: 200 })); | |
| setLivezProbe(proxyProbe); | |
| const proxyFetch = vi.fn(async () => | |
| new Response(JSON.stringify({ results: [] }), { | |
| status: 200, | |
| headers: { "content-type": "application/json" }, | |
| }), | |
| ); | |
| (globalThis as { fetch: typeof fetch }).fetch = proxyFetch as unknown as typeof fetch; | |
| for (const toolName of ["memory_recall", "memory_smart_search"]) { | |
| resetHandleForTests(); | |
| setLivezProbe(proxyProbe); | |
| await handleToolCall( | |
| toolName, | |
| { query: "typescript", limit: 3, format: "full" }, | |
| new InMemoryKV(), | |
| ); | |
| } | |
| expect(proxyFetch).toHaveBeenCalledTimes(2); | |
| for (const [, init] of proxyFetch.mock.calls) { | |
| expect((init as RequestInit).method).toBe("POST"); | |
| const body = JSON.parse((init as RequestInit).body as string); | |
| expect(body).toEqual({ query: "typescript", limit: 3, format: "full" }); | |
| } | |
| }); | |
| it("memory_recall and memory_smart_search forward safe format to smart-search proxy (`#440`)", async () => { | |
| const proxyProbe = vi.fn(async () => ({ ok: true, status: 200 })); | |
| setLivezProbe(proxyProbe); | |
| const proxyFetch = vi.fn(async () => | |
| new Response(JSON.stringify({ results: [] }), { | |
| status: 200, | |
| headers: { "content-type": "application/json" }, | |
| }), | |
| ); | |
| (globalThis as { fetch: typeof fetch }).fetch = proxyFetch as unknown as typeof fetch; | |
| for (const toolName of ["memory_recall", "memory_smart_search"]) { | |
| resetHandleForTests(); | |
| setLivezProbe(proxyProbe); | |
| await handleToolCall( | |
| toolName, | |
| { query: "typescript", limit: 3, format: "full" }, | |
| new InMemoryKV(), | |
| ); | |
| } | |
| expect(proxyFetch).toHaveBeenCalledTimes(2); | |
| expect(String(proxyFetch.mock.calls[0][0])).toContain("/agentmemory/search"); | |
| expect(String(proxyFetch.mock.calls[1][0])).toContain("/agentmemory/smart-search"); | |
| for (const [, init] of proxyFetch.mock.calls) { | |
| expect((init as RequestInit).method).toBe("POST"); | |
| const body = JSON.parse((init as RequestInit).body as string); | |
| expect(body).toEqual({ query: "typescript", limit: 3, format: "full" }); | |
| } | |
| }); |
🤖 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.test.ts` around lines 241 - 269, The test currently only
validates request bodies but not endpoints, so add assertions on the first
argument of each proxyFetch call to ensure routing is correct; after calling
handleToolCall for "memory_recall" and "memory_smart_search" (functions
referenced) inspect proxyFetch.mock.calls and assert the URL/path (first call
equals "/agentmemory/search" or full expected URL for memory_recall and second
call equals "/agentmemory/smart-search" for memory_smart_search) before checking
the RequestInit body, so the test fails if the tools are routed to the wrong
proxy endpoint.
Owner
|
Closed as superseded by #516 (merged today), which is the more complete fix:
Both PRs target the same file + functions, so they're mutually exclusive. Thanks @honor2030 for surfacing #440 — leaving it open as a comment trail in case anyone else hits the issue and lands here from search. |
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.
Summary
formatargument when the standalone MCP shim validatesmemory_recall/memory_smart_searchcalls.Test Plan
npm test -- test/mcp-standalone.test.tsfailed before the implementation whenformatwas omitted from the proxied body.npm test→ 91 files / 1009 tests passed.git diff --checkCloses #440
Summary by CodeRabbit
Release Notes