Skip to content
Merged
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
251 changes: 250 additions & 1 deletion packages/core/src/tools/mcp-client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@ import * as ClientLib from '@modelcontextprotocol/sdk/client/index.js';
import { SSEClientTransport } from '@modelcontextprotocol/sdk/client/sse.js';
import * as SdkClientStdioLib from '@modelcontextprotocol/sdk/client/stdio.js';
import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js';
import { afterEach, describe, expect, it, vi } from 'vitest';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import { AuthProviderType, type Config } from '../config/config.js';
import { GoogleCredentialProvider } from '../mcp/google-auth-provider.js';
import { MCPOAuthProvider } from '../mcp/oauth-provider.js';
import type { PromptRegistry } from '../prompts/prompt-registry.js';
import type { WorkspaceContext } from '../utils/workspaceContext.js';
import {
addMCPStatusChangeListener,
createStreamableHttpCompatibilityFetch,
createTransport,
getAllMCPServerStatuses,
getMCPServerStatus,
Expand All @@ -31,6 +33,12 @@ import {
import type { ToolRegistry } from './tool-registry.js';

const mockExistsSync = vi.hoisted(() => vi.fn(() => true));
const mockDebugLogger = vi.hoisted(() => ({
debug: vi.fn(),
error: vi.fn(),
info: vi.fn(),
warn: vi.fn(),
}));
const ORIGINAL_ENV = process.env;

vi.mock('node:fs', () => ({
Expand All @@ -41,8 +49,15 @@ vi.mock('@modelcontextprotocol/sdk/client/index.js');
vi.mock('@google/genai');
vi.mock('../mcp/oauth-provider.js');
vi.mock('../mcp/oauth-token-storage.js');
vi.mock('../utils/debugLogger.js', () => ({
createDebugLogger: vi.fn(() => mockDebugLogger),
}));

describe('mcp-client', () => {
beforeEach(() => {
vi.clearAllMocks();
});

afterEach(() => {
vi.restoreAllMocks();
process.env = ORIGINAL_ENV;
Expand Down Expand Up @@ -273,6 +288,8 @@ describe('mcp-client', () => {
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
expect((transport as any)._url).toEqual(new URL('http://test-server'));
// eslint-disable-next-line @typescript-eslint/no-explicit-any
expect((transport as any)._fetch).toEqual(expect.any(Function));
});

it('with headers', async () => {
Expand All @@ -292,6 +309,186 @@ describe('mcp-client', () => {
expect((transport as any)._requestInit?.headers).toEqual({
Authorization: 'derp',
});
// eslint-disable-next-line @typescript-eslint/no-explicit-any

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] _fetch is asserted here and at line 277, but createStreamableHttpCompatibilityFetch is injected into 4 code paths (OAuth at mcp-client.ts:552, SERVICE_ACCOUNT_IMPERSONATION at :1388, GOOGLE_CREDENTIALS at :1416, plain httpUrl at :1473). The OAuth and SERVICE_ACCOUNT_IMPERSONATION paths have no test verifying the fetch was wired. A refactor that accidentally drops the fetch assignment in one branch would go undetected.

Consider adding expect((transport as any)._fetch).toEqual(expect.any(Function)) to the existing Google Credentials httpUrl test and new tests for the OAuth and Service Account Impersonation transport paths.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in 2e80fb4. The test suite now verifies compatibility fetch wiring for OAuth Streamable HTTP, Service Account Impersonation Streamable HTTP, and Google Credentials Streamable HTTP transports, in addition to the existing plain httpUrl coverage.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The test suite covers all guard branches but leaves gaps in readResponseBodyExcerpt sub-branches:

  1. Empty bodyif (!body) return undefined (line 74) is untested. Add new Response(null, { status: 400 }) with GET + SSE Accept.
  2. Truncationbody.length > limit (line 76) is untested. Add new Response('x'.repeat(1024), { status: 400 }) and verify the ... suffix.
  3. Synthetic response shape — tests assert response.status === 405 but not body or statusText. Add expect(await response.text()).toBe('') and expect(response.statusText).toBe('Method Not Allowed').

— qwen-latest-series-invite-beta-v38 via Qwen Code /review

expect((transport as any)._fetch).toEqual(expect.any(Function));
});

it('treats 400 from optional GET SSE stream as unsupported', async () => {
const fetchFn = vi
.fn<typeof fetch>()
.mockResolvedValue(new Response('bad method', { status: 400 }));
const fetchWithFallback = createStreamableHttpCompatibilityFetch(
'spring-ai',
fetchFn,
);

const response = await fetchWithFallback('http://test-server/mcp', {
method: 'GET',
headers: { Accept: 'text/event-stream' },
});

expect(fetchFn).toHaveBeenCalledTimes(1);
expect(response.status).toBe(405);
expect(response.statusText).toBe('Method Not Allowed');
expect(await response.text()).toBe('');
});

it('omits response body diagnostics when the fallback body is empty', async () => {
const fetchFn = vi
.fn<typeof fetch>()
.mockResolvedValue(new Response(null, { status: 400 }));
const fetchWithFallback = createStreamableHttpCompatibilityFetch(
'empty-body',
fetchFn,
);

const response = await fetchWithFallback('http://test-server/mcp', {
method: 'GET',
headers: { Accept: 'text/event-stream' },
});

expect(response.status).toBe(405);
expect(mockDebugLogger.warn).toHaveBeenCalledWith(
expect.not.stringContaining('Response body:'),
);
});

it('truncates Streamable HTTP GET SSE error body diagnostics', async () => {
const fetchFn = vi
.fn<typeof fetch>()
.mockResolvedValue(new Response('x'.repeat(1024), { status: 400 }));
const fetchWithFallback = createStreamableHttpCompatibilityFetch(
'large-error-body',
fetchFn,
);

const response = await fetchWithFallback('http://test-server/mcp', {
method: 'GET',
headers: { Accept: 'text/event-stream' },
});

expect(response.status).toBe(405);
expect(mockDebugLogger.warn).toHaveBeenCalledWith(
expect.stringContaining(
`Response body: ${JSON.stringify(`${'x'.repeat(512)}...`)}`,
),
);
expect(mockDebugLogger.warn).not.toHaveBeenCalledWith(
expect.stringContaining('x'.repeat(600)),
);
});

it('treats parameterized GET SSE Accept headers as unsupported', async () => {
const fetchFn = vi
.fn<typeof fetch>()
.mockResolvedValue(new Response('bad method', { status: 400 }));
const fetchWithFallback = createStreamableHttpCompatibilityFetch(
'spring-ai',
fetchFn,
);

const response = await fetchWithFallback('http://test-server/mcp', {
method: 'GET',
headers: {
Accept: 'application/json, text/event-stream; charset=utf-8',
},
});

expect(response.status).toBe(405);
});

it('does not hide Streamable HTTP GET SSE server errors', async () => {
const fetchFn = vi
.fn<typeof fetch>()
.mockResolvedValue(new Response('server exploded', { status: 502 }));
const fetchWithFallback = createStreamableHttpCompatibilityFetch(
'server-error',
fetchFn,
);

const response = await fetchWithFallback('http://test-server/mcp', {
method: 'GET',
headers: { Accept: 'text/event-stream' },
});

expect(response.status).toBe(502);
});

it('does not rewrite the SDK-native GET SSE unsupported sentinel', async () => {
const fetchFn = vi
.fn<typeof fetch>()
.mockResolvedValue(
new Response('method not allowed', { status: 405 }),
);
const fetchWithFallback = createStreamableHttpCompatibilityFetch(
'native-unsupported',
fetchFn,
);

const response = await fetchWithFallback('http://test-server/mcp', {
method: 'GET',
headers: { Accept: 'text/event-stream' },
});

expect(response.status).toBe(405);
expect(await response.text()).toBe('method not allowed');
});

it('does not rewrite resumable GET SSE errors with Last-Event-ID', async () => {
const fetchFn = vi
.fn<typeof fetch>()
.mockResolvedValue(
new Response('{"error":"invalid cursor"}', { status: 400 }),
);
const fetchWithFallback = createStreamableHttpCompatibilityFetch(
'resume-error',
fetchFn,
);

const response = await fetchWithFallback('http://test-server/mcp', {
method: 'GET',
headers: {
Accept: 'text/event-stream',
'Last-Event-ID': 'event-123',
},
});

expect(response.status).toBe(400);
expect(await response.text()).toBe('{"error":"invalid cursor"}');
});

it('does not rewrite non-SSE GET responses', async () => {
const fetchFn = vi
.fn<typeof fetch>()
.mockResolvedValue(new Response('bad request', { status: 400 }));
const fetchWithFallback = createStreamableHttpCompatibilityFetch(
'plain-get',
fetchFn,
);

const response = await fetchWithFallback('http://test-server/mcp', {
method: 'GET',
headers: { Accept: 'application/json' },
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The three new tests cover GET+400, GET+502, and GET+non-SSE+400, but none verify that a POST request receiving a 400 response passes through unchanged. POST is the primary MCP JSON-RPC channel — a regression that accidentally rewrites POST responses would silently break all tool calls.

Suggested change
});
it('does not rewrite POST responses', async () => {
const fetchFn = vi
.fn<typeof fetch>()
.mockResolvedValue(new Response('bad request', { status: 400 }));
const fetchWithFallback = createStreamableHttpCompatibilityFetch(
'post-test',
fetchFn,
);
const response = await fetchWithFallback('http://test-server/mcp', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
});
expect(response.status).toBe(400);
});

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in ef4fe4e. The compatibility wrapper now has POST passthrough coverage: a POST request returning 400 remains 400, so the optional GET SSE fallback cannot accidentally rewrite the primary JSON-RPC channel.

expect(response.status).toBe(400);
});

it('does not rewrite POST responses', async () => {
const fetchFn = vi
.fn<typeof fetch>()
.mockResolvedValue(new Response('bad request', { status: 400 }));
const fetchWithFallback = createStreamableHttpCompatibilityFetch(
'post-test',
fetchFn,
);

const response = await fetchWithFallback('http://test-server/mcp', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
});

expect(response.status).toBe(400);
});
});

Expand Down Expand Up @@ -474,6 +671,8 @@ describe('mcp-client', () => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const authProvider = (transport as any)._authProvider;
expect(authProvider).toBeInstanceOf(GoogleCredentialProvider);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
expect((transport as any)._fetch).toEqual(expect.any(Function));
});

it('should use GoogleCredentialProvider with SSE transport', async () => {
Expand Down Expand Up @@ -512,6 +711,56 @@ describe('mcp-client', () => {
);
});
});

describe('authenticated Streamable HTTP compatibility fetch', () => {
it('wires the compatibility fetch for OAuth httpUrl transports', async () => {
const getValidToken = vi.fn().mockResolvedValue('oauth-token');
vi.mocked(MCPOAuthProvider).mockImplementation(
() =>
({
getValidToken,
}) as unknown as MCPOAuthProvider,
);

const transport = await createTransport(
'oauth-test-server',
{
httpUrl: 'http://test-server',
oauth: {
enabled: true,
clientId: 'client-id',
},
},
false,
);

expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
expect(getValidToken).toHaveBeenCalledWith('oauth-test-server', {
enabled: true,
clientId: 'client-id',
});
// eslint-disable-next-line @typescript-eslint/no-explicit-any
expect((transport as any)._fetch).toEqual(expect.any(Function));
});

it('wires the compatibility fetch for service account httpUrl transports', async () => {
const transport = await createTransport(
'service-account-test-server',
{
httpUrl: 'http://test-server',
authProviderType: AuthProviderType.SERVICE_ACCOUNT_IMPERSONATION,
targetAudience: 'client.apps.googleusercontent.com',
targetServiceAccount:
'service-account@example-project.iam.gserviceaccount.com',
},
false,
);

expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
expect((transport as any)._fetch).toEqual(expect.any(Function));
});
});
});
describe('isEnabled', () => {
const funcDecl = { name: 'myTool' };
Expand Down
Loading
Loading