From cac97c142c2a6f3bcf7c81bfef72ce7ad91f54e6 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 25 Aug 2026 02:02:26 -0400 Subject: [PATCH] fix: drop the unreachable root rule, correct the always-false suggestion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 2. - Remove `non-object-root` entirely. It could never fire: the SDK types `inputSchema` with `type: literal("object")`, so a tool with any other root fails `ListToolsResultSchema` and `salvageListItems` drops it before any client sees it. Verified against a live server advertising `inputSchema: {type: "array"}` — `listAllTools()` returns []. A rule that cannot fire is worse than none, because the docs then claim a check the tool does not perform; the condition is already reported through the malformed-items surface. Pinned by an integration test so a change in that behavior is a signal rather than a silent gap. - Fix the bare-`false` suggestion. `properties: {a: false}` forbids the property; deleting the entry *permits* it with any value under the default `additionalProperties`, so the advice inverted the schema it replaced. Now recommends `{"not": {}}` and says why deletion differs. - Exempt `not`-parented schemas from `untyped-schema`, so the `{"not": {}}` above does not trip this module's own rule — under negation an always-accepting subschema means always-reject, making "accepts any value" the exact opposite of what it does. Scoped to that position, not inherited by its subtree. - Say plainly, in the `true` and untyped suggestions, that they narrow the schema deliberately rather than implying an equivalent rewrite. `true` has no portable object-form equivalent — `{}` is the very shape this lint flags. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- AGENTS.md | 25 ++++-- clients/cli/README.md | 17 ++-- clients/web/src/test/core/schemaLint.test.ts | 87 ++++++++++++------- .../integration/mcp/raw-tool-schemas.test.ts | 18 ++++ core/json/schemaLint.ts | 67 +++++++------- 5 files changed, 136 insertions(+), 78 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 3177fa34..e6331094 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -162,15 +162,22 @@ v2/main/ │ │ # 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) +│ │ # remote `$ref`, a schema carrying no constraining +│ │ # keyword). 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. Equally deliberately it has NO +│ │ # "inputSchema must be an object" rule, even though +│ │ # MCP requires that: the SDK types inputSchema with +│ │ # `type: literal("object")`, so such a tool fails +│ │ # ListToolsResultSchema and salvageListItems drops it +│ │ # before any client sees it — a rule that cannot fire +│ │ # is worse than none, because the docs then claim a +│ │ # check the tool does not perform. 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/clients/cli/README.md b/clients/cli/README.md index 8e7d8e7b..29891664 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -174,11 +174,18 @@ Error: tool "info" ``` 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. +shipping MCP client refuses outright (a bare `true` or `false` where a schema +object belongs), **warnings** are ones handled unevenly (an array-form `type`, +a remote `$ref`, a schema carrying no constraining 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. + +There is deliberately **no "inputSchema must be an object" check**, even though +MCP requires one. The SDK types `inputSchema` with `type: literal("object")`, +so a tool whose input root is anything else fails `ListToolsResultSchema` and +is dropped from the list before the lint could see it — it is reported through +the malformed-items path instead. A rule that cannot fire would only make this +documentation claim a check the CLI does not perform. The report goes to stderr, so the result on stdout stays parseable. Under `--format json` the findings are folded into the same envelope instead diff --git a/clients/web/src/test/core/schemaLint.test.ts b/clients/web/src/test/core/schemaLint.test.ts index f6763931..5f109e08 100644 --- a/clients/web/src/test/core/schemaLint.test.ts +++ b/clients/web/src/test/core/schemaLint.test.ts @@ -131,14 +131,48 @@ describe("lintToolSchemas — boolean-schema", () => { expect(findings[0]!.suggestion).toContain("additionalProperties"); }); - it("flags a bare false, with a different suggestion", () => { + it("suggests the always-false object form for a bare false", () => { + // `properties: {a: false}` forbids the property. Deleting the entry would + // *permit* it with any value under the default `additionalProperties`, so + // the suggestion has to preserve the constraint rather than drop it. const findings = lintToolSchemas( tool({ inputSchema: { type: "object", properties: { data: false } }, }), ); expect(rules(findings)).toEqual(["boolean-schema"]); - expect(findings[0]!.suggestion).toContain("Remove the property"); + expect(findings[0]!.suggestion).toContain('{"not": {}}'); + expect(findings[0]!.suggestion).toContain("different contract"); + expect(findings[0]!.suggestion).not.toMatch(/^Remove the property/); + }); + + it('does not flag the `{"not": {}}` form it recommends', () => { + // The suggestion above would be self-defeating if this module reported its + // own recommended replacement. + expect( + lintToolSchemas( + tool({ + inputSchema: { type: "object", properties: { data: { not: {} } } }, + }), + ), + ).toEqual([]); + }); + + it("still flags an unconstrained schema elsewhere under a not branch", () => { + // The exemption is scoped to the `not` position itself, not inherited by + // everything beneath it. + const findings = lintToolSchemas( + tool({ + inputSchema: { + type: "object", + properties: { a: { not: { properties: { b: {} } } } }, + }, + }), + ); + expect(rules(findings)).toEqual(["untyped-schema"]); + expect(paths(findings)).toEqual([ + "inputSchema.properties.a.not.properties.b", + ]); }); it.each([ @@ -274,9 +308,7 @@ describe("lintToolSchemas — type-union", () => { expect(findings[0]!.suggestion).toContain("anyOf"); }); - 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. + it("raises only the union rule for an array-typed root", () => { const findings = lintToolSchemas( tool({ inputSchema: { type: ["object", "null"] } }), ); @@ -354,38 +386,27 @@ describe("lintToolSchemas — untyped-schema", () => { }); }); -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("does NOT flag a non-object outputSchema root", () => { - // The SDK's `Tool` schema types `inputSchema` with `type: literal("object")` - // but `outputSchema` as a plain loose object with no `type` constraint, so - // a string-typed output is conforming. Flagging it would report an error - // MCP does not require and would fail `--strict` on a valid server. - expect(lintToolSchemas(tool({ outputSchema: { type: "string" } }))).toEqual( - [], - ); - }); - - it("does not flag a nested non-object schema", () => { - const findings = lintToolSchemas( - tool({ +describe("lintToolSchemas — no root-type rule", () => { + // There is deliberately no "inputSchema must be an object" rule: the SDK + // types `inputSchema` with `type: literal("object")`, so such a tool fails + // `ListToolsResultSchema` and `salvageListItems` drops it before any client + // sees it. `raw-tool-schemas.test.ts` pins that live behaviour; these cases + // pin that the module stays quiet rather than reporting a check nothing can + // reach. + it.each([ + ["a non-object inputSchema root", { inputSchema: { type: "array" } }], + ["a non-object outputSchema root", { outputSchema: { type: "string" } }], + [ + "a nested non-object schema", + { inputSchema: { type: "object", properties: { a: { type: "array", items: { type: "string" } } }, }, - }), - ); - expect(findings).toEqual([]); + }, + ], + ])("reports nothing for %s", (_label, overrides) => { + expect(lintToolSchemas(tool(overrides))).toEqual([]); }); }); 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 76791dae..6e03487b 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 @@ -110,6 +110,24 @@ describe("rawToolSchemas override (#1005)", () => { ]); }); + it("drops a tool whose inputSchema root is not an object, before any client sees it", async () => { + // This is why `schemaLint` has no "inputSchema must be an object" rule. + // The SDK types `inputSchema` with `type: literal("object")`, so such a + // tool fails `ListToolsResultSchema` and `salvageListItems` removes it — + // no client can pass it to the lint, and a rule for it could never fire. + // If this ever starts returning the tool, that rule becomes worth adding + // back and this test is the signal. + const started = await start({ + rawToolSchemas: { + echo: { inputSchema: { type: "array", items: { type: "string" } } }, + }, + }); + const connected = await connect(started.url); + + const { tools } = await connected.listAllTools(); + expect(tools.map((t) => t.name)).toEqual(["get_weather"]); + }); + it("leaves an unnamed tool's real schema alone", async () => { const started = await start({ rawToolSchemas: { echo: { outputSchema: { type: "object" } } }, diff --git a/core/json/schemaLint.ts b/core/json/schemaLint.ts index 50b54484..ea1b9b36 100644 --- a/core/json/schemaLint.ts +++ b/core/json/schemaLint.ts @@ -40,7 +40,6 @@ export type SchemaLintRule = | "boolean-schema" | "type-union" | "untyped-schema" - | "non-object-root" | "remote-ref"; /** One problem found at one location in one schema. */ @@ -275,8 +274,16 @@ function walk( 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`.", + ? // `true` accepts any JSON value, and no portable object form says + // exactly that — `{}` is the same shape this lint flags as untyped. + // So the advice is to narrow, and it says so rather than implying an + // equivalent rewrite. + 'Declare what the value actually is — e.g. `{"type": "object", "additionalProperties": true}` for a free-form object. That narrows the schema deliberately; `true` accepts any JSON value at all.' + : // `false` in a property position means the property is FORBIDDEN. + // Deleting the entry is not the same thing: with `additionalProperties` + // absent (default `true`) the property becomes allowed with any value, + // so the suggestion has to preserve the always-false constraint. + 'Use the object form of the always-false schema — `{"not": {}}`. (Deleting the entry is a different contract: under the default `additionalProperties` the property would then be allowed with any value, not forbidden.)', ); return; } @@ -286,7 +293,7 @@ function walk( // is the right place to reject it, so this lint stays quiet and stops here. if (!isRecord(node)) return; - lintNode(node, path, ctx); + lintNode(node, path, keyword, ctx); for (const key of SUBSCHEMA_MAP_KEYWORDS) { const map = node[key]; @@ -319,7 +326,12 @@ function walk( } /** Rules that apply to a single schema object, ignoring its children. */ -function lintNode(node: SchemaRecord, path: string, ctx: WalkContext): void { +function lintNode( + node: SchemaRecord, + path: string, + keyword: string | undefined, + ctx: WalkContext, +): void { const type = node.type; if (Array.isArray(type)) { @@ -361,14 +373,19 @@ function lintNode(node: SchemaRecord, path: string, ctx: WalkContext): void { const constrains = Object.keys(node).some((key) => CONSTRAINING_KEYWORDS.has(key), ); - if (!constrains) { + // 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 + // "accepts any value" would be the exact opposite of what it does, so the + // rule stops at that position rather than contradicting itself. + if (!constrains && keyword !== "not") { 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}`.', + 'Declare what the value actually is — e.g. `{"type": "object", "additionalProperties": true}` for a free-form object. That narrows the schema deliberately, which is the point: as written it constrains nothing.', ); } } @@ -376,37 +393,25 @@ function lintNode(node: SchemaRecord, path: string, ctx: WalkContext): void { /** * Lint one of a tool's schemas. * - * `inputSchema` gets one extra rule at the root: MCP requires it to describe - * an object, and the SDK enforces that — its `Tool` schema types `inputSchema` - * with `type: literal("object")`, so a non-object root is refused by the - * parser rather than merely degraded. + * There is deliberately **no root-level "inputSchema must be an object" rule**, + * even though MCP does require that. The SDK's `ToolSchema` types `inputSchema` + * with `type: literal("object")`, so a tool whose input root is anything else + * fails `ListToolsResultSchema.safeParse`; `salvageListItems` then drops it + * before `ManagedToolsState` hands the list to anyone. Verified against a live + * server advertising `inputSchema: {type: "array"}` — `listAllTools()` returns + * an empty array, so no client can ever pass such a tool to this module. * - * That rule is **scoped to `inputSchema` deliberately**. The same SDK types - * `outputSchema` as a plain loose object with no `type` constraint at all, so - * a tool whose output is `{"type": "string"}` is conforming; flagging it would - * report an error MCP does not require and would fail `--strict` on a valid - * server. + * A rule that cannot fire is worse than no rule: it makes the documentation + * claim a check the tool does not perform. The condition is already reported + * through the salvage path's malformed-items surface, which is where a dropped + * tool legitimately belongs. */ function lintSchema( schema: unknown, kind: SchemaKind, findings: SchemaFinding[], ): void { - const ctx: WalkContext = { schema: kind, findings }; - if (kind === "inputSchema" && 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 inputSchema to describe an object.`, - 'Wrap the value in an object schema — `{"type": "object", "properties": {…}}`.', - ); - } - } - walk(schema, "", undefined, 0, ctx); + walk(schema, "", undefined, 0, { schema: kind, findings }); } /**