diff --git a/docs/contributing/adding-tools.md b/docs/contributing/adding-tools.md index b952bb4c3..60aa794a5 100644 --- a/docs/contributing/adding-tools.md +++ b/docs/contributing/adding-tools.md @@ -108,16 +108,18 @@ 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)` -suffix, for example `"Search AI Conversations (Deprecated)"`. +show instead of the snake_case `name`. Titles must be unique across tools; the +[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 **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 +343,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..c90f6cf18 --- /dev/null +++ b/packages/mcp-core/src/server.metadata.test.ts @@ -0,0 +1,344 @@ +/** + * 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 = { + 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, + }, + // @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" } }), + }); + + 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..4ec2ec7cd 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,10 @@ const params = { * ...(await importOriginal()), * runSearchEvents: vi.fn(async () => "ok"), * })); + * + * 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 new file mode 100644 index 000000000..a3c5533e7 --- /dev/null +++ b/packages/mcp-core/src/test-utils/search-events-stub.ts @@ -0,0 +1,50 @@ +import type { SearchReplaysStructuredContent } from "../tools/support/search-events/replays"; +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: [], + }, + }; + +/** {@link SEARCH_EVENTS_STUB} for search_replays, which has its own schema. */ +export const SEARCH_REPLAYS_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/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-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(` 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..efdea86c0 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,34 @@ 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)) { +// 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 = seen.get(tool.title); - assert(!owner, `${tool.name} reuses the title of ${owner}`); - seen.set(tool.title, tool.name); + const owner = all.find( + (other) => other !== tool && other.title === tool.title, + ); + assert(!owner, `${tool.name} reuses the title of ${owner?.name}`); } }); -test("all tools declare complete MCP safety annotations", () => { +// 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)) { - // 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); + }); +}