diff --git a/README.md b/README.md index 9eb14804..670a5bb4 100644 --- a/README.md +++ b/README.md @@ -290,15 +290,22 @@ The **TUI** had the same gap and is worth checking against the same server (`--t #### Unportable tool schemas -`unportable-schemas-http.json` serves three tools, two of whose advertised +`unportable-schemas-http.json` serves four tools, three of whose advertised schemas carry constructs that are legal JSON Schema and are refused or -mishandled by real MCP clients: `echo` declares `outputSchema.properties.data` -as a bare `true` (what Go's `jsonschema` package emits for `interface{}` — -the case reported in [#1005](https://github.com/modelcontextprotocol/inspector/issues/1005)), -an array-form `"type": ["null","boolean"]`, and an `opts` property that -constrains nothing; `add` points a property at a remote `$ref`. `get_temp` is -left clean, so a flagged tool sits next to an unflagged one. Plain -streamable-HTTP — connect with the **default (legacy)** protocol era. +mishandled by real MCP clients: + +| Tool | What it carries | +| --- | --- | +| `get_temp` | `outputSchema.properties.data` as a bare `true` — what Go's `jsonschema` package emits for `interface{}`, the case reported in [#1005](https://github.com/modelcontextprotocol/inspector/issues/1005) | +| `echo` | an array-form `"type": ["null","boolean"]`, and an `opts` property that constrains nothing | +| `add` | a property pointing at a remote `$ref` | +| `get_weather` | nothing — left clean, so a flagged tool sits beside an unflagged one | + +Plain streamable-HTTP — connect with the **default (legacy)** protocol era. +Every tool here is still **runnable**: the override replaces only the +*advertised* schema, and the flagship bare-`true` rides `get_temp` (which +returns structured content) rather than `echo` — see the caveat under +[Try it](#unportable-tool-schemas) below. The Inspector is where a server author looks first, so a construct that will fail downstream is named here rather than passed through silently. All three @@ -313,8 +320,9 @@ mcp-inspector --cli http://127.0.0.1:6603/mcp --method tools/list --strict # e - **CLI** — `--strict` prints the full report (path, issue, suggested fix) on stderr and exits `6` on an error-severity finding; without it, one summary line. See [Schema portability](./clients/cli/README.md#schema-portability---strict). -- **TUI** — the tools list marks `echo` with a red `!` and `add` with a yellow - `?`; the detail pane lists each finding under **Schema Portability**. +- **TUI** — the tools list marks `get_temp` with a red `!` and `echo`/`add` + with a yellow `?`; the detail pane lists each finding under **Schema + Portability**. - **Web** — the Tools sidebar row carries the same flag as a hover-labelled icon, and selecting the tool shows a **Schema portability** section above the argument form. @@ -329,6 +337,13 @@ test server's `rawToolSchemas` override, because the Zod-built presets cannot express any of them — which is the same reason a real server hits this only when its schemas come from another generator. +⚠️ **If you add an `outputSchema` override of your own, put it on a tool that +returns structured content.** A conforming client validates a tool result +against the advertised output schema, so an override on a preset that returns +none makes every call to it fail with "declares an output schema but returned +no structured content" — a confusing thing to hit from a fixture. That is why +the bare `true` rides `get_temp` here and not `echo`. + #### RFC 6570 resource templates `rfc6570-templates-http.json` serves two resource templates straight out of [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) — `events_by_topic` (`foobar://events/{topic}`) and `events_by_query` (`foobar://events{?topic}`) — each echoing the URI it was matched against, plus a plain `foobar://events` resource (see below). Plain streamable-HTTP; connect with the **default (legacy)** protocol era. diff --git a/clients/cli/__tests__/schema-lint-report.test.ts b/clients/cli/__tests__/schema-lint-report.test.ts index 0ab49217..f7e700ef 100644 --- a/clients/cli/__tests__/schema-lint-report.test.ts +++ b/clients/cli/__tests__/schema-lint-report.test.ts @@ -207,6 +207,9 @@ describe("emitResult — schema lint wiring (#1005)", () => { }); it("does not lint an --app-info run", async () => { + // `runCli` rejects `--strict --app-info` outright (see below), so this is + // the defensive half: any other Node runner reusing `emitResult` gets the + // app-info early return rather than a report interleaved with its NDJSON. await emitResult( DIRTY_LIST, { hasApp: true, toolName: "info" }, @@ -248,4 +251,23 @@ describe("--strict argument validation", () => { ]), ).rejects.toThrow("--strict requires --method tools/list."); }); + + it("is rejected alongside --app-info rather than silently ignored", async () => { + // `tools/list --app-info` returns NDJSON straight from `runMethod` and + // never reaches `emitResult`, so accepting the pair would hand a CI caller + // a `--strict` gate that can never fail. + await expect( + runCli([ + "node", + "cli", + "--cli", + "--method", + "tools/list", + "--strict", + "--app-info", + "--server-url", + "http://127.0.0.1:1/mcp", + ]), + ).rejects.toThrow("--strict cannot be combined with --app-info"); + }); }); diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index ba2de59a..57300275 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -1013,6 +1013,17 @@ async function parseArgs(argv?: string[]): Promise { throw new Error("--strict requires --method tools/list."); } + // `tools/list --app-info` takes a different path out of `runMethod` — it + // returns NDJSON that the caller writes directly, never reaching + // `emitResult` where the lint runs. Rejecting the pair is the honest + // outcome: silently accepting it would give a CI caller a `--strict` gate + // that can never fail. + if (options.strict && options.appInfo) { + throw new Error( + "--strict cannot be combined with --app-info; run tools/list twice, once for each.", + ); + } + // --tool-args-json passes arguments verbatim with no key=value coercion (so // `"012"` stays a string and nested objects work without shell escaping). let toolArg = options.toolArg; diff --git a/clients/web/src/components/groups/ToolListItem/ToolListItem.tsx b/clients/web/src/components/groups/ToolListItem/ToolListItem.tsx index a74b3f09..b1ca1003 100644 --- a/clients/web/src/components/groups/ToolListItem/ToolListItem.tsx +++ b/clients/web/src/components/groups/ToolListItem/ToolListItem.tsx @@ -57,6 +57,11 @@ const ListItemButton = UnstyledButton.withProps({ // Schema-portability flag (#1005). Sits in the row rather than only in the // detail panel so a problem tool is visible without clicking each one — the // findings are rare enough that a marked row means something. +// +// Coloured from the same `--inspector-*` severity tokens the detail panel's +// findings list uses, rather than from Mantine's `color="yellow"`: those are +// the pairings this app already tunes against its own surfaces in both colour +// schemes, and the default yellow is what failed the a11y check there. const SchemaFlagIcon = ThemeIcon.withProps({ size: "xs", variant: "transparent", @@ -86,7 +91,11 @@ export function ToolListItem({ tool, selected, onClick }: ToolListItemProps) { withArrow > { }); it("reports nothing when a tool declares neither schema", () => { - const bare = { name: "t" } as unknown as Tool; - expect(lintToolSchemas(bare)).toEqual([]); + expect( + lintToolSchemas( + tool({ inputSchema: undefined, outputSchema: undefined }), + ), + ).toEqual([]); }); }); @@ -174,6 +177,16 @@ describe("lintToolSchemas — boolean-schema", () => { { type: "object", propertyNames: true, properties: {} }, "inputSchema.propertyNames", ], + [ + "a dependentSchemas entry", + { type: "object", dependentSchemas: { a: true }, properties: {} }, + "inputSchema.dependentSchemas.a", + ], + [ + "a draft-07 schema-valued dependencies entry", + { type: "object", dependencies: { a: true }, properties: {} }, + "inputSchema.dependencies.a", + ], ["the schema root", true, "inputSchema"], ])("flags a bare boolean under %s", (_label, inputSchema, path) => { const findings = lintToolSchemas(tool({ inputSchema })); @@ -192,7 +205,7 @@ describe("lintToolSchemas — boolean-schema", () => { }); describe("lintToolSchemas — type-union", () => { - it("flags the nullable array form and suggests the single type", () => { + it("flags the nullable array form and suggests anyOf branches", () => { const findings = lintToolSchemas( tool({ inputSchema: { @@ -203,10 +216,27 @@ describe("lintToolSchemas — type-union", () => { ); expect(rules(findings)).toEqual(["type-union"]); expect(findings[0]!.severity).toBe("warning"); - expect(findings[0]!.suggestion).toContain('"type": "boolean"'); + expect(findings[0]!.suggestion).toContain( + '{"anyOf": [{"type": "null"}, {"type": "boolean"}]}', + ); }); - it("suggests anyOf when more than one non-null member survives", () => { + it("never suggests un-requiring the property as the equivalent fix", () => { + // Absent and `null` are different contracts, so "drop it from `required`" + // is not a portable spelling of the same schema — it is a change to it. + const findings = lintToolSchemas( + tool({ + inputSchema: { + type: "object", + properties: { a: { type: ["null", "string"] } }, + }, + }), + ); + expect(findings[0]!.suggestion).toContain("absent is not the same as"); + expect(findings[0]!.suggestion).not.toMatch(/leave the property out/); + }); + + it("keeps every member in the suggestion when none is null", () => { const findings = lintToolSchemas( tool({ inputSchema: { @@ -215,10 +245,12 @@ describe("lintToolSchemas — type-union", () => { }, }), ); - expect(findings[0]!.suggestion).toContain("anyOf"); + expect(findings[0]!.suggestion).toContain( + '{"anyOf": [{"type": "string"}, {"type": "number"}]}', + ); }); - it("ignores non-string members when picking the suggestion", () => { + it("drops non-string members from the suggested branches", () => { const findings = lintToolSchemas( tool({ inputSchema: { @@ -227,7 +259,19 @@ describe("lintToolSchemas — type-union", () => { }, }), ); - expect(findings[0]!.suggestion).toContain('"type": "string"'); + expect(findings[0]!.suggestion).toContain( + '{"anyOf": [{"type": "string"}]}', + ); + }); + + it("falls back to a placeholder when no member is a string", () => { + const findings = lintToolSchemas( + tool({ + inputSchema: { type: "object", properties: { a: { type: [3] } } }, + }), + ); + expect(rules(findings)).toEqual(["type-union"]); + expect(findings[0]!.suggestion).toContain("anyOf"); }); it("does not raise non-object-root for an array-typed root", () => { @@ -258,6 +302,51 @@ describe("lintToolSchemas — untyped-schema", () => { expect(paths(findings)).toEqual(["inputSchema.properties.a"]); }); + it("flags a schema whose only keyword is unrecognized", () => { + // JSON Schema ignores keywords it does not know, so `{"vendorHint": true}` + // accepts every value. A denylist of known annotations would let it pass + // simply because the keyword is unfamiliar — hence the allowlist. + const findings = lintToolSchemas( + tool({ + inputSchema: { + type: "object", + properties: { a: { vendorHint: true, "x-internal": 1 } }, + }, + }), + ); + expect(rules(findings)).toEqual(["untyped-schema"]); + }); + + it("flags a definition-only schema", () => { + // `$defs` holds subschemas for *other* schemas to reference; it constrains + // the current instance not at all. + const findings = lintToolSchemas( + tool({ + inputSchema: { + type: "object", + properties: { a: { $defs: { x: { type: "string" } } } }, + }, + }), + ); + expect(rules(findings)).toEqual(["untyped-schema"]); + expect(paths(findings)).toEqual(["inputSchema.properties.a"]); + }); + + it.each([ + ["pattern", { pattern: "^a" }], + ["format", { format: "email" }], + ["required", { required: ["a"] }], + ["minimum", { minimum: 0 }], + ["contains", { contains: { type: "string" } }], + ["propertyNames", { propertyNames: { pattern: "^a" } }], + ["dependentRequired", { dependentRequired: { a: ["b"] } }], + ])("does not flag a schema constrained only by %s", (_label, sub) => { + const findings = lintToolSchemas( + tool({ inputSchema: { type: "object", properties: { a: sub } } }), + ); + expect(findings).toEqual([]); + }); + it("flags an entirely empty root schema", () => { const findings = lintToolSchemas(tool({ inputSchema: {} })); expect(rules(findings)).toEqual(["untyped-schema"]); @@ -277,12 +366,14 @@ describe("lintToolSchemas — non-object-root", () => { expect(findings[0]!.issue).toContain("inputSchema"); }); - it("names outputSchema when that is the offending root", () => { - const findings = lintToolSchemas( - tool({ outputSchema: { type: "string" } }), + it("does NOT flag a non-object outputSchema root", () => { + // The SDK's `Tool` schema types `inputSchema` with `type: literal("object")` + // but `outputSchema` as a plain loose object with no `type` constraint, so + // a string-typed output is conforming. Flagging it would report an error + // MCP does not require and would fail `--strict` on a valid server. + expect(lintToolSchemas(tool({ outputSchema: { type: "string" } }))).toEqual( + [], ); - expect(rules(findings)).toEqual(["non-object-root"]); - expect(findings[0]!.issue).toContain("outputSchema"); }); it("does not flag a nested non-object schema", () => { @@ -332,6 +423,21 @@ describe("lintToolSchemas — walk robustness", () => { expect(findings).toEqual([]); }); + it("skips the draft-07 property-name-array form of dependencies", () => { + // `dependencies: {a: ["b"]}` is a required-property list, not a schema + // position, so the walk must not descend into it. + const findings = lintToolSchemas( + tool({ + inputSchema: { + type: "object", + properties: { a: { type: "string" }, b: { type: "string" } }, + dependencies: { a: ["b"] }, + }, + }), + ); + expect(findings).toEqual([]); + }); + it("skips keyword values of the wrong shape", () => { const findings = lintToolSchemas( tool({ diff --git a/clients/web/src/test/integration/mcp/raw-tool-schemas.test.ts b/clients/web/src/test/integration/mcp/raw-tool-schemas.test.ts new file mode 100644 index 00000000..76791dae --- /dev/null +++ b/clients/web/src/test/integration/mcp/raw-tool-schemas.test.ts @@ -0,0 +1,244 @@ +import { describe, it, expect, afterEach } from "vitest"; +import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; +import { lintTools } from "@inspector/core/json/schemaLint.js"; +import { + createTestServerHttp, + type TestServerHttp, + createTestServerInfo, + createEchoTool, + createGetWeatherTool, + createGetTempTool, +} from "@modelcontextprotocol/inspector-test-server"; +import type { ServerConfig } from "@modelcontextprotocol/inspector-test-server"; + +/** + * Live coverage of `ServerConfig.rawToolSchemas` (#1005) — the only way this + * repo can advertise a tool schema containing a bare `true`, an array-form + * `type`, or a remote `$ref`, since every preset builds its schema from Zod + * and Zod cannot emit any of them. + * + * The lint itself is unit-tested against hand-built `Tool` objects, which + * bypasses this path entirely: handler installation, config resolution, and + * the interaction with pagination and duplication could all regress while + * those stayed green. This file drives the real server over a real transport + * and asserts on what actually reaches the wire. + */ +describe("rawToolSchemas override (#1005)", () => { + let client: InspectorClient | null = null; + let server: TestServerHttp | null = null; + + afterEach(async () => { + if (client) { + try { + await client.disconnect(); + } catch { + // ignore + } + client = null; + } + if (server) { + try { + await server.stop(); + } catch { + // ignore + } + server = null; + } + }); + + async function start(config: Partial): Promise { + const started = createTestServerHttp({ + serverInfo: createTestServerInfo("raw-tool-schemas-test", "1.0.0"), + tools: [createEchoTool(), createGetWeatherTool()], + ...config, + }); + await started.start(); + server = started; + return started; + } + + async function connect(url: string): Promise { + const connected = new InspectorClient( + { type: "streamable-http", url }, + { environment: { transport: createTransportNode } }, + ); + await connected.connect(); + client = connected; + return connected; + } + + it("advertises the raw document, and the lint finds it end to end", async () => { + const started = await start({ + rawToolSchemas: { + echo: { + inputSchema: { + type: "object", + properties: { + message: { type: "string" }, + show_ids: { type: ["null", "boolean"] }, + }, + }, + outputSchema: { + type: "object", + properties: { data: true }, + }, + }, + }, + }); + const connected = await connect(started.url); + + const { tools } = await connected.listAllTools(); + const echo = tools.find((t) => t.name === "echo"); + + // The bare `true` survives the wire — a Zod-built schema could not + // produce it, so its presence here proves the override is what is served. + expect(echo?.outputSchema).toEqual({ + type: "object", + properties: { data: true }, + }); + expect( + (echo?.inputSchema as { properties?: Record }).properties + ?.show_ids, + ).toEqual({ type: ["null", "boolean"] }); + + const results = lintTools(tools); + expect(results.map((r) => r.toolName)).toEqual(["echo"]); + expect(results[0]?.findings.map((f) => f.rule).sort()).toEqual([ + "boolean-schema", + "type-union", + ]); + }); + + it("leaves an unnamed tool's real schema alone", async () => { + const started = await start({ + rawToolSchemas: { echo: { outputSchema: { type: "object" } } }, + }); + const connected = await connect(started.url); + + const { tools } = await connected.listAllTools(); + const weather = tools.find((t) => t.name === "get_weather"); + // get_weather keeps the Zod-derived schema, which is portable. + expect(weather?.inputSchema).toMatchObject({ type: "object" }); + expect(lintTools(tools)).toEqual([]); + }); + + it("replaces only the schema named, leaving the sibling untouched", async () => { + const started = await start({ + rawToolSchemas: { + echo: { outputSchema: { type: "object", properties: { data: true } } }, + }, + }); + const connected = await connect(started.url); + + const { tools } = await connected.listAllTools(); + const echo = tools.find((t) => t.name === "echo"); + // Only `outputSchema` was overridden, so the Zod-built input survives. + expect(echo?.inputSchema).toMatchObject({ type: "object" }); + expect( + (echo?.inputSchema as { properties?: Record }) + .properties, + ).toHaveProperty("message"); + }); + + it("ignores a name that is not registered", async () => { + const started = await start({ + rawToolSchemas: { not_a_tool: { inputSchema: true } }, + }); + const connected = await connect(started.url); + + const { tools } = await connected.listAllTools(); + expect(tools.map((t) => t.name)).toEqual(["echo", "get_weather"]); + expect(lintTools(tools)).toEqual([]); + }); + + it("still calls the tool when only the input schema was overridden", async () => { + const started = await start({ + rawToolSchemas: { + echo: { + inputSchema: { + type: "object", + properties: { message: { type: "string" }, opts: {} }, + }, + }, + }, + }); + const connected = await connect(started.url); + + // The handler and its real Zod validation are untouched by the override — + // the fixture would be misleading if the tool it flags could not be run. + const { tools } = await connected.listAllTools(); + const echo = tools.find((t) => t.name === "echo")!; + const result = await connected.callTool(echo, { message: "hi" }); + expect(JSON.stringify(result)).toContain("hi"); + }); + + it("makes the call fail when an outputSchema is put on a tool with no structured content", async () => { + // Pinning the sharp edge documented on `ServerConfig.rawToolSchemas`: a + // conforming client validates results against the advertised output + // schema, so an override on a preset that returns none breaks every call. + // This is why the showcase config puts its bare-`true` outputSchema on + // `get_temp` (which does return structured content) rather than on `echo`. + const started = await start({ + rawToolSchemas: { + echo: { outputSchema: { type: "object", properties: { data: true } } }, + }, + }); + const connected = await connect(started.url); + + const { tools } = await connected.listAllTools(); + const echo = tools.find((t) => t.name === "echo")!; + await expect(connected.callTool(echo, { message: "hi" })).rejects.toThrow( + /returned no structured content/, + ); + }); + + it("keeps a structured-content tool callable under an outputSchema override", async () => { + // The shape the showcase config actually ships: the bare `true` rides a + // tool that returns structured content, and the override stays permissive + // enough (no `required`) that the real payload still validates — so the + // flagged tool is still runnable from the Tools tab. + const started = await start({ + tools: [createGetTempTool(), createEchoTool()], + rawToolSchemas: { + get_temp: { + outputSchema: { + type: "object", + properties: { data: true, city: { type: "string" } }, + }, + }, + }, + }); + const connected = await connect(started.url); + + const { tools } = await connected.listAllTools(); + const getTemp = tools.find((t) => t.name === "get_temp")!; + expect(lintTools(tools).map((r) => r.toolName)).toEqual(["get_temp"]); + + const result = await connected.callTool(getTemp, { + city: "Oslo", + units: "C", + }); + expect(JSON.stringify(result)).toContain("Oslo"); + }); + + it("applies before duplication, so both copies carry the raw schema", async () => { + const started = await start({ + duplicateToolNames: ["echo"], + rawToolSchemas: { + echo: { outputSchema: { type: "object", properties: { data: true } } }, + }, + }); + const connected = await connect(started.url); + + const { tools } = await connected.listAllTools(); + const echoes = tools.filter((t) => t.name === "echo"); + expect(echoes).toHaveLength(2); + for (const e of echoes) { + expect(e.outputSchema).toEqual({ + type: "object", + properties: { data: true }, + }); + } + }); +}); diff --git a/core/json/schemaLint.ts b/core/json/schemaLint.ts index 3ae0cf9c..50b54484 100644 --- a/core/json/schemaLint.ts +++ b/core/json/schemaLint.ts @@ -103,37 +103,97 @@ const SUBSCHEMA_ARRAY_KEYWORDS = [ "prefixItems", ] as const; -/** Keywords whose value is a name → subschema map. */ +/** + * Keywords whose value is a name → subschema map. + * + * `dependencies` is the pre-2019 form and may hold either a subschema or an + * array of property names; the array case is not a schema position, and the + * walk skips it because a non-object, non-boolean node is not a schema. + */ const SUBSCHEMA_MAP_KEYWORDS = [ "properties", "patternProperties", + "dependentSchemas", + "dependencies", "$defs", "definitions", ] as const; /** - * Keywords that carry no validation meaning — metadata a schema may hold - * alongside its constraints. A schema built *only* from these accepts every - * value, which is what the `untyped-schema` rule reports. + * Keywords that actually constrain the instance being validated — the + * assertion and applicator vocabularies, plus reference keywords. * - * Defining the rule this way, rather than as "has no `type`", is deliberate: - * `{"properties": {…}}` with no `type` does constrain its input, so calling it - * unconstrained would be false. That shape is left alone; the rule fires on - * `{}` and on annotation-only schemas — the object-literal spelling of the - * same "any" that a bare `true` expresses. + * This is an **allowlist**, and that direction is load-bearing. JSON Schema + * ignores keywords it does not recognize, so a schema is unconstrained unless + * something it carries is known to constrain: a denylist of known-annotation + * keywords would let `{"vendorHint": true}` — which accepts every value — + * pass as constrained simply because the keyword is unfamiliar. + * + * Two categories are deliberately absent. **Annotations** (`title`, + * `description`, `default`, `examples`, `deprecated`, `readOnly`, + * `writeOnly`, `$comment`) assert nothing. So do the **definition-only + * containers** `$defs` / `definitions` and the identifier keywords `$id`, + * `$schema`, `$anchor`, `$vocabulary`: they hold or name subschemas for + * *other* schemas to reference, and constrain the current instance not at all. + * + * `format` is included even though 2020-12's default vocabulary treats it as + * an annotation rather than an assertion. A schema declaring one is plainly an + * attempt to constrain, and reporting it as accepting anything would be noise + * rather than a finding an author can act on. + * + * Defining the rule this way, rather than as "has no `type`", is also + * deliberate: `{"properties": {…}}` with no `type` does constrain its input, + * so calling it unconstrained would be false. The rule fires on `{}`, on + * annotation-only schemas, and on `$defs`-only schemas — the object-literal + * spellings of the same "any" that a bare `true` expresses. */ -const ANNOTATION_ONLY_KEYWORDS: ReadonlySet = new Set([ - "title", - "description", - "default", - "examples", - "deprecated", - "readOnly", - "writeOnly", - "$comment", - "$id", - "$schema", - "$anchor", +const CONSTRAINING_KEYWORDS: ReadonlySet = new Set([ + // Assertions + "type", + "enum", + "const", + "multipleOf", + "maximum", + "exclusiveMaximum", + "minimum", + "exclusiveMinimum", + "maxLength", + "minLength", + "pattern", + "format", + "maxItems", + "minItems", + "uniqueItems", + "maxContains", + "minContains", + "maxProperties", + "minProperties", + "required", + "dependentRequired", + // Applicators + "allOf", + "anyOf", + "oneOf", + "not", + "if", + "then", + "else", + "items", + "prefixItems", + "contains", + "additionalItems", + "unevaluatedItems", + "properties", + "patternProperties", + "additionalProperties", + "unevaluatedProperties", + "propertyNames", + "dependentSchemas", + "dependencies", + // References + "$ref", + "$dynamicRef", + "$recursiveRef", ]); /** @@ -263,18 +323,26 @@ function lintNode(node: SchemaRecord, path: string, ctx: WalkContext): void { const type = node.type; if (Array.isArray(type)) { - const nonNull = type.filter( - (t): t is string => typeof t === "string" && t !== "null", - ); + const members = type.filter((t): t is string => typeof t === "string"); + // The suggestion is always `anyOf`, never "drop it from `required`". + // Accepting an explicit JSON `null` and permitting the property to be + // absent are independent: on a required `["null","boolean"]` field, + // un-requiring it would both stop accepting `null` and start accepting + // omission — a different contract, not the same one spelled portably. + // `anyOf` branches each carrying a single `type` are equivalent, and this + // lint treats them as portable. + const branches = members.length > 0 ? members : type.map(() => "…"); add( ctx, "type-union", "warning", path, `\`type\` is an array (${JSON.stringify(type)}). The array form is legal JSON Schema, but several MCP clients read \`type\` as a single string and either reject the tool or drop the constraint.`, - nonNull.length === 1 - ? `Use \`"type": "${nonNull[0]}"\` and leave the property out of \`required\` to express "may be absent".` - : "Split the alternatives into `anyOf` branches, each with a single `type`.", + `Split it into \`anyOf\` branches, each with a single \`type\` — \`{"anyOf": [${branches + .map((t) => `{"type": "${t}"}`) + .join( + ", ", + )}]}\`. (Making the property optional instead is a different contract: absent is not the same as \`null\`.)`, ); } @@ -290,8 +358,8 @@ function lintNode(node: SchemaRecord, path: string, ctx: WalkContext): void { ); } - const constrains = Object.keys(node).some( - (key) => !ANNOTATION_ONLY_KEYWORDS.has(key), + const constrains = Object.keys(node).some((key) => + CONSTRAINING_KEYWORDS.has(key), ); if (!constrains) { add( @@ -306,9 +374,18 @@ function lintNode(node: SchemaRecord, path: string, ctx: WalkContext): void { } /** - * Lint one of a tool's schemas. The root gets one extra rule: MCP requires a - * tool schema to describe an object, and a root of any other type is refused - * by the SDK-side parsers rather than merely degraded. + * Lint one of a tool's schemas. + * + * `inputSchema` gets one extra rule at the root: MCP requires it to describe + * an object, and the SDK enforces that — its `Tool` schema types `inputSchema` + * with `type: literal("object")`, so a non-object root is refused by the + * parser rather than merely degraded. + * + * That rule is **scoped to `inputSchema` deliberately**. The same SDK types + * `outputSchema` as a plain loose object with no `type` constraint at all, so + * a tool whose output is `{"type": "string"}` is conforming; flagging it would + * report an error MCP does not require and would fail `--strict` on a valid + * server. */ function lintSchema( schema: unknown, @@ -316,7 +393,7 @@ function lintSchema( findings: SchemaFinding[], ): void { const ctx: WalkContext = { schema: kind, findings }; - if (isRecord(schema)) { + if (kind === "inputSchema" && isRecord(schema)) { const type = schema.type; if (typeof type === "string" && type !== "object") { add( @@ -324,7 +401,7 @@ function lintSchema( "non-object-root", "error", "", - `Root \`type\` is "${type}". MCP requires a tool's ${kind} to describe an object.`, + `Root \`type\` is "${type}". MCP requires a tool's inputSchema to describe an object.`, 'Wrap the value in an object schema — `{"type": "object", "properties": {…}}`.', ); } diff --git a/test-servers/configs/unportable-schemas-http.json b/test-servers/configs/unportable-schemas-http.json index 52430cef..03696323 100644 --- a/test-servers/configs/unportable-schemas-http.json +++ b/test-servers/configs/unportable-schemas-http.json @@ -3,8 +3,22 @@ "name": "unportable-schemas-showcase", "version": "1.0.0" }, - "tools": [{ "preset": "echo" }, { "preset": "add" }, { "preset": "get_temp" }], + "tools": [ + { "preset": "get_temp" }, + { "preset": "echo" }, + { "preset": "add" }, + { "preset": "get_weather" } + ], "rawToolSchemas": { + "get_temp": { + "outputSchema": { + "type": "object", + "properties": { + "data": true, + "city": { "type": "string" } + } + } + }, "echo": { "inputSchema": { "type": "object", @@ -13,13 +27,6 @@ "show_ids": { "type": ["null", "boolean"] }, "opts": {} } - }, - "outputSchema": { - "type": "object", - "properties": { - "data": true, - "topic": { "type": "string" } - } } }, "add": { diff --git a/test-servers/src/composable-test-server.ts b/test-servers/src/composable-test-server.ts index 9e2fe35e..89d619b8 100644 --- a/test-servers/src/composable-test-server.ts +++ b/test-servers/src/composable-test-server.ts @@ -507,6 +507,15 @@ export interface ServerConfig { * *advertised* schema is replaced; the tool's handler and its real Zod * validation are untouched, so calling it still behaves as its preset does. * + * ⚠️ **Overriding `outputSchema` changes what a client will accept.** A + * conforming client validates a tool result against the advertised output + * schema, so putting one on a preset whose handler returns no + * `structuredContent` makes every call to it fail — the Inspector rejects it + * with "declares an output schema but returned no structured content". Put an + * `outputSchema` override only on a tool that actually returns structured + * content (`get_temp`, `list_items`), and keep the override permissive enough + * that the real payload still validates. + * * A name that isn't registered is ignored. */ rawToolSchemas?: Record<