From 2f9e0901c1fe2aaf9ef6a0664478b84019643984 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 25 Aug 2026 01:18:53 -0400 Subject: [PATCH] feat: flag unportable tool schemas in all three clients MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1005 A tool schema can be perfectly legal JSON Schema and still be rejected by the client the server is meant to run against — Go's `jsonschema` emits a bare `true` for `interface{}`, which the SDK accepts and Claude Code refuses with an opaque "Invalid input". The Inspector passed it through silently. Adds `core/json/schemaLint.ts`, a portability lint over a tool's inputSchema/outputSchema, and surfaces one shared verdict everywhere: the CLI's `--strict` report (exit 6 on an error-severity finding), the TUI's tool list marker + detail pane, and the web Tools sidebar flag + detail section. Deliberately not a JSON Schema validator: a census of 617 public servers found zero that fail the SDK's own parse, so a conformance check would report nothing on real servers. Each rule is instead a construct that is legal JSON Schema and known to be refused or mishandled by a shipping client — a bare boolean in a schema position, an array-form `type`, a remote `$ref`, a schema with no validation keyword, a non-object root. `test-servers/configs/unportable-schemas-http.json` exercises every rule, via a new `rawToolSchemas` override — the Zod-built presets cannot emit any of these shapes, which is the same reason a real server only hits this when its schemas come from another generator. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- AGENTS.md | 15 +- README.md | 47 +- clients/cli/README.md | 46 ++ .../cli/__tests__/schema-lint-report.test.ts | 251 ++++++++++ clients/cli/src/cli.ts | 10 + clients/cli/src/error-handler.ts | 2 + clients/cli/src/handlers/emit-result.ts | 28 +- clients/cli/src/handlers/method-types.ts | 5 + .../cli/src/handlers/schema-lint-report.ts | 62 +++ clients/tui/README.md | 2 +- clients/tui/__tests__/ToolsTab.test.tsx | 82 +++- clients/tui/src/components/ToolsTab.tsx | 78 ++- clients/web/README.md | 21 + .../SchemaFindingsList.stories.tsx | 86 ++++ .../SchemaFindingsList.test.tsx | 61 +++ .../SchemaFindingsList/SchemaFindingsList.tsx | 103 ++++ .../ToolDetailPanel/ToolDetailPanel.test.tsx | 24 + .../ToolDetailPanel/ToolDetailPanel.tsx | 9 +- .../groups/ToolListItem/ToolListItem.test.tsx | 44 ++ .../groups/ToolListItem/ToolListItem.tsx | 41 +- clients/web/src/test/core/schemaLint.test.ts | 463 ++++++++++++++++++ core/json/schemaLint.ts | 414 ++++++++++++++++ .../configs/unportable-schemas-http.json | 39 ++ test-servers/src/composable-test-server.ts | 49 +- test-servers/src/load-config.ts | 9 + test-servers/src/resolve-config.ts | 1 + 26 files changed, 1979 insertions(+), 13 deletions(-) create mode 100644 clients/cli/__tests__/schema-lint-report.test.ts create mode 100644 clients/cli/src/handlers/schema-lint-report.ts create mode 100644 clients/web/src/components/elements/SchemaFindingsList/SchemaFindingsList.stories.tsx create mode 100644 clients/web/src/components/elements/SchemaFindingsList/SchemaFindingsList.test.tsx create mode 100644 clients/web/src/components/elements/SchemaFindingsList/SchemaFindingsList.tsx create mode 100644 clients/web/src/test/core/schemaLint.test.ts create mode 100644 core/json/schemaLint.ts create mode 100644 test-servers/configs/unportable-schemas-http.json diff --git a/AGENTS.md b/AGENTS.md index 01e95395..3177fa34 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -157,7 +157,20 @@ v2/main/ │ │ # Shared by BOTH form builders — web SchemaForm │ │ # and TUI schemaToForm — since each dispatches on │ │ # a single `type` string and would otherwise miss -│ │ # a nullable field entirely — #1928/#2015) +│ │ # a nullable field entirely — #1928/#2015; +│ │ # schemaLint.ts: tool-schema PORTABILITY lint — +│ │ # constructs that are legal JSON Schema and are +│ │ # refused or mishandled by real MCP clients (a bare +│ │ # `true` in `properties`, an array-form `type`, a +│ │ # remote `$ref`, a schema carrying no validation +│ │ # keyword, a non-object root). Deliberately NOT a +│ │ # validator: a census of 617 public servers found 0 +│ │ # that fail the SDK's own parse, so a conformance +│ │ # check reports nothing on real servers. ONE verdict +│ │ # for all three clients — the CLI's `--strict` report +│ │ # (exit 6), the TUI's tool detail pane, the web Tools +│ │ # tab — so they cannot disagree about what is +│ │ # portable; the report TEXT is shared too — #1005) │ ├── logging/ # Silent pino logger singleton │ ├── mcp/ # InspectorClient runtime + state stores │ │ # (uriTemplate.ts: RFC 6570 parse/classify/expand. diff --git a/README.md b/README.md index c8c15bf4..9eb14804 100644 --- a/README.md +++ b/README.md @@ -34,8 +34,9 @@ inspector/ │ │ # plus per-server secret storage — the keychain/file/memory SecretStore │ │ # implementations, the selection policy, and the descriptor the banner and UI report │ ├── client/ # Install-level client config (`client.json`): browser-safe parse/validate + Node load/save, remote backend, secrets -│ ├── json/ # JSON + parameter/argument conversion utilities, and the nullable-union -│ │ # schema collapse shared by the web and TUI form builders +│ ├── json/ # JSON + parameter/argument conversion utilities, the nullable-union +│ │ # schema collapse shared by the web and TUI form builders, and the +│ │ # tool-schema portability lint all three clients report from │ ├── logging/ # Silent pino logger singleton │ ├── mcp/ # InspectorClient runtime, state stores, transports, config import, │ │ # and the RFC 6570 URI-template helpers the web form and TUI expand through @@ -152,6 +153,7 @@ Each config below is a ready-made server for exercising one feature by hand. Loa | `structured-output-http.json` | Tools tab: a result's `structuredContent` section | [#1908](https://github.com/modelcontextprotocol/inspector/issues/1908) | | `duplicate-tool-names-http.json` | A `tools/list` that repeats a tool name | [#1957](https://github.com/modelcontextprotocol/inspector/issues/1957) | | `nullable-fields-http.json` | Tools tab: nullable (`anyOf` + `null`) arguments | [#1928](https://github.com/modelcontextprotocol/inspector/issues/1928) | +| `unportable-schemas-http.json` **(legacy era)** | Tool schemas a real client rejects, flagged in all three clients | [#1005](https://github.com/modelcontextprotocol/inspector/issues/1005) | | `rfc6570-templates-http.json` | Resources tab: RFC 6570 resource-template expansion | [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) | | `advertised-extensions-http.json` | Tool registration gated on advertised extensions | [#1739](https://github.com/modelcontextprotocol/inspector/issues/1739) | | `oauth-custom-resource-metadata-http.json` **(legacy era)** | OAuth discovery driven by the challenge's `resource_metadata` | [#2071](https://github.com/modelcontextprotocol/inspector/issues/2071) | @@ -286,6 +288,47 @@ Open the Tools tab and select `record_shipment`: `direction` must render as a ** The **TUI** had the same gap and is worth checking against the same server (`--tui`, then test `record_shipment`): `direction` is a select, `quantity` an integer field, `express` a boolean. Both clients now share one collapse step — `normalizeNullableUnion` in [`core/json/nullableUnion.ts`](./core/json/nullableUnion.ts) — precisely so they cannot drift on which schemas they can render. +#### Unportable tool schemas + +`unportable-schemas-http.json` serves three tools, two 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. + +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 +clients report the same verdict from +[`core/json/schemaLint.ts`](./core/json/schemaLint.ts), each with the room it +has: + +```bash +mcp-inspector --cli http://127.0.0.1:6603/mcp --method tools/list --strict # exits 6 +``` + +- **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**. +- **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. + +This is deliberately **not** a JSON Schema validator. A census of 617 public +servers (14,804 tool schemas) reported on that issue found **0** that fail the +SDK's own `ListToolsResultSchema.safeParse`, so a conformance check would +report nothing on essentially every real server. What bites is the narrower +subset each consumer accepts, and each rule here is a construct known to be +refused or degraded by a shipping client. The schemas are supplied through the +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. + #### 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/README.md b/clients/cli/README.md index e0572746..8e7d8e7b 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -120,6 +120,7 @@ Options that specify the MCP server (catalog/config file, ad-hoc command/URL, en | `--tool-metadata ` | Tool-specific `_meta` entries for `tools/call`. Same JSON-parsed value handling as `--metadata`. | | `--connect-timeout ` | Connection timeout in ms. Defaults to `15000` for ad-hoc `--server-url`/target runs (so a black-holed host fails fast) and to the file-level timeout for `--catalog`/`--config` runs. `0` disables the timeout. | | `--app-info` | Probe a tool's MCP App UI metadata without invoking it. With `--method tools/call --tool-name `: prints one JSON line (`hasApp`, `resourceUri`, `csp`, `permissions`, `domain`, …) and exits `0` if the tool has an app or `2` (`no_app`) if not. With `--method tools/list`: emits NDJSON — one app-info line per tool over a single connection. | +| `--strict` | With `--method tools/list`: report tool-schema portability problems in full (path, issue, suggested fix) on stderr, and exit `6` if any is error-severity. Without it, a one-line count is printed instead. See [Schema portability](#schema-portability---strict). | | `--format ` | Output format. `text` (default) pretty-prints the result. `json` emits a single JSON object on stdout (`{ "result": … }`, plus `{ "appInfo": … }` as a sibling key for App tools) with no banners, so the whole output pipes cleanly into `jq`. | | `--relogin` | Delete stored OAuth for this server URL from the shared store before connect; interactive login still only runs if the server requires auth. Requires an HTTP/SSE URL (rejected for stdio). Conflicts with `--stored-auth-only` / `--use-stored-auth` / `--wait-for-auth` / catalog short-circuits. | | `--stored-auth-only` | **CI / non-interactive safe:** never start interactive OAuth / step-up (and never auto-open a browser); use the shared store if present, otherwise fail immediately with `auth_required`. Prefer this over a bare pipe/CI run that would otherwise attempt interactive login. | @@ -152,6 +153,50 @@ mcp-inspector --cli --method tools/call --tool-name my_app_tool --forma A `tools/call` that returns `isError:true` still prints its payload but exits `5` (`tool_is_error`) so `&&` chains don't proceed on a failed call. +#### Schema portability (`--strict`) + +A tool schema can be perfectly legal JSON Schema and still be refused by the +client the server is meant to run against. `--strict` (with `--method +tools/list`) names those constructs — path, what is wrong, and a concrete fix — +on **stderr**, and exits `6` if any is error-severity: + +```bash +mcp-inspector --cli --method tools/list --strict +``` + +```text +Error: tool "info" + Path: outputSchema.properties.data + Issue: Bare `true` used where a schema object is expected. + Suggestion: Spell the "anything" schema out — `{"type": "object", "additionalProperties": true}` … + +1 error, 3 warnings across 2 tools. +``` + +Severity decides the exit code, not the report: **errors** are constructs a +shipping MCP client refuses outright (a bare `true` in a `properties` map, a +non-object schema root), **warnings** are ones handled unevenly (an array-form +`type`, a remote `$ref`, a schema carrying no validation keyword at all). Only +errors fail the run — a `--strict` that failed on warnings would be unusable as +a CI gate against servers that are in fact fine. + +The report goes to stderr, so the result on stdout stays parseable. Under +`--format json` the findings are folded into the same envelope instead +(`{"result":…,"schemaFindings":[…]}`), so a caller reads one document rather +than correlating two. + +**Without `--strict` nothing changes except one line.** A `tools/list` whose +schemas have findings prints a single stderr summary — `Schema portability: 1 +error, 3 warnings across 2 tools. Re-run with --strict for details.` — and +still exits `0`. A clean list prints nothing at all. + +This is deliberately not a JSON Schema validator. A census of 617 public +servers found **zero** that fail the SDK's own parser, so a conformance check +would report nothing on essentially every real server; what bites is the +narrower subset each consumer accepts, which is what these rules encode. The +same verdict drives the TUI's tool detail pane and the web Tools tab — all +three read `core/json/schemaLint`. + ### CLI-specific (OAuth for HTTP servers) The CLI runs the same loopback callback server as the TUI (`http://127.0.0.1:6276/oauth/callback` by default). @@ -250,6 +295,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. | 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 new file mode 100644 index 00000000..0ab49217 --- /dev/null +++ b/clients/cli/__tests__/schema-lint-report.test.ts @@ -0,0 +1,251 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { emitResult, runCli } from "../src/cli.js"; +import { CliExitCodeError, EXIT_CODES } from "../src/error-handler.js"; +import { + lintListResult, + toolsFromResult, + writeSchemaLintReport, +} from "../src/handlers/schema-lint-report.js"; + +/** A `tools/list` result with one bare-`true` property — the issue's case. */ +const DIRTY_LIST = { + tools: [ + { + name: "info", + inputSchema: { type: "object", properties: {} }, + outputSchema: { type: "object", properties: { data: true } }, + }, + ], +}; + +/** Same shape, but every schema portable. */ +const CLEAN_LIST = { + tools: [{ name: "ok", inputSchema: { type: "object", properties: {} } }], +}; + +describe("toolsFromResult", () => { + it("returns the tools array", () => { + expect(toolsFromResult(DIRTY_LIST).map((t) => t.name)).toEqual(["info"]); + }); + + it.each([ + ["a missing tools key", {}], + ["a non-array tools value", { tools: "nope" }], + ])("returns nothing for %s", (_label, result) => { + expect(toolsFromResult(result)).toEqual([]); + }); + + it("skips entries that are not tool-shaped", () => { + const result = { + tools: [null, 3, {}, { name: 7 }, { name: "real", inputSchema: {} }], + }; + expect(toolsFromResult(result).map((t) => t.name)).toEqual(["real"]); + }); +}); + +describe("lintListResult", () => { + it("reports the offending tool", () => { + const results = lintListResult(DIRTY_LIST); + expect(results.map((r) => r.toolName)).toEqual(["info"]); + }); + + it("reports nothing for a clean list", () => { + expect(lintListResult(CLEAN_LIST)).toEqual([]); + }); +}); + +describe("writeSchemaLintReport", () => { + let stderr: string; + let originalWrite: typeof process.stderr.write; + + beforeEach(() => { + stderr = ""; + originalWrite = process.stderr.write; + process.stderr.write = ((chunk: unknown): boolean => { + stderr += typeof chunk === "string" ? chunk : String(chunk); + return true; + }) as typeof process.stderr.write; + }); + + afterEach(() => { + process.stderr.write = originalWrite; + }); + + it("writes nothing at all when there are no findings", () => { + writeSchemaLintReport([], true); + expect(stderr).toBe(""); + }); + + it("writes a one-line hint without --strict", () => { + writeSchemaLintReport(lintListResult(DIRTY_LIST), false); + expect(stderr.trimEnd().split("\n")).toHaveLength(1); + expect(stderr).toContain("Re-run with --strict"); + }); + + it("writes the full report under --strict", () => { + writeSchemaLintReport(lintListResult(DIRTY_LIST), true); + expect(stderr).toContain('Error: tool "info"'); + expect(stderr).toContain("Path: outputSchema.properties.data"); + expect(stderr).toContain("Suggestion: "); + expect(stderr).not.toContain("Re-run with --strict"); + }); +}); + +describe("emitResult — schema lint wiring (#1005)", () => { + let stdout: string; + let stderr: string; + let originalOut: typeof process.stdout.write; + let originalErr: typeof process.stderr.write; + + beforeEach(() => { + stdout = ""; + stderr = ""; + originalOut = process.stdout.write; + originalErr = process.stderr.write; + process.stdout.write = ((chunk: unknown, ...rest: unknown[]): boolean => { + stdout += typeof chunk === "string" ? chunk : String(chunk); + ( + rest.find((r) => typeof r === "function") as (() => void) | undefined + )?.(); + return true; + }) as typeof process.stdout.write; + process.stderr.write = ((chunk: unknown): boolean => { + stderr += typeof chunk === "string" ? chunk : String(chunk); + return true; + }) as typeof process.stderr.write; + }); + + afterEach(() => { + process.stdout.write = originalOut; + process.stderr.write = originalErr; + }); + + it("exits 6 under --strict when a finding is error-severity", async () => { + const promise = emitResult(DIRTY_LIST, undefined, { + method: "tools/list", + strict: true, + format: "text", + }); + 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"); + }); + // The result still goes to stdout; only the report is on stderr. + expect(JSON.parse(stdout)).toEqual(DIRTY_LIST); + expect(stderr).toContain("Path: outputSchema.properties.data"); + }); + + it("succeeds under --strict when only warnings were found", async () => { + const warnOnly = { + tools: [ + { + name: "w", + inputSchema: { + type: "object", + properties: { a: { type: ["null", "boolean"] } }, + }, + }, + ], + }; + await expect( + emitResult(warnOnly, undefined, { + method: "tools/list", + strict: true, + format: "text", + }), + ).resolves.toBeUndefined(); + expect(stderr).toContain('Warning: tool "w"'); + }); + + it("prints only the hint, and does not fail, without --strict", async () => { + await expect( + emitResult(DIRTY_LIST, undefined, { + method: "tools/list", + format: "text", + }), + ).resolves.toBeUndefined(); + expect(stderr).toContain("Re-run with --strict"); + expect(stderr).not.toContain("Suggestion:"); + }); + + it("folds findings into the --format json envelope under --strict", async () => { + await emitResult(DIRTY_LIST, undefined, { + method: "tools/list", + strict: true, + format: "json", + }).catch(() => { + // exit 6 is expected here; the envelope is what this test asserts. + }); + const envelope = JSON.parse(stdout) as { + result: unknown; + schemaFindings?: { toolName: string }[]; + }; + expect(envelope.result).toEqual(DIRTY_LIST); + expect(envelope.schemaFindings?.[0]?.toolName).toBe("info"); + }); + + it("leaves the json envelope alone when nothing was found", async () => { + await emitResult(CLEAN_LIST, undefined, { + method: "tools/list", + strict: true, + format: "json", + }); + expect(JSON.parse(stdout)).toEqual({ result: CLEAN_LIST }); + expect(stderr).toBe(""); + }); + + it("does not lint a method other than tools/list", async () => { + // The same payload under `tools/call` must produce no report at all — + // `--strict` is a tools/list flag and the CLI rejects it elsewhere. + await emitResult(DIRTY_LIST, undefined, { + method: "tools/call", + strict: true, + format: "text", + }); + expect(stderr).toBe(""); + }); + + it("does not lint an --app-info run", async () => { + await emitResult( + DIRTY_LIST, + { hasApp: true, toolName: "info" }, + { method: "tools/list", strict: true, appInfo: true, format: "text" }, + ); + expect(stderr).toBe(""); + }); + + it("reports isError before the schema verdict", async () => { + // Both would throw; the tool-level failure is the more specific one and + // must win, so a caller branching on exit 5 is not shadowed by exit 6. + const promise = emitResult({ ...DIRTY_LIST, isError: true }, undefined, { + method: "tools/list", + strict: true, + toolName: "info", + format: "text", + }); + await promise.catch((e: CliExitCodeError) => { + expect(e.exitCode).toBe(EXIT_CODES.TOOL_ERROR); + }); + await expect(promise).rejects.toBeInstanceOf(CliExitCodeError); + }); +}); + +describe("--strict argument validation", () => { + it("is rejected with a method other than tools/list", async () => { + await expect( + runCli([ + "node", + "cli", + "--cli", + "--method", + "tools/call", + "--tool-name", + "x", + "--strict", + "--server-url", + "http://127.0.0.1:1/mcp", + ]), + ).rejects.toThrow("--strict requires --method tools/list."); + }); +}); diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index c1a3673d..ba2de59a 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -684,6 +684,10 @@ async function parseArgs(argv?: string[]): Promise { "--app-info", "Probe the tool's MCP App UI metadata (resourceUri, csp, permissions, domain) and emit it as one JSON line; exit 2 when the tool has no app. Use with --method tools/call --tool-name (the tool itself is not invoked) or --method tools/list (one NDJSON line per tool).", ) + .option( + "--strict", + "Report tool-schema portability problems in full (path, issue, suggested fix) on stderr, and exit 6 if any is error-severity. Use with --method tools/list. Without it, a one-line count is printed instead.", + ) .option( "--connect-timeout ", `Connection timeout in ms (default ${DEFAULT_CONNECT_TIMEOUT_MS} for ad-hoc --server-url / target invocations; 0 = no timeout).`, @@ -784,6 +788,7 @@ async function parseArgs(argv?: string[]): Promise { serverUrl?: string; header?: Record; appInfo?: boolean; + strict?: boolean; connectTimeout?: number; format?: OutputFormat; toolArgsJson?: string; @@ -1004,6 +1009,10 @@ async function parseArgs(argv?: string[]): Promise { ); } + if (options.strict && options.method !== "tools/list") { + throw new Error("--strict requires --method tools/list."); + } + // --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; @@ -1047,6 +1056,7 @@ async function parseArgs(argv?: string[]): Promise { metadata: options.metadata, toolMeta: options.toolMetadata, appInfo: options.appInfo === true, + strict: options.strict === true, format: options.format, }; diff --git a/clients/cli/src/error-handler.ts b/clients/cli/src/error-handler.ts index 84b72d65..f14e8768 100644 --- a/clients/cli/src/error-handler.ts +++ b/clients/cli/src/error-handler.ts @@ -8,6 +8,7 @@ * - 3: server requires authentication (401 / WWW-Authenticate / OAuth) * - 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 */ export const EXIT_CODES = { OK: 0, @@ -16,6 +17,7 @@ export const EXIT_CODES = { AUTH_REQUIRED: 3, UNREACHABLE: 4, TOOL_ERROR: 5, + SCHEMA_INVALID: 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 b4d370de..9610b675 100644 --- a/clients/cli/src/handlers/emit-result.ts +++ b/clients/cli/src/handlers/emit-result.ts @@ -1,10 +1,13 @@ import { awaitableLog } from "../utils/awaitable-log.js"; import { CliExitCodeError, EXIT_CODES } from "../error-handler.js"; +import { lintListResult, writeSchemaLintReport } from "./schema-lint-report.js"; +import { countFindings } from "@inspector/core/json/schemaLint.js"; import type { CliAppInfo, McpResponse, MethodArgs } from "./method-types.js"; /** * Write the method result (and any app-info) to stdout, honouring `--format` - * and `--app-info`, then map `isError`/no-app outcomes onto the exit-code map. + * and `--app-info`, then map `isError`/no-app/schema outcomes onto the + * exit-code map. */ export async function emitResult( result: McpResponse, @@ -28,14 +31,23 @@ export async function emitResult( return; } + // Lint before writing, because `--format json --strict` folds the findings + // into the same envelope as the result rather than emitting a second + // document a caller would have to correlate. + const lint = + args.method === "tools/list" ? lintListResult(result) : undefined; + if (json) { const envelope: Record = { result }; if (appInfo?.hasApp) envelope.appInfo = appInfo; + if (args.strict && lint && lint.length > 0) envelope.schemaFindings = lint; await awaitableLog(JSON.stringify(envelope) + "\n"); } else { await awaitableLog(JSON.stringify(result, null, 2) + "\n"); } + if (lint) writeSchemaLintReport(lint, args.strict === true); + if ((result as { isError?: unknown }).isError === true) { throw new CliExitCodeError( EXIT_CODES.TOOL_ERROR, @@ -43,4 +55,18 @@ export async function emitResult( { code: "tool_is_error" }, ); } + + // Only `--strict` fails the run. Warnings never do: they mark constructs + // that are handled unevenly rather than refused, so failing on them would + // make `--strict` unusable as a CI gate on servers that are in fact fine. + if (args.strict && lint) { + const { errors } = countFindings(lint); + if (errors > 0) { + throw new CliExitCodeError( + EXIT_CODES.SCHEMA_INVALID, + `${errors} tool schema error${errors === 1 ? "" : "s"} found (--strict).`, + { code: "schema_invalid" }, + ); + } + } } diff --git a/clients/cli/src/handlers/method-types.ts b/clients/cli/src/handlers/method-types.ts index 424b941b..51d8657c 100644 --- a/clients/cli/src/handlers/method-types.ts +++ b/clients/cli/src/handlers/method-types.ts @@ -24,6 +24,11 @@ export type MethodArgs = { toolMeta?: RequestMetadata; metadata?: RequestMetadata; appInfo?: boolean; + /** + * `--strict`: report tool-schema portability findings in full and exit + * non-zero when any is error-severity (#1005). `tools/list` only. + */ + strict?: boolean; format?: OutputFormat; /** Task id for tasks/get, tasks/cancel, tasks/result. */ taskId?: string; diff --git a/clients/cli/src/handlers/schema-lint-report.ts b/clients/cli/src/handlers/schema-lint-report.ts new file mode 100644 index 00000000..7b685ed1 --- /dev/null +++ b/clients/cli/src/handlers/schema-lint-report.ts @@ -0,0 +1,62 @@ +import type { Tool } from "@modelcontextprotocol/client"; +import { + formatSchemaLintReport, + lintTools, + summarizeFindings, + type ToolSchemaFindings, +} from "@inspector/core/json/schemaLint.js"; +import type { McpResponse } from "./method-types.js"; + +/** + * Read the `tools` array out of a `tools/list` result. The result is typed as + * an opaque `McpResponse` at this layer, and a server can return anything, so + * each entry is narrowed to "an object with a string `name`" before it is + * treated as a {@link Tool}. Everything else is skipped rather than crashing + * the report — an unlintable entry is not the CLI's problem to surface here. + */ +export function toolsFromResult(result: McpResponse): Tool[] { + const tools = (result as { tools?: unknown }).tools; + if (!Array.isArray(tools)) return []; + return tools.filter( + (t): t is Tool => + typeof t === "object" && + t !== null && + typeof (t as { name?: unknown }).name === "string", + ); +} + +/** + * Lint the tools in a `tools/list` result (#1005). + * + * Always run, `--strict` or not — it is a pure in-memory walk over a list the + * CLI already holds, and the non-strict hint below needs the count. What + * `--strict` changes is what happens with the result, in + * {@link writeSchemaLintReport}. + */ +export function lintListResult(result: McpResponse): ToolSchemaFindings[] { + return lintTools(toolsFromResult(result)); +} + +/** + * Write the schema-lint outcome to **stderr**, so it never contaminates the + * result on stdout that a `--format json` consumer is parsing. + * + * Under `--strict` this is the full report from the issue — path, issue, + * suggestion, one block per finding. Without it, a single line naming the + * count and how to see the detail: a server author who has not asked for the + * lint should still learn it found something, but a multi-page report nobody + * requested would be worse than silence. + */ +export function writeSchemaLintReport( + results: readonly ToolSchemaFindings[], + strict: boolean, +): void { + if (results.length === 0) return; + if (!strict) { + process.stderr.write( + `Schema portability: ${summarizeFindings(results)}. Re-run with --strict for details.\n`, + ); + return; + } + process.stderr.write(`${formatSchemaLintReport(results)}\n`); +} diff --git a/clients/tui/README.md b/clients/tui/README.md index 1ae2fb51..93930003 100644 --- a/clients/tui/README.md +++ b/clients/tui/README.md @@ -75,7 +75,7 @@ The TUI provides terminal-native tabs and panes for interacting with your MCP se - **Resources**: Browse and read resources exposed by the server. - **Prompts**: List and test prompts. -- **Tools**: View available tools and execute them with form-like inputs. +- **Tools**: View available tools and execute them with form-like inputs. A tool whose advertised schema carries a portability problem is flagged in the list — red `!` for a construct a shipping MCP client refuses, yellow `?` for one handled unevenly — and the detail pane lists each finding under **Schema Portability** with the path, the problem, and a concrete fix. The verdict comes from [`core/json/schemaLint.ts`](../../core/json/schemaLint.ts), shared with the web Tools tab and the CLI's `--strict` report, so the three cannot disagree ([#1005](https://github.com/modelcontextprotocol/inspector/issues/1005)). - **Protocol**: View JSON-RPC request/response/notification history (matches the web Protocol monitor). - **Network**: View HTTP fetch traffic for SSE / Streamable HTTP servers (matches the web Network monitor). - **Console**: View stdio stderr from the connected server process (matches the web Console monitor). diff --git a/clients/tui/__tests__/ToolsTab.test.tsx b/clients/tui/__tests__/ToolsTab.test.tsx index 0500a695..0c1913d6 100644 --- a/clients/tui/__tests__/ToolsTab.test.tsx +++ b/clients/tui/__tests__/ToolsTab.test.tsx @@ -8,7 +8,7 @@ import type { Tool } from "@modelcontextprotocol/client"; // renders children directly and stubs scrollBy/scrollTo/getViewportHeight. vi.mock("ink-scroll-view", () => import("./helpers/inkScrollViewMock.js")); -import { ToolsTab } from "../src/components/ToolsTab.js"; +import { ToolsTab, schemaMarker } from "../src/components/ToolsTab.js"; // Ink processes stdin keypresses asynchronously — await this after stdin.write // and after rerender() before asserting. @@ -205,4 +205,84 @@ describe("ToolsTab", () => { await tick(); expect(onViewDetails).toHaveBeenCalledWith(tools[0]); }); + + describe("schema portability (#1005)", () => { + it("marks nothing when every tool's schema is portable", () => { + const { lastFrame } = render( + , + ); + const frame = lastFrame() ?? ""; + expect(frame).not.toContain("Schema Portability"); + }); + + it("flags an offending tool in the list and details it in the pane", () => { + const dirty = makeTool({ + name: "info", + description: undefined, + outputSchema: { type: "object", properties: { data: true } }, + } as Partial); + const { lastFrame } = render( + , + ); + const frame = lastFrame() ?? ""; + // List row carries the error glyph… + expect(frame).toContain("info !"); + // …and the detail pane names the path, issue and fix. + expect(frame).toContain("Schema Portability (1)"); + expect(frame).toContain("outputSchema.properties.data"); + expect(frame).toContain("Fix:"); + }); + + it("uses the warning glyph when nothing is error-severity", () => { + const warned = makeTool({ + name: "warned", + description: undefined, + inputSchema: { + type: "object", + properties: { a: { type: ["null", "boolean"] } }, + }, + } as Partial); + const { lastFrame } = render( + , + ); + const frame = lastFrame() ?? ""; + expect(frame).toContain("warned ?"); + expect(frame).toContain("Schema Portability (1)"); + }); + }); +}); + +describe("schemaMarker", () => { + it("returns no glyph for undefined or empty findings", () => { + expect(schemaMarker(undefined)).toBeUndefined(); + expect(schemaMarker([])).toBeUndefined(); + }); + + it("prefers the error glyph when both severities are present", () => { + expect( + schemaMarker([ + { + rule: "type-union", + severity: "warning", + schema: "inputSchema", + path: "", + issue: "i", + suggestion: "s", + }, + { + rule: "boolean-schema", + severity: "error", + schema: "inputSchema", + path: "", + issue: "i", + suggestion: "s", + }, + ]), + ).toEqual({ glyph: "!", color: "red" }); + }); }); diff --git a/clients/tui/src/components/ToolsTab.tsx b/clients/tui/src/components/ToolsTab.tsx index 92eac0f5..0ac47a5b 100644 --- a/clients/tui/src/components/ToolsTab.tsx +++ b/clients/tui/src/components/ToolsTab.tsx @@ -1,9 +1,43 @@ -import React, { useState, useEffect, useRef } from "react"; +import React, { useState, useEffect, useMemo, useRef } from "react"; import { Box, Text, useInput, type Key } from "ink"; import { ScrollView, type ScrollViewRef } from "ink-scroll-view"; import type { Tool } from "@modelcontextprotocol/client"; +import { + describeSchemaPath, + lintToolSchemas, + type SchemaFinding, +} from "@inspector/core/json/schemaLint.js"; import { useSelectableList } from "../hooks/useSelectableList.js"; +/** + * How each severity renders in the terminal (#1005). One table, read by both + * the list row's flag and the detail block's heading, so the two can never + * disagree about what a severity looks like. + */ +const SEVERITY_MARKER = { + error: { glyph: "!", color: "red" }, + warning: { glyph: "?", color: "yellow" }, +} as const satisfies Record< + SchemaFinding["severity"], + { glyph: string; color: string } +>; + +/** + * The one-glyph flag a tool row carries when its schemas have portability + * findings: `!` in red when something is refused outright by a real client, + * `?` in yellow when it is merely handled unevenly. `undefined` — no glyph at + * all — for a clean tool, which is the overwhelming majority, so the list + * stays quiet unless there is something to say. + */ +export function schemaMarker( + findings: readonly SchemaFinding[] | undefined, +): { glyph: string; color: string } | undefined { + if (!findings || findings.length === 0) return undefined; + return findings.some((f) => f.severity === "error") + ? SEVERITY_MARKER.error + : SEVERITY_MARKER.warning; +} + interface ToolsTabProps { tools: Tool[]; isConnected: boolean; @@ -33,6 +67,13 @@ export function ToolsTab({ { resetWhen: tools }, ); const [error] = useState(null); + // Tool-schema portability findings, one entry per tool, same order (#1005). + // Pure walk over data already in memory, so it is recomputed only when the + // list itself changes rather than on every keypress. + const findingsByTool = useMemo( + () => tools.map((tool) => lintToolSchemas(tool)), + [tools], + ); const scrollViewRef = useRef(null); const listWidth = Math.floor(width * 0.4); const detailWidth = width - listWidth; @@ -90,6 +131,7 @@ export function ToolsTab({ }, [selectedIndex]); const selectedTool = tools[selectedIndex] || null; + const selectedFindings = findingsByTool[selectedIndex] ?? []; return ( @@ -133,11 +175,15 @@ export function ToolsTab({ .map((tool, i) => { const index = firstVisible + i; const isSelected = index === selectedIndex; + const marker = schemaMarker(findingsByTool[index]); return ( {isSelected ? "▶ " : " "} {tool.name || `Tool ${index + 1}`} + {marker && ( + {marker.glyph} + )} ); @@ -200,6 +246,36 @@ export function ToolsTab({ )} + {/* Schema portability findings (#1005) */} + {selectedFindings.length > 0 && ( + <> + + + Schema Portability ({selectedFindings.length}): + + + {selectedFindings.map((finding, idx) => { + const marker = SEVERITY_MARKER[finding.severity]; + return ( + + + {marker.glyph}{" "} + {describeSchemaPath(finding.schema, finding.path)} + + {finding.issue} + Fix: {finding.suggestion} + + ); + })} + + )} + {/* Input Schema */} {selectedTool.inputSchema && ( <> diff --git a/clients/web/README.md b/clients/web/README.md index 885152ff..f00ee3fe 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -56,6 +56,27 @@ Components live under `src/components/` in four layers, smallest to largest: Every screen and element has a `*.stories.tsx` (see [Storybook](#storybook)). Styling follows the Mantine-first rules in [`AGENTS.md`](../../AGENTS.md) — theme variants and component props over CSS, `--inspector-*` tokens over raw colors. +## Tool-schema portability (`#1005`) + +The Tools tab flags tools whose advertised `inputSchema` / `outputSchema` +carries a construct that is legal JSON Schema and is refused or mishandled by +real MCP clients — the reported case being Go's `jsonschema` package emitting a +bare `true` for an `interface{}` field, which Claude Code rejects with an +opaque `"Invalid input"`. + +Two surfaces, one verdict: the sidebar row (`ToolListItem`) carries a +hover-labelled icon — red when something is refused outright, yellow when it is +merely handled unevenly — and selecting the tool renders a **Schema +portability** section (`SchemaFindingsList`) above the argument form, one block +per finding with its path, the problem, and a concrete fix. + +Both read [`core/json/schemaLint.ts`](../../core/json/schemaLint.ts), which is +also what backs the TUI's detail pane and the CLI's `--strict` report — so the +three clients cannot disagree about whether a schema is portable. That module's +header explains why it is a portability lint rather than a JSON Schema +validator. `test-servers/configs/unportable-schemas-http.json` is a server that +exercises every rule. + ## Non-component code: `src/lib` vs `src/utils` Two grab-bag directories, split by one rule: **`utils` = functions that compute; `lib` = things that instantiate, adapt, or touch the environment.** If it does I/O or wraps a subsystem, it's `lib`; if it's a pure transform, it's `utils`. diff --git a/clients/web/src/components/elements/SchemaFindingsList/SchemaFindingsList.stories.tsx b/clients/web/src/components/elements/SchemaFindingsList/SchemaFindingsList.stories.tsx new file mode 100644 index 00000000..185830a4 --- /dev/null +++ b/clients/web/src/components/elements/SchemaFindingsList/SchemaFindingsList.stories.tsx @@ -0,0 +1,86 @@ +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { expect, within } from "storybook/test"; +import type { Tool } from "@modelcontextprotocol/client"; +import { lintToolSchemas } from "@inspector/core/json/schemaLint.js"; +import { SchemaFindingsList } from "./SchemaFindingsList"; + +/** Lint a schema fixture the way the real panels do, so the stories show the + * module's own wording rather than hand-written findings that could drift. */ +function findingsFor(tool: Partial) { + return lintToolSchemas({ name: "info", ...tool } as Tool); +} + +const meta: Meta = { + title: "Elements/SchemaFindingsList", + component: SchemaFindingsList, +}; + +export default meta; +type Story = StoryObj; + +/** The reported case: Go's `jsonschema` emits `true` for an `interface{}`. */ +export const BareTrueProperty: Story = { + args: { + findings: findingsFor({ + outputSchema: { + type: "object", + properties: { data: true, topic: { type: "string" } }, + }, + } as Partial), + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await expect(canvas.getByText("Schema portability (1)")).toBeVisible(); + await expect(canvas.getByText("error")).toBeVisible(); + await expect( + canvas.getByText("outputSchema.properties.data"), + ).toBeVisible(); + }, +}; + +/** Warning-only: an array-form `type`, legal but read unevenly. */ +export const TypeUnionWarning: Story = { + args: { + findings: findingsFor({ + inputSchema: { + type: "object", + properties: { show_ids: { type: ["null", "boolean"] } }, + }, + } as Partial), + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await expect(canvas.getByText("warning")).toBeVisible(); + await expect(canvas.queryByText("error")).toBeNull(); + }, +}; + +/** Both severities on one tool, which is what a real problem server looks like. */ +export const Mixed: Story = { + args: { + findings: findingsFor({ + inputSchema: { + type: "object", + properties: { + show_ids: { type: ["null", "boolean"] }, + extra: {}, + ref: { $ref: "https://example.com/s.json" }, + }, + }, + outputSchema: { type: "object", properties: { data: true } }, + } as Partial), + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await expect(canvas.getByText("Schema portability (4)")).toBeVisible(); + }, +}; + +/** The common case — nothing to say, so the section renders nothing at all. */ +export const Clean: Story = { + args: { findings: [] }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await expect(canvas.queryByTestId("schema-findings")).toBeNull(); + }, +}; diff --git a/clients/web/src/components/elements/SchemaFindingsList/SchemaFindingsList.test.tsx b/clients/web/src/components/elements/SchemaFindingsList/SchemaFindingsList.test.tsx new file mode 100644 index 00000000..4386eaf4 --- /dev/null +++ b/clients/web/src/components/elements/SchemaFindingsList/SchemaFindingsList.test.tsx @@ -0,0 +1,61 @@ +import { describe, it, expect } from "vitest"; +import { screen } from "@testing-library/react"; +import type { Tool } from "@modelcontextprotocol/client"; +import { lintToolSchemas } from "@inspector/core/json/schemaLint.js"; +import { renderWithMantine } from "../../../test/renderWithMantine"; +import { SchemaFindingsList } from "./SchemaFindingsList"; + +function findingsFor(tool: Partial) { + return lintToolSchemas({ name: "info", ...tool } as Tool); +} + +describe("SchemaFindingsList", () => { + it("renders nothing when there are no findings", () => { + renderWithMantine(); + expect(screen.queryByTestId("schema-findings")).not.toBeInTheDocument(); + }); + + it("shows the path, severity, issue and fix for an error finding", () => { + renderWithMantine( + )} + />, + ); + expect(screen.getByText("Schema portability (1)")).toBeInTheDocument(); + expect(screen.getByText("error")).toBeInTheDocument(); + expect( + screen.getByText("outputSchema.properties.data"), + ).toBeInTheDocument(); + expect(screen.getByText(/Bare `true`/)).toBeInTheDocument(); + expect(screen.getByText(/^Fix: /)).toBeInTheDocument(); + }); + + it("labels a warning finding as such", () => { + renderWithMantine( + )} + />, + ); + expect(screen.getByText("warning")).toBeInTheDocument(); + expect(screen.queryByText("error")).not.toBeInTheDocument(); + }); + + it("renders one block per finding", () => { + renderWithMantine( + )} + />, + ); + expect(screen.getByText("Schema portability (2)")).toBeInTheDocument(); + expect(screen.getAllByText("error")).toHaveLength(2); + }); +}); diff --git a/clients/web/src/components/elements/SchemaFindingsList/SchemaFindingsList.tsx b/clients/web/src/components/elements/SchemaFindingsList/SchemaFindingsList.tsx new file mode 100644 index 00000000..73eb67ab --- /dev/null +++ b/clients/web/src/components/elements/SchemaFindingsList/SchemaFindingsList.tsx @@ -0,0 +1,103 @@ +import { Code, Group, Stack, Text } from "@mantine/core"; +import { + describeSchemaPath, + type SchemaFinding, +} from "@inspector/core/json/schemaLint.js"; + +// Section wrapper: heading + one block per finding. +const FindingsSection = Stack.withProps({ + gap: "xs", +}); + +const FindingsTitle = Text.withProps({ + size: "sm", + fw: 600, +}); + +const FindingsNote = Text.withProps({ + size: "xs", + c: "var(--inspector-text-secondary)", +}); + +// One finding: severity badge + path on the first row, then issue and fix. +const FindingBlock = Stack.withProps({ + gap: 2, +}); + +const FindingHeadRow = Group.withProps({ + gap: "xs", + wrap: "nowrap", + align: "center", +}); + +const FindingText = Text.withProps({ + size: "xs", + c: "var(--inspector-text-secondary)", +}); + +/** + * Severity label. + * + * Deliberately coloured text rather than a filled `Badge`: a filled `yellow` + * badge puts white on `yellow-7`, which at this size is 3.92:1 and fails the + * story's a11y check, and neither `autoContrast` (yellow-7 sits below + * Mantine's default luminance threshold, so it stays white) nor + * `variant="light"` (3.34:1) fixes it. The `--inspector-*` severity tokens are + * the pairings this app already uses against its own surfaces, in both colour + * schemes. + */ +const SeverityLabel = Text.withProps({ + size: "xs", + fw: 700, + tt: "uppercase", +}); + +/** + * Severity → text-colour token. `error` is a construct a shipping MCP client + * refuses outright; `warning` is one handled unevenly. + */ +function severityColor(severity: SchemaFinding["severity"]): string { + return severity === "error" + ? "var(--inspector-danger-text)" + : "var(--inspector-warning-text)"; +} + +export interface SchemaFindingsListProps { + /** Findings for one tool, in walk order. Renders nothing when empty. */ + findings: readonly SchemaFinding[]; +} + +/** + * Tool-schema portability findings for one tool (#1005). + * + * The same verdict the CLI's `--strict` report and the TUI's detail pane show + * — all three read `core/json/schemaLint`, so they cannot disagree about + * whether a schema is portable, only about how much room they have to say so. + */ +export function SchemaFindingsList({ findings }: SchemaFindingsListProps) { + if (findings.length === 0) return null; + + return ( + + Schema portability ({findings.length}) + {findings.map((finding, index) => ( + + + + {finding.severity} + + {describeSchemaPath(finding.schema, finding.path)} + + {finding.issue} + Fix: {finding.suggestion} + + ))} + + These constructs are legal JSON Schema but are refused or mishandled by + some MCP clients, so a tool can work here and fail there. + + + ); +} diff --git a/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.test.tsx b/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.test.tsx index cdad33ef..9e54987a 100644 --- a/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.test.tsx +++ b/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.test.tsx @@ -510,6 +510,30 @@ describe("ToolDetailPanel", () => { }); }); + describe("schema portability findings (#1005)", () => { + it("lists a finding with its path when a schema is unportable", () => { + renderWithMantine( + , + ); + expect(screen.getByText("Schema portability (1)")).toBeInTheDocument(); + expect( + screen.getByText("outputSchema.properties.data"), + ).toBeInTheDocument(); + }); + + it("omits the section for a portable tool", () => { + renderWithMantine(); + expect(screen.queryByTestId("schema-findings")).not.toBeInTheDocument(); + }); + }); + // #2020: Execute used to be gated on `isExecuting` alone — this panel never // called `hasMissingRequiredFields`, unlike the App and elicitation panels — // so arguments that could not be sent were executed anyway and simply arrived diff --git a/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx b/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx index 1984cac3..94ecc893 100644 --- a/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx +++ b/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx @@ -11,7 +11,7 @@ import { Switch, Text, } from "@mantine/core"; -import { useId, useState } from "react"; +import { useId, useMemo, useState } from "react"; import { RiArrowDownSLine, RiArrowRightSLine } from "react-icons/ri"; import type { ProgressNotification, @@ -24,7 +24,9 @@ import { toFormSchema, } from "../../../utils/jsonUtils"; import { getMirroredHeaderParams } from "@inspector/core/json/xMcpHeader.js"; +import { lintToolSchemas } from "@inspector/core/json/schemaLint.js"; import { AnnotationBadge } from "../../elements/AnnotationBadge/AnnotationBadge"; +import { SchemaFindingsList } from "../../elements/SchemaFindingsList/SchemaFindingsList"; import { ProgressDisplay } from "../../elements/ProgressDisplay/ProgressDisplay"; import { SchemaForm } from "../SchemaForm/SchemaForm"; @@ -219,6 +221,9 @@ export function ToolDetailPanel({ // SEP-2243: args this tool declares as `x-mcp-header` — their values mirror // into `Mcp-Param-{Name}` headers on a `tools/call` (#1632). const mirroredParams = getMirroredHeaderParams(tool); + // Memoized on the tool: this panel re-renders on every keystroke in the + // argument form, and the walk depends on nothing that changes in between. + const schemaFindings = useMemo(() => lintToolSchemas(tool), [tool]); // Descriptions are shown by default (most are short); the chevron lets the // user hide a long one to keep the form and Execute footer in view. Reset to @@ -338,6 +343,8 @@ export function ToolDetailPanel({ )} + + { ); expect(screen.queryByRole("presentation")).not.toBeInTheDocument(); }); + + it("shows no schema flag for a clean tool", () => { + renderWithMantine( + {}} />, + ); + expect( + screen.queryByLabelText(/Schema portability/), + ).not.toBeInTheDocument(); + }); + + it("flags a tool whose schema carries an error-severity finding", () => { + renderWithMantine( + {}} + />, + ); + expect( + screen.getByLabelText("Schema portability errors"), + ).toBeInTheDocument(); + }); + + it("flags a warning-only tool distinctly", () => { + renderWithMantine( + {}} + />, + ); + expect( + screen.getByLabelText("Schema portability warnings"), + ).toBeInTheDocument(); + }); }); diff --git a/clients/web/src/components/groups/ToolListItem/ToolListItem.tsx b/clients/web/src/components/groups/ToolListItem/ToolListItem.tsx index d109d096..a74b3f09 100644 --- a/clients/web/src/components/groups/ToolListItem/ToolListItem.tsx +++ b/clients/web/src/components/groups/ToolListItem/ToolListItem.tsx @@ -1,5 +1,16 @@ -import { Group, Image, Stack, Text, UnstyledButton } from "@mantine/core"; +import { + Group, + Image, + Stack, + Text, + ThemeIcon, + Tooltip, + UnstyledButton, +} from "@mantine/core"; +import { useMemo } from "react"; +import { RiErrorWarningLine } from "react-icons/ri"; import type { Tool } from "@modelcontextprotocol/client"; +import { lintToolSchemas } from "@inspector/core/json/schemaLint.js"; import { resolveDisplayLabel } from "../../../utils/toolUtils"; export interface ToolListItemProps { @@ -43,9 +54,20 @@ const ListItemButton = UnstyledButton.withProps({ variant: "listItem", }); +// 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. +const SchemaFlagIcon = ThemeIcon.withProps({ + size: "xs", + variant: "transparent", + role: "img", +}); + export function ToolListItem({ tool, selected, onClick }: ToolListItemProps) { const { name, title, icons } = tool; const iconSrc = icons?.[0]?.src; + const findings = useMemo(() => lintToolSchemas(tool), [tool]); + const hasError = findings.some((f) => f.severity === "error"); return ( {resolveDisplayLabel(name, title)} {title && {name}} + {findings.length > 0 && ( + + + + + + )} ); diff --git a/clients/web/src/test/core/schemaLint.test.ts b/clients/web/src/test/core/schemaLint.test.ts new file mode 100644 index 00000000..064e919d --- /dev/null +++ b/clients/web/src/test/core/schemaLint.test.ts @@ -0,0 +1,463 @@ +import { describe, it, expect } from "vitest"; +import type { Tool } from "@modelcontextprotocol/client"; +import { + countFindings, + describeSchemaPath, + formatSchemaLintReport, + lintToolSchemas, + lintTools, + summarizeFindings, + type SchemaFinding, + type SchemaLintRule, +} from "@inspector/core/json/schemaLint.js"; + +/** + * Minimal well-formed tool; each test overrides the schema under test. + * + * Overrides are `Record` rather than `Partial` on + * purpose: the SDK types a tool schema as an *object* schema with a literal + * `type: "object"`, and the whole point of this suite is to feed the lint the + * malformed shapes a real server can send — an array-form `type`, a `properties` + * that is not a map, a root that is a bare boolean. `Partial` rejects + * every one of them, so the fixture accepts the loose shape and narrows once, + * here, instead of scattering a cast over each case. + */ +function tool(overrides: Record = {}): Tool { + return { + name: "t", + inputSchema: { type: "object", properties: {} }, + ...overrides, + } as Tool; +} + +/** The rules raised, in order — the shape most assertions here care about. */ +function rules(findings: readonly SchemaFinding[]): SchemaLintRule[] { + return findings.map((f) => f.rule); +} + +function paths(findings: readonly SchemaFinding[]): string[] { + return findings.map((f) => describeSchemaPath(f.schema, f.path)); +} + +describe("lintToolSchemas — clean schemas", () => { + it.each([ + ["an empty object schema", { type: "object", properties: {} }], + [ + "a typed property", + { type: "object", properties: { a: { type: "string" } } }, + ], + [ + "an enum property with no type", + { type: "object", properties: { a: { enum: ["x", "y"] } } }, + ], + [ + "a const property with no type", + { type: "object", properties: { a: { const: 3 } } }, + ], + [ + "a local $ref", + { + type: "object", + properties: { a: { $ref: "#/$defs/x" } }, + $defs: { x: { type: "string" } }, + }, + ], + [ + "additionalProperties: true", + { type: "object", properties: {}, additionalProperties: true }, + ], + [ + "additionalProperties: false", + { type: "object", properties: {}, additionalProperties: false }, + ], + [ + "unevaluatedProperties / additionalItems / unevaluatedItems booleans", + { + type: "object", + unevaluatedProperties: false, + additionalItems: true, + unevaluatedItems: false, + }, + ], + [ + "a shape-implying schema with no type", + { + type: "object", + properties: { a: { properties: { b: { type: "string" } } } }, + }, + ], + [ + "an anyOf branch set", + { + type: "object", + properties: { + a: { anyOf: [{ type: "string" }, { type: "null" }] }, + }, + }, + ], + [ + "a typed array with typed items", + { + type: "object", + properties: { a: { type: "array", items: { type: "number" } } }, + }, + ], + ])("reports nothing for %s", (_label, inputSchema) => { + expect(lintToolSchemas(tool({ inputSchema }))).toEqual([]); + }); + + it("reports nothing when a tool declares neither schema", () => { + const bare = { name: "t" } as unknown as Tool; + expect(lintToolSchemas(bare)).toEqual([]); + }); +}); + +describe("lintToolSchemas — boolean-schema", () => { + it("flags the Go interface{} case from the issue", () => { + const findings = lintToolSchemas( + tool({ + outputSchema: { + type: "object", + properties: { data: true, topic: { type: "string" } }, + }, + }), + ); + expect(rules(findings)).toEqual(["boolean-schema"]); + expect(paths(findings)).toEqual(["outputSchema.properties.data"]); + expect(findings[0]!.severity).toBe("error"); + expect(findings[0]!.suggestion).toContain("additionalProperties"); + }); + + it("flags a bare false, with a different suggestion", () => { + const findings = lintToolSchemas( + tool({ + inputSchema: { type: "object", properties: { data: false } }, + }), + ); + expect(rules(findings)).toEqual(["boolean-schema"]); + expect(findings[0]!.suggestion).toContain("Remove the property"); + }); + + it.each([ + [ + "items", + { type: "object", properties: { a: { items: true } } }, + "inputSchema.properties.a.items", + ], + [ + "an anyOf branch", + { type: "object", properties: { a: { anyOf: [true] } } }, + "inputSchema.properties.a.anyOf[0]", + ], + [ + "a prefixItems entry", + { type: "object", properties: { a: { prefixItems: [true] } } }, + "inputSchema.properties.a.prefixItems[0]", + ], + [ + "a $defs entry", + { type: "object", $defs: { x: true }, properties: {} }, + "inputSchema.$defs.x", + ], + [ + "a patternProperties entry", + { type: "object", patternProperties: { "^a": true }, properties: {} }, + 'inputSchema.patternProperties["^a"]', + ], + [ + "a tuple-form items entry", + { type: "object", properties: { a: { items: [true] } } }, + "inputSchema.properties.a.items[0]", + ], + [ + "propertyNames", + { type: "object", propertyNames: true, properties: {} }, + "inputSchema.propertyNames", + ], + ["the schema root", true, "inputSchema"], + ])("flags a bare boolean under %s", (_label, inputSchema, path) => { + const findings = lintToolSchemas(tool({ inputSchema })); + expect(rules(findings)).toEqual(["boolean-schema"]); + expect(paths(findings)).toEqual([path]); + }); + + it("quotes a non-identifier property name in the path", () => { + const findings = lintToolSchemas( + tool({ + inputSchema: { type: "object", properties: { "odd key": true } }, + }), + ); + expect(paths(findings)).toEqual(['inputSchema.properties["odd key"]']); + }); +}); + +describe("lintToolSchemas — type-union", () => { + it("flags the nullable array form and suggests the single type", () => { + const findings = lintToolSchemas( + tool({ + inputSchema: { + type: "object", + properties: { show_ids: { type: ["null", "boolean"] } }, + }, + }), + ); + expect(rules(findings)).toEqual(["type-union"]); + expect(findings[0]!.severity).toBe("warning"); + expect(findings[0]!.suggestion).toContain('"type": "boolean"'); + }); + + it("suggests anyOf when more than one non-null member survives", () => { + const findings = lintToolSchemas( + tool({ + inputSchema: { + type: "object", + properties: { a: { type: ["string", "number"] } }, + }, + }), + ); + expect(findings[0]!.suggestion).toContain("anyOf"); + }); + + it("ignores non-string members when picking the suggestion", () => { + const findings = lintToolSchemas( + tool({ + inputSchema: { + type: "object", + properties: { a: { type: ["string", 3] } }, + }, + }), + ); + expect(findings[0]!.suggestion).toContain('"type": "string"'); + }); + + it("does not raise non-object-root for an array-typed root", () => { + // The root rule reads a *string* `type`; an array root is the union rule's + // business, and reporting both would name one defect twice. + const findings = lintToolSchemas( + tool({ inputSchema: { type: ["object", "null"] } }), + ); + expect(rules(findings)).toEqual(["type-union"]); + }); +}); + +describe("lintToolSchemas — untyped-schema", () => { + it.each([ + ["an empty schema", {}], + ["an annotation-only schema", { description: "anything at all" }], + [ + "a title/default-only schema", + { title: "T", default: 1, examples: [1], deprecated: false }, + ], + ])("flags %s", (_label, sub) => { + const findings = lintToolSchemas( + tool({ + inputSchema: { type: "object", properties: { a: sub } }, + }), + ); + expect(rules(findings)).toEqual(["untyped-schema"]); + expect(paths(findings)).toEqual(["inputSchema.properties.a"]); + }); + + it("flags an entirely empty root schema", () => { + const findings = lintToolSchemas(tool({ inputSchema: {} })); + expect(rules(findings)).toEqual(["untyped-schema"]); + expect(paths(findings)).toEqual(["inputSchema"]); + }); +}); + +describe("lintToolSchemas — non-object-root", () => { + it("flags a non-object inputSchema root", () => { + const findings = lintToolSchemas( + tool({ + inputSchema: { type: "array", items: { type: "string" } }, + }), + ); + expect(rules(findings)).toEqual(["non-object-root"]); + expect(findings[0]!.severity).toBe("error"); + expect(findings[0]!.issue).toContain("inputSchema"); + }); + + it("names outputSchema when that is the offending root", () => { + const findings = lintToolSchemas( + tool({ outputSchema: { type: "string" } }), + ); + expect(rules(findings)).toEqual(["non-object-root"]); + expect(findings[0]!.issue).toContain("outputSchema"); + }); + + it("does not flag a nested non-object schema", () => { + const findings = lintToolSchemas( + tool({ + inputSchema: { + type: "object", + properties: { a: { type: "array", items: { type: "string" } } }, + }, + }), + ); + expect(findings).toEqual([]); + }); +}); + +describe("lintToolSchemas — remote-ref", () => { + it.each([ + ["an https ref", "https://example.com/schema.json"], + ["a sibling-file ref", "common.json#/$defs/x"], + ])("flags %s", (_label, ref) => { + const findings = lintToolSchemas( + tool({ + inputSchema: { type: "object", properties: { a: { $ref: ref } } }, + }), + ); + expect(rules(findings)).toEqual(["remote-ref"]); + expect(findings[0]!.issue).toContain(ref); + }); + + it("ignores an empty $ref rather than calling it remote", () => { + const findings = lintToolSchemas( + tool({ + inputSchema: { type: "object", properties: { a: { $ref: "" } } }, + }), + ); + expect(findings).toEqual([]); + }); +}); + +describe("lintToolSchemas — walk robustness", () => { + it("skips a value that is not a schema at all", () => { + const findings = lintToolSchemas( + tool({ + inputSchema: { type: "object", properties: { a: "nonsense" } }, + }), + ); + expect(findings).toEqual([]); + }); + + it("skips keyword values of the wrong shape", () => { + const findings = lintToolSchemas( + tool({ + inputSchema: { + type: "object", + properties: "not-a-map", + anyOf: "not-an-array", + }, + }), + ); + expect(findings).toEqual([]); + }); + + it("stops at the depth cap instead of recursing forever", () => { + // 200 nested `items`, deeper than MAX_DEPTH (64), each level clean; the + // bare `true` at the bottom sits past the cap and is therefore not + // reported. The assertion is that the walk terminates and stays quiet. + let nested: unknown = true; + for (let i = 0; i < 200; i++) nested = { type: "array", items: nested }; + const findings = lintToolSchemas( + tool({ + inputSchema: { type: "object", properties: { a: nested } }, + }), + ); + expect(findings).toEqual([]); + }); + + it("walks both schemas, inputSchema first", () => { + const findings = lintToolSchemas( + tool({ + inputSchema: { type: "object", properties: { a: true } }, + outputSchema: { type: "object", properties: { b: true } }, + }), + ); + expect(paths(findings)).toEqual([ + "inputSchema.properties.a", + "outputSchema.properties.b", + ]); + }); +}); + +describe("lintTools", () => { + it("drops clean tools and keeps the rest in list order", () => { + const results = lintTools([ + tool({ name: "clean" }), + tool({ + name: "dirty", + inputSchema: { type: "object", properties: { a: true } }, + }), + tool({ name: "also-clean" }), + ]); + expect(results.map((r) => r.toolName)).toEqual(["dirty"]); + expect(results[0]!.findings).toHaveLength(1); + }); + + it("returns an empty list when everything is clean", () => { + expect(lintTools([tool(), tool({ name: "b" })])).toEqual([]); + }); +}); + +describe("reporting helpers", () => { + const results = lintTools([ + tool({ + name: "info", + inputSchema: { + type: "object", + properties: { show_ids: { type: ["null", "boolean"] } }, + }, + outputSchema: { type: "object", properties: { data: true } }, + }), + ]); + + it("counts by severity", () => { + expect(countFindings(results)).toEqual({ errors: 1, warnings: 1 }); + }); + + it("counts nothing for an empty result", () => { + expect(countFindings([])).toEqual({ errors: 0, warnings: 0 }); + }); + + it("summarizes with singular nouns at one", () => { + expect(summarizeFindings(results)).toBe("1 error, 1 warning across 1 tool"); + }); + + it("pluralizes above one", () => { + const two = lintTools([ + tool({ + name: "a", + inputSchema: { type: "object", properties: { x: true, y: true } }, + }), + tool({ + name: "b", + inputSchema: { type: "object", properties: { x: true } }, + }), + ]); + expect(summarizeFindings(two)).toBe("3 errors, 0 warnings across 2 tools"); + }); + + it("renders a report with a path, issue and suggestion per finding", () => { + const report = formatSchemaLintReport(results); + expect(report).toContain('Error: tool "info"'); + expect(report).toContain('Warning: tool "info"'); + expect(report).toContain("Path: outputSchema.properties.data"); + expect(report).toContain("Path: inputSchema.properties.show_ids"); + expect(report).toContain("Issue: "); + expect(report).toContain("Suggestion: "); + expect(report.trimEnd().endsWith("1 error, 1 warning across 1 tool.")).toBe( + true, + ); + }); + + it("renders a zero summary for an empty result", () => { + expect(formatSchemaLintReport([])).toBe( + "0 errors, 0 warnings across 0 tools.", + ); + }); +}); + +describe("describeSchemaPath", () => { + it("names the schema alone at the root", () => { + expect(describeSchemaPath("inputSchema", "")).toBe("inputSchema"); + }); + + it("joins a nested path onto the schema name", () => { + expect(describeSchemaPath("outputSchema", "properties.a")).toBe( + "outputSchema.properties.a", + ); + }); +}); diff --git a/core/json/schemaLint.ts b/core/json/schemaLint.ts new file mode 100644 index 00000000..3ae0cf9c --- /dev/null +++ b/core/json/schemaLint.ts @@ -0,0 +1,414 @@ +/** + * Tool-schema portability lint (#1005). + * + * A tool's `inputSchema`/`outputSchema` can be a perfectly legal JSON Schema + * and still be rejected outright by the client the server is actually meant to + * run against. The reported case is Go's `jsonschema` package emitting a bare + * `true` for an `interface{}` field: valid Draft 2020-12, accepted by the MCP + * SDK's own parser, and refused by Claude Code with a bare `"Invalid input"` + * pointing at `["tools", 7, "outputSchema", "properties", "data"]`. The + * Inspector is where a server author looks *first*, so a construct that will + * fail downstream should be named here rather than silently passed through. + * + * This is deliberately **not** a JSON Schema validator, for a measured reason: + * a census of 617 public servers (14,804 tool schemas) reported on the issue + * found **0** that fail the SDK's own `ListToolsResultSchema.safeParse` — so a + * conformance check would report nothing on essentially every real server. + * What bites instead is the narrower subset each consumer accepts, which is + * what the rules below encode. Every rule is a construct that is legal JSON + * Schema and is known to be refused or quietly mishandled by real MCP clients. + * + * Kept pure and dependency-free so all three clients share one verdict: the + * CLI's `--strict` report, the TUI's tool detail pane, and the web Tools tab + * must not disagree about whether a schema is portable. + */ + +import type { Tool } from "@modelcontextprotocol/client"; + +/** Which of a tool's two schemas a finding was raised against. */ +export type SchemaKind = "inputSchema" | "outputSchema"; + +/** + * `error` — the construct is refused by at least one shipping MCP client, so + * the tool is unusable there. `warning` — accepted unevenly, or degraded + * (silently coerced, ignored) rather than rejected. + */ +export type SchemaLintSeverity = "error" | "warning"; + +/** Stable identifier for a lint rule, so a caller can filter or suppress. */ +export type SchemaLintRule = + | "boolean-schema" + | "type-union" + | "untyped-schema" + | "non-object-root" + | "remote-ref"; + +/** One problem found at one location in one schema. */ +export interface SchemaFinding { + rule: SchemaLintRule; + severity: SchemaLintSeverity; + /** Which schema the finding is in. */ + schema: SchemaKind; + /** + * Location within that schema, in the dotted/bracketed form the report + * prints — `properties.data`, `anyOf[1].items`. Empty string means the + * schema root. + */ + path: string; + /** What is wrong, phrased for a server author. */ + issue: string; + /** A concrete replacement, not a restatement of the problem. */ + suggestion: string; +} + +/** Every finding raised against one tool, in walk order. */ +export interface ToolSchemaFindings { + toolName: string; + findings: SchemaFinding[]; +} + +/** + * Keywords whose value is a schema *or* a boolean by design. `true`/`false` + * here is the idiomatic spelling ("allow anything else" / "allow nothing + * else") and every client understands it, so the boolean-schema rule must not + * fire on them — the issue's own suggested fix is `additionalProperties: true`. + */ +const BOOLEAN_VALUED_KEYWORDS: ReadonlySet = new Set([ + "additionalProperties", + "unevaluatedProperties", + "additionalItems", + "unevaluatedItems", +]); + +/** Keywords whose value is a single subschema. */ +const SUBSCHEMA_KEYWORDS = [ + "items", + "contains", + "not", + "propertyNames", + "if", + "then", + "else", + "additionalProperties", + "unevaluatedProperties", + "additionalItems", + "unevaluatedItems", +] as const; + +/** Keywords whose value is an array of subschemas. */ +const SUBSCHEMA_ARRAY_KEYWORDS = [ + "allOf", + "anyOf", + "oneOf", + "prefixItems", +] as const; + +/** Keywords whose value is a name → subschema map. */ +const SUBSCHEMA_MAP_KEYWORDS = [ + "properties", + "patternProperties", + "$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. + * + * 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. + */ +const ANNOTATION_ONLY_KEYWORDS: ReadonlySet = new Set([ + "title", + "description", + "default", + "examples", + "deprecated", + "readOnly", + "writeOnly", + "$comment", + "$id", + "$schema", + "$anchor", +]); + +/** + * Depth cap for the walk. A `$ref`-free JSON document cannot be cyclic, but + * this module is handed whatever a server sent, so bound the recursion rather + * than trusting it. + */ +const MAX_DEPTH = 64; + +type SchemaRecord = Record; + +function isRecord(value: unknown): value is SchemaRecord { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +/** + * Append a key to a path, quoting it when it is not a bare identifier so the + * printed path stays unambiguous for an odd property name. + */ +function childPath(parent: string, key: string): string { + if (/^[A-Za-z_$][A-Za-z0-9_$]*$/.test(key)) { + return parent === "" ? key : `${parent}.${key}`; + } + return `${parent}[${JSON.stringify(key)}]`; +} + +function indexPath(parent: string, index: number): string { + return `${parent}[${index}]`; +} + +/** Human label for a path, so the root reads as something rather than blank. */ +export function describeSchemaPath(schema: SchemaKind, path: string): string { + return path === "" ? schema : `${schema}.${path}`; +} + +interface WalkContext { + schema: SchemaKind; + findings: SchemaFinding[]; +} + +function add( + ctx: WalkContext, + rule: SchemaLintRule, + severity: SchemaLintSeverity, + path: string, + issue: string, + suggestion: string, +): void { + ctx.findings.push({ + rule, + severity, + schema: ctx.schema, + path, + issue, + suggestion, + }); +} + +/** + * Visit one schema position. `keyword` is the parent keyword this node sits + * under (undefined at the root), which is what decides whether a bare boolean + * is idiomatic here or a portability defect. + */ +function walk( + node: unknown, + path: string, + keyword: string | undefined, + depth: number, + ctx: WalkContext, +): void { + if (depth > MAX_DEPTH) return; + + if (typeof node === "boolean") { + if (keyword !== undefined && BOOLEAN_VALUED_KEYWORDS.has(keyword)) return; + add( + ctx, + "boolean-schema", + "error", + path, + `Bare \`${String(node)}\` used where a schema object is expected.`, + node + ? 'Spell the "anything" schema out — `{"type": "object", "additionalProperties": true}` for a free-form object, or the concrete type if you know it.' + : "Remove the property (or the keyword) rather than schema-ing it away with `false`.", + ); + return; + } + + // Anything that is neither a boolean nor an object is not a schema at all. + // That is malformed rather than merely unportable, and the SDK's own parser + // is the right place to reject it, so this lint stays quiet and stops here. + if (!isRecord(node)) return; + + lintNode(node, path, ctx); + + for (const key of SUBSCHEMA_MAP_KEYWORDS) { + const map = node[key]; + if (!isRecord(map)) continue; + for (const [name, sub] of Object.entries(map)) { + walk(sub, childPath(childPath(path, key), name), key, depth + 1, ctx); + } + } + + for (const key of SUBSCHEMA_ARRAY_KEYWORDS) { + const arr = node[key]; + if (!Array.isArray(arr)) continue; + arr.forEach((sub, i) => { + walk(sub, indexPath(childPath(path, key), i), key, depth + 1, ctx); + }); + } + + for (const key of SUBSCHEMA_KEYWORDS) { + if (!(key in node)) continue; + const value = node[key]; + // Draft-04 tuple form: `items` may be an array of schemas. + if (key === "items" && Array.isArray(value)) { + value.forEach((sub, i) => { + walk(sub, indexPath(childPath(path, key), i), key, depth + 1, ctx); + }); + continue; + } + walk(value, childPath(path, key), key, depth + 1, ctx); + } +} + +/** Rules that apply to a single schema object, ignoring its children. */ +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", + ); + 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`.", + ); + } + + const ref = node.$ref; + if (typeof ref === "string" && ref !== "" && !ref.startsWith("#")) { + add( + ctx, + "remote-ref", + "warning", + path, + `\`$ref\` points outside this document (\`${ref}\`). Clients do not fetch remote schemas, so the constraint is dropped or the tool is rejected.`, + "Inline the referenced schema, or move it into `$defs` and reference it as `#/$defs/`.", + ); + } + + const constrains = Object.keys(node).some( + (key) => !ANNOTATION_ONLY_KEYWORDS.has(key), + ); + if (!constrains) { + add( + ctx, + "untyped-schema", + "warning", + path, + "Schema carries no validation keyword at all, so it accepts any value — the object-literal spelling of a bare `true`.", + 'Give it an explicit `type`. For a genuinely free-form value use `{"type": "object", "additionalProperties": true}`.', + ); + } +} + +/** + * 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. + */ +function lintSchema( + schema: unknown, + kind: SchemaKind, + findings: SchemaFinding[], +): void { + const ctx: WalkContext = { schema: kind, findings }; + if (isRecord(schema)) { + const type = schema.type; + if (typeof type === "string" && type !== "object") { + add( + ctx, + "non-object-root", + "error", + "", + `Root \`type\` is "${type}". MCP requires a tool's ${kind} to describe an object.`, + 'Wrap the value in an object schema — `{"type": "object", "properties": {…}}`.', + ); + } + } + walk(schema, "", undefined, 0, ctx); +} + +/** + * Lint both of a tool's schemas. Returns findings in walk order — inputSchema + * first, then outputSchema — so a report reads top-down through the tool. + */ +export function lintToolSchemas(tool: Tool): SchemaFinding[] { + const findings: SchemaFinding[] = []; + if (tool.inputSchema !== undefined) { + lintSchema(tool.inputSchema, "inputSchema", findings); + } + if (tool.outputSchema !== undefined) { + lintSchema(tool.outputSchema, "outputSchema", findings); + } + return findings; +} + +/** Lint a tool list, dropping the tools that came back clean. */ +export function lintTools(tools: readonly Tool[]): ToolSchemaFindings[] { + const results: ToolSchemaFindings[] = []; + for (const tool of tools) { + const findings = lintToolSchemas(tool); + if (findings.length > 0) results.push({ toolName: tool.name, findings }); + } + return results; +} + +/** Count findings by severity across a lint result. */ +export function countFindings(results: readonly ToolSchemaFindings[]): { + errors: number; + warnings: number; +} { + let errors = 0; + let warnings = 0; + for (const { findings } of results) { + for (const f of findings) { + if (f.severity === "error") errors++; + else warnings++; + } + } + return { errors, warnings }; +} + +/** `2 errors` / `1 warning` — one pluralized count, used in every surface. */ +function plural(count: number, noun: string): string { + return `${count} ${noun}${count === 1 ? "" : "s"}`; +} + +/** + * One-line summary of a lint result, shared by the CLI's non-strict hint, the + * TUI's tools header and the web Tools tab so the three cannot disagree. + */ +export function summarizeFindings( + results: readonly ToolSchemaFindings[], +): string { + const { errors, warnings } = countFindings(results); + return `${plural(errors, "error")}, ${plural(warnings, "warning")} across ${plural(results.length, "tool")}`; +} + +/** + * Render a lint result as plain text — the CLI's `--strict` report, and the + * body the TUI's detail pane prints. Shared so the two cannot describe the + * same finding differently. + */ +export function formatSchemaLintReport( + results: readonly ToolSchemaFindings[], +): string { + const lines: string[] = []; + for (const { toolName, findings } of results) { + for (const f of findings) { + lines.push( + `${f.severity === "error" ? "Error" : "Warning"}: tool "${toolName}"`, + ); + lines.push(` Path: ${describeSchemaPath(f.schema, f.path)}`); + lines.push(` Issue: ${f.issue}`); + lines.push(` Suggestion: ${f.suggestion}`); + lines.push(""); + } + } + lines.push(`${summarizeFindings(results)}.`); + return lines.join("\n"); +} diff --git a/test-servers/configs/unportable-schemas-http.json b/test-servers/configs/unportable-schemas-http.json new file mode 100644 index 00000000..52430cef --- /dev/null +++ b/test-servers/configs/unportable-schemas-http.json @@ -0,0 +1,39 @@ +{ + "serverInfo": { + "name": "unportable-schemas-showcase", + "version": "1.0.0" + }, + "tools": [{ "preset": "echo" }, { "preset": "add" }, { "preset": "get_temp" }], + "rawToolSchemas": { + "echo": { + "inputSchema": { + "type": "object", + "properties": { + "message": { "type": "string" }, + "show_ids": { "type": ["null", "boolean"] }, + "opts": {} + } + }, + "outputSchema": { + "type": "object", + "properties": { + "data": true, + "topic": { "type": "string" } + } + } + }, + "add": { + "inputSchema": { + "type": "object", + "properties": { + "a": { "$ref": "https://example.com/schemas/number.json" }, + "b": { "type": "number" } + } + } + } + }, + "transport": { + "type": "streamable-http", + "port": 6603 + } +} diff --git a/test-servers/src/composable-test-server.ts b/test-servers/src/composable-test-server.ts index 45d740a1..9e2fe35e 100644 --- a/test-servers/src/composable-test-server.ts +++ b/test-servers/src/composable-test-server.ts @@ -495,6 +495,24 @@ export interface ServerConfig { * ignored. */ duplicateToolNames?: string[]; + /** + * Replace a registered tool's `inputSchema` / `outputSchema` in `tools/list` + * with a **raw** JSON Schema document (#1005). + * + * The presets build their schemas from Zod, which by construction cannot + * emit the constructs the schema-portability lint exists to find — a bare + * `true` in a `properties` map, an array-form `type`, a remote `$ref`. Those + * come from other generators (Go's `jsonschema` package emits `true` for + * `interface{}`), so reproducing them needs a hand-written document. Only the + * *advertised* schema is replaced; the tool's handler and its real Zod + * validation are untouched, so calling it still behaves as its preset does. + * + * A name that isn't registered is ignored. + */ + rawToolSchemas?: Record< + string, + { inputSchema?: unknown; outputSchema?: unknown } + >; /** * Gate a tool's visibility in `tools/list` on a client-declared extension * (SEP-2133 `capabilities.extensions`). Maps extension id → tool name (the @@ -1186,11 +1204,34 @@ export function createMcpServer(config: ServerConfig): McpServer { })), ]; - // Tools pagination, and/or the duplicate-name override — both need the same - // hand-built list, so the handler is installed when either is configured. + // Swap in hand-written JSON Schema documents for the named tools (#1005). + // Applied before duplication so a duplicated row carries the same schema its + // twin does, which is what a real concatenated tool list looks like. + const rawToolSchemas = config.rawToolSchemas ?? {}; + const withRawSchemas = (tools: Tool[]): Tool[] => + Object.keys(rawToolSchemas).length === 0 + ? tools + : tools.map((tool) => { + const override = rawToolSchemas[tool.name]; + if (!override) return tool; + const patched: Record = { ...tool }; + if (override.inputSchema !== undefined) { + patched.inputSchema = override.inputSchema; + } + if (override.outputSchema !== undefined) { + patched.outputSchema = override.outputSchema; + } + return patched as Tool; + }); + + // Tools pagination, the duplicate-name override, and the raw-schema override + // all need the same hand-built list, so the handler is installed when any of + // them is configured. if ( capabilities.tools && - (maxPageSize.tools !== undefined || duplicateToolNames.size > 0) + (maxPageSize.tools !== undefined || + duplicateToolNames.size > 0 || + Object.keys(rawToolSchemas).length > 0) ) { mcpServer.server.setRequestHandler("tools/list", async (request) => { const cursor = request.params?.cursor; @@ -1224,7 +1265,7 @@ export function createMcpServer(config: ServerConfig): McpServer { } // Duplicate before paginating, so a duplicated pair can straddle a page // boundary exactly as a real server's would. - const allTools = withDuplicates(registeredTools); + const allTools = withDuplicates(withRawSchemas(registeredTools)); const startIndex = cursor ? parseInt(cursor, 10) : 0; const endIndex = startIndex + pageSize; diff --git a/test-servers/src/load-config.ts b/test-servers/src/load-config.ts index ca1f75ef..670f172b 100644 --- a/test-servers/src/load-config.ts +++ b/test-servers/src/load-config.ts @@ -79,6 +79,15 @@ export interface ConfigFile { * no preset can produce. See {@link ServerConfig.duplicateToolNames} (#1957). */ duplicateToolNames?: string[]; + /** + * Replace a registered tool's advertised `inputSchema`/`outputSchema` with a + * raw JSON Schema document — the constructs a Zod-built preset cannot emit. + * See {@link ServerConfig.rawToolSchemas} (#1005). + */ + rawToolSchemas?: Record< + string, + { inputSchema?: unknown; outputSchema?: unknown } + >; /** * Gate a tool's `tools/list` visibility on a client-declared extension. Maps * extension id → tool name; the tool appears only when the connected client diff --git a/test-servers/src/resolve-config.ts b/test-servers/src/resolve-config.ts index c84a0865..693c0bc3 100644 --- a/test-servers/src/resolve-config.ts +++ b/test-servers/src/resolve-config.ts @@ -93,6 +93,7 @@ export function resolveConfig(config: ConfigFile): ServerConfig { appElicitation: config.appElicitation, maxPageSize: config.maxPageSize, duplicateToolNames: config.duplicateToolNames, + rawToolSchemas: config.rawToolSchemas, extensionGatedTools: config.extensionGatedTools, serverType: isHttp ? (transport.type as "sse" | "streamable-http")