Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/skip-structured-content-validation-on-error.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@modelcontextprotocol/sdk": patch
---

Skip structuredContent validation when tool result has isError: true

Check warning on line 5 in .changeset/skip-structured-content-validation-on-error.md

View check run for this annotation

Claude / Claude Code Review

Changeset targets wrong package name

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`.
Comment on lines +1 to +5
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

4 changes: 2 additions & 2 deletions packages/client/src/client/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -879,8 +879,8 @@ export class Client extends Protocol<ClientContext> {
);
}

// Only validate structured content if present (not when there's an error)
if (result.structuredContent) {
// Only validate structured content if present and not an error result
if (result.structuredContent && !result.isError) {
try {
// Validate the structured content against the schema
const validationResult = validator(result.structuredContent);
Expand Down
4 changes: 2 additions & 2 deletions packages/client/src/experimental/tasks/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,8 @@ export class ExperimentalClientTasks {
return;
}

// Only validate structured content if present (not when there's an error)
if (result.structuredContent) {
// Only validate structured content if present and not an error result
if (result.structuredContent && !result.isError) {
try {
// Validate the structured content against the schema
const validationResult = validator(result.structuredContent);
Expand Down
170 changes: 170 additions & 0 deletions test/integration/test/client/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1945,6 +1945,98 @@ describe('outputSchema validation', () => {
);
});

/***
* Test: Skip structuredContent validation when isError is true
*/
test('should not validate structuredContent when isError is true', async () => {
const server = new Server(
{
name: 'test-server',
version: '1.0.0'
},
{
capabilities: {
tools: {}
}
}
);

// Set up server handlers
server.setRequestHandler('initialize', async request => ({
protocolVersion: request.params.protocolVersion,
capabilities: { tools: {} },
serverInfo: {
name: 'test-server',
version: '1.0.0'
}
}));

server.setRequestHandler('tools/list', async () => ({
tools: [
{
name: 'test-tool',
description: 'A test tool',
inputSchema: {
type: 'object',
properties: {}
},
outputSchema: {
type: 'object',
properties: {
result: { type: 'string' }
},
required: ['result']
}
}
]
}));

server.setRequestHandler('tools/call', async () => {
// Return isError with structuredContent that does NOT match the schema
return {
isError: true,
content: [{ type: 'text', text: 'Something went wrong' }],
structuredContent: { wrongField: 123 }
};
});

const client = new Client(
{
name: 'test-client',
version: '1.0.0'
},
{
capabilities: {
tasks: {
requests: {
tools: {
call: {}
},
tasks: {
get: true,
list: {},
result: true
}
}
}
}
}
);

const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();

await Promise.all([client.connect(clientTransport), server.connect(serverTransport)]);

// List tools to cache the schemas
await client.listTools();

// Call the tool - should NOT throw, error results skip validation
const result = await client.callTool({ name: 'test-tool' });
expect(result.isError).toBe(true);
expect(result.content).toEqual([{ type: 'text', text: 'Something went wrong' }]);
expect(result.structuredContent).toEqual({ wrongField: 123 });
});

/***
* Test: Handle Tools Without outputSchema Normally
*/
Expand Down Expand Up @@ -3998,6 +4090,84 @@ test('callToolStream() should not validate structuredContent when isError is tru
await server.close();
});

test('callToolStream() should not validate structuredContent when isError is true and structuredContent is invalid', async () => {
const server = new Server(
{
name: 'test-server',
version: '1.0.0'
},
{
capabilities: {
tools: {}
}
}
);

server.setRequestHandler('tools/list', async () => ({
tools: [
{
name: 'test-tool',
description: 'A test tool',
inputSchema: {
type: 'object',
properties: {}
},
outputSchema: {
type: 'object',
properties: {
result: { type: 'string' }
},
required: ['result']
}
}
]
}));

server.setRequestHandler('tools/call', async () => {
// Return isError with structuredContent that does NOT match the schema
return {
isError: true,
content: [{ type: 'text', text: 'Something went wrong' }],
structuredContent: { wrongField: 123 }
};
});

const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();

const client = new Client(
{
name: 'test-client',
version: '1.0.0'
},
{
capabilities: {}
}
);

await Promise.all([client.connect(clientTransport), server.connect(serverTransport)]);

await client.listTools();

const stream = client.experimental.tasks.callToolStream({ name: 'test-tool', arguments: {} });

const messages = [];
for await (const message of stream) {
messages.push(message);
}

// Should have received result (not error), with isError flag set
expect(messages.length).toBe(1);
expect(messages[0]!.type).toBe('result');
if (messages[0]!.type === 'result') {
expect(messages[0]!.result.isError).toBe(true);
expect(messages[0]!.result.content).toEqual([{ type: 'text', text: 'Something went wrong' }]);
expect(messages[0]!.result.structuredContent).toEqual({ wrongField: 123 });
}

await client.close();
await server.close();
});

describe('getSupportedElicitationModes', () => {
test('should support nothing when capabilities are undefined', () => {
const result = getSupportedElicitationModes(undefined);
Expand Down
Loading