From d339f0c3d9f1db4135c63e7b52c2495e5b3c785f Mon Sep 17 00:00:00 2001 From: Ben Senescu <44480372+bensenescu@users.noreply.github.com> Date: Tue, 16 Jun 2026 13:52:59 -0400 Subject: [PATCH] Fix MCP output validation and error reporting --- README.md | 2 + src/server/mcp/instrumentation.test.ts | 88 +++++++++++++ src/server/mcp/instrumentation.ts | 85 +++++++++++++ src/server/mcp/output-schemas.ts | 8 +- src/server/mcp/server.ts | 118 +++++++++++++++--- .../tools/output-schema-validation.test.ts | 53 ++++++++ 6 files changed, 335 insertions(+), 19 deletions(-) create mode 100644 src/server/mcp/instrumentation.test.ts create mode 100644 src/server/mcp/instrumentation.ts create mode 100644 src/server/mcp/tools/output-schema-validation.test.ts diff --git a/README.md b/README.md index 7580e61..fb0b818 100644 --- a/README.md +++ b/README.md @@ -16,6 +16,7 @@ OpenSEO is an SEO tool for _the people_. If tools like Semrush or Ahrefs are too Image ## Table of Contents + - [Main SEO Workflows](#main-seo-workflows) - [OpenSEO MCP](#openseo-mcp) - [OpenSEO Agent Skills](#openseo-agent-skills) @@ -32,6 +33,7 @@ OpenSEO is an SEO tool for _the people_. If tools like Semrush or Ahrefs are too - [SEO API Cost Reference](#seo-api-cost-reference) ## Hosted Version + If you not interested in self hosting, or just want to support the project, we also have a hosted version: [openseo.so](https://openseo.so) diff --git a/src/server/mcp/instrumentation.test.ts b/src/server/mcp/instrumentation.test.ts new file mode 100644 index 0000000..b1ac5a4 --- /dev/null +++ b/src/server/mcp/instrumentation.test.ts @@ -0,0 +1,88 @@ +import type { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { z } from "zod"; +import type { ToolExtra } from "@/server/mcp/context"; +import { AppError } from "@/server/lib/errors"; + +const mocks = vi.hoisted(() => ({ + captureServerError: vi.fn(), +})); + +// waitUntil runs the capture promise inline so assertions see the call. +vi.mock("cloudflare:workers", () => ({ + waitUntil: (promise: Promise) => void promise, +})); + +vi.mock("@/server/lib/posthog", () => ({ + captureServerError: mocks.captureServerError, +})); + +const toolExtra: ToolExtra = { + signal: new AbortController().signal, + requestId: 1, + sendNotification: vi.fn(), + sendRequest: vi.fn(), +}; + +const outputSchema = { items: z.array(z.object({}).passthrough()) }; + +function okResult(structuredContent: Record): CallToolResult { + return { content: [{ type: "text", text: "ok" }], structuredContent }; +} + +describe("instrumentMcpToolHandler", () => { + beforeEach(() => { + mocks.captureServerError.mockReset(); + }); + + it("passes a valid result through without reporting", async () => { + const { instrumentMcpToolHandler } = await import("./instrumentation"); + const wrapped = instrumentMcpToolHandler("demo", outputSchema, async () => + okResult({ items: [{ domain: "example.com" }] }), + ); + + const result = await wrapped({}, toolExtra); + + expect(result.structuredContent).toEqual({ + items: [{ domain: "example.com" }], + }); + expect(mocks.captureServerError).not.toHaveBeenCalled(); + }); + + it("reports an output schema mismatch the SDK would silently reject", async () => { + const { instrumentMcpToolHandler } = await import("./instrumentation"); + const wrapped = instrumentMcpToolHandler("demo", outputSchema, async () => + okResult({ items: "not-an-array" }), + ); + + await wrapped({}, toolExtra); + + expect(mocks.captureServerError).toHaveBeenCalledTimes(1); + expect(mocks.captureServerError.mock.calls[0][1]).toMatchObject({ + errorCode: "MCP_OUTPUT_VALIDATION", + tool: "demo", + }); + }); + + it("reports and rethrows a reportable handler error", async () => { + const { instrumentMcpToolHandler } = await import("./instrumentation"); + const boom = new Error("upstream exploded"); + const wrapped = instrumentMcpToolHandler("demo", outputSchema, async () => { + throw boom; + }); + + await expect(wrapped({}, toolExtra)).rejects.toThrow("upstream exploded"); + expect(mocks.captureServerError).toHaveBeenCalledTimes(1); + expect(mocks.captureServerError.mock.calls[0][0]).toBe(boom); + }); + + it("rethrows expected errors without reporting them", async () => { + const { instrumentMcpToolHandler } = await import("./instrumentation"); + const wrapped = instrumentMcpToolHandler("demo", outputSchema, async () => { + throw new AppError("NOT_FOUND"); + }); + + await expect(wrapped({}, toolExtra)).rejects.toThrow("NOT_FOUND"); + expect(mocks.captureServerError).not.toHaveBeenCalled(); + }); +}); diff --git a/src/server/mcp/instrumentation.ts b/src/server/mcp/instrumentation.ts new file mode 100644 index 0000000..470cea7 --- /dev/null +++ b/src/server/mcp/instrumentation.ts @@ -0,0 +1,85 @@ +import { waitUntil } from "cloudflare:workers"; +import type { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; +import { + getParseErrorMessage, + normalizeObjectSchema, + safeParseAsync, + type AnySchema, + type ZodRawShapeCompat, +} from "@modelcontextprotocol/sdk/server/zod-compat.js"; +import { asAppError } from "@/server/lib/errors"; +import { captureServerError } from "@/server/lib/posthog"; +import { shouldCaptureAppErrorCode } from "@/shared/error-codes"; +import type { ToolExtra } from "@/server/mcp/context"; + +type ToolHandler = ( + args: TArgs, + extra: ToolExtra, +) => CallToolResult | Promise; + +/** + * Wraps an MCP tool handler so failures reach PostHog. Unlike TanStack server + * functions (covered by errorHandlingMiddleware), the MCP route has no error + * middleware, so tool failures are otherwise invisible in error reporting. + * + * This captures two classes of failure: + * - Exceptions thrown by the handler (DataForSEO outages, auth failures, …), + * gated by shouldCaptureAppErrorCode to keep expected errors out of PostHog. + * - Output-schema validation failures. The SDK validates structuredContent + * against the output schema *after* the handler returns and converts a + * failure into a -32602 JSON-RPC error it never rethrows, so we re-run the + * same validation (via the SDK's own helpers) to surface the mismatch + * instead of shipping it silently. + */ +export function instrumentMcpToolHandler( + toolName: string, + outputSchema: AnySchema | ZodRawShapeCompat | undefined, + handler: ToolHandler, +): (args: TArgs, extra: ToolExtra) => Promise { + const normalizedOutputSchema = normalizeObjectSchema(outputSchema); + + return async (args, extra) => { + try { + const result = await handler(args, extra); + if ( + normalizedOutputSchema && + !result.isError && + result.structuredContent + ) { + const validation = await safeParseAsync( + normalizedOutputSchema, + result.structuredContent, + ); + if (!validation.success) { + // getParseErrorMessage reports type-level mismatches (expected vs + // received *types*), so it carries no row data. Keep it that way: + // output schemas must not gain value-echoing refinements (enums on + // user data, etc.) that would surface response values in PostHog. + waitUntil( + captureServerError( + new Error(`MCP output validation failed for ${toolName}`), + { + errorCode: "MCP_OUTPUT_VALIDATION", + tool: toolName, + issues: getParseErrorMessage(validation.error).slice(0, 500), + }, + ), + ); + } + } + return result; + } catch (error) { + const appError = asAppError(error); + if (shouldCaptureAppErrorCode(appError?.code)) { + console.error(`mcp.tool error (${toolName}):`, error); + waitUntil( + captureServerError(error, { + errorCode: appError?.code ?? "INTERNAL_ERROR", + tool: toolName, + }), + ); + } + throw error; + } + }; +} diff --git a/src/server/mcp/output-schemas.ts b/src/server/mcp/output-schemas.ts index deb0bc5..572ad79 100644 --- a/src/server/mcp/output-schemas.ts +++ b/src/server/mcp/output-schemas.ts @@ -11,7 +11,13 @@ const mcpMetaOutputSchema = z }) .passthrough(); -export const looseObjectOutputSchema = z.record(z.string(), z.unknown()); +// Tools that pass DataForSEO rows straight through to structuredContent hand the +// MCP SDK typed class instances (e.g. DataforseoLabsSerpCompetitorsLiveItem), not +// plain objects. Zod 4's z.record() requires a plain-object prototype and rejects +// class instances ("expected record, received "), which the SDK surfaces +// as a -32602 output validation error. A loose object schema accepts any object +// shape, so it validates both plain rows and typed instances. +export const looseObjectOutputSchema = z.object({}).passthrough(); export const optionalMetaOutputSchema = { meta: mcpMetaOutputSchema.optional(), diff --git a/src/server/mcp/server.ts b/src/server/mcp/server.ts index 21334d4..eb4911d 100644 --- a/src/server/mcp/server.ts +++ b/src/server/mcp/server.ts @@ -1,4 +1,5 @@ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { instrumentMcpToolHandler } from "@/server/mcp/instrumentation"; import { getBacklinksOverviewTool } from "@/server/mcp/tools/get-backlinks-overview"; import { getDomainKeywordSuggestionsTool } from "@/server/mcp/tools/get-domain-keyword-suggestions"; import { getDomainOverviewTool } from "@/server/mcp/tools/get-domain-overview"; @@ -22,91 +23,172 @@ import { } from "@/server/mcp/tools/search-console-tools"; import { whoamiTool } from "@/server/mcp/tools/whoami"; +// Each handler is wrapped with instrumentMcpToolHandler so failures reach +// PostHog — the MCP route has no error middleware of its own. Tools are +// registered one explicit call at a time (not via a loop/helper) so each one's +// input/output schema types stay concrete, which the SDK's registerTool +// generics require to type the handler callback. export function registerOpenSeoMcpTools(server: McpServer) { - server.registerTool(whoamiTool.name, whoamiTool.config, whoamiTool.handler); + server.registerTool( + whoamiTool.name, + whoamiTool.config, + instrumentMcpToolHandler( + whoamiTool.name, + whoamiTool.config.outputSchema, + whoamiTool.handler, + ), + ); server.registerTool( listProjectsTool.name, listProjectsTool.config, - listProjectsTool.handler, + instrumentMcpToolHandler( + listProjectsTool.name, + listProjectsTool.config.outputSchema, + listProjectsTool.handler, + ), ); server.registerTool( listSavedKeywordsTool.name, listSavedKeywordsTool.config, - listSavedKeywordsTool.handler, + instrumentMcpToolHandler( + listSavedKeywordsTool.name, + listSavedKeywordsTool.config.outputSchema, + listSavedKeywordsTool.handler, + ), ); server.registerTool( researchKeywordsTool.name, researchKeywordsTool.config, - researchKeywordsTool.handler, + instrumentMcpToolHandler( + researchKeywordsTool.name, + researchKeywordsTool.config.outputSchema, + researchKeywordsTool.handler, + ), ); server.registerTool( saveKeywordsTool.name, saveKeywordsTool.config, - saveKeywordsTool.handler, + instrumentMcpToolHandler( + saveKeywordsTool.name, + saveKeywordsTool.config.outputSchema, + saveKeywordsTool.handler, + ), ); server.registerTool( getDomainOverviewTool.name, getDomainOverviewTool.config, - getDomainOverviewTool.handler, + instrumentMcpToolHandler( + getDomainOverviewTool.name, + getDomainOverviewTool.config.outputSchema, + getDomainOverviewTool.handler, + ), ); server.registerTool( getDomainKeywordSuggestionsTool.name, getDomainKeywordSuggestionsTool.config, - getDomainKeywordSuggestionsTool.handler, + instrumentMcpToolHandler( + getDomainKeywordSuggestionsTool.name, + getDomainKeywordSuggestionsTool.config.outputSchema, + getDomainKeywordSuggestionsTool.handler, + ), ); server.registerTool( getBacklinksOverviewTool.name, getBacklinksOverviewTool.config, - getBacklinksOverviewTool.handler, + instrumentMcpToolHandler( + getBacklinksOverviewTool.name, + getBacklinksOverviewTool.config.outputSchema, + getBacklinksOverviewTool.handler, + ), ); server.registerTool( getSerpResultsTool.name, getSerpResultsTool.config, - getSerpResultsTool.handler, + instrumentMcpToolHandler( + getSerpResultsTool.name, + getSerpResultsTool.config.outputSchema, + getSerpResultsTool.handler, + ), ); server.registerTool( getRankTrackerTool.name, getRankTrackerTool.config, - getRankTrackerTool.handler, + instrumentMcpToolHandler( + getRankTrackerTool.name, + getRankTrackerTool.config.outputSchema, + getRankTrackerTool.handler, + ), ); server.registerTool( getRankedKeywordsTool.name, getRankedKeywordsTool.config, - getRankedKeywordsTool.handler, + instrumentMcpToolHandler( + getRankedKeywordsTool.name, + getRankedKeywordsTool.config.outputSchema, + getRankedKeywordsTool.handler, + ), ); server.registerTool( findSerpCompetitorsTool.name, findSerpCompetitorsTool.config, - findSerpCompetitorsTool.handler, + instrumentMcpToolHandler( + findSerpCompetitorsTool.name, + findSerpCompetitorsTool.config.outputSchema, + findSerpCompetitorsTool.handler, + ), ); server.registerTool( searchLocalBusinessesTool.name, searchLocalBusinessesTool.config, - searchLocalBusinessesTool.handler, + instrumentMcpToolHandler( + searchLocalBusinessesTool.name, + searchLocalBusinessesTool.config.outputSchema, + searchLocalBusinessesTool.handler, + ), ); server.registerTool( getLocalSerpResultsTool.name, getLocalSerpResultsTool.config, - getLocalSerpResultsTool.handler, + instrumentMcpToolHandler( + getLocalSerpResultsTool.name, + getLocalSerpResultsTool.config.outputSchema, + getLocalSerpResultsTool.handler, + ), ); server.registerTool( getGoogleBusinessQuestionsTool.name, getGoogleBusinessQuestionsTool.config, - getGoogleBusinessQuestionsTool.handler, + instrumentMcpToolHandler( + getGoogleBusinessQuestionsTool.name, + getGoogleBusinessQuestionsTool.config.outputSchema, + getGoogleBusinessQuestionsTool.handler, + ), ); server.registerTool( getKeywordMetricsTool.name, getKeywordMetricsTool.config, - getKeywordMetricsTool.handler, + instrumentMcpToolHandler( + getKeywordMetricsTool.name, + getKeywordMetricsTool.config.outputSchema, + getKeywordMetricsTool.handler, + ), ); server.registerTool( getSearchConsolePerformanceTool.name, getSearchConsolePerformanceTool.config, - getSearchConsolePerformanceTool.handler, + instrumentMcpToolHandler( + getSearchConsolePerformanceTool.name, + getSearchConsolePerformanceTool.config.outputSchema, + getSearchConsolePerformanceTool.handler, + ), ); server.registerTool( inspectUrlsTool.name, inspectUrlsTool.config, - inspectUrlsTool.handler, + instrumentMcpToolHandler( + inspectUrlsTool.name, + inspectUrlsTool.config.outputSchema, + inspectUrlsTool.handler, + ), ); } diff --git a/src/server/mcp/tools/output-schema-validation.test.ts b/src/server/mcp/tools/output-schema-validation.test.ts new file mode 100644 index 0000000..e1f6700 --- /dev/null +++ b/src/server/mcp/tools/output-schema-validation.test.ts @@ -0,0 +1,53 @@ +import { + normalizeObjectSchema, + safeParseAsync, +} from "@modelcontextprotocol/sdk/server/zod-compat.js"; +import { describe, expect, it, vi } from "vitest"; + +vi.mock("cloudflare:workers", () => ({ + env: {}, +})); + +// A class instance reproduces what the DataForSEO SDK hands the tools: an +// object whose prototype is not Object.prototype (e.g. +// DataforseoLabsSerpCompetitorsLiveItem). Zod 4's z.record() rejects those +// ("expected record, received "), so a record-based output schema +// makes the MCP server fail these passthrough tools with a -32602 output +// validation error even though the API call succeeded. +class ProviderRow { + constructor( + public domain: string, + public rank_absolute: number, + ) {} +} + +describe("DataForSEO research tool output schemas", () => { + // Every tool that streams provider rows straight to structuredContent. + it.each([ + ["find_serp_competitors", "competitors"], + ["get_local_serp_results", "results"], + ["search_local_businesses", "businesses"], + ["get_google_business_questions", "questions"], + ["get_ranked_keywords", "keywords"], + ])( + "%s accepts typed (non-plain-object) provider rows", + async (toolName, field) => { + const tools = await import("./dataforseo-research-tools"); + const tool = Object.values(tools).find((t) => t.name === toolName); + if (!tool) throw new Error(`tool ${toolName} not found`); + + const schema = normalizeObjectSchema(tool.config.outputSchema); + if (!schema) throw new Error("output schema did not normalize"); + + // Mirror the MCP server: validate structuredContent against the tool's + // own output schema. Extra keys (e.g. get_ranked_keywords' totalCount) + // are allowed by the passthrough schemas, so one payload covers all. + const result = await safeParseAsync(schema, { + [field]: [new ProviderRow("example.com", 1)], + totalCount: 1, + }); + + expect(result.success).toBe(true); + }, + ); +});