fix(client): skip outputSchema validation when result is an error

[bokelley]

Motivation and Context

Fixes #1943.

Client-side callTool validates structuredContent against outputSchema whenever it is present, regardless of result.isError. The server-side validateToolOutput correctly short-circuits on isError (packages/server/src/server/mcp.ts:276). The asymmetry makes any tool whose error envelope carries structuredContent (e.g. { error: { code, message } }) unusable with outputSchema — the client throws Structured content does not match the tool's output schema before the caller can see the error payload.

Per @truffle-dev's triage on the issue, the same missing !result.isError guard exists in two places with the same comment:

• packages/client/src/client/client.ts:893 (main callTool)
• packages/client/src/experimental/tasks/client.ts:139 (callToolStream)

Fixing only the first leaves tasks-mode clients with the same bug. This PR fixes both.

How Has This Been Tested?

Added two integration tests in test/integration/test/client/client.test.ts:

1. should not validate structuredContent against outputSchema when isError is true — covers client.callTool.
2. callToolStream() should not validate structuredContent when isError is true (with structuredContent present) — complements the existing test at line 4014, which only covers the isError + no-structuredContent case.

Both tests return an error envelope whose structuredContent does not match outputSchema and assert the call resolves with isError: true and the original structuredContent preserved. Without the fix, both throw ProtocolError with the "does not match the tool's output schema" message.

pnpm --filter @modelcontextprotocol/test-integration test -- client/client.test.ts
# Test Files  16 passed (16)
#      Tests  425 passed (425)

Typecheck and build also pass:

pnpm typecheck:all
pnpm build:all

Breaking Changes

None. The fix aligns client behavior with server behavior and the existing comment on the guard.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed (changeset included)

Additional context

The guard comment already read "Only validate structured content if present (not when there's an error)" — the !result.isError check was just missing. Server-side reference for contrast at packages/server/src/server/mcp.ts:276:

if (result.isError) {
    return;
}

Found while migrating @adcp/client to use registerTool with structured error envelopes.

[changeset-bot]

🦋 Changeset detected

Latest commit: ed8e535

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@modelcontextprotocol/client	Patch

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

[pkg-pr-new]

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@1945

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@1945

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@1945

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@1945

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@1945

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@1945

commit: ed8e535

[truffle-dev]

Fix matches the triage. Both guard sites closed with the same && !result.isError clause, and the new tests hit both call paths using structuredContent that intentionally violates outputSchema, the shape that was throwing before.

One merge-useful framing for reviewers: the comment above each guard already reads "Only validate structured content if present (not when there's an error)". The patch makes the guard match the stated intent rather than introducing new intent, so this is strictly a bug fix (semver-patch) aligning the client with the server-side short-circuit at packages/server/src/server/mcp.ts:276.

Test coverage pairs with the existing callToolStream() test on main at line 4014 (isError + no structuredContent). Together they cover both paths through each guard.

[bokelley]

Fix is correct — symmetric with server-side validateToolOutput at packages/server/src/server/mcp.ts:276, and the changeset scope/level look right.

Two things before merge:

1. Duplicate test name. The new streaming test shares the exact name of the existing one at old line 4087 ('callToolStream() should not validate structuredContent when isError is true'). They cover different cases — existing: isError with no structuredContent; new: isError with mismatched structuredContent — but identical names make failure triage ambiguous. Suggest renaming the new one to 'callToolStream() should not validate structuredContent against outputSchema when isError is true' (mirrors the non-streaming test in the outputSchema validation describe block), and/or narrowing the old one to '...when isError is true and structuredContent is absent'.

2. Unnecessary cast. The new test does messages[0]!.result as CallToolResult and adds a CallToolResult import; the existing line-4087 test accesses .result.isError with no cast. Drop the cast to match established style, or add a one-line comment if the inference really requires it.

Optional: a third test with isError: true and structuredContent that matches outputSchema would document that validation is skipped regardless of shape — guards against a future "optimization" that re-introduces the bug. Not a blocker.

[truffle-dev]

On the optional third test: the same coverage gap exists on the server side. test/integration/test/server/mcp.test.ts:1444 (should skip outputSchema validation when isError is true) covers the isError + no structuredContent branch only. Neither side has isError: true with structuredContent that matches outputSchema. That's exactly the path a future "skip only when content mismatches" refactor could silently re-introduce.

Adding it on the client side alone leaves the server-side invariant untested. If the merge bar allows, a paired server test (tool returns isError: true with a structuredContent that matches its own outputSchema; assert client sees result.isError === true without a ProtocolError) keeps the symmetry explicit. Happy to send it as a follow-up PR if useful.

On the two flagged items: rename to 'callToolStream() should not validate structuredContent against outputSchema when isError is true' mirrors the non-streaming describe-block wording. Cast drop is safe: the existing callToolStream() test reads messages[0]!.result.isError inside if (messages[0]!.type === 'result') with no CallToolResult assertion, so the narrow holds.

[Zelys-DFKH]

Went through the same two-site analysis on the issue before #1947 got closed, so I can confirm the guard logic here is right. The rename truffle-dev suggested ('callToolStream() should not validate structuredContent against outputSchema when isError is true') fits cleanly with the non-streaming describe block.

On the server-side test gap: happy to take that as a follow-up if it helps get this merged without expanding scope. One test, path is clear.

[truffle-dev]

Thanks Zelys-DFKH. Split is the right call. The third test probes a separate policy question (skip on every error, or only when content mismatches the schema), cleaner as its own thread. Happy to review the follow-up when it is up.