Skip to content

Commit cf47f89

Browse files
committed
fix(mcp): treat tools/list timeouts and HTTP errors as an unavailable server; fix test route contexts
- Share the v2 discovery classification (timeout, connection, protocol, HTTP error) as isMcpServerUnavailableError and use it in the internal discover route - Restore the original POST refresh catch, which never sees per-server failures - Pass a route context in the new and existing oauth start and mask client tests; drop the oauth start test-types baseline entry
1 parent 897a0e0 commit cf47f89

7 files changed

Lines changed: 66 additions & 42 deletions

File tree

‎apps/sim/app/api/mcp/oauth/start/route.test.ts‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
permissionsMockFns,
1010
resetDbChainMock,
1111
} from '@sim/testing'
12+
import { createRouteContext } from '@sim/testing/helpers/http'
1213
import { NextRequest } from 'next/server'
1314
import { beforeEach, describe, expect, it, vi } from 'vitest'
1415

@@ -61,7 +62,7 @@ describe('MCP OAuth start route', () => {
6162
'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
6263
)
6364

64-
await GET(request)
65+
await GET(request, createRouteContext({}))
6566

6667
// The route must call the guarded wrapper (which defaults fetchFn to the
6768
// SSRF-guarded fetch internally) rather than the raw SDK `auth()` — see
@@ -82,7 +83,7 @@ describe('MCP OAuth start route', () => {
8283
'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
8384
)
8485

85-
const response = await GET(request)
86+
const response = await GET(request, createRouteContext({}))
8687

8788
expect(mcpOauthMockFns.mockMcpAuthGuarded).toHaveBeenCalledTimes(1)
8889
expect(response.status).toBe(504)
@@ -105,7 +106,7 @@ describe('MCP OAuth start route', () => {
105106
'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
106107
)
107108

108-
const response = await GET(request)
109+
const response = await GET(request, createRouteContext({}))
109110
const body = await response.json()
110111

111112
expect(response.status).toBe(409)
@@ -121,7 +122,7 @@ describe('MCP OAuth start route', () => {
121122
'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
122123
)
123124

124-
const response = await GET(request)
125+
const response = await GET(request, createRouteContext({}))
125126
const body = await response.json()
126127

127128
expect(response.status).toBe(500)
@@ -138,7 +139,7 @@ describe('MCP OAuth start route', () => {
138139
'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
139140
)
140141

141-
const response = await GET(request)
142+
const response = await GET(request, createRouteContext({}))
142143
const body = await response.json()
143144

144145
expect(response.status).toBe(422)
@@ -200,7 +201,8 @@ describe('MCP OAuth start route: authorization-server refusals', () => {
200201
GET(
201202
new NextRequest(
202203
'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
203-
)
204+
),
205+
createRouteContext({})
204206
)
205207

206208
it.each([

‎apps/sim/app/api/mcp/tools/discover/route.test.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ vi.mock('@/lib/mcp/middleware', () => ({
2424

2525
vi.mock('@/lib/mcp/service', () => mcpServiceMock)
2626

27+
import { StreamableHTTPError } from '@modelcontextprotocol/sdk/client/streamableHttp.js'
28+
import { ErrorCode, McpError } from '@modelcontextprotocol/sdk/types.js'
2729
import { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
2830
import { McpConnectionError, McpServerCooldownError } from '@/lib/mcp/types'
2931
import { GET } from '@/app/api/mcp/tools/discover/route'
@@ -56,6 +58,11 @@ describe('GET /api/mcp/tools/discover', () => {
5658
it.each([
5759
['cannot be reached', () => new McpConnectionError('connect ECONNREFUSED', 'Docs')],
5860
['is cooling down after a recent failure', () => new McpServerCooldownError('mcp-1')],
61+
['times out on tools/list', () => new McpError(ErrorCode.RequestTimeout, 'Request timed out')],
62+
[
63+
'answers tools/list with an HTTP error',
64+
() => new StreamableHTTPError(503, 'Service Unavailable'),
65+
],
5966
])('answers 422 when the registered server %s', async (_name, makeError) => {
6067
mcpServiceMockFns.mockDiscoverServerTools.mockRejectedValueOnce(makeError())
6168

‎apps/sim/app/api/mcp/tools/discover/route.ts‎

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,13 @@ import {
1212
} from '@/lib/mcp/middleware'
1313
import { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
1414
import { mcpService } from '@/lib/mcp/service'
15+
import { McpOauthAuthorizationRequiredError, type McpToolDiscoveryResponse } from '@/lib/mcp/types'
1516
import {
16-
McpConnectionError,
17-
McpOauthAuthorizationRequiredError,
18-
type McpToolDiscoveryResponse,
19-
} from '@/lib/mcp/types'
20-
import { categorizeError, createMcpErrorResponse, createMcpSuccessResponse } from '@/lib/mcp/utils'
17+
categorizeError,
18+
createMcpErrorResponse,
19+
createMcpSuccessResponse,
20+
isMcpServerUnavailableError,
21+
} from '@/lib/mcp/utils'
2122

2223
const logger = createLogger('McpToolDiscoveryAPI')
2324
const MCP_REFRESH_DISCOVERY_CONCURRENCY = 5
@@ -28,26 +29,30 @@ export const dynamic = 'force-dynamic'
2829
* Renders a discovery failure that is not an authorization problem.
2930
*
3031
* Discovery talks to a server the workspace registered, so a server that cannot be
31-
* reached, times out, or is cooling down after a recent failure is a fault of that
32-
* server rather than of Sim: it answers 422 (the request was valid; the dependency it
33-
* names cannot serve it), the status this API family already uses for an upstream that
34-
* cannot complete the operation, and logs at warn. Anything else keeps
35-
* {@link categorizeError}'s status and logs at error only when that status is a 5xx.
32+
* reached, times out on connect or on `tools/list`, answers with a protocol or HTTP
33+
* error, or is cooling down after a recent failure is a fault of that server rather
34+
* than of Sim: it answers 422 (the request was valid; the dependency it names cannot
35+
* serve it), the status this API family already uses for an upstream that cannot
36+
* complete the operation, and logs at warn. Anything else keeps {@link categorizeError}'s
37+
* status and logs at error only when that status is a 5xx.
3638
*/
37-
function discoveryFailureResponse(requestId: string, error: unknown, defaultMessage: string) {
38-
if (error instanceof McpConnectionError) {
39+
function discoveryFailureResponse(requestId: string, error: unknown) {
40+
if (isMcpServerUnavailableError(error)) {
3941
logger.warn(`[${requestId}] MCP server unavailable for tool discovery`, {
4042
error: getErrorMessage(error),
4143
})
42-
return createMcpErrorResponse(error, defaultMessage, 422)
44+
return createMcpErrorResponse(error, 'Failed to discover MCP tools', 422)
4345
}
4446
const { message, status } = categorizeError(error)
4547
if (status >= 500) {
46-
logger.error(`[${requestId}] ${defaultMessage}:`, error)
48+
logger.error(`[${requestId}] Error discovering MCP tools:`, error)
4749
} else {
48-
logger.warn(`[${requestId}] ${defaultMessage}`, { error: getErrorMessage(error), status })
50+
logger.warn(`[${requestId}] Failed to discover MCP tools`, {
51+
error: getErrorMessage(error),
52+
status,
53+
})
4954
}
50-
return createMcpErrorResponse(new Error(message), defaultMessage, status)
55+
return createMcpErrorResponse(new Error(message), 'Failed to discover MCP tools', status)
5156
}
5257

5358
async function settleWithConcurrency<T, R>(
@@ -134,7 +139,7 @@ export const GET = withRouteHandler(
134139
) {
135140
return createMcpErrorResponse(error, 'OAuth re-authorization required', 401)
136141
}
137-
return discoveryFailureResponse(requestId, error, 'Failed to discover MCP tools')
142+
return discoveryFailureResponse(requestId, error)
138143
}
139144
})
140145
)
@@ -199,7 +204,9 @@ export const POST = withRouteHandler(
199204
) {
200205
return createMcpErrorResponse(error, 'OAuth re-authorization required', 401)
201206
}
202-
return discoveryFailureResponse(requestId, error, 'Failed to refresh tool discovery')
207+
logger.error(`[${requestId}] Error refreshing tool discovery:`, error)
208+
const { message, status } = categorizeError(error)
209+
return createMcpErrorResponse(new Error(message), 'Failed to refresh tool discovery', status)
203210
}
204211
})
205212
)

‎apps/sim/app/api/v2/mcp-servers/utils.ts‎

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,12 @@
11
import { UnauthorizedError } from '@modelcontextprotocol/sdk/client/auth.js'
2-
import { StreamableHTTPError } from '@modelcontextprotocol/sdk/client/streamableHttp.js'
3-
import { McpError as McpSdkError } from '@modelcontextprotocol/sdk/types.js'
42
import { type V2McpServer, v2McpServerSchema } from '@/lib/api/contracts/v2/mcp-servers'
53
import { createV2ResourceConcealmentPolicy, type V2ErrorPolicy } from '@/lib/api/server/routes'
64
import { isTimeoutError } from '@/lib/core/execution-limits'
75
import { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
86
import { projectMcpHeaders } from '@/lib/mcp/projection'
97
import type { McpServerRow } from '@/lib/mcp/queries'
10-
import {
11-
McpConnectionError,
12-
McpOauthAuthorizationRequiredError,
13-
McpServerCooldownError,
14-
} from '@/lib/mcp/types'
8+
import { McpOauthAuthorizationRequiredError, McpServerCooldownError } from '@/lib/mcp/types'
9+
import { isMcpServerUnavailableError } from '@/lib/mcp/utils'
1510
import { v2Error } from '@/app/api/v2/lib/response'
1611

1712
/**
@@ -116,12 +111,7 @@ export const v2McpToolDiscoveryErrorPolicy = {
116111
)
117112
}
118113

119-
if (
120-
isTimeoutError(error) ||
121-
error instanceof McpConnectionError ||
122-
error instanceof McpSdkError ||
123-
error instanceof StreamableHTTPError
124-
) {
114+
if (isMcpServerUnavailableError(error)) {
125115
return v2Error('SERVICE_UNAVAILABLE', unreachableServerMessage(error))
126116
}
127117

‎apps/sim/lib/guardrails/mask-client.test.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { resetUrlsMock, urlsMockFns } from '@sim/testing'
2+
import { createRouteContext } from '@sim/testing/helpers/http'
23
import { authInternalMock, authInternalMockFns } from '@sim/testing/mocks/auth-internal.mock'
34
import { hybridAuthMockFns } from '@sim/testing/mocks/hybrid-auth.mock'
45
import { utilsHelpersMock, utilsHelpersMockFns } from '@sim/testing/mocks/utils-helpers.mock'
@@ -133,7 +134,8 @@ describe('maskPIIBatchViaHttp', () => {
133134
async (url: string, init: { body: string; headers: Record<string, string> }) => {
134135
if (url.startsWith('http://app.internal:3000')) {
135136
return maskBatchRoute(
136-
new NextRequest(url, { method: 'POST', body: init.body, headers: init.headers })
137+
new NextRequest(url, { method: 'POST', body: init.body, headers: init.headers }),
138+
createRouteContext({})
137139
)
138140
}
139141
const { texts } = JSON.parse(init.body) as { texts: string[] }

‎apps/sim/lib/mcp/utils.ts‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import { UnauthorizedError } from '@modelcontextprotocol/sdk/client/auth.js'
2+
import { StreamableHTTPError } from '@modelcontextprotocol/sdk/client/streamableHttp.js'
3+
import { McpError as McpSdkError } from '@modelcontextprotocol/sdk/types.js'
24
import { generateShortId } from '@sim/utils/id'
35
import { NextResponse } from 'next/server'
4-
import { DEFAULT_EXECUTION_TIMEOUT_MS } from '@/lib/core/execution-limits'
6+
import { DEFAULT_EXECUTION_TIMEOUT_MS, isTimeoutError } from '@/lib/core/execution-limits'
57
import {
68
type McpApiResponse,
79
McpConnectionError,
@@ -136,6 +138,23 @@ export function mcpOrchestrationStatus(errorCode: string | undefined): number {
136138
return 500
137139
}
138140

141+
/**
142+
* Reports whether a registered MCP server failed to answer usefully: it could not be
143+
* reached, timed out, is cooling down after a recent failure, or answered `tools/list`
144+
* with a protocol or HTTP error. Typed only, never matched on message text, because
145+
* `McpConnectionError` interpolates the server's display name into its message. Check
146+
* authorization errors first: `UnauthorizedError` is not one of these, but a caller that
147+
* wants to answer reauthorization separately must do so before asking.
148+
*/
149+
export function isMcpServerUnavailableError(error: unknown): boolean {
150+
return (
151+
isTimeoutError(error) ||
152+
error instanceof McpConnectionError ||
153+
error instanceof McpSdkError ||
154+
error instanceof StreamableHTTPError
155+
)
156+
}
157+
139158
/**
140159
* Enhanced error categorization for more specific HTTP status codes.
141160
* Returns safe, generic messages to prevent leaking internal details.

‎scripts/check-test-types.baseline.json‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -210,9 +210,6 @@
210210
"apps/sim/app/api/mcp/oauth/callback/route.test.ts": {
211211
"TS2554": 7
212212
},
213-
"apps/sim/app/api/mcp/oauth/start/route.test.ts": {
214-
"TS2554": 5
215-
},
216213
"apps/sim/app/api/mcp/serve/[serverId]/route.test.ts": {
217214
"TS2322": 2
218215
},

0 commit comments

Comments
 (0)