Skip to content

Return MCP parameter validation errors as tool results - #17824

Closed
mfazekas wants to merge 1 commit into
pingdotgg:mainfrom
mfazekas:fix/mcp-param-errors-tool-results
Closed

mfazekas wants to merge 1 commit into
pingdotgg:mainfrom
mfazekas:fix/mcp-param-errors-tool-results

Conversation

@mfazekas

@mfazekas mfazekas commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Effect's McpServer reports invalid tool arguments as a JSON-RPC InvalidParams (-32602) protocol error. Some clients, including the Claude app, show only "MCP tool call failed" for protocol errors, hiding the reason from both the user and the model. Seen with t3_thread_launch when the client sent workspaceStrategy as a JSON string.

Change

Extends patches/effect@4.0.2.patch so McpServer returns parameter validation failures as an isError: true tool result carrying the validation message. The model sees what was wrong and can retry. pnpm-lock.yaml changes are only the new patch hash.

Note: this interacts with #17823, whose t3_thread_read unknown-param test expects a protocol error. Whichever lands second needs that assertion switched to isError / content (a few-line test change). Happy to rebase.

Scope and approval

Small, focused fix: a known failure mode is reported in a form clients can show. No tool schemas or behavior on valid input change.

Verification

Updated McpHttpServer.test.ts (preview_click) and toolkits/core.test.ts (t3_thread_organize) to assert the isError result and message; both pass, as do the rest of the MCP tests. Reproduced the original t3_thread_launch failure from Claude Code before the fix.

Effect's McpServer reported invalid tool arguments as a JSON-RPC
InvalidParams error. Some clients (the Claude app) show only a generic
"tool call failed" for protocol errors, hiding the validation message
from both the user and the model. Return an isError tool result with the
message instead, so callers see what was wrong and can retry.
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Oct 10, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 10, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 6e81243

Macroscope's review found this PR approvable — This is a small, well-scoped MCP error-handling fix: invalid tool parameters are returned as explicit error results while valid calls and other failure paths remain unchanged. Targeted production-registration tests cover the new response shape.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 232e8857-0ab0-4fcd-a136-73dc15d2f297


📥 Commits

Reviewing files that changed from the base of the PR and between 98beed1 and 6e81243.



⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml


📒 Files selected for processing (3)
  • apps/server/src/mcp/McpHttpServer.test.ts
  • apps/server/src/mcp/toolkits/core.test.ts
  • patches/effect@4.0.2.patch


Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.




📝 Walkthrough
📝 Walkthrough

Walkthrough

The patch changes MCP parameter-validation failures to return tool error results and adds an optional RPC client hook that runs when a ping times out. MCP tests now check the returned error results.

Changes

MCP Parameter Validation

Layer / File(s) Summary
Return tool errors for invalid parameters
patches/effect@4.0.2.patch, apps/server/src/mcp/McpHttpServer.test.ts, apps/server/src/mcp/toolkits/core.test.ts
The MCP server returns a tool error result with the validation message instead of failing with JSON-RPC InvalidParams. Tests check the returned error result and message.

RPC Ping Timeout Hook

Layer / File(s) Summary
Run a hook on ping timeout
patches/effect@4.0.2.patch
ConnectionHooks gains an optional onPingTimeout effect. On ping timeout, the socket runs the hook before failing with the existing socket read error.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Suggested reviewers: juliusmarminge



Merge Risk: ⚪ Minimal · up to 6e812

Invalid MCP arguments now return tool-level errors, while ping timeouts retain the existing disconnect handling. The supplied evidence identifies no actionable merge-blocking regression.

Architecture Summary

Architecture risk: 🔵 Low · up to 6e812

The change affects 2 systems.

Changed systems: apps/server, patches

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — apps/server (service) was modified; 2 changed files map to changed impact.
  • observed — patches (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in apps/server/src/mcp/McpHttpServer.test.ts: The test now asserts that malformed tool arguments produce an error result with an error message identifying preview_click; it no longer expects the call to fail as an effect with an InvalidParams tag.
  • observed — Modified behavior in apps/server/src/mcp/toolkits/core.test.ts: The test now checks invalid tool arguments through the returned MCP tool result: it expects isError and text containing the invalid-parameters message. Previously it flipped the call’s failure and asserted the error tag was InvalidParams.
  • observed — Modified behavior in patches/effect@4.0.2.patch: The MCP server’s parameter-validation path now returns toolErrorResult with the validation message instead of failing with JSON-RPC InvalidParams. The patch also adds a comment describing this result path.
  • observed — Modified behavior in patches/effect@4.0.2.patch: ConnectionHooks gains an optional onPingTimeout effect hook, documented to run when the socket is dropped for an unresponsive server, before onDisconnect.


Pre-merge checks | Passed 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely describes the main change: MCP parameter validation errors now return as tool results.
Description check Passed The description includes all required sections. It explains the problem, the implementation, scope justification, and focused verification results. It also identifies the interaction with PR #17823.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR



  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@maria-rcks

Copy link
Copy Markdown
Collaborator

Note

Written by claude-opus-5-5 on behalf of Maria

Hi! We are cleaning up open PRs, and this one does not say which model or harness was used to create it. If this change is really important, we recommend rebuilding the PR with a newer model and noting the model and harness in the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants