From 99954956dfb0af8565c73d9260961334728b3a42 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 5 Nov 2026 22:00:00 +0000 Subject: [PATCH 1/3] test(mcp): Lint the metadata clients receive from the server Add a vitest linter for the initialize and tools/list surface in mcp-core, a structured-output guard for the mcp-core suite, and server.json checks in @sentry/mcp-server, so the MCP metadata we just added does not drift back. The guard mirrors the SDK's tools/call validation: a tool with an outputSchema must return structuredContent that matches it. Tools whose outputSchema PR is still open are listed as pending, and their entry fails once the schema lands, so the PR that lands it deletes the entry. Co-Authored-By: David Cramer <23610+dcramer@users.noreply.github.com> --- docs/contributing/adding-tools.md | 9 +- docs/contributing/tool-responses.md | 38 ++ docs/releases/stdio.md | 6 + packages/mcp-core/src/server.metadata.test.ts | 350 ++++++++++++++++++ .../src/test-utils/dataset-search-tool.ts | 3 + .../src/test-utils/search-events-stub.ts | 29 ++ .../src/test-utils/structured-output-guard.ts | 46 +++ .../src/tools/catalog/search-errors.test.ts | 14 +- .../src/tools/catalog/search-logs.test.ts | 14 +- .../src/tools/catalog/search-traces.test.ts | 14 +- packages/mcp-core/src/tools/tools.test.ts | 41 +- packages/mcp-core/src/tools/types.ts | 8 +- packages/mcp-core/vitest.config.ts | 6 +- packages/mcp-server/package.json | 2 +- packages/mcp-server/test/server-json.test.mjs | 110 ++++++ 15 files changed, 639 insertions(+), 51 deletions(-) create mode 100644 packages/mcp-core/src/server.metadata.test.ts create mode 100644 packages/mcp-core/src/test-utils/search-events-stub.ts create mode 100644 packages/mcp-core/src/test-utils/structured-output-guard.ts create mode 100644 packages/mcp-server/test/server-json.test.mjs diff --git a/docs/contributing/adding-tools.md b/docs/contributing/adding-tools.md index b952bb4c3..31930ef51 100644 --- a/docs/contributing/adding-tools.md +++ b/docs/contributing/adding-tools.md @@ -108,16 +108,16 @@ export default defineTool({ **REQUIRED**: Every tool sets `title`, a short Title Case display name such as `"Search Issues"`. The server exports it as the MCP tool `title`, which clients -show instead of the snake_case `name`. Titles must be unique across tools; -`tools.test.ts` enforces this. Deprecated aliases add a ` (Deprecated)` +show instead of the snake_case `name`. Titles must be unique across tools; the +[metadata linter](tool-responses.md#metadata-linter) enforces this. Deprecated aliases add a ` (Deprecated)` suffix, for example `"Search AI Conversations (Deprecated)"`. ### Safety Annotations **REQUIRED**: Every tool must declare `readOnlyHint`, `destructiveHint`, and `openWorldHint` explicitly (each `true` or `false`, never omitted). This is -enforced at compile time (the `annotations` type requires them) and by -`tools.test.ts`, because filters and confirmation gates depend on these hints; +enforced at compile time (the `annotations` type requires them) and by the +[metadata linter](tool-responses.md#metadata-linter), because filters and confirmation gates depend on these hints; an absent hint is a silent gap. Read-only tools must set `destructiveHint: false` (not leave it undefined). @@ -341,6 +341,7 @@ pnpm eval your-tool - [ ] Unit tests with snapshots - [ ] Mock responses - [ ] 1-2 eval tests (if critical) +- [ ] `outputSchema` declared, or a reasoned skip entry in the [metadata linter](tool-responses.md#metadata-linter) - [ ] Run quality checks ## Agent-in-Tool Pattern diff --git a/docs/contributing/tool-responses.md b/docs/contributing/tool-responses.md index 937b29f39..bed4344ce 100644 --- a/docs/contributing/tool-responses.md +++ b/docs/contributing/tool-responses.md @@ -244,6 +244,44 @@ Policy (encoded in `packages/mcp-core/src/internal/tool-helpers/results.ts`): Reference: MCP 2025-11-25 Tools specification, sections "Structured Content" and "Output Schema". +## Metadata Linter + +`packages/mcp-core/src/server.metadata.test.ts` builds the server with every +skill granted, in stable and experimental mode, and checks what a client +receives from `initialize` and `tools/list`: + +- `serverInfo` keeps `name: "Sentry MCP"` and sets `title`, `description`, an + https `websiteUrl`, and https `icons`. +- `instructions` are non-empty and at most `MAX_SERVER_INSTRUCTIONS_LENGTH` + characters. +- Every tool name matches `^[A-Za-z0-9_.-]{1,128}$`, its `inputSchema` root is + an object, and it advertises no `execution.taskSupport`. +- Every tool has a unique, non-empty `title`; sets `readOnlyHint`, + `destructiveHint`, and `openWorldHint`; and has a description of at most + `MAX_TOOL_DESCRIPTION_LENGTH` characters. +- A tool exports only the `_meta` keys that `ALLOWED_TOOL_META` lists for it. +- Every tool declares an object `outputSchema`, unless it has a skip entry. + +`test-utils/structured-output-guard.ts` wraps `defineTool` for the whole +mcp-core suite, the same way the SDK validates `tools/call`: a handler with an +`outputSchema` that returns no `structuredContent`, or `structuredContent` that +does not match the schema, fails the test that called it. Error results +(`isError: true`) are skipped. The linter checks that each listed tool with an +`outputSchema` has a colocated `*.test.ts` that reads its +`structuredContent`. + +To skip the `outputSchema` check, add the tool to one of two maps, with a +reason: + +- `OUTPUT_SCHEMA_EXEMPT` for tools that return no structured content by design, + such as the markdown-only tools above. +- `OUTPUT_SCHEMA_PENDING` for tools whose `outputSchema` has not landed, with + the PR, for example `"pending getsentry/toolkit#2004"`. + +An entry fails once its tool has an `outputSchema`, so the PR that adds one +also deletes the entry. To add a `_meta` key, add it to `ALLOWED_TOOL_META` +with a comment that says which client reads it. + ## Suggested Actions Use `suggestedActions` when a tool result reveals a concrete follow-up tool call diff --git a/docs/releases/stdio.md b/docs/releases/stdio.md index e1a7736f2..a07668544 100644 --- a/docs/releases/stdio.md +++ b/docs/releases/stdio.md @@ -94,6 +94,12 @@ skipped. An identical active listing is a no-op, so reruns are safe. Different metadata for an existing version fails because registry versions are immutable; publish those changes with the next release. +`packages/mcp-server/test/server-json.test.mjs` runs with `test:bundle` and +checks the manifest: its name, npm package, repository, title, website, icons, +and that its environment variables are the `SENTRY_*` variables the bundled +`mcp --help` lists. It does not validate against the registry schema, which is +not vendored; tests make no network requests. + ## User Installation Users run the server through npx in their MCP client configuration: diff --git a/packages/mcp-core/src/server.metadata.test.ts b/packages/mcp-core/src/server.metadata.test.ts new file mode 100644 index 000000000..9b8f3c9ef --- /dev/null +++ b/packages/mcp-core/src/server.metadata.test.ts @@ -0,0 +1,350 @@ +/** + * Metadata linter for what an MCP client receives from `initialize` and + * `tools/list`. See docs/contributing/tool-responses.md#metadata-linter for + * the checks and for how to add a skip entry. + */ +import fs from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; +import { beforeAll, describe, expect, it, vi } from "vitest"; +import { z } from "zod"; +import { defineTool } from "./internal/tool-helpers/define"; +import { buildServer } from "./server"; +import { MAX_SERVER_INSTRUCTIONS_LENGTH } from "./server-instructions"; +import type { Skill } from "./skills"; +import catalogTools from "./tools/catalog/index"; +import { MAX_TOOL_DESCRIPTION_LENGTH } from "./tools/types"; +import type { ServerContext } from "./types"; + +vi.mock("@sentry/core/server", () => ({ + wrapMcpServerWithSentry: vi.fn((server) => server), +})); + +const ALL_SKILLS = new Set([ + "inspect", + "triage", + "project-management", + "seer", +]); + +/** The only `_meta` keys each tool may export. */ +const ALLOWED_TOOL_META: Record = { + // Claude Code keeps it loaded so a pasted Sentry URL works without a tool + // search. See docs/mcp/harnesses.md. + get_sentry_resource: ["anthropic/alwaysLoad"], +}; + +/** Tools that return no `structuredContent` by design. */ +const OUTPUT_SCHEMA_EXEMPT: Record = { + get_issue_details: + "markdown-only by policy (docs/contributing/tool-responses.md)", + get_trace_details: + "markdown-only by policy (docs/contributing/tool-responses.md)", + get_event_stacktrace: + "markdown-only by policy (docs/contributing/tool-responses.md)", + get_span_details: "delegates to the get_trace_details renderer", + get_doc: "returns doc text", + search_docs: "returns doc text", + get_snapshot_image: "returns an image", + get_event_attachment: "its download path returns image or resource content", + get_sentry_resource: "passes through whatever tool it resolves to", + analyze_issue_with_seer: "has narrative polling paths", +}; + +/** + * PENDING: tools whose outputSchema has not landed yet. Delete an entry when + * its PR merges; a test below fails until you do. + */ +const OUTPUT_SCHEMA_PENDING: Record = { + get_issue_activity: "pending getsentry/toolkit#2001", + search_issues: "pending getsentry/toolkit#2004", + search_metrics: "pending getsentry/toolkit#2010", + search_profiles: "pending getsentry/toolkit#2012", + get_dashboard_details: "pending getsentry/toolkit#2018", + get_monitor_details: "pending getsentry/toolkit#2020", + get_profile_details: "pending getsentry/toolkit#2024", + search_replays: "pending getsentry/toolkit#2025", + search_attributes: "pending: experimental-only, no outputSchema PR yet", +}; + +const CATALOG_DIR = path.join( + path.dirname(fileURLToPath(import.meta.url)), + "tools/catalog", +); + +type ListedTool = Awaited>["tools"][number]; + +interface ClientSurface { + serverInfo: ReturnType; + instructions: string | undefined; + tools: ListedTool[]; +} + +async function readSurface(experimentalMode: boolean): Promise { + const context: ServerContext = { + accessToken: "test-token", + grantedSkills: ALL_SKILLS, + constraints: { organizationSlug: null, projectSlug: null }, + sentryHost: "sentry.io", + }; + const server = buildServer({ context, experimentalMode }); + const [clientTransport, serverTransport] = + InMemoryTransport.createLinkedPair(); + const client = new Client({ name: "metadata-lint", version: "1.0.0" }); + await server.connect(serverTransport); + await client.connect(clientTransport); + try { + const listed = await client.listTools(); + // A second page would hide tools from every check below. + expect(listed.nextCursor).toBeUndefined(); + return { + serverInfo: client.getServerVersion(), + instructions: client.getInstructions(), + tools: listed.tools, + }; + } finally { + await client.close(); + await server.close(); + } +} + +/** Each server mode a client can get, keyed by its test label. */ +const MODES = { stable: false, experimental: true } as const; + +const surfaces = new Map(); + +beforeAll(async () => { + for (const [mode, experimentalMode] of Object.entries(MODES)) { + surfaces.set(mode, await readSurface(experimentalMode)); + } +}); + +function failures( + tools: ListedTool[], + check: (tool: ListedTool) => string | undefined, +): string[] { + return tools.flatMap((tool) => { + const failure = check(tool); + return failure ? [`${tool.name}: ${failure}`] : []; + }); +} + +describe.each(Object.keys(MODES))("%s mode", (mode) => { + const surface = () => { + const found = surfaces.get(mode); + if (!found) throw new Error(`no ${mode} surface`); + return found; + }; + + it("identifies the server in serverInfo", () => { + const { serverInfo } = surface(); + // Telemetry and clients key on this name; clients display `title`. + expect(serverInfo?.name).toBe("Sentry MCP"); + for (const field of ["title", "description", "websiteUrl"] as const) { + expect(serverInfo?.[field]?.trim(), `serverInfo.${field}`).toBeTruthy(); + } + expect(serverInfo?.websiteUrl).toMatch(/^https:\/\//); + expect(serverInfo?.icons?.length, "serverInfo.icons").toBeGreaterThan(0); + for (const icon of serverInfo?.icons ?? []) { + expect(icon.src, "serverInfo.icons[].src").toMatch(/^https:\/\//); + expect(icon.mimeType, "serverInfo.icons[].mimeType").toBeTruthy(); + } + }); + + it(`returns instructions of at most ${MAX_SERVER_INSTRUCTIONS_LENGTH} characters`, () => { + const { instructions } = surface(); + expect(instructions?.trim(), "instructions").toBeTruthy(); + expect(instructions?.length).toBeLessThanOrEqual( + MAX_SERVER_INSTRUCTIONS_LENGTH, + ); + }); + + it("uses valid tool names, object input schemas, and no task support", () => { + const problems = failures(surface().tools, (tool) => { + if (!/^[A-Za-z0-9_.-]{1,128}$/.test(tool.name)) { + return "name must match ^[A-Za-z0-9_.-]{1,128}$"; + } + if (tool.inputSchema.type !== "object") { + return "inputSchema root must be an object"; + } + // The server implements no MCP tasks, so no tool may advertise them. + return tool.execution?.taskSupport === undefined + ? undefined + : `advertises execution.taskSupport: ${tool.execution.taskSupport}`; + }); + expect(problems).toEqual([]); + }); + + it("gives every tool a unique title", () => { + const owners = new Map(); + const problems = failures(surface().tools, (tool) => { + const title = tool.title?.trim(); + if (!title) return "missing title"; + const owner = owners.get(title); + owners.set(title, tool.name); + // Deprecated aliases are told apart by their " (Deprecated)" suffix. + return owner ? `reuses the title of ${owner}` : undefined; + }); + expect(problems).toEqual([]); + }); + + it("sets every safety hint explicitly", () => { + const hints = ["readOnlyHint", "destructiveHint", "openWorldHint"] as const; + const problems = failures(surface().tools, (tool) => { + const missing = hints.filter( + (hint) => typeof tool.annotations?.[hint] !== "boolean", + ); + return missing.length > 0 ? `missing ${missing.join(", ")}` : undefined; + }); + expect(problems).toEqual([]); + }); + + it(`keeps descriptions at most ${MAX_TOOL_DESCRIPTION_LENGTH} characters`, () => { + const problems = failures(surface().tools, (tool) => { + const length = tool.description?.trim().length ?? 0; + if (length === 0) return "missing description"; + return length > MAX_TOOL_DESCRIPTION_LENGTH + ? `description is ${length} characters` + : undefined; + }); + expect(problems).toEqual([]); + }); + + it("exports only allowlisted _meta keys", () => { + const problems = failures(surface().tools, (tool) => { + const allowed = ALLOWED_TOOL_META[tool.name] ?? []; + const extra = Object.keys(tool._meta ?? {}).filter( + (key) => !allowed.includes(key), + ); + return extra.length > 0 + ? `_meta key ${extra.join(", ")} is not in ALLOWED_TOOL_META` + : undefined; + }); + expect(problems).toEqual([]); + }); + + it("declares an object outputSchema unless the tool is skipped", () => { + const problems = failures(surface().tools, (tool) => { + if (tool.outputSchema) { + return tool.outputSchema.type === "object" + ? undefined + : "outputSchema root must be an object"; + } + return tool.name in OUTPUT_SCHEMA_EXEMPT || + tool.name in OUTPUT_SCHEMA_PENDING + ? undefined + : "missing outputSchema; add one, or an OUTPUT_SCHEMA_EXEMPT or OUTPUT_SCHEMA_PENDING entry with a reason"; + }); + expect(problems).toEqual([]); + }); +}); + +describe("outputSchema skip lists", () => { + const listed = () => + new Map( + [...surfaces.values()].flatMap(({ tools }) => + tools.map((tool) => [tool.name, tool] as const), + ), + ); + + it("name listed tools without an outputSchema", () => { + const tools = listed(); + const problems = [ + ...Object.keys(OUTPUT_SCHEMA_EXEMPT).map((name) => ["EXEMPT", name]), + ...Object.keys(OUTPUT_SCHEMA_PENDING).map((name) => ["PENDING", name]), + ].flatMap(([list, name]) => { + const tool = tools.get(name); + if (!tool) return [`${name}: in OUTPUT_SCHEMA_${list} but not listed`]; + if (tool.outputSchema) { + return [ + `${name}: has an outputSchema now; delete its OUTPUT_SCHEMA_${list} entry`, + ]; + } + return []; + }); + expect(problems).toEqual([]); + }); + + it("do not overlap", () => { + expect( + Object.keys(OUTPUT_SCHEMA_EXEMPT).filter( + (name) => name in OUTPUT_SCHEMA_PENDING, + ), + ).toEqual([]); + }); + + it("give every entry a reason", () => { + for (const [name, reason] of Object.entries({ + ...OUTPUT_SCHEMA_EXEMPT, + ...OUTPUT_SCHEMA_PENDING, + })) { + expect(reason.trim(), name).toBeTruthy(); + } + }); +}); + +describe("structured output", () => { + it("validates handler results against outputSchema in every test", async () => { + // test-utils/structured-output-guard.ts wraps defineTool for the suite. + const tool = defineTool({ + name: "lint_probe", + title: "Lint Probe", + description: "Probe.", + skills: ["inspect"], + requiredScopes: [], + inputSchema: {}, + outputSchema: z.object({ count: z.number() }), + annotations: { + readOnlyHint: true, + destructiveHint: false, + openWorldHint: false, + }, + handler: async () => ({ structuredContent: { count: "one" } }), + }); + + await expect(tool.handler({}, {} as ServerContext)).rejects + .toThrowErrorMatchingInlineSnapshot(` + [Error: lint_probe returned structuredContent that does not match its outputSchema: [ + { + "expected": "number", + "code": "invalid_type", + "path": [ + "count" + ], + "message": "Invalid input: expected number, received string" + } + ]] + `); + }); + + it("covers every listed tool with an outputSchema in its colocated tests", () => { + const catalogFiles = fs.readdirSync(CATALOG_DIR); + const problems = [...surfaces.values()] + .flatMap(({ tools }) => tools) + .filter((tool) => tool.outputSchema && tool.name in catalogTools) + .flatMap((tool) => { + // Matches .test.ts and variants such as .structured.test.ts. + const base = tool.name.replaceAll("_", "-"); + const testFile = new RegExp(`^${base}(\\..+)?\\.test\\.ts$`); + const readsStructuredContent = catalogFiles + .filter((file) => testFile.test(file)) + .some((file) => { + const contents = fs.readFileSync( + path.join(CATALOG_DIR, file), + "utf8", + ); + return ( + contents.includes("structuredContent") || + contents.includes("getStructuredContent(") || + contents.includes("assertStructuredOnlyResult(") + ); + }); + return readsStructuredContent + ? [] + : [`${tool.name}: no ${base}*.test.ts reads structuredContent`]; + }); + expect([...new Set(problems)]).toEqual([]); + }); +}); diff --git a/packages/mcp-core/src/test-utils/dataset-search-tool.ts b/packages/mcp-core/src/test-utils/dataset-search-tool.ts index abf122ba9..e8c31facc 100644 --- a/packages/mcp-core/src/test-utils/dataset-search-tool.ts +++ b/packages/mcp-core/src/test-utils/dataset-search-tool.ts @@ -20,6 +20,9 @@ const params = { * ...(await importOriginal()), * runSearchEvents: vi.fn(async () => "ok"), * })); + * + * A tool with an outputSchema must return `SEARCH_EVENTS_STUB` from + * `./search-events-stub` instead of "ok"; search-errors.test.ts shows how. */ export async function inspectDatasetSearchTool

(tool: { inputSchema: object; diff --git a/packages/mcp-core/src/test-utils/search-events-stub.ts b/packages/mcp-core/src/test-utils/search-events-stub.ts new file mode 100644 index 000000000..3b510294d --- /dev/null +++ b/packages/mcp-core/src/test-utils/search-events-stub.ts @@ -0,0 +1,29 @@ +import type { SearchEventsStructuredContent } from "../tools/support/search-events/structured"; +import type { StructuredToolOutput } from "../tools/types"; + +/** + * An empty structured search result. Dataset search tool tests that mock + * `runSearchEvents` return it, so the result still matches the tool's + * outputSchema (see structured-output-guard.ts). + */ +export const SEARCH_EVENTS_STUB: StructuredToolOutput = + { + structuredContent: { + query: "anything", + explanation: null, + executedSearch: { + dataset: "stub", + query: "", + fields: [], + sort: null, + timeRange: { statsPeriod: null, start: null, end: null }, + }, + url: "https://sentry.io/", + presentationHint: "", + notes: [], + resultType: "samples", + results: [], + timeSeries: null, + suggestedActions: [], + }, + }; diff --git a/packages/mcp-core/src/test-utils/structured-output-guard.ts b/packages/mcp-core/src/test-utils/structured-output-guard.ts new file mode 100644 index 000000000..f3ccd4633 --- /dev/null +++ b/packages/mcp-core/src/test-utils/structured-output-guard.ts @@ -0,0 +1,46 @@ +/** + * Vitest setup file: every tool defined with `defineTool` checks its + * `structuredContent` against its `outputSchema` whenever a test calls the + * handler. + * + * Tool tests call `tool.handler()` directly, which skips the MCP SDK's own + * output validation in `tools/call`. With this guard, a tool whose result + * drifts from its declared schema fails its existing fixture tests instead of + * failing only for real clients. `server.metadata.test.ts` relies on it; see + * docs/contributing/tool-responses.md#metadata-linter. + */ +import { vi } from "vitest"; + +vi.mock("../internal/tool-helpers/define", async (importOriginal) => { + const actual = + await importOriginal(); + const defineTool: typeof actual.defineTool = (config) => { + const { outputSchema } = config; + if (!outputSchema) { + return actual.defineTool(config); + } + // Mirrors the SDK's output validation in tools/call. + const handler: typeof config.handler = async (params, context) => { + const result = await config.handler(params, context); + if (typeof result === "object" && "isError" in result && result.isError) { + return result; + } + if (typeof result !== "object" || !("structuredContent" in result)) { + throw new Error( + `${config.name} has an outputSchema but returned no structuredContent`, + ); + } + const parsed = await outputSchema.safeParseAsync( + result.structuredContent, + ); + if (!parsed.success) { + throw new Error( + `${config.name} returned structuredContent that does not match its outputSchema: ${parsed.error.message}`, + ); + } + return result; + }; + return actual.defineTool({ ...config, handler }); + }; + return { ...actual, defineTool }; +}); diff --git a/packages/mcp-core/src/tools/catalog/search-errors.test.ts b/packages/mcp-core/src/tools/catalog/search-errors.test.ts index 8daf3bb34..2114f033f 100644 --- a/packages/mcp-core/src/tools/catalog/search-errors.test.ts +++ b/packages/mcp-core/src/tools/catalog/search-errors.test.ts @@ -3,10 +3,16 @@ import { inspectDatasetSearchTool } from "../../test-utils/dataset-search-tool"; import searchErrors from "./search-errors"; // The shared handler is covered by search-events.test.ts. -vi.mock("../support/search-events/search", async (importOriginal) => ({ - ...(await importOriginal()), - runSearchEvents: vi.fn(async () => "ok"), -})); +vi.mock("../support/search-events/search", async (importOriginal) => { + const { SEARCH_EVENTS_STUB } = + await import("../../test-utils/search-events-stub"); + return { + ...(await importOriginal< + typeof import("../support/search-events/search") + >()), + runSearchEvents: vi.fn(async () => SEARCH_EVENTS_STUB), + }; +}); it("search_errors runs the shared handler locked to errors", async () => { expect(await inspectDatasetSearchTool(searchErrors)).toMatchInlineSnapshot(` diff --git a/packages/mcp-core/src/tools/catalog/search-logs.test.ts b/packages/mcp-core/src/tools/catalog/search-logs.test.ts index 86fc046d5..82e417180 100644 --- a/packages/mcp-core/src/tools/catalog/search-logs.test.ts +++ b/packages/mcp-core/src/tools/catalog/search-logs.test.ts @@ -3,10 +3,16 @@ import { inspectDatasetSearchTool } from "../../test-utils/dataset-search-tool"; import searchLogs from "./search-logs"; // The shared handler is covered by search-events.test.ts. -vi.mock("../support/search-events/search", async (importOriginal) => ({ - ...(await importOriginal()), - runSearchEvents: vi.fn(async () => "ok"), -})); +vi.mock("../support/search-events/search", async (importOriginal) => { + const { SEARCH_EVENTS_STUB } = + await import("../../test-utils/search-events-stub"); + return { + ...(await importOriginal< + typeof import("../support/search-events/search") + >()), + runSearchEvents: vi.fn(async () => SEARCH_EVENTS_STUB), + }; +}); it("search_logs runs the shared handler locked to logs", async () => { expect(await inspectDatasetSearchTool(searchLogs)).toMatchInlineSnapshot(` diff --git a/packages/mcp-core/src/tools/catalog/search-traces.test.ts b/packages/mcp-core/src/tools/catalog/search-traces.test.ts index 6f1e7cc15..806ac386e 100644 --- a/packages/mcp-core/src/tools/catalog/search-traces.test.ts +++ b/packages/mcp-core/src/tools/catalog/search-traces.test.ts @@ -3,10 +3,16 @@ import { inspectDatasetSearchTool } from "../../test-utils/dataset-search-tool"; import searchTraces from "./search-traces"; // The shared handler is covered by search-events.test.ts. -vi.mock("../support/search-events/search", async (importOriginal) => ({ - ...(await importOriginal()), - runSearchEvents: vi.fn(async () => "ok"), -})); +vi.mock("../support/search-events/search", async (importOriginal) => { + const { SEARCH_EVENTS_STUB } = + await import("../../test-utils/search-events-stub"); + return { + ...(await importOriginal< + typeof import("../support/search-events/search") + >()), + runSearchEvents: vi.fn(async () => SEARCH_EVENTS_STUB), + }; +}); it("search_traces runs the shared handler locked to spans", async () => { expect(await inspectDatasetSearchTool(searchTraces)).toMatchInlineSnapshot(` diff --git a/packages/mcp-core/src/tools/tools.test.ts b/packages/mcp-core/src/tools/tools.test.ts index 0fc41f888..3cb7a197b 100644 --- a/packages/mcp-core/src/tools/tools.test.ts +++ b/packages/mcp-core/src/tools/tools.test.ts @@ -16,11 +16,12 @@ import { TOP_LEVEL_TOOL_NAMES, WRAPPER_TOOL_NAMES, } from "./surfaces.js"; -import { isToolVisibleInMode, resolveDescription } from "./types.js"; +import { + isToolVisibleInMode, + MAX_TOOL_DESCRIPTION_LENGTH, + resolveDescription, +} from "./types.js"; -// VSCode (via OpenAI) limits to 1024 characters, but its tough to hit that right now, -// so instead lets limit the blast damage and hope that e.g. OpenAI will increase the limit. -const DESCRIPTION_MAX_LENGTH = 2048; const PUBLIC_TOOL_HARD_LIMIT = 25; const __dirname = path.dirname(fileURLToPath(import.meta.url)); const CATALOG_DIR = path.join(__dirname, "catalog"); @@ -52,40 +53,16 @@ test(`all tool descriptions under maximum length`, () => { experimentalMode, }).length; assert( - length < DESCRIPTION_MAX_LENGTH, - `${tool.name} description must be less than ${DESCRIPTION_MAX_LENGTH} characters in ${experimentalMode ? "experimental" : "stable"} mode (was ${length})`, + length <= MAX_TOOL_DESCRIPTION_LENGTH, + `${tool.name} description must be at most ${MAX_TOOL_DESCRIPTION_LENGTH} characters in ${experimentalMode ? "experimental" : "stable"} mode (was ${length})`, ); } } }); -test("all tools declare a unique display title", () => { - const seen = new Map(); - for (const tool of Object.values(tools.default)) { - assert(tool.title.trim(), `${tool.name} must define a non-empty title`); - const owner = seen.get(tool.title); - assert(!owner, `${tool.name} reuses the title of ${owner}`); - seen.set(tool.title, tool.name); - } -}); - -test("all tools declare complete MCP safety annotations", () => { +// server.metadata.test.ts checks titles and that every hint is set. +test("no tool is both read-only and destructive", () => { for (const tool of Object.values(tools.default)) { - // Every tool must declare readOnlyHint, destructiveHint, and openWorldHint - // explicitly (true or false, never undefined). Filters and confirmation - // gates rely on these, so an absent hint is a silent gap. - assert( - typeof tool.annotations.readOnlyHint === "boolean", - `${tool.name} must define readOnlyHint (true or false, not undefined)`, - ); - assert( - typeof tool.annotations.destructiveHint === "boolean", - `${tool.name} must define destructiveHint (true or false, not undefined)`, - ); - assert( - typeof tool.annotations.openWorldHint === "boolean", - `${tool.name} must define openWorldHint (true or false, not undefined)`, - ); assert( !( tool.annotations.readOnlyHint === true && diff --git a/packages/mcp-core/src/tools/types.ts b/packages/mcp-core/src/tools/types.ts index 374831db8..95737b8c6 100644 --- a/packages/mcp-core/src/tools/types.ts +++ b/packages/mcp-core/src/tools/types.ts @@ -83,6 +83,12 @@ export type ToolDescription = | string | ((context: DescriptionContext) => string); +/** + * Longest allowed tool description, in characters. VS Code (via OpenAI) + * limits descriptions to 1,024; this cap limits the damage until that rises. + */ +export const MAX_TOOL_DESCRIPTION_LENGTH = 2_048; + /** * Resolves a tool description to a string. * Handles both static strings and dynamic description functions. @@ -143,7 +149,7 @@ export interface ToolConfig< // readOnlyHint, destructiveHint, and openWorldHint are required so every // tool declares its safety posture explicitly. Filters and confirmation // gates rely on these; an undefined hint is a silent gap. Enforced further - // by tools.test.ts (see "complete MCP safety annotations"). + // by server.metadata.test.ts ("sets every safety hint explicitly"). readOnlyHint: boolean; destructiveHint: boolean; idempotentHint?: boolean; diff --git a/packages/mcp-core/vitest.config.ts b/packages/mcp-core/vitest.config.ts index 24b49d693..a4451b349 100644 --- a/packages/mcp-core/vitest.config.ts +++ b/packages/mcp-core/vitest.config.ts @@ -38,6 +38,10 @@ export default defineConfig({ reporter: ["text", "json", "html"], include: ["**/*.ts"], }, - setupFiles: ["dotenv/config", "src/test-setup.ts"], + setupFiles: [ + "dotenv/config", + "src/test-setup.ts", + "src/test-utils/structured-output-guard.ts", + ], }, }); diff --git a/packages/mcp-server/package.json b/packages/mcp-server/package.json index 5af348097..56bdcda54 100644 --- a/packages/mcp-server/package.json +++ b/packages/mcp-server/package.json @@ -41,7 +41,7 @@ ], "scripts": { "bundle": "pnpm --filter @sentry/mcp-core run build && pnpm --filter sentry run generate && tsx script/bundle.ts", - "test:bundle": "node --test test/bundle.test.mjs" + "test:bundle": "node --test test/bundle.test.mjs test/server-json.test.mjs" }, "devDependencies": { "esbuild": "^0.28.1", diff --git a/packages/mcp-server/test/server-json.test.mjs b/packages/mcp-server/test/server-json.test.mjs new file mode 100644 index 000000000..ee1fa3e9d --- /dev/null +++ b/packages/mcp-server/test/server-json.test.mjs @@ -0,0 +1,110 @@ +/** + * Checks the MCP Registry manifest (`server.json` at the repository root) + * against this package and the CLI it bundles. Run `pnpm run bundle` first: + * the env var check reads the bundled `mcp --help`. + * + * Schema validation against the registry's `server.schema.json` is not here: + * the schema is not vendored, and tests do not use the network. + */ +import assert from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import { mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { after, test } from "node:test"; + +const packageRoot = new URL("..", import.meta.url).pathname; +const manifest = JSON.parse( + readFileSync(join(packageRoot, "package.json"), "utf-8"), +); +const serverJson = JSON.parse( + readFileSync(join(packageRoot, "../../server.json"), "utf-8"), +); +const configDir = mkdtempSync(join(tmpdir(), "sentry-mcp-test-")); +after(() => rmSync(configDir, { recursive: true, force: true })); +const npmPackage = serverJson.packages?.find( + (entry) => entry.registryType === "npm", +); + +/** Env vars the CLI's `mcp --help` lists under "Environment variables". */ +function sentryEnvVarsFromHelp() { + const result = spawnSync( + process.execPath, + [join(packageRoot, "dist/sentry-mcp.cjs"), "mcp", "--help"], + { + encoding: "utf-8", + env: { + PATH: process.env.PATH, + SENTRY_CONFIG_DIR: configDir, + SENTRY_CLI_NO_TELEMETRY: "1", + }, + timeout: 30_000, + }, + ); + assert.equal(result.status, 0, result.stderr); + const section = result.stdout.split("Environment variables:\n")[1]; + assert.ok(section, "mcp --help lists no environment variables"); + return new Set(section.split("\n\n")[0].match(/\bSENTRY_[A-Z_]+\b/g)); +} + +/** Each check returns a failure message, or nothing when it passes. */ +const CHECKS = { + "name is the package's mcpName": () => + serverJson.name === manifest.mcpName + ? undefined + : `name is ${serverJson.name}, mcpName is ${manifest.mcpName}`, + "the npm package is this stdio package": () => + npmPackage?.identifier === manifest.name && + npmPackage.transport?.type === "stdio" + ? undefined + : `npm package is ${JSON.stringify(npmPackage)}`, + "repository.url is getsentry/toolkit": () => + serverJson.repository?.url === "https://github.com/getsentry/toolkit" + ? undefined + : `repository.url is ${serverJson.repository?.url}`, + "title is set": () => + serverJson.title?.trim() ? undefined : "title is missing", + "websiteUrl is set": () => + serverJson.websiteUrl?.startsWith("https://") + ? undefined + : `websiteUrl is ${serverJson.websiteUrl}`, + "icons are set": () => + serverJson.icons?.length > 0 && + serverJson.icons.every( + (icon) => icon.src?.startsWith("https://") && icon.mimeType, + ) + ? undefined + : `icons are ${JSON.stringify(serverJson.icons)}`, + "environment variables match the CLI": () => { + const declared = npmPackage?.environmentVariables ?? []; + const byName = new Map( + declared.map((variable) => [variable.name, variable]), + ); + const expected = sentryEnvVarsFromHelp(); + const problems = []; + const names = new Set(byName.keys()); + if (names.symmetricDifference(expected).size > 0) { + problems.push( + `declares ${[...names].join(", ")}; mcp --help lists ${[...expected].join(", ")}`, + ); + } + // `auth login` works without a token, so the token is optional. + const token = byName.get("SENTRY_AUTH_TOKEN"); + if (token && (token.isRequired !== false || token.isSecret !== true)) { + problems.push("SENTRY_AUTH_TOKEN must be optional and secret"); + } + for (const name of ["SENTRY_HOST", "SENTRY_URL"]) { + const variable = byName.get(name); + if (variable && (variable.isRequired || variable.isSecret)) { + problems.push(`${name} must be optional and not secret`); + } + } + return problems.length > 0 ? problems.join("; ") : undefined; + }, +}; + +for (const [name, check] of Object.entries(CHECKS)) { + test(`server.json: ${name}`, () => { + assert.equal(check(), undefined); + }); +} From 61c0e154a3a84373a08eb89aedf68921f7c7936d Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 5 Nov 2026 22:58:17 +0000 Subject: [PATCH 2/3] test(mcp): Check titles of tools missing from tools/list The metadata linter only sees listed tools, so deprecated aliases and the DSN-gated feedback tool lost their title-uniqueness check when it moved out of tools.test.ts. Check those definitions directly. Co-Authored-By: David Cramer <23610+dcramer@users.noreply.github.com> --- docs/contributing/adding-tools.md | 6 ++++-- packages/mcp-core/src/tools/tools.test.ts | 20 +++++++++++++++++++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/docs/contributing/adding-tools.md b/docs/contributing/adding-tools.md index 31930ef51..60aa794a5 100644 --- a/docs/contributing/adding-tools.md +++ b/docs/contributing/adding-tools.md @@ -109,8 +109,10 @@ export default defineTool({ **REQUIRED**: Every tool sets `title`, a short Title Case display name such as `"Search Issues"`. The server exports it as the MCP tool `title`, which clients show instead of the snake_case `name`. Titles must be unique across tools; the -[metadata linter](tool-responses.md#metadata-linter) enforces this. Deprecated aliases add a ` (Deprecated)` -suffix, for example `"Search AI Conversations (Deprecated)"`. +[metadata linter](tool-responses.md#metadata-linter) enforces this for listed +tools, and `tools.test.ts` for tools that never appear in tools/list. Deprecated +aliases add a ` (Deprecated)` suffix, for example +`"Search AI Conversations (Deprecated)"`. ### Safety Annotations diff --git a/packages/mcp-core/src/tools/tools.test.ts b/packages/mcp-core/src/tools/tools.test.ts index 3cb7a197b..efdea86c0 100644 --- a/packages/mcp-core/src/tools/tools.test.ts +++ b/packages/mcp-core/src/tools/tools.test.ts @@ -60,7 +60,25 @@ test(`all tool descriptions under maximum length`, () => { } }); -// server.metadata.test.ts checks titles and that every hint is set. +// server.metadata.test.ts checks the titles of listed tools. These tools never +// appear in its tools/list: deprecated aliases, and the feedback tool, which +// is hidden without a telemetry DSN. +test("unlisted tools declare a unique display title", () => { + const all = Object.values(tools.default); + const unlisted = all.filter( + (tool) => tool.deprecated || tool.name === "submit_feedback_to_sentry_team", + ); + assert(unlisted.length > 0, "expected deprecated or hidden tools"); + for (const tool of unlisted) { + assert(tool.title.trim(), `${tool.name} must define a non-empty title`); + const owner = all.find( + (other) => other !== tool && other.title === tool.title, + ); + assert(!owner, `${tool.name} reuses the title of ${owner?.name}`); + } +}); + +// server.metadata.test.ts checks that every hint is set. test("no tool is both read-only and destructive", () => { for (const tool of Object.values(tools.default)) { assert( From 3fcb6866a915789160e76f25b7248f8aadb408d4 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 5 Nov 2026 23:36:59 +0000 Subject: [PATCH 3/3] test(mcp): Prune pending outputSchema entries after merges Drop the pending entries whose outputSchema PRs have merged, move the search_metrics, search_profiles and search_replays tests to structured search stubs, and mark the guard probe's deliberately invalid result as a type error now that handler results are typed from outputSchema. --- packages/mcp-core/src/server.metadata.test.ts | 10 ++------- .../src/test-utils/dataset-search-tool.ts | 5 +++-- .../src/test-utils/search-events-stub.ts | 21 +++++++++++++++++++ .../src/tools/catalog/search-metrics.test.ts | 14 +++++++++---- .../src/tools/catalog/search-profiles.test.ts | 14 +++++++++---- .../src/tools/catalog/search-replays.test.ts | 14 +++++++++---- 6 files changed, 56 insertions(+), 22 deletions(-) diff --git a/packages/mcp-core/src/server.metadata.test.ts b/packages/mcp-core/src/server.metadata.test.ts index 9b8f3c9ef..c90f6cf18 100644 --- a/packages/mcp-core/src/server.metadata.test.ts +++ b/packages/mcp-core/src/server.metadata.test.ts @@ -58,14 +58,6 @@ const OUTPUT_SCHEMA_EXEMPT: Record = { * its PR merges; a test below fails until you do. */ const OUTPUT_SCHEMA_PENDING: Record = { - get_issue_activity: "pending getsentry/toolkit#2001", - search_issues: "pending getsentry/toolkit#2004", - search_metrics: "pending getsentry/toolkit#2010", - search_profiles: "pending getsentry/toolkit#2012", - get_dashboard_details: "pending getsentry/toolkit#2018", - get_monitor_details: "pending getsentry/toolkit#2020", - get_profile_details: "pending getsentry/toolkit#2024", - search_replays: "pending getsentry/toolkit#2025", search_attributes: "pending: experimental-only, no outputSchema PR yet", }; @@ -301,6 +293,8 @@ describe("structured output", () => { destructiveHint: false, openWorldHint: false, }, + // @ts-expect-error -- the probe breaks its schema on purpose; the guard + // catches what types miss, such as unvalidated upstream data. handler: async () => ({ structuredContent: { count: "one" } }), }); diff --git a/packages/mcp-core/src/test-utils/dataset-search-tool.ts b/packages/mcp-core/src/test-utils/dataset-search-tool.ts index e8c31facc..4ec2ec7cd 100644 --- a/packages/mcp-core/src/test-utils/dataset-search-tool.ts +++ b/packages/mcp-core/src/test-utils/dataset-search-tool.ts @@ -21,8 +21,9 @@ const params = { * runSearchEvents: vi.fn(async () => "ok"), * })); * - * A tool with an outputSchema must return `SEARCH_EVENTS_STUB` from - * `./search-events-stub` instead of "ok"; search-errors.test.ts shows how. + * A tool with an outputSchema must return `SEARCH_EVENTS_STUB` (or + * `SEARCH_REPLAYS_STUB` for search_replays) from `./search-events-stub` + * instead of "ok"; search-errors.test.ts shows how. */ export async function inspectDatasetSearchTool

(tool: { inputSchema: object; diff --git a/packages/mcp-core/src/test-utils/search-events-stub.ts b/packages/mcp-core/src/test-utils/search-events-stub.ts index 3b510294d..a3c5533e7 100644 --- a/packages/mcp-core/src/test-utils/search-events-stub.ts +++ b/packages/mcp-core/src/test-utils/search-events-stub.ts @@ -1,3 +1,4 @@ +import type { SearchReplaysStructuredContent } from "../tools/support/search-events/replays"; import type { SearchEventsStructuredContent } from "../tools/support/search-events/structured"; import type { StructuredToolOutput } from "../tools/types"; @@ -27,3 +28,23 @@ export const SEARCH_EVENTS_STUB: StructuredToolOutput = + { + structuredContent: { + query: "anything", + explanation: null, + executedSearch: { + query: "", + sort: "-started_at", + environment: [], + timeRange: { statsPeriod: null, start: null, end: null }, + }, + url: "https://sentry.io/", + presentationHint: "", + notes: [], + replays: [], + suggestedActions: [], + }, + }; diff --git a/packages/mcp-core/src/tools/catalog/search-metrics.test.ts b/packages/mcp-core/src/tools/catalog/search-metrics.test.ts index 5307a3334..2b8d895a2 100644 --- a/packages/mcp-core/src/tools/catalog/search-metrics.test.ts +++ b/packages/mcp-core/src/tools/catalog/search-metrics.test.ts @@ -3,10 +3,16 @@ import { inspectDatasetSearchTool } from "../../test-utils/dataset-search-tool"; import searchMetrics from "./search-metrics"; // The shared handler is covered by search-events.test.ts. -vi.mock("../support/search-events/search", async (importOriginal) => ({ - ...(await importOriginal()), - runSearchEvents: vi.fn(async () => "ok"), -})); +vi.mock("../support/search-events/search", async (importOriginal) => { + const { SEARCH_EVENTS_STUB } = + await import("../../test-utils/search-events-stub"); + return { + ...(await importOriginal< + typeof import("../support/search-events/search") + >()), + runSearchEvents: vi.fn(async () => SEARCH_EVENTS_STUB), + }; +}); it("search_metrics runs the shared handler locked to metrics", async () => { expect(await inspectDatasetSearchTool(searchMetrics)).toMatchInlineSnapshot(` diff --git a/packages/mcp-core/src/tools/catalog/search-profiles.test.ts b/packages/mcp-core/src/tools/catalog/search-profiles.test.ts index 6d995d81e..1ace88f8d 100644 --- a/packages/mcp-core/src/tools/catalog/search-profiles.test.ts +++ b/packages/mcp-core/src/tools/catalog/search-profiles.test.ts @@ -3,10 +3,16 @@ import { inspectDatasetSearchTool } from "../../test-utils/dataset-search-tool"; import searchProfiles from "./search-profiles"; // The shared handler is covered by search-events.test.ts. -vi.mock("../support/search-events/search", async (importOriginal) => ({ - ...(await importOriginal()), - runSearchEvents: vi.fn(async () => "ok"), -})); +vi.mock("../support/search-events/search", async (importOriginal) => { + const { SEARCH_EVENTS_STUB } = + await import("../../test-utils/search-events-stub"); + return { + ...(await importOriginal< + typeof import("../support/search-events/search") + >()), + runSearchEvents: vi.fn(async () => SEARCH_EVENTS_STUB), + }; +}); it("search_profiles runs the shared handler locked to profiles", async () => { expect(await inspectDatasetSearchTool(searchProfiles)).toMatchInlineSnapshot(` diff --git a/packages/mcp-core/src/tools/catalog/search-replays.test.ts b/packages/mcp-core/src/tools/catalog/search-replays.test.ts index 1dbabec37..a733ac30e 100644 --- a/packages/mcp-core/src/tools/catalog/search-replays.test.ts +++ b/packages/mcp-core/src/tools/catalog/search-replays.test.ts @@ -3,10 +3,16 @@ import { inspectDatasetSearchTool } from "../../test-utils/dataset-search-tool"; import searchReplays from "./search-replays"; // The shared handler is covered by search-events.test.ts. -vi.mock("../support/search-events/search", async (importOriginal) => ({ - ...(await importOriginal()), - runSearchEvents: vi.fn(async () => "ok"), -})); +vi.mock("../support/search-events/search", async (importOriginal) => { + const { SEARCH_REPLAYS_STUB } = + await import("../../test-utils/search-events-stub"); + return { + ...(await importOriginal< + typeof import("../support/search-events/search") + >()), + runSearchEvents: vi.fn(async () => SEARCH_REPLAYS_STUB), + }; +}); it("search_replays runs the shared handler locked to replays", async () => { expect(await inspectDatasetSearchTool(searchReplays)).toMatchInlineSnapshot(`