diff --git a/README.md b/README.md index 670a5bb4..662be445 100644 --- a/README.md +++ b/README.md @@ -304,8 +304,8 @@ mishandled by real MCP clients: 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. +returns structured content) rather than `echo` — see the ⚠️ caveat at the end +of this section for why that placement matters. 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 diff --git a/clients/cli/README.md b/clients/cli/README.md index 29891664..1df35ead 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -165,14 +165,21 @@ mcp-inspector --cli --method tools/list --strict ``` ```text -Error: tool "info" +Error: tool "get_temp" Path: outputSchema.properties.data Issue: Bare `true` used where a schema object is expected. - Suggestion: Spell the "anything" schema out — `{"type": "object", "additionalProperties": true}` … + Suggestion: Declare what the value actually is — e.g. `{"type": "object", "additionalProperties": true}` for a free-form object. That narrows the schema deliberately; `true` accepts any JSON value at all. -1 error, 3 warnings across 2 tools. +1 error, 3 warnings across 3 tools. ``` +Note the shape of that suggestion. Where a replacement genuinely narrows the +schema it says so, rather than implying an equivalent rewrite — and where an +equivalent exists it gives that instead: an array-form `type` becomes `anyOf` +branches (not "drop it from `required`", since absent is not the same as +`null`), and a bare `false` becomes `{"not": {}}` (not "delete the entry", +which would *permit* the property under the default `additionalProperties`). + Severity decides the exit code, not the report: **errors** are constructs a shipping MCP client refuses outright (a bare `true` or `false` where a schema object belongs), **warnings** are ones handled unevenly (an array-form `type`, @@ -302,7 +309,7 @@ prose from stderr: | `3` | Server requires authentication (401/403, `WWW-Authenticate`, OAuth). | | `4` | Server unreachable (DNS, connection refused, timeout, `fetch failed`). | | `5` | Tool error (`tools/call` returned `isError:true`, or the tool was not found). | -| `6` | `--strict` found an error-severity tool-schema portability problem. | +| `6` | `--strict` found an error-severity tool-schema portability problem (`schema_unportable` — the schema is valid JSON Schema, just not portable). | On any non-zero exit the CLI also writes a single JSON line to **stderr** — the `ErrorEnvelope`: diff --git a/clients/cli/__tests__/schema-lint-report.test.ts b/clients/cli/__tests__/schema-lint-report.test.ts index f7e700ef..431375c8 100644 --- a/clients/cli/__tests__/schema-lint-report.test.ts +++ b/clients/cli/__tests__/schema-lint-report.test.ts @@ -128,8 +128,11 @@ describe("emitResult — schema lint wiring (#1005)", () => { }); await expect(promise).rejects.toBeInstanceOf(CliExitCodeError); await promise.catch((e: CliExitCodeError) => { - expect(e.exitCode).toBe(EXIT_CODES.SCHEMA_INVALID); - expect(e.envelope?.code).toBe("schema_invalid"); + expect(e.exitCode).toBe(EXIT_CODES.SCHEMA_UNPORTABLE); + // Not `schema_invalid`: the lint reports schemas that ARE valid JSON + // Schema and are merely unportable, so the code an automated caller + // branches on must not claim otherwise. + expect(e.envelope?.code).toBe("schema_unportable"); }); // The result still goes to stdout; only the report is on stderr. expect(JSON.parse(stdout)).toEqual(DIRTY_LIST); diff --git a/clients/cli/src/error-handler.ts b/clients/cli/src/error-handler.ts index f14e8768..fe8d551a 100644 --- a/clients/cli/src/error-handler.ts +++ b/clients/cli/src/error-handler.ts @@ -9,6 +9,11 @@ * - 4: server unreachable (DNS, connect refused, timeout, fetch failure) * - 5: tool error (`tools/call` returned `isError:true`, or tool not found) * - 6: `--strict` found an error-severity tool-schema portability finding + * + * Note 6 is `SCHEMA_UNPORTABLE`, not "invalid": the whole premise of the lint + * is that these schemas ARE valid JSON Schema and are merely refused by some + * clients. Calling the outcome "invalid" would misreport it to the automated + * callers this map exists for. */ export const EXIT_CODES = { OK: 0, @@ -17,7 +22,7 @@ export const EXIT_CODES = { AUTH_REQUIRED: 3, UNREACHABLE: 4, TOOL_ERROR: 5, - SCHEMA_INVALID: 6, + SCHEMA_UNPORTABLE: 6, } as const; /** Machine-readable error envelope written as one JSON line on stderr. */ diff --git a/clients/cli/src/handlers/emit-result.ts b/clients/cli/src/handlers/emit-result.ts index 9610b675..d6c3f37e 100644 --- a/clients/cli/src/handlers/emit-result.ts +++ b/clients/cli/src/handlers/emit-result.ts @@ -62,10 +62,13 @@ export async function emitResult( if (args.strict && lint) { const { errors } = countFindings(lint); if (errors > 0) { + // "unportable", not "invalid": these schemas are valid JSON Schema and + // are merely refused by some clients, so an envelope code of + // `schema_invalid` would tell an automated caller the wrong thing. throw new CliExitCodeError( - EXIT_CODES.SCHEMA_INVALID, - `${errors} tool schema error${errors === 1 ? "" : "s"} found (--strict).`, - { code: "schema_invalid" }, + EXIT_CODES.SCHEMA_UNPORTABLE, + `${errors} tool schema portability error${errors === 1 ? "" : "s"} found (--strict).`, + { code: "schema_unportable" }, ); } } 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 index 6e03487b..f9b0fb4a 100644 --- a/clients/web/src/test/integration/mcp/raw-tool-schemas.test.ts +++ b/clients/web/src/test/integration/mcp/raw-tool-schemas.test.ts @@ -1,7 +1,9 @@ import { describe, it, expect, afterEach } from "vitest"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; 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 { countFindings, lintTools } from "@inspector/core/json/schemaLint.js"; import { createTestServerHttp, type TestServerHttp, @@ -9,6 +11,8 @@ import { createEchoTool, createGetWeatherTool, createGetTempTool, + loadConfig, + resolveConfig, } from "@modelcontextprotocol/inspector-test-server"; import type { ServerConfig } from "@modelcontextprotocol/inspector-test-server"; @@ -211,6 +215,49 @@ describe("rawToolSchemas override (#1005)", () => { ); }); + it("serves the shape the showcase config declares", async () => { + // Covers the JSON → ConfigFile → ServerConfig plumbing, not just the + // in-process option: a config file is how the manual repro and the + // screenshots in #1005 are produced, and `resolveConfig` forwarding + // `rawToolSchemas` is a line nothing else exercises. Without this, the + // shipped fixture could silently lose its overrides while every other + // test in this file stayed green. + const configPath = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "../../../../../../test-servers/configs/unportable-schemas-http.json", + ); + const resolved = resolveConfig(loadConfig(configPath)); + expect(Object.keys(resolved.rawToolSchemas ?? {}).sort()).toEqual([ + "add", + "echo", + "get_temp", + ]); + + // Let the harness pick the port instead of the config's fixed one, so this + // test can't collide with a manually-running showcase server. + const started = await start({ + tools: resolved.tools, + rawToolSchemas: resolved.rawToolSchemas, + }); + const connected = await connect(started.url); + + const { tools } = await connected.listAllTools(); + // The exact verdict the README and the PR screenshots claim: one error on + // get_temp, warnings on echo and add, get_weather clean. + const results = lintTools(tools); + expect( + results.map((r) => ({ + tool: r.toolName, + rules: r.findings.map((f) => f.rule).sort(), + })), + ).toEqual([ + { tool: "get_temp", rules: ["boolean-schema"] }, + { tool: "echo", rules: ["type-union", "untyped-schema"] }, + { tool: "add", rules: ["remote-ref"] }, + ]); + expect(countFindings(results)).toEqual({ errors: 1, warnings: 3 }); + }); + 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