From 2cb23a8b21dc4f9221ce86a5dadfe2c73cc67141 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 25 Aug 2026 02:37:10 -0400 Subject: [PATCH] fix: map the new exit code, keep add runnable, distinguish severity by shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 4. - Add `SCHEMA_UNPORTABLE` to `codeForExit`. `CliExitCodeError` lets a caller omit the envelope, so without the case an envelope-less throw reported the generic `error` — the same silent mislabelling the rename was meant to prevent, one layer down. Covered via the envelope-less construction that exposed it. - Keep a local `type` (and the preset's `required`) alongside the remote `$ref` in the showcase config. The TUI's `schemaToForm` dispatches on `type`, so a `$ref`-only property fell through to a string field that `add`'s numeric handler then rejected — making the README's "every showcase tool is runnable" claim false there. The `remote-ref` finding is unaffected, since that rule reads `$ref` independently. - Restrict `type-union` to well-formed unions: non-empty, unique, and all recognized JSON Schema type names. Its message asserts the construct is *legal* JSON Schema, which is false for `[]`, `[3]` or `["bananas"]`, and the `anyOf` it suggested for those would be invalid too. Malformed schemas stay the SDK parser's business. - Drop `if`/`then`/`else` from the presence-only allowlist. They only constrain as a pair, so `{"if": {"const": 1}}` accepts every value and was evading `untyped-schema`; the pairing is now checked explicitly. - Distinguish the web sidebar severities by SHAPE, not just colour — a circle for an error, a triangle for a warning — and name the severity in the tooltip. Colour alone left a colour-blind reader unable to tell them apart. - Fix the stale "across 2 tools" count in the CLI README's hint sample. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- clients/cli/README.md | 2 +- clients/cli/__tests__/error-handler.test.ts | 12 ++++ clients/cli/src/error-handler.ts | 2 + clients/tui/__tests__/schemaToForm.test.ts | 26 +++++++ .../groups/ToolListItem/ToolListItem.tsx | 10 ++- clients/web/src/test/core/schemaLint.test.ts | 63 ++++++++++++----- .../integration/mcp/raw-tool-schemas.test.ts | 20 ++++++ core/json/schemaLint.ts | 68 ++++++++++++++++--- .../configs/unportable-schemas-http.json | 11 ++- 9 files changed, 180 insertions(+), 34 deletions(-) diff --git a/clients/cli/README.md b/clients/cli/README.md index 1df35ead..491f5a08 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -201,7 +201,7 @@ 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 +error, 3 warnings across 3 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 diff --git a/clients/cli/__tests__/error-handler.test.ts b/clients/cli/__tests__/error-handler.test.ts index 88b62621..e6e62703 100644 --- a/clients/cli/__tests__/error-handler.test.ts +++ b/clients/cli/__tests__/error-handler.test.ts @@ -64,6 +64,18 @@ describe("classifyError", () => { expect(envelope.url).toBe("https://x.example/mcp"); }); + it("derives schema_unportable from the exit code when no envelope is given", () => { + // `CliExitCodeError` lets a caller omit the envelope, in which case the + // machine code comes from `codeForExit`. A new exit code that is missing + // there degrades to the generic `error`, which is exactly the kind of + // silent mislabelling the code map exists to prevent (#1005). + const { exitCode, envelope } = classifyError( + new CliExitCodeError(EXIT_CODES.SCHEMA_UNPORTABLE, "1 finding"), + ); + expect(exitCode).toBe(EXIT_CODES.SCHEMA_UNPORTABLE); + expect(envelope.code).toBe("schema_unportable"); + }); + it("classifies a WWW-Authenticate message as AUTH_REQUIRED without a status", () => { const { exitCode } = classifyError( new Error("Dynamic client registration failed: WWW-Authenticate Bearer"), diff --git a/clients/cli/src/error-handler.ts b/clients/cli/src/error-handler.ts index fe8d551a..2d4e0e5c 100644 --- a/clients/cli/src/error-handler.ts +++ b/clients/cli/src/error-handler.ts @@ -191,6 +191,8 @@ function codeForExit(exitCode: number): string { return "unreachable"; case EXIT_CODES.TOOL_ERROR: return "tool_error"; + case EXIT_CODES.SCHEMA_UNPORTABLE: + return "schema_unportable"; default: return "error"; } diff --git a/clients/tui/__tests__/schemaToForm.test.ts b/clients/tui/__tests__/schemaToForm.test.ts index f47450e3..0a2d2264 100644 --- a/clients/tui/__tests__/schemaToForm.test.ts +++ b/clients/tui/__tests__/schemaToForm.test.ts @@ -381,5 +381,31 @@ describe("schemaToForm", () => { type: "string", }); }); + + // #1005: the unportable-schemas showcase advertises a remote `$ref` so the + // lint has a `remote-ref` finding to report. A `$ref`-only property falls + // through to a string field here, which the tool's real (numeric) handler + // then rejects — so the fixture keeps a local `type` alongside the ref and + // the tool stays runnable. This pins the half of that contract the TUI + // owns; the wire half is in `raw-tool-schemas.test.ts`. + it("types a property that carries both a remote $ref and a local type", () => { + const form = schemaToForm( + { + properties: { + a: { + type: "number", + $ref: "https://example.com/schemas/number.json", + }, + }, + }, + "refWithLocalType", + ); + // `float` is this form builder's numeric field kind — the point is that + // it is NOT the string fallback a `$ref`-only property would get. + expect(form.sections[0]!.fields[0]).toMatchObject({ + name: "a", + type: "float", + }); + }); }); }); diff --git a/clients/web/src/components/groups/ToolListItem/ToolListItem.tsx b/clients/web/src/components/groups/ToolListItem/ToolListItem.tsx index b1ca1003..16d9b7ae 100644 --- a/clients/web/src/components/groups/ToolListItem/ToolListItem.tsx +++ b/clients/web/src/components/groups/ToolListItem/ToolListItem.tsx @@ -8,7 +8,7 @@ import { UnstyledButton, } from "@mantine/core"; import { useMemo } from "react"; -import { RiErrorWarningLine } from "react-icons/ri"; +import { RiAlertLine, RiErrorWarningLine } from "react-icons/ri"; import type { Tool } from "@modelcontextprotocol/client"; import { lintToolSchemas } from "@inspector/core/json/schemaLint.js"; import { resolveDisplayLabel } from "../../../utils/toolUtils"; @@ -87,7 +87,7 @@ export function ToolListItem({ tool, selected, onClick }: ToolListItemProps) { {findings.length > 0 && ( - + {/* Distinct SHAPES, not just distinct colours: an octagon for an + error and a triangle for a warning, so the severity survives + for a colour-blind reader. The tooltip and the accessible + label name it in words as well. */} + {hasError ? : } )} diff --git a/clients/web/src/test/core/schemaLint.test.ts b/clients/web/src/test/core/schemaLint.test.ts index 5f109e08..5a387fa2 100644 --- a/clients/web/src/test/core/schemaLint.test.ts +++ b/clients/web/src/test/core/schemaLint.test.ts @@ -284,28 +284,32 @@ describe("lintToolSchemas — type-union", () => { ); }); - it("drops non-string members from the suggested branches", () => { - const findings = lintToolSchemas( - tool({ - inputSchema: { - type: "object", - properties: { a: { type: ["string", 3] } }, - }, - }), - ); - expect(findings[0]!.suggestion).toContain( - '{"anyOf": [{"type": "string"}]}', - ); + it.each([ + ["an empty array, which matches nothing", []], + ["a non-string member", ["string", 3]], + ["only non-string members", [3]], + ["an unrecognized type name", ["string", "bananas"]], + ["a duplicated member", ["string", "string"]], + ])("stays quiet on a malformed type array — %s", (_label, type) => { + // The rule's message asserts the construct is *legal* JSON Schema, and the + // `anyOf` it suggests would be invalid for these. Malformed schemas are + // the SDK parser's business, the same way a non-schema node is. + expect( + lintToolSchemas( + tool({ inputSchema: { type: "object", properties: { a: { type } } } }), + ), + ).toEqual([]); }); - it("falls back to a placeholder when no member is a string", () => { + it.each([ + ["integer", ["integer", "null"]], + ["array", ["array", "null"]], + ["object", ["object", "string"]], + ])("still fires for a well-formed union containing %s", (_label, type) => { const findings = lintToolSchemas( - tool({ - inputSchema: { type: "object", properties: { a: { type: [3] } } }, - }), + tool({ inputSchema: { type: "object", properties: { a: { type } } } }), ); expect(rules(findings)).toEqual(["type-union"]); - expect(findings[0]!.suggestion).toContain("anyOf"); }); it("raises only the union rule for an array-typed root", () => { @@ -364,6 +368,31 @@ describe("lintToolSchemas — untyped-schema", () => { expect(paths(findings)).toEqual(["inputSchema.properties.a"]); }); + it.each([ + ["a lone if", { if: { const: 1 } }], + ["a lone then", { then: { type: "string" } }], + ["a lone else", { else: { type: "string" } }], + ])("flags %s, which asserts nothing on its own", (_label, sub) => { + // `if` has no assertion effect without `then`/`else`, and either of those + // is ignored without an `if` — so these accept every value, and counting + // the keyword's mere presence would let them pass as constrained. + const findings = lintToolSchemas( + tool({ inputSchema: { type: "object", properties: { a: sub } } }), + ); + expect(rules(findings)).toEqual(["untyped-schema"]); + }); + + it.each([ + ["if + then", { if: { const: 1 }, then: { type: "string" } }], + ["if + else", { if: { const: 1 }, else: { type: "string" } }], + ])("does not flag %s, which is a real conditional", (_label, sub) => { + expect( + lintToolSchemas( + tool({ inputSchema: { type: "object", properties: { a: sub } } }), + ), + ).toEqual([]); + }); + it.each([ ["pattern", { pattern: "^a" }], ["format", { format: "email" }], 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 f9b0fb4a..b8853ba4 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 @@ -256,6 +256,26 @@ describe("rawToolSchemas override (#1005)", () => { { tool: "add", rules: ["remote-ref"] }, ]); expect(countFindings(results)).toEqual({ errors: 1, warnings: 3 }); + + // Every flagged tool in the fixture must stay runnable — the README and + // the PR say so, and a showcase that errors on its own obvious button is + // worse than no showcase. Each is called with the arguments its ADVERTISED + // schema renders, including the properties the override added, since that + // is what a user clicking through the form would send. + const byName = (n: string) => tools.find((t) => t.name === n)!; + await expect( + connected.callTool(byName("get_temp"), { city: "Oslo", units: "C" }), + ).resolves.toBeDefined(); + await expect( + connected.callTool(byName("echo"), { + message: "hi", + show_ids: null, + opts: {}, + }), + ).resolves.toBeDefined(); + await expect( + connected.callTool(byName("add"), { a: 1, b: 2 }), + ).resolves.toBeDefined(); }); it("keeps a structured-content tool callable under an outputSchema override", async () => { diff --git a/core/json/schemaLint.ts b/core/json/schemaLint.ts index ea1b9b36..5b7147cb 100644 --- a/core/json/schemaLint.ts +++ b/core/json/schemaLint.ts @@ -174,9 +174,10 @@ const CONSTRAINING_KEYWORDS: ReadonlySet = new Set([ "anyOf", "oneOf", "not", - "if", - "then", - "else", + // `if` / `then` / `else` are deliberately absent — they only constrain as a + // PAIR. `if` alone has no assertion effect, and `then` / `else` are ignored + // without it, so `{"if": {"const": 1}}` accepts every value. The pairing is + // checked separately in `constrainsInstance`. "items", "prefixItems", "contains", @@ -195,6 +196,57 @@ const CONSTRAINING_KEYWORDS: ReadonlySet = new Set([ "$recursiveRef", ]); +/** + * The seven type names JSON Schema recognizes. A `type` naming anything else + * is malformed rather than unportable, which this module deliberately does not + * report — see {@link isPortableTypeUnion}. + */ +const JSON_SCHEMA_TYPES: ReadonlySet = new Set([ + "null", + "boolean", + "object", + "array", + "number", + "string", + "integer", +]); + +/** + * Does this schema constrain the instance at all? + * + * Presence of a {@link CONSTRAINING_KEYWORDS} member, plus the one keyword + * group that cannot be judged by presence alone: `if` asserts nothing without + * a `then` or an `else`, and either of those is ignored without an `if`. So + * `{"if": {"const": 1}}` and `{"then": {"type": "string"}}` both accept every + * value, and counting the keyword's mere presence would let them pass as + * constrained. + */ +function constrainsInstance(node: SchemaRecord): boolean { + if (Object.keys(node).some((key) => CONSTRAINING_KEYWORDS.has(key))) { + return true; + } + return "if" in node && ("then" in node || "else" in node); +} + +/** + * Is an array-valued `type` the *legal* union this rule is about? + * + * The rule reports a construct that is valid JSON Schema and unportable, and + * its message says exactly that — so it must not fire on an array that is + * simply malformed (`[]`, which matches nothing; `[3]`; `["bananas"]`; + * `["string", "string"]`). Saying "this is legal JSON Schema, but…" of those + * would be false, and the `anyOf` replacement it suggests would be invalid + * too. Malformed schemas are the SDK parser's business, the same way a + * non-object, non-boolean node is; this module stays quiet on them. + */ +function isPortableTypeUnion(type: readonly unknown[]): type is string[] { + if (type.length === 0) return false; + if (!type.every((t) => typeof t === "string" && JSON_SCHEMA_TYPES.has(t))) { + return false; + } + return new Set(type).size === type.length; +} + /** * 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 @@ -334,8 +386,7 @@ function lintNode( ): void { const type = node.type; - if (Array.isArray(type)) { - const members = type.filter((t): t is string => typeof t === "string"); + if (Array.isArray(type) && isPortableTypeUnion(type)) { // The suggestion is always `anyOf`, never "drop it from `required`". // Accepting an explicit JSON `null` and permitting the property to be // absent are independent: on a required `["null","boolean"]` field, @@ -343,14 +394,13 @@ function lintNode( // omission — a different contract, not the same one spelled portably. // `anyOf` branches each carrying a single `type` are equivalent, and this // lint treats them as portable. - const branches = members.length > 0 ? members : type.map(() => "…"); add( ctx, "type-union", "warning", path, `\`type\` is an array (${JSON.stringify(type)}). The array form is legal JSON Schema, but several MCP clients read \`type\` as a single string and either reject the tool or drop the constraint.`, - `Split it into \`anyOf\` branches, each with a single \`type\` — \`{"anyOf": [${branches + `Split it into \`anyOf\` branches, each with a single \`type\` — \`{"anyOf": [${type .map((t) => `{"type": "${t}"}`) .join( ", ", @@ -370,9 +420,7 @@ function lintNode( ); } - const constrains = Object.keys(node).some((key) => - CONSTRAINING_KEYWORDS.has(key), - ); + const constrains = constrainsInstance(node); // Under `not`, an always-accepting subschema is the idiomatic spelling of // always-*reject* — `{"not": {}}` is the object form of `false`, and the one // this module's own `boolean-schema` suggestion recommends. Reporting it as diff --git a/test-servers/configs/unportable-schemas-http.json b/test-servers/configs/unportable-schemas-http.json index 03696323..dd2b24f3 100644 --- a/test-servers/configs/unportable-schemas-http.json +++ b/test-servers/configs/unportable-schemas-http.json @@ -26,16 +26,21 @@ "message": { "type": "string" }, "show_ids": { "type": ["null", "boolean"] }, "opts": {} - } + }, + "required": ["message"] } }, "add": { "inputSchema": { "type": "object", "properties": { - "a": { "$ref": "https://example.com/schemas/number.json" }, + "a": { + "type": "number", + "$ref": "https://example.com/schemas/number.json" + }, "b": { "type": "number" } - } + }, + "required": ["a", "b"] } } },