diff --git a/clients/cli/README.md b/clients/cli/README.md index 491f5a08..e1dd5928 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -164,12 +164,30 @@ on **stderr**, and exits `6` if any is error-severity: mcp-inspector --cli --method tools/list --strict ``` +Complete output against the `unportable-schemas-http.json` showcase — one +block per finding, then the summary: + ```text Error: tool "get_temp" Path: outputSchema.properties.data Issue: Bare `true` used where a schema object is expected. Suggestion: Declare what the value actually is — e.g. `{"type": "object", "additionalProperties": true}` for a free-form object. That narrows the schema deliberately; `true` accepts any JSON value at all. +Warning: tool "echo" + Path: inputSchema.properties.show_ids + Issue: `type` is an array (["null","boolean"]). 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. + Suggestion: Split it into `anyOf` branches, each with a single `type` — `{"anyOf": [{"type": "null"}, {"type": "boolean"}]}`. (Making the property optional instead is a different contract: absent is not the same as `null`.) + +Warning: tool "echo" + Path: inputSchema.properties.opts + Issue: Schema carries no validation keyword at all, so it accepts any value — the object-literal spelling of a bare `true`. + Suggestion: 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. + +Warning: tool "add" + Path: inputSchema.properties.a + Issue: `$ref` points outside this document (`https://example.com/schemas/number.json`). Clients do not fetch remote schemas, so the constraint is dropped or the tool is rejected. + Suggestion: Inline the referenced schema, or move it into `$defs` and reference it as `#/$defs/`. + 1 error, 3 warnings across 3 tools. ``` diff --git a/clients/cli/__tests__/schema-lint-report.test.ts b/clients/cli/__tests__/schema-lint-report.test.ts index f2c5575a..997e98fe 100644 --- a/clients/cli/__tests__/schema-lint-report.test.ts +++ b/clients/cli/__tests__/schema-lint-report.test.ts @@ -61,8 +61,11 @@ describe("writeSchemaLintReport", () => { beforeEach(() => { stderr = ""; originalWrite = process.stderr.write; - process.stderr.write = ((chunk: unknown): boolean => { + process.stderr.write = ((chunk: unknown, ...rest: unknown[]): boolean => { stderr += typeof chunk === "string" ? chunk : String(chunk); + ( + rest.find((r) => typeof r === "function") as (() => void) | undefined + )?.(); return true; }) as typeof process.stderr.write; }); @@ -71,24 +74,46 @@ describe("writeSchemaLintReport", () => { process.stderr.write = originalWrite; }); - it("writes nothing at all when there are no findings", () => { - writeSchemaLintReport([], true); + it("writes nothing at all when there are no findings", async () => { + await writeSchemaLintReport([], true); expect(stderr).toBe(""); }); - it("writes a one-line hint without --strict", () => { - writeSchemaLintReport(lintListResult(DIRTY_LIST), false); + it("writes a one-line hint without --strict", async () => { + await 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); + it("writes the full report under --strict", async () => { + await 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"); }); + + it("resolves only once stderr has taken the write", async () => { + // Both CLI exit paths call `process.exit()` as soon as the work returns, + // which discards anything still buffered on a piped stderr. The report is + // only safe if this settles on the write callback rather than fire-and- + // forget, so the fake defers its callback to a later tick. + let flushed = false; + process.stderr.write = (( + chunk: unknown, + cb?: (err?: Error | null) => void, + ): boolean => { + stderr += typeof chunk === "string" ? chunk : String(chunk); + setTimeout(() => { + flushed = true; + cb?.(); + }, 0); + return false; + }) as typeof process.stderr.write; + + await writeSchemaLintReport(lintListResult(DIRTY_LIST), true); + expect(flushed).toBe(true); + }); }); describe("emitResult — schema lint wiring (#1005)", () => { @@ -109,8 +134,11 @@ describe("emitResult — schema lint wiring (#1005)", () => { )?.(); return true; }) as typeof process.stdout.write; - process.stderr.write = ((chunk: unknown): boolean => { + process.stderr.write = ((chunk: unknown, ...rest: unknown[]): boolean => { stderr += typeof chunk === "string" ? chunk : String(chunk); + ( + rest.find((r) => typeof r === "function") as (() => void) | undefined + )?.(); return true; }) as typeof process.stderr.write; }); diff --git a/clients/cli/src/handlers/emit-result.ts b/clients/cli/src/handlers/emit-result.ts index d6c3f37e..459a6e7e 100644 --- a/clients/cli/src/handlers/emit-result.ts +++ b/clients/cli/src/handlers/emit-result.ts @@ -46,7 +46,10 @@ export async function emitResult( await awaitableLog(JSON.stringify(result, null, 2) + "\n"); } - if (lint) writeSchemaLintReport(lint, args.strict === true); + // Awaited: the throw below (and the CLI's own exit path) reaches + // `process.exit()` immediately, which discards anything still buffered on a + // piped stderr. + if (lint) await writeSchemaLintReport(lint, args.strict === true); if ((result as { isError?: unknown }).isError === true) { throw new CliExitCodeError( diff --git a/clients/cli/src/handlers/schema-lint-report.ts b/clients/cli/src/handlers/schema-lint-report.ts index 7b685ed1..a38aaa93 100644 --- a/clients/cli/src/handlers/schema-lint-report.ts +++ b/clients/cli/src/handlers/schema-lint-report.ts @@ -5,6 +5,7 @@ import { summarizeFindings, type ToolSchemaFindings, } from "@inspector/core/json/schemaLint.js"; +import { awaitableError } from "../utils/awaitable-log.js"; import type { McpResponse } from "./method-types.js"; /** @@ -47,16 +48,16 @@ export function lintListResult(result: McpResponse): ToolSchemaFindings[] { * lint should still learn it found something, but a multi-page report nobody * requested would be worse than silence. */ -export function writeSchemaLintReport( +export async function writeSchemaLintReport( results: readonly ToolSchemaFindings[], strict: boolean, -): void { +): Promise { if (results.length === 0) return; if (!strict) { - process.stderr.write( + await awaitableError( `Schema portability: ${summarizeFindings(results)}. Re-run with --strict for details.\n`, ); return; } - process.stderr.write(`${formatSchemaLintReport(results)}\n`); + await awaitableError(`${formatSchemaLintReport(results)}\n`); } diff --git a/clients/cli/src/utils/awaitable-log.ts b/clients/cli/src/utils/awaitable-log.ts index 144f0112..4d148b9a 100644 --- a/clients/cli/src/utils/awaitable-log.ts +++ b/clients/cli/src/utils/awaitable-log.ts @@ -5,3 +5,21 @@ export function awaitableLog(logValue: string): Promise { }); }); } + +/** + * The stderr twin of {@link awaitableLog}. + * + * Both CLI exit paths call `process.exit()` as soon as the work returns + * (`src/index.ts`, and `handleError` in `src/error-handler.ts`). When stderr + * is a pipe or a file rather than a TTY, `write` is asynchronous, so anything + * still buffered at that moment is discarded — which for a multi-block + * `--strict` report means a truncated or entirely missing diagnostic on + * exactly the redirected-output runs a CI caller uses. + */ +export function awaitableError(logValue: string): Promise { + return new Promise((resolve) => { + process.stderr.write(logValue, () => { + resolve(); + }); + }); +} diff --git a/clients/web/src/test/core/schemaLint.test.ts b/clients/web/src/test/core/schemaLint.test.ts index b7866ddf..6200a31d 100644 --- a/clients/web/src/test/core/schemaLint.test.ts +++ b/clients/web/src/test/core/schemaLint.test.ts @@ -492,6 +492,32 @@ describe("lintToolSchemas — remote-ref", () => { ).toEqual([]); }); + it.each([ + ["examples", { examples: [{ $id: "payload-field" }] }], + ["default", { default: { $id: "payload-field" } }], + ["const", { const: { $id: "payload-field" } }], + ["enum", { enum: [{ $id: "payload-field" }] }], + ])( + "is not disabled by an $id that is instance data under %s", + (_label, dataCarrier) => { + // `$id` inside `examples`/`default`/`const`/`enum` is a payload key, not + // an embedded schema resource — reading one there would switch this rule + // off for the whole document. + const findings = lintToolSchemas( + tool({ + inputSchema: { + type: "object", + properties: { + a: { type: "string", ...dataCarrier }, + b: { $ref: "https://example.com/s.json" }, + }, + }, + }), + ); + expect(rules(findings)).toEqual(["remote-ref"]); + }, + ); + it("still fires when no $id is declared anywhere", () => { const findings = lintToolSchemas( tool({ diff --git a/core/json/schemaLint.ts b/core/json/schemaLint.ts index 4c38e0c0..a4b98bbd 100644 --- a/core/json/schemaLint.ts +++ b/core/json/schemaLint.ts @@ -306,16 +306,20 @@ interface WalkContext { * schema, so this costs almost nothing in practice. If it ever matters, the * refinement is to resolve each `$ref` against the enclosing base and compare * with the declared ids — not to drop the guard. + * + * Descends through {@link forEachSubschema} — **subschema positions only**. + * A scan over every object value would also reach instance data, where an + * `$id` key declares nothing: `examples: [{"$id": "payload-field"}]` would + * otherwise switch this rule off for the entire document. */ function declaresAnyId(node: unknown, depth = 0): boolean { if (depth > MAX_DEPTH || !isRecord(node)) return false; if (typeof node.$id === "string" && node.$id !== "") return true; - return Object.values(node).some((value) => { - if (Array.isArray(value)) { - return value.some((entry) => declaresAnyId(entry, depth + 1)); - } - return declaresAnyId(value, depth + 1); + let found = false; + forEachSubschema(node, "", (child) => { + if (!found) found = declaresAnyId(child, depth + 1); }); + return found; } function add( @@ -336,6 +340,53 @@ function add( }); } +/** + * Call `visit` once for every position under `node` that JSON Schema treats as + * a **subschema** — and only those. + * + * The single traversal both {@link walk} and {@link declaresAnyId} run on, so + * the two cannot disagree about what counts as a schema. That matters more + * than the deduplication: an independent scan that descended through *every* + * object value would also reach instance data — `examples: [{"$id": "x"}]`, + * a `default`, a `const` — where `$id` is just a payload key and declares no + * embedded resource. Reading one there would suppress `remote-ref` for the + * whole document. + */ +function forEachSubschema( + node: SchemaRecord, + path: string, + visit: (child: unknown, childPath: string, keyword: string) => void, +): void { + for (const key of SUBSCHEMA_MAP_KEYWORDS) { + const map = node[key]; + if (!isRecord(map)) continue; + for (const [name, sub] of Object.entries(map)) { + visit(sub, childPath(childPath(path, key), name), key); + } + } + + for (const key of SUBSCHEMA_ARRAY_KEYWORDS) { + const arr = node[key]; + if (!Array.isArray(arr)) continue; + arr.forEach((sub, i) => { + visit(sub, indexPath(childPath(path, key), i), key); + }); + } + + 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) => { + visit(sub, indexPath(childPath(path, key), i), key); + }); + continue; + } + visit(value, childPath(path, key), key); + } +} + /** * 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 @@ -380,34 +431,9 @@ function walk( lintNode(node, path, keyword, 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); - } + forEachSubschema(node, path, (child, childPathValue, childKeyword) => { + walk(child, childPathValue, childKeyword, depth + 1, ctx); + }); } /** Rules that apply to a single schema object, ignoring its children. */