fix: parse JSON-string MCP tool arguments before type check#255
Conversation
Some LLMs (DeepSeek V4 Pro, others) emit MCP tool call arguments as
JSON-encoded strings (e.g. '{"headless": true}') rather than as
native objects. This causes validateParams() to reject valid MCP
tool calls with 'Invalid JSON argument' errors.
The fix adds a JSON.parse() guard before the existing type check,
falling through silently if parsing fails. The existing code path
then handles it as before (either accepts the object or rejects
malformed input).
This matches the fix applied to Roo Code v3.54.0 which was field-
tested across multiple MCP providers (playwright-stealth etc).
pajitosingh
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
📝 WalkthroughWalkthroughThe PR makes ChangesArguments JSON Parsing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Thank you for your contribution! I had one comment around increasing test coverage, but this looks great!
| try { | ||
| params.arguments = JSON.parse(params.arguments) | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
Could we add a test in useMcpToolTool.spec.ts that passes nativeArgs.arguments as a JSON-encoded string (e.g. '{"headless": true}' as unknown as Record<string, unknown>) and verifies callTool receives the parsed object? This is the primary behavior change and currently has no regression coverage.
There was a problem hiding this comment.
@edelauna Done! Added the test in useMcpToolTool.spec.ts — the new test "should parse JSON-string arguments and pass parsed object to callTool" passes nativeArgs.arguments as the JSON string '{"headless": true}' and verifies via callToolMock that callTool receives the parsed object { headless: true }, not the raw string.
The test has been pushed to this PR branch (commit c92ab3f). Ready for re-review.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Was this fixed in main branch? Just switched over from Roo to Zoo today. |
See: #255 (review) |
Add a test that passes nativeArgs.arguments as a JSON-encoded string
(e.g. '{"headless": true}') and verifies that callTool receives the
parsed object rather than the raw string.
This addresses the review feedback from @edelauna on PR Zoo-Code-Org#255, ensuring
the primary behavior change has regression coverage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/tools/__tests__/useMcpToolTool.spec.ts (1)
257-302: ⚡ Quick winConsider adding a test case for malformed JSON strings.
The new test comprehensively covers the happy path (valid JSON string → parsed object → forwarded to callTool). To ensure complete coverage of the JSON-string parsing logic, consider adding a test case that verifies malformed JSON strings (e.g.,
'{"invalid}','not json') are properly rejected.According to the implementation in context snippet 1, if
JSON.parsefails, the catch is silent and the string remains a string, which then fails thetypeof params.arguments !== "object"check. An explicit test would confirm this error path works correctly with the new parsing logic.🧪 Suggested test case for malformed JSON
it("should reject malformed JSON-string arguments", async () => { const callToolMock = vi.fn() mockProviderRef.deref.mockReturnValue({ getMcpHub: () => ({ callTool: callToolMock, getAllServers: vi.fn().mockReturnValue([ { name: "test_server", tools: [{ name: "test_tool", description: "Test Tool" }] }, ]), }), postMessageToWebview: vi.fn(), }) const block: ToolUse = { type: "tool_use", name: "use_mcp_tool", params: { server_name: "test_server", tool_name: "test_tool", arguments: '{"invalid json}', }, nativeArgs: { server_name: "test_server", tool_name: "test_tool", arguments: '{"invalid json}' as unknown as Record<string, unknown>, }, partial: false, } await useMcpToolTool.handle(mockTask as Task, block as any, { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, }) expect(mockTask.consecutiveMistakeCount).toBe(1) expect(mockTask.recordToolError).toHaveBeenCalledWith("use_mcp_tool") expect(mockTask.say).toHaveBeenCalledWith("error", expect.stringContaining("invalid JSON argument")) expect(callToolMock).not.toHaveBeenCalled() })Based on learnings: Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling.
🤖 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/core/tools/__tests__/useMcpToolTool.spec.ts` around lines 257 - 302, Add a unit test that verifies malformed JSON strings in ToolUse.params.arguments are rejected by useMcpToolTool.handle: create a block where params.arguments is an invalid JSON string (e.g. '{"invalid json}'), mock providerRef.deref to return a hub with a callTool spy, call useMcpToolTool.handle with the usual helpers, and assert that mockTask.consecutiveMistakeCount increments to 1, mockTask.recordToolError was called with "use_mcp_tool", mockTask.say was called with "error" and a message containing "invalid JSON argument", and callTool was NOT invoked; this mirrors the existing happy-path test but asserts the error path for malformed JSON parsing.
🤖 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/core/tools/__tests__/useMcpToolTool.spec.ts`:
- Around line 257-302: Add a unit test that verifies malformed JSON strings in
ToolUse.params.arguments are rejected by useMcpToolTool.handle: create a block
where params.arguments is an invalid JSON string (e.g. '{"invalid json}'), mock
providerRef.deref to return a hub with a callTool spy, call
useMcpToolTool.handle with the usual helpers, and assert that
mockTask.consecutiveMistakeCount increments to 1, mockTask.recordToolError was
called with "use_mcp_tool", mockTask.say was called with "error" and a message
containing "invalid JSON argument", and callTool was NOT invoked; this mirrors
the existing happy-path test but asserts the error path for malformed JSON
parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b8c2ffa6-a66a-470f-8c9e-329ed2bf674b
📒 Files selected for processing (1)
src/core/tools/__tests__/useMcpToolTool.spec.ts
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Thank you for the contribution and adding in the test.
* fix: parse JSON-string MCP tool arguments before type check
Some LLMs (DeepSeek V4 Pro, others) emit MCP tool call arguments as
JSON-encoded strings (e.g. '{"headless": true}') rather than as
native objects. This causes validateParams() to reject valid MCP
tool calls with 'Invalid JSON argument' errors.
The fix adds a JSON.parse() guard before the existing type check,
falling through silently if parsing fails. The existing code path
then handles it as before (either accepts the object or rejects
malformed input).
This matches the fix applied to Roo Code v3.54.0 which was field-
tested across multiple MCP providers (playwright-stealth etc).
* test: add coverage for JSON-string MCP tool argument parsing
Add a test that passes nativeArgs.arguments as a JSON-encoded string
(e.g. '{"headless": true}') and verifies that callTool receives the
parsed object rather than the raw string.
This addresses the review feedback from @edelauna on PR #255, ensuring
the primary behavior change has regression coverage.
2 participants
Related GitHub Issue
Port of the fix for Roo Code issue #10919 — the same JSON-string arguments bug affects Zoo Code when used with DeepSeek V4 Pro and other LLMs that emit MCP tool call arguments as JSON-encoded strings.
Description
Problem
Some LLMs (DeepSeek V4 Pro and others) emit MCP tool call
argumentsas JSON-encoded strings rather than as native objects:{ "arguments": "{\"headless\": true}" }instead of:
{ "arguments": { "headless": true } }The existing
validateParams()method inUseMcpToolTool.tsrejects these with "Invalid JSON argument" errors because the type check (typeof params.arguments !== "object") fails on strings.This is the same bug that plagued Roo Code v3.54.0 (see Roo-Code issues #10919 and the archive discussion).
Fix
Adds a
JSON.parse()guard before the existing type check. Ifargumentsarrives as a string, attempt to parse it into an object. If parsing fails (malformed JSON), we fall through silently and the existing code path handles it as before (rejects with the appropriate error).Safety
typeofcheck passes the string guard, goes straight to the object type check → no changeJSON.parse()unwraps the string into an object → type check passes → fixJSON.parse()throws,catch {}swallows it → falls through to the existing error path → graceful rejectionTest Procedure
Unit test: Run
npx vitest run src/core/tools/__tests__/useMcpToolTool.spec.tsnativeArgs.argumentsas the JSON-encoded string'{"headless": true}'and verifiescallToolreceives the parsed object{ headless: true }Manual integration test (DeepSeek V4 Pro or similar model):
browser_navigateto a URL)Regression check: Existing tool calls with object-format arguments (the normal case) must continue to work without any behavioral change.
Pre-Submission Checklist
Documentation Updates
Additional Notes
This fix has been field-tested on Roo Code v3.54.0 with the identical fix across multiple MCP providers (playwright-stealth, Context7, etc.). The root cause is model behavior, not server-specific — any MCP server can be affected when the LLM emits string-encoded arguments.