diff --git a/clients/cli/README.md b/clients/cli/README.md index e1dd5928..45b7d0f9 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -164,8 +164,10 @@ 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: +The complete **report** against the `unportable-schemas-http.json` showcase — +one block per finding, then the summary. On a non-zero exit the shared error +handler adds one more stderr line after this, the +[`ErrorEnvelope`](#exit-codes--error-envelopes) (`{"error":{"code":"schema_unportable",…}}`): ```text Error: tool "get_temp" diff --git a/clients/cli/__tests__/error-handler.test.ts b/clients/cli/__tests__/error-handler.test.ts index e6e62703..59552a54 100644 --- a/clients/cli/__tests__/error-handler.test.ts +++ b/clients/cli/__tests__/error-handler.test.ts @@ -19,15 +19,19 @@ describe("handleError", () => { vi.restoreAllMocks(); }); - it("emits a JSON error envelope on stderr and exits with the classified code", () => { - const writeSpy = vi - .spyOn(process.stderr, "write") - .mockImplementation((() => true) as never); + it("emits a JSON error envelope on stderr and exits with the classified code", async () => { + const writeSpy = vi.spyOn(process.stderr, "write").mockImplementation((( + _chunk: unknown, + cb?: () => void, + ) => { + cb?.(); + return true; + }) as never); const exitSpy = vi .spyOn(process, "exit") .mockImplementation((() => undefined) as never); - handleError(new Error("boom")); + await handleError(new Error("boom")); expect(exitSpy).toHaveBeenCalledWith(EXIT_CODES.USAGE); const written = writeSpy.mock.calls[0]![0] as string; @@ -38,18 +42,49 @@ describe("handleError", () => { expect(parsed.error.message).toBe("boom"); }); - it("uses a CliExitCodeError's exitCode and envelope code", () => { - vi.spyOn(process.stderr, "write").mockImplementation((() => true) as never); + it("uses a CliExitCodeError's exitCode and envelope code", async () => { + vi.spyOn(process.stderr, "write").mockImplementation((( + _chunk: unknown, + cb?: () => void, + ) => { + cb?.(); + return true; + }) as never); const exitSpy = vi .spyOn(process, "exit") .mockImplementation((() => undefined) as never); - handleError( + await handleError( new CliExitCodeError(EXIT_CODES.NO_APP, "no app", { code: "no_app" }), ); expect(exitSpy).toHaveBeenCalledWith(EXIT_CODES.NO_APP); }); + + it("exits only after stderr has taken the envelope", async () => { + // The whole point of the await: a pipe defers the write, and + // `process.exit()` discards anything still queued. A spy that called back + // synchronously would pass with or without it, so this one defers. + let flushed = false; + vi.spyOn(process.stderr, "write").mockImplementation((( + _chunk: unknown, + cb?: () => void, + ) => { + setTimeout(() => { + flushed = true; + cb?.(); + }, 0); + return false; + }) as never); + const exitSpy = vi + .spyOn(process, "exit") + .mockImplementation((() => undefined) as never); + + await handleError(new Error("boom")); + + expect(flushed).toBe(true); + expect(exitSpy).toHaveBeenCalledWith(EXIT_CODES.USAGE); + }); }); describe("classifyError", () => { diff --git a/clients/cli/src/error-handler.ts b/clients/cli/src/error-handler.ts index 2d4e0e5c..0deaf527 100644 --- a/clients/cli/src/error-handler.ts +++ b/clients/cli/src/error-handler.ts @@ -1,3 +1,5 @@ +import { awaitableError } from "./utils/awaitable-log.js"; + /** * Exit-code map. Non-zero codes let an automated caller (CI, an agent) branch * on the failure class without regex-scraping stderr: @@ -218,8 +220,19 @@ export function formatErrorOutput( }; } -export function handleError(error: unknown): never { +/** + * The binary's last-resort error sink: write the envelope, then exit. + * + * **Async, and the await is load-bearing.** On a pipe or a file — as opposed + * to a TTY — `process.stderr.write` is asynchronous, and `process.exit()` + * discards whatever is still queued. Size is not the safeguard it looks like: + * the envelope being small enough for the pipe buffer says nothing about + * whether the write has been *performed* by the time the process goes away. + * So this settles on the write callback first, exactly as the `--strict` + * schema report does. + */ +export async function handleError(error: unknown): Promise { const { exitCode, stderr } = formatErrorOutput(error); - process.stderr.write(stderr); + await awaitableError(stderr); process.exit(exitCode); } diff --git a/clients/launcher/src/index.ts b/clients/launcher/src/index.ts index faf5e39f..e42f4e53 100644 --- a/clients/launcher/src/index.ts +++ b/clients/launcher/src/index.ts @@ -79,10 +79,12 @@ async function run(): Promise { // this `undefined`; fall through to the generic sink with the real message // rather than throwing "handleError is not a function" over it. if (typeof handleError !== "function") throw err; - // write-then-exit like the direct bin; the envelope is a few hundred bytes, - // well inside the pipe buffer, so the truncation risk noted above (which is - // about a large stdout payload) doesn't apply to this stderr line. - handleError(err); // writes the envelope + process.exit(code); never returns + // Write-then-exit like the direct bin — and AWAIT it. The envelope being + // a few hundred bytes is not the safeguard it looks like: on a pipe the + // write is asynchronous, so `process.exit()` discards it whether or not + // it would have fitted in the buffer. `handleError` settles on the write + // callback before exiting; skipping the await puts the truncation back. + await handleError(err); // writes the envelope + process.exit(code) } } else { const { runTui } = await import(clientEntry("tui")); diff --git a/clients/web/src/test/core/schemaLint.test.ts b/clients/web/src/test/core/schemaLint.test.ts index 6200a31d..b96ac634 100644 --- a/clients/web/src/test/core/schemaLint.test.ts +++ b/clients/web/src/test/core/schemaLint.test.ts @@ -212,6 +212,20 @@ describe("lintToolSchemas — boolean-schema", () => { { type: "object", propertyNames: true, properties: {} }, "inputSchema.propertyNames", ], + [ + "contentSchema", + { + type: "object", + properties: { + a: { + type: "string", + contentMediaType: "application/json", + contentSchema: true, + }, + }, + }, + "inputSchema.properties.a.contentSchema", + ], [ "a dependentSchemas entry", { type: "object", dependentSchemas: { a: true }, properties: {} }, @@ -402,6 +416,9 @@ describe("lintToolSchemas — untyped-schema", () => { ["contains", { contains: { type: "string" } }], ["propertyNames", { propertyNames: { pattern: "^a" } }], ["dependentRequired", { dependentRequired: { a: ["b"] } }], + ["contentMediaType", { contentMediaType: "application/json" }], + ["contentEncoding", { contentEncoding: "base64" }], + ["contentSchema", { contentSchema: { type: "object" } }], ])("does not flag a schema constrained only by %s", (_label, sub) => { const findings = lintToolSchemas( tool({ inputSchema: { type: "object", properties: { a: sub } } }), diff --git a/core/json/schemaLint.ts b/core/json/schemaLint.ts index a4b98bbd..b054683a 100644 --- a/core/json/schemaLint.ts +++ b/core/json/schemaLint.ts @@ -92,6 +92,10 @@ const SUBSCHEMA_KEYWORDS = [ "unevaluatedProperties", "additionalItems", "unevaluatedItems", + // 2019-09/2020-12: the schema the *decoded* string content must satisfy. + // A real schema position, so a bare `true` or a remote `$ref` under it is a + // real finding. + "contentSchema", ] as const; /** Keywords whose value is an array of subschemas. */ @@ -135,10 +139,11 @@ const SUBSCHEMA_MAP_KEYWORDS = [ * `$schema`, `$anchor`, `$vocabulary`: they hold or name subschemas for * *other* schemas to reference, and constrain the current instance not at all. * - * `format` is included even though 2020-12's default vocabulary treats it as - * an annotation rather than an assertion. A schema declaring one is plainly an - * attempt to constrain, and reporting it as accepting anything would be noise - * rather than a finding an author can act on. + * One category is admitted despite being annotation-only in 2020-12's default + * vocabulary: `format`, and the three content keywords (`contentMediaType`, + * `contentEncoding`, `contentSchema`). A schema declaring any of them is + * plainly an attempt to constrain, and reporting it as accepting anything + * would be noise rather than a finding an author can act on. * * Defining the rule this way, rather than as "has no `type`", is also * deliberate: `{"properties": {…}}` with no `type` does constrain its input, @@ -160,6 +165,9 @@ const CONSTRAINING_KEYWORDS: ReadonlySet = new Set([ "minLength", "pattern", "format", + "contentMediaType", + "contentEncoding", + "contentSchema", "maxItems", "minItems", "uniqueItems",