Skip to content

Commit e3ce679

Browse files
waleedlatif1waleedlatif1
authored
fix(mcp): answer workflow MCP JSON-RPC errors over HTTP 200 instead of session-loss statuses (#9561)
* fix(mcp): answer workflow MCP JSON-RPC errors over HTTP 200 instead of session-loss statuses * fix(mcp): build the routed test request from a standard Request
1 parent 5216fd4 commit e3ce679

2 files changed

Lines changed: 151 additions & 46 deletions

File tree

‎apps/sim/app/api/mcp/serve/[serverId]/route.test.ts‎

Lines changed: 113 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@
22
* Tests for MCP serve route auth propagation.
33
*/
44

5+
import { Client } from '@modelcontextprotocol/sdk/client/index.js'
6+
import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'
7+
import { ErrorCode, McpError } from '@modelcontextprotocol/sdk/types.js'
58
import { createRouteContext } from '@sim/testing/helpers/http'
69
import { authInternalMock } from '@sim/testing/mocks/auth-internal.mock'
710
import {
@@ -18,7 +21,7 @@ import {
1821
import { hybridAuthMockFns } from '@sim/testing/mocks/hybrid-auth.mock'
1922
import { permissionsMock, permissionsMockFns } from '@sim/testing/mocks/permissions.mock'
2023
import { NextRequest } from 'next/server'
21-
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
24+
import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
2225

2326
const {
2427
mockExecuteWorkflowService,
@@ -101,16 +104,14 @@ const SERVER_RESOURCE = 'http://localhost:3000/api/mcp/serve/server-1'
101104
const SERVER_RESOURCE_METADATA =
102105
'http://localhost:3000/.well-known/oauth-protected-resource/api/mcp/serve/server-1'
103106

104-
function toolCallRequest(headers: Record<string, string>) {
107+
function toolCallRequest(
108+
headers: Record<string, string>,
109+
params: Record<string, unknown> = { name: 'tool_a', arguments: { q: 'test' } }
110+
) {
105111
return new NextRequest(SERVER_RESOURCE, {
106112
method: 'POST',
107113
headers: { Accept: 'application/json', ...headers },
108-
body: JSON.stringify({
109-
jsonrpc: '2.0',
110-
id: 1,
111-
method: 'tools/call',
112-
params: { name: 'tool_a', arguments: { q: 'test' } },
113-
}),
114+
body: JSON.stringify({ jsonrpc: '2.0', id: 1, method: 'tools/call', params }),
114115
})
115116
}
116117

@@ -1454,6 +1455,110 @@ describe('MCP Serve Route', () => {
14541455
expect(body.error.message).toContain('tools/list response is too large')
14551456
})
14561457

1458+
describe('JSON-RPC errors answering a well-formed request', () => {
1459+
const PUBLIC_SERVER = {
1460+
id: 'server-1',
1461+
name: 'Public Server',
1462+
workspaceId: 'ws-1',
1463+
isPublic: true,
1464+
createdBy: 'owner-1',
1465+
}
1466+
const DEPLOYED_TOOL_LOOKUPS = [
1467+
[PUBLIC_SERVER],
1468+
[{ toolName: 'tool_a', workflowId: 'wf-1' }],
1469+
[{ workspaceId: 'ws-1', deploymentVersionId: 'deployment-1' }],
1470+
]
1471+
const sdkClients: Client[] = []
1472+
1473+
afterEach(async () => {
1474+
await Promise.all(sdkClients.splice(0).map((client) => client.close()))
1475+
})
1476+
1477+
/** Drives the route with the real SDK client, whose transport reads HTTP 400/404 as a lost session. */
1478+
async function connectSdkClient() {
1479+
dbChainMockFns.limit.mockResolvedValue([PUBLIC_SERVER])
1480+
const routeFetch = async (url: string | URL, init?: RequestInit) => {
1481+
const request = new NextRequest(new Request(url, init))
1482+
const context = createRouteContext({ serverId: 'server-1' })
1483+
return request.method === 'GET' ? GET(request, context) : POST(request, context)
1484+
}
1485+
const client = new Client({ name: 'route-test', version: '1.0.0' })
1486+
sdkClients.push(client)
1487+
await client.connect(
1488+
new StreamableHTTPClientTransport(new URL(SERVER_RESOURCE), { fetch: routeFetch })
1489+
)
1490+
return client
1491+
}
1492+
1493+
it('delivers MethodNotFound as a JSON-RPC error the SDK client can read', async () => {
1494+
const client = await connectSdkClient()
1495+
const failure = await client.listPrompts().catch((error: unknown) => error)
1496+
1497+
expect(failure).toBeInstanceOf(McpError)
1498+
expect(failure).toMatchObject({ code: ErrorCode.MethodNotFound })
1499+
})
1500+
1501+
it.each([
1502+
{
1503+
failure: 'tools/call params without a tool name',
1504+
lookups: [[PUBLIC_SERVER]],
1505+
params: { name: '' },
1506+
code: ErrorCode.InvalidParams,
1507+
},
1508+
{
1509+
failure: 'an unknown tool',
1510+
lookups: [[PUBLIC_SERVER], []],
1511+
code: ErrorCode.InvalidParams,
1512+
},
1513+
{
1514+
failure: 'an undeployed workflow',
1515+
lookups: [
1516+
[PUBLIC_SERVER],
1517+
[{ toolName: 'tool_a', workflowId: 'wf-1' }],
1518+
[{ workspaceId: 'ws-1', deploymentVersionId: null }],
1519+
],
1520+
code: ErrorCode.InternalError,
1521+
},
1522+
{
1523+
failure: 'a workflow archived after the tool lookup',
1524+
lookups: DEPLOYED_TOOL_LOOKUPS,
1525+
serviceStatus: 404,
1526+
code: ErrorCode.InternalError,
1527+
},
1528+
{
1529+
failure: 'tool arguments the workflow rejects',
1530+
lookups: DEPLOYED_TOOL_LOOKUPS,
1531+
serviceStatus: 400,
1532+
code: ErrorCode.InvalidParams,
1533+
},
1534+
])(
1535+
'answers $failure over HTTP 200 to a JSON-only client',
1536+
async ({ lookups, params, serviceStatus, code }) => {
1537+
for (const rows of lookups) dbChainMockFns.limit.mockResolvedValueOnce(rows)
1538+
if (serviceStatus) {
1539+
mockExecuteWorkflowService.mockResolvedValueOnce({
1540+
ok: false,
1541+
failure: { kind: 'precheck', statusCode: serviceStatus, message: 'Refused' },
1542+
})
1543+
}
1544+
1545+
const response = await POST(
1546+
toolCallRequest({}, params),
1547+
createRouteContext({ serverId: 'server-1' })
1548+
)
1549+
const body = await response.json()
1550+
1551+
expect(response.status).toBe(200)
1552+
expect(body.error.code).toBe(code)
1553+
if (serviceStatus) {
1554+
expect(body.error.data.httpStatus).toBe(serviceStatus)
1555+
} else {
1556+
expect(mockExecuteWorkflowService).not.toHaveBeenCalled()
1557+
}
1558+
}
1559+
)
1560+
})
1561+
14571562
describe('initialize protocol version negotiation', () => {
14581563
async function callInitialize(protocolVersion?: string) {
14591564
dbChainMockFns.limit.mockResolvedValueOnce([

‎apps/sim/app/api/mcp/serve/[serverId]/route.ts‎

Lines changed: 38 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -139,8 +139,25 @@ function createError(
139139
}
140140
}
141141

142+
const SESSION_SIGNAL_STATUSES = new Set([400, 401, 404])
143+
144+
/**
145+
* Answers a well-formed request with a JSON-RPC error. Streamable HTTP clients read a 400, 401,
146+
* or 404 as a lost or unauthenticated session and reconnect instead of surfacing the error, so
147+
* those go out at 200, as the SDK server sends them; every other status keeps its limit, retry,
148+
* or alarm meaning. A malformed message has no request to answer and keeps its 400.
149+
*/
150+
function jsonRpcErrorResponse(
151+
error: JSONRPCError,
152+
init: { status?: number; headers?: HeadersInit } = {}
153+
): NextResponse {
154+
const status =
155+
init.status === undefined || SESSION_SIGNAL_STATUSES.has(init.status) ? 200 : init.status
156+
return NextResponse.json(error, { status, headers: init.headers })
157+
}
158+
142159
function clientCancelledJsonRpcResponse(id: RequestId): NextResponse {
143-
return NextResponse.json(
160+
return jsonRpcErrorResponse(
144161
createError(id, ErrorCode.ConnectionClosed, 'Client cancelled request'),
145162
{
146163
status: 499,
@@ -753,11 +770,8 @@ export const POST = withRouteHandler(
753770

754771
const paramsValidation = mcpToolCallParamsSchema.safeParse(rpcParams)
755772
if (!paramsValidation.success) {
756-
return NextResponse.json(
757-
createError(id, ErrorCode.InvalidParams, 'Invalid tool call parameters'),
758-
{
759-
status: 400,
760-
}
773+
return jsonRpcErrorResponse(
774+
createError(id, ErrorCode.InvalidParams, 'Invalid tool call parameters')
761775
)
762776
}
763777

@@ -780,11 +794,8 @@ export const POST = withRouteHandler(
780794
}
781795

782796
default:
783-
return NextResponse.json(
784-
createError(id, ErrorCode.MethodNotFound, `Method not found: ${method}`),
785-
{
786-
status: 404,
787-
}
797+
return jsonRpcErrorResponse(
798+
createError(id, ErrorCode.MethodNotFound, `Method not found: ${method}`)
788799
)
789800
}
790801
} catch (error) {
@@ -804,7 +815,7 @@ async function handleToolsList(
804815
try {
805816
const duplicateToolName = await getDuplicateToolName(serverId)
806817
if (duplicateToolName) {
807-
return NextResponse.json(
818+
return jsonRpcErrorResponse(
808819
createError(id, ErrorCode.InvalidRequest, 'MCP server has duplicate tool names', {
809820
code: 'duplicate_tool_name',
810821
toolName: duplicateToolName,
@@ -920,7 +931,7 @@ async function handleToolsList(
920931
maxBytes: error.maxBytes,
921932
observedBytes: error.observedBytes,
922933
})
923-
return NextResponse.json(
934+
return jsonRpcErrorResponse(
924935
createError(id, ErrorCode.InternalError, 'MCP tools/list response is too large', {
925936
code: 'payload_too_large',
926937
maxBytes: error.maxBytes,
@@ -931,7 +942,7 @@ async function handleToolsList(
931942
)
932943
}
933944
logger.error('Error listing tools:', error)
934-
return NextResponse.json(createError(id, ErrorCode.InternalError, 'Failed to list tools'), {
945+
return jsonRpcErrorResponse(createError(id, ErrorCode.InternalError, 'Failed to list tools'), {
935946
status: 500,
936947
})
937948
}
@@ -941,19 +952,14 @@ async function handleToolsCall(
941952
id: RequestId,
942953
serverId: string,
943954
serverWorkspaceId: string,
944-
params: { name: string; arguments?: Record<string, unknown> } | undefined,
955+
params: { name: string; arguments?: Record<string, unknown> },
945956
executeAuthContext?: ExecuteAuthContext | null,
946957
publicServerOwnerId?: string,
947958
simViaHeader?: string | null,
948959
requestSignal?: AbortSignal
949960
): Promise<NextResponse> {
950961
let abortSignal: ManagedAbortSignal | null = null
951962
try {
952-
if (!params?.name) {
953-
return NextResponse.json(createError(id, ErrorCode.InvalidParams, 'Tool name required'), {
954-
status: 400,
955-
})
956-
}
957963
abortSignal = createManagedAbortSignal(
958964
requestSignal ?? new AbortController().signal,
959965
getMaxExecutionTimeout()
@@ -979,7 +985,7 @@ async function handleToolsCall(
979985
const abortedAfterToolLookup = callerAbortedJsonRpcResponse(id, abortSignal)
980986
if (abortedAfterToolLookup) return abortedAfterToolLookup
981987
if (matchingTools.length > 1) {
982-
return NextResponse.json(
988+
return jsonRpcErrorResponse(
983989
createError(id, ErrorCode.InvalidRequest, `Duplicate tool name: ${params.name}`, {
984990
code: 'duplicate_tool_name',
985991
toolName: params.name,
@@ -990,11 +996,8 @@ async function handleToolsCall(
990996
}
991997
const [tool] = matchingTools
992998
if (!tool) {
993-
return NextResponse.json(
994-
createError(id, ErrorCode.InvalidParams, `Tool not found: ${params.name}`),
995-
{
996-
status: 404,
997-
}
999+
return jsonRpcErrorResponse(
1000+
createError(id, ErrorCode.InvalidParams, `Tool not found: ${params.name}`)
9981001
)
9991002
}
10001003

@@ -1023,11 +1026,8 @@ async function handleToolsCall(
10231026
* the deploy status GET route.
10241027
*/
10251028
if (!wf?.deploymentVersionId) {
1026-
return NextResponse.json(
1027-
createError(id, ErrorCode.InternalError, 'Workflow is not deployed'),
1028-
{
1029-
status: 400,
1030-
}
1029+
return jsonRpcErrorResponse(
1030+
createError(id, ErrorCode.InternalError, 'Workflow is not deployed')
10311031
)
10321032
}
10331033

@@ -1036,7 +1036,7 @@ async function handleToolsCall(
10361036
serverId,
10371037
workflowId: tool.workflowId,
10381038
})
1039-
return NextResponse.json(
1039+
return jsonRpcErrorResponse(
10401040
createError(id, ErrorCode.InternalError, 'Workflow is unavailable for this MCP server'),
10411041
{ status: 403 }
10421042
)
@@ -1103,7 +1103,7 @@ async function handleToolsCall(
11031103
Math.ceil(failure.retryAfterMs / 1000)
11041104
).toString()
11051105
}
1106-
return NextResponse.json(
1106+
return jsonRpcErrorResponse(
11071107
createError(
11081108
id,
11091109
getWorkflowErrorCode(failure.statusCode, { code: failure.code }),
@@ -1125,7 +1125,7 @@ async function handleToolsCall(
11251125
}
11261126

11271127
if (serviceResult.aborted === 'client') {
1128-
return NextResponse.json(
1128+
return jsonRpcErrorResponse(
11291129
createError(id, ErrorCode.ConnectionClosed, 'Client cancelled request', {
11301130
httpStatus: 499,
11311131
retryable: false,
@@ -1136,7 +1136,7 @@ async function handleToolsCall(
11361136
}
11371137

11381138
if (serviceResult.aborted === 'timeout') {
1139-
return NextResponse.json(
1139+
return jsonRpcErrorResponse(
11401140
createError(
11411141
id,
11421142
ErrorCode.InternalError,
@@ -1181,7 +1181,7 @@ async function handleToolsCall(
11811181
)
11821182
} catch (error) {
11831183
if (abortSignal?.isTimedOut()) {
1184-
return NextResponse.json(
1184+
return jsonRpcErrorResponse(
11851185
createError(id, ErrorCode.InternalError, 'Tool execution timed out', {
11861186
code: 'timeout',
11871187
retryable: true,
@@ -1199,7 +1199,7 @@ async function handleToolsCall(
11991199
observedBytes: error.observedBytes,
12001200
label: error.label,
12011201
})
1202-
return NextResponse.json(
1202+
return jsonRpcErrorResponse(
12031203
createError(
12041204
id,
12051205
error.label === 'MCP workflow execution request body'
@@ -1217,7 +1217,7 @@ async function handleToolsCall(
12171217
)
12181218
}
12191219
logger.error('Error calling tool:', error)
1220-
return NextResponse.json(createError(id, ErrorCode.InternalError, 'Tool execution failed'), {
1220+
return jsonRpcErrorResponse(createError(id, ErrorCode.InternalError, 'Tool execution failed'), {
12211221
status: 500,
12221222
})
12231223
} finally {

0 commit comments

Comments
 (0)