fix: skip client-side structuredContent validation when isError is true#1690
Conversation
When a tool with outputSchema returns isError: true along with structuredContent that doesn't match the schema, the client-side validation was still running and throwing, preventing the error from reaching the caller. Now both callTool() and callToolStream() skip schema validation for error results, matching the server-side behavior. Fixes modelcontextprotocol#654
🦋 Changeset detectedLatest commit: 696b4bf The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
travisbreaks
left a comment
travisbreaks
left a comment
There was a problem hiding this comment.
Fix is correct and the logic matches what the server already does (early return on isError at mcp.ts:284). The outputSchema defines the contract for successful tool output, so validating it against error responses is a category error. Good catch on the client-side inconsistency: the "missing structuredContent" guard already checked !result.isError, but the "validate structuredContent" block didn't.
One gap in test coverage: the callToolStream test only asserts on isError and content but doesn't verify that structuredContent is preserved in the result. The callTool test does verify this (expect(result.structuredContent).toEqual({ wrongField: 123 })). Worth adding the same assertion to the stream test for parity, since the whole point is that the content passes through unmodified.
Also: changeset bot flagged this needs a changeset before merge.
Add structuredContent assertion to callToolStream test for parity with callTool test, and add changeset file for the patch.
|
Added — |
joaquinhuigomez
force-pushed
the
fix/skip-output-validation-on-error
branch
from
d0bd36a to
696b4bf
Compare
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| --- | ||
| "@modelcontextprotocol/sdk": patch | ||
| --- | ||
|
|
||
| Skip structuredContent validation when tool result has isError: true |
There was a problem hiding this comment.
🟡 The changeset targets @modelcontextprotocol/sdk (the private root package) instead of @modelcontextprotocol/client. Since @modelcontextprotocol/sdk has "private": true, this changeset will not trigger a version bump for the published @modelcontextprotocol/client package where the actual code changes live. Change line 2 to "@modelcontextprotocol/client": patch.
Extended reasoning...
The changeset file .changeset/skip-structured-content-validation-on-error.md references "@modelcontextprotocol/sdk": patch on line 2. However, @modelcontextprotocol/sdk is the root monorepo package, which is marked "private": true in the root package.json and is never published to npm.
All code changes in this PR are in packages/client/ — specifically packages/client/src/client/client.ts and packages/client/src/experimental/tasks/client.ts. The package at that path is @modelcontextprotocol/client, which is the package that consumers actually install.
Every other changeset in this repository correctly targets specific sub-packages. For example, existing changesets reference @modelcontextprotocol/client, @modelcontextprotocol/server, @modelcontextprotocol/core, or @modelcontextprotocol/node — none of them target the root @modelcontextprotocol/sdk.
When changeset version runs, it will see the target @modelcontextprotocol/sdk, find that it is private, and skip bumping it. Since @modelcontextprotocol/client does not depend on @modelcontextprotocol/sdk, the updateInternalDependencies setting will not cascade the bump either. The net result is that this bug fix will be merged but never released as a new version of @modelcontextprotocol/client.
The fix is straightforward: change line 2 of the changeset from "@modelcontextprotocol/sdk": patch to "@modelcontextprotocol/client": patch.
|
Closing — issue #654 was resolved server-side by PR #655, and the client-side validation now also correctly skips structuredContent validation when |
4 participants
No description provided.