fix(core): generalize MCP compliance fix for tool results (#27045)

This commit is contained in:
Coco Sheng
2026-05-20 13:31:00 -04:00
committed by GitHub
parent aebdba6565
commit 99ae4d8b81
5 changed files with 275 additions and 191 deletions
+49 -37
View File
@@ -40,6 +40,7 @@ import {
discoverPrompts,
type McpContext,
} from './mcp-client.js';
import { McpComplianceTransport } from './mcp-compliance-transport.js';
import type { ToolRegistry } from './tool-registry.js';
import type { ResourceRegistry } from '../resources/resource-registry.js';
import * as fs from 'node:fs';
@@ -71,6 +72,9 @@ const MOCK_CONTEXT_DEFAULT = {
let MOCK_CONTEXT: McpContext = MOCK_CONTEXT_DEFAULT;
const unwrap = (t: any) =>
t instanceof McpComplianceTransport ? t.transport : t;
vi.mock('@modelcontextprotocol/sdk/client/stdio.js');
vi.mock('@modelcontextprotocol/sdk/client/index.js');
vi.mock('@google/genai');
@@ -1937,7 +1941,7 @@ describe('mcp-client', () => {
MOCK_CONTEXT,
);
const testableTransport = transport as unknown as {
const testableTransport = unwrap(transport) as unknown as {
_authProvider?: {
tokens: () => Promise<{ access_token: string } | undefined>;
};
@@ -1983,7 +1987,7 @@ describe('mcp-client', () => {
MOCK_CONTEXT,
);
const testableTransport = transport as unknown as {
const testableTransport = unwrap(transport) as unknown as {
_authProvider?: {
tokens: () => Promise<
{ access_token: string; expires_in?: number } | undefined
@@ -2032,7 +2036,7 @@ describe('mcp-client', () => {
MOCK_CONTEXT,
);
const testableTransport = transport as unknown as {
const testableTransport = unwrap(transport) as unknown as {
_authProvider?: {
tokens: () => Promise<{ access_token: string } | undefined>;
};
@@ -2075,7 +2079,7 @@ describe('mcp-client', () => {
MOCK_CONTEXT,
);
const testableTransport = transport as unknown as {
const testableTransport = unwrap(transport) as unknown as {
_authProvider?: {
tokens: () => Promise<{ access_token: string } | undefined>;
};
@@ -2128,7 +2132,7 @@ describe('mcp-client', () => {
MOCK_CONTEXT,
);
const testableTransport = transport as unknown as {
const testableTransport = unwrap(transport) as unknown as {
_authProvider?: {
tokens: () => Promise<
{ access_token: string; expires_in?: number } | undefined
@@ -2167,7 +2171,7 @@ describe('mcp-client', () => {
);
const wrappedFetch = (
transport as unknown as {
unwrap(transport) as unknown as {
_fetch: (
url: URL | string,
init?: RequestInit,
@@ -2209,7 +2213,7 @@ describe('mcp-client', () => {
// For SSEClientTransport, the fetch is private or passed to the SDK.
// We can check if it creates the transport successfully.
expect(transport).toBeInstanceOf(SSEClientTransport);
expect(unwrap(transport)).toBeInstanceOf(SSEClientTransport);
} finally {
vi.unstubAllEnvs();
vi.unstubAllGlobals();
@@ -2227,8 +2231,8 @@ describe('mcp-client', () => {
false,
MOCK_CONTEXT,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
expect(transport).toMatchObject({
expect(unwrap(transport)).toBeInstanceOf(StreamableHTTPClientTransport);
expect(unwrap(transport)).toMatchObject({
_url: new URL('http://test-server'),
_requestInit: { headers: {} },
});
@@ -2245,8 +2249,8 @@ describe('mcp-client', () => {
MOCK_CONTEXT,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
expect(transport).toMatchObject({
expect(unwrap(transport)).toBeInstanceOf(StreamableHTTPClientTransport);
expect(unwrap(transport)).toMatchObject({
_url: new URL('http://test-server'),
_requestInit: {
headers: { Authorization: 'derp' },
@@ -2265,8 +2269,8 @@ describe('mcp-client', () => {
MOCK_CONTEXT,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
expect(transport).toMatchObject({
expect(unwrap(transport)).toBeInstanceOf(StreamableHTTPClientTransport);
expect(unwrap(transport)).toMatchObject({
_url: new URL('http://test-server'),
_requestInit: { headers: {} },
});
@@ -2283,8 +2287,8 @@ describe('mcp-client', () => {
MOCK_CONTEXT,
);
expect(transport).toBeInstanceOf(SSEClientTransport);
expect(transport).toMatchObject({
expect(unwrap(transport)).toBeInstanceOf(SSEClientTransport);
expect(unwrap(transport)).toMatchObject({
_url: new URL('http://test-server'),
_requestInit: { headers: {} },
});
@@ -2300,8 +2304,8 @@ describe('mcp-client', () => {
MOCK_CONTEXT,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
expect(transport).toMatchObject({
expect(unwrap(transport)).toBeInstanceOf(StreamableHTTPClientTransport);
expect(unwrap(transport)).toMatchObject({
_url: new URL('http://test-server'),
_requestInit: { headers: {} },
});
@@ -2319,8 +2323,8 @@ describe('mcp-client', () => {
MOCK_CONTEXT,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
expect(transport).toMatchObject({
expect(unwrap(transport)).toBeInstanceOf(StreamableHTTPClientTransport);
expect(unwrap(transport)).toMatchObject({
_url: new URL('http://test-server'),
_requestInit: {
headers: { Authorization: 'Bearer token' },
@@ -2340,8 +2344,8 @@ describe('mcp-client', () => {
MOCK_CONTEXT,
);
expect(transport).toBeInstanceOf(SSEClientTransport);
expect(transport).toMatchObject({
expect(unwrap(transport)).toBeInstanceOf(SSEClientTransport);
expect(unwrap(transport)).toMatchObject({
_url: new URL('http://test-server'),
_requestInit: {
headers: { 'X-API-Key': 'key123' },
@@ -2361,8 +2365,8 @@ describe('mcp-client', () => {
);
// httpUrl should take priority and create HTTP transport
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
expect(transport).toMatchObject({
expect(unwrap(transport)).toBeInstanceOf(StreamableHTTPClientTransport);
expect(unwrap(transport)).toMatchObject({
_url: new URL('http://test-server-http'),
_requestInit: { headers: {} },
});
@@ -2587,8 +2591,10 @@ describe('mcp-client', () => {
MOCK_CONTEXT,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
const testableTransport = transport as unknown as TestableTransport;
expect(unwrap(transport)).toBeInstanceOf(StreamableHTTPClientTransport);
const testableTransport = unwrap(
transport,
) as unknown as TestableTransport;
const authProvider = testableTransport._authProvider;
expect(authProvider).toBeInstanceOf(GoogleCredentialProvider);
const googUserProject =
@@ -2618,9 +2624,11 @@ describe('mcp-client', () => {
MOCK_CONTEXT,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
expect(unwrap(transport)).toBeInstanceOf(StreamableHTTPClientTransport);
expect(mockGetRequestHeaders).toHaveBeenCalled();
const testableTransport = transport as unknown as TestableTransport;
const testableTransport = unwrap(
transport,
) as unknown as TestableTransport;
const headers = testableTransport._requestInit?.headers;
expect(headers?.['X-Goog-User-Project']).toBe('provider-project');
});
@@ -2650,8 +2658,10 @@ describe('mcp-client', () => {
MOCK_CONTEXT,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
const testableTransport = transport as unknown as TestableTransport;
expect(unwrap(transport)).toBeInstanceOf(StreamableHTTPClientTransport);
const testableTransport = unwrap(
transport,
) as unknown as TestableTransport;
const headers = testableTransport._requestInit?.headers;
expect(headers?.['X-Goog-User-Project']).toBe('provider-project');
});
@@ -2671,8 +2681,10 @@ describe('mcp-client', () => {
MOCK_CONTEXT,
);
expect(transport).toBeInstanceOf(SSEClientTransport);
const testableTransport = transport as unknown as TestableTransport;
expect(unwrap(transport)).toBeInstanceOf(SSEClientTransport);
const testableTransport = unwrap(
transport,
) as unknown as TestableTransport;
const authProvider = testableTransport._authProvider;
expect(authProvider).toBeInstanceOf(GoogleCredentialProvider);
});
@@ -2842,7 +2854,7 @@ describe('connectToMcpServer with OAuth', () => {
let capturedTransport: TestableTransport | undefined;
vi.mocked(mockedClient.connect).mockImplementationOnce(
async (transport) => {
capturedTransport = transport as unknown as TestableTransport;
capturedTransport = unwrap(transport) as unknown as TestableTransport;
return Promise.resolve();
},
);
@@ -2860,8 +2872,8 @@ describe('connectToMcpServer with OAuth', () => {
expect(mockedClient.connect).toHaveBeenCalledTimes(2);
expect(mockAuthProvider.authenticate).toHaveBeenCalledOnce();
const authHeader = (capturedTransport as TestableTransport)._requestInit
?.headers?.['Authorization'];
const authHeader = (unwrap(capturedTransport) as TestableTransport)
._requestInit?.headers?.['Authorization'];
expect(authHeader).toBe('Bearer test-access-token');
});
@@ -2887,7 +2899,7 @@ describe('connectToMcpServer with OAuth', () => {
let capturedTransport: TestableTransport | undefined;
vi.mocked(mockedClient.connect).mockImplementationOnce(
async (transport) => {
capturedTransport = transport as unknown as TestableTransport;
capturedTransport = unwrap(transport) as unknown as TestableTransport;
return Promise.resolve();
},
);
@@ -2906,8 +2918,8 @@ describe('connectToMcpServer with OAuth', () => {
expect(mockAuthProvider.authenticate).toHaveBeenCalledOnce();
expect(OAuthUtils.discoverOAuthConfig).toHaveBeenCalledWith(serverUrl);
const authHeader = (capturedTransport as TestableTransport)._requestInit
?.headers?.['Authorization'];
const authHeader = (unwrap(capturedTransport) as TestableTransport)
._requestInit?.headers?.['Authorization'];
expect(authHeader).toBe('Bearer test-access-token-from-discovery');
});
+23 -25
View File
@@ -49,7 +49,7 @@ import {
import { GoogleCredentialProvider } from '../mcp/google-auth-provider.js';
import { ServiceAccountImpersonationProvider } from '../mcp/sa-impersonation-provider.js';
import { DiscoveredMCPTool } from './mcp-tool.js';
import { XcodeMcpBridgeFixTransport } from './xcode-mcp-fix-transport.js';
import { McpComplianceTransport } from './mcp-compliance-transport.js';
import type { CallableTool, FunctionCall, Part, Tool } from '@google/genai';
import { basename } from 'node:path';
@@ -1055,7 +1055,7 @@ async function createTransportWithOAuth(
mcpServerConfig: MCPServerConfig,
accessToken: string,
cliConfig: McpContext,
): Promise<StreamableHTTPClientTransport | SSEClientTransport | null> {
): Promise<Transport | null> {
try {
const headers: Record<string, string> = {
Authorization: `Bearer ${accessToken}`,
@@ -1070,7 +1070,12 @@ async function createTransportWithOAuth(
),
};
return createUrlTransport(mcpServerName, mcpServerConfig, transportOptions);
const transport = createUrlTransport(
mcpServerName,
mcpServerConfig,
transportOptions,
);
return transport ? new McpComplianceTransport(transport) : null;
} catch (error) {
cliConfig.emitMcpDiagnostic(
'error',
@@ -2318,7 +2323,9 @@ export async function createTransport(
authProvider,
};
return createUrlTransport(mcpServerName, mcpServerConfig, transportOptions);
return new McpComplianceTransport(
createUrlTransport(mcpServerName, mcpServerConfig, transportOptions),
);
}
if (mcpServerConfig.command) {
@@ -2354,32 +2361,23 @@ export async function createTransport(
}
}
let transport: Transport = new StdioClientTransport({
command: mcpServerConfig.command,
args: mcpServerConfig.args || [],
env: finalEnv,
cwd: mcpServerConfig.cwd,
stderr: 'pipe',
});
// Fix for Xcode 26.3 mcpbridge non-compliant responses
// It returns JSON in `content` instead of `structuredContent`
if (
mcpServerConfig.command === 'xcrun' &&
mcpServerConfig.args?.includes('mcpbridge')
) {
transport = new XcodeMcpBridgeFixTransport(transport);
}
const transport: Transport = new McpComplianceTransport(
new StdioClientTransport({
command: mcpServerConfig.command,
args: mcpServerConfig.args || [],
env: finalEnv,
cwd: mcpServerConfig.cwd,
stderr: 'pipe',
}),
);
if (debugMode) {
// The `XcodeMcpBridgeFixTransport` wrapper hides the underlying `StdioClientTransport`,
// The `McpComplianceTransport` wrapper hides the underlying `StdioClientTransport`,
// which exposes `stderr` for debug logging. We need to unwrap it to attach the listener.
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
const underlyingTransport =
transport instanceof XcodeMcpBridgeFixTransport
? // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-type-assertion
(transport as any).transport
transport instanceof McpComplianceTransport
? transport.transport
: transport;
if (
@@ -0,0 +1,197 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import { describe, it, expect, vi } from 'vitest';
import { McpComplianceTransport } from './mcp-compliance-transport.js';
import type { Transport } from '@modelcontextprotocol/sdk/shared/transport.js';
import type { JSONRPCMessage } from '@modelcontextprotocol/sdk/types.js';
import { EventEmitter } from 'node:events';
describe('McpComplianceTransport', () => {
const createMockTransport = () => {
const transport = new EventEmitter() as unknown as Transport &
EventEmitter & {
start: ReturnType<typeof vi.fn>;
close: ReturnType<typeof vi.fn>;
send: ReturnType<typeof vi.fn>;
};
transport.start = vi.fn().mockResolvedValue(undefined);
transport.close = vi.fn().mockResolvedValue(undefined);
transport.send = vi.fn().mockResolvedValue(undefined);
return transport;
};
it('should forward non-response messages without modification', async () => {
const mockTransport = createMockTransport();
const complianceTransport = new McpComplianceTransport(mockTransport);
const onMessage = vi.fn();
complianceTransport.onmessage = onMessage;
const request: JSONRPCMessage = {
jsonrpc: '2.0',
id: 1,
method: 'tools/list',
};
mockTransport.onmessage!(request);
expect(onMessage).toHaveBeenCalledWith(request);
});
it('should fix non-compliant tool results (JSON in content)', async () => {
const mockTransport = createMockTransport();
const complianceTransport = new McpComplianceTransport(mockTransport);
const onMessage = vi.fn();
complianceTransport.onmessage = onMessage;
const rawResponse: JSONRPCMessage = {
jsonrpc: '2.0',
id: 1,
result: {
content: [
{
type: 'text',
text: JSON.stringify({ foo: 'bar' }),
},
],
},
};
mockTransport.onmessage!(rawResponse);
const fixedResponse = onMessage.mock.calls[0][0];
expect(fixedResponse.result.structuredContent).toEqual({ foo: 'bar' });
// Original content should still be there
expect(fixedResponse.result.content[0].text).toBe(
JSON.stringify({ foo: 'bar' }),
);
});
it('should NOT modify already compliant responses', async () => {
const mockTransport = createMockTransport();
const complianceTransport = new McpComplianceTransport(mockTransport);
const onMessage = vi.fn();
complianceTransport.onmessage = onMessage;
const compliantResponse: JSONRPCMessage = {
jsonrpc: '2.0',
id: 1,
result: {
content: [{ type: 'text', text: 'some display text' }],
structuredContent: { foo: 'bar' },
},
};
mockTransport.onmessage!(compliantResponse);
expect(onMessage).toHaveBeenCalledWith(compliantResponse);
expect(onMessage.mock.calls[0][0].result.structuredContent).toEqual({
foo: 'bar',
});
});
it('should NOT modify content that is not JSON', async () => {
const mockTransport = createMockTransport();
const complianceTransport = new McpComplianceTransport(mockTransport);
const onMessage = vi.fn();
complianceTransport.onmessage = onMessage;
const textResponse: JSONRPCMessage = {
jsonrpc: '2.0',
id: 1,
result: {
content: [{ type: 'text', text: 'just some plain text, not JSON' }],
},
};
mockTransport.onmessage!(textResponse);
const result = onMessage.mock.calls[0][0].result;
expect(result.structuredContent).toBeUndefined();
expect(result.content[0].text).toBe('just some plain text, not JSON');
});
it('should handle empty content gracefully', async () => {
const mockTransport = createMockTransport();
const complianceTransport = new McpComplianceTransport(mockTransport);
const onMessage = vi.fn();
complianceTransport.onmessage = onMessage;
const emptyResponse: JSONRPCMessage = {
jsonrpc: '2.0',
id: 1,
result: {
content: [],
},
};
mockTransport.onmessage!(emptyResponse);
expect(onMessage.mock.calls[0][0].result.structuredContent).toBeUndefined();
});
it('should only parse text content blocks', async () => {
const mockTransport = createMockTransport();
const complianceTransport = new McpComplianceTransport(mockTransport);
const onMessage = vi.fn();
complianceTransport.onmessage = onMessage;
const mediaResponse: JSONRPCMessage = {
jsonrpc: '2.0',
id: 1,
result: {
content: [
{
type: 'image',
data: 'base64data',
mimeType: 'image/png',
},
],
},
};
mockTransport.onmessage!(mediaResponse);
expect(onMessage.mock.calls[0][0].result.structuredContent).toBeUndefined();
});
it('should handle responses with multiple content blocks (only fixes the first one if it is text)', async () => {
const mockTransport = createMockTransport();
const complianceTransport = new McpComplianceTransport(mockTransport);
const onMessage = vi.fn();
complianceTransport.onmessage = onMessage;
const multiResponse: JSONRPCMessage = {
jsonrpc: '2.0',
id: 1,
result: {
content: [
{ type: 'text', text: JSON.stringify({ first: true }) },
{ type: 'text', text: 'second item' },
],
},
};
mockTransport.onmessage!(multiResponse);
expect(onMessage.mock.calls[0][0].result.structuredContent).toEqual({
first: true,
});
});
it('should handle error responses gracefully', async () => {
const mockTransport = createMockTransport();
const complianceTransport = new McpComplianceTransport(mockTransport);
const onMessage = vi.fn();
complianceTransport.onmessage = onMessage;
const errorResponse: JSONRPCMessage = {
jsonrpc: '2.0',
id: 1,
error: {
code: -32000,
message: 'Internal error',
},
};
mockTransport.onmessage!(errorResponse);
expect(onMessage).toHaveBeenCalledWith(errorResponse);
});
});
@@ -12,19 +12,16 @@ import type {
import { EventEmitter } from 'node:events';
/**
* A wrapper transport that intercepts messages from Xcode's mcpbridge and fixes
* A wrapper transport that intercepts messages from MCP servers and fixes
* non-compliant responses.
*
* Issue: Xcode 26.3's mcpbridge returns tool results in `content` but misses
* `structuredContent` when the tool has an output schema.
* Issue: Some MCP servers (e.g., Xcode 26.3's mcpbridge) return tool results in
* `content` but miss `structuredContent` when the tool has an output schema.
*
* Fix: Parse the text content as JSON and populate `structuredContent`.
* Fix: Parse the text content as JSON and populate `structuredContent` if it's missing.
*/
export class XcodeMcpBridgeFixTransport
extends EventEmitter
implements Transport
{
constructor(private readonly transport: Transport) {
export class McpComplianceTransport extends EventEmitter implements Transport {
constructor(readonly transport: Transport) {
super();
// Forward messages from the underlying transport
@@ -1,120 +0,0 @@
/**
* @license
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import { describe, it, expect } from 'vitest';
import { EventEmitter } from 'node:events';
import { XcodeMcpBridgeFixTransport } from './xcode-mcp-fix-transport.js';
import type { Transport } from '@modelcontextprotocol/sdk/shared/transport.js';
import type { JSONRPCMessage } from '@modelcontextprotocol/sdk/types.js';
// Mock Transport that simulates the mcpbridge behavior
class MockBadMcpBridgeTransport extends EventEmitter implements Transport {
onclose?: () => void;
onerror?: (error: Error) => void;
onmessage?: (message: JSONRPCMessage) => void;
async start() {}
async close() {}
async send(_message: JSONRPCMessage) {}
// eslint-disable-next-line @typescript-eslint/no-explicit-any
emitMessage(msg: any) {
this.onmessage?.(msg);
}
}
describe('Xcode MCP Bridge Fix', () => {
it('intercepts and fixes the non-compliant mcpbridge response', async () => {
const mockTransport = new MockBadMcpBridgeTransport();
const fixTransport = new XcodeMcpBridgeFixTransport(mockTransport);
// We need to capture what the fixTransport emits to its listeners
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const messages: any[] = [];
fixTransport.onmessage = (msg) => {
messages.push(msg);
};
await fixTransport.start();
// SCENARIO 1: Bad response from Xcode
// It has `content` stringified JSON, but misses `structuredContent`
const badPayload = {
jsonrpc: '2.0',
id: 1,
result: {
content: [
{
type: 'text',
text: JSON.stringify({
windows: [{ title: 'HelloWorld', path: '/path/to/project' }],
}),
},
],
// Missing: structuredContent
},
};
mockTransport.emitMessage(badPayload);
// Verify the message received by the client (listener of fixTransport)
const fixedMsg = messages.find((m) => m.id === 1);
expect(fixedMsg).toBeDefined();
expect(fixedMsg.result.structuredContent).toBeDefined();
expect(fixedMsg.result.structuredContent.windows[0].title).toBe(
'HelloWorld',
);
// SCENARIO 2: Good response (should be untouched)
const goodPayload = {
jsonrpc: '2.0',
id: 2,
result: {
content: [{ type: 'text', text: 'normal text' }],
structuredContent: { some: 'data' },
},
};
mockTransport.emitMessage(goodPayload);
const goodMsg = messages.find((m) => m.id === 2);
expect(goodMsg).toBeDefined();
expect(goodMsg.result.structuredContent).toEqual({ some: 'data' });
});
it('ignores responses that cannot be parsed as JSON', async () => {
const mockTransport = new MockBadMcpBridgeTransport();
const fixTransport = new XcodeMcpBridgeFixTransport(mockTransport);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const messages: any[] = [];
fixTransport.onmessage = (msg) => {
messages.push(msg);
};
await fixTransport.start();
const nonJsonPayload = {
jsonrpc: '2.0',
id: 3,
result: {
content: [
{
type: 'text',
text: "Just some plain text that isn't JSON",
},
],
},
};
mockTransport.emitMessage(nonJsonPayload);
const msg = messages.find((m) => m.id === 3);
expect(msg).toBeDefined();
expect(msg.result.structuredContent).toBeUndefined();
expect(msg.result.content[0].text).toBe(
"Just some plain text that isn't JSON",
);
});
});