From f720323b5e9325389eb6195f4990d1154f7f6418 Mon Sep 17 00:00:00 2001 From: Travis Fischer Date: Thu, 12 Jun 2025 02:43:55 +0700 Subject: [PATCH] feat: improve mcp edge metadata and error handling --- .../src/__snapshots__/mcp-e2e.test.ts.snap | 32 +++++++ apps/e2e/src/http-e2e.test.ts | 9 +- apps/e2e/src/http-fixtures.ts | 1 - apps/e2e/src/mcp-e2e.test.ts | 36 ++++++-- apps/e2e/src/mcp-fixtures.ts | 88 ++++++++++++++----- apps/gateway/src/lib/durable-mcp-server.ts | 57 ++++++------ .../src/lib/handle-mcp-tool-call-error.ts | 35 +------- .../src/lib/resolve-origin-tool-call.ts | 4 +- ...http-response-to-mcp-tool-call-response.ts | 34 +++---- packages/core/src/rate-limit-headers.ts | 12 --- packages/hono/src/error-handler.ts | 2 +- packages/hono/src/header-utils.ts | 24 ++--- readme.md | 1 + 13 files changed, 193 insertions(+), 142 deletions(-) diff --git a/apps/e2e/src/__snapshots__/mcp-e2e.test.ts.snap b/apps/e2e/src/__snapshots__/mcp-e2e.test.ts.snap index efa89c6e..cde27015 100644 --- a/apps/e2e/src/__snapshots__/mcp-e2e.test.ts.snap +++ b/apps/e2e/src/__snapshots__/mcp-e2e.test.ts.snap @@ -1,5 +1,37 @@ // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html +exports[`Basic MCP => OpenAPI @ 010332cf get_post success > 2.0: @dev/test-basic-openapi@010332cf/mcp get_post 1`] = ` +{ + "content": [], + "isError": false, + "structuredContent": { + "body": "dignissimos aperiam dolorem qui eum +facilis quibusdam animi sint suscipit qui sint possimus cum +quaerat magni maiores excepturi +ipsam ut commodi dolor voluptatum modi aut vitae", + "id": 8, + "title": "dolorem dolore est ipsam", + "userId": 1, + }, +} +`; + +exports[`Basic MCP => OpenAPI @ latest get_post success > 1.0: @dev/test-basic-openapi@latest/mcp get_post 1`] = ` +{ + "content": [], + "isError": false, + "structuredContent": { + "body": "et iusto sed quo iure +voluptatem occaecati omnis eligendi aut ad +voluptatem doloribus vel accusantium quis pariatur +molestiae porro eius odio et labore et velit aut", + "id": 3, + "title": "ea molestias quasi exercitationem repellat qui ipsa sit aut", + "userId": 1, + }, +} +`; + exports[`Basic MCP => OpenAPI get_post success > 0.0: @dev/test-basic-openapi/mcp get_post 1`] = ` { "content": [], diff --git a/apps/e2e/src/http-e2e.test.ts b/apps/e2e/src/http-e2e.test.ts index 9526fb5a..3814f3fe 100644 --- a/apps/e2e/src/http-e2e.test.ts +++ b/apps/e2e/src/http-e2e.test.ts @@ -36,10 +36,15 @@ for (const [i, fixtureSuite] of fixtureSuites.entries()) { fixture.response?.snapshot ?? fixtureSuite.snapshot ?? (status >= 200 && status < 300) - const debugFixture = !!(fixture.debug ?? fixtureSuite.debug) + const debugFixture = !!( + fixture.debug ?? + fixtureSuite.debug ?? + fixture.only ?? + fixtureSuite.only + ) const fixtureName = `${i}.${j}: ${method} ${fixture.path}` - let testFn = fixture.only ? test.only : test + let testFn = (fixture.only ?? fixture.debug) ? test.only : test if (fixtureSuite.sequential) { testFn = testFn.sequential } diff --git a/apps/e2e/src/http-fixtures.ts b/apps/e2e/src/http-fixtures.ts index c70844e1..57289587 100644 --- a/apps/e2e/src/http-fixtures.ts +++ b/apps/e2e/src/http-fixtures.ts @@ -111,7 +111,6 @@ export const fixtureSuites: E2ETestFixtureSuite[] = [ fixtures: [ { path: '@dev/test-basic-openapi/getPost', - response: { // Missing `postId` parameter. status: 400 diff --git a/apps/e2e/src/mcp-e2e.test.ts b/apps/e2e/src/mcp-e2e.test.ts index 8d1557c0..dc3fdffc 100644 --- a/apps/e2e/src/mcp-e2e.test.ts +++ b/apps/e2e/src/mcp-e2e.test.ts @@ -33,14 +33,22 @@ for (const [i, fixtureSuite] of fixtureSuites.entries()) { const { isError, result: expectedResult, + content: expectedContent, + structuredContent: expectedStructuredContent, + _meta: expectedMeta, validate } = fixture.response ?? {} const snapshot = fixture.response?.snapshot ?? fixtureSuite.snapshot ?? !isError - const debugFixture = !!(fixture.debug ?? fixtureSuite.debug) + const debugFixture = !!( + fixture.debug ?? + fixtureSuite.debug ?? + fixture.only ?? + fixtureSuite.only + ) const fixtureName = `${i}.${j}: ${fixtureSuite.path} ${fixture.request.name}` - let testFn = fixture.only ? test.only : test + let testFn = (fixture.only ?? fixture.debug) ? test.only : test if (fixtureSuite.sequential) { testFn = testFn.sequential } @@ -60,6 +68,11 @@ for (const [i, fixtureSuite] of fixtureSuites.entries()) { name: fixture.request.name, arguments: fixture.request.args }) + + if (debugFixture) { + console.log(fixtureName, '=>', result) + } + if (isError) { expect(result.isError).toBeTruthy() } else { @@ -70,6 +83,21 @@ for (const [i, fixtureSuite] of fixtureSuites.entries()) { expect(result).toEqual(expectedResult) } + if (expectedContent) { + expect(result.content).toEqual(expectedContent) + } + + if (expectedStructuredContent) { + expect(result.structuredContent).toEqual(expectedStructuredContent) + } + + if (expectedMeta) { + expect(result._meta).toBeDefined() + for (const [key, value] of Object.entries(expectedMeta)) { + expect(result._meta![key]).toEqual(value) + } + } + if (snapshot) { expect(result).toMatchSnapshot() } @@ -85,10 +113,6 @@ for (const [i, fixtureSuite] of fixtureSuites.entries()) { expect(result).toEqual(fixtureResult) } } - - if (debugFixture) { - console.log(fixtureName, '=>', result) - } } ) } diff --git a/apps/e2e/src/mcp-fixtures.ts b/apps/e2e/src/mcp-fixtures.ts index 16e54ca4..9412e7bf 100644 --- a/apps/e2e/src/mcp-fixtures.ts +++ b/apps/e2e/src/mcp-fixtures.ts @@ -14,8 +14,11 @@ export type MCPE2ETestFixture = { } response?: { - isError?: boolean result?: any + isError?: boolean + content?: Array> + structuredContent?: any + _meta?: Record validate?: (result: any) => void | Promise /** @default true */ snapshot?: boolean @@ -60,6 +63,34 @@ export const fixtureSuites: MCPE2ETestFixtureSuite[] = [ } ] }, + { + title: 'Basic MCP => OpenAPI @ latest get_post success ', + path: '@dev/test-basic-openapi@latest/mcp', + fixtures: [ + { + request: { + name: 'get_post', + args: { + postId: 3 + } + } + } + ] + }, + { + title: 'Basic MCP => OpenAPI @ 010332cf get_post success ', + path: '@dev/test-basic-openapi@010332cf/mcp', + fixtures: [ + { + request: { + name: 'get_post', + args: { + postId: 8 + } + } + } + ] + }, { title: 'Basic MCP => MCP "echo" tool call success', path: '@dev/test-basic-mcp/mcp', @@ -75,14 +106,12 @@ export const fixtureSuites: MCPE2ETestFixtureSuite[] = [ } }, response: { - result: { - content: [ - { - type: 'text', - text: JSON.stringify({ nala: 'kitten', num: 123, now }) - } - ] - } + content: [ + { + type: 'text', + text: JSON.stringify({ nala: 'kitten', num: 123, now }) + } + ] } }, { @@ -95,18 +124,37 @@ export const fixtureSuites: MCPE2ETestFixtureSuite[] = [ } }, response: { - result: { - content: [ - { - type: 'text', - text: JSON.stringify({ - nala: 'kitten', - num: 123, - now: `${now}` - }) - } - ] + content: [ + { + type: 'text', + text: JSON.stringify({ + nala: 'kitten', + num: 123, + now: `${now}` + }) + } + ] + } + } + ] + }, + { + title: 'Basic MCP => MCP "echo" tool call errors', + path: '@dev/test-basic-openapi/mcp', + snapshot: false, + only: true, + fixtures: [ + { + request: { + name: 'get_post', + args: { + nala: 'kitten', + num: 123, + now } + }, + response: { + isError: true } } ] diff --git a/apps/gateway/src/lib/durable-mcp-server.ts b/apps/gateway/src/lib/durable-mcp-server.ts index 916f5f8f..5b754684 100644 --- a/apps/gateway/src/lib/durable-mcp-server.ts +++ b/apps/gateway/src/lib/durable-mcp-server.ts @@ -107,52 +107,49 @@ export class DurableMcpServerBase extends McpAgent< waitUntil: this.ctx.waitUntil.bind(this.ctx) }) - const { - originResponse, - toolCallResponse: resolvedToolCallResponse, - rateLimitResult - } = resolvedOriginToolCallResult - - if (originResponse) { + if (resolvedOriginToolCallResult.originResponse) { toolCallResponse = await transformHttpResponseToMcpToolCallResponse({ - tool, - ...resolvedOriginToolCallResult + ...resolvedOriginToolCallResult, + tool }) - } else if (resolvedToolCallResponse) { - if (resolvedToolCallResponse._meta || rateLimitResult) { - toolCallResponse = { - ...resolvedToolCallResponse, - _meta: { - ...resolvedToolCallResponse._meta, - ...pruneEmpty({ - headers: rateLimitResult - ? getRateLimitHeaders(rateLimitResult) - : undefined - }) - } - } - } else { - toolCallResponse = resolvedToolCallResponse - } } else { - assert(false, 500) + toolCallResponse = resolvedOriginToolCallResult.toolCallResponse + assert(toolCallResponse, 500, 'Missing tool call response') } - assert(toolCallResponse, 500, 'Missing tool call response') return toolCallResponse } catch (err: unknown) { // Gracefully handle tool call exceptions, whether they're thrown by the // origin or internally by the gateway. toolCallResponse = handleMcpToolCallError(err, { - deployment, - consumer, toolName, - sessionId, env: this.env }) return toolCallResponse } finally { + assert(toolCallResponse, 500, 'Missing tool call response') + + // Augment the MCP tool call response with agentic metadata, which + // makes it easier to debug tool calls and adds some much-needed HTTP + // header-like functionality to tool call responses. + toolCallResponse._meta = { + ...toolCallResponse._meta, + agentic: pruneEmpty({ + ...(toolCallResponse._meta?.agentic as any), + deploymentId: deployment.id, + consumerId: consumer?.id, + cacheStatus: resolvedOriginToolCallResult?.cacheStatus, + toolName, + headers: { + ...(toolCallResponse._meta?.agentic as any)?.headers, + ...getRateLimitHeaders( + resolvedOriginToolCallResult?.rateLimitResult + ) + } + }) + } + // Record tool call usage, whether the call was successful or not. recordToolCallUsage({ ...this.props, diff --git a/apps/gateway/src/lib/handle-mcp-tool-call-error.ts b/apps/gateway/src/lib/handle-mcp-tool-call-error.ts index de387573..964a5fd1 100644 --- a/apps/gateway/src/lib/handle-mcp-tool-call-error.ts +++ b/apps/gateway/src/lib/handle-mcp-tool-call-error.ts @@ -1,44 +1,25 @@ -import type { AdminDeployment } from '@agentic/platform-types' import type { ContentfulStatusCode } from 'hono/utils/http-status' -import { - getRateLimitHeaders, - HttpError, - pruneEmpty -} from '@agentic/platform-core' +import { HttpError } from '@agentic/platform-core' import * as Sentry from '@sentry/cloudflare' import { HTTPException } from 'hono/http-exception' import { HTTPError } from 'ky' import type { RawEnv } from './env' -import type { - AdminConsumer, - McpToolCallResponse, - RateLimitResult -} from './types' +import type { McpToolCallResponse } from './types' /** * Turns a thrown error into an MCP error tool call response, and attempts to * capture as much context as possible for potential debugging. * - * @note This function is synchronous and must never throw. + * @note This function is synchronous and should never throw. */ export function handleMcpToolCallError( err: any, { - deployment, - consumer, toolName, - sessionId, - requestId, - rateLimitResult, env }: { - deployment: AdminDeployment - consumer?: AdminConsumer toolName: string - sessionId: string - requestId?: string - rateLimitResult?: RateLimitResult env: RawEnv } ): McpToolCallResponse { @@ -47,16 +28,6 @@ export function handleMcpToolCallError( let status: ContentfulStatusCode = 500 const res: McpToolCallResponse = { - _meta: pruneEmpty({ - deploymentId: deployment.id, - consumerId: consumer?.id, - toolName, - sessionId, - requestId, - headers: rateLimitResult - ? getRateLimitHeaders(rateLimitResult) - : undefined - }), isError: true, content: [ { diff --git a/apps/gateway/src/lib/resolve-origin-tool-call.ts b/apps/gateway/src/lib/resolve-origin-tool-call.ts index 05d493c5..bcf662f6 100644 --- a/apps/gateway/src/lib/resolve-origin-tool-call.ts +++ b/apps/gateway/src/lib/resolve-origin-tool-call.ts @@ -276,7 +276,7 @@ export async function resolveOriginToolCall({ body: JSON.stringify({ name: tool.name, args: toolCallArgs, - metadata: originMcpRequestMetadata! + metadata: originMcpRequestMetadata }) }) @@ -301,7 +301,7 @@ export async function resolveOriginToolCall({ const toolCallResponseString = await originMcpClient.callTool({ name: tool.name, args: toolCallArgs, - metadata: originMcpRequestMetadata! + metadata: originMcpRequestMetadata }) const toolCallResponse = JSON.parse( toolCallResponseString diff --git a/apps/gateway/src/lib/transform-http-response-to-mcp-tool-call-response.ts b/apps/gateway/src/lib/transform-http-response-to-mcp-tool-call-response.ts index 98ea263d..b92199f9 100644 --- a/apps/gateway/src/lib/transform-http-response-to-mcp-tool-call-response.ts +++ b/apps/gateway/src/lib/transform-http-response-to-mcp-tool-call-response.ts @@ -1,36 +1,26 @@ import type { Tool } from '@agentic/platform-types' -import { - assert, - getRateLimitHeaders, - HttpError, - pruneEmpty -} from '@agentic/platform-core' +import { assert, HttpError } from '@agentic/platform-core' import contentType from 'fast-content-type-parse' -import type { - McpToolCallResponse, - RateLimitResult, - ToolCallArgs -} from './types' +import type { McpToolCallResponse, ToolCallArgs } from './types' import { cfValidateJsonSchema } from './cf-validate-json-schema' export async function transformHttpResponseToMcpToolCallResponse({ originRequest, originResponse, tool, - toolCallArgs, - rateLimitResult + toolCallArgs }: { originRequest: Request originResponse: Response tool: Tool toolCallArgs: ToolCallArgs - rateLimitResult?: RateLimitResult }) { const { type: mimeType } = contentType.safeParse( originResponse.headers.get('content-type') || 'application/octet-stream' ) + // TODO: move these logs should be higher up // eslint-disable-next-line no-console console.log('httpOriginResponse', { tool: tool.name, @@ -41,8 +31,7 @@ export async function transformHttpResponseToMcpToolCallResponse({ mimeType, status: originResponse.status // headers: Object.fromEntries(originResponse.headers.entries()) - }, - rateLimitResult + } }) if (originResponse.status >= 400) { @@ -62,22 +51,19 @@ export async function transformHttpResponseToMcpToolCallResponse({ status: originResponse.status, // headers: Object.fromEntries(originResponse.headers.entries()), message - }, - rateLimitResult + } }) throw new HttpError({ statusCode: originResponse.status, message, - cause: originResponse, - headers: getRateLimitHeaders(rateLimitResult) + cause: originResponse }) } - const result: McpToolCallResponse = pruneEmpty({ - isError: originResponse.status >= 400, - _meta: getRateLimitHeaders(rateLimitResult) - }) + const result: McpToolCallResponse = { + isError: originResponse.status >= 400 + } if (tool.outputSchema) { assert( diff --git a/packages/core/src/rate-limit-headers.ts b/packages/core/src/rate-limit-headers.ts index 2806276e..0e48486f 100644 --- a/packages/core/src/rate-limit-headers.ts +++ b/packages/core/src/rate-limit-headers.ts @@ -26,15 +26,3 @@ export function getRateLimitHeaders( return headers } - -export function applyRateLimitHeaders( - rateLimitResult: RateLimitResult, - res: Response -) { - const rateLimitHeaders = getRateLimitHeaders(rateLimitResult) - if (!rateLimitHeaders) return - - for (const [key, value] of Object.entries(rateLimitHeaders)) { - res.headers.set(key, value) - } -} diff --git a/packages/hono/src/error-handler.ts b/packages/hono/src/error-handler.ts index a4cea0a2..499cf599 100644 --- a/packages/hono/src/error-handler.ts +++ b/packages/hono/src/error-handler.ts @@ -16,7 +16,7 @@ import { * Hono error handler that sanitizes all types of internal, http, json-rpc, and * unexpected errors and responds with an appropate HTTP Response. * - * @note This function is synchronous and must never throw. + * @note This function is synchronous and should never throw. */ export function errorHandler( err: Error | HTTPResponseError, diff --git a/packages/hono/src/header-utils.ts b/packages/hono/src/header-utils.ts index 289d414e..aa313245 100644 --- a/packages/hono/src/header-utils.ts +++ b/packages/hono/src/header-utils.ts @@ -3,18 +3,6 @@ import { type RateLimitResult } from '@agentic/platform-core' -export function applyRateLimitHeaders({ - res, - rateLimitResult -}: { - res: Response - rateLimitResult?: RateLimitResult -}) { - const rateLimitHeaders = getRateLimitHeaders(rateLimitResult) - - applyHeaders({ res, headers: rateLimitHeaders }) -} - export function applyHeaders({ res, headers @@ -28,3 +16,15 @@ export function applyHeaders({ res.headers.set(key, value) } } + +export function applyRateLimitHeaders({ + res, + rateLimitResult +}: { + res: Response + rateLimitResult?: RateLimitResult +}) { + const rateLimitHeaders = getRateLimitHeaders(rateLimitResult) + + applyHeaders({ res, headers: rateLimitHeaders }) +} diff --git a/readme.md b/readme.md index bf376894..4df4bd95 100644 --- a/readme.md +++ b/readme.md @@ -42,6 +42,7 @@ - **Public MCP server interface** - how does oauth work with this flow? - proper error handling support within this flow; will currently get generic errors + - pass requestId to DurableMcpServer somehow on a per-request basis - **Origin MCP servers** - how to guarantee that the request is coming from agentic? - `_meta` for tool calls